refactor(cli): promote migration plan/show to aggregate, retire drift surface (TML-2457)#482
Merged
wmadden merged 18 commits intoMay 12, 2026
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughPhase-based migration planning: Phase 1 seeds per-extension contract artefacts and materialises descriptor-shipped migration packages to disk (recording newly created ChangesMigration planning: seed + aggregate load
sequenceDiagram
participant CLI as Migration CLI
participant Seed as Phase 1: Seed
participant Loader as Phase 2: Aggregate Loader
participant Planner as Migration Planner
participant User as User
CLI->>Seed: runContractSpaceSeedPhase(extensionPacks, migrationsDir)
Seed->>Seed: emit contract.json, placeholder .d.ts, refs/head.json
Seed->>Seed: materialize missing descriptor migration packages
Seed-->>CLI: emittedExtensionDirs[], seeded records
CLI->>Loader: buildContractSpaceAggregate(declaredExtensions, appMigrationPackages)
Loader->>Loader: validate aggregate from seeded disk state
Loader-->>CLI: aggregate
CLI->>Planner: planAggregate(aggregate.app.contract, aggregate.app.spaceId)
Planner-->>CLI: planned operations
CLI->>CLI: buildPlanSummary(plannedOpsCount, emittedExtensionDirsCount)
CLI-->>User: MigrationPlanResult (includes emittedExtensionDirs + summary)
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~70 minutes
🚥 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/1-framework/3-tooling/cli/src/control-api/operations/db-apply-aggregate.ts (1)
130-140:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate the orphan-marker preflight to apply mode only.
This runs before the
mode === 'plan'branch, sodb init/update --dry-runnow fails on orphan markers even though plan mode never advances markers. That blocks users from inspecting the aggregate plan before they clean up a retired space.Suggested fix
- const orphanMarkerError = detectOrphanMarkers(aggregate, markerRows); - if (orphanMarkerError !== null) { - throw orphanMarkerError; - } + if (mode === 'apply') { + const orphanMarkerError = detectOrphanMarkers(aggregate, markerRows); + if (orphanMarkerError !== null) { + throw orphanMarkerError; + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/1-framework/3-tooling/cli/src/control-api/operations/db-apply-aggregate.ts` around lines 130 - 140, The orphan-marker preflight (call to detectOrphanMarkers using aggregate and markerRows that sets orphanMarkerError and throws it) should only run when performing an apply, not during plan/dry-run; move or conditionally execute the detectOrphanMarkers check so it is skipped when mode === 'plan' (or when a dry-run flag is set) to avoid failing db init/update --dry-run while still preserving the check for actual apply operations.
🧹 Nitpick comments (7)
test/integration/test/cli.db-update.e2e.test.ts (1)
311-313: 💤 Low valueRemove transient milestone/task IDs from comment.
The comment references "M6 T6.5/T6.6" which are transient project artifacts that should not appear in source code. Describe the behavior change itself instead.
Proposed fix
- // M6 T6.5/T6.6: `Signature:` was renamed to per-space `marker:` - // (or `App-space marker:` in single-space fallback). + // `Signature:` was renamed to per-space `marker:` + // (or `App-space marker:` in single-space fallback). expect(applyOutput).toMatch(/marker:/);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/integration/test/cli.db-update.e2e.test.ts` around lines 311 - 313, Remove the transient milestone/task IDs ("M6 T6.5/T6.6") from the test comment and replace them with a short description of the behavior change: that the old "Signature:" label was renamed to a per-space "marker:" (and to "App-space marker:" when using the single-space fallback). Update the comment surrounding expect(applyOutput).toMatch(/marker:/) to reflect that behavior change (mention "Signature: renamed to per-space marker / App-space marker fallback") and delete any numeric milestone/task references.packages/1-framework/3-tooling/cli/src/utils/extension-pack-inputs.ts (1)
142-152: ⚡ Quick winPrefer an own-property check for
contractSpace.
'contractSpace' in rawalso matches prototype properties, so a class-based descriptor with an inherited getter can be treated as contract-space-bearing even when it never declared one itself.Object.hasOwn(raw as object, 'contractSpace')keeps the no-accessor behavior and matches the loader's intended semantics.Suggested change
- if ('contractSpace' in (raw as object)) { + if (Object.hasOwn(raw as object, 'contractSpace')) {As per coding guidelines, "Always use
Object.hasOwn()instead ofhasOwnProperty()to check for own properties."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/1-framework/3-tooling/cli/src/utils/extension-pack-inputs.ts` around lines 142 - 152, The function toDeclaredExtensionsFromRaw currently checks `'contractSpace' in (raw as object)` which matches inherited properties; change that check to use Object.hasOwn(raw as object, 'contractSpace') so only own properties are detected, leaving the rest of the logic (casting to `{ readonly id: string; readonly targetId: string }` and pushing to DeclaredExtensionEntry) unchanged; update the conditional in the loop over extensionPacks inside toDeclaredExtensionsFromRaw accordingly.packages/1-framework/3-tooling/cli/src/utils/contract-space-seed-phase.ts (1)
186-200: ⚡ Quick winDrop the generated
@ts-nocheckdirective.This placeholder
.d.tsalready type-checks as-is, so emitting//@ts-nocheck`` only weakens the repo's TS guarantees and can hide accidental regressions in future template edits.Suggested change
function buildPlaceholderContractDts(spaceId: string): string { return [ - '// `@ts-nocheck`', '/**', ` * Placeholder \`.d.ts\` for extension space "${spaceId}".`,As per coding guidelines, "Never use
@ts-nocheck."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/1-framework/3-tooling/cli/src/utils/contract-space-seed-phase.ts` around lines 186 - 200, The placeholder generator buildPlaceholderContractDts currently emits a leading "// `@ts-nocheck`" which weakens TypeScript guarantees; remove that entry from the returned array so the function no longer includes the `@ts-nocheck` directive (leave the rest of the JSDoc, export {}, and formatting intact so the string join still produces the same output minus the ts-nocheck line).packages/1-framework/3-tooling/cli/src/control-api/operations/apply-aggregate.ts (1)
91-95: 💤 Low valueRemove transient artifact references from comment.
The comment references "M6 AC4 / AC5" which are milestone acceptance criteria IDs. These should not appear in source code comments.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/1-framework/3-tooling/cli/src/control-api/operations/apply-aggregate.ts` around lines 91 - 95, Update the comment on the readonly property perSpace (typed as AggregatePerSpaceExecutionEntry[]) to remove the transient artifact references "M6 AC4 / AC5"; replace them with a short, stable descriptive phrase (for example "markers populated per acceptance criteria" or simply remove the parenthetical entirely) so the comment contains no milestone/AC IDs while still conveying that markers are populated for each space.packages/1-framework/3-tooling/cli/src/control-api/types.ts (2)
529-541: ⚡ Quick winRemove sub-spec section reference from JSDoc.
The comment references "Sub-spec §
migration applysemantics + § Required changes 1" — this is a transient project artifact. Describe the behavior directly instead.♻️ Suggested fix
/** * Options for the aggregate-walking `migrationApply` operation. * * The control-api operation is responsible for: loading the * contract-space aggregate, reading per-space marker rows from the * live database, plotting per-space paths via `graphWalkStrategy` * (replay-only — no synth, no introspection), and dispatching * through the shared `applyAggregate` primitive. The CLI command * just resolves the descriptor surface (config, refs, contract * envelope, app-space migration packages) and hands the inputs in. - * - * Sub-spec § `migration apply` semantics + § Required changes 1. */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/1-framework/3-tooling/cli/src/control-api/types.ts` around lines 529 - 541, The JSDoc for the migrationApply operation currently includes a transient project reference "Sub-spec § `migration apply` semantics + § Required changes 1"; update the comment for the migrationApply options by removing that sub-spec reference and replacing it with a concise plain-language description of the behavior and responsibilities (load contract-space aggregate, read per-space marker rows, plot per-space paths via graphWalkStrategy in replay-only mode, and dispatch via applyAggregate) so the docs are self-contained and do not reference internal artifacts; locate the block around the migrationApply/options JSDoc in types.ts and edit the comment text accordingly.
312-323: ⚡ Quick winRemove sub-spec section references from JSDoc.
The comment references "M6 sub-spec § Output shape contract" and "F4 / F7 from
e2e-verification.md" — these are transient project artifacts that should not appear in source comments. Describe the constraint or behavior itself instead.♻️ Suggested fix
/** * Per-space breakdown of an aggregate plan / apply. * * Surfaces the canonical schedule shape — extensions alphabetically, * then app — together with the operations attributed to each space and, * when the run was applied, the resulting per-space marker hash. - * - * M6 sub-spec § Output shape contract — every space involved in a run - * is observable in the success summary, including its post-apply - * marker, so the per-space invariant is visible to the user (closing - * F4 / F7 from `e2e-verification.md`). + * + * Every space involved in a run is observable in the success summary, + * including its post-apply marker, so the per-space invariant is + * visible to the user. */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/1-framework/3-tooling/cli/src/control-api/types.ts` around lines 312 - 323, Remove the transient project references in the JSDoc block that begins "Per-space breakdown of an aggregate plan / apply" and replace the "M6 sub-spec § Output shape contract — ... F4 / F7" sentence with a plain description of the behavior: state that the success summary includes every space involved in a run, lists extensions (alphabetically) then app, and includes each space's post-apply marker hash so the per-space invariant is observable to the user.packages/1-framework/3-tooling/migration/src/exports/aggregate.ts (1)
21-25: ⚡ Quick winVerify the new edge-ref type is exported from the public aggregate surface.
AggregatePerSpacePlannow carriesmigrationEdges, but this export list still doesn't surfaceAggregateMigrationEdgeRef. If consumers are meant to import the new edge metadata from@prisma-next/migration-tools/aggregate, add it here alongside the other planner types.Based on learnings: in packages with a
src/exportsdirectory, treat files there as explicit public API export surfaces.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/1-framework/3-tooling/migration/src/exports/aggregate.ts` around lines 21 - 25, The public aggregate export surface is missing the new edge-ref type; add an export for type AggregateMigrationEdgeRef (so consumers can import the migrationEdges metadata referenced on AggregatePerSpacePlan) to the aggregate exports list — e.g. export { type AggregateMigrationEdgeRef } from the module that defines it (referencing AggregatePerSpacePlan and AggregateMigrationEdgeRef to locate the declarations) alongside the existing graph-walk exports.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/1-framework/3-tooling/cli/src/commands/migration-plan.ts`:
- Around line 67-79: The plan result currently only exposes emittedExtensionDirs
and thus misses seed-phase updates that rewrite files without creating dirs;
update the plan result type/return in migration-plan.ts (where
emittedExtensionDirs is declared) to also include either the full seeded update
records or at minimum an array like seededExtensionUpdates: readonly { spaceId:
string; action: 'created' | 'updated' | 'skipped' }[]; ensure the code that
computes newMigrationDirs / seed-phase work populates this new
seededExtensionUpdates (and keep emittedExtensionDirs derived/flattened from it)
so JSON/no-op callers can detect non-directory mutations.
- Around line 290-299: Replace the ad-hoc familyInstanceForValidate created from
{} and any with the real family instance from the actual control stack: call
createControlStack(config) before invoking buildContractSpaceAggregate, grab the
family instance produced by that control stack (instead of calling
config.family.create({} as any)), and pass a validateContract wrapper that calls
that realInstance.validateContract; this ensures the driver parameter is
provided to ControlFamilyDescriptor.create() and removes the use of any.
In `@packages/1-framework/3-tooling/cli/src/commands/migration-show.ts`:
- Around line 315-323: The loop over aggregate.extensions currently skips
extensions whose on-disk directory is empty (when resolveLatestFromDir returns
value === null), which silently hides missing seeds/materializations; instead,
when extPkgResult.ok is true but extPkgResult.value === null, push an explicit
placeholder or error entry into spaces so the result reflects the loaded
extension even if nothing is on disk. Modify the branch inside the loop that
checks extPkgResult.value: if null, call or construct a placeholder space result
(include ext.spaceId, a marker like "missing-on-disk" or an error message, and
the client) and push it onto spaces; keep the existing behavior for
extPkgResult.ok === false (return extPkgResult) and continue to use
pkgToSpaceResult for non-null values.
- Around line 229-255: The code instantiates the family with a bogus empty
object (familyInstanceForValidate = config.family.create({} as any)), bypassing
the control-plane stack; replace that with a proper ControlStack built from the
existing config: call createControlStack(config) and use the resulting stack to
create the family instance (e.g., const stack = createControlStack(config);
const familyInstanceForValidate = stack.family.create(...)) so you no longer
pass {} as any or need the biome-ignore any suppressions, and keep
validateContract using familyInstanceForValidate.validateContract as before.
In `@packages/1-framework/3-tooling/cli/src/commands/migration-status.ts`:
- Around line 740-750: The current validateContract helper creates a family with
config.family.create({} as any), which bypasses create-time requirements (like
driver) and can hide errors; change validateContract to accept or receive an
existing family/validator instance instead of constructing one here, remove the
ad-hoc config.family.create({} as any) call, and thread the provided
family/validator through to call familyInstance.validateContract(json). Ensure
callers of validateContract pass a family created with the required driver
parameter (i.e., use ControlFamilyDescriptor.create(driver, ...) or the existing
family/validator already available in the command pipeline) so family
construction never uses an empty object and validation errors surface correctly.
- Around line 707-717: Currently any exception from client.readAllMarkers() is
swallowed and treated as "markers unknown"; change this to only fallback when
the method truly doesn't exist. Before calling, check that client.readAllMarkers
is a function (e.g., typeof client.readAllMarkers === 'function'); if it's
absent set allMarkers = null and skip the call, otherwise call await
client.readAllMarkers() and allow real runtime/query errors to propagate (do not
catch them and convert to null). Reference client.readAllMarkers and the
allMarkers variable when making this change.
- Around line 489-499: pendingCount is incorrectly counting operations via
walked.result.plan.operations.length (inflating counts when a migration has many
ops and missing zero-op migrations); update the logic in the migration-status
computation (where pendingCount is set, referencing pendingCount,
walked.result.plan.operations, graphWalkStrategy, liveMarker, and
MigrationStatusSpaceEntry['status']) to count pending migrations instead of
operations — e.g., derive pendingCount from the plan's migration entries (or
count distinct migration identifiers referenced by operations) so each authored
migration counts once and zero-op migrations are still reported as pending when
appropriate.
In
`@packages/1-framework/3-tooling/cli/src/control-api/operations/migration-apply.ts`:
- Around line 185-188: The fallback for "no marker" uses '' which differs from
the empty-hash sentinel used by graphWalkStrategy and can drop the structural
path; change the line that computes fromHash (currently using
liveMarker?.storageHash ?? '') to use the same empty-hash sentinel constant used
by graphWalkStrategy (e.g., EMPTY_CONTRACT_HASH or the project’s EMPTY_HASH
constant) so findPathWithDecision is called with the correct sentinel; update
imports or references as needed and keep the rest (structural,
findPathWithDecision, MIGRATION.NO_INVARIANT_PATH) unchanged.
In `@packages/1-framework/3-tooling/cli/src/utils/formatters/migrations.ts`:
- Around line 76-81: Replace the transient M6/F1/F4/F7 references with a direct
description of the formatter contract: state that this formatter produces a
per-space execution breakdown as an array of AggregatePerSpaceExecutionEntry
items, emitted in canonical schedule order (extensions listed alphabetically,
followed by the app), and that each entry contains per-space markers plus
operations grouped by space; ensure the comment names
AggregatePerSpaceExecutionEntry so readers can find the type definition.
In `@packages/1-framework/3-tooling/cli/test/commands/migration-show.test.ts`:
- Around line 460-499: The test "renders per-space section headings when
multiple spaces are present" currently only asserts presence of headings and dir
names; update it to assert ordering by checking the positions in the stripped
output produced by formatMigrationShowOutput: compute stripped.indexOf('── app
──') and stripped.indexOf('── cipherstash ──') (and/or the dir names
'20260101_100000_app_init' and '0000000001-init') and add assertions that the
app indices are less than the cipherstash indices so the test fails if the
sections render in the wrong order.
In `@packages/1-framework/3-tooling/cli/test/output.json-shapes.test.ts`:
- Around line 65-74: Test only asserts one perSpace entry shape, so a change
that reorders result.perSpace could slip through; update the test to explicitly
assert the canonical ordering of the full perSpace array by checking the
sequence of spaceId values on the actual MigrationApplyResult (e.g., read the
test's result.perSpace and assert result.perSpace.map(p => p.spaceId) equals the
expected array like ['pgvector', ...]) while keeping the existing per-entry
shape checks (the symbols to update are the test case that builds `entry:
MigrationApplyResult['perSpace'][number]`, the `expect(Object.keys(entry)...`
assertions, and whatever variable holds the full `result.perSpace` in this test
file).
In `@packages/1-framework/3-tooling/cli/test/output.migration-commands.test.ts`:
- Around line 22-24: The comment above the test expectation still references
transient planning artifacts like "M6 sub-spec § Output shape § footer" and must
be replaced with a stable, behavior-centric description; update the comment that
precedes the assertion on expect(stripped).toContain('Next: prisma-next
migration status') (and the other comments around lines covering the similar
block at 57-65) to simply state the observable behavior being asserted — e.g.,
"No-op apply still includes the canonical next-step hint so users know where to
verify state" — and remove any "M6", "AC*" or sub-spec references.
In `@packages/3-extensions/cipherstash/test/cipherstash-codec.test.ts`:
- Line 153: The test description contains transient artifact references ("AC7"
and "pre-M6"); update the describe block titled "operation labels (AC7 —
first-time-user-readable)" and any related inline comments to remove
milestone/acceptance IDs and instead state the behavior being validated (e.g.,
"operation labels — first-time-user readable" or "operation labels are
human-readable for first-time users"); locate and edit the describe(...) string
and nearby comments in the cipherstash-codec.test.ts test (the describe block
starting with "operation labels (AC7 — first-time-user-readable)") and change
the text and comments to describe the constraint/behavior without referencing
transient artifacts.
---
Outside diff comments:
In
`@packages/1-framework/3-tooling/cli/src/control-api/operations/db-apply-aggregate.ts`:
- Around line 130-140: The orphan-marker preflight (call to detectOrphanMarkers
using aggregate and markerRows that sets orphanMarkerError and throws it) should
only run when performing an apply, not during plan/dry-run; move or
conditionally execute the detectOrphanMarkers check so it is skipped when mode
=== 'plan' (or when a dry-run flag is set) to avoid failing db init/update
--dry-run while still preserving the check for actual apply operations.
---
Nitpick comments:
In
`@packages/1-framework/3-tooling/cli/src/control-api/operations/apply-aggregate.ts`:
- Around line 91-95: Update the comment on the readonly property perSpace (typed
as AggregatePerSpaceExecutionEntry[]) to remove the transient artifact
references "M6 AC4 / AC5"; replace them with a short, stable descriptive phrase
(for example "markers populated per acceptance criteria" or simply remove the
parenthetical entirely) so the comment contains no milestone/AC IDs while still
conveying that markers are populated for each space.
In `@packages/1-framework/3-tooling/cli/src/control-api/types.ts`:
- Around line 529-541: The JSDoc for the migrationApply operation currently
includes a transient project reference "Sub-spec § `migration apply` semantics +
§ Required changes 1"; update the comment for the migrationApply options by
removing that sub-spec reference and replacing it with a concise plain-language
description of the behavior and responsibilities (load contract-space aggregate,
read per-space marker rows, plot per-space paths via graphWalkStrategy in
replay-only mode, and dispatch via applyAggregate) so the docs are
self-contained and do not reference internal artifacts; locate the block around
the migrationApply/options JSDoc in types.ts and edit the comment text
accordingly.
- Around line 312-323: Remove the transient project references in the JSDoc
block that begins "Per-space breakdown of an aggregate plan / apply" and replace
the "M6 sub-spec § Output shape contract — ... F4 / F7" sentence with a plain
description of the behavior: state that the success summary includes every space
involved in a run, lists extensions (alphabetically) then app, and includes each
space's post-apply marker hash so the per-space invariant is observable to the
user.
In `@packages/1-framework/3-tooling/cli/src/utils/contract-space-seed-phase.ts`:
- Around line 186-200: The placeholder generator buildPlaceholderContractDts
currently emits a leading "// `@ts-nocheck`" which weakens TypeScript guarantees;
remove that entry from the returned array so the function no longer includes the
`@ts-nocheck` directive (leave the rest of the JSDoc, export {}, and formatting
intact so the string join still produces the same output minus the ts-nocheck
line).
In `@packages/1-framework/3-tooling/cli/src/utils/extension-pack-inputs.ts`:
- Around line 142-152: The function toDeclaredExtensionsFromRaw currently checks
`'contractSpace' in (raw as object)` which matches inherited properties; change
that check to use Object.hasOwn(raw as object, 'contractSpace') so only own
properties are detected, leaving the rest of the logic (casting to `{ readonly
id: string; readonly targetId: string }` and pushing to DeclaredExtensionEntry)
unchanged; update the conditional in the loop over extensionPacks inside
toDeclaredExtensionsFromRaw accordingly.
In `@packages/1-framework/3-tooling/migration/src/exports/aggregate.ts`:
- Around line 21-25: The public aggregate export surface is missing the new
edge-ref type; add an export for type AggregateMigrationEdgeRef (so consumers
can import the migrationEdges metadata referenced on AggregatePerSpacePlan) to
the aggregate exports list — e.g. export { type AggregateMigrationEdgeRef } from
the module that defines it (referencing AggregatePerSpacePlan and
AggregateMigrationEdgeRef to locate the declarations) alongside the existing
graph-walk exports.
In `@test/integration/test/cli.db-update.e2e.test.ts`:
- Around line 311-313: Remove the transient milestone/task IDs ("M6 T6.5/T6.6")
from the test comment and replace them with a short description of the behavior
change: that the old "Signature:" label was renamed to a per-space "marker:"
(and to "App-space marker:" when using the single-space fallback). Update the
comment surrounding expect(applyOutput).toMatch(/marker:/) to reflect that
behavior change (mention "Signature: renamed to per-space marker / App-space
marker fallback") and delete any numeric milestone/task references.
🪄 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: 1bfc8532-0e63-4dd4-b656-ef3d165fdfcb
⛔ Files ignored due to path filters (3)
projects/extension-contract-spaces/plan.mdis excluded by!projects/**projects/extension-contract-spaces/specs/migration-cli-aggregate.spec.mdis excluded by!projects/**projects/extension-contract-spaces/specs/migration-plan-show-aggregate.spec.mdis excluded by!projects/**
📒 Files selected for processing (50)
packages/1-framework/1-core/framework-components/src/control/control-capabilities.tspackages/1-framework/1-core/framework-components/src/control/control-migration-types.tspackages/1-framework/3-tooling/cli/src/commands/db-init.tspackages/1-framework/3-tooling/cli/src/commands/db-update.tspackages/1-framework/3-tooling/cli/src/commands/migration-apply.tspackages/1-framework/3-tooling/cli/src/commands/migration-plan.tspackages/1-framework/3-tooling/cli/src/commands/migration-show.tspackages/1-framework/3-tooling/cli/src/commands/migration-status.tspackages/1-framework/3-tooling/cli/src/control-api/client.tspackages/1-framework/3-tooling/cli/src/control-api/operations/apply-aggregate.tspackages/1-framework/3-tooling/cli/src/control-api/operations/db-apply-aggregate.tspackages/1-framework/3-tooling/cli/src/control-api/operations/migration-apply.tspackages/1-framework/3-tooling/cli/src/control-api/types.tspackages/1-framework/3-tooling/cli/src/utils/contract-space-aggregate-loader.tspackages/1-framework/3-tooling/cli/src/utils/contract-space-extension-migrations-pass.tspackages/1-framework/3-tooling/cli/src/utils/contract-space-migrate-pass.tspackages/1-framework/3-tooling/cli/src/utils/contract-space-seed-phase.tspackages/1-framework/3-tooling/cli/src/utils/extension-pack-inputs.tspackages/1-framework/3-tooling/cli/src/utils/formatters/migrations.tspackages/1-framework/3-tooling/cli/test/commands/migration-invariants.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-plan-command.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-show.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-status-aggregate-spaces.test.tspackages/1-framework/3-tooling/cli/test/control-api/client.test.tspackages/1-framework/3-tooling/cli/test/control-api/db-update.test.tspackages/1-framework/3-tooling/cli/test/output.db-update.test.tspackages/1-framework/3-tooling/cli/test/output.json-shapes.test.tspackages/1-framework/3-tooling/cli/test/output.migration-commands.test.tspackages/1-framework/3-tooling/cli/test/utils/contract-space-aggregate-loader.ac15.test.tspackages/1-framework/3-tooling/cli/test/utils/contract-space-extension-migrations-pass.test.tspackages/1-framework/3-tooling/cli/test/utils/contract-space-migrate-pass.test.tspackages/1-framework/3-tooling/cli/test/utils/contract-space-seed-phase.test.tspackages/1-framework/3-tooling/cli/test/utils/extension-pack-inputs.test.tspackages/1-framework/3-tooling/migration/src/aggregate/loader.tspackages/1-framework/3-tooling/migration/src/aggregate/planner-types.tspackages/1-framework/3-tooling/migration/src/aggregate/strategies/graph-walk.tspackages/1-framework/3-tooling/migration/src/detect-space-contract-drift.tspackages/1-framework/3-tooling/migration/src/exports/aggregate.tspackages/1-framework/3-tooling/migration/src/exports/spaces.tspackages/1-framework/3-tooling/migration/test/aggregate/loader.test.tspackages/1-framework/3-tooling/migration/test/deletable-node-modules.test.tspackages/1-framework/3-tooling/migration/test/detect-space-contract-drift.test.tspackages/2-mongo-family/9-family/src/core/mongo-target-descriptor.tspackages/3-extensions/cipherstash/src/core/cipherstash-codec.tspackages/3-extensions/cipherstash/test/cipherstash-codec.test.tspackages/3-targets/6-adapters/sqlite/test/migrations/db-init-update.cli.test.tstest/integration/test/cli.db-init.contract-space-verifier.test.tstest/integration/test/cli.db-update.e2e.test.tstest/integration/test/cli.db-verify.aggregate-schema.test.tstest/integration/test/cli.loader.drift.test.ts
💤 Files with no reviewable changes (9)
- packages/1-framework/3-tooling/migration/src/detect-space-contract-drift.ts
- packages/1-framework/3-tooling/migration/src/exports/spaces.ts
- test/integration/test/cli.loader.drift.test.ts
- packages/1-framework/3-tooling/migration/test/detect-space-contract-drift.test.ts
- packages/1-framework/3-tooling/cli/test/control-api/client.test.ts
- packages/1-framework/3-tooling/cli/src/utils/contract-space-migrate-pass.ts
- packages/1-framework/3-tooling/cli/test/utils/contract-space-migrate-pass.test.ts
- packages/1-framework/3-tooling/cli/test/utils/contract-space-extension-migrations-pass.test.ts
- packages/1-framework/3-tooling/cli/src/utils/contract-space-extension-migrations-pass.ts
wmadden
force-pushed
the
tml-2457-reduce-app_space_id-coupling-route-through-aggregate-slot
branch
from
b30f49a to
2789c80
Compare
@prisma-next/mongo-runtime
@prisma-next/family-mongo
@prisma-next/sql-runtime
@prisma-next/family-sql
@prisma-next/extension-arktype-json
@prisma-next/extension-cipherstash
@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/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: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/1-framework/3-tooling/cli/src/commands/migration-show.ts (1)
246-255: 💤 Low valueUse
ifDefined()for conditional object spread.Line 253 uses an inline conditional spread pattern. Per coding guidelines, prefer
ifDefined()from@prisma-next/utils/definedfor these cases.♻️ Suggested refactor
+import { ifDefined } from '@prisma-next/utils/defined'; + // ... const client = createControlClient({ family: config.family, target: config.target, adapter: config.adapter, - ...(config.driver ? { driver: config.driver } : {}), + ...ifDefined('driver', config.driver), extensionPacks: config.extensionPacks ?? [], });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 current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/1-framework/3-tooling/cli/src/commands/migration-show.ts` around lines 246 - 255, Replace the inline conditional spread for driver with ifDefined: import ifDefined from '@prisma-next/utils/defined' and change the object spread inside createControlClient to use ...ifDefined({ driver: config.driver }) instead of ...(config.driver ? { driver: config.driver } : {}), leaving other properties (family, target, adapter, extensionPacks) unchanged; ensure the new import is added at the top of the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/1-framework/3-tooling/cli/src/utils/extension-pack-inputs.ts`:
- Around line 141-149: The check using "'contractSpace' in (raw as object)" in
toDeclaredExtensionsFromRaw can throw for null or primitive values; update the
loop to first guard that raw is a non-null object (e.g. typeof raw === 'object'
&& raw !== null) before using the in operator, then narrow/cast to the expected
shape (pack with id and targetId) and only push when those properties exist;
this fixes the TypeError when extensionPacks contains nulls/primitives while
keeping the existing push semantics in toDeclaredExtensionsFromRaw.
---
Nitpick comments:
In `@packages/1-framework/3-tooling/cli/src/commands/migration-show.ts`:
- Around line 246-255: Replace the inline conditional spread for driver with
ifDefined: import ifDefined from '@prisma-next/utils/defined' and change the
object spread inside createControlClient to use ...ifDefined({ driver:
config.driver }) instead of ...(config.driver ? { driver: config.driver } : {}),
leaving other properties (family, target, adapter, extensionPacks) unchanged;
ensure the new import is added at the top of the file.
🪄 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: 02b00545-1707-456e-b7d6-7d6a2a9291bd
📒 Files selected for processing (22)
packages/1-framework/3-tooling/cli/src/commands/migration-plan.tspackages/1-framework/3-tooling/cli/src/commands/migration-show.tspackages/1-framework/3-tooling/cli/src/utils/contract-space-aggregate-loader.tspackages/1-framework/3-tooling/cli/src/utils/contract-space-extension-migrations-pass.tspackages/1-framework/3-tooling/cli/src/utils/contract-space-migrate-pass.tspackages/1-framework/3-tooling/cli/src/utils/contract-space-seed-phase.tspackages/1-framework/3-tooling/cli/src/utils/extension-pack-inputs.tspackages/1-framework/3-tooling/cli/src/utils/formatters/migrations.tspackages/1-framework/3-tooling/cli/test/commands/migration-plan-command.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-show.test.tspackages/1-framework/3-tooling/cli/test/utils/contract-space-aggregate-loader.ac15.test.tspackages/1-framework/3-tooling/cli/test/utils/contract-space-extension-migrations-pass.test.tspackages/1-framework/3-tooling/cli/test/utils/contract-space-migrate-pass.test.tspackages/1-framework/3-tooling/cli/test/utils/contract-space-seed-phase.test.tspackages/1-framework/3-tooling/cli/test/utils/extension-pack-inputs.test.tspackages/1-framework/3-tooling/migration/src/aggregate/loader.tspackages/1-framework/3-tooling/migration/src/detect-space-contract-drift.tspackages/1-framework/3-tooling/migration/src/exports/aggregate.tspackages/1-framework/3-tooling/migration/src/exports/spaces.tspackages/1-framework/3-tooling/migration/test/aggregate/loader.test.tspackages/1-framework/3-tooling/migration/test/deletable-node-modules.test.tspackages/1-framework/3-tooling/migration/test/detect-space-contract-drift.test.ts
💤 Files with no reviewable changes (8)
- packages/1-framework/3-tooling/migration/src/exports/spaces.ts
- packages/1-framework/3-tooling/cli/src/utils/contract-space-extension-migrations-pass.ts
- packages/1-framework/3-tooling/cli/src/utils/contract-space-migrate-pass.ts
- packages/1-framework/3-tooling/migration/src/exports/aggregate.ts
- packages/1-framework/3-tooling/migration/src/detect-space-contract-drift.ts
- packages/1-framework/3-tooling/migration/test/detect-space-contract-drift.test.ts
- packages/1-framework/3-tooling/cli/test/utils/contract-space-migrate-pass.test.ts
- packages/1-framework/3-tooling/cli/test/utils/contract-space-extension-migrations-pass.test.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/1-framework/3-tooling/cli/test/commands/migration-plan-command.test.ts
- packages/1-framework/3-tooling/cli/src/utils/contract-space-aggregate-loader.ts
- packages/1-framework/3-tooling/cli/src/utils/contract-space-seed-phase.ts
- packages/1-framework/3-tooling/migration/test/deletable-node-modules.test.ts
- packages/1-framework/3-tooling/cli/test/utils/contract-space-aggregate-loader.ac15.test.ts
- packages/1-framework/3-tooling/cli/test/utils/extension-pack-inputs.test.ts
- packages/1-framework/3-tooling/cli/test/utils/contract-space-seed-phase.test.ts
- packages/1-framework/3-tooling/cli/src/commands/migration-plan.ts
- packages/1-framework/3-tooling/cli/src/utils/formatters/migrations.ts
- packages/1-framework/3-tooling/migration/src/aggregate/loader.ts
- packages/1-framework/3-tooling/cli/test/commands/migration-show.test.ts
- packages/1-framework/3-tooling/migration/test/aggregate/loader.test.ts
Contributor
Author
|
Addressed the nitpicks from the CodeRabbit review (#482 (review)):
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/1-framework/3-tooling/cli/src/commands/migration-show.ts`:
- Around line 140-145: The code treats a non-null result from readMigrationsDir
followed by findLatestMigration(graph) === null as "no package" and returns
ok(null); instead treat this as data corruption and return a failure: replace
the check in migration-show.ts (around readMigrationsDir, reconstructGraph,
findLatestMigration) so that when allPackages.length > 0 and
findLatestMigration(graph) returns null you return an error result (or throw)
with a clear message about a corrupted/invalid migration graph rather than
ok(null), so callers don’t collapse the broken history into kind: 'missing'.
- Around line 279-280: The branch that accepts path-like targets (the if block
using looksLikePath(target) and readMigrationPackage(resolve(target))) should
validate that the resolved path is inside the application migrations directory
before proceeding; modify the logic in migration-show.ts so after computing
resolved = resolve(target) you check it is a descendant of the app migrations
root (use the same migration dir computation used elsewhere in this file/module)
and reject (throw an error or return a clear message) if it is not, rather than
loading it and emitting aggregate.app.spaceId; ensure the check is applied to
the path branch that calls readMigrationPackage to prevent accidentally treating
extension-space package paths as app-space results.
🪄 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: 4629ed7d-62d4-425a-9f14-e8294aeb93b1
📒 Files selected for processing (1)
packages/1-framework/3-tooling/cli/src/commands/migration-show.ts
wmadden
force-pushed
the
tml-2457-reduce-app_space_id-coupling-route-through-aggregate-slot
branch
from
9cba4a1 to
2ee0412
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/1-framework/3-tooling/cli/src/utils/formatters/migrations.ts (1)
9-12:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the
OQ-4planning artifact from these comments.Lines 9-12 and 25-26 still embed
spec OQ-4, which is the same transient-comment issue already cleaned up elsewhere in this file. Please describe the byte-identity behavior directly and drop the artifact ID.As per coding guidelines, "Source-code comments must not reference transient project artifacts including: projects//... paths, milestone-task IDs, milestone-named acceptance criteria, and sub-spec section references."✏️ Suggested wording
- * preview block. SQL statements get a trailing `;` if missing — matches the - * legacy `string[]`-based renderer byte-for-byte (per spec OQ-4). Other + * preview block. SQL statements get a trailing `;` if missing — matching the + * legacy `string[]`-based renderer byte-for-byte. Other - * legacy `DDL preview` label (preserves CLI byte-identity for SQL targets per - * spec OQ-4); previews from any other family — or a mix that includes any + * legacy `DDL preview` label to preserve CLI byte-identity for SQL targets; + * previews from any other family — or a mix that includes anyAlso applies to: 25-26
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/1-framework/3-tooling/cli/src/utils/formatters/migrations.ts` around lines 9 - 12, Remove the transient artifact reference "OQ-4" from the comment blocks that describe rendering a single statement of an OperationPreview and the later related comment (mentions lines referencing the legacy renderer); instead state the behavior directly — e.g. say SQL statements get a trailing ';' if missing to match the legacy `string[]`-based renderer exactly (byte-for-byte), and that other languages like 'mongodb-shell' render verbatim. Update the comment text around the OperationPreview description and the subsequent note (the one that also references the legacy renderer) to drop any OQ-4 ID and use the explicit byte-identity phrasing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/1-framework/3-tooling/cli/src/utils/formatters/migrations.ts`:
- Around line 9-12: Remove the transient artifact reference "OQ-4" from the
comment blocks that describe rendering a single statement of an OperationPreview
and the later related comment (mentions lines referencing the legacy renderer);
instead state the behavior directly — e.g. say SQL statements get a trailing ';'
if missing to match the legacy `string[]`-based renderer exactly
(byte-for-byte), and that other languages like 'mongodb-shell' render verbatim.
Update the comment text around the OperationPreview description and the
subsequent note (the one that also references the legacy renderer) to drop any
OQ-4 ID and use the explicit byte-identity phrasing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 20335559-540c-469f-858a-0b5129e9bd0a
⛔ Files ignored due to path filters (2)
projects/extension-contract-spaces/plan.mdis excluded by!projects/**projects/extension-contract-spaces/specs/migration-plan-show-aggregate.spec.mdis excluded by!projects/**
📒 Files selected for processing (35)
examples/cipherstash-integration/migrations/cipherstash/contract.d.tspackages/1-framework/3-tooling/cli/src/commands/migration-plan.tspackages/1-framework/3-tooling/cli/src/commands/migration-show.tspackages/1-framework/3-tooling/cli/src/commands/migration-status.tspackages/1-framework/3-tooling/cli/src/control-api/operations/db-apply-aggregate.tspackages/1-framework/3-tooling/cli/src/control-api/operations/migration-apply.tspackages/1-framework/3-tooling/cli/src/control-api/types.tspackages/1-framework/3-tooling/cli/src/utils/contract-space-aggregate-loader.tspackages/1-framework/3-tooling/cli/src/utils/contract-space-extension-migrations-pass.tspackages/1-framework/3-tooling/cli/src/utils/contract-space-migrate-pass.tspackages/1-framework/3-tooling/cli/src/utils/contract-space-seed-phase.tspackages/1-framework/3-tooling/cli/src/utils/extension-pack-inputs.tspackages/1-framework/3-tooling/cli/src/utils/formatters/migrations.tspackages/1-framework/3-tooling/cli/test/commands/migration-plan-command.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-show.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-status-aggregate-spaces.test.tspackages/1-framework/3-tooling/cli/test/output.json-shapes.test.tspackages/1-framework/3-tooling/cli/test/output.migration-commands.test.tspackages/1-framework/3-tooling/cli/test/utils/contract-space-aggregate-loader.ac15.test.tspackages/1-framework/3-tooling/cli/test/utils/contract-space-extension-migrations-pass.test.tspackages/1-framework/3-tooling/cli/test/utils/contract-space-migrate-pass.test.tspackages/1-framework/3-tooling/cli/test/utils/contract-space-seed-phase.test.tspackages/1-framework/3-tooling/cli/test/utils/extension-pack-inputs.test.tspackages/1-framework/3-tooling/migration/src/aggregate/loader.tspackages/1-framework/3-tooling/migration/src/aggregate/planner.tspackages/1-framework/3-tooling/migration/src/detect-space-contract-drift.tspackages/1-framework/3-tooling/migration/src/exports/aggregate.tspackages/1-framework/3-tooling/migration/src/exports/spaces.tspackages/1-framework/3-tooling/migration/test/aggregate/loader.test.tspackages/1-framework/3-tooling/migration/test/deletable-node-modules.test.tspackages/1-framework/3-tooling/migration/test/detect-space-contract-drift.test.tspackages/3-extensions/cipherstash/test/cipherstash-codec.test.tspackages/3-targets/6-adapters/sqlite/test/migrations/db-init-update.cli.test.tstest/integration/test/cli.db-verify.aggregate-schema.test.tstest/integration/test/cli.loader.drift.test.ts
💤 Files with no reviewable changes (8)
- packages/1-framework/3-tooling/migration/test/detect-space-contract-drift.test.ts
- packages/1-framework/3-tooling/migration/src/detect-space-contract-drift.ts
- test/integration/test/cli.loader.drift.test.ts
- packages/1-framework/3-tooling/cli/test/utils/contract-space-extension-migrations-pass.test.ts
- packages/1-framework/3-tooling/migration/src/exports/spaces.ts
- packages/1-framework/3-tooling/cli/test/utils/contract-space-migrate-pass.test.ts
- packages/1-framework/3-tooling/cli/src/utils/contract-space-migrate-pass.ts
- packages/1-framework/3-tooling/cli/src/utils/contract-space-extension-migrations-pass.ts
✅ Files skipped from review due to trivial changes (10)
- test/integration/test/cli.db-verify.aggregate-schema.test.ts
- packages/3-extensions/cipherstash/test/cipherstash-codec.test.ts
- packages/1-framework/3-tooling/cli/src/control-api/operations/db-apply-aggregate.ts
- packages/1-framework/3-tooling/cli/test/output.json-shapes.test.ts
- packages/1-framework/3-tooling/cli/src/control-api/operations/migration-apply.ts
- packages/1-framework/3-tooling/cli/test/output.migration-commands.test.ts
- packages/1-framework/3-tooling/cli/src/control-api/types.ts
- packages/1-framework/3-tooling/cli/test/utils/extension-pack-inputs.test.ts
- packages/3-targets/6-adapters/sqlite/test/migrations/db-init-update.cli.test.ts
- examples/cipherstash-integration/migrations/cipherstash/contract.d.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/1-framework/3-tooling/cli/test/commands/migration-plan-command.test.ts
- packages/1-framework/3-tooling/cli/test/utils/contract-space-aggregate-loader.ac15.test.ts
- packages/1-framework/3-tooling/cli/src/commands/migration-status.ts
- packages/1-framework/3-tooling/cli/test/commands/migration-status-aggregate-spaces.test.ts
- packages/1-framework/3-tooling/cli/test/utils/contract-space-seed-phase.test.ts
- packages/1-framework/3-tooling/migration/src/aggregate/planner.ts
- packages/1-framework/3-tooling/cli/src/commands/migration-show.ts
- packages/1-framework/3-tooling/cli/src/utils/contract-space-aggregate-loader.ts
- packages/1-framework/3-tooling/cli/src/utils/contract-space-seed-phase.ts
- packages/1-framework/3-tooling/migration/test/deletable-node-modules.test.ts
- packages/1-framework/3-tooling/cli/test/commands/migration-show.test.ts
- packages/1-framework/3-tooling/migration/src/exports/aggregate.ts
- packages/1-framework/3-tooling/migration/src/aggregate/loader.ts
- packages/1-framework/3-tooling/cli/src/utils/extension-pack-inputs.ts
wmadden
force-pushed
the
tml-2457-reduce-app_space_id-coupling-route-through-aggregate-slot
branch
2 times, most recently
from
8bd449a to
9a36495
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/1-framework/3-tooling/cli/test/commands/migration-show.test.ts (1)
30-36:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClean up the temp directories these tests create.
createTempDir()now backs several new tests, but nothing removes the directories afterward. That leaves real state behind between runs.Suggested fix
-import { mkdir } from 'node:fs/promises'; +import { mkdir, rm } from 'node:fs/promises'; ... -import { describe, expect, it } from 'vitest'; +import { afterEach, describe, expect, it } from 'vitest'; ... +const tempDirs = new Set<string>(); + async function createTempDir(prefix: string): Promise<string> { const dir = join( tmpdir(), `test-migration-show-${prefix}-${Date.now()}-${Math.random().toString(36).slice(2)}`, ); await mkdir(dir, { recursive: true }); + tempDirs.add(dir); return dir; } + +afterEach(async () => { + await Promise.all( + [...tempDirs].map((dir) => rm(dir, { recursive: true, force: true })), + ); + tempDirs.clear(); +});As per coding guidelines,
packages/1-framework/3-tooling/cli/**: “Tests must clean up their own ephemeral directories.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/1-framework/3-tooling/cli/test/commands/migration-show.test.ts` around lines 30 - 36, The tests create temporary directories via createTempDir (which uses mkdir) but never remove them; update the test suite to track each created path and remove them in an afterEach or afterAll hook (use fs.rm or rimraf with recursive: true / force: true) or change createTempDir to return a cleanup function; ensure cleanup runs even on failures by registering it in the test framework hooks and reference createTempDir when locating where to capture and delete the directories.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/1-framework/3-tooling/cli/src/commands/migration-show.ts`:
- Around line 115-119: The containment guard misidentifies cross-drive targets
because relative(appMigrationsDir, targetPath) can return an absolute path on
Windows; update the check that computes isOutsideAppDir (which currently uses
targetPath, relativeToApp, appMigrationsDir) to also reject absolute
relativeToApp values by calling path.isAbsolute(relativeToApp) (or isAbsolute on
the imported path module) so that any absolute path returned by relative is
treated as outside the app dir; keep the existing checks ('' | '.' |
startsWith('..')) and add the isAbsolute check to the OR chain.
---
Outside diff comments:
In `@packages/1-framework/3-tooling/cli/test/commands/migration-show.test.ts`:
- Around line 30-36: The tests create temporary directories via createTempDir
(which uses mkdir) but never remove them; update the test suite to track each
created path and remove them in an afterEach or afterAll hook (use fs.rm or
rimraf with recursive: true / force: true) or change createTempDir to return a
cleanup function; ensure cleanup runs even on failures by registering it in the
test framework hooks and reference createTempDir when locating where to capture
and delete the directories.
🪄 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: 69231072-3590-4b34-835e-97bbc74af202
📒 Files selected for processing (35)
examples/cipherstash-integration/migrations/cipherstash/contract.d.tspackages/1-framework/3-tooling/cli/src/commands/migration-plan.tspackages/1-framework/3-tooling/cli/src/commands/migration-show.tspackages/1-framework/3-tooling/cli/src/commands/migration-status.tspackages/1-framework/3-tooling/cli/src/control-api/operations/db-apply-aggregate.tspackages/1-framework/3-tooling/cli/src/control-api/operations/migration-apply.tspackages/1-framework/3-tooling/cli/src/control-api/types.tspackages/1-framework/3-tooling/cli/src/utils/contract-space-aggregate-loader.tspackages/1-framework/3-tooling/cli/src/utils/contract-space-extension-migrations-pass.tspackages/1-framework/3-tooling/cli/src/utils/contract-space-migrate-pass.tspackages/1-framework/3-tooling/cli/src/utils/contract-space-seed-phase.tspackages/1-framework/3-tooling/cli/src/utils/extension-pack-inputs.tspackages/1-framework/3-tooling/cli/src/utils/formatters/migrations.tspackages/1-framework/3-tooling/cli/test/commands/migration-plan-command.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-show.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-status-aggregate-spaces.test.tspackages/1-framework/3-tooling/cli/test/output.json-shapes.test.tspackages/1-framework/3-tooling/cli/test/output.migration-commands.test.tspackages/1-framework/3-tooling/cli/test/utils/contract-space-aggregate-loader.ac15.test.tspackages/1-framework/3-tooling/cli/test/utils/contract-space-extension-migrations-pass.test.tspackages/1-framework/3-tooling/cli/test/utils/contract-space-migrate-pass.test.tspackages/1-framework/3-tooling/cli/test/utils/contract-space-seed-phase.test.tspackages/1-framework/3-tooling/cli/test/utils/extension-pack-inputs.test.tspackages/1-framework/3-tooling/migration/src/aggregate/loader.tspackages/1-framework/3-tooling/migration/src/aggregate/planner.tspackages/1-framework/3-tooling/migration/src/detect-space-contract-drift.tspackages/1-framework/3-tooling/migration/src/exports/aggregate.tspackages/1-framework/3-tooling/migration/src/exports/spaces.tspackages/1-framework/3-tooling/migration/test/aggregate/loader.test.tspackages/1-framework/3-tooling/migration/test/deletable-node-modules.test.tspackages/1-framework/3-tooling/migration/test/detect-space-contract-drift.test.tspackages/3-extensions/cipherstash/test/cipherstash-codec.test.tspackages/3-targets/6-adapters/sqlite/test/migrations/db-init-update.cli.test.tstest/integration/test/cli.db-verify.aggregate-schema.test.tstest/integration/test/cli.loader.drift.test.ts
💤 Files with no reviewable changes (8)
- packages/1-framework/3-tooling/migration/test/detect-space-contract-drift.test.ts
- test/integration/test/cli.loader.drift.test.ts
- packages/1-framework/3-tooling/cli/src/utils/contract-space-migrate-pass.ts
- packages/1-framework/3-tooling/migration/src/detect-space-contract-drift.ts
- packages/1-framework/3-tooling/cli/test/utils/contract-space-migrate-pass.test.ts
- packages/1-framework/3-tooling/cli/test/utils/contract-space-extension-migrations-pass.test.ts
- packages/1-framework/3-tooling/cli/src/utils/contract-space-extension-migrations-pass.ts
- packages/1-framework/3-tooling/migration/src/exports/spaces.ts
✅ Files skipped from review due to trivial changes (7)
- packages/1-framework/3-tooling/migration/src/aggregate/planner.ts
- packages/3-extensions/cipherstash/test/cipherstash-codec.test.ts
- packages/1-framework/3-tooling/cli/test/output.json-shapes.test.ts
- packages/1-framework/3-tooling/cli/test/output.migration-commands.test.ts
- test/integration/test/cli.db-verify.aggregate-schema.test.ts
- packages/1-framework/3-tooling/cli/src/control-api/types.ts
- packages/3-targets/6-adapters/sqlite/test/migrations/db-init-update.cli.test.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/1-framework/3-tooling/cli/src/control-api/operations/db-apply-aggregate.ts
- packages/1-framework/3-tooling/cli/test/commands/migration-status-aggregate-spaces.test.ts
- packages/1-framework/3-tooling/migration/test/deletable-node-modules.test.ts
- packages/1-framework/3-tooling/cli/test/utils/contract-space-aggregate-loader.ac15.test.ts
- packages/1-framework/3-tooling/cli/test/commands/migration-plan-command.test.ts
- examples/cipherstash-integration/migrations/cipherstash/contract.d.ts
- packages/1-framework/3-tooling/cli/test/utils/contract-space-seed-phase.test.ts
- packages/1-framework/3-tooling/cli/src/utils/contract-space-seed-phase.ts
- packages/1-framework/3-tooling/cli/src/control-api/operations/migration-apply.ts
- packages/1-framework/3-tooling/cli/src/utils/extension-pack-inputs.ts
- packages/1-framework/3-tooling/cli/test/utils/extension-pack-inputs.test.ts
- packages/1-framework/3-tooling/cli/src/utils/contract-space-aggregate-loader.ts
- packages/1-framework/3-tooling/migration/src/aggregate/loader.ts
- packages/1-framework/3-tooling/migration/src/exports/aggregate.ts
- packages/1-framework/3-tooling/cli/src/commands/migration-status.ts
- packages/1-framework/3-tooling/cli/src/utils/formatters/migrations.ts
- packages/1-framework/3-tooling/migration/test/aggregate/loader.test.ts
Contributor
Author
|
Done — A21a addressed in a3c3bfe ( |
Adds `runContractSpaceSeedPhase` — the new phase-1 helper for the
two-phase `migration plan` pipeline (sub-spec § 4 of M6.5,
extension-contract-spaces). For every extension that exposes a
`contractSpace` it:
1. reads the on-disk head ref
2. unconditionally re-emits `contract.json`, `contract.d.ts`, and
`refs/head.json` via `emitContractSpaceArtefacts`
3. materialises every descriptor-shipped migration package not yet
present on disk via `materialiseExtensionMigrationPackageIfMissing`
The return value enumerates per-space records (`updated` vs
`unchanged`, prior/new hash, newly-materialised migration directory
names) so the caller can render a single status line per touched
extension and so phase-2 can run on a now-consistent disk state.
Output ordering is alphabetical via `planAllSpaces`, matching the
canonical sort used by every other aggregate surface and surfacing
duplicate-spaceId failures before any write.
Composes existing primitives only — no new I/O surface.
Refs: TML-2457 (M6.5 / T6.5.1)
Signed-off-by: Will Madden <madden@prisma.io>
… (T6.5.2) Drift previously fired only at apply/verify time on a descriptor field the loader is forbidden to consult (NFR3 / AC15). The remaining call sites pass the descriptor in just so the loader can hash it and compare to the on-disk pin — purely informational, never load-bearing, and structurally dead code once `migration plan` writes the pin. This commit deletes the surface end-to-end: - `detectSpaceContractDrift` + `SpaceContractDriftResult` (and unit tests) gone from `@prisma-next/migration-tools`. - `LoadAggregateError.driftViolation` gone from the aggregate loader, along with the per-entry hash callback (`hashContract`) and the `contractSpace` field on `DeclaredExtensionEntry`. - Loader layout precheck now treats every declared entry as contract-space-bearing; the CLI wrapper filters codec-only extensions out of `toDeclaredExtensions` to match. - `formatContractSpaceDriftWarning` + the `drifts` channel gone from `runContractSpaceMigratePass` (left in place as an unconditional re-emit; T6.5.3 will retire the helper itself). - `migration plan` no longer renders drift warnings. - Integration test `cli.loader.drift.test.ts` deleted. - Updated remaining loader / deletable-node_modules / extension-pack tests to match the simplified shape. Locks AC4 (zero source matches for the four drift symbols under `packages/`) and AC5 (`DeclaredExtensionEntry.contractSpace` field is gone). The loader is now strictly free of descriptor `contractJson` access at apply / verify time, tightening AC15 / NFR3 from "verify does not require the descriptor" to "verify never reads the descriptor hash." Signed-off-by: Will Madden <madden@prisma.io>
… in migration plan (T6.5.3) - Replace runContractSpaceMigratePass + runContractSpaceExtensionMigrationsPass with a single runContractSpaceSeedPhase call (phase 1). - After the no-op check, load the ContractSpaceAggregate via buildContractSpaceAggregate (phase 2) and invoke the planner against aggregate.app instead of APP_SPACE_ID literal. - Delete contract-space-migrate-pass.ts and contract-space-extension-migrations-pass.ts (and their tests) — no remaining callers. - Remove toMigratePassInputs and toExtensionMigrationsInputs from extension-pack-inputs.ts. - Update migration-plan-command.test.ts to include target field in contract JSON so the real aggregate loader passes the target-consistency check without a module-level mock. Signed-off-by: Will Madden <madden@prisma.io>
Load ContractSpaceAggregate and iterate app + extension spaces so every space's latest migration is shown. Adds per-space section headings when multiple spaces are present, matching migration status's style. Deleted the APP_SPACE_ID hard-code in migration-show.ts; the command now relies on aggregate.app.spaceId and aggregate.extensions[*].spaceId for all directory resolution. Refs: TML-2457 Signed-off-by: Will Madden <madden@prisma.io>
… path (AC15) buildContractSpaceAggregate previously called toExtensionInputs() which reads pack.contractSpace.contractJson for every declared extension. The aggregate loader only needs id + targetId to locate the on-disk directory; the contract itself is always read from disk. Introduce toDeclaredExtensionsFromRaw() in extension-pack-inputs.ts that uses `'contractSpace' in pack` to detect contract-space membership without invoking the property accessor, and wire it into buildContractSpaceAggregate. Add an AC15 regression test: an extension with a throwing contractSpace getter passes through buildContractSpaceAggregate, returning a layout- violation Result instead of a thrown exception. Refs: TML-2457 Signed-off-by: Will Madden <madden@prisma.io>
…ce missing extension spaces
Lift `createControlStack(config)` / `family.create(stack)` to the
top of `migration plan` and `migration show`, then thread the same
typed `familyInstance` into `buildContractSpaceAggregate`. This drops
the `config.family.create({} as any)` workaround and all biome-ignore
suppressions around extensionPacks / validateContract — the validator
runs against a fully composed family instance (codec lookups etc. see
a consistent stack view), matching `migration status`.
In `migration show`, model per-space results as a discriminated union
(`kind: present` vs `kind: missing`). Extension spaces declared by an
aggregate but with no on-disk migration package now surface as
explicit `missing` placeholders instead of being silently skipped, so
JSON consumers enumerate every loaded extension. The formatter renders
a one-line placeholder for missing spaces.
Strengthen the multi-space heading test to assert app-before-extension
ordering (heading-index comparison), and add a missing-space test.
Signed-off-by: Will Madden <madden@prisma.io>
…; narrow readAllMarkers fallback `pendingCount` for each space now uses `walked.result.migrationEdges.length` instead of `plan.operations.length`. A single authored migration that lowers to N ops (or zero ops) counts as exactly one unit of pending work — the previous semantics conflated user-visible migrations with their op decomposition and over-counted multi-op migrations. The `readAllMarkers()` call site now probes `typeof client.readAllMarkers === "function"` before invoking, so we only swallow the unsupported-method case. Runtime / query errors from a family that *does* expose the method propagate up instead of being silently degraded to "markers unknown" status. Tests pin both pending-count semantics (multi-op + zero-op) by mocking `graphWalkStrategy` to return controlled edge/op counts. Signed-off-by: Will Madden <madden@prisma.io>
`migration apply` recovers the structural path on `NO_INVARIANT_PATH` by replaying the graph with no required invariants. When the live marker is undefined (greenfield), the fallback used `""` as the from hash, which is never a graph node — the structural lookup silently returned no path and the error envelope lost its `structuralPath` context. Use `EMPTY_CONTRACT_HASH` instead, matching the sentinel `graphWalkStrategy` uses for the same condition. Greenfield runs hitting an unsatisfiable invariant set now surface the structural-path edges in the error envelope, the same way non-greenfield runs do. Signed-off-by: Will Madden <madden@prisma.io>
…rop @ts-nocheck, export edge-ref type - `db init/update --dry-run` (mode === "plan") no longer fails on a lingering orphan marker: the preflight is now gated to `mode === "apply"`. Users can introspect what an apply *would* do without having to clean up the orphan first; apply mode still refuses to silently advance the app marker around it. - `toDeclaredExtensionsFromRaw` uses `Object.hasOwn(raw, "contractSpace")` instead of `"contractSpace" in raw`. The `in` operator walks the prototype chain and matches prototype getters; `Object.hasOwn` matches only own properties, so a descriptor that synthesises `contractSpace` via a prototype getter is correctly skipped here. - Drop the `@ts-nocheck` directive from the placeholder `contract.d.ts` emitted by the seed phase. The placeholder is a self-contained `export {};` module that typechecks on its own; the repo bans `@ts-nocheck` outside negative type tests. Refresh the cipherstash-integration example mirror to match. - Re-export `AggregateMigrationEdgeRef` from `@prisma-next/migration-tools/aggregate` so consumers (e.g. CLI status renderers) can name the `migrationEdges` payload that the graph-walk strategy attaches to each `AggregatePerSpacePlan`. Signed-off-by: Will Madden <madden@prisma.io>
…n shape test Strip references to transient project artefacts (milestone labels like M6, acceptance-criteria ids like AC4/AC5/AC7, "sub-spec § …" prose attribution, finding ids F4/F7) from source/test comments. The behaviour each comment is describing — per-space markers visible in the success summary, aggregate-driven perSpace breakdown, action- first/column-first cipherstash operation labels — is unchanged; only the descriptions are now self-contained. Additionally pin the canonical `perSpace` ordering in the `MigrationApplyResult` JSON-shape test: asserts `perSpace.map(p => p.spaceId)` against `["pgvector", "app"]` so accidentally sorting or reordering the array is caught alongside the existing key-shape pin. Signed-off-by: Will Madden <madden@prisma.io>
…on show Matches the existing convention used in migration-apply, migration-status, and contract-emit (per CodeRabbit nitpick). Signed-off-by: Will Madden <madden@prisma.io>
Signed-off-by: Will Madden <madden@prisma.io>
… formatter Strip the transient `spec OQ-4` references from the JSDoc on `renderPreviewStatement` and `previewBlockHeader`. The comments now describe the constraint directly: SQL statements get a trailing `;` so the preview is byte-identical to the legacy renderer, and SQL-only previews keep the legacy `DDL preview` label so the output stays byte-identical to the pre-aggregate SQL CLI. Signed-off-by: Will Madden <madden@prisma.io>
Two `migration show` paths used to swallow conditions that should be runtime errors: - `resolveLatestFromDir` collapsed a non-empty migrations directory whose `findLatestMigration(graph)` returned `null` into `ok(null)`, which the caller rendered as `kind: "missing"`. After `readMigrationsDir` returned packages, a null leaf is a corruption signal, not "no on-disk package". Return `notOk(errorRuntime(...))` naming the corrupted directory so the operator sees the broken history instead of a silent placeholder. - The path-like-target branch resolved any path and emitted the result under `aggregate.app.spaceId`. Passing an extension-space package path (e.g. `migrations/cipherstash/0000000001-init`) produced a mislabeled app-space result. Extract `resolveAppTargetPath` and reject any target that does not resolve under `appMigrationsDir`, returning a clear "Target must point to an app-space migration" runtime error. Both helpers are exported and covered by focused unit tests in `migration-show.test.ts`, including a corrupted self-loop fixture and an extension-space sibling path. Signed-off-by: Will Madden <madden@prisma.io>
pathe.relative() can return an absolute path when the target cannot be expressed relative to the base (e.g. on Windows when the target is on a different drive). The previous guard only checked for "..", so a cross-drive target would slip through and be mislabeled as an app-space migration. Add isAbsolute(relativeToApp) to the rejection condition and a unit test covering the cross-drive case. Signed-off-by: Will Madden <madden@prisma.io>
Tests in migration-show.test.ts used createTempDir() to mkdir under tmpdir() but never removed the directories, leaving real state behind between runs (violating the CLI guideline that tests must clean up their own ephemeral directories). Track every returned dir in a module-level list and remove them in a suite-wide afterEach with rm({ recursive: true, force: true }) so cleanup runs even on failures.
Signed-off-by: Will Madden <madden@prisma.io>
wmadden
force-pushed
the
tml-2457-reduce-app_space_id-coupling-route-through-aggregate-slot
branch
from
a3c3bfe to
efb161f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/1-framework/3-tooling/cli/test/commands/migration-show.test.ts (1)
467-470:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove milestone/AC references from inline test comments.
Line 467 references transient artifacts (
M6,T6.6,AC8), which violates the comment policy. Keep the behavior note, but drop planning IDs.As per coding guidelines: "Source-code comments must not reference transient project artifacts including ... milestone-task IDs ... [and] milestone-named acceptance criteria ..."Suggested edit
- // M6 (T6.6 / AC8): operationClass tags no longer inlined in the - // human-readable line. Destructive ops still render a "(destructive)" - // marker (replaces the old "[destructive]" tag); additive/widening/ - // mutative/data render bare. + // Operation-class tags are no longer inlined in the human-readable line. + // Destructive ops still render a "(destructive)" marker (replacing the + // old "[destructive]" tag); additive/widening/mutative/data render bare.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/1-framework/3-tooling/cli/test/commands/migration-show.test.ts` around lines 467 - 470, The inline test comment that begins "// M6 (T6.6 / AC8): operationClass tags no longer inlined..." contains transient planning IDs; edit that comment (in migration-show.test.ts near the test describing operationClass rendering) to remove "M6 (T6.6 / AC8):" and any AC/task references while preserving the behavior note about destructive ops rendering "(destructive)" and additive/widening/mutative/data rendering bare; ensure the revised comment is purely descriptive and contains no milestone/task/AC identifiers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/1-framework/3-tooling/cli/src/utils/extension-pack-inputs.ts`:
- Around line 149-151: The current check Object.hasOwn(raw, 'contractSpace')
will pass when contractSpace is an own key with value undefined, diverging from
toDeclaredExtensions behavior; update the guard in the block handling raw (the
same `raw` variable and the entries.push path) to require the key exists AND the
value is not undefined (e.g., Object.hasOwn(raw, 'contractSpace') && (raw as
any).contractSpace !== undefined) so only objects with a defined contractSpace
are accepted, matching toDeclaredExtensions semantics.
---
Outside diff comments:
In `@packages/1-framework/3-tooling/cli/test/commands/migration-show.test.ts`:
- Around line 467-470: The inline test comment that begins "// M6 (T6.6 / AC8):
operationClass tags no longer inlined..." contains transient planning IDs; edit
that comment (in migration-show.test.ts near the test describing operationClass
rendering) to remove "M6 (T6.6 / AC8):" and any AC/task references while
preserving the behavior note about destructive ops rendering "(destructive)" and
additive/widening/mutative/data rendering bare; ensure the revised comment is
purely descriptive and contains no milestone/task/AC identifiers.
🪄 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: 296d0dff-07f9-4990-b54e-da780db3e05d
📒 Files selected for processing (35)
examples/cipherstash-integration/migrations/cipherstash/contract.d.tspackages/1-framework/3-tooling/cli/src/commands/migration-plan.tspackages/1-framework/3-tooling/cli/src/commands/migration-show.tspackages/1-framework/3-tooling/cli/src/commands/migration-status.tspackages/1-framework/3-tooling/cli/src/control-api/operations/db-apply-aggregate.tspackages/1-framework/3-tooling/cli/src/control-api/operations/migration-apply.tspackages/1-framework/3-tooling/cli/src/control-api/types.tspackages/1-framework/3-tooling/cli/src/utils/contract-space-aggregate-loader.tspackages/1-framework/3-tooling/cli/src/utils/contract-space-extension-migrations-pass.tspackages/1-framework/3-tooling/cli/src/utils/contract-space-migrate-pass.tspackages/1-framework/3-tooling/cli/src/utils/contract-space-seed-phase.tspackages/1-framework/3-tooling/cli/src/utils/extension-pack-inputs.tspackages/1-framework/3-tooling/cli/src/utils/formatters/migrations.tspackages/1-framework/3-tooling/cli/test/commands/migration-plan-command.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-show.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-status-aggregate-spaces.test.tspackages/1-framework/3-tooling/cli/test/output.json-shapes.test.tspackages/1-framework/3-tooling/cli/test/output.migration-commands.test.tspackages/1-framework/3-tooling/cli/test/utils/contract-space-aggregate-loader.ac15.test.tspackages/1-framework/3-tooling/cli/test/utils/contract-space-extension-migrations-pass.test.tspackages/1-framework/3-tooling/cli/test/utils/contract-space-migrate-pass.test.tspackages/1-framework/3-tooling/cli/test/utils/contract-space-seed-phase.test.tspackages/1-framework/3-tooling/cli/test/utils/extension-pack-inputs.test.tspackages/1-framework/3-tooling/migration/src/aggregate/loader.tspackages/1-framework/3-tooling/migration/src/aggregate/planner.tspackages/1-framework/3-tooling/migration/src/detect-space-contract-drift.tspackages/1-framework/3-tooling/migration/src/exports/aggregate.tspackages/1-framework/3-tooling/migration/src/exports/spaces.tspackages/1-framework/3-tooling/migration/test/aggregate/loader.test.tspackages/1-framework/3-tooling/migration/test/deletable-node-modules.test.tspackages/1-framework/3-tooling/migration/test/detect-space-contract-drift.test.tspackages/3-extensions/cipherstash/test/cipherstash-codec.test.tspackages/3-targets/6-adapters/sqlite/test/migrations/db-init-update.cli.test.tstest/integration/test/cli.db-verify.aggregate-schema.test.tstest/integration/test/cli.loader.drift.test.ts
💤 Files with no reviewable changes (8)
- packages/1-framework/3-tooling/migration/src/exports/spaces.ts
- packages/1-framework/3-tooling/cli/test/utils/contract-space-extension-migrations-pass.test.ts
- packages/1-framework/3-tooling/cli/src/utils/contract-space-migrate-pass.ts
- packages/1-framework/3-tooling/cli/test/utils/contract-space-migrate-pass.test.ts
- packages/1-framework/3-tooling/migration/test/detect-space-contract-drift.test.ts
- packages/1-framework/3-tooling/cli/src/utils/contract-space-extension-migrations-pass.ts
- packages/1-framework/3-tooling/migration/src/detect-space-contract-drift.ts
- test/integration/test/cli.loader.drift.test.ts
✅ Files skipped from review due to trivial changes (9)
- test/integration/test/cli.db-verify.aggregate-schema.test.ts
- packages/1-framework/3-tooling/migration/src/aggregate/planner.ts
- packages/1-framework/3-tooling/cli/test/output.json-shapes.test.ts
- packages/1-framework/3-tooling/cli/src/control-api/types.ts
- packages/1-framework/3-tooling/cli/test/output.migration-commands.test.ts
- packages/3-extensions/cipherstash/test/cipherstash-codec.test.ts
- examples/cipherstash-integration/migrations/cipherstash/contract.d.ts
- packages/1-framework/3-tooling/cli/test/utils/extension-pack-inputs.test.ts
- packages/3-targets/6-adapters/sqlite/test/migrations/db-init-update.cli.test.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/1-framework/3-tooling/cli/src/control-api/operations/migration-apply.ts
- packages/1-framework/3-tooling/cli/test/commands/migration-plan-command.test.ts
- packages/1-framework/3-tooling/cli/src/commands/migration-status.ts
- packages/1-framework/3-tooling/cli/test/utils/contract-space-seed-phase.test.ts
- packages/1-framework/3-tooling/cli/src/commands/migration-plan.ts
- packages/1-framework/3-tooling/migration/src/exports/aggregate.ts
- packages/1-framework/3-tooling/cli/test/commands/migration-status-aggregate-spaces.test.ts
- packages/1-framework/3-tooling/cli/src/utils/contract-space-aggregate-loader.ts
- packages/1-framework/3-tooling/cli/test/utils/contract-space-aggregate-loader.ac15.test.ts
- packages/1-framework/3-tooling/cli/src/commands/migration-show.ts
- packages/1-framework/3-tooling/cli/src/control-api/operations/db-apply-aggregate.ts
- packages/1-framework/3-tooling/migration/test/aggregate/loader.test.ts
- packages/1-framework/3-tooling/migration/src/aggregate/loader.ts
- packages/1-framework/3-tooling/cli/src/utils/formatters/migrations.ts
- packages/1-framework/3-tooling/cli/src/utils/contract-space-seed-phase.ts
…pace semantics Object.hasOwn(raw, "contractSpace") returned true even when the property value was explicitly undefined, but the canonical toDeclaredExtensions skips those (it checks pack.contractSpace === undefined). The descriptor-based check now mirrors that: an explicit-undefined data property is treated as "no contract-space declaration". Getter-backed contractSpace declarations stay included without invoking the accessor, preserving the AC15 contract. Signed-off-by: Will Madden <madden@prisma.io>
Signed-off-by: Will Madden <madden@prisma.io>
wmadden
deleted the
tml-2457-reduce-app_space_id-coupling-route-through-aggregate-slot
branch
This was referenced May 12, 2026
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
1 participant
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-2457
Key snippet (intention in code)
Intent
migration planandmigration showwere the last commands in themigrationfamily that didn't know about theContractSpaceAggregateintroduced in M2.5. They operated through flat, extension-unaware passes and could only plan the app space in isolation. This change promotes both commands to the aggregate and retires the "drift" concept — a discrepancy detector between descriptor-embeddedcontractJsonand on-disk pinned artifacts — whose existence was causingdb verifyand the aggregate loader to silently readcontractSpace.contractJsonfrom extension descriptors at runtime, in violation of NFR3 (the framework's runner and verifier must never import extension descriptors).The story
Introduce a phase-1 seed component that unconditionally re-emits per-space pinned artifacts and materializes any descriptor-shipped migration packages not yet on disk. Previously this was split across two passes with different data shapes; now it's a single entry point.
Rewrite
migration planas a two-phase pipeline. Phase 1 seeds. Phase 2 loads theContractSpaceAggregateand runs the planner againstaggregate.app. Because phase 1 guarantees every declared extension has its head ref pinned, the loader'sdeclaredButUnmigratedprecheck always passes — resolving the chicken-and-egg problem that previously made aggregate-loading at plan time impractical.Rewrite
migration showto enumerate all spaces in the aggregate (app first, then extensions). The result shape changes from a single flat object to{ spaces: MigrationShowSpaceResult[] }, with each entry carrying itsspaceId. The formatter adds per-space headings when there are multiple spaces, matching the sectioning style ofmigration status.Retire the drift surface. The "drift" concept is deleted:
detectSpaceContractDrift,formatContractSpaceDriftWarning,SpaceContractDriftResult, theLoadAggregateError.driftViolationvariant, and thecontractSpacefield onDeclaredExtensionEntry. The phase-1 seed unconditionally refreshes pinned artifacts, so drift is never observable.Fix a latent NFR3 violation. The aggregate loader was calling
toDeclaredExtensions(toExtensionInputs(packs)), wheretoExtensionInputsreadspack.contractSpace.contractJsonand immediately discards it — including insidedb verify. The fix introducestoDeclaredExtensionsFromRaw, which uses'contractSpace' in pack(a property existence check that does not invoke the accessor) and extracts onlyidandtargetId.Behavior changes & evidence
migration planis now aggregate-driven: extension pinned artifacts are seeded unconditionally before the no-op check, and the plan runs againstaggregate.app.migration showenumerates all contract spaces: Before it returned a single flat result for the app space only. After it returns{ spaces }— app first, then each extension space.Drift detection is gone: Before,
migration planprinted a "drift detected" warning. After, that path is deleted; the seed phase unconditionally refreshes artifacts and emits a plain "Updated <spaceId>" step message.packages/1-framework/3-tooling/migration/src/detect-space-contract-drift.ts; drift block removed from packages/1-framework/3-tooling/migration/src/aggregate/loader.tsbuildContractSpaceAggregatenever readscontractSpace.contractJsonfrom extension descriptors (NFR3 tightened): Before, the aggregate loader calledtoExtensionInputs, accessingpack.contractSpace.contractJsonand immediately discarding it. After, it callstoDeclaredExtensionsFromRaw, which checks'contractSpace' in packand extracts onlyid/targetId.Compatibility / migration / risk
MigrationShowResultloses its flat top-level fields and gainsspaces: readonly MigrationShowSpaceResult[]. Any code destructuringMigrationShowResultdirectly needs to switch toresult.spaces[0]for the app space. Within this repo, only the CLI formatter is affected and is updated.DeclaredExtensionEntryloses itscontractSpacefield.LoadAggregateErrorloses itsdriftViolationvariant. Both are breaking to code that matches on those shapes; no remaining callers within this repo after this PR.APP_SPACE_IDsource count: 74 before → 70 after. Survivors are in structurally placed, out-of-scope surfaces (legacy single-app SQL/target statement builders, framework constant declarations, migration-tools aggregate identifiers). Reducing further requires Move 2, deferred indefinitely.Follow-ups / open questions
migration planmaterializes every extension migration the descriptor ships, regardless of apply-path traversal): deferred indefinitely.2-sql/9-family,cli/src/control-api/client.ts, adapterparams: [APP_SPACE_ID]): parallel ticket (TML-2457 OQ2).Non-goals / intentionally out of scope
appSpaceId: stringinto contract IR): closed negatively by demonstration — residualAPP_SPACE_IDreferences are structurally placed and cannot be removed without a contract IR change.migration new— app-space-only by design.client.test.ts/migration-status-aggregate-spaces.test.ts(PGlite connection ordering, plan.md Open Items Add PGVector extension #4).Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests