TML-2835: make migration check single-target resolution multi-space#732
Conversation
`checkSingleTarget` now resolves a migration ref across all contract spaces (via the already-loaded aggregate + `enumerateCheckSpaces`), with `--space <id>` to narrow and a clear PRECONDITION error when a ref is ambiguous across spaces. Path-based targets resolve to the owning space's dir via `resolveTargetPathAcrossSpaces`. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
…red envelope (fix D2 regression) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
…resolution (review F-1/F-2) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
…/review Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR extends the ChangesMulti-space migration check single-target resolution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
@prisma-next/extension-author-tools
@prisma-next/mongo-runtime
@prisma-next/family-mongo
@prisma-next/sql-runtime
@prisma-next/family-sql
@prisma-next/extension-arktype-json
@prisma-next/middleware-cache
@prisma-next/mongo
@prisma-next/extension-paradedb
@prisma-next/extension-pgvector
@prisma-next/extension-postgis
@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/cli-telemetry
@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: |
size-limit report 📦
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/1-framework/3-tooling/cli/test/commands/migration-check-single-target-multi-space.test.ts (1)
21-27: 🏗️ Heavy liftPrefer fixture-driven config wiring over module-level
vi.mockThese tests currently rely on mocking
../../src/config-loader. Please switch to a real temp config fixture and pass it through command options so the suite uses direct imports and avoids module mocking.As per coding guidelines,
**/*.{test,spec}.{ts,tsx,js,jsx}: "Prefer direct imports over module mocks in test files".🤖 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-check-single-target-multi-space.test.ts` around lines 21 - 27, Remove the module-level vi.mock/vi.hoisted stubbing of loadConfig and the vi.doUnmock/vi.resetModules cleanup; instead create a real temporary config fixture file and pass its path into the command under test via the command options so the test uses the real ../../src/config-loader import. Concretely: delete references to mocks, vi.mock('../../src/config-loader'), vi.doUnmock and vi.resetModules; write a temp config fixture in the test (or use a test helper like writeTempConfig) and invoke the tested command (the migration-check invocation in this file) with an explicit config option/path so loadConfig is called against the real fixture. Ensure the fixture matches expected shape used by loadConfig and clean up the temp file after the test.
🤖 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-check.ts`:
- Around line 461-471: The loop currently records only the first parse failure
in firstParseFailure when parseMigrationRef(target, { graph: space.graph, refs:
space.refs }) fails, which makes no-hit diagnostics order-dependent; update the
logic to keep the most specific failure instead of the earliest by comparing the
failure kinds (e.g., prefer "wrong-grammar" over "not-found" or other less
specific kinds) and assign firstParseFailure = migResult.failure only if it is
more specific than the existing firstParseFailure; apply the same
specificity-preserving change to the other identical block that currently sets
firstParseFailure between lines handling scopedSpaces (the second occurrence
noted around the 492-498 region) so both loops choose the most informative parse
failure for the shared envelope.
---
Nitpick comments:
In
`@packages/1-framework/3-tooling/cli/test/commands/migration-check-single-target-multi-space.test.ts`:
- Around line 21-27: Remove the module-level vi.mock/vi.hoisted stubbing of
loadConfig and the vi.doUnmock/vi.resetModules cleanup; instead create a real
temporary config fixture file and pass its path into the command under test via
the command options so the test uses the real ../../src/config-loader import.
Concretely: delete references to mocks, vi.mock('../../src/config-loader'),
vi.doUnmock and vi.resetModules; write a temp config fixture in the test (or use
a test helper like writeTempConfig) and invoke the tested command (the
migration-check invocation in this file) with an explicit config option/path so
loadConfig is called against the real fixture. Ensure the fixture matches
expected shape used by loadConfig and clean up the temp file after the test.
🪄 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: ea28ae49-37da-49fc-8cf7-31756877be20
⛔ Files ignored due to path filters (3)
projects/migration-graph-rendering/slices/check-single-target-multi-space/code-review.mdis excluded by!projects/**projects/migration-graph-rendering/slices/check-single-target-multi-space/plan.mdis excluded by!projects/**projects/migration-graph-rendering/slices/check-single-target-multi-space/spec.mdis excluded by!projects/**
📒 Files selected for processing (4)
packages/1-framework/3-tooling/cli/src/commands/migration-check.tspackages/1-framework/3-tooling/cli/src/utils/cli-errors.tspackages/1-framework/3-tooling/cli/src/utils/migration-path-target.tspackages/1-framework/3-tooling/cli/test/commands/migration-check-single-target-multi-space.test.ts
…Rabbit review) When a single-target ref resolves in no space, report the most informative parse failure (e.g. wrong-grammar) instead of whichever space failed first, so the diagnostic is not order-dependent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
Previous run had a V8 JIT crash (SIGILL, exit 132) in the prisma-next-demo runner and a cascading MongoDB memory-server timeout in retail-store — both unrelated to this PR's changes. Signed-off-by: Will Madden <madden@prisma.io> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Completes the multi-space
migration checkwork TML-2801 began. That slice made the holistic (no-arg) check span every contract space; its single-target mode (check <ref|path>) was deliberately left app-space-only as a follow-up. This makes single-target resolution multi-space too.Changes
migration-check.ts,checkSingleTarget): a migration reference is resolved per-space against each space's graph + refs (reusing TML-2801'senumerateCheckSpaces/CheckSpace), and the matched package is checked in its own space. A migration that lives in a non-app space (e.g. an extension space) is now resolved and checked, where before it was aPRECONDITION"not found on disk".--space <id>narrows single-target resolution to one space, validated identically to the holistic path (isValidSpaceId→errorInvalidSpaceId; unknown id →errorSpaceNotFound;PRECONDITION).PRECONDITIONvia a newerrorAmbiguousMigrationReffactory (cli-errors.ts) that names the spaces and tells the user to qualify with--space.mapRefResolutionError(PN-RUN-3000), preserving the consistency contract TML-2801 established — not a bespoke string.migration-path-target.ts,resolveTargetPathAcrossSpaces).app; the--helplong description and examples are updated to describe multi-space single-target; the custom exit codes (0/2/4) remain documented in--help.Why
migration checkis the integrity verb; having single-target silently ignore non-app spaces meant a corrupted extension-space package could passcheck <ref>while the no-argcheckcaught it. Resolving across spaces (with explicit ambiguity handling) closes that asymmetry and matches the policylist/graph/statusalready follow.Behaviour note for reviewers: single-target now loads the full read aggregate (as the holistic path does) and therefore inherits the holistic path's integrity-refusal gate, where the old single-target path read only the app migrations directory. This is the spec's chosen design and is arguably more correct, but it does widen single-target's pre-resolution failure surface.
Out of scope: the holistic path (TML-2801, done); the other read verbs; runtime-validatable arktype
--jsonschemas (TML-2836);--spacesemantics forshow/log. Exit codes and theMigrationCheckResultshape are unchanged.Closes TML-2835.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests