Skip to content

refactor(postgres): modularize migration planner#260

Merged
wmadden merged 25 commits intomainfrom
refactor/modularize-planner
Apr 2, 2026
Merged

refactor(postgres): modularize migration planner#260
wmadden merged 25 commits intomainfrom
refactor/modularize-planner

Conversation

@jkomyno
Copy link
Copy Markdown
Contributor

@jkomyno jkomyno commented Mar 27, 2026

Summary

Splits the 1401-line planner.ts monolith into focused modules, each under 250 lines:

Module Lines Concern
planner-types.ts 38 Shared types (OperationClass, PostgresPlanTargetDetails, PlanningMode)
planner-sql-checks.ts 113 information_schema / pg_catalog query generators
planner-ddl-builders.ts 249 Pure SQL DDL string builders (CREATE TABLE, ADD COLUMN, FOREIGN KEY, etc.)
planner-identity-values.ts 152 Identity value (monoid neutral element) resolution
planner-schema-lookup.ts 54 Pre-computed Set-based constraint lookups
planner.ts 850 Orchestrator class (down from 1401)

The dependency graph is a clean DAG with no cycles:

planner-types
    ^
    |
planner-sql-checks    planner-schema-lookup    planner-ddl-builders
    ^                         ^                        ^
    |                         |                        |
planner-identity-values   planner-reconciliation (existing)
    ^                         ^
    |                         |
    +---- planner.ts ---------+
  • Primarily structural refactor; also fixes sequence default SQL generation (nextval() quoting) and improves user-defined type quoting in format_type checks
  • No public API changescontrol.ts re-exports are identical
  • planner-reconciliation.ts no longer imports from planner.ts
  • Each extracted module is independently unit-testable

Test plan

  • All 148 existing planner tests pass unchanged (except import paths)
  • 13 new unit tests for planner-sql-checks.ts
  • 40 new unit tests for planner-ddl-builders.ts
  • 208 total tests passing, 0 failing
  • Build passes (pnpm turbo build --filter=@prisma-next/postgres)

Stacked on #241 (fix/ddl-not-null).

Summary by CodeRabbit

  • Documentation

    • Updated migration system documentation with enhanced details on NOT NULL column handling, including codec hook consultation and fallback strategies for common data types.
  • Tests

    • Added comprehensive test coverage for SQL migration utilities and DDL builder functions.

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented Mar 27, 2026

Open in StackBlitz

@prisma-next/runtime-executor

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

@prisma-next/mongo-core

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo-core@260

@prisma-next/mongo-orm

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo-orm@260

@prisma-next/mongo-runtime

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo-runtime@260

@prisma-next/sql-runtime

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

@prisma-next/extension-paradedb

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

@prisma-next/extension-pgvector

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

@prisma-next/postgres

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

@prisma-next/sql-orm-client

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

@prisma-next/target-mongo

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/target-mongo@260

@prisma-next/adapter-mongo

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/adapter-mongo@260

@prisma-next/driver-mongo

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/driver-mongo@260

@prisma-next/contract-authoring

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

@prisma-next/contract-ts

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

@prisma-next/ids

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

@prisma-next/psl-parser

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

@prisma-next/psl-printer

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/psl-printer@260

@prisma-next/cli

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

@prisma-next/emitter

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

@prisma-next/migration-tools

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

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

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

@prisma-next/mongo-emitter

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

@prisma-next/sql-contract

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

@prisma-next/sql-errors

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

@prisma-next/sql-operations

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

@prisma-next/sql-schema-ir

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

@prisma-next/sql-contract-psl

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

@prisma-next/sql-contract-ts

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

@prisma-next/sql-contract-emitter

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

@prisma-next/family-sql

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

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

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

@prisma-next/sql-relational-core

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

@prisma-next/sql-builder

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-builder@260

@prisma-next/target-postgres

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

@prisma-next/adapter-postgres

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

@prisma-next/driver-postgres

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

@prisma-next/core-control-plane

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

@prisma-next/core-execution-plane

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

@prisma-next/config

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

@prisma-next/contract

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

@prisma-next/operations

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

@prisma-next/plan

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

@prisma-next/utils

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

commit: 6d64683

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 27, 2026

Warning

Rate limit exceeded

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

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 16 minutes and 21 seconds.

⌛ 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: a6b2b382-1e02-4016-9036-22ef0bc9f218

📥 Commits

Reviewing files that changed from the base of the PR and between 707ec60 and 6d64683.

📒 Files selected for processing (1)
  • packages/3-targets/3-targets/postgres/test/migrations/planner-sql-checks.test.ts
📝 Walkthrough

Walkthrough

This pull request refactors the PostgreSQL migration planner by decomposing a monolithic planner.ts module into specialized, focused modules: planner-ddl-builders.ts, planner-sql-checks.ts, planner-schema-lookup.ts, and planner-target-details.ts. Supporting files are updated to import from new locations. Documentation for the migration system's NOT NULL column reconciliation strategy is also refined. Comprehensive test coverage is added for the new modules.

Changes

Cohort / File(s) Summary
Documentation
docs/architecture docs/subsystems/7. Migration System.md
Updated "NOT NULL columns without defaults" reconciliation strategy to clarify codec hook consultation and explicit fallback literals for common types, with reusable 2-step SQL sequences vs. empty-table precheck.
Core Module Extraction & Organization
packages/3-targets/3-targets/postgres/src/core/migrations/planner-ddl-builders.ts
Added qualifyTableName import from new module; updated buildColumnDefaultSql for autoincrement handling and sequence name escaping; renamed buildAddColumnSql parameter from defaultLiteral to temporaryDefault; exported REFERENTIAL_ACTION_SQL constant. Removed local helper functions now in planner-sql-checks.ts.
New SQL Checks Module
packages/3-targets/3-targets/postgres/src/core/migrations/planner-sql-checks.ts
New module exporting SQL validation builders: identifier/literal escaping, constraint/column/type/default existence checks, table emptiness checks, primary key checks, and type display formatting with codec hook expansion and PostgreSQL type mapping.
New Schema Lookup Module
packages/3-targets/3-targets/postgres/src/core/migrations/planner-schema-lookup.ts
New module introducing SchemaTableLookup interface and builder functions for precomputed constraint lookup sets; exports predicate helpers for unique/index/foreign-key membership checks with deterministic string encodings.
Type Extraction & Consolidation
packages/3-targets/3-targets/postgres/src/core/migrations/planner-target-details.ts
New module exporting PostgresPlanTargetDetails, PlanningMode, and OperationClass types (relocated from planner.ts); updated buildTargetDetails function to reference locally-defined types.
Core Planner Module Refactoring
packages/3-targets/3-targets/postgres/src/core/migrations/planner.ts
Removed local type definitions and helper function implementations; centralized imports from planner-ddl-builders, planner-sql-checks, planner-schema-lookup, and planner-target-details; updated all internal references to use imported types.
Import Path Updates in Recipes & Reconciliation
packages/3-targets/3-targets/postgres/src/core/migrations/planner-recipes.ts, packages/3-targets/3-targets/postgres/src/core/migrations/planner-reconciliation.ts
Updated imports to source PostgresPlanTargetDetails from planner-target-details, buildAddColumnSql from planner-ddl-builders, and SQL checks from planner-sql-checks.
Runner & Export Layer Updates
packages/3-targets/3-targets/postgres/src/core/migrations/runner.ts, packages/3-targets/3-targets/postgres/src/exports/control.ts
Updated type imports for PostgresPlanTargetDetails to reference planner-target-details; updated renderDefaultLiteral import source to planner-ddl-builders.
Test Import Path Updates
packages/3-targets/3-targets/postgres/test/migrations/runner-fixtures.ts, packages/3-targets/3-targets/postgres/test/migrations/runner.\\*\\.integration.test.ts (5 files), packages/3-targets/3-targets/postgres/test/migrations/planner.reconciliation-unit.test.ts
Updated type-only imports for PostgresPlanTargetDetails, PlanningMode, and constraintExistsCheck to import from new module locations.
New Test Coverage
packages/3-targets/3-targets/postgres/test/migrations/planner-ddl-builders.test.ts
New test suite validating buildColumnTypeSql, buildColumnDefaultSql, renderDefaultLiteral, buildAddColumnSql, buildCreateTableSql, and buildForeignKeySql with comprehensive assertions for type mapping, defaults, sequences, and FK clauses.
New Test Coverage
packages/3-targets/3-targets/postgres/test/migrations/planner-sql-checks.test.ts
New test suite validating SQL check builders: identifier/literal quoting, constraint/column/table existence and nullability checks, primary key checks, and type display formatting with codec hook expansion.
Existing Test Updates
packages/3-targets/3-targets/postgres/test/migrations/planner.case1.test.ts
Updated sequence default assertion to expect quoted identifier via nextval('"counter_id_seq"'::regclass) instead of unquoted form.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Poem

🐰 From one file we did split apart,
Four modules now stand on their own!
DDL, checks, and schemas all,
Make refactoring feel less known.
With tests as guards and types in place,
The planner hops with newfound grace!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 9.38% 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 'refactor(postgres): modularize migration planner' accurately describes the main structural change - splitting the monolithic planner.ts into smaller, focused modules.

✏️ 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 refactor/modularize-planner

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

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

Base automatically changed from fix/ddl-not-null to main March 30, 2026 07:55
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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-ddl-builders.ts`:
- Around line 155-157: The sequence default currently emits DEFAULT
nextval(quoteIdentifier(... )::regclass) which produces an identifier not a
string; change the builder in the sequence branch (the code that returns DEFAULT
nextval(...)) to wrap the quoted identifier in a single-quoted string literal so
regclass receives a text input (i.e. produce nextval('"<seq_name>"'::regclass)),
and update the corresponding test expectation (the string asserted at the test
around line 114/115) to match 'DEFAULT nextval(\'"user_id_seq"\'::regclass)' so
the test reflects the single-quoted string-wrapped, quoted identifier form.

In
`@packages/3-targets/3-targets/postgres/src/core/migrations/planner-sql-checks.ts`:
- Around line 128-131: In buildExpectedFormatType(), replace the ad-hoc quoting
logic that checks column.nativeType case with a call to
quoteIdentifier(column.nativeType) so the type-name quoting matches the DDL
emitter (buildColumnTypeSql) and handles hyphens, reserved words and embedded
quotes; locate buildExpectedFormatType and swap the needsQuoting branch to use
quoteIdentifier(column.nativeType) whenever column.typeRef is present so the
expected format_type() output aligns with the DDL path.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 82195d65-cfea-485e-8191-bbc21644d135

📥 Commits

Reviewing files that changed from the base of the PR and between b0409e1 and 6223147.

📒 Files selected for processing (21)
  • docs/architecture docs/subsystems/7. Migration System.md
  • packages/3-targets/3-targets/postgres/src/core/migrations/planner-ddl-builders.ts
  • packages/3-targets/3-targets/postgres/src/core/migrations/planner-identity-values.ts
  • packages/3-targets/3-targets/postgres/src/core/migrations/planner-recipes.ts
  • packages/3-targets/3-targets/postgres/src/core/migrations/planner-reconciliation.ts
  • packages/3-targets/3-targets/postgres/src/core/migrations/planner-schema-lookup.ts
  • packages/3-targets/3-targets/postgres/src/core/migrations/planner-sql-checks.ts
  • packages/3-targets/3-targets/postgres/src/core/migrations/planner-target-details.ts
  • packages/3-targets/3-targets/postgres/src/core/migrations/planner-types.ts
  • packages/3-targets/3-targets/postgres/src/core/migrations/planner.ts
  • packages/3-targets/3-targets/postgres/src/core/migrations/runner.ts
  • packages/3-targets/3-targets/postgres/src/exports/control.ts
  • packages/3-targets/3-targets/postgres/test/migrations/fixtures/runner-fixtures.ts
  • packages/3-targets/3-targets/postgres/test/migrations/planner-ddl-builders.test.ts
  • packages/3-targets/3-targets/postgres/test/migrations/planner-sql-checks.test.ts
  • packages/3-targets/3-targets/postgres/test/migrations/planner.reconciliation-unit.test.ts
  • packages/3-targets/3-targets/postgres/test/migrations/runner.basic.integration.test.ts
  • packages/3-targets/3-targets/postgres/test/migrations/runner.errors.integration.test.ts
  • packages/3-targets/3-targets/postgres/test/migrations/runner.execution-checks.integration.test.ts
  • packages/3-targets/3-targets/postgres/test/migrations/runner.idempotency.integration.test.ts
  • packages/3-targets/3-targets/postgres/test/migrations/runner.policy.integration.test.ts
💤 Files with no reviewable changes (1)
  • packages/3-targets/3-targets/postgres/src/core/migrations/planner-target-details.ts

Copy link
Copy Markdown
Contributor

@wmadden wmadden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Review range: origin/main...HEAD (22 files, +997/−417)

Summary

A well-executed structural refactor that splits the ~1400-line planner.ts monolith into focused modules. The extraction is clean, the DAG property holds, and existing tests pass with only import-path updates. The PR also embeds several behavioral improvements (sequence default fix, user-defined type quoting, optional table scoping) that are correct but should be acknowledged as non-refactoring changes.

Note for the author: Please address the comments below or note your reason not to. No need to seek re-review — this is approved.


What Looks Solid

  • Clean module boundaries: Each extracted module has a single, clear responsibility. The naming convention (planner-*) is consistent and discoverable.
  • DAG property: The upward dependency from planner-reconciliation → planner is properly broken by moving shared types to planner-types.
  • Comprehensive new tests: 40 tests for DDL builders and 13+ for SQL checks. The test organization mirrors the source module structure.
  • Correct sequence default fix: The nextval() SQL generation was genuinely broken before (missing string quotes). The fix in buildColumnDefaultSql is correct and the assertion in planner.case1.test.ts was updated accordingly.
  • Improved user-defined type quoting: The new formatUserDefinedTypeName properly handles reserved words, special characters, and mixed-case identifiers — a real correctness improvement over the simple toLowerCase() check.

Blocking Issues

None identified. The changes are safe to merge.


Non-Blocking Concerns

F01 — constraintExistsCheck signature change is a silent behavioral change
planner-sql-checks.ts lines 14–36

The table parameter changed from required to optional (table?: string). When omitted, the generated SQL matches constraints by name + schema across all tables, not scoped to a specific table. The old signature enforced table scoping at the type level. This widening could allow future callers to accidentally produce unscoped constraint checks, which could match the wrong constraint in schemas with same-named constraints on different tables.

Suggestion: If the optional-table behavior is intentional (e.g., for primary key checks where table scoping is implicit via indrelid), document the trade-off with a brief comment. Otherwise, consider keeping table required and creating a separate constraintExistsCheckUnscoped function to make the choice explicit.


F02 — planner-schema-lookup.ts has no dedicated unit tests
planner-schema-lookup.ts lines 1–54

The buildSchemaLookupMap, hasUniqueConstraint, hasIndex, and hasForeignKey functions are tested only indirectly through the planner integration tests. These are pure functions with clear inputs/outputs — ideal candidates for focused unit tests.

Suggestion: Add a planner-schema-lookup.test.ts to match the pattern established for the other extracted modules.


F03 — PR body has unfilled placeholder

The PR body contains Stacked on #<!-- insert ddl-not-null PR number --> — the PR number was never filled in, making it harder to navigate the PR stack.

Suggestion: Fill in the actual PR number or link before merge.


F04 — Behavioral changes should be called out in the PR body

The PR body states "No behavior changes — pure structural refactor," but the diff contains at least three behavioral changes:

  1. buildColumnDefaultSql sequence SQL fix
  2. buildExpectedFormatType user-defined type quoting improvement
  3. constraintExistsCheck optional table parameter

While all three are improvements, mislabeling behavioral changes as "pure refactor" can mislead reviewers who rely on the PR description to scope their review.

Suggestion: Update the PR body to acknowledge these behavioral improvements, e.g.: "Primarily structural refactor; also fixes sequence default SQL generation and improves user-defined type quoting in format_type checks."


F05 — REFERENTIAL_ACTION_SQL exported without apparent need
planner-ddl-builders.ts line 212

REFERENTIAL_ACTION_SQL was changed from a private const to export const. No call site outside planner-ddl-builders.ts uses it. Exporting it widens the module's public surface unnecessarily.

Suggestion: Revert to non-exported const unless there's a planned consumer.


F06 — Missing blank line between functions in planner.ts
planner.ts lines 776–777

The blank line between canUseSharedTemporaryDefaultStrategy and sortDependencies was removed, reducing visual separation between two unrelated functions.


F07 — Comments added to buildColumnDefaultSql are low-value
planner-ddl-builders.ts lines 130–135, 148, 156

Several comments narrate what the code does rather than why. Per project conventions ("Don't add comments if avoidable, prefer code which expresses its intent"), these add noise. The JSDoc could be kept if trimmed to just the autoincrement note (since that behavior is non-obvious).


Nits

F08 — The module-level doc comment in planner-identity-values.ts is good, but removing the per-function JSDoc on resolveIdentityValue lost the useful "codec hooks first, built-in fallback second" scanning hint.

F09 — Test helper col() in planner-ddl-builders.test.ts defaults codecId to 'pg/text@1' for all columns, so tests for int4, bool, etc. carry a misleading codec ID. Doesn't affect outcomes but could confuse future readers.


Acceptance-Criteria Traceability

# Criterion Status
1 planner.ts ≤ 850 lines ✅ 792 lines
2 Each extracted module ≤ 250 lines ⚠️ planner-sql-checks.ts is 313 lines (~90 are a static reserved-word set)
3 DAG with no circular imports ✅ Verified
4 All existing tests pass ✅ Per PR body + diff review
5 New unit tests for sql-checks and ddl-builders ✅ 230 + 300 lines
6 control.ts re-exports identical ✅ Diff verified
7 Build passes ✅ Per PR body (not independently verified)

jkomyno and others added 20 commits April 2, 2026 17:06
Both commands previously returned raw `VerifyDatabaseSchemaResult` on
schema verification failure instead of the standard `CliErrorEnvelope`.
This made agents parsing `--json` output handle two different shapes.

Now both commands map schema verification failures through a new
`errorSchemaVerificationFailed` factory (PN-RTM-3004), which wraps
the full verification tree in `meta.verificationResult`. Human TTY
output still renders the tree before the error summary.
Add 13 journey-based e2e test files covering 30 CLI scenarios (A-Z + P2-P5)
across greenfield setup, schema evolution, brownfield adoption, drift detection,
migration edge cases, and error scenarios. Each journey is a single it() block
with descriptive assertion labels for step-level failure identification.

Infrastructure:
- journey-test-helpers.ts: CommandResult-based runners for all CLI commands,
  contract swap helper, JSON parsing, SQL helper
- 5 contract fixtures (base, additive, destructive, add-table, v3)
- Config templates with {{DB_URL}} placeholder replacement

Known limitations (marked as TODO in tests):
- db schema-verify and db update hit Vite SSR module resolution error
  after DDL changes (PN-CLI-4999) — affects drift-schema journeys M/N
- Migration chain breakage recovery (P3) requires manual intervention
- Target mismatch (U) needs better simulation approach
Q (apply noop), R (plan noop), and X (show variants) are now tail
steps of Journey B. They reuse B's already-applied migration chain
instead of spinning up 3 separate PGlite instances for 4 assertions.
Journey I's JSON assertions are already covered per-command in A.09,
A.10, B.10, and isolated command tests. Journey J (help output)
has near-zero regression prevention value. Journey Y (global flags)
is retained as a lightweight no-DB test.
- 30 → 22 journeys, 13 → 10 test files
- Removed: H (redundant with N), I (JSON covered inline), J (help),
  P4 and P5 (covered by command-specific tests)
- Merged: Q, R, X as tail steps of Journey B
- Updated cross-reference matrix, file structure, acceptance criteria,
  implementation phases, and parallelism estimates
Reverts the errorSchemaVerificationFailed wrapping introduced in 0f57e7f.
The db sign e2e test expects the raw VerifyDatabaseSchemaResult shape
(with `schema` at top level) in JSON failure output, not a CliErrorEnvelope.
…ables

When adding a NOT NULL column without an explicit DEFAULT to a table that
may contain rows, the planner now emits a 2-step DDL:

  1. ALTER TABLE … ADD COLUMN … DEFAULT <zero> NOT NULL
  2. ALTER TABLE … ALTER COLUMN … DROP DEFAULT

This uses a type-appropriate zero value (e.g. '' for text, 0 for int4,
false for bool) as a temporary default so PostgreSQL can backfill existing
rows, then immediately drops it so future inserts must provide a value.

The runner is also updated to always apply planned operations when the
plan origin is null (db update mode), even if the marker hash already
matches the destination. This fixes schema drift recovery where manual
DDL changed the database without changing the contract.

Closes TML-2067
…tegy

Add db update section to Migration System subsystem docs explaining:
- Live introspection vs serialized migration replay
- Runner origin-based skip semantics (origin: null always applies)
- Temporary zero-default strategy for NOT NULL columns
- Assert error code PN-RTM-3004 in brownfield sign failure, not just ok: false
- Add db verify after destructive update to confirm schema change was applied
- Check stdout is ANSI-free with stripAnsi instead of just checking output length
- brownfield: actual sign error code is PN-SCHEMA-0001, not PN-RTM-3004
- help-and-flags: revert stripAnsi check — TerminalUI decoration leaks
  ANSI into stdout even with --no-color, so the original assertion was
  correct
Move shared types (OperationClass, PostgresPlanTargetDetails, PlanningMode)
and buildTargetDetails to a dedicated planner-types module. This breaks
the implicit coupling where planner-reconciliation.ts imported types
from the planner orchestrator.
Move SQL check generators (constraintExistsCheck, columnExistsCheck,
columnNullabilityCheck, tableIsEmptyCheck, columnHasNoDefaultCheck,
tableHasPrimaryKeyCheck, qualifyTableName, toRegclassLiteral) to a
dedicated module. These are pure functions producing SQL strings for
information_schema/pg_catalog queries.
Move pure SQL DDL string builders (buildCreateTableSql, buildColumnTypeSql,
buildColumnDefaultSql, renderDefaultLiteral, buildAddColumnSql,
buildForeignKeySql, REFERENTIAL_ACTION_SQL, safety assertions) to a
dedicated module. These are now independently unit-testable.
Move pre-computed Set-based constraint lookup structures
(SchemaTableLookup, buildSchemaLookupMap, hasUniqueConstraint,
hasIndex, hasForeignKey) to a dedicated module.
Direct unit tests for the extracted modules, exercising each function
independently without instantiating the planner. Covers SQL check
generators, DDL builders, column type rendering, default literals,
safety assertions, and FK referential action mapping.
- Remove unnecessary export from REFERENTIAL_ACTION_SQL
- Add blank line between unrelated functions in planner.ts
- Trim redundant comments in buildColumnDefaultSql
- Document optional table param semantics on constraintExistsCheck
- Add unit tests for planner-schema-lookup (16 tests)
@wmadden wmadden force-pushed the refactor/modularize-planner branch from 9875e02 to a2dcb13 Compare April 2, 2026 15:08
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/architecture` docs/subsystems/7. Migration System.md:
- Around line 307-315: There are unresolved Git merge conflict markers in the
"NOT NULL columns without defaults." section; remove the conflict markers and
keep the intended final content that documents codec-hook-driven temporary
default resolution and the reusable 2-step add/drop-default recipe (the
paragraph that mentions consulting codec hooks, built-in fallbacks like '' for
text, 0 for integers, false for booleans, '{}' for arrays, ''::tsvector,
length-aware bit(n) literals, emitting add column with temporary default then
dropping it, and falling back to empty-table precheck when no safe default
exists). Ensure the final text replaces the entire conflict block and contains
no leftover <<<<<<<, =======, or >>>>>>> markers.

In `@packages/1-framework/3-tooling/cli/src/commands/db-schema-verify.ts`:
- Around line 46-69: The preflight setup (calls like loadConfig,
resolveContractPath, maskConnectionUrl, formatStyledHeader and ui.stderr) can
throw before the Result/structured-error flow reaches handleResult; wrap that
entire setup and the client/progress-adapter build logic (the code around lines
where the client and adapter are created) inside the repo’s performAction()
pattern (or at minimum catch exceptions and convert them into a
CliStructuredError/Err result) so any error from loadConfig or the adapter
creation is returned as a Result<CliStructuredError> to handleResult instead of
letting Commander surface raw rejections; locate and move the code that performs
loadConfig, computes configPath/contractPath, pushes details, and calls
ui.stderr into the performAction callback (or wrap it with try/catch that
returns Err(new CliStructuredError(...))) so standard CLI envelope/exit-code
handling is preserved.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 2c777139-01af-4ae9-9389-6b908e28c0f9

📥 Commits

Reviewing files that changed from the base of the PR and between 9875e02 and a2dcb13.

📒 Files selected for processing (2)
  • docs/architecture docs/subsystems/7. Migration System.md
  • packages/1-framework/3-tooling/cli/src/commands/db-schema-verify.ts

wmadden added 3 commits April 2, 2026 17:37
The rebase onto origin/main introduced duplicate files (planner-sql.ts,
planner-target-details.ts) that conflicted with our extracted modules
(planner-ddl-builders.ts, planner-sql-checks.ts, planner-types.ts).
Restore correct imports and remove duplicates.
- Remove merge conflict markers in Migration System docs (kept the
  codec-hooks-driven paragraph as the most complete version)
- Delete orphaned db-schema-verify.ts (removed in main by PR #243,
  incorrectly preserved during rebase)
Cover all three code paths in buildExpectedFormatType:
- FORMAT_TYPE_DISPLAY mappings (int2→smallint, float8→double precision, etc.)
- User-defined type quoting via formatUserDefinedTypeName (reserved words,
  mixed-case, hyphens, spaces, digit-leading names)
- Codec hook expansion (typeParams + expandNativeType delegation)
wmadden added 2 commits April 2, 2026 18:11
The rebase incorrectly replaced Alberto's descriptive
planner-target-details.ts (from PR #241) with our generic
planner-types.ts. The original name is better: it describes what the
module contains (target detail types and builder), whereas
"planner-types" is vague and misleading given it also exports a
function.
The expandNativeType mock in planner-sql-checks.test.ts declared
typeParams as required, but ExpandNativeTypeInput has it optional.
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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-sql-checks.ts`:
- Around line 105-116: The code currently normalizes expected types with
expandNativeType() which yields DDL spellings that differ from PostgreSQL's
format_type() output; update the postcheck path so instead of expandNativeType()
you route through a format-type-specific expansion that produces
PostgreSQL-canonical names (use/extend the FORMAT_TYPE_DISPLAY mapping and add a
new expansion for parameterized families like varchar/bpchar/char to output
"character varying(<n>)" and "character(<n>)" as format_type() would); invoke
that new expansion before calling columnTypeCheck() so comparisons use
format_type()-style spellings; reference FORMAT_TYPE_DISPLAY,
expandNativeType(), columnTypeCheck(), and format_type() when locating where to
change.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 1d5ef538-1812-4ef1-bf2a-fd6fddc6b850

📥 Commits

Reviewing files that changed from the base of the PR and between a2dcb13 and 707ec60.

📒 Files selected for processing (17)
  • docs/architecture docs/subsystems/7. Migration System.md
  • packages/3-targets/3-targets/postgres/src/core/migrations/planner-recipes.ts
  • packages/3-targets/3-targets/postgres/src/core/migrations/planner-reconciliation.ts
  • packages/3-targets/3-targets/postgres/src/core/migrations/planner-sql-checks.ts
  • packages/3-targets/3-targets/postgres/src/core/migrations/planner-target-details.ts
  • packages/3-targets/3-targets/postgres/src/core/migrations/planner.ts
  • packages/3-targets/3-targets/postgres/src/core/migrations/runner.ts
  • packages/3-targets/3-targets/postgres/src/exports/control.ts
  • packages/3-targets/3-targets/postgres/test/migrations/fixtures/runner-fixtures.ts
  • packages/3-targets/3-targets/postgres/test/migrations/planner-ddl-builders.test.ts
  • packages/3-targets/3-targets/postgres/test/migrations/planner-sql-checks.test.ts
  • packages/3-targets/3-targets/postgres/test/migrations/planner.reconciliation-unit.test.ts
  • packages/3-targets/3-targets/postgres/test/migrations/runner.basic.integration.test.ts
  • packages/3-targets/3-targets/postgres/test/migrations/runner.errors.integration.test.ts
  • packages/3-targets/3-targets/postgres/test/migrations/runner.execution-checks.integration.test.ts
  • packages/3-targets/3-targets/postgres/test/migrations/runner.idempotency.integration.test.ts
  • packages/3-targets/3-targets/postgres/test/migrations/runner.policy.integration.test.ts
✅ Files skipped from review due to trivial changes (9)
  • packages/3-targets/3-targets/postgres/src/core/migrations/runner.ts
  • packages/3-targets/3-targets/postgres/test/migrations/runner.execution-checks.integration.test.ts
  • packages/3-targets/3-targets/postgres/test/migrations/fixtures/runner-fixtures.ts
  • packages/3-targets/3-targets/postgres/test/migrations/runner.errors.integration.test.ts
  • packages/3-targets/3-targets/postgres/src/exports/control.ts
  • packages/3-targets/3-targets/postgres/test/migrations/runner.idempotency.integration.test.ts
  • packages/3-targets/3-targets/postgres/test/migrations/runner.basic.integration.test.ts
  • packages/3-targets/3-targets/postgres/test/migrations/runner.policy.integration.test.ts
  • docs/architecture docs/subsystems/7. Migration System.md

@wmadden wmadden merged commit 8259906 into main Apr 2, 2026
15 checks passed
wmadden added a commit that referenced this pull request Apr 2, 2026
- Remove unnecessary export from REFERENTIAL_ACTION_SQL
- Add blank line between unrelated functions in planner.ts
- Trim redundant comments in buildColumnDefaultSql
- Document optional table param semantics on constraintExistsCheck
- Add unit tests for planner-schema-lookup (16 tests)
@wmadden wmadden deleted the refactor/modularize-planner branch April 2, 2026 17:18
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.

2 participants