Wire up class-based migration.ts new/apply/plan CLI commands#354
Merged
Wire up class-based migration.ts new/apply/plan CLI commands#354
new/apply/plan CLI commands#354Conversation
|
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:
📝 WalkthroughWalkthroughReplaces the Migration.plan() method with an abstract Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(0,120,200,0.5)
participant CLI
participant Planner
participant TargetCapability
participant FS
end
CLI->>Planner: plan(options { contract, fromHash, toHash, ... })
Planner->>TargetCapability: inspect migrations capability
alt descriptor strategy
TargetCapability->>Planner: planWithDescriptors -> descriptors
Planner->>TargetCapability: renderDescriptorTypeScript(descriptors, scaffoldCtx)
TargetCapability->>FS: writeMigrationTs(packageDir, content)
else class-based strategy
TargetCapability->>Planner: emit() -> MigrationPlan (operations + targetId)
Planner->>FS: copyContractToMigrationDir(packageDir, contractPath)
Planner->>FS: write ops.json and migration.json (attest)
end
CLI->>FS: read/write migration.json / ops.json
Estimated code review effort🎯 5 (Critical) | ⏱️ ~110 minutes Possibly related PRs
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)
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/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-pipeline-builder
@prisma-next/mongo-lowering
@prisma-next/mongo-wire
@prisma-next/sql-contract
@prisma-next/sql-errors
@prisma-next/sql-operations
@prisma-next/sql-schema-ir
@prisma-next/sql-contract-psl
@prisma-next/sql-contract-ts
@prisma-next/sql-contract-emitter
@prisma-next/sql-lane-query-builder
@prisma-next/sql-relational-core
@prisma-next/sql-builder
@prisma-next/target-postgres
@prisma-next/target-sqlite
@prisma-next/adapter-postgres
@prisma-next/adapter-sqlite
@prisma-next/driver-postgres
@prisma-next/driver-sqlite
commit: |
4 tasks
wmadden
force-pushed
the
tml-2219-a-cli-emit-unification
branch
from
3bde981 to
53f8188
Compare
wmadden
force-pushed
the
tml-2219-b-class-flow-scaffolding
branch
from
d1c595a to
1e0bc67
Compare
wmadden
force-pushed
the
tml-2219-a-cli-emit-unification
branch
from
53f8188 to
ba88a32
Compare
wmadden
force-pushed
the
tml-2219-b-class-flow-scaffolding
branch
from
1e0bc67 to
f001281
Compare
wmadden
changed the title
new/apply/plan with migration.ts files
wmadden
changed the title
new/apply/plan with migration.ts filesnew/apply/plan CLI commands
wmadden
force-pushed
the
tml-2219-a-cli-emit-unification
branch
2 times, most recently
from
50911ef to
75531bc
Compare
wmadden
force-pushed
the
tml-2219-b-class-flow-scaffolding
branch
from
f001281 to
92a699e
Compare
Contributor
Author
|
Re-triggering CI after rebase |
wmadden
force-pushed
the
tml-2219-b-class-flow-scaffolding
branch
from
1690cff to
1f00d90
Compare
Introduces the class-based migration authoring surface as the canonical form for migration files: - `Migration` base class evolves from a `.plan()` method to an `.operations` getter and `.describe()` metadata method, with a static `Migration.run(import.meta.url, M)` helper that produces fully-attested ops.json + migration.json artifacts when a migration file is run directly. - Targets opt into class-flow scaffolding by implementing `MigrationPlanner.emptyMigration(...)`, returning a `MigrationPlanWithAuthoringSurface` that the CLI can serialize back to a `migration.ts` source file via `renderTypeScript()`. - MongoDB target ships `PlannerProducedMongoMigration`, which extends the framework `Migration` base directly (avoiding a cycle with `family-mongo`) and renders authored MongoDB migration sources. - Postgres target adds a parallel `scaffolding.ts` and authoring-surface tests for its descriptor planner. - Adds the `placeholder(...)` utility (PN-MIG-2001) used by scaffolded migration sources to mark slots the user must fill in. Emit detects unfilled placeholders and surfaces a structured `PN-MIG-2001` error. - Adds an internal lint script enforcing framework/target import boundaries for the new spine. The class-flow shape is now the single supported authoring surface; the old `.plan()` API and existing example migrations are migrated to the new shape.
…adata Adds a test exercising `PlannerProducedMongoMigration.renderTypeScript()` with `kind` and `labels` set, which previously left the conditional spread branches in `renderTypeScript` uncovered. Restores 100% branch coverage so the CI coverage job stops failing for `3-mongo-target/1-mongo-target`.
wmadden
force-pushed
the
tml-2219-b-class-flow-scaffolding
branch
from
eca86d0 to
3ca7de9
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/1-framework/3-tooling/cli/test/lib/migration-emit.test.ts (1)
144-164:⚠️ Potential issue | 🟡 MinorRefactor error assertion to use async expectation pattern.
Replace manual try/catch with
await expect(...).rejects.toMatchObject(...)for cleaner, more idiomatic error testing:♻️ Suggested refactor
- let thrown: unknown; - try { - await emitMigration( - DIR, - makeCtx({ targetId: 'test-target' }) as unknown as Parameters<EmitMigration>[1], - ); - } catch (error) { - thrown = error; - } - - expect(thrown).toMatchObject({ - code: '2011', - message: 'Target migrations capability is incomplete', - }); - expect((thrown as Error & { why: string }).why).toContain( - 'implements neither `resolveDescriptors`', - ); + await expect( + emitMigration( + DIR, + makeCtx({ targetId: 'test-target' }) as unknown as Parameters<EmitMigration>[1], + ), + ).rejects.toMatchObject({ + code: '2011', + message: 'Target migrations capability is incomplete', + why: expect.stringContaining('implements neither `resolveDescriptors`'), + });🤖 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/lib/migration-emit.test.ts` around lines 144 - 164, Replace the manual try/catch in this test with the async Jest rejects pattern: call emitMigration once into a promise variable (e.g., const p = emitMigration(DIR, makeCtx({ targetId: 'test-target' }) as unknown as Parameters<EmitMigration>[1])) and then use await expect(p).rejects.toMatchObject({ code: '2011', message: 'Target migrations capability is incomplete' }); and also assert the why text with await expect(p).rejects.toHaveProperty('why', expect.stringContaining('implements neither `resolveDescriptors`')); this keeps the test concise and avoids running emitMigration twice while preserving the same assertions on emitMigration and the mocks.hasMigrationTs setup.packages/1-framework/3-tooling/cli/src/commands/migration-new.ts (1)
233-246:⚠️ Potential issue | 🟠 MajorUse
CliStructuredError.is()type guard instead of manual property checks.The code uses a weak defensive check that only verifies a string
codeproperty, but other errors (like Node.js system errors) may have stringcodeproperties without conforming toCliStructuredErrorshape. The proper pattern, established and used throughout the codebase (migration-emit.ts, migration-plan.ts, db-update.ts, contract-emit.ts, and others), is to use theCliStructuredError.is()type guard which validates the full error structure includingname,domain, andtoEnvelopefunction.Correct pattern
} catch (error) { - if ( - error instanceof Error && - 'code' in error && - typeof (error as { code?: unknown }).code === 'string' - ) { - return notOk(error as CliStructuredError); + if (CliStructuredError.is(error)) { + return notOk(error); } return notOk(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/cli/src/commands/migration-new.ts` around lines 233 - 246, Replace the manual defensive check in the catch block with the established type guard: use CliStructuredError.is(error) to detect structured CLI errors (instead of checking for a string 'code' property) and return notOk(error) when true; otherwise call notOk(errorUnexpected(...)) as before. Update the catch handling around the existing notOk and errorUnexpected calls so it references the CliStructuredError.is() guard and preserves the same error message envelope behavior.
🧹 Nitpick comments (9)
packages/1-framework/3-tooling/cli/test/control-api/db-update.test.ts (1)
48-50: Optional cleanup: deduplicate the repeatedrenderTypeScriptthrow stub.A shared helper (e.g.,
const unusedRenderTypeScript = ...) would reduce fixture noise and keep intent clearer.♻️ Minimal refactor sketch
+const unusedRenderTypeScript = () => { + throw new Error('not used in db update tests'); +}; // ... - renderTypeScript: () => { - throw new Error('not used in db update tests'); - }, + renderTypeScript: unusedRenderTypeScript,Also applies to: 251-253, 376-378
🤖 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/control-api/db-update.test.ts` around lines 48 - 50, Multiple fixtures repeat the same renderTypeScript stub throwing "not used in db update tests"; extract a single shared constant (e.g., unusedRenderTypeScript) at the top of the test file and replace all inline occurrences of renderTypeScript: () => { throw new Error('not used in db update tests'); } with renderTypeScript: unusedRenderTypeScript so the behavior is deduplicated and intent is clearer; ensure the new constant is in scope for all test fixtures that currently reference renderTypeScript.packages/1-framework/3-tooling/migration/src/io.ts (1)
93-93: Guard.jsonsuffix before deriving the.d.tspath.Current slicing assumes
contractJsonPathalways ends with.json. A defensive check makes misuse fail fast and avoids malformed sibling paths.♻️ Suggested hardening
- const dtsPath = `${contractJsonPath.slice(0, -'.json'.length)}.d.ts`; + if (!contractJsonPath.endsWith('.json')) { + throw new Error(`Expected contract path to end with ".json": ${contractJsonPath}`); + } + const dtsPath = `${contractJsonPath.slice(0, -'.json'.length)}.d.ts`;🤖 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/io.ts` at line 93, The code derives dtsPath from contractJsonPath by slicing off ".json" without checking the suffix, which can produce incorrect paths; update the logic around the dtsPath calculation (the line assigning dtsPath and any helper around it) to first verify contractJsonPath endsWith ".json" and if not, throw a clear error (or handle appropriately), then derive the .d.ts sibling (or use a safe replacement approach) so misuse fails fast and malformed sibling paths are avoided.packages/1-framework/3-tooling/cli/test/lib/migration-strategy.test.ts (1)
52-61: Use direct throw matchers for the sync error path.Line 53–57 can be simplified and made more expressive with
toThrowError.♻️ Suggested refactor
- let caught: unknown; - try { - migrationStrategy(capability, 'broken'); - } catch (error) { - caught = error; - } - - expect(caught).toBeInstanceOf(CliStructuredError); - expect((caught as CliStructuredError).code).toBe('2011'); - expect((caught as CliStructuredError).meta).toMatchObject({ targetId: 'broken' }); + expect(() => migrationStrategy(capability, 'broken')).toThrowError( + expect.objectContaining({ + code: '2011', + meta: expect.objectContaining({ targetId: 'broken' }), + }), + );As per coding guidelines:
Use expect(() => ...).toThrow(...) to assert errors instead of manual try/catch blocks in tests.🤖 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/lib/migration-strategy.test.ts` around lines 52 - 61, Replace the manual try/catch assertion with a direct throw matcher: call migrationStrategy(capability, 'broken') inside an expect wrapper and use toThrowError to assert it throws a CliStructuredError, then assert the thrown error's .code equals '2011' and .meta matches { targetId: 'broken' } using the matcher form (referencing migrationStrategy, CliStructuredError, code, meta, and capability to locate the test).packages/1-framework/3-tooling/cli/test/commands/migration-emit.test.ts (1)
134-141: Prefer matcher-based rejection assertion over manual try/catch.Line 135–139 uses control-flow flags for expected failure. Use an async matcher for clearer intent and failures.
♻️ Suggested refactor
- let thrown = false; - try { - await executeCommand(command, ['--dir', 'migrations/20260101_test', '--json']); - } catch { - thrown = true; - } - - expect(thrown).toBe(true); + await expect( + executeCommand(command, ['--dir', 'migrations/20260101_test', '--json']), + ).rejects.toBeDefined();As per coding guidelines:
Use expect(() => ...).toThrow(...) to assert errors instead of manual try/catch blocks in tests.🤖 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-emit.test.ts` around lines 134 - 141, The test uses a manual try/catch with a thrown flag to assert rejection when calling executeCommand(command, ['--dir', 'migrations/20260101_test', '--json']); replace that pattern with Jest's async rejection matcher: await expect(executeCommand(command, ['--dir', 'migrations/20260101_test', '--json'])).rejects.toThrow(); remove the thrown flag and try/catch block so the test uses the clearer matcher-based assertion on executeCommand.packages/3-targets/3-targets/postgres/test/migrations/planner.authoring-surface.test.ts (1)
72-82: Useexpect().toThrow()instead of manual try/catch.Per coding guidelines, prefer
expect(() => ...).toThrow(...)for error assertions. This also provides cleaner failure messages.♻️ Proposed refactor
- let caught: unknown; - try { - success.plan.renderTypeScript(); - } catch (err) { - caught = err; - } - - expect(caught).toBeInstanceOf(CliStructuredError); - expect((caught as CliStructuredError).code).toBe('2010'); - expect((caught as CliStructuredError).meta).toMatchObject({ targetId: 'postgres' }); + expect(() => success.plan.renderTypeScript()).toThrow( + expect.objectContaining({ + code: '2010', + meta: expect.objectContaining({ targetId: 'postgres' }), + }), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-targets/3-targets/postgres/test/migrations/planner.authoring-surface.test.ts` around lines 72 - 82, Replace the manual try/catch block that calls success.plan.renderTypeScript() and asserts on the caught value with Jest's built-in throw matcher: call expect(() => success.plan.renderTypeScript()).toThrow(CliStructuredError) to assert the error type, and if you still need to assert the error.code and error.meta, use expect(() => success.plan.renderTypeScript()).toThrowError(expect.objectContaining({ code: '2010', meta: expect.objectContaining({ targetId: 'postgres' }) })); this removes the manual caught variable and leverages Jest's expect().toThrow APIs while keeping references to success.plan.renderTypeScript and CliStructuredError.packages/2-mongo-family/9-family/test/mongo-emit.test.ts (1)
146-162: Useawait expect(...).rejects.toThrow()for async error assertions.Since
mongoEmitis async, userejects.toThrow()instead of manual try/catch for cleaner assertions per coding guidelines.♻️ Proposed refactor
- let thrown: unknown; - try { - await mongoEmit({ dir: pkgDir, frameworkComponents: [] }); - } catch (error) { - thrown = error; - } - - expect(CliStructuredError.is(thrown)).toBe(true); - expect(thrown).toMatchObject({ - code: '2001', - domain: 'MIG', - meta: { slot: 'backfill-product-status:run' }, - }); + await expect(mongoEmit({ dir: pkgDir, frameworkComponents: [] })).rejects.toMatchObject({ + code: '2001', + domain: 'MIG', + meta: { slot: 'backfill-product-status:run' }, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-mongo-family/9-family/test/mongo-emit.test.ts` around lines 146 - 162, Replace the manual try/catch used to capture errors from the async mongoEmit call with Jest's async assertion helpers: remove the thrown variable and try/catch and use await expect(mongoEmit({ dir: pkgDir, frameworkComponents: [] })).rejects.toMatchObject({ code: '2001', domain: 'MIG', meta: { slot: 'backfill-product-status:run' } }); and add a separate await expect(mongoEmit(...)).rejects.toSatisfy(err => CliStructuredError.is(err)) if you still need to assert CliStructuredError.is; target the mongoEmit invocation and the CliStructuredError.is check when making these replacements.packages/3-mongo-target/1-mongo-target/test/migration-e2e.test.ts (1)
270-283: Silent skip when dist files are missing may mask CI failures.The early return when dist files don't exist means this test silently passes if the build step was skipped. Consider using
it.skipIf()or logging a warning to make the skip explicit.♻️ Proposed alternative using explicit skip
- it('runs via ./migration.ts on POSIX (or node migration.ts on Windows) and prints operations JSON', async () => { + it('runs via ./migration.ts on POSIX (or node migration.ts on Windows) and prints operations JSON', async ({ skip }) => { const distsExist = await Promise.all([ stat(familyMongoDistMigration).then( () => true, () => false, ), stat(targetMongoDistMigration).then( () => true, () => false, ), ]); if (!distsExist.every(Boolean)) { + skip('dist files not built'); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-mongo-target/1-mongo-target/test/migration-e2e.test.ts` around lines 270 - 283, Instead of silently returning inside the test when the dist files are missing, make the skip explicit: move the stat checks for familyMongoDistMigration and targetMongoDistMigration out of the it(...) body and compute distsExist before defining the test, then conditionally register the test via test.skip(...) (or it.skip(...)) when !distsExist.every(Boolean); alternatively, if you must keep the check inside the test, replace the early return with a clear console.warn and call this.skip() / fail the test explicitly so CI doesn't silently pass — reference the distsExist variable and the familyMongoDistMigration and targetMongoDistMigration symbols to locate where to change the logic.packages/3-mongo-target/1-mongo-target/src/core/planner-produced-migration.ts (1)
45-52: Consider usingifDefined()utility for conditional spreads.Per coding guidelines, prefer
ifDefined()from@prisma-next/utils/definedfor conditional object spreads:♻️ Suggested refactor
+import { ifDefined } from '@prisma-next/utils/defined'; + renderTypeScript(): string { return renderCallsToTypeScript(this.calls, { from: this.meta.from, to: this.meta.to, - ...(this.meta.kind !== undefined ? { kind: this.meta.kind } : {}), - ...(this.meta.labels !== undefined ? { labels: this.meta.labels } : {}), + ...ifDefined(this.meta.kind, (kind) => ({ kind })), + ...ifDefined(this.meta.labels, (labels) => ({ labels })), }); }As per coding guidelines: "Use
ifDefined()from@prisma-next/utils/definedfor conditional object spreads instead of inline conditional spread patterns"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-mongo-target/1-mongo-target/src/core/planner-produced-migration.ts` around lines 45 - 52, In renderTypeScript(), replace the inline conditional spread expressions for meta.kind and meta.labels with the ifDefined() helper from `@prisma-next/utils/defined`: import ifDefined and call renderCallsToTypeScript(this.calls, { from: this.meta.from, to: this.meta.to, ...ifDefined(this.meta.kind, v => ({ kind: v })), ...ifDefined(this.meta.labels, v => ({ labels: v })) }) so the conditional properties are added via ifDefined rather than inline ternary spreads; update imports accordingly and keep function name renderTypeScript and variables this.calls/this.meta unchanged.packages/3-mongo-target/1-mongo-target/test/planner-produced-migration.test.ts (1)
84-97: Consider asserting optional metadata appears in rendered output.This test verifies
renderTypeScript()succeeds with optionalkindandlabels, but doesn't assert they appear in the output. If the authoring surface should embed this metadata, consider adding assertions:const source = migration.renderTypeScript(); expect(source).toContain('class M extends Migration'); expect(source).toContain(META.from); expect(source).toContain(META.to); + expect(source).toContain('baseline'); + expect(source).toContain('initial');If the metadata is intentionally omitted from the rendered TypeScript (only used at runtime), this test is fine as-is.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-mongo-target/1-mongo-target/test/planner-produced-migration.test.ts` around lines 84 - 97, The test currently builds a PlannerProducedMongoMigration (using CreateIndexCall) and calls renderTypeScript but doesn't assert that the optional metadata (kind and labels passed via META) are present in the rendered output; update the test to assert that the rendered source contains the kind string (e.g., 'baseline') and each label (e.g., 'initial', 'seed') so the test verifies metadata propagation from PlannerProducedMongoMigration.renderTypeScript; locate the test block that constructs migration and source and add assertions that source.includes the kind and labels.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/1-framework/1-core/errors/test/migration.test.ts`:
- Around line 24-39: The test replaced a try/catch with expect(() =>
...).toThrow() but lost validation of the thrown object's structure and the
.toEnvelope().code check; modify the test for placeholder('foo') to use
expect(() => { try { placeholder('foo'); } catch (e) { thrown = e as
CliStructuredError; throw e; } }).toThrow(expect.objectContaining({ code:
'2001', domain: 'MIG', meta: { slot: 'foo' } })), where thrown is declared as
CliStructuredError | undefined so you can afterwards assert
thrown?.toEnvelope().code === 'PN-MIG-2001'; keep references to placeholder,
CliStructuredError, and toEnvelope exactly as in the diff.
In `@packages/1-framework/3-tooling/migration/src/migration-base.ts`:
- Around line 201-205: The code only checks Array.isArray(ops) but doesn't
validate each operation object before persisting ops.json; update the logic that
handles instance.operations (ops) to iterate and validate every element has
required fields id, label, and operationClass (and that they are the expected
types/format), and throw a descriptive Error referencing "operations" if any
entry is missing/invalid so the write path is aborted; add the validation
immediately after const ops = instance.operations and before any file write to
ensure ops.json is never written with malformed entries.
- Around line 151-190: readExistingManifest currently swallows parse/hash
mismatches and allows stale/malformed migration.json fields (fromContract,
toContract, hints) to be reused; change it to fail fast: make
readExistingManifest throw on JSON parse errors instead of returning null, and
validate the manifest contents before returning—specifically verify
existing.fromContract?.storage?.storageHash === meta.from and
existing.toContract?.storage?.storageHash === meta.to (or otherwise validate the
stored contract bookends match the incoming meta) and that required
hints/authorship shapes exist; if any validation fails, throw a clear error so
the caller (which builds baseManifest and then calls computeMigrationId) cannot
silently reuse inconsistent fields. Ensure the calling code handles the thrown
error appropriately (propagates to the user) rather than treating it as “no
manifest.”
In `@scripts/lint-framework-target-imports.mjs`:
- Around line 72-73: The current comment-filter in
scripts/lint-framework-target-imports.mjs only checks trimmed.startsWith('*')
and trimmed.startsWith('//'), which misses block comment markers and allows
one-line block/JSDoc comments to be linted; update the conditional that skips
comment lines to also detect block comment starts and ends by adding checks for
trimmed.startsWith('/*') (including '/**') and trimmed === '*/' (or
trimmed.startsWith('*/')), so lines that begin a block comment or close one are
skipped by the same continue path (adjust the condition that uses trimmed to
include these additional cases).
---
Outside diff comments:
In `@packages/1-framework/3-tooling/cli/src/commands/migration-new.ts`:
- Around line 233-246: Replace the manual defensive check in the catch block
with the established type guard: use CliStructuredError.is(error) to detect
structured CLI errors (instead of checking for a string 'code' property) and
return notOk(error) when true; otherwise call notOk(errorUnexpected(...)) as
before. Update the catch handling around the existing notOk and errorUnexpected
calls so it references the CliStructuredError.is() guard and preserves the same
error message envelope behavior.
In `@packages/1-framework/3-tooling/cli/test/lib/migration-emit.test.ts`:
- Around line 144-164: Replace the manual try/catch in this test with the async
Jest rejects pattern: call emitMigration once into a promise variable (e.g.,
const p = emitMigration(DIR, makeCtx({ targetId: 'test-target' }) as unknown as
Parameters<EmitMigration>[1])) and then use await
expect(p).rejects.toMatchObject({ code: '2011', message: 'Target migrations
capability is incomplete' }); and also assert the why text with await
expect(p).rejects.toHaveProperty('why', expect.stringContaining('implements
neither `resolveDescriptors`')); this keeps the test concise and avoids running
emitMigration twice while preserving the same assertions on emitMigration and
the mocks.hasMigrationTs setup.
---
Nitpick comments:
In `@packages/1-framework/3-tooling/cli/test/commands/migration-emit.test.ts`:
- Around line 134-141: The test uses a manual try/catch with a thrown flag to
assert rejection when calling executeCommand(command, ['--dir',
'migrations/20260101_test', '--json']); replace that pattern with Jest's async
rejection matcher: await expect(executeCommand(command, ['--dir',
'migrations/20260101_test', '--json'])).rejects.toThrow(); remove the thrown
flag and try/catch block so the test uses the clearer matcher-based assertion on
executeCommand.
In `@packages/1-framework/3-tooling/cli/test/control-api/db-update.test.ts`:
- Around line 48-50: Multiple fixtures repeat the same renderTypeScript stub
throwing "not used in db update tests"; extract a single shared constant (e.g.,
unusedRenderTypeScript) at the top of the test file and replace all inline
occurrences of renderTypeScript: () => { throw new Error('not used in db update
tests'); } with renderTypeScript: unusedRenderTypeScript so the behavior is
deduplicated and intent is clearer; ensure the new constant is in scope for all
test fixtures that currently reference renderTypeScript.
In `@packages/1-framework/3-tooling/cli/test/lib/migration-strategy.test.ts`:
- Around line 52-61: Replace the manual try/catch assertion with a direct throw
matcher: call migrationStrategy(capability, 'broken') inside an expect wrapper
and use toThrowError to assert it throws a CliStructuredError, then assert the
thrown error's .code equals '2011' and .meta matches { targetId: 'broken' }
using the matcher form (referencing migrationStrategy, CliStructuredError, code,
meta, and capability to locate the test).
In `@packages/1-framework/3-tooling/migration/src/io.ts`:
- Line 93: The code derives dtsPath from contractJsonPath by slicing off ".json"
without checking the suffix, which can produce incorrect paths; update the logic
around the dtsPath calculation (the line assigning dtsPath and any helper around
it) to first verify contractJsonPath endsWith ".json" and if not, throw a clear
error (or handle appropriately), then derive the .d.ts sibling (or use a safe
replacement approach) so misuse fails fast and malformed sibling paths are
avoided.
In `@packages/2-mongo-family/9-family/test/mongo-emit.test.ts`:
- Around line 146-162: Replace the manual try/catch used to capture errors from
the async mongoEmit call with Jest's async assertion helpers: remove the thrown
variable and try/catch and use await expect(mongoEmit({ dir: pkgDir,
frameworkComponents: [] })).rejects.toMatchObject({ code: '2001', domain: 'MIG',
meta: { slot: 'backfill-product-status:run' } }); and add a separate await
expect(mongoEmit(...)).rejects.toSatisfy(err => CliStructuredError.is(err)) if
you still need to assert CliStructuredError.is; target the mongoEmit invocation
and the CliStructuredError.is check when making these replacements.
In
`@packages/3-mongo-target/1-mongo-target/src/core/planner-produced-migration.ts`:
- Around line 45-52: In renderTypeScript(), replace the inline conditional
spread expressions for meta.kind and meta.labels with the ifDefined() helper
from `@prisma-next/utils/defined`: import ifDefined and call
renderCallsToTypeScript(this.calls, { from: this.meta.from, to: this.meta.to,
...ifDefined(this.meta.kind, v => ({ kind: v })), ...ifDefined(this.meta.labels,
v => ({ labels: v })) }) so the conditional properties are added via ifDefined
rather than inline ternary spreads; update imports accordingly and keep function
name renderTypeScript and variables this.calls/this.meta unchanged.
In `@packages/3-mongo-target/1-mongo-target/test/migration-e2e.test.ts`:
- Around line 270-283: Instead of silently returning inside the test when the
dist files are missing, make the skip explicit: move the stat checks for
familyMongoDistMigration and targetMongoDistMigration out of the it(...) body
and compute distsExist before defining the test, then conditionally register the
test via test.skip(...) (or it.skip(...)) when !distsExist.every(Boolean);
alternatively, if you must keep the check inside the test, replace the early
return with a clear console.warn and call this.skip() / fail the test explicitly
so CI doesn't silently pass — reference the distsExist variable and the
familyMongoDistMigration and targetMongoDistMigration symbols to locate where to
change the logic.
In
`@packages/3-mongo-target/1-mongo-target/test/planner-produced-migration.test.ts`:
- Around line 84-97: The test currently builds a PlannerProducedMongoMigration
(using CreateIndexCall) and calls renderTypeScript but doesn't assert that the
optional metadata (kind and labels passed via META) are present in the rendered
output; update the test to assert that the rendered source contains the kind
string (e.g., 'baseline') and each label (e.g., 'initial', 'seed') so the test
verifies metadata propagation from
PlannerProducedMongoMigration.renderTypeScript; locate the test block that
constructs migration and source and add assertions that source.includes the kind
and labels.
In
`@packages/3-targets/3-targets/postgres/test/migrations/planner.authoring-surface.test.ts`:
- Around line 72-82: Replace the manual try/catch block that calls
success.plan.renderTypeScript() and asserts on the caught value with Jest's
built-in throw matcher: call expect(() =>
success.plan.renderTypeScript()).toThrow(CliStructuredError) to assert the error
type, and if you still need to assert the error.code and error.meta, use
expect(() =>
success.plan.renderTypeScript()).toThrowError(expect.objectContaining({ code:
'2010', meta: expect.objectContaining({ targetId: 'postgres' }) })); this
removes the manual caught variable and leverages Jest's expect().toThrow APIs
while keeping references to success.plan.renderTypeScript and
CliStructuredError.
🪄 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: 356f6008-9bf4-49c5-941a-cb4815e48625
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (61)
examples/mongo-demo/migrations/20260415_add-posts-author-index/migration.tsexamples/mongo-demo/test/manual-migration.test.tsexamples/retail-store/migrations/20260415_add-product-validation/migration.tsexamples/retail-store/test/manual-migration.test.tspackages/1-framework/1-core/errors/src/control.tspackages/1-framework/1-core/errors/src/exports/migration.tspackages/1-framework/1-core/errors/src/migration.tspackages/1-framework/1-core/errors/test/migration.test.tspackages/1-framework/1-core/framework-components/src/control-migration-types.tspackages/1-framework/1-core/framework-components/src/exports/control.tspackages/1-framework/3-tooling/cli/README.mdpackages/1-framework/3-tooling/cli/src/commands/migration-emit.tspackages/1-framework/3-tooling/cli/src/commands/migration-new.tspackages/1-framework/3-tooling/cli/src/commands/migration-plan.tspackages/1-framework/3-tooling/cli/src/control-api/operations/db-init.tspackages/1-framework/3-tooling/cli/src/control-api/operations/db-update.tspackages/1-framework/3-tooling/cli/src/lib/migration-emit.tspackages/1-framework/3-tooling/cli/src/lib/migration-strategy.tspackages/1-framework/3-tooling/cli/src/utils/cli-errors.tspackages/1-framework/3-tooling/cli/test/commands/migration-emit.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-plan-command.test.tspackages/1-framework/3-tooling/cli/test/control-api/db-update.test.tspackages/1-framework/3-tooling/cli/test/lib/migration-emit.test.tspackages/1-framework/3-tooling/cli/test/lib/migration-strategy.test.tspackages/1-framework/3-tooling/migration/src/exports/io.tspackages/1-framework/3-tooling/migration/src/exports/migration-ts.tspackages/1-framework/3-tooling/migration/src/io.tspackages/1-framework/3-tooling/migration/src/migration-base.tspackages/1-framework/3-tooling/migration/src/migration-ts.tspackages/1-framework/3-tooling/migration/src/runtime-detection.tspackages/1-framework/3-tooling/migration/test/io.test.tspackages/1-framework/3-tooling/migration/test/migration-base.test.tspackages/1-framework/3-tooling/migration/test/migration-ts.test.tspackages/1-framework/3-tooling/migration/test/runtime-detection.test.tspackages/2-mongo-family/4-query/query-ast/src/exports/control.tspackages/2-mongo-family/4-query/query-ast/src/migration-operation-types.tspackages/2-mongo-family/9-family/src/core/mongo-emit.tspackages/2-mongo-family/9-family/src/core/mongo-target-descriptor.tspackages/2-mongo-family/9-family/test/mongo-emit.test.tspackages/3-mongo-target/1-mongo-target/package.jsonpackages/3-mongo-target/1-mongo-target/src/core/mongo-planner.tspackages/3-mongo-target/1-mongo-target/src/core/planner-produced-migration.tspackages/3-mongo-target/1-mongo-target/src/core/render-typescript.tspackages/3-mongo-target/1-mongo-target/src/exports/control.tspackages/3-mongo-target/1-mongo-target/src/exports/migration.tspackages/3-mongo-target/1-mongo-target/test/migration-e2e.test.tspackages/3-mongo-target/1-mongo-target/test/mongo-planner.test.tspackages/3-mongo-target/1-mongo-target/test/planner-produced-migration.test.tspackages/3-mongo-target/1-mongo-target/test/render-ops.test.tspackages/3-mongo-target/1-mongo-target/test/render-typescript.test.tspackages/3-mongo-target/2-mongo-adapter/test/mongo-runner.test.tspackages/3-targets/3-targets/postgres/package.jsonpackages/3-targets/3-targets/postgres/src/core/migrations/scaffolding.tspackages/3-targets/3-targets/postgres/src/exports/control.tspackages/3-targets/3-targets/postgres/test/migrations/planner.authoring-surface.test.tspackages/3-targets/3-targets/postgres/test/migrations/planner.contract-to-schema-ir.test.tspackages/3-targets/3-targets/postgres/test/migrations/scaffolding.test.tsscripts/lint-framework-target-imports.mjstest/integration/test/mongo/migration-e2e.test.tstest/integration/test/mongo/migration-m2-vocabulary.test.tstest/integration/test/mongo/migration-psl-authoring.test.ts
Contributor
Author
|
Triage of CodeRabbit review #4136389683 (outside-diff + nitpicks) Decomposed and triaged independently from the inline threads (already triaged separately). Summary of decisions on the 11 findings in the review body: Will address in this PR
Will not address
Inline review threads have been triaged in their own threads (3 deferred to TML-2271 / TML-2272 and resolved; 2 acknowledged and left open for the implementer). |
Replace manual try/catch in the placeholder() test with
expect(() => ...).toThrow(expect.objectContaining({...})) per the
prefer-to-throw rule. The toEnvelope() coverage is preserved by
asserting against errorUnfilledPlaceholder() (the underlying builder)
without re-invoking the throwing call twice.
Addresses CodeRabbit review action A01 on PR #354.
Extend the comment-skip filter in lint-framework-target-imports.mjs to also match `/*`, `/**`, and `*/` line prefixes so single-line block and JSDoc comments containing the forbidden `@prisma-next/target-*` substring do not trigger false-positive failures. Addresses CodeRabbit review action A05 on PR #354.
Replace manual try/catch in the "throws errorTargetHasIncompleteMigrationCapabilities when neither capability is present" test with await expect(...).rejects.toMatchObject per the prefer-to-throw rule. Avoids running emitMigration twice and preserves all assertions (code, message, why substring). Addresses CodeRabbit review action A06a on PR #354.
Replace the manual instanceof Error + duck-typed code-string check in migration-new.ts with CliStructuredError.is(error). Aligns with the pattern used across migration-emit, migration-plan, db-update, and contract-emit. Real correctness gain: Node system errors (e.g. ENOENT) have string code properties without conforming to CliStructuredError, so they now correctly fall through to errorUnexpected(...). Addresses CodeRabbit review action A06b on PR #354.
Replace manual try/catch in the no-hooks-registered case with a single
expect(() => migrationStrategy(...)).toThrow(expect.objectContaining({
code, meta })) per the prefer-to-throw rule. Drops the unused
CliStructuredError import.
Addresses CodeRabbit review action A06e on PR #354.
Replace the let thrown = false / try { ... } catch { thrown = true; }
control-flow flag in the PN-MIG-2001 propagation test with
await expect(executeCommand(...)).rejects.toBeDefined() per the
prefer-to-throw rule.
Addresses CodeRabbit review action A06f on PR #354.
Replace manual try/catch in the descriptor-flow renderTypeScript()
test with expect(() => ...).toThrow(expect.objectContaining({ code,
meta })) per the prefer-to-throw rule. Drops the now-unused
CliStructuredError import.
Addresses CodeRabbit review action A06g on PR #354.
Contributor
Author
|
Iterate-loop #2 — A10a + A11/A12/A13 addressed Following up on the previous iterate-loop summary. The 4
Per-thread Done replies + resolutions posted on A11/A12/A13. A10a is sourced from the review body and has no inline thread — this comment is the receipt. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/3-mongo-target/1-mongo-target/test/migration-e2e.test.ts (1)
164-178:⚠️ Potential issue | 🟠 MajorAdd
readonly targetId = 'mongo';to generated migration class.The
renderCallsToTypeScript()function omits the required abstract memberreadonly targetIdfrom the generatedMigrationsubclass. While tests pass becausetsxexecutes without type checking, the generated scaffolds fail TypeScript compilation (tsc --noEmit). The tests at lines 164–178, 198–220, 236–248, and 289–302 only verify runtime execution viaops.jsonoutput; they do not validate that the generated source is valid TypeScript. Addreadonly targetId = 'mongo';to the generated class, and assert this field is present in the rendered output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-mongo-target/1-mongo-target/test/migration-e2e.test.ts` around lines 164 - 178, renderCallsToTypeScript currently emits a Migration subclass that omits the required abstract member `readonly targetId`, causing generated files to fail `tsc` type-checks; update the renderer (the function renderCallsToTypeScript in the TS render module) to include `readonly targetId = 'mongo';` in the generated class body for the Migration subclass and update any template or string generation that builds the class so the rendered source contains that field; ensure the Migration subclass output (the string produced by renderCallsToTypeScript) includes the `readonly targetId = 'mongo';` line so compiled consumers pass `tsc --noEmit`.
🧹 Nitpick comments (1)
packages/1-framework/3-tooling/cli/test/commands/migration-emit.test.ts (1)
134-136: Also assert the failure exit code.These cases now prove the JSON envelope, but not that the command exits non-zero. A regression that still prints the right error payload while calling
process.exit(0)would pass here; please assert the mockedprocess.exit()call alongside the envelope checks.As per coding guidelines, "
packages/1-framework/3-tooling/cli/**/*.{test,spec}.ts: Test CLI commands by spying onprocess.exit()to verify correct exit codes are called."Also applies to: 161-163, 179-181, 207-209, 224-226
🤖 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-emit.test.ts` around lines 134 - 136, The test currently only checks the JSON error envelope returned by executeCommand(command, ['--dir', 'migrations/20260101_test', '--json']) but does not assert the CLI exit code; update the test to spy on process.exit (e.g., jest.spyOn(process, 'exit').mockImplementation(() => { throw new Error('process.exit called'); })) before calling executeCommand, then assert that process.exit was called with a non-zero code (e.g., expect(process.exit).toHaveBeenCalledWith(expect.any(Number)) or a specific non-zero value) after the promise rejects; apply the same pattern to the other failing-case blocks that use executeCommand/command in this file so each test both verifies the JSON envelope and that process.exit was invoked with a non-zero exit code.
🤖 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-new.ts`:
- Around line 205-209: The call to assertFrameworkComponentsCompatible(...) is
currently outside the try/catch and can throw a CliStructuredError that escapes
the Result flow; move the assertion call into the existing try block that
handles the rest of validation so its errors are caught and returned via the
notOk() path. Specifically, relocate the
assertFrameworkComponentsCompatible(config.family.familyId,
config.target.targetId, [config.target, config.adapter,
...(config.extensionPacks ?? [])]) invocation inside the try that wraps the
migration creation/validation logic and ensure no duplicate early-return logic
is introduced when the assertion fails.
---
Outside diff comments:
In `@packages/3-mongo-target/1-mongo-target/test/migration-e2e.test.ts`:
- Around line 164-178: renderCallsToTypeScript currently emits a Migration
subclass that omits the required abstract member `readonly targetId`, causing
generated files to fail `tsc` type-checks; update the renderer (the function
renderCallsToTypeScript in the TS render module) to include `readonly targetId =
'mongo';` in the generated class body for the Migration subclass and update any
template or string generation that builds the class so the rendered source
contains that field; ensure the Migration subclass output (the string produced
by renderCallsToTypeScript) includes the `readonly targetId = 'mongo';` line so
compiled consumers pass `tsc --noEmit`.
---
Nitpick comments:
In `@packages/1-framework/3-tooling/cli/test/commands/migration-emit.test.ts`:
- Around line 134-136: The test currently only checks the JSON error envelope
returned by executeCommand(command, ['--dir', 'migrations/20260101_test',
'--json']) but does not assert the CLI exit code; update the test to spy on
process.exit (e.g., jest.spyOn(process, 'exit').mockImplementation(() => { throw
new Error('process.exit called'); })) before calling executeCommand, then assert
that process.exit was called with a non-zero code (e.g.,
expect(process.exit).toHaveBeenCalledWith(expect.any(Number)) or a specific
non-zero value) after the promise rejects; apply the same pattern to the other
failing-case blocks that use executeCommand/command in this file so each test
both verifies the JSON envelope and that process.exit was invoked with a
non-zero exit code.
🪄 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: 6cf4f798-5ec1-40b1-acbb-cfff14313683
📒 Files selected for processing (3)
packages/1-framework/3-tooling/cli/src/commands/migration-new.tspackages/1-framework/3-tooling/cli/test/commands/migration-emit.test.tspackages/3-mongo-target/1-mongo-target/test/migration-e2e.test.ts
Contributor
Author
|
Re-triage of new review activity (iterate-loop #3) Six new unresolved review threads + a third CodeRabbit review (#4136492857) + author self-comments came in after iterate-loop #2 (
Defers grouped under TML-2275 ("Model 'no origin contract' / baseline-origin properly across db-init, db-update, and Migration"). All three threads (A19/A20/A21) resolved with thumbs-down + ticket link. Pending will-address for next implementer pass: A16, A17, A18, A22a (folded into A18). All four inline threads left unresolved with thumbs-up + ack reply. |
wmadden
commented
Apr 19, 2026
…on new Move the framework-components compatibility assertion inside the existing try block in `migration new` so the `CliStructuredError` it throws on incompatible components flows through the catch and is returned via `notOk(error)` instead of crashing the command. Mirrors the pattern used by `migration emit` and `migration plan` for the same call.
Promote the runtime list of CLI error domains to a single `as const` tuple and derive `CliErrorDomain` from it via `(typeof CLI_ERROR_DOMAINS)[number]` so the type and the runtime membership check can no longer drift apart. The membership Set is constructed directly from the same tuple; `isCliErrorDomain` continues to gate by Set lookup.
`MongoMigration` is the family-owned base class for class-flow Mongo
migrations, and the `targetId` is fixed at `mongo` for every subclass.
Declare it as `readonly targetId = "mongo" as const` on the base class
itself so:
- user-authored migrations (the mongo-demo + retail-store example
migrations) no longer need to repeat the `override readonly targetId`
line, and
- renderer-generated scaffolds emitted by `renderCallsToTypeScript`
inherit `targetId` automatically, satisfying the abstract contract
on `Migration` without the renderer having to embed the literal.
Removes the now-redundant override from the two example user
migrations and from the three inline `class M extends Migration { ... }`
fixture strings in target-mongo `migration-e2e.test.ts`. Also subsumes
the CodeRabbit outside-diff finding that flagged
`renderCallsToTypeScript()` for omitting `targetId`.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/1-framework/3-tooling/cli/src/commands/migration-new.ts (1)
212-225: Consider rendering before mutating disk to avoid partial scaffolds on planner failure.Right now, package files are written before planner/render calls. If planner creation or
renderTypeScript()fails, you can leave behind a partially scaffolded migration directory. Consider computing rendered source first (or writing to a temp dir then committing writes).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/cli/src/commands/migration-new.ts` around lines 212 - 225, The current flow writes files (writeMigrationPackage, copyContractToMigrationDir) before running planner/renderer, risking partial scaffolds if migrations.createPlanner / planner.emptyMigration / renderTypeScript() throws; change the sequence to produce the rendered TypeScript first (call createControlStack, config.family.create, migrations.createPlanner, planner.emptyMigration, then call emptyPlan.renderTypeScript()) and only after successful render call writeMigrationPackage/copyContractToMigrationDir/writeMigrationTs — alternatively render and write into a temp directory and atomically move/commit to packageDir after all steps succeed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/1-framework/3-tooling/cli/src/commands/migration-new.ts`:
- Around line 212-225: The current flow writes files (writeMigrationPackage,
copyContractToMigrationDir) before running planner/renderer, risking partial
scaffolds if migrations.createPlanner / planner.emptyMigration /
renderTypeScript() throws; change the sequence to produce the rendered
TypeScript first (call createControlStack, config.family.create,
migrations.createPlanner, planner.emptyMigration, then call
emptyPlan.renderTypeScript()) and only after successful render call
writeMigrationPackage/copyContractToMigrationDir/writeMigrationTs —
alternatively render and write into a temp directory and atomically move/commit
to packageDir after all steps succeed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 77a2ab7e-af0a-4023-b650-cc4f3cb1335d
📒 Files selected for processing (6)
examples/mongo-demo/migrations/20260415_add-posts-author-index/migration.tsexamples/retail-store/migrations/20260415_add-product-validation/migration.tspackages/1-framework/1-core/errors/src/control.tspackages/1-framework/3-tooling/cli/src/commands/migration-new.tspackages/2-mongo-family/9-family/src/core/mongo-migration.tspackages/3-mongo-target/1-mongo-target/test/migration-e2e.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/retail-store/migrations/20260415_add-product-validation/migration.ts
Replace the `...(existing?.authorship ? { authorship } : {})` ternary in
`buildAttestedManifest` with `ifDefined("authorship", existing?.authorship)`
to match the codebase convention for conditionally-included optional
properties under exactOptionalPropertyTypes.
Addresses review comment:
#354 (comment)
The preamble in `mongo-emit.ts` was duplicating the cross-cutting story already documented in `@prisma-next/cli/lib/migration-emit` (CLI dispatch, who attests `migration.json`, the relationship to `Migration.run`, why both flows produce byte-identical artifacts). Two copies of the same narrative will drift. Trim the preamble to mongo-specific responsibilities only (accepted authoring shapes, dynamic-import + structured error propagation, dispatch on default-export shape, MigrationPlan-shape validation for the factory form, ops.json persistence) and link to the framework-side helper for the cross-cutting story. Addresses review comment: #354 (comment)
…ationPlanner directly
Drop the inline wrapper in `migrations.createPlanner` that bolted
`renderTypeScript()` onto plans and provided an out-of-band
`emptyMigration()`. Push both responsibilities into `PostgresMigrationPlanner`
itself so it implements `MigrationPlanner<"sql", "postgres">` directly:
- `plan()` accepts the framework option shape (contract/schema as
`unknown`, optional `fromHash`) and attaches the throwing
`renderTypeScript()` stub to success plans, since Postgres is
descriptor-flow and never authors plans back to TypeScript from a
planner-produced plan.
- `emptyMigration()` returns the descriptor-flow stub (a plan with no
operations whose `renderTypeScript()` calls `renderDescriptorTypeScript`).
- SQL-typed callers (notably this package`s unit tests) keep working
because their stricter `SqlMigrationPlannerPlanOptions` payloads
structurally satisfy the looser framework option shape, and the
success plan still extends `SqlMigrationPlan<PostgresPlanTargetDetails>`.
`migrations.createPlanner` is now just `return createPostgresMigrationPlanner()`.
Addresses review comment:
#354 (comment)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/1-framework/3-tooling/migration/src/migration-base.ts (1)
21-25: UseSchema.array()for the Arktype array field.This schema is using the string DSL for an array where the repo standard is the typed
.array()form.♻️ Suggested change
const MigrationMetaSchema = type({ from: 'string', to: 'string', 'kind?': "'regular' | 'baseline'", - 'labels?': 'string[]', + 'labels?': type('string').array(), });As per coding guidelines, "Use
Schema.array()for array definitions in Arktype (avoid tuple mistakes)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/3-tooling/migration/src/migration-base.ts` around lines 21 - 25, The MigrationMetaSchema currently defines the labels array using the string DSL; change it to use Arktype's typed API by replacing the string-based 'labels?' definition with Schema.array(Schema.string()). Locate the MigrationMetaSchema symbol in migration-base.ts and update the optional labels field to Schema.array(Schema.string()) (or Schema.array(Schema.string(), { optional: true }) depending on project convention) so the schema follows the repo standard and avoids tuple/DSL pitfalls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/1-framework/3-tooling/migration/src/migration-base.ts`:
- Around line 161-167: The toContract stub is being force-cast to Contract
(toContract: Contract) while missing many required fields, causing
validateContract(migration.toContract) to fail at runtime; replace the blind
cast by either constructing a full minimal contract via the existing
minimalContract(...) helper (or analogous factory) and assign its result to
toContract, or define and use a dedicated ContractBookend/ContractStub type for
the migration bookend and change the toContract declaration to that type; update
usages that call validateContract to only validate when a real Contract is
present or adapt them to accept the new Bookend type (references: toContract,
Contract, validateContract, minimalContract).
---
Nitpick comments:
In `@packages/1-framework/3-tooling/migration/src/migration-base.ts`:
- Around line 21-25: The MigrationMetaSchema currently defines the labels array
using the string DSL; change it to use Arktype's typed API by replacing the
string-based 'labels?' definition with Schema.array(Schema.string()). Locate the
MigrationMetaSchema symbol in migration-base.ts and update the optional labels
field to Schema.array(Schema.string()) (or Schema.array(Schema.string(), {
optional: true }) depending on project convention) so the schema follows the
repo standard and avoids tuple/DSL pitfalls.
🪄 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: fa23396f-901a-4837-a07a-6ea0cd8493d3
📒 Files selected for processing (4)
packages/1-framework/3-tooling/migration/src/migration-base.tspackages/2-mongo-family/9-family/src/core/mongo-emit.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner.tspackages/3-targets/3-targets/postgres/src/exports/control.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/2-mongo-family/9-family/src/core/mongo-emit.ts
…Schema labels
Replace the string-DSL `labels?: string[]` declaration with
`labels?: type("string").array()` per the repo arktype-usage rule
(prefer `Schema.array()` over the string form).
Behaviour-preserving; migration-tools typecheck + 152 tests pass.
Contributor
Author
|
Iterate-loop #4 follow-up on CodeRabbit review #4137274537 and #4136518291. Both findings were nitpicks attached to the review body (not to resolvable threads). A27b — Arktype
|
wmadden
added a commit
that referenced
this pull request
Apr 20, 2026
Replace manual try/catch in the placeholder() test with
expect(() => ...).toThrow(expect.objectContaining({...})) per the
prefer-to-throw rule. The toEnvelope() coverage is preserved by
asserting against errorUnfilledPlaceholder() (the underlying builder)
without re-invoking the throwing call twice.
Addresses CodeRabbit review action A01 on PR #354.
wmadden
added a commit
that referenced
this pull request
Apr 20, 2026
Extend the comment-skip filter in lint-framework-target-imports.mjs to also match `/*`, `/**`, and `*/` line prefixes so single-line block and JSDoc comments containing the forbidden `@prisma-next/target-*` substring do not trigger false-positive failures. Addresses CodeRabbit review action A05 on PR #354.
wmadden
added a commit
that referenced
this pull request
Apr 20, 2026
Replace manual try/catch in the "throws errorTargetHasIncompleteMigrationCapabilities when neither capability is present" test with await expect(...).rejects.toMatchObject per the prefer-to-throw rule. Avoids running emitMigration twice and preserves all assertions (code, message, why substring). Addresses CodeRabbit review action A06a on PR #354.
wmadden
added a commit
that referenced
this pull request
Apr 20, 2026
Replace the manual instanceof Error + duck-typed code-string check in migration-new.ts with CliStructuredError.is(error). Aligns with the pattern used across migration-emit, migration-plan, db-update, and contract-emit. Real correctness gain: Node system errors (e.g. ENOENT) have string code properties without conforming to CliStructuredError, so they now correctly fall through to errorUnexpected(...). Addresses CodeRabbit review action A06b on PR #354.
wmadden
added a commit
that referenced
this pull request
Apr 20, 2026
Replace manual try/catch in the no-hooks-registered case with a single
expect(() => migrationStrategy(...)).toThrow(expect.objectContaining({
code, meta })) per the prefer-to-throw rule. Drops the unused
CliStructuredError import.
Addresses CodeRabbit review action A06e on PR #354.
wmadden
added a commit
that referenced
this pull request
Apr 20, 2026
Replace the let thrown = false / try { ... } catch { thrown = true; }
control-flow flag in the PN-MIG-2001 propagation test with
await expect(executeCommand(...)).rejects.toBeDefined() per the
prefer-to-throw rule.
Addresses CodeRabbit review action A06f on PR #354.
wmadden
added a commit
that referenced
this pull request
Apr 20, 2026
Replace manual try/catch in the descriptor-flow renderTypeScript()
test with expect(() => ...).toThrow(expect.objectContaining({ code,
meta })) per the prefer-to-throw rule. Drops the now-unused
CliStructuredError import.
Addresses CodeRabbit review action A06g on PR #354.
wmadden
added a commit
that referenced
this pull request
Apr 20, 2026
Replace try/catch in the PN-MIG-2001 propagation test with
await expect(mongoEmit(...)).rejects.toMatchObject({ code, domain,
meta }) per the prefer-to-throw rule. Other tests in this file still
use the manual try/catch pattern (out of scope for this action) so
the CliStructuredError import is preserved.
Addresses CodeRabbit review action A06h on PR #354.
wmadden
added a commit
that referenced
this pull request
Apr 20, 2026
Use Vitest ctx.skip(reason) instead of an early return when the family-mongo and target-mongo dist migration entrypoints are not built. Silent early-return looks like a passing test in the runner output; ctx.skip(reason) reports the test as skipped with a clear explanation pointing at the missing build. Addresses CodeRabbit review action A06i on PR #354.
wmadden
added a commit
that referenced
this pull request
Apr 20, 2026
Replace inline `...(this.meta.kind !== undefined ? { kind: ... } : {})`
spreads in PlannerProducedMongoMigration.renderTypeScript() with
`...ifDefined(key, value)` from @prisma-next/utils/defined per the
use-if-defined rule. Behaviour preserved.
Addresses CodeRabbit review action A06j on PR #354.
wmadden
added a commit
that referenced
this pull request
Apr 20, 2026
Extend the optional-metadata renderTypeScript test to assert that `kind` and `labels` are actually embedded in the rendered source (\`kind: "baseline"\`, \`labels: ["initial", "seed"]\`). Add a paired case asserting that no `kind:` or `labels:` keys appear when the metadata is omitted, documenting the absence contract too. Addresses CodeRabbit review action A06k on PR #354.
wmadden
added a commit
that referenced
this pull request
Apr 20, 2026
Replace the `...(existing?.authorship ? { authorship } : {})` ternary in
`buildAttestedManifest` with `ifDefined("authorship", existing?.authorship)`
to match the codebase convention for conditionally-included optional
properties under exactOptionalPropertyTypes.
Addresses review comment:
#354 (comment)
wmadden
added a commit
that referenced
this pull request
Apr 20, 2026
The preamble in `mongo-emit.ts` was duplicating the cross-cutting story already documented in `@prisma-next/cli/lib/migration-emit` (CLI dispatch, who attests `migration.json`, the relationship to `Migration.run`, why both flows produce byte-identical artifacts). Two copies of the same narrative will drift. Trim the preamble to mongo-specific responsibilities only (accepted authoring shapes, dynamic-import + structured error propagation, dispatch on default-export shape, MigrationPlan-shape validation for the factory form, ops.json persistence) and link to the framework-side helper for the cross-cutting story. Addresses review comment: #354 (comment)
wmadden
added a commit
that referenced
this pull request
Apr 20, 2026
…ationPlanner directly
Drop the inline wrapper in `migrations.createPlanner` that bolted
`renderTypeScript()` onto plans and provided an out-of-band
`emptyMigration()`. Push both responsibilities into `PostgresMigrationPlanner`
itself so it implements `MigrationPlanner<"sql", "postgres">` directly:
- `plan()` accepts the framework option shape (contract/schema as
`unknown`, optional `fromHash`) and attaches the throwing
`renderTypeScript()` stub to success plans, since Postgres is
descriptor-flow and never authors plans back to TypeScript from a
planner-produced plan.
- `emptyMigration()` returns the descriptor-flow stub (a plan with no
operations whose `renderTypeScript()` calls `renderDescriptorTypeScript`).
- SQL-typed callers (notably this package`s unit tests) keep working
because their stricter `SqlMigrationPlannerPlanOptions` payloads
structurally satisfy the looser framework option shape, and the
success plan still extends `SqlMigrationPlan<PostgresPlanTargetDetails>`.
`migrations.createPlanner` is now just `return createPostgresMigrationPlanner()`.
Addresses review comment:
#354 (comment)
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.
Summary
This is PR B in the stacked split of #349 (base: PR A —
tml-2219-a-cli-emit-unification).Introduces the class-flow migration authoring spine: the canonical
shape for migration files becomes a TypeScript class extending the
framework
Migrationbase, with.operations,.describe(), and aself-running
Migration.run(import.meta.url, M)helper.Highlights
Migrationbase class evolves from.plan()to a.operationsgetter +
.describe()metadata.Migration.run(...)produces fullyattested
ops.json+migration.jsonartifacts when the migrationfile is run directly.
implementing
MigrationPlanner.emptyMigration(...), returning aMigrationPlanWithAuthoringSurfacethat the CLI can serialize back toa
migration.tssource file viarenderTypeScript().PlannerProducedMongoMigration(extendsMigrationdirectly to avoid a cycle withfamily-mongo), and theauthoring-surface
renderTypeScript()for authored Mongo migrationsources.
scaffolding.ts+ authoring-surfacetests for the descriptor planner.
placeholder(...)utility (PN-MIG-2001): scaffolded sources markunfilled slots with
placeholder(...). Emit detects unfilledplaceholders and surfaces
PN-MIG-2001.scripts/.The class-flow shape is now the single supported authoring surface;
the old
.plan()API and existing example migrations are migrated.Stacked PR context
This PR depends on the following stack:
Test plan
pnpm typecheck,pnpm lint,pnpm test)examples/mongo-demo,examples/retail-store) run via directtsxinvocationmigration.tsfor an empty Mongo plan compiles and runsPN-MIG-2001from emitSummary by CodeRabbit
New Features
Behavior Changes
Documentation
Tests