feat(migrations): data migration support with descriptor-based planner#268
Merged
feat(migrations): data migration support with descriptor-based planner#268
Conversation
|
Warning Rate limit exceeded
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 53 minutes and 50 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (60)
📝 WalkthroughWalkthroughAdds descriptor-based migration authoring and execution including a new 'data' migration operation class, descriptor schemas/builders, CLI scaffolding/evaluation for migration.ts, descriptor planning/resolution for Postgres, data-transform execution lifecycle, draft-aware migration bundle handling (dashed edges), and related tests and package exports. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI
participant FS as Filesystem
participant Config as ConfigLoader
participant Planner as DescriptorPlanner
participant Resolver as DescriptorResolver
participant DB as Postgres
participant Runner as MigrationRunner
User->>CLI: migration new --name X
CLI->>Config: load config
CLI->>FS: read contract.json & migrations dir
CLI->>FS: write migration manifest (migrationId: null)
CLI->>FS: scaffold migration.ts
User->>CLI: migration verify --config
CLI->>FS: read & evaluate migration.ts -> descriptors
CLI->>Resolver: resolveDescriptors(descriptors, context)
Resolver->>Planner: planWithDescriptors(context) [optional path]
Planner-->>Resolver: descriptors / conflicts
Resolver->>DB: lower query plans -> SerializedQueryPlan (sql + params)
Resolver-->>FS: write ops.json (resolved MigrationPlanOperation[])
CLI->>FS: attest migration (writeMigrationManifest)
User->>CLI: migration apply
CLI->>FS: load attested bundles/graph
CLI->>Runner: execute plan
Runner->>DB: schema ops -> precheck/execute/postcheck
Runner->>DB: data ops -> check -> run steps -> postcheck
Runner-->>CLI: apply complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
@prisma-next/mongo-runtime
@prisma-next/family-mongo
@prisma-next/sql-runtime
@prisma-next/family-sql
@prisma-next/middleware-telemetry
@prisma-next/extension-paradedb
@prisma-next/extension-pgvector
@prisma-next/postgres
@prisma-next/sql-orm-client
@prisma-next/sqlite
@prisma-next/target-mongo
@prisma-next/adapter-mongo
@prisma-next/driver-mongo
@prisma-next/contract
@prisma-next/utils
@prisma-next/config
@prisma-next/errors
@prisma-next/framework-components
@prisma-next/operations
@prisma-next/contract-authoring
@prisma-next/ids
@prisma-next/psl-parser
@prisma-next/psl-printer
@prisma-next/cli
@prisma-next/emitter
@prisma-next/migration-tools
@prisma-next/vite-plugin-contract-emit
@prisma-next/runtime-executor
@prisma-next/mongo-codec
@prisma-next/mongo-contract
@prisma-next/mongo-value
@prisma-next/mongo-contract-psl
@prisma-next/mongo-contract-ts
@prisma-next/mongo-emitter
@prisma-next/mongo-schema-ir
@prisma-next/mongo-query-ast
@prisma-next/mongo-orm
@prisma-next/mongo-pipeline-builder
@prisma-next/mongo-lowering
@prisma-next/mongo-wire
@prisma-next/sql-contract
@prisma-next/sql-errors
@prisma-next/sql-operations
@prisma-next/sql-schema-ir
@prisma-next/sql-contract-psl
@prisma-next/sql-contract-ts
@prisma-next/sql-contract-emitter
@prisma-next/sql-lane-query-builder
@prisma-next/sql-relational-core
@prisma-next/sql-builder
@prisma-next/target-postgres
@prisma-next/target-sqlite
@prisma-next/adapter-postgres
@prisma-next/adapter-sqlite
@prisma-next/driver-postgres
@prisma-next/driver-sqlite
commit: |
saevarb
force-pushed
the
feat/data-migrations
branch
3 times, most recently
from
214212a to
c0811c9
Compare
saevarb
force-pushed
the
feat/data-migrations
branch
from
c0811c9 to
0b63d7f
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
saevarb
force-pushed
the
feat/data-migrations
branch
from
0b63d7f to
db07b45
Compare
saevarb
changed the title
saevarb
force-pushed
the
feat/data-migrations
branch
from
de15296 to
cc004c7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/1-framework/3-tooling/cli/src/commands/migration-status.ts (1)
439-466:⚠️ Potential issue | 🟠 MajorDraft-only repos still fall through the “no migrations” path.
You now collect
draftsand emit a warning, but the earlyattested.length === 0success return below still omits bothdraftsandgraph. A fresh repo aftermigration newwill therefore report “No migrations found” in JSON/default rendering even though a draft exists, which breaks the new draft-visibility workflow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/cli/src/commands/migration-status.ts` around lines 439 - 466, The current early return that triggers when attested.length === 0 omits drafts and graph causing draft-only repos to report "No migrations"; update the logic in migration-status.ts (around the attested/drafts/graph handling after loadAllBundles) so that the success return includes drafts and graph when drafts.length > 0 (either change the early return condition to consider drafts or construct the success payload to include drafts and graph even when attested is empty), ensuring functions/variables attested, drafts, graph and the success/notOk return path are updated accordingly.
🧹 Nitpick comments (9)
turbo.json (1)
44-46: Consider addinginputsandoutputsfor optimal caching.The new
emittask lacksinputsandoutputsarrays. Most other tasks in this config define them so Turbo can:
- Invalidate cache when relevant files change
- Skip execution when inputs haven't changed
- Track generated artifacts
Based on the AI summary, this task generates contract/migration inputs after the CLI build completes. Consider specifying which source files trigger the emit (inputs) and what artifacts it produces (outputs).
📦 Example configuration with inputs/outputs
"@prisma-next/sql-builder#emit": { - "dependsOn": ["@prisma-next/cli#build"] + "dependsOn": ["@prisma-next/cli#build"], + "inputs": ["src/**", "package.json"], + "outputs": ["dist/**"] }Note: Adjust the actual paths based on what files the emit task reads and generates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@turbo.json` around lines 44 - 46, Add explicit inputs and outputs to the "@prisma-next/sql-builder#emit" Turbo task so caching is accurate: update the task definition for "@prisma-next/sql-builder#emit" to include an "inputs" array that lists the source files this emit step reads (e.g., schema, contract, migration, and CLI build artifacts such as "packages/sql-builder/src/**", "prisma/schema.prisma", or any contract templates) and an "outputs" array that lists the generated artifacts the task produces (e.g., "packages/sql-builder/dist/**", "packages/sql-builder/generated/**" or the specific contract/migration files), ensuring the task consumes the CLI build result (dependsOn remains) and Turbo can invalidate/skip runs correctly.packages/3-targets/6-adapters/postgres/src/core/enum-control-hooks.ts (1)
700-717: Clarify reorder-only enum diffs in the verification message.When values are only reordered, this path reports
+[ ] -[ ], which is hard to interpret. Consider a dedicated “reordered” message branch for rebuild diffs with no adds/removals.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-targets/6-adapters/postgres/src/core/enum-control-hooks.ts` around lines 700 - 717, The verification message currently prints +[ ] -[ ] for reorder-only enum diffs; update the message generation in the enum diff path (around determineEnumDiff, diff.kind and the object with kind 'enum_values_changed') to detect reorder-only cases (either diff.kind === 'reorder' if available, or when addedValues.length === 0 && removedValues.length === 0) and emit a dedicated message like `Enum type "${typeName}" values reordered: [${desired.join(', ')}]` instead of the +/-. Keep the existing messages for 'add_values' and rebuild cases with real adds/removals.packages/3-targets/3-targets/postgres/src/core/migrations/operation-descriptors.ts (2)
12-66: Consider consolidating duplicate type imports.Types are re-exported on lines 12-57, then imported again on lines 59-66 for internal use. This mirrors the pattern in the sql-family file and could be consolidated.
🤖 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/operation-descriptors.ts` around lines 12 - 66, The file currently duplicates type exports and imports from '@prisma-next/family-sql/operation-descriptors' — remove the second import block (lines importing Buildable, DataTransformDescriptor, SqlMigrationOpDescriptor, builders as structuralBuilders, TODO, TodoMarker) and consolidate into a single import/export so the same type symbols (e.g., Buildable, SqlMigrationOpDescriptor, TODO, TodoMarker, DataTransformDescriptor) are imported once and re-exported together with the operation helpers (addColumn, addEnumValues, createTable, etc.); update the top export/import so internal usage and re-exports reference the same imported symbols instead of importing them twice.
105-132: VerifyresolveInputhandles all Buildable cases correctly.The
resolveInputfunction checks'build' in inputbutinputcould be aFunction(callback) which also has properties. The type narrowing assumes that ifinputhas abuildmethod, it's aBuildable, but a function with a coincidentalbuildproperty could slip through.This is likely safe in practice since migration authors won't accidentally create functions with
buildmethods, but worth a note.🛡️ More defensive check
function resolveInput(input: QueryPlanInput): QueryPlanInput { if (typeof input === 'symbol' || typeof input === 'function') return input; - if ('build' in input && typeof (input as Buildable).build === 'function') { + if (typeof input === 'object' && input !== null && 'build' in input && typeof input.build === 'function') { return (input as Buildable).build(); } return input; }🤖 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/operation-descriptors.ts` around lines 105 - 132, Summary: resolveInput currently checks 'build' in input without ensuring input is an object, so a Function with a build property could be misclassified; change the guard to only treat non-null objects as Buildable. Update resolveInput to first return for typeof input === 'symbol' || typeof input === 'function' (keep existing) and then check if typeof input === 'object' && input !== null && 'build' in input && typeof (input as Buildable).build === 'function' before calling build; also scan usages such as dataTransform's handling of options.check and options.run to ensure they call resolveInput only for non-function/non-symbol inputs (or rely on the updated resolveInput) so functions aren't mistaken for Buildable.packages/2-sql/9-family/src/core/migrations/operation-descriptors.ts (1)
13-61: Consider consolidating duplicate type imports.Types are re-exported on lines 13-36, then imported again on lines 40-61 for internal use. You could use a single import with
typemodifier and re-export in one statement.♻️ Suggested consolidation
-// Re-export types and schemas from the schema source of truth -export type { - AddColumnDescriptor, - // ... all types -} from './descriptor-schemas'; - -export { MigrationDescriptorArraySchema } from './descriptor-schemas'; - -import type { - AddColumnDescriptor, - // ... same types again -} from './descriptor-schemas'; +// Re-export types and schemas from the schema source of truth +export type { + AddColumnDescriptor, + AddEnumValuesDescriptor, + // ... all types +} from './descriptor-schemas'; + +export { MigrationDescriptorArraySchema } from './descriptor-schemas'; + +// Use inline type imports in function signatures instead of +// duplicating the import block, or use a single combined import: +import type * as Schemas from './descriptor-schemas'; +// Then reference as Schemas.AddColumnDescriptor, etc.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/9-family/src/core/migrations/operation-descriptors.ts` around lines 13 - 61, Duplicate type imports for descriptors (AddColumnDescriptor, AddEnumValuesDescriptor, AddForeignKeyDescriptor, AddPrimaryKeyDescriptor, AddUniqueDescriptor, AlterColumnTypeDescriptor, CreateDependencyDescriptor, CreateEnumTypeDescriptor, CreateIndexDescriptor, CreateTableDescriptor, DropColumnDescriptor, DropConstraintDescriptor, DropDefaultDescriptor, DropEnumTypeDescriptor, DropIndexDescriptor, DropNotNullDescriptor, DropTableDescriptor, RenameTypeDescriptor, SetDefaultDescriptor, SetNotNullDescriptor) are present; remove the second import block and consolidate into a single statement by re-exporting the types from './descriptor-schemas' (use a single export type { ... } from './descriptor-schemas';) while keeping the existing export { MigrationDescriptorArraySchema } from './descriptor-schemas'; so there is only one import/re-export for those descriptor types and no duplicate import type block.packages/1-framework/3-tooling/cli/test/utils/formatters/graph-migration-mapper.test.ts (1)
199-201: Strengthen dashed-edge assertions to lock endpoint behaviorThese checks confirm edge presence, but they don’t fully validate edge routing. Add
from/toendpoint assertions in both detached-node tests to avoid false positives when dashed edges are attached to the wrong node.Suggested assertion hardening
const dashedEdge = result.graph.edges.find((e) => e.style === 'dashed'); expect(dashedEdge).toBeDefined(); + expect(dashedEdge!.from).toBe(result.options.spineTarget); expect(dashedEdge!.to).toBe(sid('DETACHED_HASH')); @@ const dashedEdge = result.graph.edges.find((e) => e.style === 'dashed'); expect(dashedEdge).toBeDefined(); + expect(dashedEdge!.from).toBe(result.options.spineTarget); + expect(dashedEdge!.to).toBe(sid('OFF_GRAPH'));Also applies to: 221-223
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/cli/test/utils/formatters/graph-migration-mapper.test.ts` around lines 199 - 201, The tests currently only assert that a dashed edge exists and its `to` endpoint, which can hide cases where the edge's `from` is incorrect; update the detached-node tests that find `dashedEdge` (the assertion around `result.graph.edges.find((e) => e.style === 'dashed')`) to also assert `dashedEdge!.from` equals the expected source node id (use the same `sid(...)` helper for the expected source) in addition to the existing `dashedEdge!.to === sid('DETACHED_HASH')` check; apply the same strengthening to the second detached-node test (the one near the other assertions at 221-223) so both tests lock both endpoints.packages/3-targets/3-targets/postgres/package.json (1)
48-48: Route migration-builder APIs through canonical plane entrypoints
./migration-builderslooks migration-plane specific but is exported as a standalone subpath. Prefer exposing this via./control(or moving truly shared bits to shared-plane exports) to keep plane boundaries consistent.As per coding guidelines: “Do not export plane-specific files via the wrong entrypoint… Export multi-plane entrypoints from
package.jsonwith separate./adapter,./control, and./runtimesubpaths.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-targets/3-targets/postgres/package.json` at line 48, Replace the standalone subpath export "./migration-builders" with a control-plane entrypoint by moving its mapping to the package.json control export (i.e., stop exporting "./migration-builders" and instead expose the same "./dist/migration-builders.mjs" via the "./control" subpath); update the package.json exports so consumers import the migration-builder API through "./control" (or refactor truly shared pieces into shared-plane exports) to keep plane boundaries consistent.test/integration/test/cli.migration-apply.e2e.test.ts (1)
289-384: Drop this migrated resume journey from the legacy suite.This now covers the same duplicate-email → unique-constraint → resume flow as
test/integration/test/cli-journeys/migration-apply-edge-cases.e2e.test.ts. Keeping both doubles a slow dev-database path and makes future behavior changes easy to update in one place but miss in the other.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/test/cli.migration-apply.e2e.test.ts` around lines 289 - 384, Remove the duplicated legacy test "resume after partial apply" from the legacy suite: delete (or skip) the describe block named 'resume after partial apply' / the it block titled 'resumes from last successful migration after failure' in test/integration/test/cli.migration-apply.e2e.test.ts; this block exercises the duplicate-email → unique-constraint → resume flow already covered by cli-journeys/migration-apply-edge-cases.e2e.test.ts, so remove references to its helpers (setupTestDirectoryFromFixtures, emitContract, runMigrationPlan, runMigrationApply, withDevDatabase, withClient, readMigrationsDir, getExitCode) to avoid running the slow dev-database path twice. Ensure any imports only used by this block are cleaned up.packages/1-framework/3-tooling/cli/test/utils/formatters/graph-render.test.ts (1)
136-148: Split this formatter test file before adding more cases.This file is already over the 500-line limit, and these new dashed-edge/truncation cases add another distinct concern on top of extraction, truncation, rendering, and marker coverage. Moving the dashed-edge/truncation scenarios into a sibling file would keep snapshots and failures much easier to navigate.
As per coding guidelines, "
**/*.test.ts: 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."Also applies to: 229-233, 411-426
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/cli/test/utils/formatters/graph-render.test.ts` around lines 136 - 148, The test file has grown past the 500-line guideline and the new dashed-edge/truncation cases introduce a separate concern; split out the dashed-edge and truncation tests into a new sibling test file by moving the test(s) that reference RenderGraph, extractRelevantSubgraph and any related dashed/truncation snapshots into a new file (e.g., graph-render.dashed.test.ts), update imports to reference RenderGraph and extractRelevantSubgraph from the same module, and run/update snapshots so the original file stays under 500 lines and the new file contains only the dashed-edge/truncation scenarios.
🤖 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/1-framework/1-core/framework-components/src/control-migration-types.ts`:
- Around line 341-358: Remove the orphaned/duplicated JSDoc block for
resolveDescriptors that precedes the actual JSDoc for planWithDescriptors:
locate the duplicate comment text describing resolveDescriptors (the block that
is not attached to a function) and delete it so only the correct JSDoc remains
immediately above planWithDescriptors; ensure the real resolveDescriptors and
planWithDescriptors function declarations keep their proper single JSDoc each
(reference symbols: resolveDescriptors, planWithDescriptors).
In `@packages/1-framework/3-tooling/cli/src/commands/migration-apply.ts`:
- Around line 198-205: loadAllBundles() returns drafts and attested sets, but
migration-apply.ts currently uses migrations.graph (which includes drafts) for
markerHash/destinationHash checks and path selection while materializing steps
from migrations.attested, causing draft-only targets to pass early then fail
later; after calling loadAllBundles(migrationsDir) (the migrations variable),
construct an attested-only graph from migrations.attested and use that
attestedGraph for all validation and path-picking logic
(markerHash/destinationHash checks and path resolution in place of
migrations.graph), while keeping migrations.drafts only for the
ui.warn/diagnostic messages (e.g., the existing drafts warning and the “verify
this draft first” guidance); update all the decision points that currently
reference migrations.graph (the sections around the current marker/destination
validation and path selection and the code between the earlier mentioned ranges)
to reference the attested-only graph so materialization later (which uses
migrations.attested) cannot fail with “Migration package not found.”
In `@packages/1-framework/3-tooling/cli/src/commands/migration-new.ts`:
- Around line 129-138: Before using findLatestMigration to set
fromHash/fromContract, detect whether the attested graph is branched by checking
for multiple leaf packages (i.e., more than one package in attested that
represents a leaf of the migration graph); if more than one leaf exists, do not
call findLatestMigration or silently pick a branch—instead return/throw a
structured error that explains the history is branched and instructs the user to
supply --from (include the names/ids of the leaf packages for clarity). Update
the block around findLatestMigration/latestMigration/fromHash/fromContract in
migration-new.ts to perform this branch-detection and produce the error when
multiple leaves are found.
In `@packages/1-framework/3-tooling/cli/src/commands/migration-plan.ts`:
- Around line 301-368: The current flow calls writeMigrationPackage and
scaffoldMigrationTs early, which leaves a draft package on disk if the non-draft
path (evaluateMigrationTs, migrations.resolveDescriptors, writeMigrationOps,
attestMigration) fails; update the logic so that when
descriptorResult.needsDataMigration === false you either create the package in a
temporary directory and atomically rename/move it into place only after
resolveDescriptors, writeMigrationOps and attestMigration all succeed, or ensure
you delete/cleanup the packageDir on any thrown error before returning
notOk(mapMigrationToolsError(error)); target the functions
writeMigrationPackage, scaffoldMigrationTs, evaluateMigrationTs,
migrations.resolveDescriptors, writeMigrationOps, attestMigration and the catch
block that calls mapMigrationToolsError to implement the temp-dir-or-cleanup
behavior.
In `@packages/1-framework/3-tooling/cli/src/commands/migration-verify.ts`:
- Around line 114-123: The code currently calls attestMigration(dir) and returns
an attested result when the stored migrationId mismatches ops.json; instead,
change migration-verify.ts so that on mismatch you do not auto-attest for
non-draft packages: check package draft status (e.g., a function/flag used
elsewhere in this file or project) and if the package is non-draft, return or
throw a verification error (fail the command) describing the stale migrationId
and include both stored and expected IDs; only when the package is a draft
should you call attestMigration(dir) and return the ok(...) attested response
with migrationId.
In
`@packages/1-framework/3-tooling/cli/src/utils/formatters/graph-migration-mapper.ts`:
- Around line 219-227: The current guard drops edges when matchingDraft.from
references another draft not present in graph.nodes; update the logic around
matchingDraft / fromHash in graph-migration-mapper.ts to instead traverse
input.draftEdges (or input.draftEdges keyed by .from) from matchingDraft.from up
the chain until you find an ancestor that exists in graph.nodes or reach
spineTargetHash, and use that ancestor as fromHash, or alternatively materialize
the intermediate draft nodes/edges (creating node entries and dashed edges for
each draft in the chain) before pushing to edgeList; ensure you preserve the
dashed style and label (matchingDraft.dirName + " [draft]") for draft edges and
fall back to spineTargetHash only if no attested ancestor is found.
In `@packages/1-framework/3-tooling/cli/src/utils/formatters/graph-render.ts`:
- Around line 913-923: The truncation step currently prunes nodes reachable only
from the visible spine and thus can remove dashed (draft) edges and their
off-graph endpoints; update truncateGraph to mirror extractRelevantSubgraph’s
behavior by first collecting dashed edges (graph.edges.filter(e => e.style ===
'dashed')) and adding both endpoints (e.from/e.to) to the kept node set before
computing reachable nodes from the spine window, and ensure the final edge
filter always includes edges whose pair is in edgePairs OR where e.style ===
'dashed' so dashed edges and their endpoints survive truncation.
In `@packages/1-framework/3-tooling/migration/src/exports/types.ts`:
- Around line 5-7: Remove the legacy aliases for BaseMigrationBundle from the
export surface: drop the re-exports "BaseMigrationBundle as MigrationBundle" and
"BaseMigrationBundle as MigrationPackage" so only the canonical types
(BaseMigrationBundle, AttestedMigrationBundle, DraftMigrationBundle) are
exported; then update any call sites that import MigrationBundle or
MigrationPackage to import the appropriate new symbol (BaseMigrationBundle /
AttestedMigrationBundle / DraftMigrationBundle) instead. Ensure no new
compatibility aliases are introduced in the exports file and run a project-wide
search/replace for uses of MigrationBundle and MigrationPackage to switch them
to the explicit types.
In `@packages/1-framework/3-tooling/migration/src/migration-ts.ts`:
- Around line 119-125: The generated import for the Contract type uses
relativeContractDts which can produce bare specifiers like "contract.d" or
"types/contract.d"; update the code around hasDataTransform and
options.contractJsonPath to ensure the import path is a local relative path by
prefixing "./" when relativeContractDts does not already start with "./" or
"../" before calling lines.push(`import type { Contract } from
"${relativeContractDts}"`), so the generated migration.ts imports the local .d
file instead of resolving via node_modules.
In `@packages/2-sql/4-lanes/relational-core/src/query-operation-registry.ts`:
- Around line 24-33: The entries() method currently returns
Object.freeze(Object.fromEntries(map)) which only shallow-freezes the top-level
record and because register(descriptor) stores caller-derived nested objects via
const { method: _, ...entry } = descriptor those nested fields (args, returns,
lowering) remain mutable; to fix, deep-freeze the stored entry at registration
(in register) using the project's recursive freeze helper/pattern (see
plan-helpers.ts) before map.set so map holds immutable nested structures, and
ensure entries() still returns a frozen snapshot (you can keep Object.freeze on
Object.fromEntries(map) but the stored entries must be recursively frozen in
register to prevent external mutation).
In `@packages/2-sql/9-family/src/core/migrations/contract-to-schema-ir.ts`:
- Around line 263-268: When re-keying storage.types by nativeType (the
byNativeType map built from storage.types and typeInstance.nativeType) add a
guard that detects collisions: while iterating Object.values(storage.types)
check if byNativeType already has that nativeType and, if so, throw a clear
error that includes the conflicting nativeType and the involved type identifiers
(e.g., the existing byNativeType[nativeType] and the new typeInstance) so the
failure is fail-fast; keep using annotationNamespace and return shape unchanged
once no collisions are found.
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/descriptor-planner.ts`:
- Around line 418-460: The final ordering places dropOps before patternOps which
causes destructive drops (e.g., column drops) to run before generated
dataTransform patternOps can read data; update the descriptors assembly so
patternOps comes before dropOps (i.e., build descriptors from ...depOps,
...tableOps, ...columnOps, ...patternOps, ...dropOps, ...alterOps,
...constraintOps) or otherwise ensure dropOps that target columns referenced by
patternOps are moved after patternOps; modify the array construction around the
symbols depOps, dropOps, patternOps, tableOps, columnOps, alterOps,
constraintOps and update any related ordering logic to preserve that patternOps
execute before destructive drops.
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/operation-descriptors.ts`:
- Around line 134-140: The descriptor currently hardcodes source: 'migration.ts'
which breaks traceability; update the factory that returns the dataTransform
descriptor in operation-descriptors.ts to accept a source string (e.g., add a
source?: string parameter to the function that builds the returned object) and
set source to that parameter instead of the literal, and if source is not
provided derive a fallback using a caller location (Error().stack) or a
lightweight caller utility so generated descriptors contain the originating file
path; ensure the returned object still has kind: 'dataTransform', name, check,
and run fields unchanged.
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/operation-resolver.ts`:
- Around line 695-727: The generated enum-add operation from
resolveAddEnumValues creates ALTER TYPE ... ADD VALUE steps that must run
outside a transaction; add a non-transactional marker to the returned ResolvedOp
(e.g., set nonTransactional: true or similar) and propagate that flag into the
SqlMigrationPlanOperation so the runner can execute such operations without
BEGIN/COMMIT; update resolveAddEnumValues to include the new property alongside
id/label/operationClass/target/execute/postcheck and then update the migration
planner/runner to honor that property when deciding transactional boundaries so
ALTER TYPE steps run non-transactionally.
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/planner-strategies.ts`:
- Around line 84-94: Update enumRebuildRecipe to drop and recreate column
defaults: add dropDefault and setDefault to the imports from
./operation-descriptors, then modify enumRebuildRecipe so that for each entry in
columnRefs you emit a dropDefault(ref.table, ref.column) before the
alterColumnType(...) call and, after renameType(tempName, nativeType) (and after
dropEnumType/native recreation), emit setDefault(ref.table, ref.column,
ref.default) for each columnRef to restore the original default; keep the
existing createEnumType, alterColumnType, dropEnumType, and renameType steps but
surround alters with the dropDefault/setDefault pair to avoid type-binding
errors.
In `@packages/3-targets/3-targets/postgres/src/core/migrations/runner.ts`:
- Around line 279-343: The current executeDataTransform allows op.run to execute
without a query-based op.check, which permits silent partial/no-op backfills;
require a query-based check when a run is present: in executeDataTransform
(handling DataTransformOperation), add an early validation so if op.run is
non-empty and op.check is not an object with sql/params (i.e., op.check === null
|| op.check === false || op.check === true) return a runnerFailure (e.g., new
code path 'INVALID_TRANSFORM') with a clear message and meta including
operationId and dataTransformName; this ensures callers must provide a check
query for run steps and preserves the check→run→check lifecycle (alternatively,
if you prefer to force post-run validation, ensure op.check.sql is executed
after any run by treating boolean false as an error and running the check only
when an SQL check object exists).
In
`@packages/3-targets/3-targets/postgres/test/migrations/descriptor-planner.scenarios.md`:
- Around line 45-46: Update the enum scenarios in
descriptor-planner.scenarios.md to reflect the new rebuild-based enum evolution
instead of marking "Enum values added" and "Enum values removed" as conflicts:
remove or replace the two bullets that say "conflict (not yet supported)" and
state that enum additions/removals are handled by the rebuild recipe (mentioning
"rebuild recipe" / "enum evolution via rebuild") and briefly note any relevant
behavior expected during rebuilds so tests/readers know the current flow.
In
`@packages/3-targets/3-targets/postgres/test/migrations/descriptor-planner.scenarios.test.ts`:
- Around line 131-1183: The test file is too large and bundles many distinct
scenario groups; split the big describe-suite into multiple focused test files
(e.g., additive, reconciliation, types, dependencies, ordering,
data-safety-gaps, mixed/complex, old-planner-parity) by moving each top-level
describe(...) block (names like 'additive — fresh database', 'additive —
existing contract', 'reconciliation — drops', 'reconciliation — alters',
'types', 'dependencies', 'ordering', 'combined / realistic', 'data-safety gaps',
'mixed / complex', 'old planner parity') into its own test file that preserves
all its it(...) cases and test titles; ensure each new file imports the shared
test helpers and symbols used by the tests (functions/constructors like plan,
contract, table, uuidCol, textCol, col, intCol, boolCol, descriptorKinds,
descriptorSummary, defaultComponents, pgvectorDescriptor, and any local helpers
such as enumType or componentsWithPgvector) by either moving those shared
helpers into a test-utils module or re-declaring/duplicating minimal shared
fixtures in the new files; keep behavior identical (no code changes to tests)
and run the suite to confirm tests still execute.
---
Outside diff comments:
In `@packages/1-framework/3-tooling/cli/src/commands/migration-status.ts`:
- Around line 439-466: The current early return that triggers when
attested.length === 0 omits drafts and graph causing draft-only repos to report
"No migrations"; update the logic in migration-status.ts (around the
attested/drafts/graph handling after loadAllBundles) so that the success return
includes drafts and graph when drafts.length > 0 (either change the early return
condition to consider drafts or construct the success payload to include drafts
and graph even when attested is empty), ensuring functions/variables attested,
drafts, graph and the success/notOk return path are updated accordingly.
---
Nitpick comments:
In
`@packages/1-framework/3-tooling/cli/test/utils/formatters/graph-migration-mapper.test.ts`:
- Around line 199-201: The tests currently only assert that a dashed edge exists
and its `to` endpoint, which can hide cases where the edge's `from` is
incorrect; update the detached-node tests that find `dashedEdge` (the assertion
around `result.graph.edges.find((e) => e.style === 'dashed')`) to also assert
`dashedEdge!.from` equals the expected source node id (use the same `sid(...)`
helper for the expected source) in addition to the existing `dashedEdge!.to ===
sid('DETACHED_HASH')` check; apply the same strengthening to the second
detached-node test (the one near the other assertions at 221-223) so both tests
lock both endpoints.
In
`@packages/1-framework/3-tooling/cli/test/utils/formatters/graph-render.test.ts`:
- Around line 136-148: The test file has grown past the 500-line guideline and
the new dashed-edge/truncation cases introduce a separate concern; split out the
dashed-edge and truncation tests into a new sibling test file by moving the
test(s) that reference RenderGraph, extractRelevantSubgraph and any related
dashed/truncation snapshots into a new file (e.g., graph-render.dashed.test.ts),
update imports to reference RenderGraph and extractRelevantSubgraph from the
same module, and run/update snapshots so the original file stays under 500 lines
and the new file contains only the dashed-edge/truncation scenarios.
In `@packages/2-sql/9-family/src/core/migrations/operation-descriptors.ts`:
- Around line 13-61: Duplicate type imports for descriptors
(AddColumnDescriptor, AddEnumValuesDescriptor, AddForeignKeyDescriptor,
AddPrimaryKeyDescriptor, AddUniqueDescriptor, AlterColumnTypeDescriptor,
CreateDependencyDescriptor, CreateEnumTypeDescriptor, CreateIndexDescriptor,
CreateTableDescriptor, DropColumnDescriptor, DropConstraintDescriptor,
DropDefaultDescriptor, DropEnumTypeDescriptor, DropIndexDescriptor,
DropNotNullDescriptor, DropTableDescriptor, RenameTypeDescriptor,
SetDefaultDescriptor, SetNotNullDescriptor) are present; remove the second
import block and consolidate into a single statement by re-exporting the types
from './descriptor-schemas' (use a single export type { ... } from
'./descriptor-schemas';) while keeping the existing export {
MigrationDescriptorArraySchema } from './descriptor-schemas'; so there is only
one import/re-export for those descriptor types and no duplicate import type
block.
In `@packages/3-targets/3-targets/postgres/package.json`:
- Line 48: Replace the standalone subpath export "./migration-builders" with a
control-plane entrypoint by moving its mapping to the package.json control
export (i.e., stop exporting "./migration-builders" and instead expose the same
"./dist/migration-builders.mjs" via the "./control" subpath); update the
package.json exports so consumers import the migration-builder API through
"./control" (or refactor truly shared pieces into shared-plane exports) to keep
plane boundaries consistent.
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/operation-descriptors.ts`:
- Around line 12-66: The file currently duplicates type exports and imports from
'@prisma-next/family-sql/operation-descriptors' — remove the second import block
(lines importing Buildable, DataTransformDescriptor, SqlMigrationOpDescriptor,
builders as structuralBuilders, TODO, TodoMarker) and consolidate into a single
import/export so the same type symbols (e.g., Buildable,
SqlMigrationOpDescriptor, TODO, TodoMarker, DataTransformDescriptor) are
imported once and re-exported together with the operation helpers (addColumn,
addEnumValues, createTable, etc.); update the top export/import so internal
usage and re-exports reference the same imported symbols instead of importing
them twice.
- Around line 105-132: Summary: resolveInput currently checks 'build' in input
without ensuring input is an object, so a Function with a build property could
be misclassified; change the guard to only treat non-null objects as Buildable.
Update resolveInput to first return for typeof input === 'symbol' || typeof
input === 'function' (keep existing) and then check if typeof input === 'object'
&& input !== null && 'build' in input && typeof (input as Buildable).build ===
'function' before calling build; also scan usages such as dataTransform's
handling of options.check and options.run to ensure they call resolveInput only
for non-function/non-symbol inputs (or rely on the updated resolveInput) so
functions aren't mistaken for Buildable.
In `@packages/3-targets/6-adapters/postgres/src/core/enum-control-hooks.ts`:
- Around line 700-717: The verification message currently prints +[ ] -[ ] for
reorder-only enum diffs; update the message generation in the enum diff path
(around determineEnumDiff, diff.kind and the object with kind
'enum_values_changed') to detect reorder-only cases (either diff.kind ===
'reorder' if available, or when addedValues.length === 0 && removedValues.length
=== 0) and emit a dedicated message like `Enum type "${typeName}" values
reordered: [${desired.join(', ')}]` instead of the +/-. Keep the existing
messages for 'add_values' and rebuild cases with real adds/removals.
In `@test/integration/test/cli.migration-apply.e2e.test.ts`:
- Around line 289-384: Remove the duplicated legacy test "resume after partial
apply" from the legacy suite: delete (or skip) the describe block named 'resume
after partial apply' / the it block titled 'resumes from last successful
migration after failure' in
test/integration/test/cli.migration-apply.e2e.test.ts; this block exercises the
duplicate-email → unique-constraint → resume flow already covered by
cli-journeys/migration-apply-edge-cases.e2e.test.ts, so remove references to its
helpers (setupTestDirectoryFromFixtures, emitContract, runMigrationPlan,
runMigrationApply, withDevDatabase, withClient, readMigrationsDir, getExitCode)
to avoid running the slow dev-database path twice. Ensure any imports only used
by this block are cleaned up.
In `@turbo.json`:
- Around line 44-46: Add explicit inputs and outputs to the
"@prisma-next/sql-builder#emit" Turbo task so caching is accurate: update the
task definition for "@prisma-next/sql-builder#emit" to include an "inputs" array
that lists the source files this emit step reads (e.g., schema, contract,
migration, and CLI build artifacts such as "packages/sql-builder/src/**",
"prisma/schema.prisma", or any contract templates) and an "outputs" array that
lists the generated artifacts the task produces (e.g.,
"packages/sql-builder/dist/**", "packages/sql-builder/generated/**" or the
specific contract/migration files), ensuring the task consumes the CLI build
result (dependsOn remains) and Turbo can invalidate/skip runs correctly.
🪄 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: 2a5ecb5c-6b69-4972-9f86-d65446d04814
⛔ Files ignored due to path filters (15)
packages/1-framework/3-tooling/cli/test/utils/formatters/__snapshots__/graph-render.test.ts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yamlprojects/graph-based-migrations/plans/data-migrations-plan.mdis excluded by!projects/**projects/graph-based-migrations/specs/april-milestone.mdis excluded by!projects/**projects/graph-based-migrations/specs/chat.mdis excluded by!projects/**projects/graph-based-migrations/specs/data-migration-scenarios.mdis excluded by!projects/**projects/graph-based-migrations/specs/data-migrations-response.mdis excluded by!projects/**projects/graph-based-migrations/specs/data-migrations-solutions.mdis excluded by!projects/**projects/graph-based-migrations/specs/data-migrations-spec.mdis excluded by!projects/**projects/graph-based-migrations/specs/data-migrations.mdis excluded by!projects/**projects/graph-based-migrations/specs/mongodb-applicability.mdis excluded by!projects/**projects/graph-based-migrations/specs/planner-replacement-proposal.mdis excluded by!projects/**projects/graph-based-migrations/walkthrough.mdis excluded by!projects/**test/integration/test/sql-builder/fixtures/generated/contract.d.tsis excluded by!**/generated/**test/integration/test/sql-builder/fixtures/generated/contract.jsonis excluded by!**/generated/**
📒 Files selected for processing (85)
packages/1-framework/1-core/framework-components/src/control-migration-types.tspackages/1-framework/1-core/framework-components/src/control-result-types.tspackages/1-framework/1-core/framework-components/src/exports/control.tspackages/1-framework/3-tooling/cli/package.jsonpackages/1-framework/3-tooling/cli/src/cli.tspackages/1-framework/3-tooling/cli/src/commands/migration-apply.tspackages/1-framework/3-tooling/cli/src/commands/migration-new.tspackages/1-framework/3-tooling/cli/src/commands/migration-plan.tspackages/1-framework/3-tooling/cli/src/commands/migration-status.tspackages/1-framework/3-tooling/cli/src/commands/migration-verify.tspackages/1-framework/3-tooling/cli/src/control-api/operations/migration-apply.tspackages/1-framework/3-tooling/cli/src/utils/command-helpers.tspackages/1-framework/3-tooling/cli/src/utils/formatters/graph-migration-mapper.tspackages/1-framework/3-tooling/cli/src/utils/formatters/graph-render.tspackages/1-framework/3-tooling/cli/src/utils/formatters/graph-types.tspackages/1-framework/3-tooling/cli/test/utils/formatters/graph-migration-mapper.test.tspackages/1-framework/3-tooling/cli/test/utils/formatters/graph-render.test.tspackages/1-framework/3-tooling/cli/test/utils/formatters/test-graphs.tspackages/1-framework/3-tooling/cli/tsdown.config.tspackages/1-framework/3-tooling/migration/package.jsonpackages/1-framework/3-tooling/migration/src/attestation.tspackages/1-framework/3-tooling/migration/src/errors.tspackages/1-framework/3-tooling/migration/src/exports/io.tspackages/1-framework/3-tooling/migration/src/exports/migration-ts.tspackages/1-framework/3-tooling/migration/src/exports/types.tspackages/1-framework/3-tooling/migration/src/io.tspackages/1-framework/3-tooling/migration/src/migration-ts.tspackages/1-framework/3-tooling/migration/src/types.tspackages/1-framework/3-tooling/migration/tsdown.config.tspackages/2-sql/2-authoring/contract-ts/package.jsonpackages/2-sql/4-lanes/relational-core/package.jsonpackages/2-sql/4-lanes/relational-core/src/exports/query-operations.tspackages/2-sql/4-lanes/relational-core/src/query-operation-registry.tspackages/2-sql/4-lanes/relational-core/tsdown.config.tspackages/2-sql/4-lanes/sql-builder/package.jsonpackages/2-sql/4-lanes/sql-builder/src/exports/types.tspackages/2-sql/4-lanes/sql-builder/src/runtime/index.tspackages/2-sql/4-lanes/sql-builder/test/fixtures/contract.tspackages/2-sql/9-family/package.jsonpackages/2-sql/9-family/src/core/migrations/contract-to-schema-ir.tspackages/2-sql/9-family/src/core/migrations/descriptor-schemas.tspackages/2-sql/9-family/src/core/migrations/operation-descriptors.tspackages/2-sql/9-family/src/core/migrations/types.tspackages/2-sql/9-family/src/core/schema-verify/verify-helpers.tspackages/2-sql/9-family/src/exports/operation-descriptors.tspackages/2-sql/9-family/test/contract-to-schema-ir.test.tspackages/2-sql/9-family/tsdown.config.tspackages/3-extensions/pgvector/package.jsonpackages/3-extensions/pgvector/src/exports/control.tspackages/3-targets/3-targets/postgres/package.jsonpackages/3-targets/3-targets/postgres/src/core/migrations/descriptor-planner.tspackages/3-targets/3-targets/postgres/src/core/migrations/operation-descriptors.tspackages/3-targets/3-targets/postgres/src/core/migrations/operation-resolver.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner-reconciliation.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner-strategies.tspackages/3-targets/3-targets/postgres/src/core/migrations/runner.tspackages/3-targets/3-targets/postgres/src/exports/control.tspackages/3-targets/3-targets/postgres/src/exports/migration-builders.tspackages/3-targets/3-targets/postgres/test/migrations/descriptor-planner.scenarios.mdpackages/3-targets/3-targets/postgres/test/migrations/descriptor-planner.scenarios.test.tspackages/3-targets/3-targets/postgres/tsdown.config.tspackages/3-targets/6-adapters/postgres/package.jsonpackages/3-targets/6-adapters/postgres/src/core/enum-control-hooks.tspackages/3-targets/6-adapters/postgres/test/enum-control-hooks.basic.test.tstest/integration/test/cli-journeys/data-transform.e2e.test.tstest/integration/test/cli-journeys/migration-apply-edge-cases.e2e.test.tstest/integration/test/cli.migration-apply.e2e.test.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-unique-email.tstest/integration/test/sql-builder/distinct.test.tstest/integration/test/sql-builder/execution.test.tstest/integration/test/sql-builder/extension-functions.test.tstest/integration/test/sql-builder/fixtures/contract.tstest/integration/test/sql-builder/fixtures/prisma-next.config.tstest/integration/test/sql-builder/group-by.test.tstest/integration/test/sql-builder/join.test.tstest/integration/test/sql-builder/mutation-defaults.test.tstest/integration/test/sql-builder/mutation.test.tstest/integration/test/sql-builder/order-by.test.tstest/integration/test/sql-builder/pagination.test.tstest/integration/test/sql-builder/select.test.tstest/integration/test/sql-builder/setup.tstest/integration/test/sql-builder/subquery.test.tstest/integration/test/sql-builder/where.test.tstest/integration/test/utils/journey-test-helpers.tsturbo.json
💤 Files with no reviewable changes (3)
- packages/3-extensions/pgvector/package.json
- packages/3-targets/6-adapters/postgres/package.json
- packages/2-sql/2-authoring/contract-ts/package.json
saevarb
force-pushed
the
feat/data-migrations
branch
3 times, most recently
from
6c0e2e0 to
0b4515e
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/3-targets/3-targets/postgres/src/core/migrations/planner-reconciliation.ts (1)
99-107:⚠️ Potential issue | 🟠 MajorDon't drop
enum_values_changedon the reconciliation path.
buildReconciliationPlan()still backs the olddb update/db initflows, and classifyingenum_values_changedas additive makes those commands ignore enum drift entirely: no operation is emitted and no conflict is returned. Until reconciliation can rebuild enums, this should stay visible as a conflict instead of being skipped.🤖 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 99 - 107, The switch in isAdditiveIssue currently classifies 'enum_values_changed' as additive which causes buildReconciliationPlan to ignore enum drift; remove 'enum_values_changed' from the case list in isAdditiveIssue so that it is not treated as additive (i.e., do not return true for 'enum_values_changed') and allow the reconciliation flow to surface it as a conflict; locate the isAdditiveIssue function and delete the 'enum_values_changed' case branch so other code (e.g., buildReconciliationPlan) will handle it as a non-additive issue.
♻️ Duplicate comments (7)
packages/1-framework/3-tooling/cli/src/utils/formatters/graph-render.ts (1)
913-923:⚠️ Potential issue | 🟠 MajorTruncation can still prune the only dashed draft edge.
Keeping dashed edges here is not sufficient because
truncateGraph()later retains only nodes reachable from the visible spine window. When a draft edge starts from an older migration outside that window, the dashed edge and off-graph contract node still disappear from the rendered default view.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/cli/src/utils/formatters/graph-render.ts` around lines 913 - 923, The truncation step later (truncateGraph) can still remove dashed (draft) edges and their off-graph endpoints because only nodes reachable from the visible spine window are kept; to fix this, ensure dashed edges and their endpoints are treated as reachable before or inside truncateGraph by adding dashed endpoints to the visible node set used by truncateGraph (or by expanding truncateGraph's reachability logic to always include nodes referenced by any graph.edges with style === 'dashed'); update the code that builds nodeSet/edgePairs (or truncateGraph) so dashedEdges and their e.from/e.to IDs are injected into the reachable/visible node collection so filteredNodes/filteredEdges cannot prune them.packages/1-framework/3-tooling/cli/src/utils/formatters/graph-migration-mapper.ts (1)
219-227:⚠️ Potential issue | 🟠 MajorNested draft chains still collapse into an orphan contract node.
If the matching draft points to another draft hash,
fromHashis not ingraph.nodes, so Lines 221-228 drop the dashed edge entirely. The common draft-on-top-of-draft case still loses its lineage instead of rendering from the nearest attested ancestor (or materializing the intermediate draft node).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/cli/src/utils/formatters/graph-migration-mapper.ts` around lines 219 - 227, The dashed edge creation drops lineage when a draft points to another draft because fromHash is set to matchingDraft?.from without resolving draft chains; update the logic using matchingDraft (and input.draftEdges) to walk the draft chain: start at matchingDraft.from and follow draftEdges (find by d.to) until you reach a hash that exists in graph.nodes or equals spineTargetHash (guard against cycles/lengths), then set fromHash to that resolved attested ancestor (or spineTargetHash) before pushing into edgeList; use the resolved fromHash (and keep the dashed style and label logic) so chained drafts render from the nearest attested ancestor instead of being dropped.packages/1-framework/3-tooling/migration/src/migration-ts.ts (1)
119-125:⚠️ Potential issue | 🟠 MajorFix the generated
Contractimport to stay relative.When
relative()returnscontract.dortypes/contract.d, Line 124 emits a bare specifier, so Node resolves it via package lookup instead of the migration directory. Prefix./when the path does not already start with..Proposed fix
if (hasDataTransform && options.contractJsonPath) { const relativeContractDts = relative(packageDir, options.contractJsonPath).replace( /\.json$/, '.d', ); - lines.push(`import type { Contract } from "${relativeContractDts}"`); + const contractImportPath = relativeContractDts.startsWith('.') + ? relativeContractDts + : `./${relativeContractDts}`; + lines.push(`import type { Contract } from "${contractImportPath}"`); lines.push(`import { createBuilders } from "@prisma-next/target-postgres/migration-builders"`);In Node.js ESM / native TypeScript import resolution, is `import type { X } from "contract.d"` treated as a bare package specifier rather than a relative local file import?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/migration/src/migration-ts.ts` around lines 119 - 125, The generated Contract import sometimes becomes a bare specifier; update the code that builds relativeContractDts (used when hasDataTransform && options.contractJsonPath) to ensure the import stays relative by prefixing './' when relativeContractDts does not start with '.'; adjust the lines.push call that emits `import type { Contract } from "${relativeContractDts}"` so it uses the possibly-prefixed path (e.g., compute a safeImportPath = relativeContractDts.startsWith('.') ? relativeContractDts : `./${relativeContractDts}` and use safeImportPath in the import string).packages/3-targets/3-targets/postgres/src/core/migrations/descriptor-planner.ts (1)
418-460:⚠️ Potential issue | 🟠 MajorKeep destructive drops after pattern-generated backfills.
Putting
dropOpsahead ofpatternOpsmeans a generateddataTransformcan lose the source column/table it needs to read. That still makes split/reshape migrations unsafe; the final ordering needs to preserve pattern-generated backfills before destructive drops.🤖 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/descriptor-planner.ts` around lines 418 - 460, The current descriptor ordering places dropOps before patternOps, which can remove columns/tables needed by pattern-generated dataTransform backfills; update the final descriptors array so patternOps come before destructive dropOps (i.e., build descriptors as [...depOps, ...tableOps, ...columnOps, ...patternOps, ...alterOps, ...constraintOps, ...dropOps] or otherwise ensure patternOps precede dropOps), adjusting only the order of depOps/dropOps/patternOps in the descriptors assembly to preserve backfill reads.packages/1-framework/3-tooling/cli/src/commands/migration-verify.ts (1)
114-123:⚠️ Potential issue | 🟠 MajorDon't re-attest non-draft migrations on mismatch.
After the refresh step, a stored/computed
migrationIdmismatch still means the package contents changed. Auto-attesting here silently blesses an edited historical migration instead of failing integrity verification; only drafts should be attested automatically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/cli/src/commands/migration-verify.ts` around lines 114 - 123, The current mismatch-handling code in migration-verify.ts re-attests any migration by calling attestMigration(dir) which silently blesses edited historical migrations; change this so only draft migrations are auto-attested: detect whether the migration in dir is a draft (e.g., via the migration metadata or an existing helper like isDraftMigration or by reading the migration manifest in the same code path), and if it's a draft call attestMigration(dir) and return the 'attested' response, but if it is not a draft do not call attestMigration — instead return a failure/verification error (e.g., ok: false or status: 'mismatch' with a clear message) so integrity verification fails for non-draft migrations.packages/1-framework/3-tooling/cli/src/commands/migration-plan.ts (1)
301-368:⚠️ Potential issue | 🟠 MajorClean up the draft package when the non-draft path fails.
writeMigrationPackage()andscaffoldMigrationTs()happen before evaluate/resolve/attest. If any later step throws, the partially written draft remains on disk and the earlier “existing draft” check blocks every retry for the same target hash until someone deletes it manually.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/cli/src/commands/migration-plan.ts` around lines 301 - 368, A partially written draft package created by writeMigrationPackage + scaffoldMigrationTs is left on disk if later steps (evaluateMigrationTs, migrations.resolveDescriptors, writeMigrationOps, attestMigration) throw, blocking retries; after creating the draft and before returning notOk(mapMigrationToolsError(error)) ensure the draft is cleaned up on failure. Concretely: after the early draft-return for descriptorResult.needsDataMigration keep current behavior, but for the non-draft path wrap evaluateMigrationTs/resolveDescriptors/writeMigrationOps/attestMigration in a try/catch/finally and on any thrown error remove the draft packageDir (or call a helper like removeMigrationPackage(packageDir) / fs.rm) before rethrowing or returning notOk(mapMigrationToolsError(error)); reference functions/vars: writeMigrationPackage, scaffoldMigrationTs, evaluateMigrationTs, migrations.resolveDescriptors, writeMigrationOps, attestMigration, mapMigrationToolsError, and packageDir.packages/3-targets/3-targets/postgres/src/core/migrations/planner-strategies.ts (1)
84-94:⚠️ Potential issue | 🟠 MajorDrop and restore defaults around enum rebuilds.
ALTER COLUMN ... TYPE ... USING ...does not carry an enum-typed default through the cast path. Rebuilds will fail on columns with existing defaults unless those defaults are dropped before the type swap and recreated afterward.In PostgreSQL, when changing a column from one enum type to another with `ALTER TABLE ... ALTER COLUMN ... TYPE ... USING ...`, do existing column defaults need to be dropped and recreated because the `USING` clause does not convert the default expression?🤖 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-strategies.ts` around lines 84 - 94, The enum-rebuild sequence must drop per-column defaults before ALTER COLUMN ... TYPE and restore them after the type swap because the USING cast does not convert enum-typed defaults; modify the sequence around createEnumType/alterColumnType/dropEnumType/renameType to (1) read and store each column's current default expression, (2) emit an operation to drop the default for each column (e.g., alterColumnDropDefault for the refs) before calling alterColumnType(ref.table, ref.column, ...), and (3) after renameType(native swap) emit operations to set the saved defaults back (e.g., alterColumnSetDefault(ref.table, ref.column, savedDefaultExpr)) so defaults are preserved through the enum rebuild.
🧹 Nitpick comments (2)
packages/2-sql/4-lanes/sql-builder/test/fixtures/contract.ts (1)
1-1: Avoid re-exporting from a non-exports/file.Line 1 uses a direct re-export. In this repo, non-
exports/modules should import then export explicitly.♻️ Suggested adjustment
-export { default as contract } from './generated/contract.json' with { type: 'json' }; +import contractJson from './generated/contract.json' with { type: 'json' }; + +export const contract = contractJson;As per coding guidelines, "Do not reexport things from one file in another, except in the exports/ folders".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-builder/test/fixtures/contract.ts` at line 1, Replace the direct re-export in the module (the `export { default as contract } from './generated/contract.json' with { type: 'json' };` statement) with an explicit import followed by an explicit export: import the default from './generated/contract.json' into a local binding (e.g., `contract`) and then export that binding as the module's export; ensure you keep the JSON import hint (`with { type: 'json' }`) when importing so the JSON is loaded correctly.packages/1-framework/3-tooling/cli/test/utils/formatters/test-graphs.ts (1)
28-29: UseifDefined()for the optional label spread.This helper introduces the inline conditional spread pattern the repo explicitly avoids in TS files. Switching to
ifDefined('label', label)keeps it consistent with the formatter codepaths.♻️ Suggested cleanup
+import { ifDefined } from '@prisma-next/utils/defined'; + function dashedEdge(from: string, to: string, label?: string): GraphEdge { - return { from, to, style: 'dashed', ...(label !== undefined ? { label } : {}) }; + return { + from, + to, + style: 'dashed', + ...ifDefined('label', label), + }; }As per coding guidelines, "Use
ifDefined()from@prisma-next/utils/definedfor conditional object spreads instead of inline conditional spread patterns".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/cli/test/utils/formatters/test-graphs.ts` around lines 28 - 29, The dashedEdge helper uses an inline conditional spread for the optional label; replace that pattern by importing and using ifDefined from `@prisma-next/utils/defined` and spread the result (e.g., ...ifDefined('label', label)) so the optional label is added consistently with other formatter codepaths; update the dashedEdge function to use ifDefined('label', label) instead of ...(label !== undefined ? { label } : {}).
🤖 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/1-framework/3-tooling/migration/src/migration-ts.ts`:
- Around line 130-137: The generated migration.ts can reference the placeholder
identifier TODO from serializeQueryInput() when a descriptor has kind
'dataTransform' and contractJsonPath is missing; ensure the import list includes
TODO in that untyped scaffold path. Modify the branch that builds importList
(the code using descriptors.map((d) => d.kind) and
importList.push('createTable')) to detect when any descriptor.kind ===
'dataTransform' and contractJsonPath is absent (the untyped path) and push
'TODO' into importList before emitting the import line so migration.ts imports
TODO from "@prisma-next/target-postgres/migration-builders".
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/operation-descriptors.ts`:
- Around line 98-99: Remove the deprecated type alias MigrationOpDescriptor and
update all call sites to use PostgresMigrationOpDescriptor directly;
specifically delete the line exporting "export type MigrationOpDescriptor =
PostgresMigrationOpDescriptor" and replace any remaining references to
MigrationOpDescriptor in the codebase with PostgresMigrationOpDescriptor (check
imports, type annotations, and public API exports) so only
PostgresMigrationOpDescriptor remains as the canonical exported type.
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/planner-strategies.ts`:
- Around line 75-82: The loop that builds columnRefs currently only matches
columns by column.typeRef === typeName; update the condition so it also matches
direct native-type enum columns by checking column.nativeType === nativeType &&
column.codecId === codecId (or the equivalent codec identifier used in this
scope), e.g. change the if to include an OR branch combining both checks,
keeping the pushed object ({ table: tableName, column: columnName }) the same;
reference the columnRefs array, ctx.toContract.storage.tables iteration, and the
typeName/nativeType/codecId symbols to locate where to modify the logic.
In `@packages/3-targets/3-targets/postgres/src/core/migrations/runner.ts`:
- Around line 206-216: The current branch treats any successful
executeDataTransform() result as "executed" and always pushes operation into
executedOperations and increments operationsExecuted; change it so that after
calling executeDataTransform(driver, operation, { runIdempotency }) you first
check the result to determine whether the transform actually ran (e.g. inspect a
field like dtResult.ran / dtResult.skipped / dtResult.status) and only push the
operation into executedOperations and increment operationsExecuted when the
result indicates it ran; keep returning dtResult on failure as-is and continue
the loop on skipped results without counting them.
In `@packages/3-targets/3-targets/postgres/src/exports/control.ts`:
- Around line 178-183: The return uses a blind cast
"planResult.value.descriptors as unknown as OperationDescriptor[]" which
violates the no-blind-cast rule; replace it by providing proper type narrowing:
add a type predicate function (e.g., isOperationDescriptorArray(descr: unknown):
descr is OperationDescriptor[]) that validates the shape against the already-run
MigrationDescriptorArraySchema (or call parseDescriptors() result) and use that
predicate to assert the type of planResult.value.descriptors, or alternatively
refactor the shared base types so PostgresMigrationOpDescriptor and
DataTransformDescriptor share a common OperationDescriptor-compatible base
(e.g., ensure DataTransformDescriptor is the base interface) and then remove the
cast and return planResult.value.descriptors directly; reference
planResult.value.descriptors, OperationDescriptor,
PostgresMigrationOpDescriptor, parseDescriptors(),
MigrationDescriptorArraySchema and DataTransformDescriptor when making the
change.
---
Outside diff comments:
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/planner-reconciliation.ts`:
- Around line 99-107: The switch in isAdditiveIssue currently classifies
'enum_values_changed' as additive which causes buildReconciliationPlan to ignore
enum drift; remove 'enum_values_changed' from the case list in isAdditiveIssue
so that it is not treated as additive (i.e., do not return true for
'enum_values_changed') and allow the reconciliation flow to surface it as a
conflict; locate the isAdditiveIssue function and delete the
'enum_values_changed' case branch so other code (e.g., buildReconciliationPlan)
will handle it as a non-additive issue.
---
Duplicate comments:
In `@packages/1-framework/3-tooling/cli/src/commands/migration-plan.ts`:
- Around line 301-368: A partially written draft package created by
writeMigrationPackage + scaffoldMigrationTs is left on disk if later steps
(evaluateMigrationTs, migrations.resolveDescriptors, writeMigrationOps,
attestMigration) throw, blocking retries; after creating the draft and before
returning notOk(mapMigrationToolsError(error)) ensure the draft is cleaned up on
failure. Concretely: after the early draft-return for
descriptorResult.needsDataMigration keep current behavior, but for the non-draft
path wrap
evaluateMigrationTs/resolveDescriptors/writeMigrationOps/attestMigration in a
try/catch/finally and on any thrown error remove the draft packageDir (or call a
helper like removeMigrationPackage(packageDir) / fs.rm) before rethrowing or
returning notOk(mapMigrationToolsError(error)); reference functions/vars:
writeMigrationPackage, scaffoldMigrationTs, evaluateMigrationTs,
migrations.resolveDescriptors, writeMigrationOps, attestMigration,
mapMigrationToolsError, and packageDir.
In `@packages/1-framework/3-tooling/cli/src/commands/migration-verify.ts`:
- Around line 114-123: The current mismatch-handling code in migration-verify.ts
re-attests any migration by calling attestMigration(dir) which silently blesses
edited historical migrations; change this so only draft migrations are
auto-attested: detect whether the migration in dir is a draft (e.g., via the
migration metadata or an existing helper like isDraftMigration or by reading the
migration manifest in the same code path), and if it's a draft call
attestMigration(dir) and return the 'attested' response, but if it is not a
draft do not call attestMigration — instead return a failure/verification error
(e.g., ok: false or status: 'mismatch' with a clear message) so integrity
verification fails for non-draft migrations.
In
`@packages/1-framework/3-tooling/cli/src/utils/formatters/graph-migration-mapper.ts`:
- Around line 219-227: The dashed edge creation drops lineage when a draft
points to another draft because fromHash is set to matchingDraft?.from without
resolving draft chains; update the logic using matchingDraft (and
input.draftEdges) to walk the draft chain: start at matchingDraft.from and
follow draftEdges (find by d.to) until you reach a hash that exists in
graph.nodes or equals spineTargetHash (guard against cycles/lengths), then set
fromHash to that resolved attested ancestor (or spineTargetHash) before pushing
into edgeList; use the resolved fromHash (and keep the dashed style and label
logic) so chained drafts render from the nearest attested ancestor instead of
being dropped.
In `@packages/1-framework/3-tooling/cli/src/utils/formatters/graph-render.ts`:
- Around line 913-923: The truncation step later (truncateGraph) can still
remove dashed (draft) edges and their off-graph endpoints because only nodes
reachable from the visible spine window are kept; to fix this, ensure dashed
edges and their endpoints are treated as reachable before or inside
truncateGraph by adding dashed endpoints to the visible node set used by
truncateGraph (or by expanding truncateGraph's reachability logic to always
include nodes referenced by any graph.edges with style === 'dashed'); update the
code that builds nodeSet/edgePairs (or truncateGraph) so dashedEdges and their
e.from/e.to IDs are injected into the reachable/visible node collection so
filteredNodes/filteredEdges cannot prune them.
In `@packages/1-framework/3-tooling/migration/src/migration-ts.ts`:
- Around line 119-125: The generated Contract import sometimes becomes a bare
specifier; update the code that builds relativeContractDts (used when
hasDataTransform && options.contractJsonPath) to ensure the import stays
relative by prefixing './' when relativeContractDts does not start with '.';
adjust the lines.push call that emits `import type { Contract } from
"${relativeContractDts}"` so it uses the possibly-prefixed path (e.g., compute a
safeImportPath = relativeContractDts.startsWith('.') ? relativeContractDts :
`./${relativeContractDts}` and use safeImportPath in the import string).
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/descriptor-planner.ts`:
- Around line 418-460: The current descriptor ordering places dropOps before
patternOps, which can remove columns/tables needed by pattern-generated
dataTransform backfills; update the final descriptors array so patternOps come
before destructive dropOps (i.e., build descriptors as [...depOps, ...tableOps,
...columnOps, ...patternOps, ...alterOps, ...constraintOps, ...dropOps] or
otherwise ensure patternOps precede dropOps), adjusting only the order of
depOps/dropOps/patternOps in the descriptors assembly to preserve backfill
reads.
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/planner-strategies.ts`:
- Around line 84-94: The enum-rebuild sequence must drop per-column defaults
before ALTER COLUMN ... TYPE and restore them after the type swap because the
USING cast does not convert enum-typed defaults; modify the sequence around
createEnumType/alterColumnType/dropEnumType/renameType to (1) read and store
each column's current default expression, (2) emit an operation to drop the
default for each column (e.g., alterColumnDropDefault for the refs) before
calling alterColumnType(ref.table, ref.column, ...), and (3) after
renameType(native swap) emit operations to set the saved defaults back (e.g.,
alterColumnSetDefault(ref.table, ref.column, savedDefaultExpr)) so defaults are
preserved through the enum rebuild.
---
Nitpick comments:
In `@packages/1-framework/3-tooling/cli/test/utils/formatters/test-graphs.ts`:
- Around line 28-29: The dashedEdge helper uses an inline conditional spread for
the optional label; replace that pattern by importing and using ifDefined from
`@prisma-next/utils/defined` and spread the result (e.g., ...ifDefined('label',
label)) so the optional label is added consistently with other formatter
codepaths; update the dashedEdge function to use ifDefined('label', label)
instead of ...(label !== undefined ? { label } : {}).
In `@packages/2-sql/4-lanes/sql-builder/test/fixtures/contract.ts`:
- Line 1: Replace the direct re-export in the module (the `export { default as
contract } from './generated/contract.json' with { type: 'json' };` statement)
with an explicit import followed by an explicit export: import the default from
'./generated/contract.json' into a local binding (e.g., `contract`) and then
export that binding as the module's export; ensure you keep the JSON import hint
(`with { type: 'json' }`) when importing so the JSON is loaded correctly.
🪄 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: 67705864-41da-42cb-a5ed-9a6395e10c28
⛔ Files ignored due to path filters (13)
packages/1-framework/3-tooling/cli/test/utils/formatters/__snapshots__/graph-render.test.ts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yamlprojects/graph-based-migrations/plans/data-migrations-plan.mdis excluded by!projects/**projects/graph-based-migrations/specs/april-milestone.mdis excluded by!projects/**projects/graph-based-migrations/specs/chat.mdis excluded by!projects/**projects/graph-based-migrations/specs/data-migration-scenarios.mdis excluded by!projects/**projects/graph-based-migrations/specs/data-migrations-solutions.mdis excluded by!projects/**projects/graph-based-migrations/specs/data-migrations-spec.mdis excluded by!projects/**projects/graph-based-migrations/specs/data-migrations.mdis excluded by!projects/**projects/graph-based-migrations/specs/planner-replacement-proposal.mdis excluded by!projects/**projects/graph-based-migrations/walkthrough.mdis excluded by!projects/**test/integration/test/sql-builder/fixtures/generated/contract.d.tsis excluded by!**/generated/**test/integration/test/sql-builder/fixtures/generated/contract.jsonis excluded by!**/generated/**
📒 Files selected for processing (80)
packages/1-framework/1-core/framework-components/src/control-migration-types.tspackages/1-framework/1-core/framework-components/src/control-result-types.tspackages/1-framework/1-core/framework-components/src/exports/control.tspackages/1-framework/3-tooling/cli/package.jsonpackages/1-framework/3-tooling/cli/src/cli.tspackages/1-framework/3-tooling/cli/src/commands/migration-apply.tspackages/1-framework/3-tooling/cli/src/commands/migration-new.tspackages/1-framework/3-tooling/cli/src/commands/migration-plan.tspackages/1-framework/3-tooling/cli/src/commands/migration-status.tspackages/1-framework/3-tooling/cli/src/commands/migration-verify.tspackages/1-framework/3-tooling/cli/src/control-api/operations/migration-apply.tspackages/1-framework/3-tooling/cli/src/utils/command-helpers.tspackages/1-framework/3-tooling/cli/src/utils/formatters/graph-migration-mapper.tspackages/1-framework/3-tooling/cli/src/utils/formatters/graph-render.tspackages/1-framework/3-tooling/cli/src/utils/formatters/graph-types.tspackages/1-framework/3-tooling/cli/test/utils/formatters/graph-migration-mapper.test.tspackages/1-framework/3-tooling/cli/test/utils/formatters/graph-render.test.tspackages/1-framework/3-tooling/cli/test/utils/formatters/test-graphs.tspackages/1-framework/3-tooling/cli/tsdown.config.tspackages/1-framework/3-tooling/migration/package.jsonpackages/1-framework/3-tooling/migration/src/attestation.tspackages/1-framework/3-tooling/migration/src/errors.tspackages/1-framework/3-tooling/migration/src/exports/io.tspackages/1-framework/3-tooling/migration/src/exports/migration-ts.tspackages/1-framework/3-tooling/migration/src/exports/types.tspackages/1-framework/3-tooling/migration/src/io.tspackages/1-framework/3-tooling/migration/src/migration-ts.tspackages/1-framework/3-tooling/migration/src/types.tspackages/1-framework/3-tooling/migration/tsdown.config.tspackages/2-sql/4-lanes/sql-builder/package.jsonpackages/2-sql/4-lanes/sql-builder/src/exports/types.tspackages/2-sql/4-lanes/sql-builder/src/runtime/index.tspackages/2-sql/4-lanes/sql-builder/test/fixtures/contract.tspackages/2-sql/9-family/package.jsonpackages/2-sql/9-family/src/core/migrations/contract-to-schema-ir.tspackages/2-sql/9-family/src/core/migrations/descriptor-schemas.tspackages/2-sql/9-family/src/core/migrations/operation-descriptors.tspackages/2-sql/9-family/src/core/migrations/types.tspackages/2-sql/9-family/src/core/schema-verify/verify-helpers.tspackages/2-sql/9-family/src/exports/operation-descriptors.tspackages/2-sql/9-family/test/contract-to-schema-ir.test.tspackages/2-sql/9-family/tsdown.config.tspackages/3-extensions/pgvector/package.jsonpackages/3-extensions/pgvector/src/exports/control.tspackages/3-targets/3-targets/postgres/package.jsonpackages/3-targets/3-targets/postgres/src/core/migrations/descriptor-planner.tspackages/3-targets/3-targets/postgres/src/core/migrations/operation-descriptors.tspackages/3-targets/3-targets/postgres/src/core/migrations/operation-resolver.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner-reconciliation.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner-strategies.tspackages/3-targets/3-targets/postgres/src/core/migrations/runner.tspackages/3-targets/3-targets/postgres/src/exports/control.tspackages/3-targets/3-targets/postgres/src/exports/migration-builders.tspackages/3-targets/3-targets/postgres/test/migrations/descriptor-planner.scenarios.mdpackages/3-targets/3-targets/postgres/test/migrations/descriptor-planner.scenarios.test.tspackages/3-targets/3-targets/postgres/tsdown.config.tspackages/3-targets/6-adapters/postgres/package.jsonpackages/3-targets/6-adapters/postgres/src/core/enum-control-hooks.tspackages/3-targets/6-adapters/postgres/test/enum-control-hooks.basic.test.tstest/integration/test/cli-journeys/data-transform.e2e.test.tstest/integration/test/cli-journeys/migration-apply-edge-cases.e2e.test.tstest/integration/test/cli.migration-apply.e2e.test.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-unique-email.tstest/integration/test/sql-builder/distinct.test.tstest/integration/test/sql-builder/execution.test.tstest/integration/test/sql-builder/extension-functions.test.tstest/integration/test/sql-builder/fixtures/contract.tstest/integration/test/sql-builder/fixtures/prisma-next.config.tstest/integration/test/sql-builder/group-by.test.tstest/integration/test/sql-builder/join.test.tstest/integration/test/sql-builder/mutation-defaults.test.tstest/integration/test/sql-builder/mutation.test.tstest/integration/test/sql-builder/order-by.test.tstest/integration/test/sql-builder/pagination.test.tstest/integration/test/sql-builder/select.test.tstest/integration/test/sql-builder/setup.tstest/integration/test/sql-builder/subquery.test.tstest/integration/test/sql-builder/where.test.tstest/integration/test/utils/journey-test-helpers.tsturbo.json
💤 Files with no reviewable changes (2)
- packages/3-targets/6-adapters/postgres/package.json
- packages/3-extensions/pgvector/package.json
✅ Files skipped from review due to trivial changes (15)
- packages/1-framework/3-tooling/cli/tsdown.config.ts
- packages/2-sql/9-family/tsdown.config.ts
- packages/1-framework/3-tooling/migration/tsdown.config.ts
- packages/2-sql/4-lanes/sql-builder/src/runtime/index.ts
- packages/1-framework/3-tooling/cli/package.json
- packages/2-sql/9-family/test/contract-to-schema-ir.test.ts
- packages/1-framework/3-tooling/migration/src/exports/io.ts
- test/integration/test/cli-journeys/migration-apply-edge-cases.e2e.test.ts
- packages/1-framework/3-tooling/migration/package.json
- test/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-unique-email.ts
- packages/1-framework/3-tooling/migration/src/exports/migration-ts.ts
- packages/2-sql/9-family/src/exports/operation-descriptors.ts
- packages/3-targets/3-targets/postgres/src/exports/migration-builders.ts
- packages/3-targets/3-targets/postgres/test/migrations/descriptor-planner.scenarios.md
- packages/1-framework/3-tooling/cli/src/commands/migration-apply.ts
🚧 Files skipped from review as they are similar to previous changes (29)
- packages/1-framework/3-tooling/cli/src/control-api/operations/migration-apply.ts
- packages/2-sql/9-family/package.json
- turbo.json
- packages/2-sql/9-family/src/core/migrations/contract-to-schema-ir.ts
- packages/1-framework/3-tooling/migration/src/attestation.ts
- packages/1-framework/3-tooling/cli/src/cli.ts
- packages/2-sql/4-lanes/sql-builder/src/exports/types.ts
- packages/3-targets/3-targets/postgres/package.json
- packages/3-extensions/pgvector/src/exports/control.ts
- packages/1-framework/3-tooling/migration/src/errors.ts
- packages/3-targets/6-adapters/postgres/test/enum-control-hooks.basic.test.ts
- packages/1-framework/3-tooling/cli/test/utils/formatters/graph-migration-mapper.test.ts
- test/integration/test/utils/journey-test-helpers.ts
- packages/3-targets/3-targets/postgres/tsdown.config.ts
- test/integration/test/cli.migration-apply.e2e.test.ts
- packages/1-framework/1-core/framework-components/src/exports/control.ts
- test/integration/test/sql-builder/fixtures/contract.ts
- packages/1-framework/3-tooling/cli/src/utils/command-helpers.ts
- packages/1-framework/3-tooling/migration/src/exports/types.ts
- packages/1-framework/3-tooling/cli/src/commands/migration-new.ts
- packages/1-framework/3-tooling/migration/src/types.ts
- packages/1-framework/1-core/framework-components/src/control-migration-types.ts
- test/integration/test/cli-journeys/data-transform.e2e.test.ts
- packages/1-framework/3-tooling/cli/src/utils/formatters/graph-types.ts
- packages/1-framework/3-tooling/cli/src/commands/migration-status.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/operation-resolver.ts
- packages/3-targets/3-targets/postgres/test/migrations/descriptor-planner.scenarios.test.ts
- packages/2-sql/4-lanes/sql-builder/package.json
- packages/1-framework/3-tooling/cli/test/utils/formatters/graph-render.test.ts
saevarb
force-pushed
the
feat/data-migrations
branch
from
0b4515e to
c1ea89b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/3-targets/3-targets/postgres/src/core/migrations/planner-reconciliation.ts (1)
99-107:⚠️ Potential issue | 🟠 MajorDon't classify
enum_values_changedas additive in the reconciliation planner.This makes
buildReconciliationPlan()skip the issue before it can become either an operation or a conflict. Sincedb update/db initstill use this planner, enum drift now disappears from their output instead of being surfaced to the caller. Keep it as a conflict until those commands can actually plan enum changes.🤖 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 99 - 107, The function isAdditiveIssue currently classifies 'enum_values_changed' as additive which causes buildReconciliationPlan() to ignore enum drift; update isAdditiveIssue (the switch inside that function) to remove 'enum_values_changed' from the additive cases so it is no longer returned true there, ensuring enum_values_changed is treated as a conflict by the reconciliation planner (you can optionally add a short comment explaining why).
♻️ Duplicate comments (3)
packages/2-sql/9-family/src/core/migrations/contract-to-schema-ir.ts (1)
265-267:⚠️ Potential issue | 🟠 MajorFail fast on duplicate
nativeTypecollisions when re-keying.At Line 265–267, direct assignment can silently overwrite prior entries if two
storage.typesshare the samenativeType, which can drop metadata and destabilize planning. Please add a collision guard before assignment.Suggested fix
- for (const typeInstance of Object.values(storage.types)) { - byNativeType[typeInstance.nativeType] = typeInstance; - } + for (const [typeId, typeInstance] of Object.entries(storage.types)) { + const nativeType = typeInstance.nativeType; + if (Object.hasOwn(byNativeType, nativeType)) { + throw new Error( + `Duplicate storage.types nativeType "${nativeType}" found (including "${typeId}").`, + ); + } + byNativeType[nativeType] = typeInstance; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/9-family/src/core/migrations/contract-to-schema-ir.ts` around lines 265 - 267, When re-keying storage.types into byNativeType, add a collision guard to fail fast instead of silently overwriting: before assigning byNativeType[typeInstance.nativeType] = typeInstance in the loop over Object.values(storage.types), check if byNativeType already has that nativeType key and, if so, throw an Error (including the duplicated nativeType and both conflicting type identifiers from typeInstance and the existing entry) so the collision is detected and surfaced during migration/planning.packages/1-framework/3-tooling/migration/src/migration-ts.ts (1)
119-125:⚠️ Potential issue | 🟠 MajorPrefix same-directory
Contractimports with./.
relative()can returncontract.dortypes/contract.d, which ESM treats as bare specifiers. In that case the scaffoldedmigration.tsresolves against package imports instead of the local file, so the typed-builder path breaks.💡 Proposed fix
const relativeContractDts = relative(packageDir, options.contractJsonPath).replace( /\.json$/, '.d', ); - lines.push(`import type { Contract } from "${relativeContractDts}"`); + const contractImportPath = relativeContractDts.startsWith('.') + ? relativeContractDts + : `./${relativeContractDts}`; + lines.push(`import type { Contract } from "${contractImportPath}"`);In Node.js ESM / TypeScript, is `import type { Contract } from "contract.d"` treated as a bare module specifier rather than a local relative import, requiring `./contract.d` or `../contract.d` for project files?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/migration/src/migration-ts.ts` around lines 119 - 125, The import path built for Contract can yield a bare specifier like "contract.d" which ESM treats as a package import; update the logic that computes relativeContractDts (used where hasDataTransform && options.contractJsonPath) to ensure it is a relative specifier by prefixing "./" when the computed relativeContractDts does not start with "." or "/", then use that adjusted string in the lines.push call that creates `import type { Contract } from "${relativeContractDts}"` so local same-directory files are imported correctly.packages/3-targets/3-targets/postgres/src/core/migrations/runner.ts (1)
207-216:⚠️ Potential issue | 🟠 MajorDon't count skipped data transforms as executed.
executeDataTransform()returns success for both “ran” and “skipped”, but this branch always records the operation and incrementsoperationsExecuted. That makes no-op transforms show up in the ledger/summary as executed. Return an explicit status fromexecuteDataTransform()and only count therancase.🤖 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/runner.ts` around lines 207 - 216, The caller treats any successful executeDataTransform() result as "ran" and always records the operation; change the caller in the migration runner so it only records and increments when executeDataTransform() explicitly reports it ran. Modify executeDataTransform() to return an explicit status (e.g., { ok: true, status: 'ran' | 'skipped' }) and update the branch around isDataTransformOperation(operation) to push to executedOperations and increment operationsExecuted only when dtResult.status === 'ran' (leave error handling and continue behavior unchanged).
🧹 Nitpick comments (4)
test/integration/test/cli-journeys/data-transform.e2e.test.ts (1)
97-109: Exercise the typed builder path in this journey too.This fixture uses
createBuilders()without aContractgeneric, so it only covers the untyped fallback. Since typed authoring/autocomplete is a headline feature here, importing the generatedContracttype and callingcreateBuilders<Contract>()would keep that path under e2e coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/test/cli-journeys/data-transform.e2e.test.ts` around lines 97 - 109, The test fixture currently exercises the untyped builder path by calling createBuilders() without a generic; update the migrationTs template so it imports the generated Contract type and calls createBuilders<Contract>() to exercise the typed builder path (e.g., add an import for the Contract type and change createBuilders() to createBuilders<Contract>() while leaving existing identifier names like createBuilders, addColumn, and dataTransform unchanged).packages/3-targets/3-targets/postgres/test/migrations/descriptor-planner.scenarios.test.ts (1)
47-55: UseifDefined()consistently for conditional object spreads.The
col()helper uses inline conditional spread patterns while thecontract()helper (line 93) correctly usesifDefined(). Apply the same pattern here for consistency.♻️ Suggested refactor
function col( nativeType: string, codecId: string, opts?: { nullable?: boolean; default?: StorageColumn['default']; typeParams?: Record<string, unknown>; typeRef?: string; }, ): StorageColumn { return { nativeType, codecId, nullable: opts?.nullable ?? false, - ...(opts?.default !== undefined ? { default: opts.default } : {}), - ...(opts?.typeParams !== undefined ? { typeParams: opts.typeParams } : {}), - ...(opts?.typeRef !== undefined ? { typeRef: opts.typeRef } : {}), + ...ifDefined('default', opts?.default), + ...ifDefined('typeParams', opts?.typeParams), + ...ifDefined('typeRef', opts?.typeRef), }; }As per coding guidelines: "Use
ifDefined()from@prisma-next/utils/definedfor conditional object spreads instead of inline conditional spread patterns."🤖 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/descriptor-planner.scenarios.test.ts` around lines 47 - 55, The col() helper currently uses inline conditional spreads for optional properties (nullable, default, typeParams, typeRef); import and use ifDefined from '@prisma-next/utils/defined' and replace these inline spreads with ifDefined(...) calls (matching the pattern used in contract()), e.g. wrap each optional value in ifDefined(value, { key: value }) so the returned object builds conditionally via ifDefined for default, typeParams, typeRef (and nullable if desired) instead of using ...(opts?.foo !== undefined ? { foo: opts.foo } : {}).packages/3-targets/3-targets/postgres/src/core/migrations/planner-strategies.ts (1)
144-181: Consider expandingSAFE_WIDENINGSto include more common safe conversions.The current set covers integer and float widenings. PostgreSQL also supports safe implicit casts like
varchar(n)→textandnumeric(p,s)to wider precision. These could be added later if users report false positives.🤖 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-strategies.ts` around lines 144 - 181, The SAFE_WIDENINGS set in typeChangeStrategy currently only lists numeric widenings; expand its coverage by adding common safe conversions (e.g., varchar(n)→text and numeric precision widenings) and update isSafeWidening to handle pattern-based matches (not just exact string keys) so it can detect varchar(n)→text and numeric(p,s)→numeric(p2,s2) where p2>=p and s2>=s; modify the isSafeWidening helper (and SAFE_WIDENINGS or replace it with a small rule-check function) referenced by typeChangeStrategy to perform those pattern checks before deciding to emit only alterColumnType, leaving the dataTransform fallback unchanged.packages/3-targets/3-targets/postgres/src/core/migrations/descriptor-planner.ts (1)
310-333: Track the codec registry TODO for type handling.The hardcoded
pg/enumprefix check works for now but limits extensibility. The TODO comment is appropriate — consider opening an issue to track implementing a codec-driven registry that dispatches to the correct descriptor factory.Would you like me to open a GitHub issue to track implementing a codec registry for type descriptor dispatch?
🤖 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/descriptor-planner.ts` around lines 310 - 333, Replace the hardcoded pg/enum prefix check in the 'type_missing' case with a codec-driven registry: introduce or use a codec registry (e.g., ctx.codecRegistry) that maps codecId to a descriptor factory, call the registry with typeInstance.codecId to obtain a handler and invoke it (falling back to the existing createEnumType behavior if the registry returns an enum handler), and return an unsupportedOperation issue when no handler exists; also open a GitHub issue to track implementing the codec registry for descriptor dispatch (refer to symbols: 'type_missing' case, 'typeInstance', 'codecId', 'createEnumType', and the descriptor planner).
🤖 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/runner.ts`:
- Around line 285-295: The branch that treats op.check === true as “always skip”
is incorrectly nested under the options.runIdempotency guard, so when
idempotency probes are disabled the sentinel becomes a write; update the logic
in runner.ts (the migration runner where op.check, options.runIdempotency,
driver.query, okVoid(), and the run execution are used) by moving the op.check
=== true handling out of the runIdempotency conditional and executing it before
the idempotency check so that whenever op.check === true you return okVoid()
immediately regardless of options.runIdempotency.
---
Outside diff comments:
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/planner-reconciliation.ts`:
- Around line 99-107: The function isAdditiveIssue currently classifies
'enum_values_changed' as additive which causes buildReconciliationPlan() to
ignore enum drift; update isAdditiveIssue (the switch inside that function) to
remove 'enum_values_changed' from the additive cases so it is no longer returned
true there, ensuring enum_values_changed is treated as a conflict by the
reconciliation planner (you can optionally add a short comment explaining why).
---
Duplicate comments:
In `@packages/1-framework/3-tooling/migration/src/migration-ts.ts`:
- Around line 119-125: The import path built for Contract can yield a bare
specifier like "contract.d" which ESM treats as a package import; update the
logic that computes relativeContractDts (used where hasDataTransform &&
options.contractJsonPath) to ensure it is a relative specifier by prefixing "./"
when the computed relativeContractDts does not start with "." or "/", then use
that adjusted string in the lines.push call that creates `import type { Contract
} from "${relativeContractDts}"` so local same-directory files are imported
correctly.
In `@packages/2-sql/9-family/src/core/migrations/contract-to-schema-ir.ts`:
- Around line 265-267: When re-keying storage.types into byNativeType, add a
collision guard to fail fast instead of silently overwriting: before assigning
byNativeType[typeInstance.nativeType] = typeInstance in the loop over
Object.values(storage.types), check if byNativeType already has that nativeType
key and, if so, throw an Error (including the duplicated nativeType and both
conflicting type identifiers from typeInstance and the existing entry) so the
collision is detected and surfaced during migration/planning.
In `@packages/3-targets/3-targets/postgres/src/core/migrations/runner.ts`:
- Around line 207-216: The caller treats any successful executeDataTransform()
result as "ran" and always records the operation; change the caller in the
migration runner so it only records and increments when executeDataTransform()
explicitly reports it ran. Modify executeDataTransform() to return an explicit
status (e.g., { ok: true, status: 'ran' | 'skipped' }) and update the branch
around isDataTransformOperation(operation) to push to executedOperations and
increment operationsExecuted only when dtResult.status === 'ran' (leave error
handling and continue behavior unchanged).
---
Nitpick comments:
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/descriptor-planner.ts`:
- Around line 310-333: Replace the hardcoded pg/enum prefix check in the
'type_missing' case with a codec-driven registry: introduce or use a codec
registry (e.g., ctx.codecRegistry) that maps codecId to a descriptor factory,
call the registry with typeInstance.codecId to obtain a handler and invoke it
(falling back to the existing createEnumType behavior if the registry returns an
enum handler), and return an unsupportedOperation issue when no handler exists;
also open a GitHub issue to track implementing the codec registry for descriptor
dispatch (refer to symbols: 'type_missing' case, 'typeInstance', 'codecId',
'createEnumType', and the descriptor planner).
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/planner-strategies.ts`:
- Around line 144-181: The SAFE_WIDENINGS set in typeChangeStrategy currently
only lists numeric widenings; expand its coverage by adding common safe
conversions (e.g., varchar(n)→text and numeric precision widenings) and update
isSafeWidening to handle pattern-based matches (not just exact string keys) so
it can detect varchar(n)→text and numeric(p,s)→numeric(p2,s2) where p2>=p and
s2>=s; modify the isSafeWidening helper (and SAFE_WIDENINGS or replace it with a
small rule-check function) referenced by typeChangeStrategy to perform those
pattern checks before deciding to emit only alterColumnType, leaving the
dataTransform fallback unchanged.
In
`@packages/3-targets/3-targets/postgres/test/migrations/descriptor-planner.scenarios.test.ts`:
- Around line 47-55: The col() helper currently uses inline conditional spreads
for optional properties (nullable, default, typeParams, typeRef); import and use
ifDefined from '@prisma-next/utils/defined' and replace these inline spreads
with ifDefined(...) calls (matching the pattern used in contract()), e.g. wrap
each optional value in ifDefined(value, { key: value }) so the returned object
builds conditionally via ifDefined for default, typeParams, typeRef (and
nullable if desired) instead of using ...(opts?.foo !== undefined ? { foo:
opts.foo } : {}).
In `@test/integration/test/cli-journeys/data-transform.e2e.test.ts`:
- Around line 97-109: The test fixture currently exercises the untyped builder
path by calling createBuilders() without a generic; update the migrationTs
template so it imports the generated Contract type and calls
createBuilders<Contract>() to exercise the typed builder path (e.g., add an
import for the Contract type and change createBuilders() to
createBuilders<Contract>() while leaving existing identifier names like
createBuilders, addColumn, and dataTransform unchanged).
🪄 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: 3a70cb5c-aad7-467c-9124-903dde320e72
⛔ Files ignored due to path filters (13)
packages/1-framework/3-tooling/cli/test/utils/formatters/__snapshots__/graph-render.test.ts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yamlprojects/graph-based-migrations/plans/data-migrations-plan.mdis excluded by!projects/**projects/graph-based-migrations/specs/april-milestone.mdis excluded by!projects/**projects/graph-based-migrations/specs/chat.mdis excluded by!projects/**projects/graph-based-migrations/specs/data-migration-scenarios.mdis excluded by!projects/**projects/graph-based-migrations/specs/data-migrations-solutions.mdis excluded by!projects/**projects/graph-based-migrations/specs/data-migrations-spec.mdis excluded by!projects/**projects/graph-based-migrations/specs/data-migrations.mdis excluded by!projects/**projects/graph-based-migrations/specs/planner-replacement-proposal.mdis excluded by!projects/**projects/graph-based-migrations/walkthrough.mdis excluded by!projects/**test/integration/test/sql-builder/fixtures/generated/contract.d.tsis excluded by!**/generated/**test/integration/test/sql-builder/fixtures/generated/contract.jsonis excluded by!**/generated/**
📒 Files selected for processing (80)
packages/1-framework/1-core/framework-components/src/control-migration-types.tspackages/1-framework/1-core/framework-components/src/control-result-types.tspackages/1-framework/1-core/framework-components/src/exports/control.tspackages/1-framework/3-tooling/cli/package.jsonpackages/1-framework/3-tooling/cli/src/cli.tspackages/1-framework/3-tooling/cli/src/commands/migration-apply.tspackages/1-framework/3-tooling/cli/src/commands/migration-new.tspackages/1-framework/3-tooling/cli/src/commands/migration-plan.tspackages/1-framework/3-tooling/cli/src/commands/migration-status.tspackages/1-framework/3-tooling/cli/src/commands/migration-verify.tspackages/1-framework/3-tooling/cli/src/control-api/operations/migration-apply.tspackages/1-framework/3-tooling/cli/src/utils/command-helpers.tspackages/1-framework/3-tooling/cli/src/utils/formatters/graph-migration-mapper.tspackages/1-framework/3-tooling/cli/src/utils/formatters/graph-render.tspackages/1-framework/3-tooling/cli/src/utils/formatters/graph-types.tspackages/1-framework/3-tooling/cli/test/utils/formatters/graph-migration-mapper.test.tspackages/1-framework/3-tooling/cli/test/utils/formatters/graph-render.test.tspackages/1-framework/3-tooling/cli/test/utils/formatters/test-graphs.tspackages/1-framework/3-tooling/cli/tsdown.config.tspackages/1-framework/3-tooling/migration/package.jsonpackages/1-framework/3-tooling/migration/src/attestation.tspackages/1-framework/3-tooling/migration/src/errors.tspackages/1-framework/3-tooling/migration/src/exports/io.tspackages/1-framework/3-tooling/migration/src/exports/migration-ts.tspackages/1-framework/3-tooling/migration/src/exports/types.tspackages/1-framework/3-tooling/migration/src/io.tspackages/1-framework/3-tooling/migration/src/migration-ts.tspackages/1-framework/3-tooling/migration/src/types.tspackages/1-framework/3-tooling/migration/tsdown.config.tspackages/2-sql/4-lanes/sql-builder/package.jsonpackages/2-sql/4-lanes/sql-builder/src/exports/types.tspackages/2-sql/4-lanes/sql-builder/src/runtime/index.tspackages/2-sql/4-lanes/sql-builder/test/fixtures/contract.tspackages/2-sql/9-family/package.jsonpackages/2-sql/9-family/src/core/migrations/contract-to-schema-ir.tspackages/2-sql/9-family/src/core/migrations/descriptor-schemas.tspackages/2-sql/9-family/src/core/migrations/operation-descriptors.tspackages/2-sql/9-family/src/core/migrations/types.tspackages/2-sql/9-family/src/core/schema-verify/verify-helpers.tspackages/2-sql/9-family/src/exports/operation-descriptors.tspackages/2-sql/9-family/test/contract-to-schema-ir.test.tspackages/2-sql/9-family/tsdown.config.tspackages/3-extensions/pgvector/package.jsonpackages/3-extensions/pgvector/src/exports/control.tspackages/3-targets/3-targets/postgres/package.jsonpackages/3-targets/3-targets/postgres/src/core/migrations/descriptor-planner.tspackages/3-targets/3-targets/postgres/src/core/migrations/operation-descriptors.tspackages/3-targets/3-targets/postgres/src/core/migrations/operation-resolver.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner-reconciliation.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner-strategies.tspackages/3-targets/3-targets/postgres/src/core/migrations/runner.tspackages/3-targets/3-targets/postgres/src/exports/control.tspackages/3-targets/3-targets/postgres/src/exports/migration-builders.tspackages/3-targets/3-targets/postgres/test/migrations/descriptor-planner.scenarios.mdpackages/3-targets/3-targets/postgres/test/migrations/descriptor-planner.scenarios.test.tspackages/3-targets/3-targets/postgres/tsdown.config.tspackages/3-targets/6-adapters/postgres/package.jsonpackages/3-targets/6-adapters/postgres/src/core/enum-control-hooks.tspackages/3-targets/6-adapters/postgres/test/enum-control-hooks.basic.test.tstest/integration/test/cli-journeys/data-transform.e2e.test.tstest/integration/test/cli-journeys/migration-apply-edge-cases.e2e.test.tstest/integration/test/cli.migration-apply.e2e.test.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-unique-email.tstest/integration/test/sql-builder/distinct.test.tstest/integration/test/sql-builder/execution.test.tstest/integration/test/sql-builder/extension-functions.test.tstest/integration/test/sql-builder/fixtures/contract.tstest/integration/test/sql-builder/fixtures/prisma-next.config.tstest/integration/test/sql-builder/group-by.test.tstest/integration/test/sql-builder/join.test.tstest/integration/test/sql-builder/mutation-defaults.test.tstest/integration/test/sql-builder/mutation.test.tstest/integration/test/sql-builder/order-by.test.tstest/integration/test/sql-builder/pagination.test.tstest/integration/test/sql-builder/select.test.tstest/integration/test/sql-builder/setup.tstest/integration/test/sql-builder/subquery.test.tstest/integration/test/sql-builder/where.test.tstest/integration/test/utils/journey-test-helpers.tsturbo.json
💤 Files with no reviewable changes (2)
- packages/3-extensions/pgvector/package.json
- packages/3-targets/6-adapters/postgres/package.json
✅ Files skipped from review due to trivial changes (20)
- packages/1-framework/3-tooling/cli/tsdown.config.ts
- test/integration/test/sql-builder/setup.ts
- packages/2-sql/9-family/tsdown.config.ts
- packages/1-framework/3-tooling/migration/tsdown.config.ts
- packages/3-targets/6-adapters/postgres/test/enum-control-hooks.basic.test.ts
- turbo.json
- packages/2-sql/4-lanes/sql-builder/src/exports/types.ts
- packages/1-framework/3-tooling/migration/src/attestation.ts
- packages/1-framework/3-tooling/migration/src/exports/migration-ts.ts
- packages/1-framework/3-tooling/migration/src/errors.ts
- packages/2-sql/4-lanes/sql-builder/src/runtime/index.ts
- packages/1-framework/3-tooling/cli/package.json
- packages/1-framework/3-tooling/migration/package.json
- test/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-unique-email.ts
- test/integration/test/cli-journeys/migration-apply-edge-cases.e2e.test.ts
- packages/1-framework/1-core/framework-components/src/exports/control.ts
- packages/1-framework/3-tooling/migration/src/exports/types.ts
- packages/3-targets/3-targets/postgres/test/migrations/descriptor-planner.scenarios.md
- packages/3-targets/3-targets/postgres/src/exports/migration-builders.ts
- packages/2-sql/9-family/src/exports/operation-descriptors.ts
🚧 Files skipped from review as they are similar to previous changes (27)
- packages/2-sql/9-family/package.json
- packages/1-framework/3-tooling/cli/src/utils/formatters/graph-types.ts
- packages/1-framework/3-tooling/cli/src/control-api/operations/migration-apply.ts
- packages/3-targets/3-targets/postgres/package.json
- packages/1-framework/3-tooling/migration/src/exports/io.ts
- packages/2-sql/9-family/test/contract-to-schema-ir.test.ts
- packages/1-framework/3-tooling/cli/test/utils/formatters/graph-render.test.ts
- packages/3-targets/3-targets/postgres/tsdown.config.ts
- packages/2-sql/4-lanes/sql-builder/package.json
- packages/1-framework/3-tooling/cli/test/utils/formatters/test-graphs.ts
- test/integration/test/utils/journey-test-helpers.ts
- packages/1-framework/3-tooling/cli/src/commands/migration-apply.ts
- packages/1-framework/3-tooling/cli/src/utils/command-helpers.ts
- packages/1-framework/3-tooling/cli/src/utils/formatters/graph-render.ts
- packages/1-framework/3-tooling/cli/test/utils/formatters/graph-migration-mapper.test.ts
- packages/3-extensions/pgvector/src/exports/control.ts
- packages/1-framework/3-tooling/cli/src/commands/migration-verify.ts
- packages/1-framework/1-core/framework-components/src/control-result-types.ts
- packages/1-framework/3-tooling/cli/src/commands/migration-plan.ts
- packages/1-framework/3-tooling/migration/src/types.ts
- packages/1-framework/3-tooling/cli/src/commands/migration-new.ts
- packages/1-framework/3-tooling/cli/src/commands/migration-status.ts
- test/integration/test/cli.migration-apply.e2e.test.ts
- packages/2-sql/9-family/src/core/migrations/descriptor-schemas.ts
- packages/1-framework/3-tooling/migration/src/io.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/operation-resolver.ts
- packages/2-sql/9-family/src/core/migrations/operation-descriptors.ts
…clean up dependencies Remove phantom @prisma-next/cli dependencies from target-postgres, adapter-postgres, extension-pgvector, family-sql. Move sql-builder integration tests to integration test package to break dependency cycle. Restore unit test fixtures with simplified contract.ts re-export. Export Db type from sql-builder runtime for cross-package type portability.
saevarb
force-pushed
the
feat/data-migrations
branch
2 times, most recently
from
5dd6c1f to
eaad4fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
packages/2-sql/9-family/src/core/migrations/contract-to-schema-ir.ts (1)
263-268:⚠️ Potential issue | 🟠 MajorFail fast on
nativeTypecollisions during re-keying.Line 266 currently overwrites existing entries when multiple
storage.typesshare the samenativeType, which can drop metadata silently.Suggested fix
function deriveAnnotations( storage: SqlStorage, annotationNamespace: string, ): SqlAnnotations | undefined { if (!storage.types || Object.keys(storage.types).length === 0) return undefined; // Re-key by nativeType to match the structure produced by introspection const byNativeType: Record<string, (typeof storage.types)[string]> = {}; - for (const typeInstance of Object.values(storage.types)) { - byNativeType[typeInstance.nativeType] = typeInstance; + for (const [typeName, typeInstance] of Object.entries(storage.types)) { + if (Object.hasOwn(byNativeType, typeInstance.nativeType)) { + throw new Error( + `Duplicate storage.types nativeType "${typeInstance.nativeType}" found (including "${typeName}").`, + ); + } + byNativeType[typeInstance.nativeType] = typeInstance; } return { [annotationNamespace]: { storageTypes: byNativeType } }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/9-family/src/core/migrations/contract-to-schema-ir.ts` around lines 263 - 268, Re-keying storage.types into byNativeType currently silently overwrites entries when multiple typeInstance.nativeType values collide; modify the loop that builds byNativeType so it detects an existing key for a nativeType (inspect byNativeType[typeInstance.nativeType]) and throws a clear error (including the conflicting nativeType and identifiers from the existing and incoming typeInstances) instead of overwriting; update the code around byNativeType, storage.types and typeInstance.nativeType in contract-to-schema-ir.ts to fail fast on duplicates so metadata loss is prevented.packages/1-framework/3-tooling/cli/src/commands/migration-verify.ts (1)
114-123:⚠️ Potential issue | 🟠 MajorRe-attestation applies to all mismatch cases, including packages without
migration.ts.When
migration.tsdoesn't exist, lines 58-86 are skipped, soops.jsonis unchanged. If verification then fails due to a mismatch (e.g., someone manually editedops.jsonon an attested migration), this code path will silently re-attest rather than failing with an integrity error.Consider guarding re-attestation to only apply when
migration.tswas present andops.jsonwas rewritten:🛡️ Proposed fix
+ const hadMigrationTs = await hasMigrationTs(dir); + // If migration.ts exists, always evaluate and resolve to ops.json. - if (await hasMigrationTs(dir)) { + if (hadMigrationTs) { // ... existing evaluation logic ... } // Now verify/attest with the (potentially updated) ops.json const result = await verifyMigration(dir); // ... existing ok and draft handling ... - // Mismatch — ops.json was just rewritten by migration.ts evaluation above, - // so this means the stored migrationId is stale. Re-attest. - const migrationId = await attestMigration(dir); - return ok({ - ok: true, - status: 'attested', - dir, - migrationId, - summary: `Migration re-attested with migrationId: ${migrationId}`, - }); + // Mismatch handling + if (hadMigrationTs) { + // ops.json was just rewritten, so the stored migrationId is stale. Re-attest. + const migrationId = await attestMigration(dir); + return ok({ + ok: true, + status: 'attested', + dir, + migrationId, + summary: `Migration re-attested with migrationId: ${migrationId}`, + }); + } + + // No migration.ts but mismatch — integrity violation + return notOk( + errorRuntime('Migration integrity check failed', { + why: `Stored migrationId "${result.storedMigrationId}" does not match computed "${result.computedMigrationId}"`, + fix: 'The ops.json or migration.json may have been modified after attestation. Restore from version control or re-plan the migration.', + }), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/cli/src/commands/migration-verify.ts` around lines 114 - 123, The current re-attestation always calls attestMigration(dir) on any ops.json mismatch; change this so re-attestation only happens when migration.ts was present and ops.json was actually rewritten. Add or propagate a boolean (e.g., migrationScriptPresent or opsWasRewritten) from the migration evaluation earlier (the logic that runs migration.ts and may rewrite ops.json) and, in migration-verify.ts where the mismatch branch currently calls attestMigration(dir), guard that call with that boolean; if the boolean is false, return a failed integrity result instead of re-attesting to avoid silently accepting manual edits to ops.json.
🧹 Nitpick comments (3)
packages/1-framework/3-tooling/cli/src/commands/migration-new.ts (1)
90-102: Fragile type casting for storageHash extraction.The nested casting through
unknownandRecord<string, unknown>is brittle. Consider using a schema validator or a safer extraction pattern.♻️ Proposed safer extraction
- const toStorageHash = ( - (toContractJson as unknown as Record<string, unknown>)['storage'] as - | Record<string, unknown> - | undefined - )?.['storageHash'] as string | undefined; + const storage = (toContractJson as { storage?: { storageHash?: string } }).storage; + const toStorageHash = storage?.storageHash;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/cli/src/commands/migration-new.ts` around lines 90 - 102, The current nested unsafe casts when extracting toStorageHash from toContractJson are brittle; replace the blind casts with a safe runtime check or schema validation: validate that toContractJson is an object, that it has a "storage" object, and that storage.storageHash is a string before assigning to toStorageHash (or use a lightweight validator like zod/Yup to parse toContractJson into an interface). Update the extraction logic referenced by toStorageHash and toContractJson to perform typeof checks and optional chaining, and retain the existing error path (notOk(errorRuntime(...))) using contractPathAbsolute when storageHash is missing so behavior is unchanged but robust to malformed input.packages/1-framework/3-tooling/cli/src/commands/migration-plan.ts (2)
318-322: Operation class hardcoded to 'data' may misrepresent actual operation types.When
needsDataMigrationis true, at least one descriptor haskind === 'dataTransform', but other descriptors could be DDL operations (additive, destructive, etc.). Hardcoding all to'data'misrepresents the migration to users.Consider deriving
operationClassfrom each descriptor's actual kind, or marking this as a draft placeholder until resolution.♻️ Suggested approach
operations: descriptorResult.descriptors.map((d) => ({ id: (d as { kind: string }).kind, label: (d as { kind: string }).kind, - operationClass: 'data' as const, + operationClass: (d as { kind: string }).kind === 'dataTransform' ? 'data' : 'schema', })),Or, if the descriptor type has a more precise
operationClassfield, use it directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/cli/src/commands/migration-plan.ts` around lines 318 - 322, The operationClass is currently hardcoded to 'data' in the mapping inside migration-plan.ts (the descriptorResult.descriptors.map callback), which misrepresents descriptors whose kind is DDL; update the mapper to derive operationClass per-descriptor by checking the descriptor's kind (e.g., map 'dataTransform' -> 'data', DDL kinds -> 'schema' or their appropriate class) or prefer an existing descriptor.operationClass property if present, and provide a sensible default fallback; ensure the mapping uses the descriptor object (not just casting to { kind: string }) so you preserve any existing operationClass field and maintain type-safety.
337-344: Type cast bypasses validation of evaluated descriptors.
evaluatedDescriptors as OperationDescriptor[]assumes the evaluated migration.ts produces valid descriptors. IfevaluateMigrationTsreturns an untyped or loosely-typed result, consider adding runtime validation before passing toresolveDescriptors.Per coding guidelines, blind casts should be avoided. If
evaluateMigrationTscannot return a properly typed result, consider validating with an arktype schema before use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/cli/src/commands/migration-plan.ts` around lines 337 - 344, The code is blindly casting evaluatedDescriptors to OperationDescriptor[] before calling migrations.resolveDescriptors; instead validate the runtime shape returned by evaluateMigrationTs with an arktype (or other runtime schema) that matches OperationDescriptor and only pass the validated/parsed result into migrations.resolveDescriptors. Locate evaluateMigrationTs and the call site using evaluatedDescriptors and replace the direct cast with a runtime decode/check step that throws or returns a helpful error if validation fails, ensuring resolveDescriptors always receives a properly typed OperationDescriptor[].
🤖 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/1-framework/3-tooling/cli/src/commands/migration-plan.ts`:
- Around line 333-335: Replace the thrown generic Error with a
CliStructuredError so performAction can catch and handle it: where the code
checks migrations.resolveDescriptors and currently does `throw new Error('Target
does not support resolveDescriptors')`, construct and throw a CliStructuredError
(including a short message like "Target does not support resolveDescriptors" and
relevant context fields such as { target: /* identifier from scope */ } or a
code field) so the error is structured and consumable by performAction and
downstream error handlers.
In `@packages/1-framework/3-tooling/cli/src/commands/migration-verify.ts`:
- Around line 66-71: Replace the plain Error thrown when resolveDescriptors is
missing with a CliStructuredError: in migration-verify.ts where the code checks
migrations?.resolveDescriptors, import CliStructuredError (if not already
imported) and throw new CliStructuredError({ message: 'Target does not support
resolveDescriptors. Cannot verify a migration package with migration.ts.', hint:
'Use a target that implements resolveDescriptors or update the migration
package.', metadata: { missingFeature: 'resolveDescriptors' } }) so the CLI
error handling receives a structured, user-facing error.
In
`@packages/1-framework/3-tooling/cli/test/utils/formatters/graph-migration-mapper.test.ts`:
- Around line 221-223: The test currently only checks that a dashed edge exists;
update the assertion to also verify the dashed edge's target equals
sid('OFF_GRAPH') so the OFF_GRAPH linkage is enforced—locate the dashedEdge from
result.graph.edges (as in the dashedEdge variable) and add an assertion that
dashedEdge.target === sid('OFF_GRAPH') (or uses the test's sid helper) to fail
if the off-graph link regresses.
In `@packages/3-targets/3-targets/postgres/package.json`:
- Line 49: The package.json currently exports the migration builders under the
top-level subpath "./migration-builders" which violates the multi-plane
contract; update that export key to the control-plane subpath "./control"
(replace "./migration-builders" with "./control" while keeping the value
"./dist/migration-builders.mjs") and ensure the change aligns with the mapping
in architecture.config.json so the migration plane is exported via "./control".
---
Duplicate comments:
In `@packages/1-framework/3-tooling/cli/src/commands/migration-verify.ts`:
- Around line 114-123: The current re-attestation always calls
attestMigration(dir) on any ops.json mismatch; change this so re-attestation
only happens when migration.ts was present and ops.json was actually rewritten.
Add or propagate a boolean (e.g., migrationScriptPresent or opsWasRewritten)
from the migration evaluation earlier (the logic that runs migration.ts and may
rewrite ops.json) and, in migration-verify.ts where the mismatch branch
currently calls attestMigration(dir), guard that call with that boolean; if the
boolean is false, return a failed integrity result instead of re-attesting to
avoid silently accepting manual edits to ops.json.
In `@packages/2-sql/9-family/src/core/migrations/contract-to-schema-ir.ts`:
- Around line 263-268: Re-keying storage.types into byNativeType currently
silently overwrites entries when multiple typeInstance.nativeType values
collide; modify the loop that builds byNativeType so it detects an existing key
for a nativeType (inspect byNativeType[typeInstance.nativeType]) and throws a
clear error (including the conflicting nativeType and identifiers from the
existing and incoming typeInstances) instead of overwriting; update the code
around byNativeType, storage.types and typeInstance.nativeType in
contract-to-schema-ir.ts to fail fast on duplicates so metadata loss is
prevented.
---
Nitpick comments:
In `@packages/1-framework/3-tooling/cli/src/commands/migration-new.ts`:
- Around line 90-102: The current nested unsafe casts when extracting
toStorageHash from toContractJson are brittle; replace the blind casts with a
safe runtime check or schema validation: validate that toContractJson is an
object, that it has a "storage" object, and that storage.storageHash is a string
before assigning to toStorageHash (or use a lightweight validator like zod/Yup
to parse toContractJson into an interface). Update the extraction logic
referenced by toStorageHash and toContractJson to perform typeof checks and
optional chaining, and retain the existing error path (notOk(errorRuntime(...)))
using contractPathAbsolute when storageHash is missing so behavior is unchanged
but robust to malformed input.
In `@packages/1-framework/3-tooling/cli/src/commands/migration-plan.ts`:
- Around line 318-322: The operationClass is currently hardcoded to 'data' in
the mapping inside migration-plan.ts (the descriptorResult.descriptors.map
callback), which misrepresents descriptors whose kind is DDL; update the mapper
to derive operationClass per-descriptor by checking the descriptor's kind (e.g.,
map 'dataTransform' -> 'data', DDL kinds -> 'schema' or their appropriate class)
or prefer an existing descriptor.operationClass property if present, and provide
a sensible default fallback; ensure the mapping uses the descriptor object (not
just casting to { kind: string }) so you preserve any existing operationClass
field and maintain type-safety.
- Around line 337-344: The code is blindly casting evaluatedDescriptors to
OperationDescriptor[] before calling migrations.resolveDescriptors; instead
validate the runtime shape returned by evaluateMigrationTs with an arktype (or
other runtime schema) that matches OperationDescriptor and only pass the
validated/parsed result into migrations.resolveDescriptors. Locate
evaluateMigrationTs and the call site using evaluatedDescriptors and replace the
direct cast with a runtime decode/check step that throws or returns a helpful
error if validation fails, ensuring resolveDescriptors always receives a
properly typed OperationDescriptor[].
🪄 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: 612a7f93-0929-4f3d-ab43-266015eccd63
⛔ Files ignored due to path filters (1)
packages/1-framework/3-tooling/cli/test/utils/formatters/__snapshots__/graph-render.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (46)
coverage.config.jsonpackages/1-framework/1-core/framework-components/src/control-migration-types.tspackages/1-framework/1-core/framework-components/src/control-result-types.tspackages/1-framework/1-core/framework-components/src/exports/control.tspackages/1-framework/3-tooling/cli/package.jsonpackages/1-framework/3-tooling/cli/src/cli.tspackages/1-framework/3-tooling/cli/src/commands/migration-apply.tspackages/1-framework/3-tooling/cli/src/commands/migration-new.tspackages/1-framework/3-tooling/cli/src/commands/migration-plan.tspackages/1-framework/3-tooling/cli/src/commands/migration-status.tspackages/1-framework/3-tooling/cli/src/commands/migration-verify.tspackages/1-framework/3-tooling/cli/src/control-api/operations/migration-apply.tspackages/1-framework/3-tooling/cli/src/utils/command-helpers.tspackages/1-framework/3-tooling/cli/src/utils/formatters/graph-migration-mapper.tspackages/1-framework/3-tooling/cli/src/utils/formatters/graph-render.tspackages/1-framework/3-tooling/cli/src/utils/formatters/graph-types.tspackages/1-framework/3-tooling/cli/test/utils/formatters/graph-migration-mapper.test.tspackages/1-framework/3-tooling/cli/test/utils/formatters/graph-render.test.tspackages/1-framework/3-tooling/cli/test/utils/formatters/test-graphs.tspackages/1-framework/3-tooling/cli/tsdown.config.tspackages/1-framework/3-tooling/migration/package.jsonpackages/1-framework/3-tooling/migration/src/attestation.tspackages/1-framework/3-tooling/migration/src/errors.tspackages/1-framework/3-tooling/migration/src/exports/io.tspackages/1-framework/3-tooling/migration/src/exports/migration-ts.tspackages/1-framework/3-tooling/migration/src/exports/types.tspackages/1-framework/3-tooling/migration/src/io.tspackages/1-framework/3-tooling/migration/src/migration-ts.tspackages/1-framework/3-tooling/migration/src/types.tspackages/1-framework/3-tooling/migration/tsdown.config.tspackages/2-sql/4-lanes/sql-builder/package.jsonpackages/2-sql/4-lanes/sql-builder/src/exports/types.tspackages/2-sql/4-lanes/sql-builder/src/runtime/index.tspackages/2-sql/4-lanes/sql-builder/test/fixtures/contract.tspackages/2-sql/9-family/package.jsonpackages/2-sql/9-family/src/core/migrations/contract-to-schema-ir.tspackages/2-sql/9-family/src/core/migrations/descriptor-schemas.tspackages/2-sql/9-family/src/core/migrations/operation-descriptors.tspackages/2-sql/9-family/src/core/migrations/types.tspackages/2-sql/9-family/src/core/schema-verify/verify-helpers.tspackages/2-sql/9-family/src/exports/operation-descriptors.tspackages/2-sql/9-family/test/contract-to-schema-ir.test.tspackages/2-sql/9-family/tsdown.config.tspackages/3-extensions/pgvector/package.jsonpackages/3-extensions/pgvector/src/exports/control.tspackages/3-targets/3-targets/postgres/package.json
💤 Files with no reviewable changes (1)
- packages/3-extensions/pgvector/package.json
✅ Files skipped from review due to trivial changes (14)
- packages/1-framework/3-tooling/migration/tsdown.config.ts
- coverage.config.json
- packages/2-sql/9-family/tsdown.config.ts
- packages/1-framework/3-tooling/cli/tsdown.config.ts
- packages/2-sql/4-lanes/sql-builder/src/exports/types.ts
- packages/2-sql/4-lanes/sql-builder/src/runtime/index.ts
- packages/2-sql/9-family/src/core/migrations/descriptor-schemas.ts
- packages/1-framework/3-tooling/cli/src/utils/formatters/graph-types.ts
- packages/1-framework/3-tooling/cli/src/cli.ts
- packages/1-framework/3-tooling/migration/src/exports/migration-ts.ts
- packages/1-framework/3-tooling/migration/src/exports/io.ts
- packages/1-framework/3-tooling/cli/test/utils/formatters/test-graphs.ts
- packages/1-framework/1-core/framework-components/src/exports/control.ts
- packages/2-sql/9-family/src/exports/operation-descriptors.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/1-framework/3-tooling/cli/package.json
- packages/1-framework/3-tooling/migration/src/attestation.ts
- packages/2-sql/9-family/package.json
- packages/1-framework/3-tooling/migration/src/errors.ts
- packages/2-sql/9-family/src/core/migrations/types.ts
- packages/1-framework/3-tooling/cli/test/utils/formatters/graph-render.test.ts
- packages/1-framework/3-tooling/cli/src/utils/command-helpers.ts
- packages/1-framework/1-core/framework-components/src/control-result-types.ts
- packages/1-framework/3-tooling/cli/src/utils/formatters/graph-render.ts
- packages/1-framework/3-tooling/migration/src/io.ts
- packages/1-framework/3-tooling/cli/src/commands/migration-status.ts
- packages/1-framework/3-tooling/migration/src/migration-ts.ts
- packages/1-framework/3-tooling/cli/src/utils/formatters/graph-migration-mapper.ts
- packages/1-framework/1-core/framework-components/src/control-migration-types.ts
- packages/2-sql/9-family/src/core/migrations/operation-descriptors.ts
saevarb
force-pushed
the
feat/data-migrations
branch
2 times, most recently
from
53e79ad to
65ac6c5
Compare
Descriptor types defined as arktype schemas (descriptor-schemas.ts) in sql-family — TypeScript types derived via typeof schema.infer. Operation resolver, migration.ts scaffolding/evaluation, migration verify wiring, runner data transform lifecycle, migration new command, shared IO utilities. Framework types: DataTransformOperation, SerializedQueryPlan, OperationDescriptor, EnumValuesChangedIssue discriminated union. contractToSchemaIR re-keys storage types by nativeType for correct verifier hook resolution. Enum control hooks produce EnumValuesChangedIssue.
planDescriptors with pluggable strategies: notNullBackfill, typeChange, nullableTightening, enumChange. Enum change strategy uses rebuild recipe (createEnumType + alterColumnType + dropEnumType + renameType) with dataTransform for removals. 48 scenario tests with 2 known failures (.fails) as gap map. Wired into migration plan replacing old planner fallback.
loadAllBundles returns MigrationBundleSet with attested and drafts. Status shows MIGRATION.DRAFTS diagnostic, apply warns/errors on drafts, plan guards against duplicate draft targets. Graph rendering uses DIR.dashed bitmask for draft edges, replacing detached node rendering.
queryOperations on SqlControlStaticContributions for extension function support. createBuilders<Contract>() for typed dataTransform callbacks. SQL lowered at verify time via postgres adapter. Journey test fixes: migration-new reads storage.storageHash correctly, resume-after-failure tests use unique constraint violation scenario.
Adds targeted integration tests for `operation-resolver.ts` — a file that was previously uncovered because the legacy planner pipeline bypasses it. Six tests exercise the branches with real logic: dataTransform check/run lowering, the `check: true` skip sentinel, TODO-sentinel error guard, `addColumn(overrides.nullable=true)` for the NOT NULL backfill pattern, and `alterColumnType` with `using` / `toType` overrides. The remaining per-kind glue coverage is deferred via a `warningOnly` entry in `coverage.config.json` (90-day expiry) — these cases wire shared helpers that already have 100% unit coverage, so per-kind tests would be tautological. Coverage will recover naturally when the legacy planner is deprecated and its integration tests migrate to the descriptor pipeline. Package coverage improved from 73.75% to 77.53% statements (and 73.91% to 82.06% functions). CI coverage script now reports 0 failures, 1 warning.
saevarb
force-pushed
the
feat/data-migrations
branch
from
65ac6c5 to
1a456ce
Compare
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
3 participants
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.
closes TML-2173
Intent
Introduce data migration support into the graph-based migration system. Schema changes involving data transformations (NOT NULL backfills, type conversions, enum value changes) are authored as TypeScript operation chains using a new descriptor-based planner, serialized to SQL at verify time, and executed at apply time with retry safety.
Note: the old reconciliation planner is still used by
db updateanddb init. Those two commands are unchanged. The new descriptor planner is used exclusively bymigration plan. The old planner will be removed in a follow-up once dev-push strategies are implemented.What this enables
migration plandetects data migration needs via pluggable strategies and scaffoldsmigration.tswith TODO placeholdersmigration newcreates empty migrations for manual authoringmigration verifyevaluates TypeScript, validates with arktype schemas, resolves to SQL, attestsmigration applyexecutes lowered SQL with check → run → check retry safetymigration statusshows draft migrations as dashed graph edgescreateBuilders<Contract>()provides full autocomplete on tables, columns, and extension functionsChange map
Implementation:
migration newcommandTests:
.failsas gap map)The story
Move sql-builder integration tests to break a dependency cycle. This enables
target-postgresto depend onsql-builderfor the typed query builder needed by data transform callbacks. Phantom CLI dependencies are cleaned up from 4 packages.Introduce descriptor types as arktype schemas at the sql-family level. 21 descriptor types (createTable, addColumn, alterColumnType, addEnumValues, dropEnumType, renameType, etc.) defined as arktype schemas in
descriptor-schemas.ts— TypeScript types derived viatypeof schema.infer. Shared across SQL targets. User-authored migration.ts output is validated at theresolveDescriptorsboundary.Build the operation resolver and migration.ts round-trip. The resolver converts thin descriptors into
SqlMigrationPlanOperationobjects with SQL, prechecks, and postchecks.scaffoldMigrationTs()serializes descriptors as TypeScript builder calls.evaluateMigrationTs()loads migration.ts via Node's native TypeScript import and returns the descriptor array.Build the descriptor-based planner with pluggable strategies. Four migration strategies detect data migration needs:
notNullBackfillStrategy(addColumn nullable + dataTransform + setNotNull),typeChangeStrategy(safe widenings → alterColumnType, unsafe → dataTransform + alterColumnType),nullableTighteningStrategy(dataTransform + setNotNull),enumChangeStrategy(add → addEnumValues, removal → dataTransform + rebuild recipe, reorder → rebuild recipe). The enum rebuild recipe uses primitive descriptors: createEnumType(temp) → alterColumnType(USING cast) → dropEnumType(old) → renameType(temp, old).Extend the runner with data transform lifecycle. The runner handles
operationClass: 'data'with check → skip/run → check. The first check provides retry safety; the second validates violations are resolved.Add draft migration visibility. Draft packages appear as dashed graph edges in
migration status, with[draft]labels. Status diagnostics, apply warnings, and plan guards for duplicate drafts.Wire query builder integration.
createBuilders<Contract>()returns typed builder functions wheredataTransformcallbacks receiveDb<Contract>with full autocomplete. SQL lowered at verify time via the Postgres adapter. Extension functions (e.g., pgvector cosineDistance) available viaqueryOperationson control descriptors.Known issues for follow-up
Blocking further work
ALTER TYPE ADD VALUEcan't run in a transaction — enum value addition works at the planner/resolver level but fails at apply time. NeedsnoTransactiondescriptor support. Enum removal and reorder work fine (rebuild recipe uses transactional DDL).db update/db initstill use the old reconciliation planner — 2 production call sites. The descriptor planner supports this viaplanDescriptors({ strategies: devPushStrategies })but the dev-push strategies (temp defaults, destructive changes without data transforms) haven't been implemented yet.Minor gaps
.fails) — verifier doesn't flag types referenced by amissing_table.fails) —dropColumnordered before pattern opsmigration verifyre-attests unconditionally — even for already-applied migrationsresolveSetDefaultandresolveDropDefaulthave empty postchecks — relies on post-apply schema verification as safety netNot in scope
db updatedev-push strategies — separate follow-upTest plan
pnpm typecheck:packagespassespnpm lint:packagespassespnpm lint:depspassespnpm -w run test:packagespasses (2 known.failsin scenario tests)pnpm test:journeyspasses (57/57)pnpm test:integrationpassesSummary by CodeRabbit
New Features
migration newCLI to scaffold manual migration packagesImprovements
--configfor verifyChores