feat(migration): replace parentMigrationId with graph-topology pathfinding and refs#232
Merged
feat(migration): replace parentMigrationId with graph-topology pathfinding and refs#232
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors migration ordering from parent-pointer chains to graph-topology BFS, introduces named refs persisted in Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI (migration apply/status/plan)"
participant FS as "migrations/refs.json (FS)"
participant Graph as "On-disk Migration Graph"
participant Contract as "contract.json (envelope)"
participant DB as "Database (marker)"
CLI->>FS: readRefs(refsPath) [if --ref provided]
CLI->>Contract: readContractEnvelope()
CLI->>Graph: loadMigrationBundles(migrationsDir)
CLI->>Graph: findPathWithDecision(fromHash, targetHash, refName?)
alt no path
Graph-->>CLI: null (error: NO_PATH / AMBIGUOUS_LEAF / NO_RESOLVABLE_LEAF)
CLI->>CLI: emit error to user
else path found
Graph-->>CLI: PathDecision (selectedPath + tieBreaks)
CLI->>DB: apply migrations in PathDecision.selectedPath (runner)
DB-->>CLI: marker update
CLI-->>User: result with pathDecision + markerHash
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
|
4 tasks
@prisma-next/runtime-executor
@prisma-next/sql-runtime
@prisma-next/extension-paradedb
@prisma-next/extension-pgvector
@prisma-next/postgres
@prisma-next/sql-orm-client
@prisma-next/contract-authoring
@prisma-next/contract-ts
@prisma-next/ids
@prisma-next/psl-parser
@prisma-next/cli
@prisma-next/emitter
@prisma-next/eslint-plugin
@prisma-next/migration-tools
@prisma-next/vite-plugin-contract-emit
@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/family-sql
@prisma-next/sql-kysely-lane
@prisma-next/sql-lane-query-builder
@prisma-next/sql-relational-core
@prisma-next/sql-lane
@prisma-next/target-postgres
@prisma-next/adapter-postgres
@prisma-next/driver-postgres
@prisma-next/core-control-plane
@prisma-next/core-execution-plane
@prisma-next/config
@prisma-next/contract
@prisma-next/operations
@prisma-next/plan
@prisma-next/utils
commit: |
saevarb
force-pushed
the
refactor/declarative-dependency-ir
branch
from
381bbcb to
e61c030
Compare
saevarb
force-pushed
the
feat/state-transition-migrations
branch
3 times, most recently
from
501c8b7 to
f6f2731
Compare
saevarb
force-pushed
the
refactor/declarative-dependency-ir
branch
from
257830c to
277fa38
Compare
saevarb
force-pushed
the
feat/state-transition-migrations
branch
from
f6f2731 to
503f307
Compare
saevarb
force-pushed
the
refactor/declarative-dependency-ir
branch
from
bd0c236 to
793d5c6
Compare
saevarb
force-pushed
the
feat/state-transition-migrations
branch
from
503f307 to
e5a1253
Compare
saevarb
force-pushed
the
feat/state-transition-migrations
branch
2 times, most recently
from
942a76a to
897f6a4
Compare
saevarb
changed the title
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/2-sql/3-tooling/family/src/core/migrations/contract-to-schema-ir.ts (1)
57-74:⚠️ Potential issue | 🟠 MajorDon’t let a missing renderer erase column defaults.
With
renderDefaulttyped as optional, a caller that forgets to wire the target renderer still compiles and every defaulted column is emitted as if it had no default. That hides default-only diffs from planning.🐛 Proposed change
export interface ContractToSchemaIROptions { readonly annotationNamespace: string; readonly expandNativeType?: NativeTypeExpander; - readonly renderDefault?: DefaultRenderer; + readonly renderDefault: DefaultRenderer; readonly frameworkComponents?: ReadonlyArray<TargetBoundComponentDescriptor<'sql', string>>; }return { name, nativeType, nullable: column.nullable, - ...ifDefined( - 'default', - column.default != null && renderDefault ? renderDefault(column.default, column) : undefined, - ), + ...ifDefined('default', column.default != null ? renderDefault(column.default, column) : undefined), };Also applies to: 186-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/3-tooling/family/src/core/migrations/contract-to-schema-ir.ts` around lines 57 - 74, The current logic in contract-to-schema-ir.ts (function producing SqlColumnIR) drops column defaults when renderDefault (DefaultRenderer) is undefined; update the code around renderDefault, ifDefined, and the default field so that when column.default != null and renderDefault is not provided you still include the default in the SqlColumnIR (e.g., set the default property to column.default or a preserved/raw representation) instead of omitting it; alternatively make renderDefault required or throw an explicit error when a default exists but no renderer is wired—ensure the change touches the conditional that computes the 'default' spread (uses renderDefault, ifDefined, and column.default) so defaults are not silently erased.packages/1-framework/3-tooling/cli/src/commands/migration-status.ts (1)
325-333:⚠️ Potential issue | 🟡 MinorPreserve ref targeting in the empty-history result.
This branch always returns
targetHash: EMPTY_CONTRACT_HASH. When--refhas already resolved to a non-empty hash, JSON consumers lose the selected target completely and can't tell whether the caller chose a stale ref or the empty graph. ReturnrefName/refHashhere too, or fail fast once the ref cannot be satisfied by an empty history.🤖 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 325 - 333, The empty-history response currently always sets targetHash: EMPTY_CONTRACT_HASH losing the caller's resolved ref; update the branch that returns ok({ ..., targetHash: EMPTY_CONTRACT_HASH, ... }) in migration-status.ts to also include the ref metadata (e.g. refName and refHash/contractHash) when a --ref was provided and resolved, so JSON consumers can see the selected target, or alternatively detect when a ref was supplied but cannot be satisfied by an empty history and fail fast with an error; locate the return constructing the result (references: targetHash, EMPTY_CONTRACT_HASH, contractHash) and either add refName/refHash fields to that object or throw/return an error when a non-empty resolved ref is incompatible with the empty migrations list.
🧹 Nitpick comments (11)
packages/1-framework/3-tooling/cli/test/commands/migration-e2e.test.ts (3)
99-109: Consider consolidating manifest assertions withtoMatchObject.Multiple related manifest property checks could be combined for clearer intent.
As per coding guidelines: "Prefer object matchers (toMatchObject) over multiple individual expect().toBe() calls when checking 2 or more related values in tests."
Suggested consolidation
// Verify the package const verifyResult = await verifyMigration(packageDir); - expect(verifyResult.ok).toBe(true); - expect(verifyResult.storedMigrationId).toBe(migrationId); + expect(verifyResult).toMatchObject({ + ok: true, + storedMigrationId: migrationId, + }); // Read back and validate structure const pkg = await readMigrationPackage(packageDir); - expect(pkg.manifest.from).toBe(EMPTY_CONTRACT_HASH); - expect(pkg.manifest.to).toBe('sha256:initial-hash'); - expect(pkg.manifest.migrationId).toBe(migrationId); - expect(pkg.manifest.fromContract).toBeNull(); - expect(pkg.manifest.toContract).toEqual(toContract); + expect(pkg.manifest).toMatchObject({ + from: EMPTY_CONTRACT_HASH, + to: 'sha256:initial-hash', + migrationId, + fromContract: null, + toContract, + }); expect(pkg.ops).toHaveLength(1);🤖 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/commands/migration-e2e.test.ts` around lines 99 - 109, The test contains several individual assertions on pkg.manifest; consolidate them into a single object matcher to improve clarity — after calling readMigrationPackage(packageDir) and getting pkg, replace multiple expect(pkg.manifest.<...>) calls with one expect(pkg.manifest).toMatchObject({ from: EMPTY_CONTRACT_HASH, to: 'sha256:initial-hash', migrationId: migrationId, fromContract: null, toContract: toContract }); keep the separate expect(verifyResult.ok).toBe(true), expect(verifyResult.storedMigrationId).toBe(migrationId), and expect(pkg.ops).toHaveLength(1) as-is; reference functions/variables involved: readMigrationPackage, pkg, manifest, EMPTY_CONTRACT_HASH, migrationId, toContract.
3-3: Preferpatheovernode:pathfor path operations.As per coding guidelines: "Use
patheinstead ofnode:pathfor path manipulation in TypeScript files.patheprovides consistent behavior across Windows/macOS/Linux."Suggested change
-import { join } from 'node:path'; +import { join } from 'pathe';🤖 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/commands/migration-e2e.test.ts` at line 3, The test imports join from 'node:path' which violates the guideline to use pathe; update the import to pull join from 'pathe' instead (replace "import { join } from 'node:path';" with an import from 'pathe') and ensure any uses of join in migration-e2e.test.ts remain unchanged since pathe's API is compatible with the current calls.
51-58: Consider adding cleanup for ephemeral test directories.The temp directories created here aren't cleaned up after tests complete. While the OS eventually cleans
tmpdir(), explicit cleanup prevents accumulation during repeated test runs.As per coding guidelines: "Tests must clean up their own ephemeral directories."
Option: Return cleanup function
-async function createTempDir(): Promise<string> { +async function createTempDir(): Promise<{ dir: string; cleanup: () => Promise<void> }> { const dir = join( tmpdir(), `test-migration-e2e-${Date.now()}-${Math.random().toString(36).slice(2)}`, ); await mkdir(dir, { recursive: true }); - return dir; + return { + dir, + cleanup: async () => { + const { rm } = await import('node:fs/promises'); + await rm(dir, { recursive: true, force: true }); + }, + }; }🤖 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/commands/migration-e2e.test.ts` around lines 51 - 58, The createTempDir function creates ephemeral directories but never removes them; modify createTempDir to return both the directory path and a cleanup function (e.g., async cleanup() that calls fs.rm or fs.promises.rm with { recursive: true, force: true } or equivalent) so callers/tests can explicitly delete the temp dir when finished; update all call sites in migration-e2e.test.ts to call the returned cleanup after each test or in an afterEach/afterAll hook to ensure proper cleanup.packages/1-framework/3-tooling/cli/test/commands/migration-plan.test.ts (2)
98-105: Consider usingtoMatchObjectfor grouped assertions.Multiple individual
expect().toBe()calls checking related manifest properties could be consolidated for clarity.♻️ Suggested consolidation
- expect(pkg.manifest.from).toBe(EMPTY_CONTRACT_HASH); - expect(pkg.manifest.to).toBe('sha256:test-hash'); - expect(pkg.manifest.migrationId).toBe(migrationId); - expect(pkg.manifest.kind).toBe('regular'); - expect(pkg.manifest.fromContract).toBeNull(); - expect(pkg.ops).toHaveLength(1); - expect(pkg.ops[0]!.id).toBe('table.user'); + expect(pkg.manifest).toMatchObject({ + from: EMPTY_CONTRACT_HASH, + to: 'sha256:test-hash', + migrationId, + kind: 'regular', + fromContract: null, + }); + expect(pkg.ops).toHaveLength(1); + expect(pkg.ops[0]!.id).toBe('table.user');🤖 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/commands/migration-plan.test.ts` around lines 98 - 105, Consolidate the multiple related assertions on pkg.manifest into a single grouped assertion using toMatchObject: replace the separate expect(...).toBe(...) calls for manifest.from, manifest.to, manifest.migrationId, manifest.kind, and manifest.fromContract with expect(pkg.manifest).toMatchObject({...}) and keep the existing checks for pkg.ops (e.g., expect(pkg.ops).toHaveLength(1) and expect(pkg.ops[0]!.id).toBe('table.user')); update the test around migration-plan.test.ts where pkg and pkg.manifest are asserted (the block containing expect(pkg.manifest.from)...expect(pkg.ops[0]!.id)) so that related manifest properties are asserted together for clarity.
1-3: Usepatheinstead ofnode:pathfor cross-platform consistency.The coding guidelines require using
patheovernode:pathfor path manipulation in TypeScript files.♻️ Suggested fix
import { mkdir, readFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; -import { join } from 'node:path'; +import { join } from 'pathe';Based on learnings: "In the Prisma-next repository, prefer importing and using 'pathe' over the built-in 'node:path' module for path operations across the codebase."
🤖 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/commands/migration-plan.test.ts` around lines 1 - 3, The test imports join from 'node:path' which violates the guideline to use 'pathe'; update the import to pull join from 'pathe' instead (replace "import { join } from 'node:path'" with an import from 'pathe') and ensure any uses of join in migration-plan.test.ts remain unchanged; verify other path helpers (if later added) also come from the pathe package.test/integration/test/cli-journeys/migration-plan-details.e2e.test.ts (1)
14-14: Usepatheinstead ofnode:pathin this TypeScript test.The repo standardizes path helpers on
pathe; introducingnode:pathhere adds another path API convention for the same operation.♻️ Proposed fix
-import { join } from 'node:path'; +import { join } from 'pathe';As per coding guidelines:
**/*.{ts,tsx,mts,cts}: Usepatheinstead ofnode:pathfor path manipulation in TypeScript files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/test/cli-journeys/migration-plan-details.e2e.test.ts` at line 14, Replace the direct Node import with the repo-standard path helper: remove "import { join } from 'node:path';" and instead import join from 'pathe' (i.e., use the pathe join function wherever join is used in this test file such as in the migration-plan-details.e2e.test.ts import/usage). Ensure any other path helper usages in the same file follow pathe conventions.packages/1-framework/3-tooling/cli/test/commands/migration-ref.test.ts (3)
48-55: Clean up the temp dirs created bycreateTempDir().Every test in this suite allocates a fresh directory under
tmpdir(), but nothing removes them afterwards. Please either track them inafterEach()or switch this file to a helper likewithTempDir()so the CLI test surface does not leak ephemeral state.As per coding guidelines, "Tests must clean up their own ephemeral directories."
🤖 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/commands/migration-ref.test.ts` around lines 48 - 55, createTempDir currently creates temp directories under tmpdir() but never removes them; modify the test file to track created directories returned by createTempDir and remove them in an afterEach() cleanup (or refactor tests to use an existing withTempDir() helper that auto-cleans); specifically update the createTempDir usage sites and add an array (or per-test variable) to collect paths and call fs.rm/rmdir in afterEach(), or replace calls with withTempDir to ensure ephemeral dirs are deleted after each test.
1-3: Usepathein this TS test.This repo standardizes on
pathefor TypeScript path handling, so importingjoinfromnode:pathmakes this file an unnecessary exception.As per coding guidelines, "Use
patheinstead ofnode:pathfor path manipulation in TypeScript files."🤖 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/commands/migration-ref.test.ts` around lines 1 - 3, Replace the node:path usage with pathe in this TypeScript test: remove "import { join } from 'node:path'" and instead import join from pathe (e.g., "import { join } from 'pathe'") so that the test uses the repo-standard path utility; keep the existing imports for mkdir, readFile, and tmpdir unchanged and ensure all occurrences of join in the file use the pathe-provided join.
92-454: Add a thin command-level suite here.This file only exercises
readRefs/writeRefs/findPathdirectly, so parser behavior, stdout/stderr formatting, and exit-code handling for the newmigration refcommand are still unverified. If these cases are intentionally lower-level, they belong next to the migration-tools tests rather than undercli/test/commands.As per coding guidelines, "Test CLI commands by spying on
process.exit()to verify correct exit codes are called."🤖 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/commands/migration-ref.test.ts` around lines 92 - 454, The test file only exercises low-level helpers (readRefs, writeRefs, findPath) but lacks a thin command-level suite for the new "migration ref" CLI entrypoint; add tests that invoke the CLI command (e.g., via the command handler that parses args for "migration ref") to assert parser behavior, stdout/stderr formatting, and exit-code handling by spying on process.exit and stubbing stdout/stderr; if you intended to keep only unit tests for readRefs/writeRefs/findPath then move this file to the migration-tools test suite and add a new CLI-focused spec that calls the migration-ref command to verify output and exit codes.test/integration/test/cli-journeys/diamond-convergence.e2e.test.ts (1)
18-19: Usepathefor this path helper too.This TS file still imports
joinfromnode:path, which reintroduces a different path API into the journey tests.As per coding guidelines, "Use
patheinstead ofnode:pathfor path manipulation in TypeScript files."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/test/cli-journeys/diamond-convergence.e2e.test.ts` around lines 18 - 19, The test imports join from node:path which violates the guideline; replace the node:path import with pathe's join (i.e., import { join } from 'pathe') and update any references to join in this file (diamond-convergence.e2e.test.ts) to use the pathe implementation so the journey tests use a consistent path API; leave readFileSync/writeFileSync imports from node:fs as-is.packages/1-framework/3-tooling/migration/src/refs.ts (1)
28-35: Use the repo's Arktype record form here.
type('Record<string, string>')is the one record pattern the repo explicitly avoids. Prefer the object-form schema before thenarrow()so this validator matches the rest of the codebase.♻️ Suggested change
-const RefsSchema = type('Record<string, string>').narrow((refs, ctx) => { +const RefsSchema = type({ "[string]": "string" }).narrow((refs, ctx) => {As per coding guidelines, use
type({ "[string]": Schema })for defining record types in Arktype.🤖 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/refs.ts` around lines 28 - 35, RefsSchema currently uses the string-form record pattern type('Record<string, string>') which the repo forbids; change it to the object-form Arktype record pattern by defining the schema as type({ "[string]": type/string() }) (or the appropriate string schema) and then apply the same narrow(...) validator, keeping the existing validation logic that calls validateRefName(key) and validateRefValue(value) inside the narrow; update the symbol RefsSchema accordingly so the shape uses type({ "[string]": ... }) before calling narrow and preserves the same error messages.
🤖 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-ref.ts`:
- Around line 20-69: The createRefSetCommand handler is directly using console.*
and process.exit and bypasses the shared CLI pipeline; refactor the async action
(in createRefSetCommand) to return a Result and use performAction/handleResult
so errors go through CliStructuredError handling: validate input with
validateRefName but convert failures into a CliStructuredError instead of
console.error/process.exit, call loadConfig/resolveRefsPath/readRefs/writeRefs
inside a performAction-returning function, map MigrationToolsError and other
exceptions into structured errors (or throw CliStructuredError) rather than
logging, and use parseGlobalFlags to decide JSON/quiet output only in the
success branch handled by the shared pipeline; update the other subcommand
handlers noted (lines ~72-111, 113-153, 155-194, 196-213) the same way so all
use performAction/CliStructuredError/handleResult.
- Around line 128-135: The existence check for a ref uses the
prototype-traversing "in" operator; update the check in the migration-ref
deletion logic to use Object.hasOwn(refs, name) instead so only own properties
from the parsed refs object are validated. Locate the block where
readRefs(refsPath) is assigned to refs and the conditional if (!(name in refs))
and replace that condition with a call to Object.hasOwn(refs, name) before
proceeding to compute remaining and call writeRefs(refsPath, remaining). Ensure
the behavior and error handling (console.error + process.exit) remain the same.
In `@packages/1-framework/3-tooling/cli/src/commands/migration-status.ts`:
- Around line 180-198: resolveDisplayChain currently assumes targetHash is a
descendant of markerHash and falls back to EMPTY_CONTRACT_HASH, which hides the
marker when the DB is ahead of the ref or when using a custom from anchor.
Update resolveDisplayChain to: 1) avoid hard-coding EMPTY_CONTRACT_HASH as the
default anchor unless no other anchor is available, 2) when toMarker exists but
findPath(graph, markerHash, targetHash) returns null, try the inverse path
findPath(graph, targetHash, markerHash) to detect the "database ahead of ref"
(ancestor) case and construct the display chain by combining toMarker with the
reversed inverse path (so the marker remains displayed and the chain shows
"ahead of ref"), and 3) ensure any fallback to EMPTY_CONTRACT_HASH only happens
when neither forward nor inverse paths exist; reference resolveDisplayChain,
findPath, EMPTY_CONTRACT_HASH, markerHash, targetHash, toMarker, fromMarker when
making the change.
In `@packages/1-framework/3-tooling/migration/src/dag.ts`:
- Around line 281-309: findLeaf currently seeds findReachableLeaves with
EMPTY_CONTRACT_HASH which mislabels graphs whose first migration starts from a
non-empty hash; instead compute the actual graph roots and use them as traversal
seeds. Update findLeaf to (1) if graph.nodes is empty return EMPTY_CONTRACT_HASH
as before; (2) compute roots = [...graph.nodes].filter(n => no incoming edges
exist to n in graph.edges) (exclude EMPTY_CONTRACT_HASH from roots only if it
isn't present); (3) call findReachableLeaves(graph, ...roots) (or call it
per-root and merge results) so reachable leaves reflect the true lineage; keep
the existing ambiguity / no-leaf error paths and references to
findDivergencePoint, findPath, errorAmbiguousLeaf, errorNoResolvableLeaf, and
findLatestMigration unchanged.
In `@packages/1-framework/3-tooling/migration/src/refs.ts`:
- Around line 78-80: The temp-file name built for atomic writes in writeRefs
(tmpPath) is vulnerable to collisions because it only uses Date.now(); change
tmpPath to include additional entropy (e.g., process.pid and a short
crypto-random suffix) so concurrent writeRefs() calls cannot produce the same
name. Locate the tmpPath construction in refs.ts (where tmpPath = join(dir,
`.refs.json.${Date.now()}.tmp`)) and replace it with a name that appends
process.pid and a hex string from crypto.randomBytes(6) (or use fs.mkdtemp
semantics) before writing and renaming, keeping the same rename(tmpPath,
refsPath) logic and error handling.
In `@test/integration/test/cli-journeys/adopt-migrations.e2e.test.ts`:
- Around line 68-75: The test currently only checks applyBaselineResult.ok and
markerHash, which can miss unexpected execution; update the assertions to
explicitly assert that applyBaselineResult.migrationsApplied is 0 (no-op) in
addition to ok === true and markerHash === c2Hash. Locate the parseJsonOutput
call that produces applyBaselineResult and add/replace the expect asserting
migrationsApplied toBe(0) (alongside the existing expect(applyBaselineResult.ok,
...) and expect(applyBaselineResult.markerHash, ...)) so the baseline apply is
guaranteed to have been a no-op.
In `@test/integration/test/cli-journeys/divergence-and-refs.e2e.test.ts`:
- Around line 80-85: The test currently mixes stdout and stderr when checking
for the "ambiguous leaf" diagnostic; change the assertion to inspect only stdout
(use stripAnsi(statusFail.stdout) instead of stripAnsi(statusFail.stdout +
statusFail.stderr)) so the machine-readable UI output from runMigrationStatus is
validated; update the variable usage (failOutput) and the expect call that
matches /ambiguous|AMBIGUOUS_LEAF|multiple.*leaf/i to read from stdout only.
- Around line 92-100: The test currently only asserts applyRef.exitCode and
applyResult.ok after calling runMigrationApply(ctx, ['--ref', 'production',
'--json']), which doesn't guarantee the C3 branch was used; update the test to
also assert applyResult.migrationsApplied equals the expected number and
applyResult.markerHash equals the expected C3 marker hash (use the known marker
hash for C3 in this suite) to lock routing behavior; locate the assertion area
around applyRef / parseJsonOutput / applyResult and add checks for
migrationsApplied and markerHash to validate the branch selection end-to-end.
In `@test/integration/test/cli-journeys/migration-apply-edge-cases.e2e.test.ts`:
- Around line 64-69: The test currently concatenates stdout and stderr when
checking the failure diagnostic from runMigrationApply (variable apply1), which
can mask missing machine-readable output; change the assertion to inspect only
stdout by using stripAnsi on apply1.stdout (not apply1.stdout + apply1.stderr)
so the regex match verifies machine-readable diagnostics emitted to stdout by
the CLI.
- Around line 289-299: The test currently checks presence/absence of columns
piecemeal which can miss unexpected columns; replace the two assertions after
computing columnNames (from sql(connectionString...)) with a single exact-array
assertion that columnNames equals ['id'] (use expect(columnNames).toEqual(['id']
or equivalent) to assert the final schema exactly), ensuring you reference the
columnNames variable produced from the sql(...) call in the
migration-apply-edge-cases.e2e.test.ts test block.
In `@test/integration/test/cli-journeys/migration-plan-details.e2e.test.ts`:
- Around line 145-148: The current search only matches email-related ops by
id/label; update the assertion to also verify the operation's class by checking
result.operations for an entry where (op.id.includes('email') ||
op.label.toLowerCase().includes('email')) AND op.operationClass ===
'<expectedDestructiveClass>', then assert that variable dropOp is defined and
that dropOp.operationClass equals '<expectedDestructiveClass>' (use the real
expected class string in place of <expectedDestructiveClass>) so the test fails
if the CLI emits an email op with the wrong operationClass; reference the
variables dropOp, result.operations and the property operationClass when making
the change.
In `@test/integration/test/cli-journeys/ref-routing.e2e.test.ts`:
- Around line 114-121: Change the assertion to require the diagnostic to appear
on stdout (the machine-readable channel) instead of reading stderr or depending
solely on exitCode; locate the runMigrationStatus call and the statusProdAfter
variable and replace prodAfterOutput = stripAnsi(statusProdAfter.stdout +
statusProdAfter.stderr) with using only stripAnsi(statusProdAfter.stdout), then
assert that the stdout text matches /ahead|no.*path|mismatch/i (you may still
check exitCode === 1 separately if desired, but the regex must be applied to
statusProdAfter.stdout).
In `@test/integration/test/cli-journeys/rollback-cycle.e2e.test.ts`:
- Around line 80-85: The test is concatenating stderr with stdout which can mask
regressions; change the diagnostic extraction to read only the machine-readable
stdout from the runMigrationPlan result. Locate the usage of
runMigrationPlan/planFail and the failOutput assignment (currently using
stripAnsi(planFail.stdout + planFail.stderr)) and replace it so failOutput =
stripAnsi(planFail.stdout) before the expect that matches
/no.*resolvable.*leaf|cycle|NO_RESOLVABLE_LEAF/i; keep the same expectations and
message identifiers (e.g., "J.04: plan without --from fails" and "J.04: error
mentions no resolvable leaf").
---
Outside diff comments:
In `@packages/1-framework/3-tooling/cli/src/commands/migration-status.ts`:
- Around line 325-333: The empty-history response currently always sets
targetHash: EMPTY_CONTRACT_HASH losing the caller's resolved ref; update the
branch that returns ok({ ..., targetHash: EMPTY_CONTRACT_HASH, ... }) in
migration-status.ts to also include the ref metadata (e.g. refName and
refHash/contractHash) when a --ref was provided and resolved, so JSON consumers
can see the selected target, or alternatively detect when a ref was supplied but
cannot be satisfied by an empty history and fail fast with an error; locate the
return constructing the result (references: targetHash, EMPTY_CONTRACT_HASH,
contractHash) and either add refName/refHash fields to that object or
throw/return an error when a non-empty resolved ref is incompatible with the
empty migrations list.
In
`@packages/2-sql/3-tooling/family/src/core/migrations/contract-to-schema-ir.ts`:
- Around line 57-74: The current logic in contract-to-schema-ir.ts (function
producing SqlColumnIR) drops column defaults when renderDefault
(DefaultRenderer) is undefined; update the code around renderDefault, ifDefined,
and the default field so that when column.default != null and renderDefault is
not provided you still include the default in the SqlColumnIR (e.g., set the
default property to column.default or a preserved/raw representation) instead of
omitting it; alternatively make renderDefault required or throw an explicit
error when a default exists but no renderer is wired—ensure the change touches
the conditional that computes the 'default' spread (uses renderDefault,
ifDefined, and column.default) so defaults are not silently erased.
---
Nitpick comments:
In `@packages/1-framework/3-tooling/cli/test/commands/migration-e2e.test.ts`:
- Around line 99-109: The test contains several individual assertions on
pkg.manifest; consolidate them into a single object matcher to improve clarity —
after calling readMigrationPackage(packageDir) and getting pkg, replace multiple
expect(pkg.manifest.<...>) calls with one expect(pkg.manifest).toMatchObject({
from: EMPTY_CONTRACT_HASH, to: 'sha256:initial-hash', migrationId: migrationId,
fromContract: null, toContract: toContract }); keep the separate
expect(verifyResult.ok).toBe(true),
expect(verifyResult.storedMigrationId).toBe(migrationId), and
expect(pkg.ops).toHaveLength(1) as-is; reference functions/variables involved:
readMigrationPackage, pkg, manifest, EMPTY_CONTRACT_HASH, migrationId,
toContract.
- Line 3: The test imports join from 'node:path' which violates the guideline to
use pathe; update the import to pull join from 'pathe' instead (replace "import
{ join } from 'node:path';" with an import from 'pathe') and ensure any uses of
join in migration-e2e.test.ts remain unchanged since pathe's API is compatible
with the current calls.
- Around line 51-58: The createTempDir function creates ephemeral directories
but never removes them; modify createTempDir to return both the directory path
and a cleanup function (e.g., async cleanup() that calls fs.rm or fs.promises.rm
with { recursive: true, force: true } or equivalent) so callers/tests can
explicitly delete the temp dir when finished; update all call sites in
migration-e2e.test.ts to call the returned cleanup after each test or in an
afterEach/afterAll hook to ensure proper cleanup.
In `@packages/1-framework/3-tooling/cli/test/commands/migration-plan.test.ts`:
- Around line 98-105: Consolidate the multiple related assertions on
pkg.manifest into a single grouped assertion using toMatchObject: replace the
separate expect(...).toBe(...) calls for manifest.from, manifest.to,
manifest.migrationId, manifest.kind, and manifest.fromContract with
expect(pkg.manifest).toMatchObject({...}) and keep the existing checks for
pkg.ops (e.g., expect(pkg.ops).toHaveLength(1) and
expect(pkg.ops[0]!.id).toBe('table.user')); update the test around
migration-plan.test.ts where pkg and pkg.manifest are asserted (the block
containing expect(pkg.manifest.from)...expect(pkg.ops[0]!.id)) so that related
manifest properties are asserted together for clarity.
- Around line 1-3: The test imports join from 'node:path' which violates the
guideline to use 'pathe'; update the import to pull join from 'pathe' instead
(replace "import { join } from 'node:path'" with an import from 'pathe') and
ensure any uses of join in migration-plan.test.ts remain unchanged; verify other
path helpers (if later added) also come from the pathe package.
In `@packages/1-framework/3-tooling/cli/test/commands/migration-ref.test.ts`:
- Around line 48-55: createTempDir currently creates temp directories under
tmpdir() but never removes them; modify the test file to track created
directories returned by createTempDir and remove them in an afterEach() cleanup
(or refactor tests to use an existing withTempDir() helper that auto-cleans);
specifically update the createTempDir usage sites and add an array (or per-test
variable) to collect paths and call fs.rm/rmdir in afterEach(), or replace calls
with withTempDir to ensure ephemeral dirs are deleted after each test.
- Around line 1-3: Replace the node:path usage with pathe in this TypeScript
test: remove "import { join } from 'node:path'" and instead import join from
pathe (e.g., "import { join } from 'pathe'") so that the test uses the
repo-standard path utility; keep the existing imports for mkdir, readFile, and
tmpdir unchanged and ensure all occurrences of join in the file use the
pathe-provided join.
- Around line 92-454: The test file only exercises low-level helpers (readRefs,
writeRefs, findPath) but lacks a thin command-level suite for the new "migration
ref" CLI entrypoint; add tests that invoke the CLI command (e.g., via the
command handler that parses args for "migration ref") to assert parser behavior,
stdout/stderr formatting, and exit-code handling by spying on process.exit and
stubbing stdout/stderr; if you intended to keep only unit tests for
readRefs/writeRefs/findPath then move this file to the migration-tools test
suite and add a new CLI-focused spec that calls the migration-ref command to
verify output and exit codes.
In `@packages/1-framework/3-tooling/migration/src/refs.ts`:
- Around line 28-35: RefsSchema currently uses the string-form record pattern
type('Record<string, string>') which the repo forbids; change it to the
object-form Arktype record pattern by defining the schema as type({ "[string]":
type/string() }) (or the appropriate string schema) and then apply the same
narrow(...) validator, keeping the existing validation logic that calls
validateRefName(key) and validateRefValue(value) inside the narrow; update the
symbol RefsSchema accordingly so the shape uses type({ "[string]": ... }) before
calling narrow and preserves the same error messages.
In `@test/integration/test/cli-journeys/diamond-convergence.e2e.test.ts`:
- Around line 18-19: The test imports join from node:path which violates the
guideline; replace the node:path import with pathe's join (i.e., import { join }
from 'pathe') and update any references to join in this file
(diamond-convergence.e2e.test.ts) to use the pathe implementation so the journey
tests use a consistent path API; leave readFileSync/writeFileSync imports from
node:fs as-is.
In `@test/integration/test/cli-journeys/migration-plan-details.e2e.test.ts`:
- Line 14: Replace the direct Node import with the repo-standard path helper:
remove "import { join } from 'node:path';" and instead import join from 'pathe'
(i.e., use the pathe join function wherever join is used in this test file such
as in the migration-plan-details.e2e.test.ts import/usage). Ensure any other
path helper usages in the same file follow pathe conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 5fd11a6f-95df-4275-a8e7-d4bd8afc2b3c
⛔ Files ignored due to path filters (6)
projects/on-disk-migrations-v2/assets/.gitkeepis excluded by!projects/**projects/on-disk-migrations-v2/plan.mdis excluded by!projects/**projects/on-disk-migrations-v2/plans/.gitkeepis excluded by!projects/**projects/on-disk-migrations-v2/plans/pr-232-review-fixes-plan.mdis excluded by!projects/**projects/on-disk-migrations-v2/spec.mdis excluded by!projects/**projects/on-disk-migrations-v2/specs/.gitkeepis excluded by!projects/**
📒 Files selected for processing (58)
docs/architecture docs/adrs/ADR 169 - On-disk migration persistence.mddocs/architecture docs/subsystems/7. Migration System.mdpackages/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-plan.tspackages/1-framework/3-tooling/cli/src/commands/migration-ref.tspackages/1-framework/3-tooling/cli/src/commands/migration-status.tspackages/1-framework/3-tooling/cli/src/utils/formatters/migrations.tspackages/1-framework/3-tooling/cli/test/commands/migration-apply.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-e2e.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-plan.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-ref.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-show.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-status.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-verify.test.tspackages/1-framework/3-tooling/cli/test/output.json-shapes.test.tspackages/1-framework/3-tooling/cli/tsdown.config.tspackages/1-framework/3-tooling/cli/vitest.config.tspackages/1-framework/3-tooling/migration/package.jsonpackages/1-framework/3-tooling/migration/src/dag.tspackages/1-framework/3-tooling/migration/src/errors.tspackages/1-framework/3-tooling/migration/src/exports/dag.tspackages/1-framework/3-tooling/migration/src/exports/refs.tspackages/1-framework/3-tooling/migration/src/io.tspackages/1-framework/3-tooling/migration/src/refs.tspackages/1-framework/3-tooling/migration/src/types.tspackages/1-framework/3-tooling/migration/test/attestation.test.tspackages/1-framework/3-tooling/migration/test/dag.test.tspackages/1-framework/3-tooling/migration/test/fixtures.tspackages/1-framework/3-tooling/migration/test/refs.test.tspackages/1-framework/3-tooling/migration/test/scenarios.test.tspackages/1-framework/3-tooling/migration/tsdown.config.tspackages/2-sql/3-tooling/family/src/core/migrations/contract-to-schema-ir.tspackages/2-sql/3-tooling/family/src/exports/control.tspackages/2-sql/3-tooling/family/test/contract-to-schema-ir.test.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner.tspackages/3-targets/3-targets/postgres/src/exports/control.tspackages/3-targets/3-targets/postgres/test/migrations/planner.contract-to-schema-ir.test.tstest/integration/test/cli-journeys/README.mdtest/integration/test/cli-journeys/adopt-migrations.e2e.test.tstest/integration/test/cli-journeys/converging-paths.e2e.test.tstest/integration/test/cli-journeys/diamond-convergence.e2e.test.tstest/integration/test/cli-journeys/divergence-and-refs.e2e.test.tstest/integration/test/cli-journeys/interleaved-db-update.e2e.test.tstest/integration/test/cli-journeys/migration-apply-edge-cases.e2e.test.tstest/integration/test/cli-journeys/migration-plan-details.e2e.test.tstest/integration/test/cli-journeys/ref-routing.e2e.test.tstest/integration/test/cli-journeys/rollback-cycle.e2e.test.tstest/integration/test/cli.migration-apply.e2e.test.tstest/integration/test/cli.migration-plan.e2e.test.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-additive-required.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-all.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-avatar.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-bio.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-phone-bio.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-phone.tstest/integration/test/utils/journey-test-helpers.ts
💤 Files with no reviewable changes (8)
- packages/1-framework/3-tooling/migration/src/types.ts
- packages/1-framework/3-tooling/cli/test/commands/migration-verify.test.ts
- packages/1-framework/3-tooling/cli/test/commands/migration-show.test.ts
- test/integration/test/cli.migration-plan.e2e.test.ts
- packages/1-framework/3-tooling/migration/test/attestation.test.ts
- packages/1-framework/3-tooling/migration/test/fixtures.ts
- packages/1-framework/3-tooling/migration/src/io.ts
- test/integration/test/cli.migration-apply.e2e.test.ts
saevarb
force-pushed
the
feat/state-transition-migrations
branch
from
cb0b2a1 to
0187bad
Compare
saevarb
force-pushed
the
feat/state-transition-migrations
branch
from
139f764 to
e68fe16
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/1-framework/3-tooling/migration/src/dag.ts (1)
293-316:⚠️ Potential issue | 🟠 Major
findLeaf()still silently treats disconnected graphs as empty.If
EMPTY_CONTRACT_HASHexists but has no outgoing edges while other nodes exist, this returnsEMPTY_CONTRACT_HASHinstead of surfacing an unresolved leaf state (Line 293 onward). That can hide broken migration history and makes downstreamfindLatestMigration()returnnullincorrectly.Suggested fix
const leaves = findReachableLeaves(graph, EMPTY_CONTRACT_HASH); + if (leaves.length === 1 && leaves[0] === EMPTY_CONTRACT_HASH) { + const unreachable = [...graph.nodes].filter((n) => n !== EMPTY_CONTRACT_HASH); + if (unreachable.length > 0) { + throw errorNoResolvableLeaf(unreachable); + } + } + if (leaves.length === 0) { const reachable = [...graph.nodes].filter((n) => n !== EMPTY_CONTRACT_HASH); if (reachable.length > 0) { throw errorNoResolvableLeaf(reachable);🤖 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/dag.ts` around lines 293 - 316, The findLeaf logic currently treats a disconnected graph with EMPTY_CONTRACT_HASH as a valid leaf; update the branch handling leaves.length === 0 in findLeaf so that if there are any other nodes besides EMPTY_CONTRACT_HASH OR if EMPTY_CONTRACT_HASH exists but has no outgoing edges, you throw errorNoResolvableLeaf(reachable) (use the reachable = [...graph.nodes].filter(n => n !== EMPTY_CONTRACT_HASH) calculation) instead of returning EMPTY_CONTRACT_HASH; keep references to findReachableLeaves, EMPTY_CONTRACT_HASH, and errorNoResolvableLeaf when locating and updating the code path so the unresolved/disconnected state is surfaced to callers like findLatestMigration().
🧹 Nitpick comments (7)
packages/1-framework/3-tooling/cli/test/commands/migration-apply.test.ts (1)
1-3: Consider usingpatheinstead ofnode:pathfor path operations.This test file is under
packages/, nottest/integration/test/. Per coding guidelines, TypeScript files inpackages/**should usepathefor consistent cross-platform behavior.Suggested change
import { mkdir } from 'node:fs/promises'; import { tmpdir } from 'node:os'; -import { join } from 'node:path'; +import { join } from 'pathe';As per coding guidelines: "Use
patheinstead ofnode:pathfor path manipulation in TypeScript files."🤖 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/commands/migration-apply.test.ts` around lines 1 - 3, Replace the node:path usage with pathe: remove the import of join from 'node:path' and import the appropriate function(s) from 'pathe' (e.g., join) instead, then update any uses of join in this test (migration-apply.test.ts) to use the pathe join to ensure cross-platform path handling; keep the existing mkdir/tmpdir usage unchanged.test/integration/test/cli-journeys/migration-plan-details.e2e.test.ts (1)
64-70: Group related envelope assertions withtoMatchObject.Multiple related fields from the JSON envelope are being checked individually. Consider grouping them for clarity.
♻️ Suggested refactor
- expect(result.ok, 'H.02: ok flag').toBe(true); - expect(result.noOp, 'H.02: not a noop').toBe(false); - expect(result.from, 'H.02: from is empty hash').toBe(EMPTY_CONTRACT_HASH); - expect(result.to, 'H.02: to is defined').toBeDefined(); - expect(result.migrationId, 'H.02: migrationId is defined').toBeDefined(); - expect(result.dir, 'H.02: dir is defined').toBeDefined(); - expect(result.operations.length, 'H.02: has operations').toBeGreaterThan(0); + expect(result).toMatchObject({ + ok: true, + noOp: false, + from: EMPTY_CONTRACT_HASH, + }); + expect(result.to, 'H.02: to is defined').toBeDefined(); + expect(result.migrationId, 'H.02: migrationId is defined').toBeDefined(); + expect(result.dir, 'H.02: dir is defined').toBeDefined(); + expect(result.operations.length, 'H.02: has operations').toBeGreaterThan(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/test/cli-journeys/migration-plan-details.e2e.test.ts` around lines 64 - 70, Group the related envelope assertions on the result object into a single toMatchObject assertion instead of multiple expect calls: replace the individual checks of result.ok, result.noOp, result.from, result.to, result.migrationId and result.dir with one expect(result).toMatchObject({...}) that asserts ok: true, noOp: false, from: EMPTY_CONTRACT_HASH and uses generic matchers (e.g., expect.anything() or expect.any(String)) for to, migrationId and dir; keep the separate expect(result.operations.length).toBeGreaterThan(0) check for operations.test/integration/test/cli-journeys/ref-routing.e2e.test.ts (1)
89-96: Consider grouping related apply result assertions.♻️ Suggested refactor
- expect(applyStagingResult.ok, 'M.06: ok').toBe(true); - expect(applyStagingResult.migrationsApplied, 'M.06: applied 1').toBe(1); - expect(applyStagingResult.markerHash, 'M.06: marker at C2').toBe(c2Hash); + expect(applyStagingResult).toMatchObject({ + ok: true, + migrationsApplied: 1, + markerHash: c2Hash, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/test/cli-journeys/ref-routing.e2e.test.ts` around lines 89 - 96, Group the three related assertions about the staging apply result into a single assertion to make the intent clearer and reduce repetition: locate where parseJsonOutput is used to create applyStagingResult and replace the three expects on applyStagingResult.ok, .migrationsApplied, and .markerHash with one compound assertion (e.g., using toMatchObject or toEqual) that checks { ok: true, migrationsApplied: 1, markerHash: c2Hash } so the verification of parseJsonOutput, migrationsApplied, and c2Hash occurs in one concise check.test/integration/test/cli-journeys/diamond-convergence.e2e.test.ts (2)
180-187: Apply sametoMatchObjectpattern for production result assertions.♻️ Suggested refactor
- expect(prodResult.ok, 'D.09: production ok').toBe(true); - expect(prodResult.migrationsApplied, 'D.09: production applied 1 merge').toBe(1); - expect(prodResult.markerHash, 'D.09: production marker at C5').toBe(c5Hash); + expect(prodResult).toMatchObject({ + ok: true, + migrationsApplied: 1, + markerHash: c5Hash, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/test/cli-journeys/diamond-convergence.e2e.test.ts` around lines 180 - 187, The assertions for the production run are verbose; replace the three separate expects that check prodResult.ok, prodResult.migrationsApplied, and prodResult.markerHash with a single toMatchObject assertion: after obtaining prodResult from parseJsonOutput(applyProd2), use expect(prodResult).toMatchObject({ ok: true, migrationsApplied: 1, markerHash: c5Hash }); reference parseJsonOutput, prodResult, applyProd2, and c5Hash when locating the lines to change.
168-175: Consider usingtoMatchObjectfor related assertions.Per coding guidelines, prefer object matchers over multiple individual
expect().toBe()calls when checking 2 or more related values.♻️ Suggested refactor
- expect(stagingResult.ok, 'D.08: staging ok').toBe(true); - expect(stagingResult.migrationsApplied, 'D.08: staging applied 1 merge').toBe(1); - expect(stagingResult.markerHash, 'D.08: staging marker at C5').toBe(c5Hash); + expect(stagingResult).toMatchObject({ + ok: true, + migrationsApplied: 1, + markerHash: c5Hash, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/test/cli-journeys/diamond-convergence.e2e.test.ts` around lines 168 - 175, Combine the three related assertions on stagingResult into a single object matcher: after calling parseJsonOutput<{...}>(applyStaging3) produce one assertion using expect(stagingResult).toMatchObject({ ok: true, migrationsApplied: 1, markerHash: c5Hash }) so the values parsed from applyStaging3 are validated together; update the assertions that reference stagingResult, parseJsonOutput, applyStaging3 and c5Hash accordingly and remove the separate expect(...).toBe() calls.test/integration/test/cli-journeys/interleaved-db-update.e2e.test.ts (1)
114-121: Consider grouping step 6 apply result assertions.♻️ Suggested refactor
- expect(apply3Result.ok, '6: ok').toBe(true); - expect(apply3Result.migrationsApplied, '6: applied 1').toBe(1); - expect(apply3Result.markerHash, '6: marker at C4').toBe(c4Hash); + expect(apply3Result).toMatchObject({ + ok: true, + migrationsApplied: 1, + markerHash: c4Hash, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/test/cli-journeys/interleaved-db-update.e2e.test.ts` around lines 114 - 121, Group the three separate assertions on apply3Result into a single grouped assertion to improve clarity and reduce repetition: replace the individual expects that check apply3Result.ok, apply3Result.migrationsApplied, and apply3Result.markerHash with one assertion that compares apply3Result against the expected shape/values (e.g., using toEqual or toMatchObject) so it verifies { ok: true, migrationsApplied: 1, markerHash: c4Hash } in one statement; update references to parseJsonOutput and apply3Result accordingly.packages/1-framework/3-tooling/cli/test/commands/migration-status.test.ts (1)
1-3: Consider usingpatheinstead ofnode:path.Per the coding guidelines, prefer
patheovernode:pathin TypeScript files for consistent cross-platform behavior. However, the learning specifically mentionstest/integration/test/as an exception. Since this file is underpackages/, the guideline applies.Based on learnings: "In the Prisma-next repository, prefer importing and using 'pathe' over the built-in 'node:path' module for path operations across the codebase."♻️ Proposed fix
import { mkdir, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; -import { join } from 'node:path'; +import { join } from 'pathe';🤖 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/commands/migration-status.test.ts` around lines 1 - 3, Replace the node:path import with pathe to follow the repo guideline: change the import of join (currently from 'node:path') to import join from 'pathe' (or named import if used elsewhere) in this test file; update any uses of join in migration-status.test.ts to continue working with pathe's API (function name join remains the same) so path operations become cross-platform consistent.
🤖 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/dag.ts`:
- Around line 184-186: The tie-break logic currently compares migrationId values
(sorted[0].migrationId === edge.migrationId), which can misidentify edges when
IDs are null/duplicate; change the comparison to test edge identity instead
(e.g., use strict reference equality sorted[0] === edge or compare a unique edge
identifier if one exists) so the tieBreakReasons push only occurs for the exact
edge object selected; update the conditional and any related comments to reflect
identity-based comparison.
In
`@packages/2-sql/3-tooling/family/src/core/migrations/contract-to-schema-ir.ts`:
- Around line 58-59: The function currently accepts renderDefault as an optional
DefaultRenderer which leads to silent dropping of column defaults; make
renderDefault a required parameter (change its type from DefaultRenderer |
undefined to DefaultRenderer) in the function that returns SqlColumnIR, remove
the branches that treat renderDefault as missing and instead replace them with
explicit use of renderDefault or throw a clear error if a default cannot be
rendered, and update any internal code paths (the logic that sets column.default
/ drops defaults) so they call renderDefault directly rather than skipping
defaults when it's undefined; reference the symbols renderDefault,
DefaultRenderer, and SqlColumnIR to locate and update the signature and the
default-rendering branches accordingly.
In `@packages/2-sql/3-tooling/family/test/contract-to-schema-ir.test.ts`:
- Around line 23-26: The JSON default literal isn't escaping single quotes
before being embedded in SQL, so update the logic that builds the JSON string
(the json constant used with isJsonColumn and column.nativeType) to escape
apostrophes the same way testRenderer does for text defaults (e.g. replace
single quotes with doubled single quotes) before returning `'${json}'` or
`'${json}'::${column.nativeType}`; modify the code around the json variable and
the branches that use it (referencing json, isJsonColumn and column.nativeType)
to perform this replacement so JSON containing apostrophes produces valid SQL.
---
Duplicate comments:
In `@packages/1-framework/3-tooling/migration/src/dag.ts`:
- Around line 293-316: The findLeaf logic currently treats a disconnected graph
with EMPTY_CONTRACT_HASH as a valid leaf; update the branch handling
leaves.length === 0 in findLeaf so that if there are any other nodes besides
EMPTY_CONTRACT_HASH OR if EMPTY_CONTRACT_HASH exists but has no outgoing edges,
you throw errorNoResolvableLeaf(reachable) (use the reachable =
[...graph.nodes].filter(n => n !== EMPTY_CONTRACT_HASH) calculation) instead of
returning EMPTY_CONTRACT_HASH; keep references to findReachableLeaves,
EMPTY_CONTRACT_HASH, and errorNoResolvableLeaf when locating and updating the
code path so the unresolved/disconnected state is surfaced to callers like
findLatestMigration().
---
Nitpick comments:
In `@packages/1-framework/3-tooling/cli/test/commands/migration-apply.test.ts`:
- Around line 1-3: Replace the node:path usage with pathe: remove the import of
join from 'node:path' and import the appropriate function(s) from 'pathe' (e.g.,
join) instead, then update any uses of join in this test
(migration-apply.test.ts) to use the pathe join to ensure cross-platform path
handling; keep the existing mkdir/tmpdir usage unchanged.
In `@packages/1-framework/3-tooling/cli/test/commands/migration-status.test.ts`:
- Around line 1-3: Replace the node:path import with pathe to follow the repo
guideline: change the import of join (currently from 'node:path') to import join
from 'pathe' (or named import if used elsewhere) in this test file; update any
uses of join in migration-status.test.ts to continue working with pathe's API
(function name join remains the same) so path operations become cross-platform
consistent.
In `@test/integration/test/cli-journeys/diamond-convergence.e2e.test.ts`:
- Around line 180-187: The assertions for the production run are verbose;
replace the three separate expects that check prodResult.ok,
prodResult.migrationsApplied, and prodResult.markerHash with a single
toMatchObject assertion: after obtaining prodResult from
parseJsonOutput(applyProd2), use expect(prodResult).toMatchObject({ ok: true,
migrationsApplied: 1, markerHash: c5Hash }); reference parseJsonOutput,
prodResult, applyProd2, and c5Hash when locating the lines to change.
- Around line 168-175: Combine the three related assertions on stagingResult
into a single object matcher: after calling
parseJsonOutput<{...}>(applyStaging3) produce one assertion using
expect(stagingResult).toMatchObject({ ok: true, migrationsApplied: 1,
markerHash: c5Hash }) so the values parsed from applyStaging3 are validated
together; update the assertions that reference stagingResult, parseJsonOutput,
applyStaging3 and c5Hash accordingly and remove the separate expect(...).toBe()
calls.
In `@test/integration/test/cli-journeys/interleaved-db-update.e2e.test.ts`:
- Around line 114-121: Group the three separate assertions on apply3Result into
a single grouped assertion to improve clarity and reduce repetition: replace the
individual expects that check apply3Result.ok, apply3Result.migrationsApplied,
and apply3Result.markerHash with one assertion that compares apply3Result
against the expected shape/values (e.g., using toEqual or toMatchObject) so it
verifies { ok: true, migrationsApplied: 1, markerHash: c4Hash } in one
statement; update references to parseJsonOutput and apply3Result accordingly.
In `@test/integration/test/cli-journeys/migration-plan-details.e2e.test.ts`:
- Around line 64-70: Group the related envelope assertions on the result object
into a single toMatchObject assertion instead of multiple expect calls: replace
the individual checks of result.ok, result.noOp, result.from, result.to,
result.migrationId and result.dir with one expect(result).toMatchObject({...})
that asserts ok: true, noOp: false, from: EMPTY_CONTRACT_HASH and uses generic
matchers (e.g., expect.anything() or expect.any(String)) for to, migrationId and
dir; keep the separate expect(result.operations.length).toBeGreaterThan(0) check
for operations.
In `@test/integration/test/cli-journeys/ref-routing.e2e.test.ts`:
- Around line 89-96: Group the three related assertions about the staging apply
result into a single assertion to make the intent clearer and reduce repetition:
locate where parseJsonOutput is used to create applyStagingResult and replace
the three expects on applyStagingResult.ok, .migrationsApplied, and .markerHash
with one compound assertion (e.g., using toMatchObject or toEqual) that checks {
ok: true, migrationsApplied: 1, markerHash: c2Hash } so the verification of
parseJsonOutput, migrationsApplied, and c2Hash occurs in one concise check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: c3a273e3-ac82-4a78-a257-b3b3a252d498
⛔ Files ignored due to path filters (8)
projects/on-disk-migrations-v2/assets/.gitkeepis excluded by!projects/**projects/on-disk-migrations-v2/issue-triage.mdis excluded by!projects/**projects/on-disk-migrations-v2/plan.mdis excluded by!projects/**projects/on-disk-migrations-v2/plans/.gitkeepis excluded by!projects/**projects/on-disk-migrations-v2/plans/cli-output-pipeline-plan.mdis excluded by!projects/**projects/on-disk-migrations-v2/plans/pr-232-review-fixes-plan.mdis excluded by!projects/**projects/on-disk-migrations-v2/spec.mdis excluded by!projects/**projects/on-disk-migrations-v2/specs/.gitkeepis excluded by!projects/**
📒 Files selected for processing (62)
.agents/skills/drive-note-issue/SKILL.mddocs/architecture docs/adrs/ADR 169 - On-disk migration persistence.mddocs/architecture docs/subsystems/7. Migration System.mdpackages/1-framework/3-tooling/cli/README.mdpackages/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-plan.tspackages/1-framework/3-tooling/cli/src/commands/migration-ref.tspackages/1-framework/3-tooling/cli/src/commands/migration-status.tspackages/1-framework/3-tooling/cli/src/utils/formatters/migrations.tspackages/1-framework/3-tooling/cli/src/utils/migration-types.tspackages/1-framework/3-tooling/cli/test/commands/migration-apply.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-e2e.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-plan.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-ref.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-show.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-status.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-verify.test.tspackages/1-framework/3-tooling/cli/test/output.json-shapes.test.tspackages/1-framework/3-tooling/cli/tsdown.config.tspackages/1-framework/3-tooling/cli/vitest.config.tspackages/1-framework/3-tooling/migration/package.jsonpackages/1-framework/3-tooling/migration/src/dag.tspackages/1-framework/3-tooling/migration/src/errors.tspackages/1-framework/3-tooling/migration/src/exports/dag.tspackages/1-framework/3-tooling/migration/src/exports/refs.tspackages/1-framework/3-tooling/migration/src/io.tspackages/1-framework/3-tooling/migration/src/refs.tspackages/1-framework/3-tooling/migration/src/types.tspackages/1-framework/3-tooling/migration/test/attestation.test.tspackages/1-framework/3-tooling/migration/test/dag.test.tspackages/1-framework/3-tooling/migration/test/fixtures.tspackages/1-framework/3-tooling/migration/test/refs.test.tspackages/1-framework/3-tooling/migration/test/scenarios.test.tspackages/1-framework/3-tooling/migration/tsdown.config.tspackages/2-sql/3-tooling/family/src/core/migrations/contract-to-schema-ir.tspackages/2-sql/3-tooling/family/src/exports/control.tspackages/2-sql/3-tooling/family/test/contract-to-schema-ir.test.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner.tspackages/3-targets/3-targets/postgres/src/exports/control.tspackages/3-targets/3-targets/postgres/test/migrations/planner.contract-to-schema-ir.test.tstest/integration/test/cli-journeys/README.mdtest/integration/test/cli-journeys/adopt-migrations.e2e.test.tstest/integration/test/cli-journeys/converging-paths.e2e.test.tstest/integration/test/cli-journeys/diamond-convergence.e2e.test.tstest/integration/test/cli-journeys/divergence-and-refs.e2e.test.tstest/integration/test/cli-journeys/drift-deleted-root.e2e.test.tstest/integration/test/cli-journeys/interleaved-db-update.e2e.test.tstest/integration/test/cli-journeys/migration-apply-edge-cases.e2e.test.tstest/integration/test/cli-journeys/migration-plan-details.e2e.test.tstest/integration/test/cli-journeys/ref-routing.e2e.test.tstest/integration/test/cli-journeys/rollback-cycle.e2e.test.tstest/integration/test/cli.migration-apply.e2e.test.tstest/integration/test/cli.migration-plan.e2e.test.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-additive-required.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-all.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-avatar.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-bio.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-phone-bio.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-phone.tstest/integration/test/utils/journey-test-helpers.ts
💤 Files with no reviewable changes (8)
- packages/1-framework/3-tooling/cli/test/commands/migration-verify.test.ts
- packages/1-framework/3-tooling/migration/test/attestation.test.ts
- packages/1-framework/3-tooling/cli/test/commands/migration-show.test.ts
- packages/1-framework/3-tooling/migration/src/io.ts
- packages/1-framework/3-tooling/migration/src/types.ts
- packages/1-framework/3-tooling/migration/test/fixtures.ts
- test/integration/test/cli.migration-apply.e2e.test.ts
- test/integration/test/cli.migration-plan.e2e.test.ts
✅ Files skipped from review due to trivial changes (1)
- .agents/skills/drive-note-issue/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/1-framework/3-tooling/cli/package.json
- packages/1-framework/3-tooling/cli/test/commands/migration-e2e.test.ts
- test/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-additive-required.ts
- packages/1-framework/3-tooling/cli/src/cli.ts
- packages/1-framework/3-tooling/cli/test/commands/migration-ref.test.ts
- test/integration/test/cli-journeys/README.md
- packages/1-framework/3-tooling/migration/src/exports/refs.ts
- packages/2-sql/3-tooling/family/src/exports/control.ts
- packages/1-framework/3-tooling/migration/tsdown.config.ts
- docs/architecture docs/subsystems/7. Migration System.md
- packages/3-targets/3-targets/postgres/test/migrations/planner.contract-to-schema-ir.test.ts
- test/integration/test/cli-journeys/rollback-cycle.e2e.test.ts
- test/integration/test/cli-journeys/adopt-migrations.e2e.test.ts
- packages/1-framework/3-tooling/cli/test/output.json-shapes.test.ts
- test/integration/test/cli-journeys/converging-paths.e2e.test.ts
- packages/1-framework/3-tooling/cli/test/commands/migration-plan.test.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/planner.ts
Comment on lines
+184
to
+186
| if (sorted[0] && sorted[0].migrationId === edge.migrationId) { | ||
| if (reachable.some((e) => e.migrationId !== edge.migrationId)) { | ||
| tieBreakReasons.push( |
There was a problem hiding this comment.
Tie-break attribution should compare edge identity, not migrationId.
At Line 184, sorted[0].migrationId === edge.migrationId can misclassify the selected edge when IDs are null/equal across different edges, producing incorrect tieBreakReasons.
Suggested fix
- if (sorted[0] && sorted[0].migrationId === edge.migrationId) {
- if (reachable.some((e) => e.migrationId !== edge.migrationId)) {
+ if (sorted[0] === edge) {
+ if (reachable.some((e) => e !== edge)) {
tieBreakReasons.push(
`at ${edge.from}: ${reachable.length} candidates, selected by tie-break`,
);
}
}🤖 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/dag.ts` around lines 184 - 186,
The tie-break logic currently compares migrationId values (sorted[0].migrationId
=== edge.migrationId), which can misidentify edges when IDs are null/duplicate;
change the comparison to test edge identity instead (e.g., use strict reference
equality sorted[0] === edge or compare a unique edge identifier if one exists)
so the tieBreakReasons push only occurs for the exact edge object selected;
update the conditional and any related comments to reflect identity-based
comparison.
Comment on lines
+58
to
59
| renderDefault: DefaultRenderer | undefined, | ||
| ): SqlColumnIR { |
There was a problem hiding this comment.
Make renderDefault required to avoid silent default loss.
Line [189] makes renderDefault optional, and Line [71]-[74] drops column defaults when it is missing. That creates silent IR drift instead of a hard failure when callers forget to pass a renderer.
Proposed fix
export interface ContractToSchemaIROptions {
readonly annotationNamespace: string;
readonly expandNativeType?: NativeTypeExpander;
- readonly renderDefault?: DefaultRenderer;
+ readonly renderDefault: DefaultRenderer;
readonly frameworkComponents?: ReadonlyArray<TargetBoundComponentDescriptor<'sql', string>>;
} function convertColumn(
name: string,
column: StorageColumn,
expandNativeType: NativeTypeExpander | undefined,
- renderDefault: DefaultRenderer | undefined,
+ renderDefault: DefaultRenderer,
): SqlColumnIR {
@@
...ifDefined(
'default',
- column.default != null && renderDefault ? renderDefault(column.default, column) : undefined,
+ column.default != null ? renderDefault(column.default, column) : undefined,
),
};
}Also applies to: 71-74, 186-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/2-sql/3-tooling/family/src/core/migrations/contract-to-schema-ir.ts`
around lines 58 - 59, The function currently accepts renderDefault as an
optional DefaultRenderer which leads to silent dropping of column defaults; make
renderDefault a required parameter (change its type from DefaultRenderer |
undefined to DefaultRenderer) in the function that returns SqlColumnIR, remove
the branches that treat renderDefault as missing and instead replace them with
explicit use of renderDefault or throw a clear error if a default cannot be
rendered, and update any internal code paths (the logic that sets column.default
/ drops defaults) so they call renderDefault directly rather than skipping
defaults when it's undefined; reference the symbols renderDefault,
DefaultRenderer, and SqlColumnIR to locate and update the signature and the
default-rendering branches accordingly.
e68fe16 to
d75ef75
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
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-plan.ts (1)
176-188:⚠️ Potential issue | 🟠 Major
--from sha256:emptyis rejected even though it's a valid explicit origin.This branch requires an existing migration whose
tomatches the requested hash, somigration plan --from sha256:emptyalways fails with "Starting contract not found". The explicit-origin path should acceptEMPTY_CONTRACT_HASHdirectly, and for non-empty hashes it can reconstruct from either side of an edge because packages embed both contracts.🛠️ Suggested fix
if (options.from) { fromHash = options.from; - const sourcePkg = packages.find((p) => p.manifest.to === fromHash); - if (!sourcePkg) { - return notOk( - errorRuntime('Starting contract not found', { - why: `No migration with to="${fromHash}" exists in ${migrationsRelative}`, - fix: 'Check that the --from hash matches a known migration target hash, or omit --from to use the latest migration leaf.', - }), - ); - } - fromContract = sourcePkg.manifest.toContract; + if (fromHash !== EMPTY_CONTRACT_HASH) { + const sourcePkg = + packages.find((p) => p.manifest.to === fromHash) ?? + packages.find((p) => p.manifest.from === fromHash); + if (!sourcePkg) { + return notOk( + errorRuntime('Starting contract not found', { + why: `No migration containing hash "${fromHash}" exists in ${migrationsRelative}`, + fix: 'Check that the --from hash matches a known migration endpoint, or omit --from to use the latest migration leaf.', + }), + ); + } + fromContract = + sourcePkg.manifest.to === fromHash + ? sourcePkg.manifest.toContract + : sourcePkg.manifest.fromContract; + } } else {🤖 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 176 - 188, The branch that handles options.from should accept the explicit EMPTY_CONTRACT_HASH and also match the requested hash against either side of an embedded migration edge; update the logic so: if options.from === EMPTY_CONTRACT_HASH then set fromHash = EMPTY_CONTRACT_HASH and fromContract = EMPTY_CONTRACT_HASH; otherwise set fromHash = options.from, find sourcePkg by matching (p.manifest.to === fromHash || p.manifest.from === fromHash), and when found set fromContract to the corresponding contract field on the matched side (use p.manifest.toContract if you matched .to, or p.manifest.fromContract if you matched .from); preserve the existing notOk error when no package matches.
♻️ Duplicate comments (4)
packages/2-sql/3-tooling/family/test/contract-to-schema-ir.test.ts (1)
23-26:⚠️ Potential issue | 🟡 MinorJSON literals with embedded apostrophes produce invalid SQL.
While this is test-only code,
testRendererdoesn't escape single quotes in JSON output. For{ name: "O'Reilly" }, the SQL becomes'{"name":"O'Reilly"}'which breaks SQL parsing.Proposed fix
const json = JSON.stringify(value); + const escaped = json.replaceAll("'", "''"); const isJsonColumn = column.nativeType === 'json' || column.nativeType === 'jsonb'; - if (isJsonColumn) return `'${json}'::${column.nativeType}`; - return `'${json}'`; + if (isJsonColumn) return `'${escaped}'::${column.nativeType}`; + return `'${escaped}'`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/3-tooling/family/test/contract-to-schema-ir.test.ts` around lines 23 - 26, The JSON string interpolation isn't escaping single quotes, causing invalid SQL for values like {name: "O'Reilly"}; update the code that builds the SQL literal (the json variable used with isJsonColumn and column.nativeType) to escape single quotes by replacing each ' with two single quotes (e.g. json.replace(/'/g, "''")) before wrapping in quotes and applying the ::json/::jsonb cast so the produced SQL is valid; ensure the replacement is applied in both the json and json-with-cast branches.packages/2-sql/3-tooling/family/src/core/migrations/contract-to-schema-ir.ts (1)
57-58:⚠️ Potential issue | 🟠 MajorOptional
renderDefaultsilently drops column defaults.When
renderDefaultisundefined, line 73 producesundefinedfor any column with a default, effectively stripping defaults from the IR. This silent behavior risks IR drift when callers omit the parameter.Consider making
renderDefaultrequired inContractToSchemaIROptionsand updating all call sites. If optionality is intentional for backward compatibility, add explicit documentation and consider logging a warning.Also applies to: 71-74, 189-189
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/3-tooling/family/src/core/migrations/contract-to-schema-ir.ts` around lines 57 - 58, The ContractToSchemaIROptions currently allows renderDefault to be undefined which causes contract-to-schema conversion to drop column defaults (line referencing renderDefault usage) — make renderDefault required on ContractToSchemaIROptions (remove undefined union) and update all call sites (e.g., places constructing ContractToSchemaIROptions and any calls to contractToSchemaIR) to pass a DefaultRenderer; if you must preserve optionality for compatibility, add clear docs on the option and add an explicit branch where renderDefault is undefined that either logs a warning or throws so defaults are not silently dropped (update the code paths around renderDefault usage at the sites noted).packages/1-framework/3-tooling/cli/src/commands/migration-status.ts (1)
184-203:⚠️ Potential issue | 🟠 Major
resolveDisplayChain()still breaks valid non-empty-root histories.Line 185 falls back to
EMPTY_CONTRACT_HASHwhentoMarkeris null. For histories rooted at a non-empty hash (e.g., planned from--from), this can make Line 406 fail with “Cannot reconstruct migration chain” even when marker/target are on a valid lineage.🛠️ Proposed fix
export function resolveDisplayChain( graph: MigrationGraph, targetHash: string, markerHash: string | undefined, ): readonly MigrationChainEntry[] | null { if (markerHash === undefined || markerHash === EMPTY_CONTRACT_HASH) { return findPath(graph, EMPTY_CONTRACT_HASH, targetHash); } const toMarker = findPath(graph, EMPTY_CONTRACT_HASH, markerHash); - if (!toMarker) return findPath(graph, EMPTY_CONTRACT_HASH, targetHash); + if (!toMarker) { + if (markerHash === targetHash) return []; + const fromMarker = findPath(graph, markerHash, targetHash); + if (fromMarker) return fromMarker; + const targetToMarker = findPath(graph, targetHash, markerHash); + if (targetToMarker) return targetToMarker; + return findPath(graph, EMPTY_CONTRACT_HASH, targetHash); + } if (markerHash === targetHash) return toMarker;export function buildMigrationEntries( chain: readonly MigrationChainEntry[], packages: readonly MigrationPackage[], markerHash: string | undefined, ): MigrationStatusEntry[] { const pkgByDirName = new Map(packages.map((p) => [p.dirName, p])); + const chainStartsAtMarker = markerHash !== undefined && chain[0]?.from === markerHash; const markerInChain = markerHash === undefined || markerHash === EMPTY_CONTRACT_HASH || + chainStartsAtMarker || chain.some((e) => e.to === markerHash); const entries: MigrationStatusEntry[] = []; - let reachedMarker = markerHash === undefined || markerHash === EMPTY_CONTRACT_HASH; + let reachedMarker = + markerHash === undefined || markerHash === EMPTY_CONTRACT_HASH || chainStartsAtMarker;Also applies to: 406-413
🤖 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 184 - 203, resolveDisplayChain currently always uses EMPTY_CONTRACT_HASH as the baseline when calling findPath (e.g., to build toMarker/toTarget), which breaks histories rooted at a non-empty hash; change the calls that use EMPTY_CONTRACT_HASH (the toMarker and toTarget lookups) to use the actual root/start hash passed into resolveDisplayChain (the function's initial/root parameter) so baseline paths are resolved from the true history root rather than EMPTY_CONTRACT_HASH, and keep the rest of the markerHash/targetHash logic (fromMarker, targetToMarker) intact so disconnected vs. ahead/behind cases still behave the same.packages/1-framework/3-tooling/migration/src/dag.ts (1)
184-190:⚠️ Potential issue | 🟡 MinorTie-break attribution still compares
migrationIdinstead of edge identity.The past review flagged that comparing
sorted[0].migrationId === edge.migrationIdcan misattribute the tie-break reason when multiple edges share the samemigrationId(includingnull). The comparison should use reference equality to ensure the selected edge is correctly identified.🔧 Proposed fix using reference equality
- if (sorted[0] && sorted[0].migrationId === edge.migrationId) { - if (reachable.some((e) => e.migrationId !== edge.migrationId)) { + if (sorted[0] === edge) { + if (reachable.some((e) => e !== edge)) { tieBreakReasons.push( `at ${edge.from}: ${reachable.length} candidates, selected by tie-break`, ); } }🤖 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/dag.ts` around lines 184 - 190, The tie-break attribution is incorrectly comparing migrationId values (sorted[0].migrationId === edge.migrationId) which can misattribute when multiple edges share the same migrationId; update the condition in the block that pushes into tieBreakReasons to use reference equality against the selected edge (i.e., compare the edge objects themselves, e.g. sorted[0] === edge) so the tie-break message is only added when the actual edge instance matches the chosen candidate; keep the surrounding logic that checks reachable.some(...) and the tieBreakReasons.push call unchanged.
🧹 Nitpick comments (5)
packages/1-framework/3-tooling/cli/test/output.json-shapes.test.ts (1)
5-188: These JSON-shape tests are self-fulfilling.Each assertion snapshots
Object.keys()from an object literal created in the test, so it proves the fixture has those keys, not that the command/formatter actually emits them. That leaves the public JSON contract effectively untested for omitted or extra fields at serialization time.As per coding guidelines, "Avoid tautological tests that only restate fixture input - tests must verify behavior, not mirror object shape passed by the test itself."
🤖 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/output.json-shapes.test.ts` around lines 5 - 188, The tests currently assert Object.keys() on hard-coded MigrationApplyResult and MigrationStatusResult literals (tautological); update each spec in the "MigrationApplyResult JSON shape" and "MigrationStatusResult JSON shape" suites to exercise the real serialization/formatter path instead of inspecting the fixture: construct the same result data, pass it through the actual serializer/formatter or the CLI handler used by production (e.g., call the module/function that emits JSON for MigrationApplyResult / MigrationStatusResult or invoke the CLI command that prints JSON), parse the emitted JSON and then assert the keys on the parsed output (and for pathDecision/asserted nested objects use parsed.pathDecision), so the test verifies what is actually emitted rather than the input literal.packages/1-framework/3-tooling/cli/src/commands/migration-status.ts (1)
57-57: Avoid re-exporting types from command modules.Line 57 re-exports types from another file. This file should import/use those types, while re-exports belong in dedicated exports surfaces only.
♻️ Proposed change
-export type { StatusDiagnostic, StatusRef } from '../utils/migration-types';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/1-framework/3-tooling/cli/src/commands/migration-status.ts` at line 57, This file incorrectly re-exports types StatusDiagnostic and StatusRef from '../utils/migration-types'; remove the export statement and instead import and use those types locally in migration-status.ts (e.g., replace "export type { StatusDiagnostic, StatusRef } ..." with an import-only variant) so the module consumes the types but does not re-export them — re-exports should live only in the dedicated exports surface.packages/1-framework/3-tooling/cli/test/commands/migration-status.test.ts (2)
1-3: Usepatheinstead ofnode:pathfor consistent path handling.Per repository guidelines, prefer
patheovernode:pathfor path operations to ensure consistent behavior across platforms and runtimes.♻️ Proposed fix
import { mkdir, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; -import { join } from 'node:path'; +import { join } from 'pathe';Based on learnings: "In the Prisma-next repository, prefer importing and using 'pathe' over the built-in 'node:path' module for path operations across the codebase."
🤖 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/commands/migration-status.test.ts` around lines 1 - 3, Replace the node:path import with pathe to follow repo guidelines: change the import of join from "node:path" to import from "pathe" and ensure any path usages in migration-status.test.ts that call join (and other path helpers if added) use the pathe implementation; keep existing mkdir/writeFile/tmpdir imports as-is and run tests to confirm no runtime/path differences.
1-846: Consider splitting this test file (846 lines exceeds 500-line guideline).The file contains 5 distinct test groups that could be split by functionality:
migration-status.entries.test.ts-buildMigrationEntriestestsmigration-status.formatting.test.ts-formatMigrationStatusOutputtestsmigration-status.chain.test.ts-resolveDisplayChaintestsmigration-status.refs.test.ts-readRefserror surface testsThis would improve navigability and keep each file under 500 lines.
🤖 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/commands/migration-status.test.ts` around lines 1 - 846, The test file is too large and should be split into smaller, focused test files; extract the describe blocks for buildMigrationEntries, formatMigrationStatusOutput, resolveDisplayChain, and readRefs into separate files (suggested names: migration-status.entries.test.ts, migration-status.formatting.test.ts, migration-status.chain.test.ts, migration-status.refs.test.ts). Move shared helpers (createTestContract, createManifest, createOp, createTempDir, setupChain) into a common test helper module and import them from each new test file; ensure each new file imports the specific functions under test (buildMigrationEntries, formatMigrationStatusOutput, resolveDisplayChain, readRefs) and any constants (EMPTY_CONTRACT_HASH) used. Update top-level imports (node:fs/promises, tmpdir, join, strip-ansi, vitest utilities, timeouts) only in files that need them and keep test semantics and timeouts unchanged. Run the test suite to confirm no import/export or scope regressions and adjust exports from the helper module if any utilities are currently file-local.packages/1-framework/3-tooling/migration/src/dag.ts (1)
174-180: Quadratic reachability checks infindPathWithDecision.For each edge on the path,
findPathis called for every outgoing edge candidate. This is O(path_length × branching_factor × graph_size). For typical migration graphs this is fine, but could become slow with very large graphs (hundreds of migrations with high branching).Consider caching reachability or computing it once via reverse BFS from
toHashif performance becomes an issue in practice.🤖 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/dag.ts` around lines 174 - 180, The loop in findPathWithDecision repeatedly calls findPath for each outgoing edge causing quadratic work; instead compute the set of nodes that can reach toHash once (e.g., run a reverse BFS on graph.reverseChain starting from toHash to build a reachable Set), then inside the for (const edge of path) and for each outgoing use reachable.has(e.to) (and still check e.to === toHash) rather than invoking findPath repeatedly; update references to graph.forwardChain, graph.reverseChain, toHash, path, outgoing, and the reachable variable to locate where to change the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/architecture` docs/adrs/ADR 169 - On-disk migration persistence.md:
- Line 49: The sentence under "Branch detection" incorrectly treats any two
edges with the same `from` and different `to` as an `AMBIGUOUS_LEAF`; update the
wording to say that multiple outgoing edges from the same `from` hash are
allowed to reconverge, and that `AMBIGUOUS_LEAF` applies only when traversal
from that `from` actually results in multiple distinct reachable leaves (i.e.,
the paths do not reconverge), keeping the explicit resolution requirement for
true ambiguous-leaf cases and referencing the `from`/`to` hashes and
`AMBIGUOUS_LEAF` symbol.
In `@docs/architecture` docs/subsystems/7. Migration System.md:
- Around line 147-149: The "Model" section earlier must be updated to match the
Graph-based ordering paragraph: replace any statement that the on-disk edges
form a DAG with wording that the storage edges are a directed graph (not
necessarily acyclic) allowing rollback cycles (e.g., C1 -> C2 -> C1); mention
that traversal/ordering uses BFS shortest-path with deterministic tie-breaking
(label priority → createdAt ascending → to lexicographic → edgeId lexicographic)
and that cycles are handled via visited-node tracking, and reference
MIGRATION.AMBIGUOUS_LEAF and ADR 169 as in the Graph-based ordering section so
the two descriptions are consistent.
In `@packages/1-framework/3-tooling/cli/README.md`:
- Around line 888-889: Update the README text that currently says "graph leaf
(latest migration)" to clarify DAG semantics: explain that if --from is omitted
the CLI uses a resolvable graph leaf (i.e., a single unambiguous leaf) and, in
cases of multiple leaves or no resolvable leaf, it either requires an explicit
--from <hash> or emits an explicit error; reference the flags and terms shown
("--from <hash>", "graph leaf") so readers understand the behavior in
non-linear/DAG mode and replace "latest migration" with "resolvable leaf (single
unambiguous leaf) or fail/require explicit --from".
In `@packages/1-framework/3-tooling/migration/src/refs.ts`:
- Around line 88-99: The code indexes refs[name] before verifying ownership, so
inherited keys (e.g., "constructor") can be treated as present; change the logic
to first check ownership using Object.hasOwn(refs, name) and throw the
MigrationToolsError('MIGRATION.UNKNOWN_REF', ...) when it returns false, then
read the value into hash and continue to validateRefValue(hash) and call
errorInvalidRefValue(hash) as before; update the block around refs[name],
validateRefValue, errorInvalidRefValue, and the MigrationToolsError creation so
the ownership check happens before any indexing or validation.
In `@test/integration/test/cli-journeys/drift-deleted-root.e2e.test.ts`:
- Around line 57-66: The test treats any non-hidden entry in migrationsDir as a
migration; update the two readdirSync usages to only list directories (e.g., use
readdirSync(migrationsDir, { withFileTypes: true }) and filter
Dirent.isDirectory() and names not starting with '.') so initDir and remaining
are computed from directories only; adjust references to initDir and remaining
accordingly so the assertions only consider migration directories.
- Around line 78-86: Capture the state of the migrations directory before
calling runMigrationPlan (e.g., list filenames or take a directory snapshot of
migrations/) and after the call, assert they are identical; specifically, before
invoking runMigrationPlan(ctx, ['--name', 'should-fail']) record the migrations/
contents, then after planAgain completes (and you already assert
planAgain.exitCode !== 0) compare the post-run migrations/ snapshot to the
pre-run snapshot and fail the test if any new files (such as a duplicate init
migration) were created; update the test near runMigrationPlan/planAgain to
perform this pre/post directory comparison.
---
Outside diff comments:
In `@packages/1-framework/3-tooling/cli/src/commands/migration-plan.ts`:
- Around line 176-188: The branch that handles options.from should accept the
explicit EMPTY_CONTRACT_HASH and also match the requested hash against either
side of an embedded migration edge; update the logic so: if options.from ===
EMPTY_CONTRACT_HASH then set fromHash = EMPTY_CONTRACT_HASH and fromContract =
EMPTY_CONTRACT_HASH; otherwise set fromHash = options.from, find sourcePkg by
matching (p.manifest.to === fromHash || p.manifest.from === fromHash), and when
found set fromContract to the corresponding contract field on the matched side
(use p.manifest.toContract if you matched .to, or p.manifest.fromContract if you
matched .from); preserve the existing notOk error when no package matches.
---
Duplicate comments:
In `@packages/1-framework/3-tooling/cli/src/commands/migration-status.ts`:
- Around line 184-203: resolveDisplayChain currently always uses
EMPTY_CONTRACT_HASH as the baseline when calling findPath (e.g., to build
toMarker/toTarget), which breaks histories rooted at a non-empty hash; change
the calls that use EMPTY_CONTRACT_HASH (the toMarker and toTarget lookups) to
use the actual root/start hash passed into resolveDisplayChain (the function's
initial/root parameter) so baseline paths are resolved from the true history
root rather than EMPTY_CONTRACT_HASH, and keep the rest of the
markerHash/targetHash logic (fromMarker, targetToMarker) intact so disconnected
vs. ahead/behind cases still behave the same.
In `@packages/1-framework/3-tooling/migration/src/dag.ts`:
- Around line 184-190: The tie-break attribution is incorrectly comparing
migrationId values (sorted[0].migrationId === edge.migrationId) which can
misattribute when multiple edges share the same migrationId; update the
condition in the block that pushes into tieBreakReasons to use reference
equality against the selected edge (i.e., compare the edge objects themselves,
e.g. sorted[0] === edge) so the tie-break message is only added when the actual
edge instance matches the chosen candidate; keep the surrounding logic that
checks reachable.some(...) and the tieBreakReasons.push call unchanged.
In
`@packages/2-sql/3-tooling/family/src/core/migrations/contract-to-schema-ir.ts`:
- Around line 57-58: The ContractToSchemaIROptions currently allows
renderDefault to be undefined which causes contract-to-schema conversion to drop
column defaults (line referencing renderDefault usage) — make renderDefault
required on ContractToSchemaIROptions (remove undefined union) and update all
call sites (e.g., places constructing ContractToSchemaIROptions and any calls to
contractToSchemaIR) to pass a DefaultRenderer; if you must preserve optionality
for compatibility, add clear docs on the option and add an explicit branch where
renderDefault is undefined that either logs a warning or throws so defaults are
not silently dropped (update the code paths around renderDefault usage at the
sites noted).
In `@packages/2-sql/3-tooling/family/test/contract-to-schema-ir.test.ts`:
- Around line 23-26: The JSON string interpolation isn't escaping single quotes,
causing invalid SQL for values like {name: "O'Reilly"}; update the code that
builds the SQL literal (the json variable used with isJsonColumn and
column.nativeType) to escape single quotes by replacing each ' with two single
quotes (e.g. json.replace(/'/g, "''")) before wrapping in quotes and applying
the ::json/::jsonb cast so the produced SQL is valid; ensure the replacement is
applied in both the json and json-with-cast branches.
---
Nitpick comments:
In `@packages/1-framework/3-tooling/cli/src/commands/migration-status.ts`:
- Line 57: This file incorrectly re-exports types StatusDiagnostic and StatusRef
from '../utils/migration-types'; remove the export statement and instead import
and use those types locally in migration-status.ts (e.g., replace "export type {
StatusDiagnostic, StatusRef } ..." with an import-only variant) so the module
consumes the types but does not re-export them — re-exports should live only in
the dedicated exports surface.
In `@packages/1-framework/3-tooling/cli/test/commands/migration-status.test.ts`:
- Around line 1-3: Replace the node:path import with pathe to follow repo
guidelines: change the import of join from "node:path" to import from "pathe"
and ensure any path usages in migration-status.test.ts that call join (and other
path helpers if added) use the pathe implementation; keep existing
mkdir/writeFile/tmpdir imports as-is and run tests to confirm no runtime/path
differences.
- Around line 1-846: The test file is too large and should be split into
smaller, focused test files; extract the describe blocks for
buildMigrationEntries, formatMigrationStatusOutput, resolveDisplayChain, and
readRefs into separate files (suggested names: migration-status.entries.test.ts,
migration-status.formatting.test.ts, migration-status.chain.test.ts,
migration-status.refs.test.ts). Move shared helpers (createTestContract,
createManifest, createOp, createTempDir, setupChain) into a common test helper
module and import them from each new test file; ensure each new file imports the
specific functions under test (buildMigrationEntries,
formatMigrationStatusOutput, resolveDisplayChain, readRefs) and any constants
(EMPTY_CONTRACT_HASH) used. Update top-level imports (node:fs/promises, tmpdir,
join, strip-ansi, vitest utilities, timeouts) only in files that need them and
keep test semantics and timeouts unchanged. Run the test suite to confirm no
import/export or scope regressions and adjust exports from the helper module if
any utilities are currently file-local.
In `@packages/1-framework/3-tooling/cli/test/output.json-shapes.test.ts`:
- Around line 5-188: The tests currently assert Object.keys() on hard-coded
MigrationApplyResult and MigrationStatusResult literals (tautological); update
each spec in the "MigrationApplyResult JSON shape" and "MigrationStatusResult
JSON shape" suites to exercise the real serialization/formatter path instead of
inspecting the fixture: construct the same result data, pass it through the
actual serializer/formatter or the CLI handler used by production (e.g., call
the module/function that emits JSON for MigrationApplyResult /
MigrationStatusResult or invoke the CLI command that prints JSON), parse the
emitted JSON and then assert the keys on the parsed output (and for
pathDecision/asserted nested objects use parsed.pathDecision), so the test
verifies what is actually emitted rather than the input literal.
In `@packages/1-framework/3-tooling/migration/src/dag.ts`:
- Around line 174-180: The loop in findPathWithDecision repeatedly calls
findPath for each outgoing edge causing quadratic work; instead compute the set
of nodes that can reach toHash once (e.g., run a reverse BFS on
graph.reverseChain starting from toHash to build a reachable Set), then inside
the for (const edge of path) and for each outgoing use reachable.has(e.to) (and
still check e.to === toHash) rather than invoking findPath repeatedly; update
references to graph.forwardChain, graph.reverseChain, toHash, path, outgoing,
and the reachable variable to locate where to change the logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 721771a8-9e85-42bf-b0a8-ab3ef1bb2b59
⛔ Files ignored due to path filters (8)
projects/on-disk-migrations-v2/assets/.gitkeepis excluded by!projects/**projects/on-disk-migrations-v2/issue-triage.mdis excluded by!projects/**projects/on-disk-migrations-v2/plan.mdis excluded by!projects/**projects/on-disk-migrations-v2/plans/.gitkeepis excluded by!projects/**projects/on-disk-migrations-v2/plans/cli-output-pipeline-plan.mdis excluded by!projects/**projects/on-disk-migrations-v2/plans/pr-232-review-fixes-plan.mdis excluded by!projects/**projects/on-disk-migrations-v2/spec.mdis excluded by!projects/**projects/on-disk-migrations-v2/specs/.gitkeepis excluded by!projects/**
📒 Files selected for processing (61)
docs/architecture docs/adrs/ADR 169 - On-disk migration persistence.mddocs/architecture docs/subsystems/7. Migration System.mdpackages/1-framework/3-tooling/cli/README.mdpackages/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-plan.tspackages/1-framework/3-tooling/cli/src/commands/migration-ref.tspackages/1-framework/3-tooling/cli/src/commands/migration-status.tspackages/1-framework/3-tooling/cli/src/utils/formatters/migrations.tspackages/1-framework/3-tooling/cli/src/utils/migration-types.tspackages/1-framework/3-tooling/cli/test/commands/migration-apply.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-e2e.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-plan.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-ref.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-show.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-status.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-verify.test.tspackages/1-framework/3-tooling/cli/test/output.json-shapes.test.tspackages/1-framework/3-tooling/cli/tsdown.config.tspackages/1-framework/3-tooling/cli/vitest.config.tspackages/1-framework/3-tooling/migration/package.jsonpackages/1-framework/3-tooling/migration/src/dag.tspackages/1-framework/3-tooling/migration/src/errors.tspackages/1-framework/3-tooling/migration/src/exports/dag.tspackages/1-framework/3-tooling/migration/src/exports/refs.tspackages/1-framework/3-tooling/migration/src/io.tspackages/1-framework/3-tooling/migration/src/refs.tspackages/1-framework/3-tooling/migration/src/types.tspackages/1-framework/3-tooling/migration/test/attestation.test.tspackages/1-framework/3-tooling/migration/test/dag.test.tspackages/1-framework/3-tooling/migration/test/fixtures.tspackages/1-framework/3-tooling/migration/test/refs.test.tspackages/1-framework/3-tooling/migration/test/scenarios.test.tspackages/1-framework/3-tooling/migration/tsdown.config.tspackages/2-sql/3-tooling/family/src/core/migrations/contract-to-schema-ir.tspackages/2-sql/3-tooling/family/src/exports/control.tspackages/2-sql/3-tooling/family/test/contract-to-schema-ir.test.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner.tspackages/3-targets/3-targets/postgres/src/exports/control.tspackages/3-targets/3-targets/postgres/test/migrations/planner.contract-to-schema-ir.test.tstest/integration/test/cli-journeys/README.mdtest/integration/test/cli-journeys/adopt-migrations.e2e.test.tstest/integration/test/cli-journeys/converging-paths.e2e.test.tstest/integration/test/cli-journeys/diamond-convergence.e2e.test.tstest/integration/test/cli-journeys/divergence-and-refs.e2e.test.tstest/integration/test/cli-journeys/drift-deleted-root.e2e.test.tstest/integration/test/cli-journeys/interleaved-db-update.e2e.test.tstest/integration/test/cli-journeys/migration-apply-edge-cases.e2e.test.tstest/integration/test/cli-journeys/migration-plan-details.e2e.test.tstest/integration/test/cli-journeys/ref-routing.e2e.test.tstest/integration/test/cli-journeys/rollback-cycle.e2e.test.tstest/integration/test/cli.migration-apply.e2e.test.tstest/integration/test/cli.migration-plan.e2e.test.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-additive-required.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-all.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-avatar.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-bio.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-phone-bio.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-phone.tstest/integration/test/utils/journey-test-helpers.ts
💤 Files with no reviewable changes (8)
- packages/1-framework/3-tooling/cli/test/commands/migration-show.test.ts
- test/integration/test/cli.migration-apply.e2e.test.ts
- packages/1-framework/3-tooling/cli/test/commands/migration-verify.test.ts
- test/integration/test/cli.migration-plan.e2e.test.ts
- packages/1-framework/3-tooling/migration/src/types.ts
- packages/1-framework/3-tooling/migration/test/attestation.test.ts
- packages/1-framework/3-tooling/migration/src/io.ts
- packages/1-framework/3-tooling/migration/test/fixtures.ts
🚧 Files skipped from review as they are similar to previous changes (20)
- packages/1-framework/3-tooling/migration/src/exports/refs.ts
- test/integration/test/cli-journeys/divergence-and-refs.e2e.test.ts
- test/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-avatar.ts
- packages/1-framework/3-tooling/migration/test/scenarios.test.ts
- test/integration/test/cli-journeys/README.md
- packages/1-framework/3-tooling/cli/package.json
- packages/1-framework/3-tooling/cli/test/commands/migration-plan.test.ts
- test/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-bio.ts
- test/integration/test/cli-journeys/migration-apply-edge-cases.e2e.test.ts
- test/integration/test/cli-journeys/rollback-cycle.e2e.test.ts
- packages/1-framework/3-tooling/cli/test/commands/migration-ref.test.ts
- packages/1-framework/3-tooling/cli/src/commands/migration-ref.ts
- test/integration/test/cli-journeys/converging-paths.e2e.test.ts
- test/integration/test/cli-journeys/migration-plan-details.e2e.test.ts
- test/integration/test/cli-journeys/adopt-migrations.e2e.test.ts
- test/integration/test/cli-journeys/ref-routing.e2e.test.ts
- packages/1-framework/3-tooling/cli/test/commands/migration-e2e.test.ts
- test/integration/test/utils/journey-test-helpers.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/planner.ts
- packages/3-targets/3-targets/postgres/test/migrations/planner.contract-to-schema-ir.test.ts
Comment on lines
+888
to
+889
| 3. Determines the starting point: `--from <hash>` if provided, otherwise the graph leaf (latest migration) | ||
| 4. Diffs the starting contract against the new contract using the target's migration planner |
There was a problem hiding this comment.
Clarify non-linear DAG semantics in the starting-point description.
“graph leaf (latest migration)” reads as linear-chain behavior. In DAG mode, there may be multiple leaves or no resolvable leaf, so this should describe a resolvable leaf (or explicit error) rather than “latest migration.”
Suggested wording
-3. Determines the starting point: `--from <hash>` if provided, otherwise the graph leaf (latest migration)
+3. Determines the starting point: `--from <hash>` if provided; otherwise resolves a leaf from the migration graph (and errors if ambiguous/unresolvable)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 3. Determines the starting point: `--from <hash>` if provided, otherwise the graph leaf (latest migration) | |
| 4. Diffs the starting contract against the new contract using the target's migration planner | |
| 3. Determines the starting point: `--from <hash>` if provided; otherwise resolves a leaf from the migration graph (and errors if ambiguous/unresolvable) | |
| 4. Diffs the starting contract against the new contract using the target's migration planner |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/1-framework/3-tooling/cli/README.md` around lines 888 - 889, Update
the README text that currently says "graph leaf (latest migration)" to clarify
DAG semantics: explain that if --from is omitted the CLI uses a resolvable graph
leaf (i.e., a single unambiguous leaf) and, in cases of multiple leaves or no
resolvable leaf, it either requires an explicit --from <hash> or emits an
explicit error; reference the flags and terms shown ("--from <hash>", "graph
leaf") so readers understand the behavior in non-linear/DAG mode and replace
"latest migration" with "resolvable leaf (single unambiguous leaf) or
fail/require explicit --from".
Comment on lines
+88
to
+99
| const hash = refs[name]; | ||
| if (hash === undefined) { | ||
| throw new MigrationToolsError('MIGRATION.UNKNOWN_REF', `Unknown ref "${name}"`, { | ||
| why: `No ref named "${name}" exists in refs.json.`, | ||
| fix: `Available refs: ${Object.keys(refs).join(', ') || '(none)'}. Create a ref with: set the "${name}" key in migrations/refs.json.`, | ||
| details: { refName: name, availableRefs: Object.keys(refs) }, | ||
| }); | ||
| } | ||
|
|
||
| if (!validateRefValue(hash)) { | ||
| throw errorInvalidRefValue(hash); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the file and check if it exists
fd refs.ts packages/1-framework/3-tooling/migration/srcRepository: prisma/prisma-next
Length of output: 175
🏁 Script executed:
# Check the structure of the repository to understand paths better
git ls-files | grep -E "packages/.*migration.*refs" | head -20Repository: prisma/prisma-next
Length of output: 234
🏁 Script executed:
# Read the specific file to verify the code at lines 88-99
cat -n packages/1-framework/3-tooling/migration/src/refs.ts | sed -n '80,110p'Repository: prisma/prisma-next
Length of output: 911
🏁 Script executed:
# Also read the validateRefName function to check what it allows
cat -n packages/1-framework/3-tooling/migration/src/refs.ts | grep -A 10 "validateRefName"Repository: prisma/prisma-next
Length of output: 1903
🏁 Script executed:
# Find the REF_NAME_PATTERN definition
cat -n packages/1-framework/3-tooling/migration/src/refs.ts | head -30Repository: prisma/prisma-next
Length of output: 1319
🏁 Script executed:
# Check what REF_NAME_PATTERN actually is
rg "REF_NAME_PATTERN" packages/1-framework/3-tooling/migration/src/refs.ts -A 1 -B 1Repository: prisma/prisma-next
Length of output: 300
🏁 Script executed:
# Verify the exact behavior by testing the regex pattern
python3 << 'EOF'
import re
REF_NAME_PATTERN = r'^[a-z0-9]([a-z0-9-]*[a-z0-9])?(\/[a-z0-9]([a-z0-9-]*[a-z0-9])?)*$'
test_names = [
'constructor',
'toString',
'prototype',
'hasOwnProperty',
'valid-name',
'valid/name'
]
for name in test_names:
matches = bool(re.match(REF_NAME_PATTERN, name))
print(f"{name:25} matches: {matches}")
EOFRepository: prisma/prisma-next
Length of output: 303
🏁 Script executed:
# Check if there are any tests that cover this scenario
rg "resolveRef" packages/1-framework/3-tooling/migration/test/refs.test.ts -A 5 -B 2Repository: prisma/prisma-next
Length of output: 1547
🏁 Script executed:
# Verify the actual Refs type to understand how it's created from JSON
rg "type Refs" packages/1-framework/3-tooling/migration/src/refs.ts -A 2 -B 2Repository: prisma/prisma-next
Length of output: 229
🏁 Script executed:
# Check if there's a test for prototype key specifically
rg "constructor\|prototype" packages/1-framework/3-tooling/migration/test/refs.test.tsRepository: prisma/prisma-next
Length of output: 44
🏁 Script executed:
# Test the actual prototype pollution scenario with JavaScript
node << 'EOF'
const REF_NAME_PATTERN = /^[a-z0-9]([a-z0-9-]*[a-z0-9])?(\/[a-z0-9]([a-z0-9-]*[a-z0-9])?)*$/;
function validateRefName(name) {
if (name.length === 0) return false;
if (name.includes('..')) return false;
if (name.includes('//')) return false;
if (name.startsWith('.')) return false;
return REF_NAME_PATTERN.test(name);
}
// Simulate the vulnerability
const refs = { staging: 'sha256:' + 'a'.repeat(64) };
const testNames = ['constructor', 'prototype', '__proto__'];
for (const name of testNames) {
console.log(`\nTesting name: "${name}"`);
console.log(` validateRefName("${name}"): ${validateRefName(name)}`);
console.log(` refs["${name}"] === undefined: ${refs[name] === undefined}`);
console.log(` Object.hasOwn(refs, "${name}"): ${Object.hasOwn(refs, name)}`);
console.log(` typeof refs["${name}"]: ${typeof refs[name]}`);
}
EOFRepository: prisma/prisma-next
Length of output: 622
Use Object.hasOwn() to check ref ownership before indexing.
Prototype keys like constructor and prototype pass validateRefName() because they match the regex pattern. When indexing refs["constructor"], you access the inherited Function constructor instead of an own property, causing the code to incorrectly follow the INVALID_REF_VALUE error path instead of UNKNOWN_REF.
🔧 Minimal hardening
- const hash = refs[name];
- if (hash === undefined) {
+ if (!Object.hasOwn(refs, name)) {
throw new MigrationToolsError('MIGRATION.UNKNOWN_REF', `Unknown ref "${name}"`, {
why: `No ref named "${name}" exists in refs.json.`,
fix: `Available refs: ${Object.keys(refs).join(', ') || '(none)'}. Create a ref with: set the "${name}" key in migrations/refs.json.`,
details: { refName: name, availableRefs: Object.keys(refs) },
});
}
+ const hash = refs[name]!;Per coding guidelines: "Always use Object.hasOwn() instead of hasOwnProperty() to check for own properties."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const hash = refs[name]; | |
| if (hash === undefined) { | |
| throw new MigrationToolsError('MIGRATION.UNKNOWN_REF', `Unknown ref "${name}"`, { | |
| why: `No ref named "${name}" exists in refs.json.`, | |
| fix: `Available refs: ${Object.keys(refs).join(', ') || '(none)'}. Create a ref with: set the "${name}" key in migrations/refs.json.`, | |
| details: { refName: name, availableRefs: Object.keys(refs) }, | |
| }); | |
| } | |
| if (!validateRefValue(hash)) { | |
| throw errorInvalidRefValue(hash); | |
| } | |
| if (!Object.hasOwn(refs, name)) { | |
| throw new MigrationToolsError('MIGRATION.UNKNOWN_REF', `Unknown ref "${name}"`, { | |
| why: `No ref named "${name}" exists in refs.json.`, | |
| fix: `Available refs: ${Object.keys(refs).join(', ') || '(none)'}. Create a ref with: set the "${name}" key in migrations/refs.json.`, | |
| details: { refName: name, availableRefs: Object.keys(refs) }, | |
| }); | |
| } | |
| const hash = refs[name]!; | |
| if (!validateRefValue(hash)) { | |
| throw errorInvalidRefValue(hash); | |
| } |
🤖 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/refs.ts` around lines 88 - 99,
The code indexes refs[name] before verifying ownership, so inherited keys (e.g.,
"constructor") can be treated as present; change the logic to first check
ownership using Object.hasOwn(refs, name) and throw the
MigrationToolsError('MIGRATION.UNKNOWN_REF', ...) when it returns false, then
read the value into hash and continue to validateRefValue(hash) and call
errorInvalidRefValue(hash) as before; update the block around refs[name],
validateRefValue, errorInvalidRefValue, and the MigrationToolsError creation so
the ownership check happens before any indexing or validation.
Comment on lines
+57
to
+66
| const migrationsDir = join(ctx.testDir, 'migrations'); | ||
| const migrationDirs = readdirSync(migrationsDir).sort(); | ||
| const initDir = migrationDirs.find((d) => d.endsWith('_initial')); | ||
| expect(initDir, 'P4.pre: initial dir exists').toBeDefined(); | ||
| rmSync(join(migrationsDir, initDir!), { recursive: true, force: true }); | ||
|
|
||
| // Verify only add-name remains on disk | ||
| const remaining = readdirSync(migrationsDir).filter((d) => !d.startsWith('.')); | ||
| expect(remaining, 'P4.pre: only add-name remains').toHaveLength(1); | ||
| expect(remaining[0], 'P4.pre: remaining is add-name').toMatch(/_add_name$/); |
There was a problem hiding this comment.
Filter the migrations listing to directories.
Both readdirSync() calls currently treat any non-hidden entry under migrations/ as a migration. That makes this test brittle to auxiliary files and weakens the “only one migration remains” assertion.
Proposed fix
const migrationsDir = join(ctx.testDir, 'migrations');
- const migrationDirs = readdirSync(migrationsDir).sort();
+ const migrationDirs = readdirSync(migrationsDir, { withFileTypes: true })
+ .filter((entry) => entry.isDirectory() && !entry.name.startsWith('.'))
+ .map((entry) => entry.name)
+ .sort();
const initDir = migrationDirs.find((d) => d.endsWith('_initial'));
expect(initDir, 'P4.pre: initial dir exists').toBeDefined();
rmSync(join(migrationsDir, initDir!), { recursive: true, force: true });
// Verify only add-name remains on disk
- const remaining = readdirSync(migrationsDir).filter((d) => !d.startsWith('.'));
+ const remaining = readdirSync(migrationsDir, { withFileTypes: true })
+ .filter((entry) => entry.isDirectory() && !entry.name.startsWith('.'))
+ .map((entry) => entry.name)
+ .sort();
expect(remaining, 'P4.pre: only add-name remains').toHaveLength(1);
expect(remaining[0], 'P4.pre: remaining is add-name').toMatch(/_add_name$/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/integration/test/cli-journeys/drift-deleted-root.e2e.test.ts` around
lines 57 - 66, The test treats any non-hidden entry in migrationsDir as a
migration; update the two readdirSync usages to only list directories (e.g., use
readdirSync(migrationsDir, { withFileTypes: true }) and filter
Dirent.isDirectory() and names not starting with '.') so initDir and remaining
are computed from directories only; adjust references to initDir and remaining
accordingly so the assertions only consider migration directories.
Comment on lines
+78
to
+86
| const planAgain = await runMigrationPlan(ctx, ['--name', 'should-fail']); | ||
| const planOutput = stripAnsi(planAgain.stdout); | ||
|
|
||
| // The critical assertion: the planner must NOT succeed and silently | ||
| // create a new init migration as if no migrations existed | ||
| expect(planAgain.exitCode, 'P4.02: plan fails on broken graph').not.toBe(0); | ||
| expect(planOutput, 'P4.02: mentions orphan, broken chain, or disconnected graph').toMatch( | ||
| /orphan|broken|disconnect|no.*root|no.*path|unreachable/i, | ||
| ); |
There was a problem hiding this comment.
Assert that the failed plan leaves migrations/ unchanged.
P4.02 currently proves the command failed, but not that it avoided writing a duplicate init migration before failing. A post-plan directory snapshot would make that regression visible.
Proposed fix
const planAgain = await runMigrationPlan(ctx, ['--name', 'should-fail']);
const planOutput = stripAnsi(planAgain.stdout);
// The critical assertion: the planner must NOT succeed and silently
// create a new init migration as if no migrations existed
expect(planAgain.exitCode, 'P4.02: plan fails on broken graph').not.toBe(0);
expect(planOutput, 'P4.02: mentions orphan, broken chain, or disconnected graph').toMatch(
/orphan|broken|disconnect|no.*root|no.*path|unreachable/i,
);
+ const afterFailedPlan = readdirSync(migrationsDir, { withFileTypes: true })
+ .filter((entry) => entry.isDirectory() && !entry.name.startsWith('.'))
+ .map((entry) => entry.name)
+ .sort();
+ expect(afterFailedPlan, 'P4.02: failed plan leaves migrations dir unchanged').toEqual(
+ [...remaining].sort(),
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const planAgain = await runMigrationPlan(ctx, ['--name', 'should-fail']); | |
| const planOutput = stripAnsi(planAgain.stdout); | |
| // The critical assertion: the planner must NOT succeed and silently | |
| // create a new init migration as if no migrations existed | |
| expect(planAgain.exitCode, 'P4.02: plan fails on broken graph').not.toBe(0); | |
| expect(planOutput, 'P4.02: mentions orphan, broken chain, or disconnected graph').toMatch( | |
| /orphan|broken|disconnect|no.*root|no.*path|unreachable/i, | |
| ); | |
| const planAgain = await runMigrationPlan(ctx, ['--name', 'should-fail']); | |
| const planOutput = stripAnsi(planAgain.stdout); | |
| // The critical assertion: the planner must NOT succeed and silently | |
| // create a new init migration as if no migrations existed | |
| expect(planAgain.exitCode, 'P4.02: plan fails on broken graph').not.toBe(0); | |
| expect(planOutput, 'P4.02: mentions orphan, broken chain, or disconnected graph').toMatch( | |
| /orphan|broken|disconnect|no.*root|no.*path|unreachable/i, | |
| ); | |
| const afterFailedPlan = readdirSync(migrationsDir, { withFileTypes: true }) | |
| .filter((entry) => entry.isDirectory() && !entry.name.startsWith('.')) | |
| .map((entry) => entry.name) | |
| .sort(); | |
| expect(afterFailedPlan, 'P4.02: failed plan leaves migrations dir unchanged').toEqual( | |
| [...remaining].sort(), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/integration/test/cli-journeys/drift-deleted-root.e2e.test.ts` around
lines 78 - 86, Capture the state of the migrations directory before calling
runMigrationPlan (e.g., list filenames or take a directory snapshot of
migrations/) and after the call, assert they are identical; specifically, before
invoking runMigrationPlan(ctx, ['--name', 'should-fail']) record the migrations/
contents, then after planAgain completes (and you already assert
planAgain.exitCode !== 0) compare the post-run migrations/ snapshot to the
pre-run snapshot and fail the test if any new files (such as a duplicate init
migration) were created; update the test near runMigrationPlan/planAgain to
perform this pre/post directory comparison.
SevInf
approved these changes
Mar 19, 2026
Contributor
SevInf
left a comment
SevInf
left a comment
There was a problem hiding this comment.
We did review in person and compiled a list of changes that need to be done before merging.
Approving it now, since I don't think changes really would require re-review.
wmadden
reviewed
Mar 19, 2026
Contributor
wmadden
left a comment
wmadden
left a comment
There was a problem hiding this comment.
Code Review — PR #232
PR: feat(migration): replace parentMigrationId with graph-topology pathfinding and refs
Spec: projects/on-disk-migrations-v2/spec.md
Review range: origin/feat/cli-scenarios...HEAD (38 commits, 70 files, +5472 / −1319)
Summary
This PR replaces parentMigrationId-based migration ordering with BFS shortest-path over a contract-hash graph, adds filesystem refs for multi-environment targeting, and aligns CLI commands (plan, apply, status) with the new model. The implementation is thorough, well-tested, and closely follows the spec. No blocking issues identified.
What looks solid
- Pure graph model in
dag.ts: Clean separation between graph reconstruction, pathfinding, leaf detection, and divergence analysis. No I/O or side effects — pure functions operating on an immutable graph structure. - Spec scenario coverage: Every spec scenario (S-1 through S-9) has both a pure unit test in
scenarios.test.tsand an E2E journey test against PGlite. The test pyramid is well-structured. - Structured error model: All errors use the
MigrationToolsErrorclass with stable codes (MIGRATION.*), machine-readabledetails, and actionablefixguidance. TheAMBIGUOUS_LEAFerror includes divergence-point analysis and per-branch details — excellent for agent consumption. - Atomic ref writes:
writeRefsuses write-tmp + rename for atomicity. Ref validation is strict and comprehensive (name format, hash format, path traversal prevention). resolveDisplayChaincorrectness: Routing the display chain through the marker in diamond graphs prevents incorrect applied/pending annotations. This is a subtle but important correctness fix with dedicated tests.DefaultRendererIoC pattern: Follows the establishedNativeTypeExpanderpattern — the family layer stays target-agnostic, the target provides rendering logic. Clean inversion of control.- JSON output shape stability:
output.json-shapes.test.tsuses inline snapshots to lock the JSON output shape for bothMigrationApplyResultandMigrationStatusResult. Important for agent/CI consumers.
Blocking issues
None identified.
Address in this PR
User-facing error text uses graph-internal terminology
Several error codes and messages use graph-theory terminology that won't be meaningful to users who think in terms of "migrations" and "environments," not "leaves" and "nodes."
Error codes that need renaming:
| Current code | Summary text | Problem | Suggested direction |
|---|---|---|---|
MIGRATION.AMBIGUOUS_LEAF |
"Ambiguous migration graph" | "leaf" is graph jargon. Users don't think in leaves. | MIGRATION.DIVERGENT_BRANCHES or MIGRATION.CONFLICTING_MIGRATIONS — "Your migration history has diverged" |
MIGRATION.NO_RESOLVABLE_LEAF |
"Migration graph has no resolvable leaf" | "resolvable leaf" is completely opaque. | MIGRATION.CYCLE_DETECTED or MIGRATION.NO_TARGET — "Cannot determine the migration target because the history contains a cycle" |
MIGRATION.NO_ROOT |
"Migration graph has no root" | "root" is graph jargon. | MIGRATION.NO_INITIAL_MIGRATION — "No migration starts from the initial state" |
MIGRATION.SELF_LOOP |
"Self-loop in migration graph" | "self-loop" is graph jargon. | MIGRATION.SAME_SOURCE_AND_TARGET — "Migration has the same source and target" |
MIGRATION.MARKER_DIVERGED |
"Database marker does not match any migration in the chain" | "MARKER_DIVERGED" sounds like the marker forked. | MIGRATION.MARKER_NOT_IN_HISTORY — "The database is at a state not found in the migration history" |
Error why / fix messages with graph jargon:
| Location | Current text | Problem |
|---|---|---|
errorAmbiguousLeaf why |
"Multiple leaf nodes found: ..." | "leaf nodes" means nothing to a user. Say "Multiple migration targets found" or "Migration history has diverged into N branches" |
errorAmbiguousLeaf why |
"Divergence point: ..." | "Divergence point" is reasonable but could say "branched from" for clarity |
errorNoResolvableLeaf why |
"no node has zero outgoing edges" | Pure graph theory. Say "the migration history contains a cycle (e.g., C1→C2→C1) and the system cannot determine which contract state to target" |
errorNoRoot why |
"No root migration found in the migration graph (nodes: ...)" | Dumping a list of hash nodes is not actionable. Say "No migration starts from an empty database. The initial migration may have been deleted." |
errorSelfLoop why |
from === to === "..." |
Developer-facing syntax, not user-facing. Say "starts and ends at the same contract state" |
errorAmbiguousLeaf fix |
Leads with migration ref set <name> <hash> |
Lead with the common resolution: "Delete one of the conflicting migration directories and re-plan" |
errorNoResolvableLeaf fix |
"Use --from to specify the planning origin explicitly." | Add context: "After a rollback, the migration history forms a cycle. Use --from <hash> to tell the planner which contract state to start from." |
"Chain" in user-facing text — migration-status.ts uses "chain" in several places that users see:
- Diagnostic message:
"Database marker does not match any migration in the chain"(line 462) - Error summary:
"Cannot reconstruct migration chain"(line 408) - Command description:
"Show migration chain and applied status"(lines 281, 524) - Help text:
"Displays the migration chain in order"(line 525)
These should say "migration history" or just "migrations" — "chain" implies a linear sequence, which contradicts the graph model.
General principle: Error codes are consumed by machines (agents, CI). Summaries and why/fix messages are consumed by humans. Both should use domain language ("migration", "branch", "target", "history") rather than implementation language ("leaf", "node", "graph", "root").
migration ref command issues
Three related issues with the migration ref subcommands:
-
Help formatter doesn't show positional arguments.
migration ref set --helponly shows flags — the required<name>and<hash>arguments are invisible. Affectsset,get, anddelete. Likely a bug informatCommandHelpnot rendering Commander's.argument()definitions. -
ref setdoesn't validate the hash value before writing.executeRefSetCommandcallsvalidateRefName(name)but never callsvalidateRefValue(hash). You can runmigration ref set production garbageand it silently writes invalid data torefs.json. The validation only fires later whenresolveRefreads it back. Fix: addvalidateRefValue(hash)check alongside the name validation. -
errorInvalidRefNamename collision.migration-ref.tsdefines a localerrorInvalidRefName(returnsCliStructuredError) whileerrors.tsexports one with the same name (returnsMigrationToolsError). Rename the CLI-local version to disambiguate.
db update → migrations transition has no guided workflow
When migration status detects a DB marker that doesn't match any migration, it warns "was the database managed with 'db update'?" and hints to run db verify. But it doesn't tell the user how to actually resolve the situation.
The spec (S-7, S-9) describes a clear workflow: create a baseline migration (migration plan --name baseline), then migration apply treats the baseline as a no-op. But this isn't surfaced in the CLI output.
Compounding the problem: migration status truncates hashes in the tree view (→ sha256:a1b2c3...), so the user can't easily copy them for use with ref set or --from. Consider showing full hashes (or at least in verbose mode).
Recommendation: The MIGRATION.MARKER_DIVERGED hint should guide the user through the baseline workflow: "Run 'prisma-next migration plan --name baseline' to create a baseline migration, then 'prisma-next migration apply' to adopt migrations."
Minor non-blocking concerns (no discussion needed)
Refs path resolution duplicated across CLI commands — Each command independently resolves refs.json via resolve(migrationsDir, 'refs.json'). migration-ref.ts has resolveRefsPath() but other commands inline it. Extract a shared helper.
Inline MigrationToolsError construction in resolveRef — resolveRef constructs a MigrationToolsError inline for UNKNOWN_REF while all other codes use factory functions in errors.ts. Extract errorUnknownRef(name, availableRefs) for consistency.
migration-apply.ts treats EMPTY_CONTRACT_HASH in marker as corruption — The apply command treats a marker containing EMPTY_CONTRACT_HASH as "should never happen." Consider whether this guard is too strict and whether the error should mention possible causes.
Follow-up tickets
Redesign migration status to be graph-oriented (next ticket)
migration status currently renders a single linear chain (findPath(∅ → target)) and needs findLeaf to auto-detect an endpoint when no --ref is provided. This linear-chain rendering model is the root cause of several interrelated concerns in this PR:
-
findLeafand its error machinery exist to service this constraint.findLeaf,findDivergencePoint,AMBIGUOUS_LEAF,NO_RESOLVABLE_LEAF, andNO_ROOTare all needed because the command can't render without picking a single target.migration applydemonstrates the correct pattern — it always has an explicit target and never callsfindLeaf. -
findDivergencePointbuilds full ancestor sets for each leaf when it does trigger, which is O(N × M). Acceptable today, but this function only exists because divergence is treated as an error rather than rendered as visible branches. -
migration-status.tsguardsfindLeafwith ref check —activeRefHash ?? findLeaf(graph)short-circuits when a ref is provided. This is correct but is a workaround for the linear-chain constraint.
Recommendation: Redesign migration status to render the full migration graph from the root, labeling each node as applied/pending via the DB marker. Divergence becomes visible branches in the output rather than a hard failure. migration plan should derive its starting point from the contract hash rather than leaf detection. This would eliminate a class of user-facing errors and simplify the graph module.
Add a migration check diagnostic command
Graph health problems are currently only detected as side effects of other commands (status, plan), and only when they happen to trigger findLeaf. Commands with explicit targets (apply, status --ref, plan --from) bypass these checks entirely.
Add a dedicated migration check (or migration verify) command for CI pipelines that validates graph health independently:
- Divergence: multiple branches from the same point (currently only caught by
findLeaf) - Orphaned / unreachable states: nodes disconnected from the root
- Self-loops: migrations with
from === to - Graph size: warning when the migration count exceeds a hygiene threshold (per ADR 102 squash-first)
- Corruption: missing files, invalid JSON, broken attestation
This should be a standalone CI gate, not a side effect of operational commands.
Nits
dag.ts is misnamed — the graph is not a DAG
Location: packages/1-framework/3-tooling/migration/src/dag.ts, its exports module, test file, and README references.
Issue: The file is named dag.ts ("directed acyclic graph"), but the migration graph explicitly supports cycles (rollback migrations like C1→C2→C1). The BFS pathfinder uses visited-node tracking specifically to handle cycles.
Suggestion: Rename to graph.ts (and corresponding exports, test file, README references). The type MigrationGraph is already correctly named.
MigrationChainEntry and forwardChain / reverseChain use legacy "chain" naming
MigrationChainEntry is a holdover from when migrations formed a linear chain. A migration is just a state transition — the graph is a model we impose on the collection. Rename to Migration (or MigrationEntry if disambiguation is needed). The forwardChain and reverseChain fields on MigrationGraph should also drop "chain" (e.g. forward / reverse, or outgoing / incoming).
Acceptance-criteria traceability
AC-1: For graphs with cycles, path resolution returns a deterministic shortest path and never loops
- Implementation:
- Evidence:
AC-2: For start == target, plan and apply both return explicit no-op results
- Implementation:
- Evidence:
AC-3: Revisited-hash scenarios execute correctly without parentMigrationId
- Implementation:
- Evidence:
AC-4: Divergent branches produce a hard error with diagnostics
- Implementation:
- Evidence:
AC-5: refs.json supports CRUD with validation and atomic writes
- Implementation:
- Evidence:
AC-6: status and apply consume refs with consistent output
- Implementation:
- Evidence:
AC-7: Deterministic tie-break strategy is documented and tested
- Implementation:
- Evidence:
AC-8: CI-oriented JSON output includes path-decision metadata
saevarb
force-pushed
the
feat/state-transition-migrations
branch
from
1c78ca1 to
ba03a3f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/1-framework/3-tooling/migration/src/dag.ts (2)
174-189:⚠️ Potential issue | 🟠 MajorOnly treat shortest-path branches as tie-break candidates.
reachablecurrently includes any outgoing edge that can eventually reachtoHash. In a shape likeA -> B -> DandA -> C -> E -> D, the longerCdetour still incrementsalternativeCountand can emit a tie-break reason even though the shortest path is unique. After filtering to the minimum remaining distance, compare the winner by edge identity (sorted[0] === edge) rather thanmigrationId, otherwise null/duplicate IDs still misattribute the selected edge.🤖 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/dag.ts` around lines 174 - 189, reachable currently includes any outgoing edge that can reach toHash, causing longer detours to count as alternatives; modify the logic that builds reachable (inside the loop over path) to first compute remaining distances (via findPath or a distance helper) for each outgoing edge, filter reachable to only edges with the minimum remaining distance (shortest-path branches), then compute sorted = sortedNeighbors(reachable) and determine the selected winner by identity (use strict equality sorted[0] === edge) instead of comparing migrationId so null/duplicate IDs don't misattribute selection; keep increments to alternativeCount and tieBreakReasons based on this filtered reachable set.
293-316:⚠️ Potential issue | 🟠 MajorDo not collapse a disconnected migration history to
EMPTY_CONTRACT_HASH.If the first migration is deleted but the empty node remains,
findReachableLeaves()returns[EMPTY_CONTRACT_HASH]and this function treats the graph as empty even though other nodes still exist. That makes status/plan silently ignore a corrupted history instead of surfacingNO_RESOLVABLE_LEAF.🩹 Suggested fix
const leaves = findReachableLeaves(graph, EMPTY_CONTRACT_HASH); + + if ( + leaves.length === 1 && + leaves[0] === EMPTY_CONTRACT_HASH && + [...graph.nodes].some((node) => node !== EMPTY_CONTRACT_HASH) + ) { + throw errorNoResolvableLeaf( + [...graph.nodes].filter((node) => node !== EMPTY_CONTRACT_HASH), + ); + } if (leaves.length === 0) { const reachable = [...graph.nodes].filter((n) => n !== EMPTY_CONTRACT_HASH); if (reachable.length > 0) { throw errorNoResolvableLeaf(reachable);🤖 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/dag.ts` around lines 293 - 316, findReachableLeaves() can return [EMPTY_CONTRACT_HASH] when the empty node remains but other nodes exist; current code treats that as empty graph. Change logic so EMPTY_CONTRACT_HASH is ignored as a valid leaf when other nodes are present: after computing leaves and reachable (using findReachableLeaves(graph, EMPTY_CONTRACT_HASH) and [...graph.nodes].filter(n => n !== EMPTY_CONTRACT_HASH)), if leaves contains only EMPTY_CONTRACT_HASH and reachable.length > 0 then throw errorNoResolvableLeaf(reachable) instead of returning EMPTY_CONTRACT_HASH; keep the existing ambiguity handling (findDivergencePoint, findPath, errorAmbiguousLeaf) and the normal single-leaf return for truly empty graphs.
🧹 Nitpick comments (3)
packages/1-framework/3-tooling/cli/src/commands/migration-show.ts (1)
50-56: UseisAttestedfor consistency with line 135.Line 55 uses an inline type check while line 135 uses the
isAttestedtype guard. UsingisAttestedconsistently improves maintainability and ensures the filtering logic is centralized.Proposed fix
const normalizedPrefix = prefix.startsWith('sha256:') ? prefix : `sha256:${prefix}`; - const attested = packages.filter((p) => typeof p.manifest.migrationId === 'string'); + const attested = packages.filter(isAttested); const matches = attested.filter((p) => p.manifest.migrationId!.startsWith(normalizedPrefix));Note: After this change, you can also remove the non-null assertion on
migrationIdsinceisAttestednarrows the type.🤖 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-show.ts` around lines 50 - 56, Replace the inline type check in resolveByHashPrefix with the existing isAttested type guard: use packages.filter(isAttested) instead of packages.filter((p) => typeof p.manifest.migrationId === 'string') to produce attested, then use that attested array to filter by manifest.migrationId starting with normalizedPrefix (you can remove the non-null assertion on migrationId because isAttested narrows the type).test/integration/test/cli-journeys/converging-paths.e2e.test.ts (1)
45-59: Line 51: Unused parse result is acceptable but could be cleaner.The
parseJsonOutputcall on line 51 is only used to validate JSON parsing succeeds, but the result is discarded. Consider either:
- Removing the call if exit code is sufficient validation
- Using
_prefix if intentionally discarding:const _c2Hash = parseJsonOutput<...>(plan1);This is a minor style observation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/test/cli-journeys/converging-paths.e2e.test.ts` around lines 45 - 59, The parseJsonOutput call for plan1 is only used to assert JSON parses but its return value is discarded; either remove the call entirely (since you already assert plan1.exitCode) or explicitly capture it to indicate intentional discard by assigning to a named throwaway variable (e.g., const _c2Parse = parseJsonOutput<{ to: string }>(plan1)); update the code around swapContract/runContractEmit/ runMigrationPlan (the plan1/emit1 sequence) accordingly so intent is clear.packages/1-framework/3-tooling/cli/test/commands/migration-status.test.ts (1)
1-846: Consider splitting this test file.This test file is approximately 846 lines, exceeding the 500-line guideline. The file contains multiple distinct
describeblocks that could be split into separate files:
buildMigrationEntriestests →migration-status.entries.test.tsformatMigrationStatusOutputtests →migration-status.format.test.tsresolveDisplayChaintests →migration-status.chain.test.tsreadRefs error surfacetests →migration-status.refs.test.tsCONTRACT.AHEAD diagnostictests →migration-status.diagnostics.test.tsThis would improve navigability and maintainability.
As per coding guidelines: "Keep test files under 500 lines to maintain readability and navigability. If a test file exceeds this limit, it should be split into multiple files."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/cli/test/commands/migration-status.test.ts` around lines 1 - 846, The test file is too large (~846 lines) and should be split into smaller files to match the 500-line guideline; split the describe blocks into separate test files as suggested. Create new test files and move the corresponding describe blocks: move the "buildMigrationEntries" suite to migration-status.entries.test.ts (references: buildMigrationEntries), the "formatMigrationStatusOutput" suite to migration-status.format.test.ts (references: formatMigrationStatusOutput, parseGlobalFlags), the "resolveDisplayChain" suite to migration-status.chain.test.ts (references: resolveDisplayChain), the "readRefs error surface" suite to migration-status.refs.test.ts (references: readRefs, MigrationToolsError), and the "CONTRACT.AHEAD diagnostic" suite to migration-status.diagnostics.test.ts (references: formatMigrationStatusOutput and refs handling); ensure each new file imports only the helpers and utilities it needs (e.g., createTempDir, setupChain, readMigrationsDir, reconstructGraph, isAttested, findLeaf, findPath, EMPTY_CONTRACT_HASH, timeouts, stripAnsi) and update any shared helper functions by moving them to a small test-utils module if repeated, then run the tests to verify no import or scope issues remain.
🤖 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-status.ts`:
- Around line 122-126: The status logic treats only undefined as the root
marker; update checks that compute markerInChain and reachedMarker (and the
analogous checks around the other occurrence at the 386-387 region) to also
consider EMPTY_CONTRACT_HASH as the root marker — i.e. treat markerHash ===
undefined || markerHash === EMPTY_CONTRACT_HASH the same as
before-first-migration — so change the conditions that reference markerHash to
include markerHash === EMPTY_CONTRACT_HASH (affecting variables markerInChain
and reachedMarker and the corresponding divergence checks).
In `@packages/1-framework/3-tooling/cli/src/utils/command-helpers.ts`:
- Around line 197-232: The function readContractEnvelope currently blind-casts
JSON.parse to `json` and immediately destructures/spreads it, which will throw
on null/scalar payloads and can re-include invalid fields like a non-string
`profileHash`; update readContractEnvelope to first assert that `json` is a
plain object (e.g., typeof json === 'object' && json !== null &&
!Array.isArray(json') and only then extract/validate properties, avoid spreading
the raw `json` into the returned ContractEnvelope (replace `...json` with a
`rest` object built from validated fields), and only include `profileHash` in
the returned object when `typeof profileHash === 'string'` so invalid
profileHash values are not propagated.
In `@test/integration/test/utils/journey-test-helpers.ts`:
- Around line 342-354: runMigrationRef currently destructures subcommand from
subcommandArgs and uses subcommand! which can pass undefined to runCommandRaw
when subcommandArgs is empty; update runMigrationRef to validate that
subcommandArgs has at least one element (or explicitly document the requirement)
and either throw a clear error if empty or provide a safe default before calling
createMigrationRefCommand()/runCommandRaw; refer to the function
runMigrationRef, the parameter subcommandArgs, and the local variable subcommand
when adding the guard.
---
Duplicate comments:
In `@packages/1-framework/3-tooling/migration/src/dag.ts`:
- Around line 174-189: reachable currently includes any outgoing edge that can
reach toHash, causing longer detours to count as alternatives; modify the logic
that builds reachable (inside the loop over path) to first compute remaining
distances (via findPath or a distance helper) for each outgoing edge, filter
reachable to only edges with the minimum remaining distance (shortest-path
branches), then compute sorted = sortedNeighbors(reachable) and determine the
selected winner by identity (use strict equality sorted[0] === edge) instead of
comparing migrationId so null/duplicate IDs don't misattribute selection; keep
increments to alternativeCount and tieBreakReasons based on this filtered
reachable set.
- Around line 293-316: findReachableLeaves() can return [EMPTY_CONTRACT_HASH]
when the empty node remains but other nodes exist; current code treats that as
empty graph. Change logic so EMPTY_CONTRACT_HASH is ignored as a valid leaf when
other nodes are present: after computing leaves and reachable (using
findReachableLeaves(graph, EMPTY_CONTRACT_HASH) and [...graph.nodes].filter(n =>
n !== EMPTY_CONTRACT_HASH)), if leaves contains only EMPTY_CONTRACT_HASH and
reachable.length > 0 then throw errorNoResolvableLeaf(reachable) instead of
returning EMPTY_CONTRACT_HASH; keep the existing ambiguity handling
(findDivergencePoint, findPath, errorAmbiguousLeaf) and the normal single-leaf
return for truly empty graphs.
---
Nitpick comments:
In `@packages/1-framework/3-tooling/cli/src/commands/migration-show.ts`:
- Around line 50-56: Replace the inline type check in resolveByHashPrefix with
the existing isAttested type guard: use packages.filter(isAttested) instead of
packages.filter((p) => typeof p.manifest.migrationId === 'string') to produce
attested, then use that attested array to filter by manifest.migrationId
starting with normalizedPrefix (you can remove the non-null assertion on
migrationId because isAttested narrows the type).
In `@packages/1-framework/3-tooling/cli/test/commands/migration-status.test.ts`:
- Around line 1-846: The test file is too large (~846 lines) and should be split
into smaller files to match the 500-line guideline; split the describe blocks
into separate test files as suggested. Create new test files and move the
corresponding describe blocks: move the "buildMigrationEntries" suite to
migration-status.entries.test.ts (references: buildMigrationEntries), the
"formatMigrationStatusOutput" suite to migration-status.format.test.ts
(references: formatMigrationStatusOutput, parseGlobalFlags), the
"resolveDisplayChain" suite to migration-status.chain.test.ts (references:
resolveDisplayChain), the "readRefs error surface" suite to
migration-status.refs.test.ts (references: readRefs, MigrationToolsError), and
the "CONTRACT.AHEAD diagnostic" suite to migration-status.diagnostics.test.ts
(references: formatMigrationStatusOutput and refs handling); ensure each new
file imports only the helpers and utilities it needs (e.g., createTempDir,
setupChain, readMigrationsDir, reconstructGraph, isAttested, findLeaf, findPath,
EMPTY_CONTRACT_HASH, timeouts, stripAnsi) and update any shared helper functions
by moving them to a small test-utils module if repeated, then run the tests to
verify no import or scope issues remain.
In `@test/integration/test/cli-journeys/converging-paths.e2e.test.ts`:
- Around line 45-59: The parseJsonOutput call for plan1 is only used to assert
JSON parses but its return value is discarded; either remove the call entirely
(since you already assert plan1.exitCode) or explicitly capture it to indicate
intentional discard by assigning to a named throwaway variable (e.g., const
_c2Parse = parseJsonOutput<{ to: string }>(plan1)); update the code around
swapContract/runContractEmit/ runMigrationPlan (the plan1/emit1 sequence)
accordingly so intent is clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: c678c807-dd9a-474b-b0db-7cdad52e5e03
⛔ Files ignored due to path filters (9)
projects/cli-migration-cleanup/plan.mdis excluded by!projects/**projects/on-disk-migrations-v2/assets/.gitkeepis excluded by!projects/**projects/on-disk-migrations-v2/issue-triage.mdis excluded by!projects/**projects/on-disk-migrations-v2/plan.mdis excluded by!projects/**projects/on-disk-migrations-v2/plans/.gitkeepis excluded by!projects/**projects/on-disk-migrations-v2/plans/cli-output-pipeline-plan.mdis excluded by!projects/**projects/on-disk-migrations-v2/plans/pr-232-review-fixes-plan.mdis excluded by!projects/**projects/on-disk-migrations-v2/spec.mdis excluded by!projects/**projects/on-disk-migrations-v2/specs/.gitkeepis excluded by!projects/**
📒 Files selected for processing (66)
docs/architecture docs/adrs/ADR 169 - On-disk migration persistence.mddocs/architecture docs/subsystems/7. Migration System.mdpackages/1-framework/3-tooling/cli/README.mdpackages/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-plan.tspackages/1-framework/3-tooling/cli/src/commands/migration-ref.tspackages/1-framework/3-tooling/cli/src/commands/migration-show.tspackages/1-framework/3-tooling/cli/src/commands/migration-status.tspackages/1-framework/3-tooling/cli/src/control-api/operations/migration-apply.tspackages/1-framework/3-tooling/cli/src/control-api/types.tspackages/1-framework/3-tooling/cli/src/utils/command-helpers.tspackages/1-framework/3-tooling/cli/src/utils/formatters/help.tspackages/1-framework/3-tooling/cli/src/utils/formatters/migrations.tspackages/1-framework/3-tooling/cli/src/utils/migration-types.tspackages/1-framework/3-tooling/cli/test/commands/migration-apply.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-e2e.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-plan.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-ref.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-show.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-status.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-verify.test.tspackages/1-framework/3-tooling/cli/test/output.json-shapes.test.tspackages/1-framework/3-tooling/cli/tsdown.config.tspackages/1-framework/3-tooling/cli/vitest.config.tspackages/1-framework/3-tooling/migration/package.jsonpackages/1-framework/3-tooling/migration/src/dag.tspackages/1-framework/3-tooling/migration/src/errors.tspackages/1-framework/3-tooling/migration/src/exports/dag.tspackages/1-framework/3-tooling/migration/src/exports/refs.tspackages/1-framework/3-tooling/migration/src/exports/types.tspackages/1-framework/3-tooling/migration/src/io.tspackages/1-framework/3-tooling/migration/src/refs.tspackages/1-framework/3-tooling/migration/src/types.tspackages/1-framework/3-tooling/migration/test/attestation.test.tspackages/1-framework/3-tooling/migration/test/dag.test.tspackages/1-framework/3-tooling/migration/test/fixtures.tspackages/1-framework/3-tooling/migration/test/refs.test.tspackages/1-framework/3-tooling/migration/tsdown.config.tspackages/2-sql/3-tooling/family/src/core/migrations/contract-to-schema-ir.tspackages/2-sql/3-tooling/family/src/exports/control.tspackages/2-sql/3-tooling/family/test/contract-to-schema-ir.test.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner.tspackages/3-targets/3-targets/postgres/src/exports/control.tspackages/3-targets/3-targets/postgres/test/migrations/planner.contract-to-schema-ir.test.tstest/integration/test/cli-journeys/README.mdtest/integration/test/cli-journeys/adopt-migrations.e2e.test.tstest/integration/test/cli-journeys/converging-paths.e2e.test.tstest/integration/test/cli-journeys/diamond-convergence.e2e.test.tstest/integration/test/cli-journeys/divergence-and-refs.e2e.test.tstest/integration/test/cli-journeys/drift-deleted-root.e2e.test.tstest/integration/test/cli-journeys/interleaved-db-update.e2e.test.tstest/integration/test/cli-journeys/migration-apply-edge-cases.e2e.test.tstest/integration/test/cli-journeys/migration-plan-details.e2e.test.tstest/integration/test/cli-journeys/ref-routing.e2e.test.tstest/integration/test/cli-journeys/rollback-cycle.e2e.test.tstest/integration/test/cli.migration-apply.e2e.test.tstest/integration/test/cli.migration-plan.e2e.test.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-additive-required.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-all.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-avatar.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-bio.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-phone-bio.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-phone.tstest/integration/test/utils/journey-test-helpers.ts
💤 Files with no reviewable changes (5)
- packages/1-framework/3-tooling/migration/test/attestation.test.ts
- packages/1-framework/3-tooling/cli/test/commands/migration-verify.test.ts
- packages/1-framework/3-tooling/cli/test/commands/migration-show.test.ts
- test/integration/test/cli.migration-apply.e2e.test.ts
- test/integration/test/cli.migration-plan.e2e.test.ts
✅ Files skipped from review due to trivial changes (14)
- packages/1-framework/3-tooling/cli/vitest.config.ts
- packages/1-framework/3-tooling/cli/src/control-api/operations/migration-apply.ts
- packages/3-targets/3-targets/postgres/test/migrations/planner.contract-to-schema-ir.test.ts
- packages/1-framework/3-tooling/cli/package.json
- packages/2-sql/3-tooling/family/src/exports/control.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/planner.ts
- packages/1-framework/3-tooling/cli/test/commands/migration-apply.test.ts
- packages/1-framework/3-tooling/migration/package.json
- packages/1-framework/3-tooling/migration/tsdown.config.ts
- packages/1-framework/3-tooling/cli/src/utils/migration-types.ts
- test/integration/test/cli-journeys/README.md
- docs/architecture docs/adrs/ADR 169 - On-disk migration persistence.md
- packages/1-framework/3-tooling/migration/src/exports/dag.ts
- packages/1-framework/3-tooling/cli/test/commands/migration-ref.test.ts
🚧 Files skipped from review as they are similar to previous changes (24)
- packages/1-framework/3-tooling/cli/tsdown.config.ts
- packages/1-framework/3-tooling/cli/src/cli.ts
- packages/1-framework/3-tooling/cli/test/commands/migration-e2e.test.ts
- test/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-phone-bio.ts
- test/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-all.ts
- packages/1-framework/3-tooling/cli/test/commands/migration-plan.test.ts
- packages/1-framework/3-tooling/migration/src/exports/refs.ts
- test/integration/test/cli-journeys/ref-routing.e2e.test.ts
- docs/architecture docs/subsystems/7. Migration System.md
- packages/2-sql/3-tooling/family/src/core/migrations/contract-to-schema-ir.ts
- test/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-phone.ts
- test/integration/test/cli-journeys/migration-apply-edge-cases.e2e.test.ts
- test/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-bio.ts
- test/integration/test/cli-journeys/diamond-convergence.e2e.test.ts
- packages/3-targets/3-targets/postgres/src/exports/control.ts
- test/integration/test/cli-journeys/migration-plan-details.e2e.test.ts
- test/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-avatar.ts
- packages/1-framework/3-tooling/cli/src/commands/migration-ref.ts
- packages/1-framework/3-tooling/cli/README.md
- packages/1-framework/3-tooling/migration/src/refs.ts
- packages/1-framework/3-tooling/cli/src/utils/formatters/migrations.ts
- test/integration/test/cli-journeys/interleaved-db-update.e2e.test.ts
- packages/1-framework/3-tooling/migration/test/dag.test.ts
- test/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-additive-required.ts
Comment on lines
+122
to
+126
| const markerInChain = markerHash === undefined || chain.some((e) => e.to === markerHash); | ||
|
|
||
| const entries: MigrationStatusEntry[] = []; | ||
| let reachedMarker = markerHash === undefined || markerHash === EMPTY_CONTRACT_HASH; | ||
| // Online with no marker = fresh database, all migrations are pending. | ||
| let reachedMarker = mode === 'online' && markerHash === undefined; |
There was a problem hiding this comment.
Treat EMPTY_CONTRACT_HASH as the root marker in status calculations.
A database explicitly marked at the empty hash currently falls into the divergence path because both checks only treat undefined as the “before first migration” state. Since no chain edge ever has to === EMPTY_CONTRACT_HASH, that turns a valid “all pending from root” status into unknown / MIGRATION.MARKER_DIVERGED.
🩹 Suggested fix
export function buildMigrationEntries(
chain: readonly MigrationChainEntry[],
packages: readonly MigrationBundle[],
mode: 'online' | 'offline',
markerHash: string | undefined,
): MigrationStatusEntry[] {
const pkgByDirName = new Map(packages.map((p) => [p.dirName, p]));
- const markerInChain = markerHash === undefined || chain.some((e) => e.to === markerHash);
+ const markerAtRoot =
+ markerHash === undefined || markerHash === EMPTY_CONTRACT_HASH;
+ const markerInChain = markerAtRoot || chain.some((e) => e.to === markerHash);
const entries: MigrationStatusEntry[] = [];
- let reachedMarker = mode === 'online' && markerHash === undefined;
+ let reachedMarker = mode === 'online' && markerAtRoot;- const markerInChain = markerHash === undefined || chain.some((e) => e.to === markerHash);
+ const markerAtRoot =
+ markerHash === undefined || markerHash === EMPTY_CONTRACT_HASH;
+ const markerInChain = markerAtRoot || chain.some((e) => e.to === markerHash);Also applies to: 386-387
🤖 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 122 - 126, The status logic treats only undefined as the root marker;
update checks that compute markerInChain and reachedMarker (and the analogous
checks around the other occurrence at the 386-387 region) to also consider
EMPTY_CONTRACT_HASH as the root marker — i.e. treat markerHash === undefined ||
markerHash === EMPTY_CONTRACT_HASH the same as before-first-migration — so
change the conditions that reference markerHash to include markerHash ===
EMPTY_CONTRACT_HASH (affecting variables markerInChain and reachedMarker and the
corresponding divergence checks).
Comment on lines
+197
to
+232
| export async function readContractEnvelope(config: { | ||
| contract?: { output?: string }; | ||
| }): Promise<ContractEnvelope> { | ||
| const contractPath = resolveContractPath(config); | ||
| const content = await readFile(contractPath, 'utf-8'); | ||
| const json = JSON.parse(content) as Record<string, unknown>; | ||
|
|
||
| const { storageHash, schemaVersion, target, targetFamily, profileHash } = json; | ||
|
|
||
| if (typeof storageHash !== 'string') { | ||
| throw new Error( | ||
| `Contract at ${relative(process.cwd(), contractPath)} is missing a valid storageHash. Run \`prisma-next contract emit\` to regenerate.`, | ||
| ); | ||
| } | ||
| if (typeof schemaVersion !== 'string') { | ||
| throw new Error( | ||
| `Contract at ${relative(process.cwd(), contractPath)} is missing schemaVersion.`, | ||
| ); | ||
| } | ||
| if (typeof target !== 'string') { | ||
| throw new Error(`Contract at ${relative(process.cwd(), contractPath)} is missing target.`); | ||
| } | ||
| if (typeof targetFamily !== 'string') { | ||
| throw new Error( | ||
| `Contract at ${relative(process.cwd(), contractPath)} is missing targetFamily.`, | ||
| ); | ||
| } | ||
|
|
||
| return { | ||
| ...json, | ||
| storageHash, | ||
| schemaVersion, | ||
| target, | ||
| targetFamily, | ||
| ...(typeof profileHash === 'string' ? { profileHash } : {}), | ||
| }; |
There was a problem hiding this comment.
Validate the parsed envelope before destructuring and spreading it.
JSON.parse() is blind-cast here, so a null/scalar payload will throw a generic destructuring error before any of the contract-specific validation messages run. On top of that, ...json copies a rejected non-string profileHash back into the returned envelope. Guard the top-level object first, then spread ...rest so rejected fields stay rejected.
🩹 Suggested fix
export async function readContractEnvelope(config: {
contract?: { output?: string };
}): Promise<ContractEnvelope> {
const contractPath = resolveContractPath(config);
const content = await readFile(contractPath, 'utf-8');
- const json = JSON.parse(content) as Record<string, unknown>;
+ const parsed: unknown = JSON.parse(content);
+ if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) {
+ throw new Error(
+ `Contract at ${relative(process.cwd(), contractPath)} must be a JSON object.`,
+ );
+ }
+ const json = parsed as Record<string, unknown>;
- const { storageHash, schemaVersion, target, targetFamily, profileHash } = json;
+ const { storageHash, schemaVersion, target, targetFamily, profileHash, ...rest } = json;
@@
return {
- ...json,
+ ...rest,
storageHash,
schemaVersion,
target,
targetFamily,
...(typeof profileHash === 'string' ? { profileHash } : {}),
};
}🤖 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/command-helpers.ts` around lines
197 - 232, The function readContractEnvelope currently blind-casts JSON.parse to
`json` and immediately destructures/spreads it, which will throw on null/scalar
payloads and can re-include invalid fields like a non-string `profileHash`;
update readContractEnvelope to first assert that `json` is a plain object (e.g.,
typeof json === 'object' && json !== null && !Array.isArray(json') and only then
extract/validate properties, avoid spreading the raw `json` into the returned
ContractEnvelope (replace `...json` with a `rest` object built from validated
fields), and only include `profileHash` in the returned object when `typeof
profileHash === 'string'` so invalid profileHash values are not propagated.
Comment on lines
+342
to
+354
| export async function runMigrationRef( | ||
| ctx: JourneyContext, | ||
| subcommandArgs: readonly string[], | ||
| ): Promise<CommandResult> { | ||
| const [subcommand, ...rest] = subcommandArgs; | ||
| return runCommandRaw(createMigrationRefCommand(), ctx.testDir, [ | ||
| subcommand!, | ||
| '--config', | ||
| ctx.configPath, | ||
| '--no-color', | ||
| ...rest, | ||
| ]); | ||
| } |
There was a problem hiding this comment.
Empty subcommandArgs would pass undefined as the first CLI argument.
If subcommandArgs is an empty array, subcommand will be undefined and the non-null assertion on line 348 will pass undefined to the command (which Commander may handle poorly or cause confusing errors).
Consider adding a guard or at minimum documenting the requirement:
Proposed fix
export async function runMigrationRef(
ctx: JourneyContext,
subcommandArgs: readonly string[],
): Promise<CommandResult> {
+ if (subcommandArgs.length === 0) {
+ throw new Error('runMigrationRef requires at least one subcommand argument (set|get|delete|list)');
+ }
const [subcommand, ...rest] = subcommandArgs;
return runCommandRaw(createMigrationRefCommand(), ctx.testDir, [
- subcommand!,
+ subcommand,
'--config',
ctx.configPath,
'--no-color',
...rest,
]);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export async function runMigrationRef( | |
| ctx: JourneyContext, | |
| subcommandArgs: readonly string[], | |
| ): Promise<CommandResult> { | |
| const [subcommand, ...rest] = subcommandArgs; | |
| return runCommandRaw(createMigrationRefCommand(), ctx.testDir, [ | |
| subcommand!, | |
| '--config', | |
| ctx.configPath, | |
| '--no-color', | |
| ...rest, | |
| ]); | |
| } | |
| export async function runMigrationRef( | |
| ctx: JourneyContext, | |
| subcommandArgs: readonly string[], | |
| ): Promise<CommandResult> { | |
| if (subcommandArgs.length === 0) { | |
| throw new Error('runMigrationRef requires at least one subcommand argument (set|get|delete|list)'); | |
| } | |
| const [subcommand, ...rest] = subcommandArgs; | |
| return runCommandRaw(createMigrationRefCommand(), ctx.testDir, [ | |
| subcommand, | |
| '--config', | |
| ctx.configPath, | |
| '--no-color', | |
| ...rest, | |
| ]); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/integration/test/utils/journey-test-helpers.ts` around lines 342 - 354,
runMigrationRef currently destructures subcommand from subcommandArgs and uses
subcommand! which can pass undefined to runCommandRaw when subcommandArgs is
empty; update runMigrationRef to validate that subcommandArgs has at least one
element (or explicitly document the requirement) and either throw a clear error
if empty or provide a safe default before calling
createMigrationRefCommand()/runCommandRaw; refer to the function
runMigrationRef, the parameter subcommandArgs, and the local variable subcommand
when adding the guard.
- Assert migrationsApplied === 0 for baseline no-op (adopt-migrations) - Assert migrationsApplied and markerHash after --ref apply (divergence-and-refs) - Check error diagnostics on stdout only, not stdout+stderr (divergence-and-refs, migration-apply-edge-cases, ref-routing, rollback-cycle) - Assert final column set exactly with toEqual (migration-apply-edge-cases) - Assert operationClass is destructive for drop-column op (migration-plan-details) - Replace weak exitCode || regex assertion with direct stdout match (ref-routing) Made-with: Cursor
Replace manual createDevDatabase/beforeAll/afterAll boilerplate with the shared useDevDatabase() helper across all 10 journey test files. Made-with: Cursor
…EAD in ref mode F01: Replace bare catch in migration status readRefs call with proper error handling — malformed refs.json now surfaces as a structured CLI error instead of being silently swallowed. F02: Gate CONTRACT.AHEAD diagnostic and inline hint behind non-ref mode. In --ref mode, contract being ahead of the ref target is expected multi-environment behavior, not a warning. F04: Add tests for readRefs error surface (malformed JSON, invalid ref names/values) and CONTRACT.AHEAD behavior in both ref and non-ref modes. Made-with: Cursor
… branching Made-with: Cursor
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tionId, rename to MigrationBundle Add readContractEnvelope utility for framework-level contract reading with typed envelope fields. Replace raw JSON.parse + as Record casts in migration-apply and migration-status. Tighten migrationId to non-nullable string. Rename MigrationPackage to MigrationBundle. WIP: TODO comments remain for M2-M5 cleanup tasks. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ult, targetSupportsMigrations Extract shared utilities for migration path resolution, PathDecision serialization, and target migrations type guard. Remove inline casts, duplicated config/path resolution, and PathDecision construction across migration-apply, migration-plan, and migration-status. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…uard Split MigrationManifest into DraftMigrationManifest (migrationId: null) and AttestedMigrationManifest (migrationId: string). Add isAttested type guard and AttestedMigrationBundle. Update reconstructGraph to accept AttestedMigrationBundle[]. Rename packages to bundles in CLI commands. WIP: CLI and test type errors remain — test files need updating to use createAttestedBundle or filter with isAttested. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The inline operations type on MigrationApplyStep duplicated MigrationPlanOperation with an extra index signature, forcing cast round-trips at both the producer (packageToStep) and consumer (migration-apply operation). Replace with the canonical type and remove both casts.
targetSupportsMigrations was a type guard that narrowed config.target to an intersection type incompatible with exactOptionalPropertyTypes. Change it to a boolean, add getTargetMigrations that returns the typed TargetMigrationsCapability from the migration-control-plane's ControlTargetDescriptor (which carries the optional migrations? field the config-level type omits). This fixes the TS2375 errors in migration-plan and migration-apply, and gives migrations a proper type so downstream calls (createPlanner, contractToSchema) are fully typed. Also removes unused AttestedMigrationBundle import and replaces stale MigrationPackage reference in migration-status.
Replace manual migrationId !== null / typeof checks with the isAttested type guard so TypeScript narrows to AttestedMigrationBundle. Add missing filter in tests and source files that were passing unfiltered MigrationBundle[] to reconstructGraph. Also replaces stale MigrationPackage references with MigrationBundle in migration-show.
Extracts the repeated readMigrationsDir → filter isAttested → reconstructGraph pattern into a shared loadMigrationBundles helper in command-helpers.ts. Replaces duplicate code in migration-apply, migration-plan, and migration-status, merging separate try/catch blocks where both steps had identical error handling.
- Replace "reset with db init" fix suggestions with accurate guidance
(db init is additive-only and does not reset). Suggest db sign for
marker mismatches, manual drop for corruption.
- Stop conflating undefined marker with EMPTY_CONTRACT_HASH. In
migration-apply, markerHash is now string | undefined where
undefined = no marker row. Use originHash = markerHash ??
EMPTY_CONTRACT_HASH only for path computation.
- In migration-status, pass mode ('online'|'offline') to
buildMigrationEntries instead of encoding it via EMPTY_CONTRACT_HASH
sentinel. Online + no marker = all pending, offline = all unknown.
- Resolve TODOs: redundant destination check (not redundant — keeps it),
resolveDisplayChain fallback (acceptable — caller handles divergence),
ref fallback to findLeaf (ref is validated upstream, fallback is the
no-ref default).
When the database marker doesn't match the migration graph, tailor the hint based on whether the marker matches the current contract: - Marker == contract: DB was managed with db update, suggest running migration plan to start using migrations. - Marker != contract: DB and contract have diverged, suggest db verify to inspect.
… read Merge the two separate createControlClient → connect → readMarker blocks into one. The empty-bundles path is now an early return inside the same try/catch/finally, eliminating a duplicate client lifecycle and reducing nesting by two levels.
…TODOs
Merge the nested try { try { } finally { close } } catch { warn }
into a single try/catch/finally for marker probing. Remove three
stale TODO comments (refs utility — one-liner, flatten nesting —
already flat, simplify summary — not deeply nested).
Every scenario (S-1 through S-9) was already covered by dag.test.ts unit tests with better coverage (tie-breaking, output shape, orphan detection). The scenarios file was just calling findPath/findLeaf on trivial 2-3 node graphs — the same behaviors tested directly in the dag test suite. Integration coverage lives in the CLI e2e and apply test files which test the full pipeline on real files.
- Help formatter now renders positional arguments from command.registeredArguments. Previously only flags were shown, making required args like <name> and <hash> invisible. - ref set validates the hash value before writing via validateRefValue(). Previously invalid hashes were silently written to refs.json. - Rename local errorInvalidRefName to cliErrorInvalidRefName to avoid collision with the same-named factory in migration-tools errors.ts.
The refs.json path was inlined as resolve(migrationsDir, 'refs.json') in migration-apply and migration-status, while migration-ref had its own resolveRefsPath helper. Add refsPath to the shared resolveMigrationPaths return value and use it in both commands.
…odel - Subsystem doc: replace "DAG" with "directed graph", note cycle tolerance, add BFS shortest-path and refs description, add ADR 169 reference. - ADR 039: note that cycles are now tolerated (not errors) per ADR 169, add update paragraph explaining parentMigrationId removal and refs.
Project is complete — all milestones delivered in PR #232. Canonical documentation lives in ADR 169, ADR 039, and the migration subsystem doc. Completed sub-plans (pr-232-review-fixes, cli-output-pipeline, safety-guardrails) and the framework-components spec were already done or documented elsewhere. The arktype type duplication issue is tracked in projects/arktype-config-validation/.
… doc - ADR 028: "edges in a DAG" → "edges in a graph", fromCoreHash/toCoreHash → from/to storage hashes. - ADR 123: "dag/chain-breakage" → "dag/path-breakage". - Preflight doc: from.storageHash notation → from/to storage hashes.
saevarb
force-pushed
the
feat/state-transition-migrations
branch
from
407793e to
64efaa9
Compare
wmadden
added a commit
that referenced
this pull request
Mar 20, 2026
Rewrite for clarity and accuracy: ground motivation in realistic development pain points rather than theoretical risks, replace parent-pointer chain model with graph-topology BFS pathfinding and refs (per PR #232), update on-disk format examples to match real SqlMigrationPlanOperation shape, and add coverage for named refs as the multi-environment targeting mechanism.
4 tasks
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-2049
closes TML-2046
Intent
Replace
parentMigrationId-based migration ordering with BFS shortest-path over a contract-hash graph, add filesystem refs for multi-environment targeting, and bring the CLI commands (plan,apply,status) into alignment with the new model. The goal is a migration system where contract hashes are the sole source of truth for structural state, cycles are tolerated, divergent branches are detected with actionable diagnostics, and environments can independently track their position via named refs.This implements the on-disk migrations v2 spec (FR-1 through FR-7).
Change map
migration ref set/get/delete/listCLI command--refflag,PathDecisionin JSON output--refflag, display chain routing through marker,targetHashreplacesleafHash--frombypass skips graph reconstructionDefaultRendererreplaces hardcodedconvertDefaultThe story
Remove
parentMigrationIdfrom the migration model. The manifest, graph types, I/O, and attestation hash computation all drop the parent pointer. Ordering is now determined entirely by contract-hash graph topology.Rewrite pathfinding as BFS over contract-hash edges.
findPathdoes BFS from a start hash to a target hash, with deterministic neighbor sorting (label priority →createdAt→to→migrationId).findPathWithDecisionwraps this to produce aPathDecisionstruct capturing the selected path, alternative count, and tie-break reasons.findReachableLeavesis a new BFS that discovers all exit nodes reachable from a given hash.Detect divergence and cycle-without-exit as hard errors.
findLeafnow usesfindReachableLeaves— if multiple leaves exist, it throwsAMBIGUOUS_LEAFwith the computed divergence point and per-branch details. If no leaves exist (pure cycle), it throwsNO_RESOLVABLE_LEAF. Both errors include actionable remediation guidance.Add filesystem refs for environment positions. A new
refsmodule managesmigrations/refs.json— a flatname → contractHashmap with strict name/value validation and atomic writes. Themigration refCLI command providesset,get,delete,listsubcommands with--json/--quietsupport.Make CLI commands ref-aware.
migration apply --ref <name>resolves its target from refs instead ofcontract.json.migration status --ref <name>reports state relative to a ref target, with display chain routing through the database marker for correct applied/pending status in diamond graphs.migration plan --from <hash>skips graph reconstruction entirely, avoidingAMBIGUOUS_LEAFon divergent graphs.Fix
contractToSchemaIRto use target-provided default rendering. The hardcodedconvertDefaultfunction was producing incorrect SQL for target-specific default syntax. It's replaced by aDefaultRenderercallback that the target adapter provides, following the same inversion-of-control pattern asNativeTypeExpander.Surface
PathDecisionmetadata in JSON output. Bothmigration applyandmigration statusnow includepathDecisionin their--jsonoutput, withselectedPath(each entry:dirName,migrationId,from,to),alternativeCount, andtieBreakReasonsfor CI/agent consumption.Behavior changes & evidence
Pathfinding uses BFS shortest-path instead of parent-chain walking: The migration graph is a directed graph of contract-hash nodes. BFS picks the minimal-hop path with deterministic tie-breaking. Cycles are tolerated — they're valid graph structure but never loop infinitely.
findPath,findPathWithDecision,findReachableLeaves,reconstructGraphDivergent branches produce
AMBIGUOUS_LEAFwith divergence-point analysis: WhenfindLeafdetects multiple reachable leaves, the error now identifies the deepest common ancestor, lists each branch with its edge path, and suggestsmigration ref setor--fromfor resolution.findDivergencePoint, errors.tserrorAmbiguousLeafCycle-only graphs throw
NO_RESOLVABLE_LEAF: Graphs where every node has outgoing edges (no exit node) are detected and surfaced with guidance to use--from.findLeafNamed refs enable multi-environment migration targeting:
migrations/refs.jsonmaps names likestagingandproductionto contract hashes.migration apply --refandmigration status --refconsume refs as read-only targets.migration statusdisplay chain routes through the database marker: In diamond graphs, BFS fromEMPTY_CONTRACT_HASHto the target could pick a path that doesn't pass through the marker, leading to incorrect applied/pending annotations. The display chain is now computed asEMPTY→marker+marker→targetwhen online.resolveDisplayChainmigration plan --fromskips graph reconstruction: AvoidsAMBIGUOUS_LEAFerrors when planning on divergent graphs — the graph is not built at all, so leaf resolution never runs.JSON output includes
pathDecision.selectedPath: Bothmigration apply --jsonandmigration status --jsonemit the full selected path withdirName,migrationId,from,toper edge, plusalternativeCountandtieBreakReasons.contractToSchemaIRuses target-providedDefaultRenderer: The hardcodedconvertDefaultis replaced by a callback the target adapter provides, fixing incorrect default SQL rendering.migration ref --jsonoutput fixed: Theflags.json === 'object'check was always false (parseGlobalFlagsreturns a boolean). Changed toflags.json.--jsonoutputCompatibility / migration / risk
parentMigrationIdremoved from migration manifests. ThemigrationId(content hash) of every existing migration will change becauseparentMigrationIdwas part ofstrippedMeta. No external users exist yet, so this is acceptable.leafHashrenamed totargetHashinmigration statusJSON output. Agents or scripts parsingleafHashmust update.pathDecisionadded tomigration applyandmigration statusJSON output. Additive — existing consumers are unaffected.DefaultRendereris a new required option oncontractToSchemaIR. All call sites within the repo are updated. External targets must provide a renderer.Follow-ups / open questions
foldBfsandfoldDfsprimitives with domain logic expressed as step/accumulator functions. Documented in spec.md § Open Questions.migration rebasecommand: Manual rebase (delete stale edge, replan from new base) is sufficient for v1. Tooling may be added in a future project.migration apply(tampered attestation, corrupted manifests, etc.) at safety-guardrails-journey-plan.md.Non-goals / intentionally out of scope
parentMigrationId-based migration directoriesSummary by CodeRabbit
New Features
migration refcommand (set/get/delete/list) and--refformigration apply/statusto target named environment hashes.--fromto set planning origin.Bug Fixes
Documentation
Tests