refactor(migration-tools): refs carry invariants, per-file layout#372
Open
refactor(migration-tools): refs carry invariants, per-file layout#372
Conversation
📝 WalkthroughWalkthroughMigration ref storage moved from single Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant MigrationAPI
participant FS
CLI->>MigrationAPI: resolveMigrationPaths() -> refsDir
CLI->>MigrationAPI: readRef(refsDir, name) / readRefs(refsDir)
MigrationAPI->>FS: read file `${refsDir}/${name}.json`
FS-->>MigrationAPI: file content (JSON)
MigrationAPI->>MigrationAPI: validate parse -> RefEntry {hash,invariants}
MigrationAPI-->>CLI: RefEntry (or Refs map)
CLI->>MigrationAPI: writeRef(refsDir, name, RefEntry)
MigrationAPI->>FS: write `${refsDir}/${name}.json`
FS-->>MigrationAPI: write success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
@prisma-next/mongo-runtime
@prisma-next/family-mongo
@prisma-next/sql-runtime
@prisma-next/family-sql
@prisma-next/middleware-telemetry
@prisma-next/mongo
@prisma-next/extension-paradedb
@prisma-next/extension-pgvector
@prisma-next/postgres
@prisma-next/sql-orm-client
@prisma-next/sqlite
@prisma-next/target-mongo
@prisma-next/adapter-mongo
@prisma-next/driver-mongo
@prisma-next/contract
@prisma-next/utils
@prisma-next/config
@prisma-next/errors
@prisma-next/framework-components
@prisma-next/operations
@prisma-next/ts-render
@prisma-next/contract-authoring
@prisma-next/ids
@prisma-next/psl-parser
@prisma-next/psl-printer
@prisma-next/cli
@prisma-next/emitter
@prisma-next/migration-tools
prisma-next
@prisma-next/vite-plugin-contract-emit
@prisma-next/runtime-executor
@prisma-next/mongo-codec
@prisma-next/mongo-contract
@prisma-next/mongo-value
@prisma-next/mongo-contract-psl
@prisma-next/mongo-contract-ts
@prisma-next/mongo-emitter
@prisma-next/mongo-schema-ir
@prisma-next/mongo-query-ast
@prisma-next/mongo-orm
@prisma-next/mongo-query-builder
@prisma-next/mongo-lowering
@prisma-next/mongo-wire
@prisma-next/sql-contract
@prisma-next/sql-errors
@prisma-next/sql-operations
@prisma-next/sql-schema-ir
@prisma-next/sql-contract-psl
@prisma-next/sql-contract-ts
@prisma-next/sql-contract-emitter
@prisma-next/sql-lane-query-builder
@prisma-next/sql-relational-core
@prisma-next/sql-builder
@prisma-next/target-postgres
@prisma-next/target-sqlite
@prisma-next/adapter-postgres
@prisma-next/adapter-sqlite
@prisma-next/driver-postgres
@prisma-next/driver-sqlite
commit: |
saevarb
force-pushed
the
feat/ref-invariants
branch
from
49516ef to
f52e08b
Compare
Spec covers the full end-to-end scope: ref-file refactor (P1, completed as M0), ledger foundation (M1), edge-level invariants + the suggested refactors (M2), the findPathWithInvariants pathfinder (M3), and the decision surface / CLI integration with ledger subtraction (M4). Key decisions: - D4: name is the single identity on DataTransformOperation; an invariant?: boolean flag (default true) controls routing visibility. Avoids the split-identity mental overhead of a separate invariantId field that would have duplicated name in ~95% of call sites. - D10: state-level dedup only, no dominance pruning. Realistic graphs don't produce the state-space blowup pruning targets. - D11: neighbour ordering prefers invariant-covering edges when required is non-empty; falls back to today's ordering otherwise. - D12: findLeaf / findLatestMigration stay structural. Plan ships each milestone as its own PR, M0 first.
Replaces the single top-level `refs.json` (a flat `{name → hash}` map)
with a per-ref file under `refs/<name>.json`. Each ref is now a
structured `RefEntry` that carries both its target hash and the list of
invariants the environment requires:
{ "hash": "sha256:…", "invariants": ["split-user-name"] }
This is the data-model foundation for invariant-aware (ref-aware)
routing. Routing itself still resolves to the hash — this commit does
not change how paths are selected. Follow-ups:
- Migration edges will carry "provides invariants" metadata (from
dataTransform operation descriptors).
- `findPath` / `findPathWithDecision` will be extended to require that
the cumulative provided-invariants along a path satisfies the ref's
required set, failing with a hard error when no satisfying path exists.
CLI call sites (`migration apply`, `migration status`) updated to unwrap
`.hash` from the new `RefEntry`. A stale CLI-level test that exercised
the old flat-file shape was removed; the package-level `refs.test.ts`
has fuller coverage of the new read/write surface (per-file directory,
error codes `INVALID_REF_FILE` / `INVALID_REF_NAME` / `INVALID_REF_VALUE`).
Cherry-picked from the in-progress `feat/data-migrations-invariant-routing`
branch so the ref-aware routing work can proceed on a focused branch.
All tests pass: migration-tools (191), CLI (532).
saevarb
force-pushed
the
feat/ref-invariants
branch
from
e4ef9dd to
6abbe53
Compare
saevarb
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
packages/1-framework/3-tooling/cli/src/commands/migration-status.ts (1)
376-397: Redundant ref-lookup fallback.The
if (refEntry) { … } else { resolveRef(...) }split is equivalent to just callingresolveRef(allRefs, activeRefName).hash.resolveRefperforms the same map lookup and raisesMIGRATION.UNKNOWN_REFwhen missing, plus validates the ref-name shape — which the directallRefs[activeRefName]path skips. Collapsing to a single call removes a branch and ensures invalid ref names are reported consistently.♻️ Simplification
if (options.ref) { activeRefName = options.ref; - const refEntry = allRefs[activeRefName]; - if (refEntry) { - activeRefHash = refEntry.hash; - } else { - try { - activeRefHash = resolveRef(allRefs, activeRefName).hash; - } catch (error) { - if (MigrationToolsError.is(error)) { - return notOk( - errorRuntime(error.message, { - why: error.why, - fix: error.fix, - meta: { code: error.code }, - }), - ); - } - throw error; - } - } + try { + activeRefHash = resolveRef(allRefs, activeRefName).hash; + } catch (error) { + if (MigrationToolsError.is(error)) { + return notOk( + errorRuntime(error.message, { + why: error.why, + fix: error.fix, + meta: { code: error.code }, + }), + ); + } + throw error; + } }🤖 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 376 - 397, The branch that directly reads allRefs[activeRefName] is redundant and skips validate logic; replace the if/else with a single call to resolveRef(allRefs, activeRefName).hash to obtain activeRefHash so invalid ref names are handled uniformly by resolveRef. Keep the existing try/catch around resolveRef and preserve the MigrationToolsError handling that returns notOk(errorRuntime(...)), referencing the symbols activeRefName, activeRefHash, allRefs, resolveRef, MigrationToolsError, notOk, and errorRuntime.packages/1-framework/3-tooling/cli/src/commands/migration-ref.ts (2)
47-50: Duplication withresolveMigrationPaths.
resolveRefsDirreimplements logic already present inresolveMigrationPathsfrom../utils/command-helpers.ts, which returnsrefsDiras part of its result. Reusing it keeps refs-dir resolution in one place and ensures all commands compute the same path.♻️ Proposed refactor
-import { addGlobalOptions, setCommandDescriptions } from '../utils/command-helpers'; +import { + addGlobalOptions, + resolveMigrationPaths, + setCommandDescriptions, +} from '../utils/command-helpers'; @@ -function resolveRefsDir(configPath?: string, config?: { migrations?: { dir?: string } }): string { - const base = configPath ? resolve(configPath, '..') : process.cwd(); - return resolve(base, config?.migrations?.dir ?? 'migrations', 'refs'); -}Then replace
resolveRefsDir(options.config, config)call sites withresolveMigrationPaths(options.config, config).refsDir.🤖 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-ref.ts` around lines 47 - 50, resolveRefsDir duplicates logic already implemented by resolveMigrationPaths; remove the local resolveRefsDir function and replace its call sites (e.g., resolveRefsDir(options.config, config)) with resolveMigrationPaths(options.config, config).refsDir, ensuring you import resolveMigrationPaths from ../utils/command-helpers.ts if not already imported and update any references to use the refsDir property returned by resolveMigrationPaths.
196-202:ref get(non-JSON) drops invariants from human-readable output.Given refs now carry
invariants, consider mirroring thelistcommand by appending an[invariants: …]suffix when present. Otherwise users can only discover invariants via--jsonorref list. Minor UX polish.🤖 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-ref.ts` around lines 196 - 202, The non-JSON (human) output in the ref get flow currently only prints value.hash and drops value.invariants; update the callback passed to handleResult so that when flags.json is false it prints value.hash and, if value.invariants exists and is non-empty, appends a human-readable suffix like " [invariants: …]" (e.g., join the invariants array with commas or format similarly to the ref list command). Locate the callback around handleResult(result, flags, ui, (value) => { ... }) and change the ui.output branch that currently does ui.output(value.hash) to build and output the combined string including invariants.packages/1-framework/3-tooling/migration/src/refs.ts (2)
141-146: Temp-file suffix can collide under concurrent writes.
Date.now()has millisecond granularity; twowriteRefcalls for the same name within the same tick (e.g., concurrent CLI invocations or tests) would both write to the same.name.json.<ms>.tmpand the subsequentrenameon one of them could lose the other's data mid-flight. A random suffix (e.g.,crypto.randomUUID()orrandomBytes(8).toString('hex')) makes the temp path unique regardless of timing.♻️ Proposed fix
- const tmpPath = join(dir, `.${name.split('/').pop()}.json.${Date.now()}.tmp`); + const tmpPath = join( + dir, + `.${name.split('/').pop()}.json.${Date.now()}.${randomBytes(6).toString('hex')}.tmp`, + );Plus
import { randomBytes } from 'node:crypto';.🤖 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 141 - 146, The temp-file suffix using Date.now() (in the tmpPath definition used before writeFile/rename) can collide under concurrent writes; replace the timestamp suffix with a cryptographically-unique suffix (e.g., crypto.randomUUID() or randomBytes(8).toString('hex')) and add the corresponding import from 'node:crypto' so each tmpPath for the same name is unique, leaving the rest of the logic (tmpPath variable, writeFile call, and rename to filePath) unchanged.
168-177:startsWith(refsDir)lacks a separator boundary.If
refsDiris/tmp/refsand a sibling dir/tmp/refs-backupexists,dir.startsWith(refsDir)returns true for that sibling. In practicediris always derived fromjoin(refsDir, name.json)so this won't trigger today, but the cleanup loop is robust only by accident. Consider guarding with a separator-aware check (e.g.,dir === refsDir || dir.startsWith(refsDir + '/')) to make the invariant explicit.🤖 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 168 - 177, The cleanup loop using dir, refsDir, dirname and rmdir should guard against false positives from startsWith by checking a path-separator boundary; replace the condition that uses dir.startsWith(refsDir) with a separator-aware check (e.g., require dir === refsDir or dir starts with refsDir + path.sep using path.sep) so sibling paths like /tmp/refs-backup are not treated as children of refsDir; update the while condition accordingly and keep the remainder of the rmdir/try/catch logic unchanged.packages/1-framework/3-tooling/migration/test/refs.test.ts (1)
273-348: Consider a test for files deeper than.jsonwith non-.jsonsiblings being pruned.The
ignores non-json filestest only checks at the top level. SincereadRefswalks recursively (readdir(..., { recursive: true })) and filters by.endsWith('.json'), a non-JSON file nested under a subdirectory (e.g.,envs/README.md) would also be ignored — a quick nested variant would lock that behavior in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/migration/test/refs.test.ts` around lines 273 - 348, Add a test to readRefs that confirms non-.json files are ignored in nested directories: create a subdirectory (e.g., 'envs'), write a valid JSON ref file (e.g., 'envs/staging.json') and a non-JSON sibling (e.g., 'envs/README.md'), call readRefs(refsDir) and assert the result only contains the 'envs/staging' entry; reference the readRefs helper used in refs.test.ts and mirror the style of the existing "ignores non-json files" test to ensure recursive filtering by .endsWith('.json') is validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/prisma-next-demo/migration-fixtures/linear/refs/prod.json`:
- Around line 2-6: The fixture's "hash" field is an object but must be a string
per the RefEntry schema and the nested "invariants" inside that object is
misplaced; update the "hash" property to the inner string value (e.g., replace
the object at "hash" with "contract-1" or the proper sha256 string) and move or
merge the array currently at "hash.invariants" into the top-level "invariants"
array (replacing or concatenating with the existing top-level "invariants" as
appropriate) so the file has a string "hash" and a single top-level "invariants"
array.
In `@packages/1-framework/3-tooling/cli/test/commands/migration-ref.test.ts`:
- Around line 442-448: The test currently contains a tautological assertion
`expect(markerAtB === refEntry.hash).toBe(true)`—replace it with an assertion
that verifies actual behavior: call findPath(graph, markerAtB, refEntry.hash)
and assert the expected result (e.g., an empty path or same-node result) such as
expect(findPath(graph, markerAtB, refEntry.hash)).toHaveLength(0) or
expect(...).toEqual([]); remove the original tautology and keep the check that
the path behavior for markerAtB matches the intended semantics.
- Around line 221-235: The test description "'apply does not mutate ref files'"
is inaccurate because the test never calls the migration apply path; either
change the test to invoke the actual apply flow (run the CLI command or function
that implements "migration apply" and then assert the ref files are
byte-identical before/after) referencing the migration apply entrypoint, or
rename the test to accurately describe what it does (e.g., "writeRef round-trips
through readRef/readRefs") and keep the existing assertions that
readRef/readRefs return the same objects written by writeRef; locate the current
test by the existing test name string and update it accordingly while keeping
the usage of writeRef, readRef, and readRefs intact.
In `@packages/1-framework/3-tooling/migration/src/refs.ts`:
- Around line 180-195: resolveRef currently looks up refs[name] which can return
inherited Object.prototype members (e.g., "constructor"); change resolveRef to
first check ownership with Object.hasOwn(refs, name) and throw the
MIGRATION.UNKNOWN_REF error if the property is not an own property, then read
refs[name]; alternatively ensure refs is created with no prototype
(Object.create(null)) in readRefs, but follow the repo guideline to prefer
Object.hasOwn in resolveRef (function resolveRef) to safely gate the lookup.
---
Nitpick comments:
In `@packages/1-framework/3-tooling/cli/src/commands/migration-ref.ts`:
- Around line 47-50: resolveRefsDir duplicates logic already implemented by
resolveMigrationPaths; remove the local resolveRefsDir function and replace its
call sites (e.g., resolveRefsDir(options.config, config)) with
resolveMigrationPaths(options.config, config).refsDir, ensuring you import
resolveMigrationPaths from ../utils/command-helpers.ts if not already imported
and update any references to use the refsDir property returned by
resolveMigrationPaths.
- Around line 196-202: The non-JSON (human) output in the ref get flow currently
only prints value.hash and drops value.invariants; update the callback passed to
handleResult so that when flags.json is false it prints value.hash and, if
value.invariants exists and is non-empty, appends a human-readable suffix like "
[invariants: …]" (e.g., join the invariants array with commas or format
similarly to the ref list command). Locate the callback around
handleResult(result, flags, ui, (value) => { ... }) and change the ui.output
branch that currently does ui.output(value.hash) to build and output the
combined string including invariants.
In `@packages/1-framework/3-tooling/cli/src/commands/migration-status.ts`:
- Around line 376-397: The branch that directly reads allRefs[activeRefName] is
redundant and skips validate logic; replace the if/else with a single call to
resolveRef(allRefs, activeRefName).hash to obtain activeRefHash so invalid ref
names are handled uniformly by resolveRef. Keep the existing try/catch around
resolveRef and preserve the MigrationToolsError handling that returns
notOk(errorRuntime(...)), referencing the symbols activeRefName, activeRefHash,
allRefs, resolveRef, MigrationToolsError, notOk, and errorRuntime.
In `@packages/1-framework/3-tooling/migration/src/refs.ts`:
- Around line 141-146: The temp-file suffix using Date.now() (in the tmpPath
definition used before writeFile/rename) can collide under concurrent writes;
replace the timestamp suffix with a cryptographically-unique suffix (e.g.,
crypto.randomUUID() or randomBytes(8).toString('hex')) and add the corresponding
import from 'node:crypto' so each tmpPath for the same name is unique, leaving
the rest of the logic (tmpPath variable, writeFile call, and rename to filePath)
unchanged.
- Around line 168-177: The cleanup loop using dir, refsDir, dirname and rmdir
should guard against false positives from startsWith by checking a
path-separator boundary; replace the condition that uses dir.startsWith(refsDir)
with a separator-aware check (e.g., require dir === refsDir or dir starts with
refsDir + path.sep using path.sep) so sibling paths like /tmp/refs-backup are
not treated as children of refsDir; update the while condition accordingly and
keep the remainder of the rmdir/try/catch logic unchanged.
In `@packages/1-framework/3-tooling/migration/test/refs.test.ts`:
- Around line 273-348: Add a test to readRefs that confirms non-.json files are
ignored in nested directories: create a subdirectory (e.g., 'envs'), write a
valid JSON ref file (e.g., 'envs/staging.json') and a non-JSON sibling (e.g.,
'envs/README.md'), call readRefs(refsDir) and assert the result only contains
the 'envs/staging' entry; reference the readRefs helper used in refs.test.ts and
mirror the style of the existing "ignores non-json files" test to ensure
recursive filtering by .endsWith('.json') is validated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 900e4d47-62eb-4d9d-a988-40de89a98353
⛔ Files ignored due to path filters (2)
projects/graph-based-migrations/plans/invariant-aware-routing-plan.mdis excluded by!projects/**projects/graph-based-migrations/specs/invariant-aware-routing.spec.mdis excluded by!projects/**
📒 Files selected for processing (44)
examples/prisma-next-demo/migration-fixtures/complex/refs.jsonexamples/prisma-next-demo/migration-fixtures/complex/refs/prod.jsonexamples/prisma-next-demo/migration-fixtures/complex/refs/staging.jsonexamples/prisma-next-demo/migration-fixtures/converging-branches/refs.jsonexamples/prisma-next-demo/migration-fixtures/converging-branches/refs/prod.jsonexamples/prisma-next-demo/migration-fixtures/diamond-sub-branch/refs.jsonexamples/prisma-next-demo/migration-fixtures/diamond-sub-branch/refs/experiment.jsonexamples/prisma-next-demo/migration-fixtures/diamond-sub-branch/refs/prod.jsonexamples/prisma-next-demo/migration-fixtures/diamond/refs.jsonexamples/prisma-next-demo/migration-fixtures/diamond/refs/prod.jsonexamples/prisma-next-demo/migration-fixtures/kitchen-sink/refs.jsonexamples/prisma-next-demo/migration-fixtures/kitchen-sink/refs/prod.jsonexamples/prisma-next-demo/migration-fixtures/linear/refs.jsonexamples/prisma-next-demo/migration-fixtures/linear/refs/prod.jsonexamples/prisma-next-demo/migration-fixtures/long-spine/refs.jsonexamples/prisma-next-demo/migration-fixtures/long-spine/refs/prod.jsonexamples/prisma-next-demo/migration-fixtures/long-spine/refs/staging.jsonexamples/prisma-next-demo/migration-fixtures/multi-branch/refs.jsonexamples/prisma-next-demo/migration-fixtures/multi-branch/refs/feature.jsonexamples/prisma-next-demo/migration-fixtures/multi-branch/refs/prod.jsonexamples/prisma-next-demo/migration-fixtures/multi-branch/refs/staging.jsonexamples/prisma-next-demo/migration-fixtures/multi-rollback-branch/refs.jsonexamples/prisma-next-demo/migration-fixtures/multi-rollback-branch/refs/prod.jsonexamples/prisma-next-demo/migration-fixtures/multi-rollback-branch/refs/staging.jsonexamples/prisma-next-demo/migration-fixtures/rollback-continue/refs.jsonexamples/prisma-next-demo/migration-fixtures/rollback-continue/refs/prod.jsonexamples/prisma-next-demo/migration-fixtures/sequential-diamonds/refs.jsonexamples/prisma-next-demo/migration-fixtures/sequential-diamonds/refs/prod.jsonexamples/prisma-next-demo/migration-fixtures/single-branch/refs.jsonexamples/prisma-next-demo/migration-fixtures/single-branch/refs/prod.jsonexamples/prisma-next-demo/migration-fixtures/single-branch/refs/staging.jsonexamples/prisma-next-demo/migration-fixtures/sub-branches/refs.jsonexamples/prisma-next-demo/migration-fixtures/sub-branches/refs/experiment.jsonexamples/prisma-next-demo/migration-fixtures/sub-branches/refs/feature.jsonexamples/prisma-next-demo/migration-fixtures/sub-branches/refs/prod.jsonpackages/1-framework/3-tooling/cli/src/commands/migration-apply.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/command-helpers.tspackages/1-framework/3-tooling/cli/test/commands/migration-ref.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-status.test.tspackages/1-framework/3-tooling/migration/src/exports/refs.tspackages/1-framework/3-tooling/migration/src/refs.tspackages/1-framework/3-tooling/migration/test/refs.test.ts
💤 Files with no reviewable changes (14)
- examples/prisma-next-demo/migration-fixtures/kitchen-sink/refs.json
- examples/prisma-next-demo/migration-fixtures/linear/refs.json
- examples/prisma-next-demo/migration-fixtures/converging-branches/refs.json
- examples/prisma-next-demo/migration-fixtures/rollback-continue/refs.json
- examples/prisma-next-demo/migration-fixtures/diamond-sub-branch/refs.json
- examples/prisma-next-demo/migration-fixtures/diamond/refs.json
- examples/prisma-next-demo/migration-fixtures/long-spine/refs.json
- examples/prisma-next-demo/migration-fixtures/single-branch/refs.json
- examples/prisma-next-demo/migration-fixtures/complex/refs.json
- examples/prisma-next-demo/migration-fixtures/multi-branch/refs.json
- examples/prisma-next-demo/migration-fixtures/sub-branches/refs.json
- examples/prisma-next-demo/migration-fixtures/sequential-diamonds/refs.json
- packages/1-framework/3-tooling/cli/test/commands/migration-status.test.ts
- examples/prisma-next-demo/migration-fixtures/multi-rollback-branch/refs.json
The file was corrupted during the per-file refs refactor — the hash
field was an object containing a placeholder 'contract-1' string and
experimental invariants ('foo', 'bar') not referenced anywhere else.
Restored to the original leaf-hash target with empty invariants, to
match every other fixture in the same directory.
- resolveRef: guard with Object.hasOwn so ref names like 'constructor'
don't walk the Object.prototype chain and bypass UNKNOWN_REF. Added a
regression test covering the 'constructor' lookup.
- migration-ref.test.ts: dropped a misnamed test ('apply does not
mutate ref files') that never invoked apply — duplicative of the
CRUD coverage in refs.test.ts.
- migration-ref.test.ts: replaced a tautological `markerAtB === refEntry.hash`
assertion with an actual findPath check (path from B to B is empty).
Drops the local resolveRefsDir helper; its logic (`resolve(migrations dir, 'refs')`) was identical to resolveMigrationPaths(...).refsDir. All four call sites swapped to destructure refsDir from the shared helper. Also removes the now-unused `resolve` import from pathe.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/1-framework/3-tooling/migration/src/refs.ts (1)
104-109: Silently swallowing non-ENOENTreadFileerrors inreadRefs.The empty
catch { continue }treats every read failure as a missing-file case. That's fine for a benign race (file deleted betweenreaddirandreadFile), but it also silently dropsEACCES,EPERM,EISDIR, etc., producing aRefsthat silently omits entries — callers will then see a misleadingUNKNOWN_REFdownstream. Consider narrowing the swallow to ENOENT only:♻️ Proposed change
let raw: string; try { raw = await readFile(filePath, 'utf-8'); - } catch { - continue; + } catch (error) { + if (error instanceof Error && (error as { code?: string }).code === 'ENOENT') { + continue; + } + throw error; }🤖 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 104 - 109, In readRefs, the catch block around readFile(filePath, 'utf-8') currently swallows all errors (declared via raw variable), which hides real failures; change the catch to inspect the thrown error (e.g., err.code) and only continue when err.code === 'ENOENT' (or err?.code === 'ENOENT'), otherwise rethrow the error so EACCES/EPERM/EISDIR/etc. bubble up; keep references to readRefs, readFile, filePath and the raw variable when making this change.
🤖 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/test/commands/migration-ref.test.ts`:
- Around line 419-424: The assertion using atTarget (const atTarget = markerHash
=== refEntry.hash; expect(atTarget).toBe(false);) is tautological because
markerHash and refEntry.hash are test constants; remove these two lines and
either (a) drop the redundant check entirely since findPath(graph, markerHash,
refEntry.hash) already asserts behavior, or (b) replace them with a meaningful
assertion such as verifying the path contents or distance (e.g., assert the
nodes/length returned by findPath or check resolver/distance logic related to
markerHash and refEntry.hash) so the test actually validates runtime behavior;
update the assertions around findPath, markerHash, and refEntry to reflect the
chosen approach.
---
Nitpick comments:
In `@packages/1-framework/3-tooling/migration/src/refs.ts`:
- Around line 104-109: In readRefs, the catch block around readFile(filePath,
'utf-8') currently swallows all errors (declared via raw variable), which hides
real failures; change the catch to inspect the thrown error (e.g., err.code) and
only continue when err.code === 'ENOENT' (or err?.code === 'ENOENT'), otherwise
rethrow the error so EACCES/EPERM/EISDIR/etc. bubble up; keep references to
readRefs, readFile, filePath and the raw variable when making this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: a3feccd0-cb6a-45f5-b6c6-1304628d5c81
📒 Files selected for processing (5)
examples/prisma-next-demo/migration-fixtures/linear/refs/prod.jsonpackages/1-framework/3-tooling/cli/src/commands/migration-ref.tspackages/1-framework/3-tooling/cli/test/commands/migration-ref.test.tspackages/1-framework/3-tooling/migration/src/refs.tspackages/1-framework/3-tooling/migration/test/refs.test.ts
✅ Files skipped from review due to trivial changes (1)
- examples/prisma-next-demo/migration-fixtures/linear/refs/prod.json
Comment on lines
419
to
424
| const markerHash = EMPTY_CONTRACT_HASH; | ||
| const pathToRef = findPath(graph, markerHash, refHash); | ||
| const pathToRef = findPath(graph, markerHash, refEntry.hash); | ||
| expect(pathToRef).toHaveLength(2); | ||
|
|
||
| const atTarget = markerHash === refHash; | ||
| const atTarget = markerHash === refEntry.hash; | ||
| expect(atTarget).toBe(false); |
There was a problem hiding this comment.
Tautological atTarget assertion.
markerHash === refEntry.hash just compares EMPTY_CONTRACT_HASH to HASH_B — both are constants set up in this same test, so the assertion can never fail regardless of the production code's behavior. Same class of issue as the previously-addressed markerAtB === refEntry.hash check. Consider dropping this pair of lines (the findPath(graph, markerHash, refEntry.hash) check on L420–421 already exercises the meaningful behavior) or replacing with an assertion against the resolver/distance logic you actually care about.
🤖 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 419 - 424, The assertion using atTarget (const atTarget =
markerHash === refEntry.hash; expect(atTarget).toBe(false);) is tautological
because markerHash and refEntry.hash are test constants; remove these two lines
and either (a) drop the redundant check entirely since findPath(graph,
markerHash, refEntry.hash) already asserts behavior, or (b) replace them with a
meaningful assertion such as verifying the path contents or distance (e.g.,
assert the nodes/length returned by findPath or check resolver/distance logic
related to markerHash and refEntry.hash) so the test actually validates runtime
behavior; update the assertions around findPath, markerHash, and refEntry to
reflect the chosen approach.
SevInf
reviewed
Apr 23, 2026
| try { | ||
| await rmdir(dir); | ||
| dir = dirname(dir); | ||
| } catch { |
Contributor
There was a problem hiding this comment.
This will ignore all js exceptions, not only ENOTEMPTY
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
2 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-2305
part of TML-2297
Intent
Lay the storage groundwork for invariant-aware migration routing. Environment refs need to carry required invariants alongside their target hash so that a future pathfinder (specced in the accompanying doc) can route through paths that satisfy those invariants. The ref format also moves from a single
refs.jsonblob to a per-file directory layout — one file per ref — so refs are independently versionable, diffable, and editable.This PR lands the M0 slice of the invariant-aware-routing spec. The spec + plan ship alongside it for context; subsequent PRs implement M1–M5.
Change map
RefEntry, Arktype schema, per-filereadRef/writeRef/deleteRef, directory walk forreadRefs, validation helpers, atomic-rename write, empty-parent cleanup on delete.refsPathtorefsDirin the shared path resolver..hashat the call site.ref set/list/rmrewritten against the per-file API.RefEntry.ref set,ref list,ref rm) against the per-file layout.refs/<name>.jsonper ref).refs.jsonAPI removed.The story
Refs gain invariants as a first-class field. Before this PR, a ref was
{ name: hash }inside a monolithicmigrations/refs.json. That format has nowhere to declare "to reach this target state, the database must have invariants X and Y satisfied" — which the invariant-aware routing spec needs.RefEntry = { hash, invariants: readonly string[] }replaces the bare-string value; the hash is still validated assha256:empty | sha256:<64-hex>, invariants are free-form names validated by Arktype.One file per ref, not one blob.
migrations/refs/<name>.jsonreplaces the oldmigrations/refs.json. Ref names are validated against a conservative pattern (^[a-z0-9](...)?(/[a-z0-9]...)*$) — lowercase, hyphen-allowed, slash-namespaced, no dots or trailing slashes. The layout supports nested refs (feature/my-branch) without schema pain; each ref is an independently editable file in the working tree, which plays better with git review.Storage primitives + helpers.
readRef(refsDir, name),readRefs(refsDir),writeRef(refsDir, name, entry),deleteRef(refsDir, name),resolveRef(refs, name),validateRefName,validateRefValue. Writes are atomic (tmp file +rename); deletes walk empty parent directories back up torefsDir; reads swallowENOENTand return empty to keep callers simple. Arktype schema narrows on parse to reject invalid hashes at the storage boundary.CLI call sites threaded through.
migration apply --ref,migration status --ref, andmigration ref set/list/rmall switch fromrefsPathtorefsDir.resolveRefnow returnsRefEntry, so the three callers that previously consumed a bare string (migration apply,migration status) unwrap.hashexplicitly. No behavior change for users who never pass--ref.Spec + plan land alongside. The docs commit establishes the end-to-end roadmap so future reviewers can see where M0 fits. M1–M5 decompose the remaining work into independently shippable PRs, with the test-coverage matrix mapping every acceptance criterion to a verification.
Behavior changes & evidence
Refs are structured records, not strings.
RefEntry = { hash, invariants: readonly string[] }. The CLI continues to accept a plain hash onmigration ref set <name> <hash>and implicitly usesinvariants: []; richer payloads are edited by hand in the per-file JSON for v1.On-disk layout: per-file, not monolithic.
migrations/refs/<name>.jsonreplacesmigrations/refs.json.readdirwithrecursive: true, atomic-rename write, empty-parent cleanup on delete.resolveRefreturn type changed fromstringtoRefEntry. Callers unwrap.hashexplicitly.apply --refandstatus --refend-to-end.migration ref set/list/rmrewritten against the per-file API.17 migration fixture directories migrated to the new layout. No behavior change — this is test data reflecting the new storage shape.
Compatibility / migration / risk
migrations/refs.jsonwon't be found by the new reader — refs live inmigrations/refs/*.jsonnow. The system is pre-1.0, but downstream users with an older ref file need to migrate manually (one JSON file per entry). Error surface is clean:readRefsswallows ENOENT and returns{}, so a missing directory looks like "no refs" rather than a hard failure.resolveRefnow returnsRefEntry, notstring. All call sites in this PR are updated; any consumer outside the workspace (there aren't any yet, since the package isn't published) would need to unwrap.hash.Follow-ups / open questions
This PR is M0 of the invariant-aware-routing workstream (TML-2297). The remaining milestones land as separate PRs:
prisma_contract.ledgergetsmigration_idandinvariantscolumns;ControlFamilyInstancegainsreadLedger();deriveEdgeStatusesconsults the ledger instead of inferring from graph paths. Closes TML-2130.MigrationChainEntry.invariantspopulated from ops;DataTransformOperation.invariant?: booleanopt-out. Optional renames:dag.ts→graph.ts,MigrationChainEntry→MigrationEdge.findPathWithInvariantswith state-level dedup + invariant-preferring neighbour ordering.migration apply --refandstatus --refcomputeeffective_required = ref.invariants − readLedger().appliedand route through the pathfinder.NO_INVARIANT_PATHsurfaces when unsatisfiable.data-migrations-spec.md.No open questions block this PR.
Non-goals / intentionally out of scope
ref.invariants. For v1, users edit the JSON files directly.refs.jsonblobs. Pre-existing internal consumers need to manually split their blob into per-file entries; no automated migration tool.Summary by CodeRabbit