refactor(postgres): schema-node tree restructure — database→namespace→table→policy diff tree#894
refactor(postgres): schema-node tree restructure — database→namespace→table→policy diff tree#894wmadden-electric wants to merge 49 commits into
Conversation
Reshape the forward slice sequence and shape slice 2: replace the conflated PostgresSchemaIR with a single-purpose schema-node tree (database → namespace → table → policy; roles on the root), split the diff nodes from the Contract-IR entities, make introspect return the tree (a node; consumers ensure + walk), and move database→PSL inference onto the Postgres target. Behaviour-neutral. spec.md is the prescriptive build (R1–R9, 7 units, alternatives); design.md is the durable architecture reference. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
… schema-diff nodes Slice 2 (schema-node-tree-restructure), unit 1 of 7. PostgresRlsPolicy / PostgresRole were doing two jobs: authored, serialized Contract-IR entities AND diff-tree leaves. Split them: - The entities keep their names, lose DiffableNode (id/children/ isEqualTo gone), and move out of schema-ir/ to core/ alongside the other contract-IR / entity-kind definitions. - New PostgresPolicySchemaNode / PostgresRoleSchemaNode in schema-ir/ are the diff-tree leaves (DiffableNode, static is() guards). Tables already split this way (StorageTable vs PostgresTableIR). Intermediate unit: the differ/planner still reference the entity as a node and break here; they are rewired in unit 6. The branch goes green across the unit sequence; the new-node tests pass (37/37). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
Slice 2 (schema-node-tree-restructure), unit 2 of 7. Mechanical rename of the table diff-node to the …SchemaNode scheme, and point its policy children at the Unit-1 node type: - PostgresTableIR → PostgresTableSchemaNode (file, class, Input). - Free guard isPostgresTableIR → static PostgresTableSchemaNode.is(). - Field rlsPolicies → policies: readonly PostgresPolicySchemaNode[]; children() returns this.policies (resolves the Unit-1 DiffableNode type break on the table node). Still extends SqlSchemaIRNode, freezeNode, isEqualTo => true. Intermediate unit: the projection/introspection still pass entity instances where policy nodes are now expected (Unit 5 fixes construction) and the differ/planner still use the entity guard (Unit 6). Table-node tests pass (11/11). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
Slice 2 (schema-node-tree-restructure), units 3 and 4 of 7. - PostgresNamespaceSchemaNode: one node per Postgres schema; children() = its tables; satisfies the per-schema SqlSchemaIR shape so the legacy per-schema consumers take it unchanged in unit 6. - PostgresDatabaseSchemaNode: the real tree root; children() = namespaces (roles held, not yet diffed); static is()/assert()/ensure(); narrows on nodeTarget + a nodeKind discriminant that survives the projectSchemaToSpace spread. Additive — does not yet wire/retire PostgresSchemaIR (units 5-6). New-node tests pass; reviewer SATISFIED. Also closes the carried cast/lint findings from rounds 1-2 (bare casts in postgres-rls-policy.ts removed by narrowing; root is()/assert() use blindCast like ensure(); vitest imports merged). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…nit-6 requirement Surfaced by the unit-3/4 review: verify-postgres-namespaces reads existingSchemas off the flat schema via isPostgresSchemaIR, so handing it a namespace node would silently fall back to the [public] default and regress R9. Pinned into the spec so unit 6 rewires the consumer to read existingSchemas from the database root. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
Collapse the 164-line plan to a status table + brief notes for the not-yet-done slices + compact locked decisions. Mark slice 1 (select-policies-dependable, #771) and slice 1.5 (#868) as merged; slice 2 as in progress. Implementation detail lives in the slice specs, not the plan. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
Slice 2 (schema-node-tree-restructure), unit 5 of 7. Both derivations now build PostgresDatabaseSchemaNode and introspect() returns the root: - Projection (contract-to-postgres-schema-ir.ts → contract-to-postgres- database-schema-node.ts): tables grouped by owning namespace; policies built as PostgresPolicySchemaNode from the contract entities; roles + existingSchemas + pgVersion on the root. Malformed-contract assert strengthened to per-namespace. - Introspection: the flat bare-keyed introspectNamespaces merge (silent cross-schema table collision; kept only the first schema name) is gone, replaced by one namespace node per schema; cluster roles collected once. Intermediate unit: the build is red on the consumers and the family introspect() return-type seam (Promise<SqlSchemaIR>, shared with SQLite) — both rewired in unit 6 (introspect returns a node per design §5). SQLite and CF-1 untouched here. Producer tests pass (8 projection + 121 introspection); reviewer SATISFIED. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
introspect()/verifySchema/planner-options/toSchemaView/collectSchemaDiffIssues now type the schema as the family-base SqlSchemaIRNode. The relational verify walks one per-schema namespace node at a time (namespaceSchemaNodes), never a flat merge — single-schema is one pass, byte-identical to the pre-tree verify; multi-schema contract scoping is CF-2. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…SchemaIR The differ + planner ensure the PostgresDatabaseSchemaNode root and walk the tree; guards switch to PostgresPolicySchemaNode.is/.assert. collectSchemaIssues verifies per namespace node (empty root → one empty pass so contract tables surface as missing). CF-1: existingSchemasFromSchema reads existingSchemas off the database root, never the public default. PostgresSchemaIR + its is/assert/ensure are deleted; consumers use PostgresDatabaseSchemaNode statics. Node ensure() deep-reconstructs namespace/table/policy nodes from the projectSchemaToSpace spread so the differ always sees real DiffableNodes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
SQLite planner/runner resolve the single flat namespace from the introspected node (sqliteFlatSchema). The aggregate projectSchemaToSpace + the verifier-s orphan detection learn the namespaced tree shape: they prune / enumerate tables inside each namespace node rather than a flat tables record, keeping per-space isolation without flattening namespaces. New project-schema-to-space tests cover the namespaced-tree branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
Fixtures build PostgresDatabaseSchemaNode roots (namespace nodes holding table nodes holding policy nodes) instead of the deleted flat PostgresSchemaIR; introspection tests narrow the result with PostgresDatabaseSchemaNode.assert and navigate namespaces[…].tables / .roles; policy diff paths gain the namespace segment. Covers diff-postgres-schema, rls-planner, verify-postgres-namespaces, the 6-adapters planner/introspection suites, pgvector planner suites, and the family.introspect / referential-actions integration tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
pruneNamespaceTables narrows with a local isRecord type predicate instead of four bare `as` casts, mirroring the predicate already used in verifier.ts. Net casts return below the pre-unit-6 baseline (lint:casts delta=-2). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
The unit-6 relational verify ran verifySqlSchema once per actual namespace node but passed the WHOLE contract each time, so a multi-schema database reported every contract table as missing_table from the N-1 namespaces it does not belong to. Replace verifyContractAgainstNamespaceNodes with a per-namespace pairing: resolve each contract namespace to its live DDL schema (via the target-s expected-tree projection) and check that namespace-s tables against the matching actual node. verifySqlSchema gains an optional restrictToNamespaceIds so the full contract is still consulted for value-set / control-policy resolution while only the paired namespace-s tables are checked. Single-schema and SQLite are one pairing — byte-identical to before; multi-schema is correct. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
… planner + verify The planner and the family schema verify must run the same diffing operation. Extract the per-namespace pairing into a single shared verifySqlSchemaTree in @prisma-next/family-sql/schema-verify (the relational verifySqlSchema paired by namespace identity to the actual nodes). Verify now calls it and rejects when the issues are non-empty — it owns no pairing/scoping logic of its own. The planner collectSchemaIssues calls the same function instead of its old whole-contract-per-node loop, so the planner picks up the multi-schema fix too (its flat loop had the same latent false-missing bug). Behaviour-neutral: single-schema is one pairing, byte-identical; multi-schema is correct. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
The migration planner and the schema verify now run ONE combined database-schema diff instead of each composing the relational + policy strategies themselves. `diffPostgresDatabaseSchema(...)` (new `diff-database-schema.ts`, Postgres target) composes, once each: the per-namespace-paired relational diff (`verifySqlSchemaTree` → table/column/constraint `SchemaIssue`s + the verification-tree root/counts) and the RLS policy diff (`diffPostgresSchema` → ownership-filtered `SchemaDiffIssue`s). It returns a `VerifyDatabaseSchemaResult` whose `schema` carries both shapes — exactly the existing verify-result schema shape, so nothing downstream changes. The two issue shapes stay separate (the relational findings are stringly `SchemaIssue`s, the policy findings carry the live policy nodes the planner needs to build ops); merging them onto one type is the follow-on relational port, not here. The control adapter exposes it as the `diffDatabaseSchema` seam (replacing `collectSchemaDiffIssues`). The family verify calls `controlAdapter.diffDatabaseSchema` and rejects on non-empty — composing no diff itself (SQLite, which has no structural diff, falls back to the relational diff alone). The planner calls the same `diffPostgresDatabaseSchema` and maps `schema.issues` (+ planner-only namespace presence) to DDL ops and `schema.schemaDiffIssues` to RLS ops — no re-diff. Behaviour-neutral: planner ops byte-identical, verify accept/reject and result shape unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
Database→PSL inference is target logic (it owns the dialect type/default maps and walks its own schema tree), so it moves off the SQL family onto the Postgres target descriptor — clearing the layering violation and the last expected-red. - The Postgres target descriptor gains `inferPslContract(tree)`, beside `contractSerializer`. New `core/psl-infer/infer-psl-contract.ts` (`inferPostgresPslContract`) walks the `PostgresDatabaseSchemaNode` tree and owns the Postgres maps (`postgres-type-map` / `postgres-default-mapping`, moved from the family). The enum diagnostic now reads each namespace node-s `nativeEnumTypeNames`; tables are gathered across namespaces into the model set and emitted as the single `__unspecified__` bucket — byte-identical PSL. - The shape-neutral leaf transforms (name transforms, relation inference, `mapDefault`, raw-default parser, the `PslTypeMap`/printer-config types) stay in the family and are exported from the new `@prisma-next/family-sql/psl-infer` entrypoint, which the target imports. - The family instance `inferPslContract` delegates to `target.inferPslContract` (read off the descriptor like `contractSerializer`); absent ⇒ a clear error (Mongo). The flat `sqlSchemaIrToPslAst` + `buildPslDocumentAst` are deleted. - The inference tests move to the target (`test/psl-infer/`), retargeted to `inferPostgresPslContract` via tree fixtures; their inline PSL snapshots are unchanged (byte-identity). The neutral-utility tests stay in the family. No SQL-family file imports the Postgres maps; `contract infer` output is byte-identical; the full repo typecheck and `fixtures:check` are green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
… diff
The migration runner held a third, private copy of the whole-contract-against-
each-namespace-node verify loop that the diffDatabaseSchema unification missed —
the same multi-schema false-missing bug already fixed in the family verify and
the planner. For an auth.user + public.note contract the runner-s post-apply
verify reported public.note missing because it checked the full contract against
the auth namespace node, failing db init. The slice-DoD e2e gate caught it
(multi-namespace-runtime).
The runner now delegates to the family `verifySchema`, the single shared
per-namespace-paired diff (diffDatabaseSchema / verifySqlSchemaTree) the CLI
verify and planner already use — no private per-namespace loop remains. Also
update the control-api integration test: introspect() returns the
PostgresDatabaseSchemaNode tree (namespaces), not the old flat { tables }; the
assertion now checks the real tree shape.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: willbot <w.a.madden+machine@gmail.com>
Signed-off-by: Will Madden <madden@prisma.io>
… flatten The expected-side projection called `contractToSchemaIR` once on the whole contract, collapsing every namespace into one bare-keyed `Record<string, SqlTableIR>` that throws on a cross-namespace table-name collision. So a contract with `public.thing` + `auth.thing` could not be projected — the residual flatten the tree restructure was meant to remove. New family helper `contractNamespaceToSchemaIR(storage, namespaceId, options)` converts only one namespace-s tables, keyed within that namespace, passing the full storage so cross-namespace type/value-set/enum resolution is unaffected (and cross-namespace FKs survive — `convertForeignKey` builds purely from `fk.target`). The Postgres projection now calls it per namespace instead of flattening. New test proves same-named cross-schema tables project into distinct namespace nodes without throwing. `verifySqlSchemaTree`-s `restrictToNamespaceIds` stays — it scopes the relational `verifySqlSchema`-s contract-table iteration to one namespace while keeping the full contract available for cross-namespace value-set resolution; the projection fix is on the diff-s expected tree, not what `verifySqlSchema` consumes. Behaviour-neutral: planner ops byte-identical, multi-schema + e2e green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis large PR replaces the flat Postgres schema IR ( ChangesPostgres schema IR tree and family diff/verify
AggregateContractSpace rename and unclaimed elements
Estimated code review effort: 5 (Critical) | ~150 minutes Sequence Diagram(s)sequenceDiagram
participant verifyMigration
participant verifySchemaForSpace
participant stripExtraFindings
participant collectExtraElementNames
verifyMigration->>verifySchemaForSpace: verify(schemaIntrospection, space, mode)
verifySchemaForSpace-->>verifyMigration: VerifyDatabaseSchemaResult
verifyMigration->>stripExtraFindings: strip per-space extras
stripExtraFindings-->>verifyMigration: schemaPerSpace entry
verifyMigration->>collectExtraElementNames: gather extra names across spaces
collectExtraElementNames-->>verifyMigration: unclaimed names
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
…sions + D1 follow-on Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@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/extension-supabase
@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/config-loader
@prisma-next/emitter
@prisma-next/language-server
@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: 9
🧹 Nitpick comments (4)
packages/1-framework/3-tooling/migration/src/aggregate/verifier.ts (1)
237-240: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
blindCastover a bareasfor this structural read.As per coding guidelines: "No bare
asin production code. UseblindCast<T, \"Reason\">orcastAs<T>".🤖 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/aggregate/verifier.ts` around lines 237 - 240, The structural read in verifier.ts uses a bare `as` cast, which violates the casting guideline. Update the schema narrowing in the verifier logic to use `blindCast<T, "Reason">` or `castAs<T>` instead of the direct assertion, keeping the existing `schemaIntrospection` handling and the `schema` variable in the same flow.Source: Coding guidelines
packages/1-framework/3-tooling/migration/src/aggregate/project-schema-to-space.ts (1)
68-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
blindCastover a bareasfor this structural read.The repo convention (and the sibling
namespaceSchemaNodeshelper this tree feeds) usesblindCast<T, "reason">for exactly this kind of duck-typed structural read rather than a bareas.
As per coding guidelines: "No bareasin production code. UseblindCast<T, \"Reason\">orcastAs<T>".🤖 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/aggregate/project-schema-to-space.ts` around lines 68 - 72, Replace the bare structural cast in the schema reader with the repo-standard blindCast pattern. In project-schema-to-space’s schema normalization logic, update the schemaObj assignment to use blindCast with an explicit reason instead of a plain as cast, matching the convention used by namespaceSchemaNodes and other duck-typed reads. Keep the inferred shape the same while making the cast intent explicit and consistent with the coding guidelines.Source: Coding guidelines
packages/3-targets/6-adapters/postgres/src/core/control-adapter.ts (1)
597-602: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
getPostgresVersionruns once per namespace though it's cluster-scoped.
introspectSchemacallsgetPostgresVersionat Line 1202, so a multi-namespace walk issues aSELECT version()per schema and the loop just overwritespgVersionkeeping only the last (Line 601). Consider hoisting the version query out of the per-namespace loop inintrospectand dropping it fromintrospectSchema.Also applies to: 1202-1202
🤖 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/3-targets/6-adapters/postgres/src/core/control-adapter.ts` around lines 597 - 602, `introspect` is doing a cluster-scoped PostgreSQL version lookup once per schema and then overwriting the result in the `pgVersion` accumulator. Hoist the `getPostgresVersion` call out of `introspectSchema` into `introspect` so it runs once before the `resolvedSchemas` loop, pass the version through as needed, and remove the version query from `introspectSchema` so it only handles per-namespace introspection. Ensure the `introspect` and `introspectSchema` flow still returns the same `pgVersion` value for all namespaces.packages/3-targets/3-targets/postgres/src/core/schema-ir/postgres-namespace-schema-node.ts (1)
54-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor:
annotations.pg.nativeEnumTypeNamesreuses the raw input array, not the frozen copy.Line 54 stores a defensively-copied, frozen
nativeEnumTypeNameson the instance, but Lines 58-59 embed the originalinput.nativeEnumTypeNamesreference insideannotations.pg. Depending onfreezeNode's depth, this either leaves the annotation array externally mutable (shallow freeze) or mutates the caller's array to frozen as a side effect (deep freeze). Consider reusingthis.nativeEnumTypeNamesfor the annotation to keep both consistent.Proposed tweak
this.nativeEnumTypeNames = Object.freeze([...input.nativeEnumTypeNames]); this.annotations = { pg: { schema: input.schemaName, - ...(input.nativeEnumTypeNames.length > 0 && { - nativeEnumTypeNames: input.nativeEnumTypeNames, + ...(this.nativeEnumTypeNames.length > 0 && { + nativeEnumTypeNames: this.nativeEnumTypeNames, }), }, };🤖 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/3-targets/3-targets/postgres/src/core/schema-ir/postgres-namespace-schema-node.ts` around lines 54 - 62, `PostgresNamespaceSchemaNode` is putting the raw `input.nativeEnumTypeNames` array into `annotations.pg` instead of the class’s frozen copy. Update the constructor logic to reuse `this.nativeEnumTypeNames` when building `annotations`, so `nativeEnumTypeNames` stays consistent with the defensively copied instance state and avoids leaking the caller’s array reference.
🤖 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/migration/src/aggregate/verifier.ts`:
- Around line 241-254: The table-name collection in the schema helper is
returning duplicates when the same table exists under multiple namespaces, which
later causes duplicate orphan entries. Update the logic in the schema table-name
gathering path used by detectOrphanElements to dedupe names as they are
collected, ideally by switching the local accumulator from an array to a Set and
returning unique table names while preserving the existing
namespaces/schema.tables fallback behavior.
In `@packages/2-sql/9-family/src/core/control-adapter.ts`:
- Around line 212-218: The diffDatabaseSchema input currently omits
normalizeDefault and normalizeNativeType, so the shared diff path compares raw
defaults and types and can produce false drift. Update the diffDatabaseSchema
signature in control-adapter and any implementations/callers to accept these two
normalizer fields, then thread them through to the shared diff logic used by
diffDatabaseSchema so the target diff applies the normalizations consistently.
In `@packages/2-sql/9-family/src/core/control-instance.ts`:
- Around line 730-737: After `filterSchemaDiffIssues` removes structural issues
in `control-instance.ts`, recompute the `ok` state for the returned `sqlResult`
instead of returning it unchanged. Update the
`schemaDiffIssues`/`schema.counts.fail` branch so that `ok` reflects the
filtered schema issues, especially when the original failure was caused only by
suppressed structural diffs, and keep the logic localized around the
`schemaDiffIssues` and `relationalFails` handling.
- Around line 947-955: The table flattening in toSchemaView is dropping the
namespace identity, which causes duplicate SchemaTreeNode IDs for tables with
the same name in different schemas. Update the tableEntries construction in
control-instance.ts so each entry carries the namespace/schema name from
namespaceSchemaNodes, then use that namespace when building the table-level
nodes and IDs in the subsequent tableNodes mapping. Ensure the resulting
SchemaTreeNode labels and IDs stay unique for cases like public.users and
auth.users.
In `@packages/2-sql/9-family/src/core/schema-verify/verify-sql-schema.ts`:
- Around line 340-343: The strict extra-table claim check in verify-sql-schema
should be limited to the paired namespace set, not all contract namespaces.
Update the claimed-table scan in verifySqlSchema (including the same logic used
around the missing-table handling and the matching block at the later location)
so it skips namespaces not present in restrictToNamespaceIds when that set is
provided. This will keep tree verification from letting a table claimed in auth
mask an unexpected same-named table in the public actual node.
- Around line 1407-1426: `mergeVerifyResults` is merging `schema.counts`
correctly but leaving `summary` from the first result, which can make
multi-namespace failures report stale counts. Update `mergeVerifyResults` in
`verify-sql-schema.ts` so the merged result recomputes `summary` from the
combined `a.schema` and `b.schema` data after summing counts, rather than
reusing `a.summary`; use the existing `mergeVerifyResults` flow and the
`schema`/`counts` fields to keep the summary consistent with the merged totals.
In `@packages/3-targets/3-targets/postgres/package.json`:
- Line 38: The dependency entry for `@prisma-next/psl-printer` in the postgres
package.json uses workspace:* instead of the repo’s pinned internal version
convention. Update this package’s internal `@prisma-next/`* dependency to use the
same literal workspace version string as the other internal deps in this
manifest, keeping the existing dependency naming intact.
In `@packages/3-targets/3-targets/postgres/src/core/migrations/planner.ts`:
- Around line 393-400: The namespace selection logic in namespaceSchemaNode is
too permissive because it falls back to namespaceNodes[0] even when multiple
namespaces exist and none match schemaName. Update the helper so it only returns
the first namespace for the single-namespace case, and otherwise returns no
match or raises the appropriate ambiguity handling for relational planning. Keep
the lookup tied to namespaceSchemaNodes and the byName selection so ambiguous
schema matches do not silently probe the wrong namespace.
In `@packages/3-targets/3-targets/postgres/test/rls-diffable-nodes.test.ts`:
- Around line 60-77: The role fixture in the Postgres diffable nodes tests is
using the wrong namespace coordinate for PostgresRole. Update the relevant role
assertions and any similar fixtures to use the cluster-scoped namespace sentinel
UNBOUND_NAMESPACE_ID instead of public, so the tests match the PostgresRole
contract while still verifying kind and name through the PostgresRole
constructor-based fixtures.
---
Nitpick comments:
In
`@packages/1-framework/3-tooling/migration/src/aggregate/project-schema-to-space.ts`:
- Around line 68-72: Replace the bare structural cast in the schema reader with
the repo-standard blindCast pattern. In project-schema-to-space’s schema
normalization logic, update the schemaObj assignment to use blindCast with an
explicit reason instead of a plain as cast, matching the convention used by
namespaceSchemaNodes and other duck-typed reads. Keep the inferred shape the
same while making the cast intent explicit and consistent with the coding
guidelines.
In `@packages/1-framework/3-tooling/migration/src/aggregate/verifier.ts`:
- Around line 237-240: The structural read in verifier.ts uses a bare `as` cast,
which violates the casting guideline. Update the schema narrowing in the
verifier logic to use `blindCast<T, "Reason">` or `castAs<T>` instead of the
direct assertion, keeping the existing `schemaIntrospection` handling and the
`schema` variable in the same flow.
In
`@packages/3-targets/3-targets/postgres/src/core/schema-ir/postgres-namespace-schema-node.ts`:
- Around line 54-62: `PostgresNamespaceSchemaNode` is putting the raw
`input.nativeEnumTypeNames` array into `annotations.pg` instead of the class’s
frozen copy. Update the constructor logic to reuse `this.nativeEnumTypeNames`
when building `annotations`, so `nativeEnumTypeNames` stays consistent with the
defensively copied instance state and avoids leaking the caller’s array
reference.
In `@packages/3-targets/6-adapters/postgres/src/core/control-adapter.ts`:
- Around line 597-602: `introspect` is doing a cluster-scoped PostgreSQL version
lookup once per schema and then overwriting the result in the `pgVersion`
accumulator. Hoist the `getPostgresVersion` call out of `introspectSchema` into
`introspect` so it runs once before the `resolvedSchemas` loop, pass the version
through as needed, and remove the version query from `introspectSchema` so it
only handles per-namespace introspection. Ensure the `introspect` and
`introspectSchema` flow still returns the same `pgVersion` value for all
namespaces.
🪄 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: 28eb30df-16d3-4255-8569-8ba7e658c740
⛔ Files ignored due to path filters (4)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlprojects/postgres-rls/plan.mdis excluded by!projects/**projects/postgres-rls/slices/schema-node-tree-restructure/design.mdis excluded by!projects/**projects/postgres-rls/slices/schema-node-tree-restructure/spec.mdis excluded by!projects/**
📒 Files selected for processing (97)
packages/1-framework/3-tooling/migration/src/aggregate/project-schema-to-space.tspackages/1-framework/3-tooling/migration/src/aggregate/verifier.tspackages/1-framework/3-tooling/migration/test/aggregate/project-schema-to-space.test.tspackages/2-sql/1-core/schema-ir/src/ir/sql-schema-ir.tspackages/2-sql/9-family/package.jsonpackages/2-sql/9-family/src/core/control-adapter.tspackages/2-sql/9-family/src/core/control-instance.tspackages/2-sql/9-family/src/core/migrations/contract-to-schema-ir.tspackages/2-sql/9-family/src/core/migrations/types.tspackages/2-sql/9-family/src/core/psl-contract-infer/printer-config.tspackages/2-sql/9-family/src/core/schema-verify/verify-sql-schema.tspackages/2-sql/9-family/src/exports/control.tspackages/2-sql/9-family/src/exports/psl-infer.tspackages/2-sql/9-family/src/exports/schema-verify.tspackages/2-sql/9-family/test/psl-contract-infer/default-mapping.test.tspackages/2-sql/9-family/tsdown.config.tspackages/3-extensions/pgvector/test/migrations/planner.behavior.test.tspackages/3-extensions/pgvector/test/migrations/planner.contract-to-schema-ir.test.tspackages/3-extensions/pgvector/test/migrations/planner.storage-types.test.tspackages/3-targets/3-targets/postgres/package.jsonpackages/3-targets/3-targets/postgres/src/core/authoring.tspackages/3-targets/3-targets/postgres/src/core/entity-kinds.tspackages/3-targets/3-targets/postgres/src/core/migrations/contract-to-postgres-database-schema-node.tspackages/3-targets/3-targets/postgres/src/core/migrations/contract-to-postgres-schema-ir.tspackages/3-targets/3-targets/postgres/src/core/migrations/diff-database-schema.tspackages/3-targets/3-targets/postgres/src/core/migrations/diff-postgres-schema.tspackages/3-targets/3-targets/postgres/src/core/migrations/op-factory-call.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/rls.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner.tspackages/3-targets/3-targets/postgres/src/core/migrations/runner.tspackages/3-targets/3-targets/postgres/src/core/migrations/verify-postgres-namespaces.tspackages/3-targets/3-targets/postgres/src/core/postgres-rls-policy.tspackages/3-targets/3-targets/postgres/src/core/postgres-role.tspackages/3-targets/3-targets/postgres/src/core/postgres-schema.tspackages/3-targets/3-targets/postgres/src/core/psl-infer/infer-psl-contract.tspackages/3-targets/3-targets/postgres/src/core/psl-infer/postgres-default-mapping.tspackages/3-targets/3-targets/postgres/src/core/psl-infer/postgres-type-map.tspackages/3-targets/3-targets/postgres/src/core/schema-ir/postgres-database-schema-node.tspackages/3-targets/3-targets/postgres/src/core/schema-ir/postgres-namespace-schema-node.tspackages/3-targets/3-targets/postgres/src/core/schema-ir/postgres-policy-schema-node.tspackages/3-targets/3-targets/postgres/src/core/schema-ir/postgres-role-schema-node.tspackages/3-targets/3-targets/postgres/src/core/schema-ir/postgres-schema-ir-annotations.tspackages/3-targets/3-targets/postgres/src/core/schema-ir/postgres-schema-ir.tspackages/3-targets/3-targets/postgres/src/core/schema-ir/postgres-table-schema-node.tspackages/3-targets/3-targets/postgres/src/exports/control.tspackages/3-targets/3-targets/postgres/src/exports/planner.tspackages/3-targets/3-targets/postgres/src/exports/types.tspackages/3-targets/3-targets/postgres/test/migrations/contract-to-postgres-database-schema-node.test.tspackages/3-targets/3-targets/postgres/test/migrations/contract-to-postgres-schema-ir.test.tspackages/3-targets/3-targets/postgres/test/migrations/diff-postgres-schema.test.tspackages/3-targets/3-targets/postgres/test/migrations/rls-ops.test.tspackages/3-targets/3-targets/postgres/test/migrations/rls-planner.test.tspackages/3-targets/3-targets/postgres/test/migrations/verify-postgres-namespaces.test.tspackages/3-targets/3-targets/postgres/test/postgres-contract-serializer.test.tspackages/3-targets/3-targets/postgres/test/postgres-database-schema-node.test.tspackages/3-targets/3-targets/postgres/test/postgres-namespace-schema-node.test.tspackages/3-targets/3-targets/postgres/test/postgres-table-ir.test.tspackages/3-targets/3-targets/postgres/test/postgres-table-schema-node.test.tspackages/3-targets/3-targets/postgres/test/psl-infer/fixtures.tspackages/3-targets/3-targets/postgres/test/psl-infer/infer-psl-contract.test.tspackages/3-targets/3-targets/postgres/test/psl-infer/postgres-type-map.test.tspackages/3-targets/3-targets/postgres/test/psl-infer/print-psl/print-psl.core.test.tspackages/3-targets/3-targets/postgres/test/psl-infer/print-psl/print-psl.defaults-and-types.test.tspackages/3-targets/3-targets/postgres/test/psl-infer/print-psl/print-psl.enums.test.tspackages/3-targets/3-targets/postgres/test/psl-infer/print-psl/print-psl.naming-and-constraints.test.tspackages/3-targets/3-targets/postgres/test/psl-infer/print-psl/print-psl.relations.test.tspackages/3-targets/3-targets/postgres/test/psl-policy-authoring.test.tspackages/3-targets/3-targets/postgres/test/rls-diffable-nodes.test.tspackages/3-targets/3-targets/postgres/test/rls-ir-kinds.test.tspackages/3-targets/3-targets/postgres/test/schema-ir-leaf-nodes.test.tspackages/3-targets/3-targets/sqlite/src/core/migrations/planner.tspackages/3-targets/3-targets/sqlite/src/core/migrations/runner.tspackages/3-targets/6-adapters/postgres/src/core/control-adapter.tspackages/3-targets/6-adapters/postgres/test/control-adapter.check-constraints.test.tspackages/3-targets/6-adapters/postgres/test/control-adapter.defaults.test.tspackages/3-targets/6-adapters/postgres/test/control-adapter.test.tspackages/3-targets/6-adapters/postgres/test/migrations/array-column-introspection.integration.test.tspackages/3-targets/6-adapters/postgres/test/migrations/fixtures/runner-fixtures.tspackages/3-targets/6-adapters/postgres/test/migrations/index-introspection.integration.test.tspackages/3-targets/6-adapters/postgres/test/migrations/planner.authoring-surface.test.tspackages/3-targets/6-adapters/postgres/test/migrations/planner.codec-field-event.test.tspackages/3-targets/6-adapters/postgres/test/migrations/planner.cross-space-fk-ddl.test.tspackages/3-targets/6-adapters/postgres/test/migrations/planner.fk-config.test.tspackages/3-targets/6-adapters/postgres/test/migrations/planner.integration.test.tspackages/3-targets/6-adapters/postgres/test/migrations/planner.reconciliation.integration.test.tspackages/3-targets/6-adapters/postgres/test/migrations/planner.reconciliation.test.tspackages/3-targets/6-adapters/postgres/test/migrations/planner.referential-actions.test.tspackages/3-targets/6-adapters/postgres/test/migrations/planner.semantic-satisfaction.test.tspackages/3-targets/6-adapters/postgres/test/migrations/rls-collect-extension-issues.test.tspackages/3-targets/6-adapters/postgres/test/migrations/rls-introspection.integration.test.tspackages/3-targets/6-adapters/postgres/test/migrations/rls-lifecycle-e2e.integration.test.tspackages/3-targets/6-adapters/postgres/test/migrations/rls-migration-plan.integration.test.tstest/integration/test/control-api.test.tstest/integration/test/cross-package/postgres-control-policy-planner.test.tstest/integration/test/family.introspect.integration.test.tstest/integration/test/family.introspect.test.tstest/integration/test/referential-actions.integration.test.ts
💤 Files with no reviewable changes (4)
- packages/3-targets/3-targets/postgres/test/migrations/contract-to-postgres-schema-ir.test.ts
- packages/3-targets/3-targets/postgres/test/postgres-table-ir.test.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/contract-to-postgres-schema-ir.ts
- packages/3-targets/3-targets/postgres/src/core/schema-ir/postgres-schema-ir.ts
| diffDatabaseSchema?(input: { | ||
| readonly contract: Contract<SqlStorage>; | ||
| readonly schema: SqlSchemaIRNode; | ||
| readonly strict: boolean; | ||
| readonly typeMetadataRegistry: ReadonlyMap<string, { readonly nativeType?: string }>; | ||
| readonly frameworkComponents: ReadonlyArray<TargetBoundComponentDescriptor<'sql', string>>; | ||
| }): VerifyDatabaseSchemaResult; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check combined diff implementations and whether they pass default/native-type normalizers.
rg -n -C4 '\bdiffDatabaseSchema\b|normalizeDefault|normalizeNativeType|verifySqlSchema(Tree)?\b' packages/2-sql/9-family packages/3-targetsRepository: prisma/prisma-next
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files and inspect the exact diff/normalizer flow.
files=(
packages/2-sql/9-family/src/core/control-adapter.ts
packages/2-sql/9-family/src/core/control-instance.ts
packages/2-sql/9-family/src/core/schema-verify/verify-sql-schema.ts
)
for f in "${files[@]}"; do
echo "### $f"
wc -l "$f"
ast-grep outline "$f" --view expanded || true
echo
doneRepository: prisma/prisma-next
Length of output: 5753
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the core flow around the new hook and the old fallback path.
sed -n '1,260p' packages/2-sql/9-family/src/core/control-instance.ts
echo '---'
sed -n '1,280p' packages/2-sql/9-family/src/core/schema-verify/verify-sql-schema.ts
echo '---'
sed -n '1,280p' packages/2-sql/9-family/src/core/control-adapter.tsRepository: prisma/prisma-next
Length of output: 28510
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find every diffDatabaseSchema implementation and its local helpers.
rg -n -C6 '\bdiffDatabaseSchema\b|normalizeDefault|normalizeNativeType|verifySqlSchema(Tree)?\b' \
packages/2-sql/9-family/src \
packages/3-targetsRepository: prisma/prisma-next
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrow reads around the changed control flow.
for f in \
packages/2-sql/9-family/src/core/control-instance.ts \
packages/2-sql/9-family/src/core/control-adapter.ts \
packages/2-sql/9-family/src/core/schema-verify/verify-sql-schema.ts
do
echo "### $f"
nl -ba "$f" | sed -n '1,260p' | sed -n '1,240p'
echo
doneRepository: prisma/prisma-next
Length of output: 254
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate source implementations only.
rg -n -C5 '\bdiffDatabaseSchema\b' \
packages/2-sql/9-family/src \
packages/3-targets \
--glob '!**/*.test.*'Repository: prisma/prisma-next
Length of output: 7259
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the concrete hook implementation(s) and the call site.
rg -n -C3 '\bdiffDatabaseSchema\b' packages --glob '!**/*.test.*'Repository: prisma/prisma-next
Length of output: 4807
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search source for the normalizer hooks and the new diff hook.
rg -n -C4 '\bnormalizeDefault\b|\bnormalizeNativeType\b|\bdiffDatabaseSchema\b' \
packages/2-sql/9-family/src \
--glob '!**/*.test.*'Repository: prisma/prisma-next
Length of output: 25925
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the mock target component shape used by schema verification tests.
rg -n -C8 'createMockPostgresComponent|normalizeDefault|normalizeNativeType|frameworkComponents' \
packages/2-sql/9-family/test packages/2-sql/9-family/src \
--glob '!**/*.test.*'Repository: prisma/prisma-next
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find helper files that define the test components and normalizers.
fd -a 'schema-verify.helpers.ts|control-policy' packages/2-sql/9-family/test packages/2-sql/9-family/srcRepository: prisma/prisma-next
Length of output: 466
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the concrete diffDatabaseSchema implementation bodies.
sed -n '130,240p' packages/3-targets/6-adapters/postgres/src/core/control-adapter.ts
echo '---'
sed -n '120,220p' packages/3-targets/6-adapters/sqlite/src/core/control-adapter.tsRepository: prisma/prisma-next
Length of output: 8609
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the shared Postgres diff implementation for normalizer handling.
rg -n -C6 'function diffPostgresDatabaseSchema|const diffPostgresDatabaseSchema|diffPostgresDatabaseSchema\(' \
packages/3-targets/6-adapters/postgres/src \
--glob '!**/*.test.*'Repository: prisma/prisma-next
Length of output: 1665
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the function definition and inspect its parameter list / body.
rg -n -C8 '^function diffPostgresDatabaseSchema|^const diffPostgresDatabaseSchema|^export function diffPostgresDatabaseSchema|diffPostgresDatabaseSchema\s*=\s*\(' \
packages/3-targets/6-adapters/postgres/src/core/control-adapter.tsRepository: prisma/prisma-next
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where diffPostgresDatabaseSchema is defined and what it closes over.
rg -n -C6 '\bdiffPostgresDatabaseSchema\b' packages --glob '!**/*.test.*'Repository: prisma/prisma-next
Length of output: 13209
Thread normalizers through diffDatabaseSchema The combined diff path currently has no way to receive normalizeDefault / normalizeNativeType, so the target diff compares raw defaults and types and can report false drift. Add these fields to the input and pass them into the shared diff.
🤖 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/2-sql/9-family/src/core/control-adapter.ts` around lines 212 - 218,
The diffDatabaseSchema input currently omits normalizeDefault and
normalizeNativeType, so the shared diff path compares raw defaults and types and
can produce false drift. Update the diffDatabaseSchema signature in
control-adapter and any implementations/callers to accept these two normalizer
fields, then thread them through to the shared diff logic used by
diffDatabaseSchema so the target diff applies the normalizations consistently.
| const schemaDiffIssues = filterSchemaDiffIssues( | ||
| rawSchemaDiffIssues, | ||
| sqlResult.schema.schemaDiffIssues, | ||
| contract.defaultControlPolicy, | ||
| ); | ||
| if (schemaDiffIssues.length === 0) return sqlResult; | ||
| const totalFails = sqlResult.schema.counts.fail + schemaDiffIssues.length; | ||
| const relationalFails = sqlResult.schema.counts.fail; | ||
| if (schemaDiffIssues.length === 0) { | ||
| if (schemaDiffIssues === sqlResult.schema.schemaDiffIssues) return sqlResult; | ||
| return { ...sqlResult, schema: { ...sqlResult.schema, schemaDiffIssues } }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Recompute ok after structural diff suppression.
If filterSchemaDiffIssues suppresses all structural issues, this branch returns the original sqlResult; when that result was failed only because of those structural issues, verify still fails incorrectly.
🤖 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/2-sql/9-family/src/core/control-instance.ts` around lines 730 - 737,
After `filterSchemaDiffIssues` removes structural issues in
`control-instance.ts`, recompute the `ok` state for the returned `sqlResult`
instead of returning it unchanged. Update the
`schemaDiffIssues`/`schema.counts.fail` branch so that `ok` reflects the
filtered schema issues, especially when the original failure was caused only by
suppressed structural diffs, and keep the logic localized around the
`schemaDiffIssues` and `relationalFails` handling.
| // When the caller pairs each contract namespace to its own actual node, it | ||
| // restricts the table check to that namespace; the full contract is still | ||
| // consulted for value-set / control-policy resolution. | ||
| if (restrictToNamespaceIds !== undefined && !restrictToNamespaceIds.has(namespaceId)) continue; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Scope strict extra-table claims to the paired namespace set.
Line 343 restricts missing-table checks, but the strict claimed check still scans every contract namespace. In tree verification, a contract table in auth can hide an unexpected same-named table in the public actual node.
Proposed fix
const namespaceIds = Object.keys(contract.storage.namespaces).sort((a, b) =>
a < b ? -1 : a > b ? 1 : 0,
);
+ const claimNamespaceIds = restrictToNamespaceIds ?? new Set(namespaceIds);
@@
if (strict) {
for (const tableName of Object.keys(schemaTables)) {
- const claimed = namespaceIds.some(
- (namespaceId) =>
- contract.storage.namespaces[namespaceId]?.entries.table?.[tableName] !== undefined,
- );
+ let claimed = false;
+ for (const namespaceId of claimNamespaceIds) {
+ if (contract.storage.namespaces[namespaceId]?.entries.table?.[tableName] !== undefined) {
+ claimed = true;
+ break;
+ }
+ }
if (!claimed) {Also applies to: 407-412
🤖 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/2-sql/9-family/src/core/schema-verify/verify-sql-schema.ts` around
lines 340 - 343, The strict extra-table claim check in verify-sql-schema should
be limited to the paired namespace set, not all contract namespaces. Update the
claimed-table scan in verifySqlSchema (including the same logic used around the
missing-table handling and the matching block at the later location) so it skips
namespaces not present in restrictToNamespaceIds when that set is provided. This
will keep tree verification from letting a table claimed in auth mask an
unexpected same-named table in the public actual node.
| function mergeVerifyResults( | ||
| a: VerifyDatabaseSchemaResult, | ||
| b: VerifyDatabaseSchemaResult, | ||
| ): VerifyDatabaseSchemaResult { | ||
| return { | ||
| ...a, | ||
| ok: a.ok && b.ok, | ||
| ...ifDefined('code', a.code ?? b.code), | ||
| schema: { | ||
| ...a.schema, | ||
| issues: [...a.schema.issues, ...b.schema.issues], | ||
| schemaDiffIssues: [...a.schema.schemaDiffIssues, ...b.schema.schemaDiffIssues], | ||
| counts: { | ||
| pass: a.schema.counts.pass + b.schema.counts.pass, | ||
| warn: a.schema.counts.warn + b.schema.counts.warn, | ||
| fail: a.schema.counts.fail + b.schema.counts.fail, | ||
| totalNodes: a.schema.counts.totalNodes + b.schema.counts.totalNodes, | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Recompute the merged summary after summing counts.
mergeVerifyResults sums counts.fail but keeps a.summary, so multi-namespace failures can report the first namespace’s failure count while returning larger merged counts.
Proposed fix
function mergeVerifyResults(
a: VerifyDatabaseSchemaResult,
b: VerifyDatabaseSchemaResult,
): VerifyDatabaseSchemaResult {
+ const counts = {
+ pass: a.schema.counts.pass + b.schema.counts.pass,
+ warn: a.schema.counts.warn + b.schema.counts.warn,
+ fail: a.schema.counts.fail + b.schema.counts.fail,
+ totalNodes: a.schema.counts.totalNodes + b.schema.counts.totalNodes,
+ };
+ const ok = a.ok && b.ok;
return {
...a,
- ok: a.ok && b.ok,
- ...ifDefined('code', a.code ?? b.code),
+ ok,
+ ...ifDefined('code', ok ? undefined : (a.code ?? b.code)),
+ summary: ok
+ ? 'Database schema satisfies contract'
+ : `Database schema does not satisfy contract (${counts.fail} failure${counts.fail === 1 ? '' : 's'})`,
schema: {
...a.schema,
issues: [...a.schema.issues, ...b.schema.issues],
schemaDiffIssues: [...a.schema.schemaDiffIssues, ...b.schema.schemaDiffIssues],
- counts: {
- pass: a.schema.counts.pass + b.schema.counts.pass,
- warn: a.schema.counts.warn + b.schema.counts.warn,
- fail: a.schema.counts.fail + b.schema.counts.fail,
- totalNodes: a.schema.counts.totalNodes + b.schema.counts.totalNodes,
- },
+ counts,
},
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function mergeVerifyResults( | |
| a: VerifyDatabaseSchemaResult, | |
| b: VerifyDatabaseSchemaResult, | |
| ): VerifyDatabaseSchemaResult { | |
| return { | |
| ...a, | |
| ok: a.ok && b.ok, | |
| ...ifDefined('code', a.code ?? b.code), | |
| schema: { | |
| ...a.schema, | |
| issues: [...a.schema.issues, ...b.schema.issues], | |
| schemaDiffIssues: [...a.schema.schemaDiffIssues, ...b.schema.schemaDiffIssues], | |
| counts: { | |
| pass: a.schema.counts.pass + b.schema.counts.pass, | |
| warn: a.schema.counts.warn + b.schema.counts.warn, | |
| fail: a.schema.counts.fail + b.schema.counts.fail, | |
| totalNodes: a.schema.counts.totalNodes + b.schema.counts.totalNodes, | |
| }, | |
| }, | |
| }; | |
| function mergeVerifyResults( | |
| a: VerifyDatabaseSchemaResult, | |
| b: VerifyDatabaseSchemaResult, | |
| ): VerifyDatabaseSchemaResult { | |
| const counts = { | |
| pass: a.schema.counts.pass + b.schema.counts.pass, | |
| warn: a.schema.counts.warn + b.schema.counts.warn, | |
| fail: a.schema.counts.fail + b.schema.counts.fail, | |
| totalNodes: a.schema.counts.totalNodes + b.schema.counts.totalNodes, | |
| }; | |
| const ok = a.ok && b.ok; | |
| return { | |
| ...a, | |
| ok, | |
| ...ifDefined('code', ok ? undefined : (a.code ?? b.code)), | |
| summary: ok | |
| ? 'Database schema satisfies contract' | |
| : `Database schema does not satisfy contract (${counts.fail} failure${counts.fail === 1 ? '' : 's'})`, | |
| schema: { | |
| ...a.schema, | |
| issues: [...a.schema.issues, ...b.schema.issues], | |
| schemaDiffIssues: [...a.schema.schemaDiffIssues, ...b.schema.schemaDiffIssues], | |
| counts, | |
| }, | |
| }; | |
| } |
🤖 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/2-sql/9-family/src/core/schema-verify/verify-sql-schema.ts` around
lines 1407 - 1426, `mergeVerifyResults` is merging `schema.counts` correctly but
leaving `summary` from the first result, which can make multi-namespace failures
report stale counts. Update `mergeVerifyResults` in `verify-sql-schema.ts` so
the merged result recomputes `summary` from the combined `a.schema` and
`b.schema` data after summing counts, rather than reusing `a.summary`; use the
existing `mergeVerifyResults` flow and the `schema`/`counts` fields to keep the
summary consistent with the merged totals.
| }, | ||
| "devDependencies": { | ||
| "@prisma-next/psl-parser": "workspace:0.14.0", | ||
| "@prisma-next/psl-printer": "workspace:*", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the repo’s literal workspace version for internal deps.
workspace:* bypasses the repo’s pinned internal dependency convention; match the package version used by the other @prisma-next/* entries.
Based on learnings, workspace package.json dependencies/devDependencies for internal Prisma Next packages should use workspace:<literal-version> rather than workspace:*.
Proposed fix
- "`@prisma-next/psl-printer`": "workspace:*",
+ "`@prisma-next/psl-printer`": "workspace:0.14.0",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "@prisma-next/psl-printer": "workspace:*", | |
| "`@prisma-next/psl-printer`": "workspace:0.14.0", |
🤖 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/3-targets/3-targets/postgres/package.json` at line 38, The
dependency entry for `@prisma-next/psl-printer` in the postgres package.json uses
workspace:* instead of the repo’s pinned internal version convention. Update
this package’s internal `@prisma-next/`* dependency to use the same literal
workspace version string as the other internal deps in this manifest, keeping
the existing dependency naming intact.
Source: Learnings
| const namespaceNodes = namespaceSchemaNodes(schema); | ||
| const byName = namespaceNodes.find( | ||
| (node) => | ||
| blindCast<{ readonly schemaName?: string }, 'reading the namespace node schema name'>(node) | ||
| .schemaName === schemaName, | ||
| ); | ||
| return byName ?? namespaceNodes[0]; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Avoid silently probing the first namespace on ambiguous matches.
The helper documents falling back only for the single-schema case, but Line 399 returns the first namespace even when multiple namespaces exist and none match schemaName. That can make relational strategies probe the wrong schema and reintroduce cross-schema table-name collisions.
Proposed fix
const byName = namespaceNodes.find(
(node) =>
blindCast<{ readonly schemaName?: string }, 'reading the namespace node schema name'>(node)
.schemaName === schemaName,
);
- return byName ?? namespaceNodes[0];
+ if (byName !== undefined) {
+ return byName;
+ }
+ return namespaceNodes.length === 1 ? namespaceNodes[0] : undefined;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const namespaceNodes = namespaceSchemaNodes(schema); | |
| const byName = namespaceNodes.find( | |
| (node) => | |
| blindCast<{ readonly schemaName?: string }, 'reading the namespace node schema name'>(node) | |
| .schemaName === schemaName, | |
| ); | |
| return byName ?? namespaceNodes[0]; | |
| } | |
| const namespaceNodes = namespaceSchemaNodes(schema); | |
| const byName = namespaceNodes.find( | |
| (node) => | |
| blindCast<{ readonly schemaName?: string }, 'reading the namespace node schema name'>(node) | |
| .schemaName === schemaName, | |
| ); | |
| if (byName !== undefined) { | |
| return byName; | |
| } | |
| return namespaceNodes.length === 1 ? namespaceNodes[0] : undefined; | |
| } |
🤖 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/3-targets/3-targets/postgres/src/core/migrations/planner.ts` around
lines 393 - 400, The namespace selection logic in namespaceSchemaNode is too
permissive because it falls back to namespaceNodes[0] even when multiple
namespaces exist and none match schemaName. Update the helper so it only returns
the first namespace for the single-namespace case, and otherwise returns no
match or raises the appropriate ambiguity handling for relational planning. Keep
the lookup tied to namespaceSchemaNodes and the byName selection so ambiguous
schema matches do not silently probe the wrong namespace.
| const role = new PostgresRole({ name: 'app_user', namespaceId: 'public' }); | ||
|
|
||
| it('id returns the table name', () => { | ||
| const table = new PostgresTableIR({ | ||
| name: 'profiles', | ||
| columns: {}, | ||
| foreignKeys: [], | ||
| uniques: [], | ||
| indexes: [], | ||
| rlsPolicies: [basePolicy], | ||
| }); | ||
| expect(table.id).toBe('profiles'); | ||
| it('has no id property', () => { | ||
| expect('id' in role).toBe(false); | ||
| }); | ||
|
|
||
| it('isEqualTo() always returns true', () => { | ||
| const a = new PostgresTableIR({ | ||
| name: 'profiles', | ||
| columns: {}, | ||
| foreignKeys: [], | ||
| uniques: [], | ||
| indexes: [], | ||
| rlsPolicies: [basePolicy], | ||
| }); | ||
| const b = new PostgresTableIR({ | ||
| name: 'profiles', | ||
| columns: {}, | ||
| foreignKeys: [], | ||
| uniques: [], | ||
| indexes: [], | ||
| rlsPolicies: [], | ||
| }); | ||
| expect(a.isEqualTo(b)).toBe(true); | ||
| it('has no children method', () => { | ||
| expect('children' in role).toBe(false); | ||
| }); | ||
|
|
||
| it('children() returns the policy nodes', () => { | ||
| const table = new PostgresTableIR({ | ||
| name: 'profiles', | ||
| columns: {}, | ||
| foreignKeys: [], | ||
| uniques: [], | ||
| indexes: [], | ||
| rlsPolicies: [basePolicy], | ||
| }); | ||
| expect(table.children()).toEqual([basePolicy]); | ||
| it('has no isEqualTo method', () => { | ||
| expect('isEqualTo' in role).toBe(false); | ||
| }); | ||
|
|
||
| it('instance is frozen', () => { | ||
| const table = new PostgresTableIR({ | ||
| name: 'profiles', | ||
| columns: {}, | ||
| foreignKeys: [], | ||
| uniques: [], | ||
| indexes: [], | ||
| }); | ||
| expect(Object.isFrozen(table)).toBe(true); | ||
| }); | ||
|
|
||
| describe('isPostgresTableIR guard', () => { | ||
| it('returns true for a PostgresTableIR', () => { | ||
| const table = new PostgresTableIR({ | ||
| name: 'profiles', | ||
| columns: {}, | ||
| foreignKeys: [], | ||
| uniques: [], | ||
| indexes: [], | ||
| }); | ||
| expect(isPostgresTableIR(table)).toBe(true); | ||
| }); | ||
|
|
||
| it('returns false for a PostgresRlsPolicy', () => { | ||
| expect(isPostgresTableIR(basePolicy)).toBe(false); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('PostgresRole DiffableNode', () => { | ||
| it('id returns the role name (roles are cluster-unique)', () => { | ||
| const role = new PostgresRole({ name: 'app_user', namespaceId: 'public' }); | ||
| expect(role.id).toBe('app_user'); | ||
| }); | ||
|
|
||
| it('id propagates the role name from input', () => { | ||
| const role = new PostgresRole({ name: 'anon', namespaceId: 'sentinel_namespace' }); | ||
| expect(role.id).toBe('anon'); | ||
| }); | ||
|
|
||
| it('children() returns an empty list (leaf node)', () => { | ||
| const role = new PostgresRole({ name: 'app_user', namespaceId: 'public' }); | ||
| expect(role.children()).toEqual([]); | ||
| it('retains kind, name, and namespaceId', () => { | ||
| expect(role.kind).toBe('role'); | ||
| expect(role.name).toBe('app_user'); | ||
| expect(role.namespaceId).toBe('public'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the cluster-scoped namespace sentinel in role fixtures.
PostgresRole is documented as always using UNBOUND_NAMESPACE_ID; these tests currently assert public, which codifies an invalid role coordinate.
Proposed test fix
+import { UNBOUND_NAMESPACE_ID } from '`@prisma-next/framework-components/ir`';
import { describe, expect, it } from 'vitest';
@@
- const role = new PostgresRole({ name: 'app_user', namespaceId: 'public' });
+ const role = new PostgresRole({ name: 'app_user', namespaceId: UNBOUND_NAMESPACE_ID });
@@
- expect(role.namespaceId).toBe('public');
+ expect(role.namespaceId).toBe(UNBOUND_NAMESPACE_ID);
@@
- const role = new PostgresRole({ name: 'anon', namespaceId: 'public' });
+ const role = new PostgresRole({ name: 'anon', namespaceId: UNBOUND_NAMESPACE_ID });
@@
- const role = new PostgresRole({ name: 'anon', namespaceId: 'public' });
+ const role = new PostgresRole({ name: 'anon', namespaceId: UNBOUND_NAMESPACE_ID });Also applies to: 104-106, 128-130
🤖 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/3-targets/3-targets/postgres/test/rls-diffable-nodes.test.ts` around
lines 60 - 77, The role fixture in the Postgres diffable nodes tests is using
the wrong namespace coordinate for PostgresRole. Update the relevant role
assertions and any similar fixtures to use the cluster-scoped namespace sentinel
UNBOUND_NAMESPACE_ID instead of public, so the tests match the PostgresRole
contract while still verifying kind and name through the PostgresRole
constructor-based fixtures.
…PSL slice; note the fail-loud guard) Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The inference gather flattens the tree-s tables into one bucket for the
single-namespace `__unspecified__` PSL output. It did a silent last-wins
overwrite if two namespaces held the same table name — unreachable today
(`contract infer` introspects a single namespace) but a new silent-lossy path on
this branch. It now throws an actionable error naming the table, mirroring the
`contractToSchemaIR` duplicate-name throw: same-named cross-schema tables are not
yet supported for `contract infer` (multi-namespace `namespace { … }` output is a
later slice). New test proves the tree with two same-named tables throws instead
of dropping one. Behaviour-neutral: unreachable on all single-namespace paths —
psl-infer suite, contract-infer e2e, and fixtures:check stay green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: willbot <w.a.madden+machine@gmail.com>
Signed-off-by: Will Madden <madden@prisma.io>
…tor operation The combined database-schema diff was an optional method on the SQL control adapter. Move it to the SQL target descriptor and make it required: schema comparison is target logic (dialect type/default maps, schema-tree walk), not adapter I/O, so it belongs on the descriptor alongside contractSerializer and inferPslContract. - Relocate the relational diffing code from core/schema-verify/ to core/diff/ (verify-sql-schema.ts -> sql-schema-diff.ts, plus the sibling files) with no logic changes. The family entrypoint moves from ./schema-verify to ./diff. - Add diffDatabaseSchema to SqlControlTargetDescriptor as a required operation. Postgres provides relational + policy diffing (diffPostgresDatabaseSchema); SQLite provides relational-only diffing via a new diffSqliteDatabaseSchema. - The family verifier (verifySchema) becomes a thin consumer: it introspects the actual schema, calls target.diffDatabaseSchema, applies control-policy suppression as a post-step, and fails when a surviving issue remains. It no longer composes any diffing itself and has no SQLite fallback branch. - Drop the now-dead diffDatabaseSchema method and orphaned imports from the Postgres control adapter; update importers and test fixtures accordingly. Behavior-neutral: planner op-assertions are unchanged and pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…ff flatten helper
The schema view and SQLite reached into namespaceSchemaNodes — a diff-internal
flatten helper — to walk the schema IR. Decouple both so the helper is used only
inside the diff module.
- toSchemaView now walks the schema-IR tree own structure (root -> namespaces ->
tables) directly instead of calling namespaceSchemaNodes, and drops that
import from control-instance.ts. A multi-namespace test locks the flatten
output.
- SQLite post-apply verify (runner) and the planner schema-issue collection now
go through diffSqliteDatabaseSchema (added in the prior unit) rather than
namespaceSchemaNodes(x)[0] ?? { tables: {} } + a direct verifySqlSchema call.
The duplicated empty-schema fallback is gone; the planner narrows the flat
SQLite node directly when building ops.
Behavior-neutral: schema-view output and SQLite db verify verdicts are
unchanged; the SQLite planner op-assertions are unaffected.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: willbot <w.a.madden+machine@gmail.com>
Signed-off-by: Will Madden <madden@prisma.io>
… flatten helper relationalNamespaceNode was the last consumer of namespaceSchemaNodes outside the diff module. The Postgres planner always holds a PostgresDatabaseSchemaNode, so it does not need the generic flatten helper: ensure the node and read its .namespaces directly to select the namespace by schemaName. namespaceSchemaNodes is now used only inside the diff module (and re-exported from exports/diff). Behavior-neutral: planner ops byte-identical (planner/rls-planner op-assertions unmodified and passing). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…response) Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/1-framework/3-tooling/migration/src/aggregate/verifier.ts (1)
229-253: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDedupe live entity names before building orphan list.
listEntityNameshas no uniqueness contract (readonly string[]). If any family's implementation returns the same entity name twice (e.g., unclaimed same-named table in two Postgres namespaces),orphanswill contain duplicate entries — the same class of bug flagged in a prior review on the oldcollectLiveTableNamespath, now reachable again via the delegated callback.🐛 Proposed fix
function detectOrphanElements( schemaIntrospection: unknown, members: ReadonlyArray<ContractSpaceMember>, listEntityNames: ListSchemaEntityNames, ): readonly OrphanElement[] { - const liveTableNames = listEntityNames(schemaIntrospection); - if (liveTableNames.length === 0) return []; + const liveTableNames = new Set(listEntityNames(schemaIntrospection)); + if (liveTableNames.size === 0) return [];🤖 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/aggregate/verifier.ts` around lines 229 - 253, The orphan detection in detectOrphanElements can emit duplicate table entries because listEntityNames() returns a plain string array with no uniqueness guarantee. Deduplicate the live table names before iterating them to build orphans, then keep the existing claimedTables membership check and final sort so detectOrphanElements only returns one orphan per entity name.packages/3-targets/6-adapters/postgres/src/core/control-adapter.ts (1)
556-586: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winRedundant per-namespace
getPostgresVersionround trip.
introspectSchemacallsthis.getPostgresVersion(driver)(line 1176) on every iteration of theintrospect()loop (lines 572-576), even though the Postgres version is database-wide, not namespace-scoped. For N namespaces this issues N-1 unnecessary round trips fetching the same value repeatedly.♻️ Hoist the version lookup out of the per-namespace loop
const namespaces: Record<string, PostgresNamespaceSchemaNode> = {}; - let pgVersion = 'unknown'; + const pgVersion = await this.getPostgresVersion(driver); for (const resolved of resolvedSchemas) { - const { namespace, pgVersion: version } = await this.introspectSchema(driver, resolved); + const { namespace } = await this.introspectSchema(driver, resolved); namespaces[resolved] = namespace; - pgVersion = version; }And drop
pgVersionfromintrospectSchema's return value/query accordingly.Also applies to: 660-663, 1176-1176
🤖 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/3-targets/6-adapters/postgres/src/core/control-adapter.ts` around lines 556 - 586, The introspection flow is doing a redundant database-wide Postgres version lookup inside each namespace pass. Hoist the version fetch out of `introspect()` and `introspectSchema()` so `getPostgresVersion(driver)` runs once per `introspect` call, then pass the single version through to `PostgresDatabaseSchemaNode`; update `introspectSchema` to stop returning `pgVersion` and remove any per-namespace version query usage.packages/2-sql/9-family/src/core/migrations/contract-to-schema-ir.ts (2)
378-404: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused
annotationNamespacevalidation incontractNamespaceToSchemaIR.The empty-string check on
options.annotationNamespace(line 383) guards a value that this function never actually uses — no annotations are derived or returned here, unlikecontractToSchemaIR. This looks like leftover copy-paste from the sibling function.🤖 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/2-sql/9-family/src/core/migrations/contract-to-schema-ir.ts` around lines 378 - 404, Remove the unnecessary annotationNamespace empty-string validation from contractNamespaceToSchemaIR, since this function does not use annotationNamespace at all. Keep the rest of the namespace-to-schema conversion logic intact in contractNamespaceToSchemaIR, and leave annotationNamespace validation only in the path where it is actually consumed, such as contractToSchemaIR.
378-451: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated table-conversion loop between
contractNamespaceToSchemaIRandcontractToSchemaIR.Both functions independently build
storageTypesfromstorage.typesand iterateentries.tableto callconvertTable/assertStorageTable. Consider havingcontractToSchemaIRdelegate per-namespace conversion tocontractNamespaceToSchemaIR(layering the cross-namespace duplicate-table check andderiveAnnotationson top) to avoid drift between the two implementations.🤖 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/2-sql/9-family/src/core/migrations/contract-to-schema-ir.ts` around lines 378 - 451, Both `contractNamespaceToSchemaIR` and `contractToSchemaIR` duplicate the same storage-type setup and table conversion logic, so refactor to reuse the namespace-level conversion path instead of maintaining two loops. Make `contractToSchemaIR` delegate per namespace to `contractNamespaceToSchemaIR` or extract the shared `assertStorageTable`/`convertTable` iteration into a helper, while keeping the cross-namespace duplicate-table guard and `deriveAnnotations` only in `contractToSchemaIR`. This keeps the behavior in sync and avoids drift between `contractNamespaceToSchemaIR`, `contractToSchemaIR`, and `convertTable`.
♻️ Duplicate comments (4)
packages/3-targets/3-targets/postgres/src/core/schema-ir/postgres-table-schema-node.ts (1)
97-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
isEqualTodoesn't validate node kind (see policy node comment).Consistent with the general design (identity-based equality), but unlike
PostgresPolicySchemaNode.isEqualTo, this comparesthis.id === other.idwithout confirmingotheris actually aPostgresTableSchemaNode. Low practical risk since the differ pairs same-kind nodes structurally, but flagging alongside the corresponding comment inpostgres-policy-schema-node.tsfor consistency.🤖 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/3-targets/3-targets/postgres/src/core/schema-ir/postgres-table-schema-node.ts` around lines 97 - 99, Update PostgresTableSchemaNode.isEqualTo to mirror the kind-checking pattern used in PostgresPolicySchemaNode.isEqualTo: verify the other node is actually a PostgresTableSchemaNode before comparing ids, then keep the identity-based id comparison only after that guard. This keeps equality consistent with the schema node design and avoids treating different node kinds as equal when ids collide.packages/2-sql/9-family/src/core/migrations/types.ts (1)
509-515: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
DiffDatabaseSchemaInputstill omits normalizer hooks.
DiffDatabaseSchemaInputcarriescontract,schema,strict,typeMetadataRegistry,frameworkComponentsbut nonormalizeDefault/normalizeNativeType. Per the graph evidence,PostgresMigrationPlanner.planSqlcallsdiffPostgresDatabaseSchemawith exactly this input shape (no normalizers passed), so the combined database-schema diff compares raw introspected defaults/native types against contract-declared ones with no normalization — even thoughSqlControlAdapter.normalizeDefault/normalizeNativeTypeexist precisely for this purpose and are consumed elsewhere (verifySqlSchema/verifySqlSchemaTree). This can produce false-positive drift in bothdb verifyand migration planning for targets whose adapter defines these normalizers.This is the same concern raised in a previous review round on this PR, now relocated to this interface after
diffDatabaseSchemamoved from the adapter to the target descriptor.♻️ Suggested fix
export interface DiffDatabaseSchemaInput { readonly contract: Contract<SqlStorage>; readonly schema: SqlSchemaIRNode; readonly strict: boolean; readonly typeMetadataRegistry: ReadonlyMap<string, { readonly nativeType?: string }>; readonly frameworkComponents: ReadonlyArray<TargetBoundComponentDescriptor<'sql', string>>; + readonly normalizeDefault?: DefaultNormalizer; + readonly normalizeNativeType?: NativeTypeNormalizer; }Thread these through to each
diffDatabaseSchemaimplementation (Postgres, SQLite) and their call sites (planner, verify path) from the adapter'snormalizeDefault/normalizeNativeType.🤖 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/2-sql/9-family/src/core/migrations/types.ts` around lines 509 - 515, `DiffDatabaseSchemaInput` is missing the adapter normalization hooks needed by schema diffing. Add `normalizeDefault` and `normalizeNativeType` to `DiffDatabaseSchemaInput`, then thread them from the relevant `SqlControlAdapter` into `diffDatabaseSchema`/`diffPostgresDatabaseSchema` call sites such as `PostgresMigrationPlanner.planSql` and the verify path. Update the `diffDatabaseSchema` implementations for Postgres and SQLite to use these hooks when comparing defaults/native types, matching the existing behavior in `verifySqlSchema` and `verifySqlSchemaTree`.packages/2-sql/9-family/src/core/control-instance.ts (2)
972-995: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNamespace identity is still lost when flattening tables for
toSchemaView.
tableEntriesflattensroot.namespaces/root.tablesand later builds IDs purely fromtableName(e.g.table-${tableName},column-${tableName}-${columnName}). Two tables with the same name in different namespaces (e.g.public.usersandauth.users) still collide on identicalSchemaTreeNodeIDs and labels, exactly as previously flagged.🐛 Proposed fix
- const tableEntries: ReadonlyArray<[string, SqlTableIR]> = tableRecords.flatMap((tables) => - Object.entries(tables), - ); + const tableEntries: ReadonlyArray<[string, SqlTableIR]> = + root.namespaces !== undefined + ? Object.entries(root.namespaces).flatMap(([nsKey, namespace]) => + Object.entries(namespace.tables).map( + ([tableName, table]) => [`${nsKey}.${tableName}`, table] as [string, SqlTableIR], + ), + ) + : Object.entries(root.tables ?? {});This still requires threading the qualified key through the
column-,primary-key-,unique-, andindex-ID templates below (they currently reuse the baretableNametoo).🤖 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/2-sql/9-family/src/core/control-instance.ts` around lines 972 - 995, `toSchemaView` still drops namespace identity when flattening schema tables, causing ID collisions for same-named tables across namespaces. Update the table flattening in `toSchemaView` so each entry carries its namespace-qualified key (or equivalent unique table identifier), then use that qualified value when generating `SchemaTreeNode` IDs and labels in the table, column, primary-key, unique, and index node creation paths. Keep the existing structure of `tableRecords`/`tableEntries`, but thread the namespace-qualified identifier through the later mappings instead of reusing bare `tableName`.
722-752: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
ok/code/summaryare not recomputed when control-policy suppression emptiesschemaDiffIssues.The
elsebranch provessqlResult.schema.counts.fail(akarelationalFails) excludesschemaDiffIssues— it explicitly addsschemaDiffIssues.lengthback in viatotalFails = relationalFails + schemaDiffIssues.length. That meansdiffDatabaseSchema's ownokwas computed from the unfilteredschemaDiffIssues(since it isn't passed the control policy). WhenfilterSchemaDiffIssuessuppresses every structural issue down to zero, theschemaDiffIssues.length === 0branch returnssqlResult(or a shallow variant) with the staleok/code/summaryfrom before filtering — if the pre-filter failure was caused solely by the now-suppressed issues,verifySchemastill reports failure.🐛 Proposed fix
const schemaDiffIssues = filterSchemaDiffIssues( sqlResult.schema.schemaDiffIssues, contract.defaultControlPolicy, ); const relationalFails = sqlResult.schema.counts.fail; - if (schemaDiffIssues.length === 0) { - if (schemaDiffIssues === sqlResult.schema.schemaDiffIssues) return sqlResult; - return { ...sqlResult, schema: { ...sqlResult.schema, schemaDiffIssues } }; - } - const totalFails = relationalFails + schemaDiffIssues.length; + if ( + schemaDiffIssues === sqlResult.schema.schemaDiffIssues && + schemaDiffIssues.length === sqlResult.schema.schemaDiffIssues.length + ) { + return sqlResult; + } + const totalFails = relationalFails + schemaDiffIssues.length; + const ok = totalFails === 0; return { ...sqlResult, - ok: false, - code: sqlResult.code ?? 'PN-RUN-3010', - summary: `Database schema does not satisfy contract (${totalFails} failure${totalFails === 1 ? '' : 's'})`, + ok, + ...(ok + ? {} + : { + code: sqlResult.code ?? 'PN-RUN-3010', + summary: `Database schema does not satisfy contract (${totalFails} failure${totalFails === 1 ? '' : 's'})`, + }), schema: { ...sqlResult.schema, schemaDiffIssues, counts: { ...sqlResult.schema.counts, fail: totalFails }, }, };🤖 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/2-sql/9-family/src/core/control-instance.ts` around lines 722 - 752, In verifySchema, the returned sqlResult can keep stale ok/code/summary values after filterSchemaDiffIssues removes all structural issues, because diffDatabaseSchema was computed before control-policy suppression. Update the VerifySchema path to recompute the result status from the filtered schemaDiffIssues and relationalFails whenever the filtered list differs from sqlResult.schema.schemaDiffIssues, including the zero-issue case, so the returned schema summary matches the post-filter counts.
🤖 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/control-api/operations/db-run.ts`:
- Around line 188-195: The duplicated `projectSchemaToMember` delegation in
`db-run.ts` and `db-verify.ts` should be factored into a shared CLI helper
instead of repeating the same `blindCast` wrapper and comment. Extract a small
utility in a common control-api/CLI module that accepts the schema and
`ownedByOtherNames`, performs the `familyInstance.projectSchemaToMember` call,
and reuse it from both operations to keep the cast and opaque-schema
justification in one place.
In
`@packages/3-targets/3-targets/postgres/src/core/schema-ir/postgres-database-schema-node.ts`:
- Around line 68-70: Update PostgresDatabaseSchemaNode.isEqualTo to match the
stricter comparison behavior used by PostgresRoleSchemaNode: validate that the
other DiffableNode is the expected node kind before comparing ids, and throw or
otherwise fail on mismatched kinds instead of returning a false positive. Keep
the comparison logic within isEqualTo consistent with the nodeKind-based guard
already used elsewhere so future tree-shape changes do not hide cross-type
comparisons.
In
`@packages/3-targets/3-targets/postgres/src/core/schema-ir/postgres-namespace-schema-node.ts`:
- Around line 54-56: The `isEqualTo` implementation on
`PostgresNamespaceSchemaNode` only compares `id` and does not verify that
`other` is the same node type. Update this method to first check that `other` is
a `PostgresNamespaceSchemaNode`, following the stricter pattern used by
`PostgresRoleSchemaNode.isEqualTo`, and only then compare identifiers.
In
`@packages/3-targets/3-targets/postgres/src/core/schema-ir/postgres-policy-schema-node.ts`:
- Around line 73-84: The PostgresPolicySchemaNode.isEqualTo implementation is
the only sibling that performs an explicit kind check before comparing ids,
making it inconsistent with the other schema nodes. Align it with the other
isEqualTo methods in the same diff by either removing the
PostgresPolicySchemaNode.is(node) guard and relying on the id comparison like
the table/namespace/database nodes, or applying the same kind validation pattern
consistently across all sibling nodes.
---
Outside diff comments:
In `@packages/1-framework/3-tooling/migration/src/aggregate/verifier.ts`:
- Around line 229-253: The orphan detection in detectOrphanElements can emit
duplicate table entries because listEntityNames() returns a plain string array
with no uniqueness guarantee. Deduplicate the live table names before iterating
them to build orphans, then keep the existing claimedTables membership check and
final sort so detectOrphanElements only returns one orphan per entity name.
In `@packages/2-sql/9-family/src/core/migrations/contract-to-schema-ir.ts`:
- Around line 378-404: Remove the unnecessary annotationNamespace empty-string
validation from contractNamespaceToSchemaIR, since this function does not use
annotationNamespace at all. Keep the rest of the namespace-to-schema conversion
logic intact in contractNamespaceToSchemaIR, and leave annotationNamespace
validation only in the path where it is actually consumed, such as
contractToSchemaIR.
- Around line 378-451: Both `contractNamespaceToSchemaIR` and
`contractToSchemaIR` duplicate the same storage-type setup and table conversion
logic, so refactor to reuse the namespace-level conversion path instead of
maintaining two loops. Make `contractToSchemaIR` delegate per namespace to
`contractNamespaceToSchemaIR` or extract the shared
`assertStorageTable`/`convertTable` iteration into a helper, while keeping the
cross-namespace duplicate-table guard and `deriveAnnotations` only in
`contractToSchemaIR`. This keeps the behavior in sync and avoids drift between
`contractNamespaceToSchemaIR`, `contractToSchemaIR`, and `convertTable`.
In `@packages/3-targets/6-adapters/postgres/src/core/control-adapter.ts`:
- Around line 556-586: The introspection flow is doing a redundant database-wide
Postgres version lookup inside each namespace pass. Hoist the version fetch out
of `introspect()` and `introspectSchema()` so `getPostgresVersion(driver)` runs
once per `introspect` call, then pass the single version through to
`PostgresDatabaseSchemaNode`; update `introspectSchema` to stop returning
`pgVersion` and remove any per-namespace version query usage.
---
Duplicate comments:
In `@packages/2-sql/9-family/src/core/control-instance.ts`:
- Around line 972-995: `toSchemaView` still drops namespace identity when
flattening schema tables, causing ID collisions for same-named tables across
namespaces. Update the table flattening in `toSchemaView` so each entry carries
its namespace-qualified key (or equivalent unique table identifier), then use
that qualified value when generating `SchemaTreeNode` IDs and labels in the
table, column, primary-key, unique, and index node creation paths. Keep the
existing structure of `tableRecords`/`tableEntries`, but thread the
namespace-qualified identifier through the later mappings instead of reusing
bare `tableName`.
- Around line 722-752: In verifySchema, the returned sqlResult can keep stale
ok/code/summary values after filterSchemaDiffIssues removes all structural
issues, because diffDatabaseSchema was computed before control-policy
suppression. Update the VerifySchema path to recompute the result status from
the filtered schemaDiffIssues and relationalFails whenever the filtered list
differs from sqlResult.schema.schemaDiffIssues, including the zero-issue case,
so the returned schema summary matches the post-filter counts.
In `@packages/2-sql/9-family/src/core/migrations/types.ts`:
- Around line 509-515: `DiffDatabaseSchemaInput` is missing the adapter
normalization hooks needed by schema diffing. Add `normalizeDefault` and
`normalizeNativeType` to `DiffDatabaseSchemaInput`, then thread them from the
relevant `SqlControlAdapter` into
`diffDatabaseSchema`/`diffPostgresDatabaseSchema` call sites such as
`PostgresMigrationPlanner.planSql` and the verify path. Update the
`diffDatabaseSchema` implementations for Postgres and SQLite to use these hooks
when comparing defaults/native types, matching the existing behavior in
`verifySqlSchema` and `verifySqlSchemaTree`.
In
`@packages/3-targets/3-targets/postgres/src/core/schema-ir/postgres-table-schema-node.ts`:
- Around line 97-99: Update PostgresTableSchemaNode.isEqualTo to mirror the
kind-checking pattern used in PostgresPolicySchemaNode.isEqualTo: verify the
other node is actually a PostgresTableSchemaNode before comparing ids, then keep
the identity-based id comparison only after that guard. This keeps equality
consistent with the schema node design and avoids treating different node kinds
as equal when ids collide.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3e68eb10-1603-4fa2-b4c5-0e06d57d7e95
📒 Files selected for processing (88)
packages/1-framework/1-core/framework-components/src/control/control-instances.tspackages/1-framework/3-tooling/cli/src/control-api/operations/db-run.tspackages/1-framework/3-tooling/cli/src/control-api/operations/db-verify.tspackages/1-framework/3-tooling/cli/test/config-types.test.tspackages/1-framework/3-tooling/migration/src/aggregate/planner-types.tspackages/1-framework/3-tooling/migration/src/aggregate/planner.tspackages/1-framework/3-tooling/migration/src/aggregate/project-schema-to-space.tspackages/1-framework/3-tooling/migration/src/aggregate/strategies/synth.tspackages/1-framework/3-tooling/migration/src/aggregate/verifier.tspackages/1-framework/3-tooling/migration/src/exports/aggregate.tspackages/1-framework/3-tooling/migration/test/aggregate/planner.test.tspackages/1-framework/3-tooling/migration/test/aggregate/project-schema-to-space.test.tspackages/1-framework/3-tooling/migration/test/aggregate/strategies/synth.test.tspackages/1-framework/3-tooling/migration/test/aggregate/verifier.test.tspackages/1-framework/3-tooling/migration/test/deletable-node-modules.test.tspackages/2-mongo-family/9-family/src/core/control-instance.tspackages/2-mongo-family/9-family/src/core/schema-shape.tspackages/2-mongo-family/9-family/src/exports/control.tspackages/2-mongo-family/9-family/test/schema-shape.test.tspackages/2-sql/1-core/contract/src/exports/types.tspackages/2-sql/1-core/contract/src/ir/storage-table.tspackages/2-sql/1-core/contract/src/types.tspackages/2-sql/1-core/schema-ir/src/exports/types.tspackages/2-sql/1-core/schema-ir/src/ir/sql-schema-ir-node.tspackages/2-sql/1-core/schema-ir/src/ir/sql-schema-ir.tspackages/2-sql/1-core/schema-ir/src/types.tspackages/2-sql/9-family/package.jsonpackages/2-sql/9-family/src/core/control-adapter.tspackages/2-sql/9-family/src/core/control-instance.tspackages/2-sql/9-family/src/core/diff/control-verify-emit.tspackages/2-sql/9-family/src/core/diff/schema-shape.tspackages/2-sql/9-family/src/core/diff/sql-schema-diff.tspackages/2-sql/9-family/src/core/diff/verifier-disposition.tspackages/2-sql/9-family/src/core/diff/verify-helpers.tspackages/2-sql/9-family/src/core/migrations/contract-to-schema-ir.tspackages/2-sql/9-family/src/core/migrations/types.tspackages/2-sql/9-family/src/core/psl-contract-infer/printer-config.tspackages/2-sql/9-family/src/exports/diff.tspackages/2-sql/9-family/src/exports/schema-verify.tspackages/2-sql/9-family/test/control-instance.descriptor-self-consistency.test.tspackages/2-sql/9-family/test/cross-contract-validation.test.tspackages/2-sql/9-family/test/operation-preview.test.tspackages/2-sql/9-family/test/schema-shape.test.tspackages/2-sql/9-family/test/schema-verify.basic.test.tspackages/2-sql/9-family/test/schema-verify.check-constraints.test.tspackages/2-sql/9-family/test/schema-verify.constraints.test.tspackages/2-sql/9-family/test/schema-verify.control-policy.test.tspackages/2-sql/9-family/test/schema-verify.defaults.test.tspackages/2-sql/9-family/test/schema-verify.helpers.tspackages/2-sql/9-family/test/schema-verify.referential-actions.test.tspackages/2-sql/9-family/test/schema-verify.semantic-satisfaction.test.tspackages/2-sql/9-family/test/schema-verify.storage-types.test.tspackages/2-sql/9-family/test/schema-verify.strict.test.tspackages/2-sql/9-family/test/schema-view.test.tspackages/2-sql/9-family/test/verifier-disposition.test.tspackages/2-sql/9-family/tsdown.config.tspackages/3-mongo-target/1-mongo-target/src/core/control-target.tspackages/3-targets/3-targets/postgres/src/core/migrations/contract-to-postgres-database-schema-node.tspackages/3-targets/3-targets/postgres/src/core/migrations/diff-database-schema.tspackages/3-targets/3-targets/postgres/src/core/migrations/diff-postgres-schema.tspackages/3-targets/3-targets/postgres/src/core/migrations/issue-planner.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner.tspackages/3-targets/3-targets/postgres/src/core/migrations/resolve-ddl-schema.tspackages/3-targets/3-targets/postgres/src/core/schema-ir/postgres-database-schema-node.tspackages/3-targets/3-targets/postgres/src/core/schema-ir/postgres-namespace-schema-node.tspackages/3-targets/3-targets/postgres/src/core/schema-ir/postgres-policy-schema-node.tspackages/3-targets/3-targets/postgres/src/core/schema-ir/postgres-role-schema-node.tspackages/3-targets/3-targets/postgres/src/core/schema-ir/postgres-table-schema-node.tspackages/3-targets/3-targets/postgres/src/core/schema-ir/schema-node-kinds.tspackages/3-targets/3-targets/postgres/src/exports/control.tspackages/3-targets/3-targets/postgres/src/exports/schema-ir-annotations.tspackages/3-targets/3-targets/postgres/test/migrations/contract-to-postgres-database-schema-node.test.tspackages/3-targets/3-targets/postgres/test/migrations/diff-postgres-schema.test.tspackages/3-targets/3-targets/postgres/test/postgres-database-schema-node.test.tspackages/3-targets/3-targets/postgres/test/postgres-namespace-schema-node.test.tspackages/3-targets/3-targets/postgres/test/postgres-table-schema-node.test.tspackages/3-targets/3-targets/sqlite/src/core/control-target.tspackages/3-targets/3-targets/sqlite/src/core/migrations/diff-database-schema.tspackages/3-targets/3-targets/sqlite/src/core/migrations/planner.tspackages/3-targets/3-targets/sqlite/src/core/migrations/runner.tspackages/3-targets/6-adapters/postgres/src/core/control-adapter.tspackages/3-targets/6-adapters/postgres/test/migrations/array-column-introspection.integration.test.tspackages/3-targets/6-adapters/postgres/test/migrations/rls-collect-extension-issues.test.tspackages/3-targets/6-adapters/postgres/test/migrations/rls-migration-plan.integration.test.tsprojects/postgres-rls/slices/schema-node-tree-restructure/design-diff-and-verify.mdtest/e2e/framework/test/sqlite/migrations/harness.tstest/integration/test/family.introspect.integration.test.tstest/integration/test/family.introspect.test.ts
💤 Files with no reviewable changes (4)
- packages/3-targets/3-targets/postgres/src/core/migrations/resolve-ddl-schema.ts
- packages/2-sql/9-family/src/exports/schema-verify.ts
- packages/2-sql/1-core/schema-ir/src/exports/types.ts
- packages/2-sql/1-core/schema-ir/src/ir/sql-schema-ir.ts
| projectSchemaToMember: (schema, ownedByOtherNames) => | ||
| familyInstance.projectSchemaToMember( | ||
| blindCast< | ||
| never, | ||
| 'family TSchemaIR is opaque to the CLI; schema is passed straight through' | ||
| >(schema), | ||
| ownedByOtherNames, | ||
| ), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Duplicate projectSchemaToMember wrapping across CLI operations.
The same blindCast-wrapped delegation to familyInstance.projectSchemaToMember appears here and in db-verify.ts (Lines 119-126). Consider extracting a small shared helper (e.g., in a common CLI control-api util) that both operations call, to avoid re-deriving the cast/comment each time.
🤖 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-run.ts`
around lines 188 - 195, The duplicated `projectSchemaToMember` delegation in
`db-run.ts` and `db-verify.ts` should be factored into a shared CLI helper
instead of repeating the same `blindCast` wrapper and comment. Extract a small
utility in a common control-api/CLI module that accepts the schema and
`ownedByOtherNames`, performs the `familyInstance.projectSchemaToMember` call,
and reuse it from both operations to keep the cast and opaque-schema
justification in one place.
| isEqualTo(other: DiffableNode): boolean { | ||
| return this.id === other.id; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
isEqualTo skips type validation, unlike PostgresRoleSchemaNode.
isEqualTo here only checks this.id === other.id with no nodeKind check, while PostgresRoleSchemaNode.isEqualTo validates the compared node's kind and throws on mismatch. Given children() only ever yields namespace nodes for this root, cross-type comparisons shouldn't currently occur, but the asymmetry could mask a bug if the tree shape changes later.
♻️ Optional consistency fix
isEqualTo(other: DiffableNode): boolean {
- return this.id === other.id;
+ const node = blindCast<SqlSchemaIRNode, 'diff-tree pairing guarantees a SqlSchemaIRNode'>(other);
+ return PostgresDatabaseSchemaNode.is(node) && this.id === node.id;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| isEqualTo(other: DiffableNode): boolean { | |
| return this.id === other.id; | |
| } | |
| isEqualTo(other: DiffableNode): boolean { | |
| const node = blindCast<SqlSchemaIRNode, 'diff-tree pairing guarantees a SqlSchemaIRNode'>(other); | |
| return PostgresDatabaseSchemaNode.is(node) && this.id === node.id; | |
| } |
🤖 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/3-targets/3-targets/postgres/src/core/schema-ir/postgres-database-schema-node.ts`
around lines 68 - 70, Update PostgresDatabaseSchemaNode.isEqualTo to match the
stricter comparison behavior used by PostgresRoleSchemaNode: validate that the
other DiffableNode is the expected node kind before comparing ids, and throw or
otherwise fail on mismatched kinds instead of returning a false positive. Keep
the comparison logic within isEqualTo consistent with the nodeKind-based guard
already used elsewhere so future tree-shape changes do not hide cross-type
comparisons.
| isEqualTo(other: DiffableNode): boolean { | ||
| return this.id === other.id; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Same isEqualTo type-validation gap as PostgresDatabaseSchemaNode.
Compares only id without confirming other is actually a PostgresNamespaceSchemaNode, unlike PostgresRoleSchemaNode.isEqualTo. Low risk today since children() on the database root only yields namespace nodes, but flagging for consistency with the stricter pattern used elsewhere.
🤖 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/3-targets/3-targets/postgres/src/core/schema-ir/postgres-namespace-schema-node.ts`
around lines 54 - 56, The `isEqualTo` implementation on
`PostgresNamespaceSchemaNode` only compares `id` and does not verify that
`other` is the same node type. Update this method to first check that `other` is
a `PostgresNamespaceSchemaNode`, following the stricter pattern used by
`PostgresRoleSchemaNode.isEqualTo`, and only then compare identifiers.
| isEqualTo(other: DiffableNode): boolean { | ||
| const node = blindCast< | ||
| SqlSchemaIRNode, | ||
| 'every diff-tree node the differ pairs is a SqlSchemaIRNode; the guard rejects non-policy kinds' | ||
| >(other); | ||
| if (!PostgresPolicySchemaNode.is(node)) { | ||
| throw new Error( | ||
| `PostgresPolicySchemaNode.isEqualTo: expected a PostgresPolicySchemaNode, got nodeKind=${node.nodeKind ?? 'undefined'}`, | ||
| ); | ||
| } | ||
| return this.id === node.id; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Kind-check in isEqualTo is inconsistent with sibling nodes.
This is the only node (table/namespace/database don't) that validates other.nodeKind before comparing by id, throwing if mismatched. Sibling isEqualTo implementations do a bare this.id === other.id. Given the tree only ever pairs same-kind nodes structurally, this extra check is likely unreachable in practice, but the asymmetry is worth aligning for consistency (either add the same guard everywhere, or drop it here).
🤖 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/3-targets/3-targets/postgres/src/core/schema-ir/postgres-policy-schema-node.ts`
around lines 73 - 84, The PostgresPolicySchemaNode.isEqualTo implementation is
the only sibling that performs an explicit kind check before comparing ids,
making it inconsistent with the other schema nodes. Align it with the other
isEqualTo methods in the same diff by either removing the
PostgresPolicySchemaNode.is(node) guard and relying on the id comparison like
the table/namespace/database nodes, or applying the same kind validation pattern
consistently across all sibling nodes.
wmadden
left a comment
There was a problem hiding this comment.
Better but still some work to do
Rework the diff/verify design to the settled shape from Will's second review: the differ is an SPI returning a SchemaDiff result (two issue lists + one filter over the union); verify and plan are symmetric (diff -> filter to contract space -> iterate); the framework filters issues by ownership and never prunes the schema; root/counts are verifier presentation, off the diff. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…rifyDatabaseSchemaResult Adds SchemaDiff (two issue lists + filter) and the SchemaDiffer interface to framework-components/control, next to SchemaIssue/SchemaDiffIssue. Retypes diffDatabaseSchema on the SQL target descriptor to return SchemaDiff. Postgres and SQLite factor their existing relational-walk + policy-diff computation into a private compute helper so the walk still runs once per caller; diffDatabaseSchema projects it to the two issue lists, and a new verifyDatabaseSchema descriptor field wraps the same computation in the verify envelope (ok/summary/code/target/timings) plus the pass/warn/fail tree the CLI renders. verifySchema in the family now calls verifyDatabaseSchema instead of diffDatabaseSchema so it still gets the tree without re-running the walk. Updates the planner and RLS drift tests to read .issues/.schemaDiffIssues off the SchemaDiff result directly. Behaviour-neutral: fixtures:check, the full test suites, and typecheck are all green with no diff. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…hema-pruning layer The framework no longer prunes the introspected schema before diffing. It diffs the full schema per contract-space member and scopes the resulting findings to the member: dropping the `extra` findings for entities another member claims, keyed by entity name (the coordinate the pruning layer keyed on). Extras owned by no member survive as each member undeclared tables. New framework-level helper `scopeSchemaResultToSpace` filters a `VerifyDatabaseSchemaResult` over framework types only (SchemaIssue / SchemaDiffIssue / SchemaVerificationNode) — it reads no storage shape and branches on no family. The aggregate verifier verifies each member against the full schema then scopes the result; the Mongo runner does the same via a `scopeVerifyResult` hook (replacing its input-pruning `projectSchema` hook). The plan path keeps sibling tables out of the ops the same way: synth passes the full schema plus every other member entity names as `entitiesOwnedByOtherSpaces`, and each SQL planner runs `scopePlanDiffToSpace` over its `SchemaDiff` (dropping the `extra` findings) before building ops, so no DROP is emitted for a sibling space table. Deleted: `project-schema-to-space.ts`, both family `schema-shape.ts` modules, the `projectSchemaToMember` / `listSchemaEntityNames` family callbacks + the framework interface methods + their CLI wiring, the `TSchemaResult` generic on the aggregate verifier, and the now-unused `orphanElements` / `detectOrphanElements`. Behaviour-neutral: fixtures:check clean (planner ops byte-identical), the cross-namespace-fk / supabase classification + cross-contract-fk / multi-namespace-runtime guards green, lint:deps clean (no framework storage-shape branch). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
… a member column `scopeSchemaResultToSpace` filtered the verification tree at every level by the last segment of each node contractPath. Because a contract column node (and a `storage.types` enum node) carries the column/type name as that last segment, a member own column named identically to another space table was dropped from the tree. The verdict is recomputed from the pruned tree (`ok = counts.fail === 0`) and `db verify` fails only on `!ok` — so dropping a *failing* column node named like a sibling table flipped the member to `ok`, passing verify while the real `missing_column` / type-mismatch issue was still in `issues`. A silent false-pass (same hazard on Mongo via the shared helper). Prune only `root.children` (the top-level table nodes); each surviving table keeps its full subtree — the top-level-only semantics of the pruning layer this replaced. Counts are now derived by subtracting the dropped top-level subtrees from the authoritative incoming counts (plus a root-status-flip reconciliation) rather than recomputed from the root, so a multi-schema result — whose retained root is first-namespace-only while its counts are summed across namespaces — keeps its authoritative counts unchanged when nothing is dropped. Regression test added first (a passing and a failing column both named like a sibling table survive scoping, `ok` stays false); it fails on the recursive version and passes on the fix. Gate: migration-tools 552 + framework-components 441 green; the four guards green; fixtures:check clean; lint:deps clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…y tree `pruneTopLevelTables` keyed the drop on the last `contractPath` segment of any root child. The root also carries the synthesized `storageTypes` verify-node (`kind: storageTypes`, `name: types`, `contractPath: storage.types`), so a sibling space owning a table literally named `types` dropped that node — and if it was a failing enum-drift node, `counts.fail` fell, `ok` flipped true, and `db verify` passed while the enum issue stayed in `issues`. The same false-pass class as the column case, one node over; the deleted pruning layer never touched the synthesized enum node (it pruned input-schema tables). Close the class by allowlisting the droppable child: only a top-level entity node — a SQL `table` or a Mongo `collection` — may be dropped. Any other root child (the `storageTypes` node now, and anything added later) is never droppable, regardless of name. Confirmed the enum node kind is `storageTypes`, not `table`, and that Mongo top-level nodes are `collection` (so scoping still drops sibling collections). Regression test added first (a failing `storageTypes` node scoped against an owned set containing `types` survives, `ok` stays false); it fails on the name-only filter and passes on the allowlist. A companion test locks that a Mongo `collection` a sibling claims is still dropped. Gate: migration-tools 554 + framework-components 441 + mongo-target 420 green; the four guards green; fixtures:check clean; lint:deps clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…d dead policy guards Brings every schema-diff node to the §8/§9 guard standard. All five `…SchemaNode` classes (database, namespace, table, policy, role) now expose static `is(node: SqlSchemaIRNode)` and `assert(node)` that discriminate on the node own `nodeKind` — no `instanceof`, no `unknown`, no `DiffableNode` parameter. `namespace`, `table`, and `role` gain the `assert` they were missing; `policy` `assert` message is aligned. The policy/role `isEqualTo` now delegate to `assert()` instead of inlining the kind check (behaviour unchanged — still throws on a mismatched kind). Deletes `PostgresDatabaseSchemaNode.ensure()`. It existed to reconstruct a node from the plain object `projectSchemaToSpace` spread produced; V2 deleted that spread, so every call site already holds a real, narrowed instance. The five call sites now use the value in place (after the existing `is`/`assert`), and the node constructors drop the now-dead plain-object reconstruction of their child nodes (their `*Input` unions narrow to instances). Column/FK/index/PK reconstruction from plain field data stays — that is live JSON-input handling. `StorageTable.is` / `.assert` become static methods with the parameter un-widened from `unknown` to `StorageTable | undefined` (the exact type every call site already holds); the free functions and their re-exports are removed and the five call sites updated. The dead `isPostgresRlsPolicy` / `assertPostgresRlsPolicy` free guards (no production callers) are deleted with their re-exports and tests. The two bare `as MongoContract` casts in the Mongo target descriptor become `blindCast`. Behaviour-neutral: guard results and diff/verify verdicts unchanged. fixtures:check clean; lint:deps clean; lint:casts improved (delta -10). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…ip keying V2 keys contract-space ownership on bare entity name (matching the pruning layer it replaces, so behaviour-neutral); extra_table issues carry no namespaceId, so qualified (namespaceId, name) keying is a follow-on tied to the relational port. Update design §11/§13 to the accurate interim. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…ead family method File organisation and dead-code cleanup; behaviour-neutral (fixtures clean). - Fold `diff-postgres-schema.ts` into `diff-database-schema.ts`. The policy node-diff (`diffPostgresSchema` + `filterIssuesByOwnership`) was only ever an internal step of the combined comparison, with no external consumer — it now lives beside the differ it serves. Deletes the separate file and the unused `exports/planner` re-exports; the unit test is renamed to match the surviving module and repointed. - Extract the diff-SPI types out of the catch-all `migrations/types.ts` into a named `schema-differ.ts`: `DiffDatabaseSchemaInput` plus `SqlDiffDatabaseSchema` / `SqlVerifyDatabaseSchema` aliases for the two descriptor fields. `types.ts` and `control-instance.ts` import from there. - Delete the dead `bootstrapSignMarkerQueries` method (and its interface member) on the SQL family instance — nothing calls `family.bootstrapSignMarkerQueries()`; `sign()` calls the adapter method directly, which stays. Findings reported (no merge/delete — kept and clarified): - `contractToPostgresDatabaseSchemaNode` is NOT a duplicate of `contractToSchemaIR`: it builds the Postgres *tree* (multi-schema, RLS-policy- and role-aware) and reuses the family per-namespace table conversion, whereas `contractToSchemaIR` builds the flat single-map (SQLite). A clarifying note now says so. - `verifyPostgresNamespacePresence` is LIVE — the planner uses it to emit `CREATE SCHEMA` for multi-schema plans. A note now records where it is used. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…ient planning IDs The review-flagged mechanical cleanups, all behaviour-neutral (fixtures clean). - Planner: rewrite the `relationalNamespaceNode` doc in plain English; trim the `relationalSchema` and `collectSchemaIssues` comments; drop the transient planning IDs (`CF-1`, `CF-2`) baked into comments; clarify the `asSchemaNode` bridge comment; clarify why `policyNodeToContractPolicy` reconstructs the contract entity rather than looking it up (see below). - `postgres-database-schema-node.ts`: "the real root" → "the root"; drop the `nodeKind` explanation (it belongs to the base class) and the `R4` slice id. - `printer-config.ts`: remove the orphan file-level doc comment. - `resolve-ddl-schema.ts`: reword the doc to say what the function takes and returns (the "namespace storage" phrasing was unclear). - `runner.ts`: trim the verbose app-space-verify comment. - `infer-psl-contract.ts`: reword the flatten stopgap to state it converts the schema-IR *tree* into the flat map the PSL writer walks, cite the follow-up TML-2958 (extend the writer to walk the tree), and drop the `§ A9` spec id. - `sql-schema-diff.ts`: replace the hand-rolled `if (!StorageTable.is(x)) throw` with `StorageTable.assert(x, coordinate)` — the §14 assertion-helper item. `policyNodeToContractPolicy` is load-bearing, NOT replaced with a lookup: the diff node carries the resolved DDL-schema `namespaceId`, which the emitted `createRlsPolicy` op serializes byte-for-byte; the contract-stored entity holds the raw pre-resolution coordinate, so a lookup would change the migration output. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
verify-postgres-namespaces is live (drives multi-schema CREATE SCHEMA planning), and contract-to-postgres-database-schema-node is not a duplicate of contractToSchemaIR (tree vs flat) — both stay; only the dead family bootstrapSignMarkerQueries was removed. Consolidation + type-extraction recorded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…o cross-space verify The DoD gate surfaced a real regression: the Mongo contract-space aggregate verify (`test/mongo/aggregate-e2e.test.ts`, `runner.test.ts`) failed deterministically — a spaces per-space verify returned `ok: false` when it should pass. Root cause: `countsAfterDrop` (added in the V2 verify-tree scoping) adjusted the counts for the root status flip on the assumption that the family counts the root node at its own status — true for SQL (`computeCounts` walks every node), false for Mongo (`diffMongoSchemas` tallies `fail++` per collection and never counts the root). So dropping a sibling `extra` collection subtracted its `fail` once for the subtree and again for the root flip, driving `counts.fail` to `-1`; `ok = counts.fail === 0` was then false and the verify wrongly failed. Fix: drop the family-specific root-flip arithmetic. When scoping drops nothing, keep the family authoritative counts/verdict unchanged (a multi-schema result keeps a first-namespace-only root but sums counts across namespaces, so recomputing from the root would undercount). When it drops a node, the pruned tree is self-consistent regardless of family, so recompute both counts and the verdict from a plain tree walk. No family branch, no negative counts. All V2-1/V2-2 unit tests still pass (migration-tools 554); the Mongo aggregate e2e + runner + codec-rehydration integration tests pass in isolation; the SQL scoping guards (supabase classification / cross-contract-fk) and framework-components stay green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/2-sql/9-family/src/core/control-instance.ts (2)
735-739: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRecompute
okafter filtering structural diff issues.When all
schemaDiffIssuesare suppressed, this branch can still return the original failedsqlResult, leaving verify failed with no surviving structural issues. Recompute the total failure count after filtering and setokfrom that value.Proposed fix
const schemaDiffIssues = filterSchemaDiffIssues( sqlResult.schema.schemaDiffIssues, contract.defaultControlPolicy, ); const relationalFails = sqlResult.schema.counts.fail; - if (schemaDiffIssues.length === 0) { - if (schemaDiffIssues === sqlResult.schema.schemaDiffIssues) return sqlResult; - return { ...sqlResult, schema: { ...sqlResult.schema, schemaDiffIssues } }; - } const totalFails = relationalFails + schemaDiffIssues.length; + if (totalFails === 0) { + return { + ...sqlResult, + ok: true, + summary: 'Database schema satisfies contract', + schema: { + ...sqlResult.schema, + schemaDiffIssues, + counts: { ...sqlResult.schema.counts, fail: 0 }, + }, + }; + } return {🤖 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/2-sql/9-family/src/core/control-instance.ts` around lines 735 - 739, In control-instance.ts, the branch that returns sqlResult after filtering schemaDiffIssues can leave ok stale when all structural issues are removed. Update the logic around the schemaDiffIssues handling to recompute the failure count from the filtered issues and derive ok from that updated count before returning sqlResult, so verify doesn’t stay failed with no remaining structural issues.
960-967: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCarry namespace identity into schema-view table entries.
Flattening to
[tableName, table]still collides forpublic.usersandauth.users; child IDs also use onlytableName. Include the namespace in the flattened entry and derive IDs/labels from the qualified name.🤖 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/2-sql/9-family/src/core/control-instance.ts` around lines 960 - 967, The schema-view table flattening in control-instance currently drops namespace identity, so tables with the same name in different namespaces can collide. Update the table entry निर्माण in the schema tree logic to carry the namespace alongside each table when building tableEntries from root.namespaces/root.tables, and then use that qualified namespace+table name in the tableNodes mapping for IDs and labels. Make sure the relevant schema-tree construction in control-instance distinguishes entries by fully qualified name instead of only tableName.
🧹 Nitpick comments (2)
packages/1-framework/3-tooling/migration/test/aggregate/scope-schema-result.test.ts (1)
172-184: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThis test won't catch a stale
codeleak.
resultWith()never populatescode, soexpect(scoped.code).toBeUndefined()passes trivially regardless of whetherscopeSchemaResultToSpaceactually clears a pre-existing failure code. Consider adding a case where the input result hascode: 'PN-RUN-3010'set alongsideok: false, and asserting it's cleared once scoping flipsoktotrue.🤖 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/test/aggregate/scope-schema-result.test.ts` around lines 172 - 184, The scopeSchemaResultToSpace test currently cannot detect a stale code being carried through when siblings are removed. Update the test around scopeSchemaResultToSpace and resultWith to use an input result with an existing failure code such as PN-RUN-3010 alongside ok: false, then verify that after scoping to the sibling space the result flips to ok: true and clears code to undefined while preserving the recomputed summary.packages/1-framework/3-tooling/migration/src/aggregate/scope-schema-result.ts (1)
83-106: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRoot status is recomputed even when nothing was dropped.
pruneTopLevelTablesalways recomputesroot.statusfromaggregateStatus(kept), even whendropped.length === 0. If the framework's real root-status computation ever depends on something beyond simple child aggregation (e.g. a namespace-level condition not represented as a top-level child), this could produce a spurious status flip on a true no-op call, whichcountsAfterDropwould then (incorrectly) adjust for since it only checksnewRootStatus !== oldRootStatus, not whether anything was actually dropped.Guarding the no-op case would remove this risk cheaply.
♻️ Proposed fix
return { - root: { ...root, status: aggregateStatus(kept), children: kept }, - dropped, + root: dropped.length === 0 ? root : { ...root, status: aggregateStatus(kept), children: kept }, + dropped, };🤖 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/aggregate/scope-schema-result.ts` around lines 83 - 106, In pruneTopLevelTables, avoid recomputing and replacing root.status when nothing was dropped. Keep the existing root status unchanged for the no-op path, and only derive a new status from aggregateStatus(kept) when dropped has entries. This prevents countsAfterDrop from seeing a status change on an unchanged tree and incorrectly adjusting counts based on nodeEntityName/isEntityNode handling.
🤖 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/migration/src/aggregate/scope-schema-result.ts`:
- Around line 56-61: The helper schemaDiffIssueEntityName currently uses a raw
TypeScript cast to read tableName from actual, which violates the no-bare-as
guideline. Update this logic to use the approved cast helper from
`@prisma-next/utils/casts`, such as castAs or blindCast with a clear reason, while
keeping the same undefined handling and string check in
schemaDiffIssueEntityName.
- Around line 189-195: The scoped result builder in scope-schema-result should
clear any stale failure code when `ok` flips to `true`, since the current spread
in the return object preserves `result.code` from the incoming value. Update the
return construction so `code` is explicitly removed or set to undefined/null
whenever `ok` is true, and only retain `result.code` (or the default
`PN-RUN-3010`) in the failing branch. Use the `ok` calculation and the final
object assembly in the same return path to ensure consumers of
`scopeSchemaResult` never see `ok: true` paired with a failure `code`.
In `@packages/2-sql/9-family/src/core/migrations/scope-plan-diff.ts`:
- Around line 1-12: The issue is the bare type assertion in issueEntityName,
where actual is cast directly before reading tableName. Replace that inline cast
with the approved helper (blindCast<T, "Reason"> or castAs<T>) and keep the
narrowing as tight as possible so the function still returns the table name only
when it is actually a string.
---
Duplicate comments:
In `@packages/2-sql/9-family/src/core/control-instance.ts`:
- Around line 735-739: In control-instance.ts, the branch that returns sqlResult
after filtering schemaDiffIssues can leave ok stale when all structural issues
are removed. Update the logic around the schemaDiffIssues handling to recompute
the failure count from the filtered issues and derive ok from that updated count
before returning sqlResult, so verify doesn’t stay failed with no remaining
structural issues.
- Around line 960-967: The schema-view table flattening in control-instance
currently drops namespace identity, so tables with the same name in different
namespaces can collide. Update the table entry निर्माण in the schema tree logic
to carry the namespace alongside each table when building tableEntries from
root.namespaces/root.tables, and then use that qualified namespace+table name in
the tableNodes mapping for IDs and labels. Make sure the relevant schema-tree
construction in control-instance distinguishes entries by fully qualified name
instead of only tableName.
---
Nitpick comments:
In
`@packages/1-framework/3-tooling/migration/src/aggregate/scope-schema-result.ts`:
- Around line 83-106: In pruneTopLevelTables, avoid recomputing and replacing
root.status when nothing was dropped. Keep the existing root status unchanged
for the no-op path, and only derive a new status from aggregateStatus(kept) when
dropped has entries. This prevents countsAfterDrop from seeing a status change
on an unchanged tree and incorrectly adjusting counts based on
nodeEntityName/isEntityNode handling.
In
`@packages/1-framework/3-tooling/migration/test/aggregate/scope-schema-result.test.ts`:
- Around line 172-184: The scopeSchemaResultToSpace test currently cannot detect
a stale code being carried through when siblings are removed. Update the test
around scopeSchemaResultToSpace and resultWith to use an input result with an
existing failure code such as PN-RUN-3010 alongside ok: false, then verify that
after scoping to the sibling space the result flips to ok: true and clears code
to undefined while preserving the recomputed summary.
🪄 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: 9e832c4f-7c0a-4ead-82cf-154f19624fca
⛔ Files ignored due to path filters (1)
projects/postgres-rls/slices/schema-node-tree-restructure/design-diff-and-verify.mdis excluded by!projects/**
📒 Files selected for processing (64)
packages/1-framework/1-core/framework-components/src/control/control-instances.tspackages/1-framework/1-core/framework-components/src/control/control-migration-types.tspackages/1-framework/1-core/framework-components/src/control/schema-diff.tspackages/1-framework/1-core/framework-components/src/exports/control.tspackages/1-framework/1-core/framework-components/test/schema-diff.test.tspackages/1-framework/3-tooling/cli/src/control-api/operations/db-verify.tspackages/1-framework/3-tooling/migration/src/aggregate/planner-types.tspackages/1-framework/3-tooling/migration/src/aggregate/project-schema-to-space.tspackages/1-framework/3-tooling/migration/src/aggregate/scope-schema-result.tspackages/1-framework/3-tooling/migration/src/aggregate/strategies/synth.tspackages/1-framework/3-tooling/migration/src/aggregate/verifier.tspackages/1-framework/3-tooling/migration/src/exports/aggregate.tspackages/1-framework/3-tooling/migration/test/aggregate/project-schema-to-space.test.tspackages/1-framework/3-tooling/migration/test/aggregate/scope-schema-result.test.tspackages/1-framework/3-tooling/migration/test/aggregate/strategies/synth.test.tspackages/1-framework/3-tooling/migration/test/aggregate/verifier.test.tspackages/1-framework/3-tooling/migration/test/deletable-node-modules.test.tspackages/2-sql/1-core/contract/src/exports/types.tspackages/2-sql/1-core/contract/src/ir/storage-table.tspackages/2-sql/1-core/contract/src/types.tspackages/2-sql/9-family/src/core/control-instance.tspackages/2-sql/9-family/src/core/diff/sql-schema-diff.tspackages/2-sql/9-family/src/core/migrations/contract-to-schema-ir.tspackages/2-sql/9-family/src/core/migrations/field-event-planner.tspackages/2-sql/9-family/src/core/migrations/schema-differ.tspackages/2-sql/9-family/src/core/migrations/scope-plan-diff.tspackages/2-sql/9-family/src/core/migrations/types.tspackages/2-sql/9-family/src/core/psl-contract-infer/printer-config.tspackages/2-sql/9-family/src/exports/control.tspackages/2-sql/9-family/src/exports/diff.tspackages/2-sql/9-family/test/control-instance.descriptor-self-consistency.test.tspackages/2-sql/9-family/test/cross-contract-validation.test.tspackages/2-sql/9-family/test/namespace-hydration.test.tspackages/2-sql/9-family/test/operation-preview.test.tspackages/2-sql/9-family/test/schema-verify.helpers.tspackages/2-sql/9-family/test/schema-view.test.tspackages/3-mongo-target/1-mongo-target/src/core/control-target.tspackages/3-mongo-target/1-mongo-target/src/core/mongo-runner.tspackages/3-targets/3-targets/postgres/src/core/migrations/contract-to-postgres-database-schema-node.tspackages/3-targets/3-targets/postgres/src/core/migrations/diff-database-schema.tspackages/3-targets/3-targets/postgres/src/core/migrations/diff-postgres-schema.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner.tspackages/3-targets/3-targets/postgres/src/core/migrations/resolve-ddl-schema.tspackages/3-targets/3-targets/postgres/src/core/migrations/runner.tspackages/3-targets/3-targets/postgres/src/core/migrations/verify-postgres-namespaces.tspackages/3-targets/3-targets/postgres/src/core/postgres-rls-policy.tspackages/3-targets/3-targets/postgres/src/core/psl-infer/infer-psl-contract.tspackages/3-targets/3-targets/postgres/src/core/schema-ir/postgres-database-schema-node.tspackages/3-targets/3-targets/postgres/src/core/schema-ir/postgres-namespace-schema-node.tspackages/3-targets/3-targets/postgres/src/core/schema-ir/postgres-policy-schema-node.tspackages/3-targets/3-targets/postgres/src/core/schema-ir/postgres-role-schema-node.tspackages/3-targets/3-targets/postgres/src/core/schema-ir/postgres-table-schema-node.tspackages/3-targets/3-targets/postgres/src/core/schema-ir/schema-node-kinds.tspackages/3-targets/3-targets/postgres/src/exports/control.tspackages/3-targets/3-targets/postgres/src/exports/planner.tspackages/3-targets/3-targets/postgres/src/exports/types.tspackages/3-targets/3-targets/postgres/test/migrations/diff-database-schema.test.tspackages/3-targets/3-targets/postgres/test/postgres-database-schema-node.test.tspackages/3-targets/3-targets/postgres/test/rls-diffable-nodes.test.tspackages/3-targets/3-targets/sqlite/src/core/control-target.tspackages/3-targets/3-targets/sqlite/src/core/migrations/diff-database-schema.tspackages/3-targets/3-targets/sqlite/src/core/migrations/planner.tspackages/3-targets/3-targets/sqlite/src/core/migrations/runner.tspackages/3-targets/6-adapters/postgres/test/migrations/rls-collect-extension-issues.test.ts
💤 Files with no reviewable changes (10)
- packages/3-targets/3-targets/postgres/src/exports/planner.ts
- packages/3-targets/3-targets/postgres/test/postgres-database-schema-node.test.ts
- packages/2-sql/9-family/src/exports/diff.ts
- packages/2-sql/1-core/contract/src/exports/types.ts
- packages/2-sql/9-family/src/core/psl-contract-infer/printer-config.ts
- packages/3-targets/3-targets/postgres/src/exports/types.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/diff-postgres-schema.ts
- packages/1-framework/3-tooling/migration/src/aggregate/project-schema-to-space.ts
- packages/3-targets/3-targets/postgres/src/core/postgres-rls-policy.ts
- packages/1-framework/3-tooling/migration/test/aggregate/project-schema-to-space.test.ts
✅ Files skipped from review due to trivial changes (5)
- packages/2-sql/9-family/test/operation-preview.test.ts
- packages/1-framework/1-core/framework-components/src/control/control-instances.ts
- packages/2-sql/9-family/src/core/migrations/schema-differ.ts
- packages/1-framework/3-tooling/migration/src/aggregate/planner-types.ts
- packages/1-framework/3-tooling/cli/src/control-api/operations/db-verify.ts
🚧 Files skipped from review as they are similar to previous changes (19)
- packages/3-targets/3-targets/postgres/src/core/schema-ir/schema-node-kinds.ts
- packages/2-sql/9-family/test/schema-view.test.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/verify-postgres-namespaces.ts
- packages/2-sql/9-family/test/cross-contract-validation.test.ts
- packages/2-sql/9-family/test/control-instance.descriptor-self-consistency.test.ts
- packages/3-targets/3-targets/postgres/src/core/schema-ir/postgres-role-schema-node.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/resolve-ddl-schema.ts
- packages/3-targets/3-targets/postgres/src/exports/control.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/contract-to-postgres-database-schema-node.ts
- packages/3-targets/3-targets/postgres/src/core/schema-ir/postgres-policy-schema-node.ts
- packages/2-sql/9-family/src/core/migrations/contract-to-schema-ir.ts
- packages/3-targets/3-targets/postgres/src/core/psl-infer/infer-psl-contract.ts
- packages/3-targets/3-targets/sqlite/src/core/migrations/runner.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/runner.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/planner.ts
- packages/2-sql/9-family/src/core/diff/sql-schema-diff.ts
- packages/3-targets/6-adapters/postgres/test/migrations/rls-collect-extension-issues.test.ts
- packages/3-targets/3-targets/sqlite/src/core/control-target.ts
- packages/3-targets/3-targets/sqlite/src/core/migrations/planner.ts
| function schemaDiffIssueEntityName(issue: SchemaDiffIssue): string | undefined { | ||
| const actual = issue.actual; | ||
| if (actual === undefined) return undefined; | ||
| const name = (actual as { readonly tableName?: unknown }).tableName; | ||
| return typeof name === 'string' ? name : undefined; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Bare as cast in production code.
(actual as { readonly tableName?: unknown }).tableName is a raw as cast. As per coding guidelines: "No bare as in production code. Use blindCast<T, \"Reason\"> or castAs<T> from @prisma-next/utils/casts... as const and test files are exempt."
♻️ Proposed fix
- const name = (actual as { readonly tableName?: unknown }).tableName;
+ const name = castAs<{ readonly tableName?: unknown }>(actual).tableName;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function schemaDiffIssueEntityName(issue: SchemaDiffIssue): string | undefined { | |
| const actual = issue.actual; | |
| if (actual === undefined) return undefined; | |
| const name = (actual as { readonly tableName?: unknown }).tableName; | |
| return typeof name === 'string' ? name : undefined; | |
| } | |
| function schemaDiffIssueEntityName(issue: SchemaDiffIssue): string | undefined { | |
| const actual = issue.actual; | |
| if (actual === undefined) return undefined; | |
| const name = castAs<{ readonly tableName?: unknown }>(actual).tableName; | |
| return typeof name === 'string' ? name : undefined; | |
| } |
🤖 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/aggregate/scope-schema-result.ts`
around lines 56 - 61, The helper schemaDiffIssueEntityName currently uses a raw
TypeScript cast to read tableName from actual, which violates the no-bare-as
guideline. Update this logic to use the approved cast helper from
`@prisma-next/utils/casts`, such as castAs or blindCast with a clear reason, while
keeping the same undefined handling and string check in
schemaDiffIssueEntityName.
Source: Coding guidelines
| return { | ||
| ...result, | ||
| ok, | ||
| ...(ok ? {} : { code: result.code ?? 'PN-RUN-3010' }), | ||
| summary: ok ? 'Database schema satisfies contract' : result.summary, | ||
| schema: { issues, schemaDiffIssues, root, counts }, | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stale failure code can survive a flip to ok: true.
When ok becomes true (all failures were sibling-owned), the spread order leaves result.code untouched — it's only ever set (not cleared) when !ok. If the incoming result had code set (typical when the pre-scope result was ok: false), the scoped result ends up ok: true with a stale failure code, which is an inconsistent state that any consumer branching on code would misinterpret.
🐛 Proposed fix
+ const { code: _staleCode, ...resultWithoutCode } = result;
return {
- ...result,
+ ...resultWithoutCode,
ok,
...(ok ? {} : { code: result.code ?? 'PN-RUN-3010' }),
summary: ok ? 'Database schema satisfies contract' : result.summary,
schema: { issues, schemaDiffIssues, root, counts },
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return { | |
| ...result, | |
| ok, | |
| ...(ok ? {} : { code: result.code ?? 'PN-RUN-3010' }), | |
| summary: ok ? 'Database schema satisfies contract' : result.summary, | |
| schema: { issues, schemaDiffIssues, root, counts }, | |
| }; | |
| const { code: _staleCode, ...resultWithoutCode } = result; | |
| return { | |
| ...resultWithoutCode, | |
| ok, | |
| ...(ok ? {} : { code: result.code ?? 'PN-RUN-3010' }), | |
| summary: ok ? 'Database schema satisfies contract' : result.summary, | |
| schema: { issues, schemaDiffIssues, root, counts }, | |
| }; |
🤖 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/aggregate/scope-schema-result.ts`
around lines 189 - 195, The scoped result builder in scope-schema-result should
clear any stale failure code when `ok` flips to `true`, since the current spread
in the return object preserves `result.code` from the incoming value. Update the
return construction so `code` is explicitly removed or set to undefined/null
whenever `ok` is true, and only retain `result.code` (or the default
`PN-RUN-3010`) in the failing branch. Use the `ok` calculation and the final
object assembly in the same return path to ensure consumers of
`scopeSchemaResult` never see `ok: true` paired with a failure `code`.
| import type { DiffIssue, SchemaDiff } from '@prisma-next/framework-components/control'; | ||
|
|
||
| /** The entity name a diff issue addresses, for ownership scoping. */ | ||
| function issueEntityName(issue: DiffIssue): string | undefined { | ||
| if ('outcome' in issue) { | ||
| const actual = issue.actual; | ||
| if (actual === undefined) return undefined; | ||
| const tableName = (actual as { readonly tableName?: unknown }).tableName; | ||
| return typeof tableName === 'string' ? tableName : undefined; | ||
| } | ||
| return 'table' in issue ? issue.table : undefined; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Bare as cast in production code.
(actual as { readonly tableName?: unknown }).tableName is a bare cast. Per coding guidelines, production code must use blindCast<T, "Reason"> (or castAs<T>) instead, narrowed as far as possible.
♻️ Proposed fix
+import { blindCast } from '`@prisma-next/utils/casts`';
+
function issueEntityName(issue: DiffIssue): string | undefined {
if ('outcome' in issue) {
const actual = issue.actual;
if (actual === undefined) return undefined;
- const tableName = (actual as { readonly tableName?: unknown }).tableName;
+ const tableName = blindCast<
+ { readonly tableName?: unknown },
+ 'DiffableNode may expose tableName for table-shaped nodes'
+ >(actual).tableName;
return typeof tableName === 'string' ? tableName : undefined;
}
return 'table' in issue ? issue.table : undefined;
}As per coding guidelines, "No bare as in production code. Use blindCast<T, "Reason"> or castAs<T>".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import type { DiffIssue, SchemaDiff } from '@prisma-next/framework-components/control'; | |
| /** The entity name a diff issue addresses, for ownership scoping. */ | |
| function issueEntityName(issue: DiffIssue): string | undefined { | |
| if ('outcome' in issue) { | |
| const actual = issue.actual; | |
| if (actual === undefined) return undefined; | |
| const tableName = (actual as { readonly tableName?: unknown }).tableName; | |
| return typeof tableName === 'string' ? tableName : undefined; | |
| } | |
| return 'table' in issue ? issue.table : undefined; | |
| } | |
| import type { DiffIssue, SchemaDiff } from '`@prisma-next/framework-components/control`'; | |
| import { blindCast } from '`@prisma-next/utils/casts`'; | |
| /** The entity name a diff issue addresses, for ownership scoping. */ | |
| function issueEntityName(issue: DiffIssue): string | undefined { | |
| if ('outcome' in issue) { | |
| const actual = issue.actual; | |
| if (actual === undefined) return undefined; | |
| const tableName = blindCast< | |
| { readonly tableName?: unknown }, | |
| 'DiffableNode may expose tableName for table-shaped nodes' | |
| >(actual).tableName; | |
| return typeof tableName === 'string' ? tableName : undefined; | |
| } | |
| return 'table' in issue ? issue.table : undefined; | |
| } |
🤖 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/2-sql/9-family/src/core/migrations/scope-plan-diff.ts` around lines
1 - 12, The issue is the bare type assertion in issueEntityName, where actual is
cast directly before reading tableName. Replace that inline cast with the
approved helper (blindCast<T, "Reason"> or castAs<T>) and keep the narrowing as
tight as possible so the function still returns the table name only when it is
actually a string.
Source: Coding guidelines
… issue-based verify (round 3) Will's partial review rejected V2's cross-space machinery (scope-schema-result, entitiesOwnedByOtherSpaces) as over-built. Rewrite the model: the contract-space aggregate is passive (answers ownership); the orchestration owns the verbs (runs the differ per space, composes the view, classifies extras, hands the planner its issues); verify's verdict is the space's issue list being empty; issues are node-typed (coupled to schema IR, not the contract IR). scope-schema-result and the cross-space plumbing delete. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
… casts Round 3 R1. Additive, non-breaking, behaviour-neutral (pure typing + comments). Adds a node type parameter to the diff-issue types, defaulted to `DiffableNode` so every existing use keeps compiling unchanged: - `SchemaDiffIssue<TNode extends DiffableNode = DiffableNode>` — `expected` / `actual` become `TNode`. - `SchemaDiff<TNode>` — `schemaDiffIssues: SchemaDiffIssue<TNode>[]`, and `filter(keep: (issue: DiffIssue<TNode>) => boolean): SchemaDiff<TNode>`. - `DiffIssue<TNode> = SchemaIssue | SchemaDiffIssue<TNode>`. The Postgres differ now returns the concrete type. `SqlSchemaIRNode` alone is not a `DiffableNode` (its relational subclasses carry no `id`/`isEqualTo`/ `children`), so the honest node type is `SqlSchemaDiffNode = SqlSchemaIRNode & DiffableNode` — the five `Postgres*SchemaNode` classes. `diffPostgresSchema` narrows the framework `SchemaDiffIssue<DiffableNode>` output ONCE at the diff boundary (a single justified `blindCast`, since both trees are `PostgresDatabaseSchemaNode`s), so `diffPostgresDatabaseSchema` is `SchemaDiff<SqlSchemaDiffNode>`. `scopePlanDiffToSpace` is generic over `TNode` so the type survives to the planner, which drops its per-issue `asSchemaNode` blindCast (and the helper) and reads the node directly. Two comment fixes: the empty `DiffIssue` comment now names the two issue representations; the `db-verify.ts` per-member verifier drops its `never` cast (the family instance is `ControlFamilyInstance<_, unknown>`, so `schema` already types as `unknown` — it passes straight through). Existing callers (framework `diffSchemas`, Mongo, SQLite, migration-tools, tests) are unbroken via the default `= DiffableNode` — all typecheck. `lint:casts` improves (per-issue casts → one boundary cast): delta -11. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…ion + unclaimed elements Will's resolution to the R2 tripwire: the verifier presents two distinct pieces of information — (1) per contract space, is the contract satisfied (declared elements, missing/mismatch); (2) a standalone list of live schema elements unclaimed by any contract. An unclaimed element is never forced into a contract's structure, and is reported once (fixing the N-times-per-space duplication). Also reconcile the node type to SqlSchemaDiffNode (SqlSchemaIRNode & DiffableNode). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…layer The single-space family differ (verifySqlSchema / diffMongoSchemas) is shared with the migration planner and the runner's post-apply verify (which reads its ok = counts.fail); it stays unchanged. The two-part output — Part 1 per-space contract satisfaction, Part 2 the unclaimed-elements list — is an aggregate concern, so verifyMigration (replacing scope-schema-result) strips each per-space result's extras to Part 1 and gathers them into the unclaimed list. Planner and runner single-space paths stay byte-identical. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…e unclaimed list The aggregate verifier now produces two distinct outputs instead of grafting every undeclared live element into each contract space's tree N times. Part 1 — per contract space, its contract-satisfaction view: the space's declared nodes only, each pass/fail by a missing/mismatch issue. Part 2 — one deduplicated list of live elements no contract space declares, reported once, built from the diffs' extra findings filtered by a new passive-aggregate ownership query (declaresEntity). The split lives entirely in the aggregate layer (verifyMigration). The family differ (verifySqlSchema / diffMongoSchemas) is untouched: it still grafts extra nodes so the single-space verdict the migration planner and the runner's post-apply verify depend on stays byte-identical. verifyMigration strips those extras from each per-space result (subtracting their tally from the family's authoritative counts, so Mongo's no-root-count basis is preserved) and gathers them into the unclaimed list. CLI: db verify threads the unclaimed list as a top-level field in --json / --schema-only, rendered once. Strict mode fails on a non-empty list; lenient shows it informationally. scope-schema-result.ts is retained: the plan path (synth) and the Mongo runner still consume it; the verifier no longer does. Also folds in the R1 comment correction in scope-plan-diff. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…s from the pruned tree Two verdict bugs in the Part-1 strip (stripExtraFindings): Counts basis: subtracting a stripped extra node's tally from the family's authoritative counts left SQL's root contribution stale — SQL counts the root at its own status, so a strict space whose only failures were extras kept fail=1 from the root and false-failed with an empty issue list (every clean space in a multi-space strict verify). Reuse the proven shape: when nothing was stripped, keep the authoritative counts; when a node was stripped, the pruned tree is self-consistent in both count bases, so recompute counts and verdict from a plain tree walk. Fixture now counts the root at its own status; both bases (SQL root-counted, Mongo root-not-counted) are pinned by tests. Strip scope: the strip removed ALL extra_* issues and all extra schemaDiffIssues, including an extra column on the space's own declared table and extra RLS policies — whose contributions remain in the tree/counts. A space could fail with no visible evidence, unreachable in Part 2 too (the ownership query filters declared-table names). The strip is now scoped to top-level entity extras only (extra_table issues + the grafted entity nodes); nested extras and policy schemaDiffIssues stay in Part 1 as the space's own drift. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…rdict The dropped-branch recompute in stripExtraFindings derived the verdict from a tree walk alone. But the family folds schemaDiffIssues.length into counts.fail after its own walk (control-instance totalFails), and policy issues carry no tree node - so a space that was relationally clean but had an extra RLS policy on its own table flipped ok:true whenever a sibling extra node was dropped. db verify --strict would false-pass with live policy drift. Re-fold: after countTree, add schemaDiffIssues.length to counts.fail before deriving ok, matching how the family built the counts being replaced. The composed case (policy schemaDiffIssue + dropped sibling extra) is pinned red- first: after the strip, ok stays false and the schemaDiffIssue stays in Part 1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…oss-space plumbing The planner no longer works out contract-space ownership. Its plan() input swaps the entitiesOwnedByOtherSpaces name-set for an optional keepDiffIssue predicate, applied blindly to its schema diff via SchemaDiff.filter before building ops. The orchestration (synthStrategy) constructs the predicate over the passive aggregate's new declaringSpaces ownership query: drop the extra findings for elements a sibling contract space declares, keep everything else (including extras no space declares, which the planner may DROP under a destructive policy). scopePlanDiffToSpace and its export are deleted; the postgres and sqlite planners hold no scoping logic. The orchestration does not run the diff itself: the differ lives on the SQL family target descriptor, invisible to the framework strategy, its strictness derives from the planner's policy, and plan() needs the schema regardless for the strategy layer's existence probes — so the faithful seam is the predicate, not a pre-computed diff. scope-schema-result.ts (scopeSchemaResultToSpace / otherMemberEntityNames) is deleted. Its last consumer, the Mongo runner's per-space post-apply verify, is rewired to a mongo-target-local equivalent (scope-verify-result.ts) with identical verdicts: drop the extra findings for collections a sibling space claims, keep truly undeclared extras so genuine drift still fails; counts stay authoritative when nothing is dropped and are recomputed from the pruned tree otherwise. Mongo runner results carry no schemaDiffIssues, so no re-fold. Planner ops are byte-identical (fixtures:check clean) and the multi-space guards stay green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
… follow-ons R3 landed the plan-side scoping as a blind keep-predicate injected by the orchestration (the differ is target-descriptor-only, the diff recipe is planner-internal, and plan() consumes the schema for existence probes) — update §6 and record the rejected pre-scoped-issues shape in §15. Add the multi-namespace verify-tree false-fail residual to §13. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…re "member" and "schema result" Every identifier, parameter, comment, and user-facing string that used "member" for a contract space now says space: AggregateContractSpace (ContractSpaceMember; the bare ContractSpace name is taken by the framework descriptor SPI type), createAggregateContractSpace, verifySchemaForSpace, planSpacePath / SpacePathOutcome, spaceOrder, createPerSpaceVerifier, and the locals/prose across the aggregate, its strategies, the CLI wiring, and tests. "member" survives only where it is not contract-space vocabulary (graph membership, TypeScript class/AST members, enum members). The loose "schema result(s)" term is retired: combineVerifyResults / CombinedVerifyResult (file renamed to combine-verify-results.ts), and prose now says per-space verify results. The schemaResults field stays - it directly mirrors the framework type VerifyDatabaseSchemaResult and is consumed by example apps and guard tests. Stale comments describing pre-collapse behaviour are rewritten: the planner no longer "scopes the resulting diff" itself (it applies the orchestration- built keep-predicate), and SqlSchemaIRNode.nodeKind no longer cites the deleted .ensure guard or projectSchemaToSpace spread. Names and comments only; no behaviour change (planner ops byte-identical). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/db-verify.ts`:
- Around line 545-556: Schema-only verification can exit with no diagnostics
when `--quiet` is set because `handleResult`’s callback in `db-verify.ts` passes
`flags` straight into `formatSchemaVerifyOutput`, which suppresses output for
quiet mode. Update the schema-only success path to mirror the full-mode failure
branch by forcing a non-quiet flags object before calling
`formatSchemaVerifyOutput`, using the existing `flags`, `handleResult`, and
`formatSchemaVerifyOutput` flow so `--schema-only --quiet` still prints the
failure details.
In `@packages/1-framework/3-tooling/cli/src/utils/combine-verify-results.ts`:
- Around line 73-81: The summary logic in combineVerifyResults uses a bare cast
on firstFailure, which violates the production casting guideline. Replace the
`(firstFailure as VerifyDatabaseSchemaResult).summary` access with a safe cast
helper from `@prisma-next/utils/casts`, using blindCast or castAs with a short
reason that documents the okAll/appResult.ok invariant. Keep the existing
branching in combineVerifyResults unchanged and apply the cast only where
firstFailure is read.
In `@packages/1-framework/3-tooling/migration/src/aggregate/aggregate.ts`:
- Around line 241-263: The namespace collection logic in
collectAggregateNamespaces currently overwrites same-key entries with
last-write-wins, which can drop earlier tables from shared namespaceIds. Update
the aggregation in collectAggregateNamespaces to merge namespace contents per
key instead of assigning merged[key] = ns, or explicitly detect and reject
incompatible namespace shapes before building the object passed to
familyInstance.introspect. Keep the fix localized to collectAggregateNamespaces
and its merged namespace accumulation.
In
`@packages/1-framework/3-tooling/migration/src/aggregate/unclaimed-elements.ts`:
- Around line 30-36: The helper schemaDiffIssueEntityName still uses a bare cast
to read tableName from issue.actual, which violates the production casting
guideline. Update this function to match the pattern used by issueEntityName in
strategies/synth.ts by replacing the inline as cast with blindCast from
`@prisma-next/utils/casts`, while keeping the same undefined and string checks for
the extracted name.
In `@packages/3-mongo-target/1-mongo-target/src/core/scope-verify-result.ts`:
- Around line 61-80: The countTree helper is currently including the synthetic
root in its pass/warn/fail/totalNodes totals, which makes the recomputed
verification counts off by one. Update countTree in scope-verify-result.ts to
traverse and count only node.children from the provided root instead of visiting
the root itself, while keeping the existing counting logic for
SchemaVerificationNode statuses unchanged.
🪄 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: eed8a146-92f6-4958-9a3a-d6082bf60d11
⛔ Files ignored due to path filters (1)
projects/postgres-rls/slices/schema-node-tree-restructure/design-diff-and-verify.mdis excluded by!projects/**
📒 Files selected for processing (63)
docs/architecture docs/subsystems/7. Migration System.mdpackages/1-framework/1-core/framework-components/src/control/control-migration-types.tspackages/1-framework/1-core/framework-components/src/control/schema-diff.tspackages/1-framework/3-tooling/cli/src/commands/db-verify.tspackages/1-framework/3-tooling/cli/src/commands/migrate.tspackages/1-framework/3-tooling/cli/src/commands/migration-check.tspackages/1-framework/3-tooling/cli/src/commands/migration-graph.tspackages/1-framework/3-tooling/cli/src/commands/migration-list.tspackages/1-framework/3-tooling/cli/src/commands/migration-plan.tspackages/1-framework/3-tooling/cli/src/commands/migration-status.tspackages/1-framework/3-tooling/cli/src/control-api/operations/db-init.tspackages/1-framework/3-tooling/cli/src/control-api/operations/db-run.tspackages/1-framework/3-tooling/cli/src/control-api/operations/db-verify.tspackages/1-framework/3-tooling/cli/src/control-api/operations/migrate.tspackages/1-framework/3-tooling/cli/src/control-api/operations/run-migration.tspackages/1-framework/3-tooling/cli/src/control-api/types.tspackages/1-framework/3-tooling/cli/src/utils/combine-schema-results.tspackages/1-framework/3-tooling/cli/src/utils/combine-verify-results.tspackages/1-framework/3-tooling/cli/src/utils/extension-pack-inputs.tspackages/1-framework/3-tooling/cli/src/utils/formatters/verify.tspackages/1-framework/3-tooling/cli/src/utils/plan-resolution.tspackages/1-framework/3-tooling/cli/test/commands/migrate-show.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-plan-command.test.tspackages/1-framework/3-tooling/cli/test/commands/migration-read-commands-parity.test.tspackages/1-framework/3-tooling/cli/test/control-api/apply.test.tspackages/1-framework/3-tooling/cli/test/control-api/db-verify.per-space-verifier.test.tspackages/1-framework/3-tooling/cli/test/utils/combine-verify-results.test.tspackages/1-framework/3-tooling/cli/test/utils/plan-resolution.test.tspackages/1-framework/3-tooling/migration/src/aggregate/aggregate.tspackages/1-framework/3-tooling/migration/src/aggregate/check-integrity.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/planner.tspackages/1-framework/3-tooling/migration/src/aggregate/strategies/graph-walk.tspackages/1-framework/3-tooling/migration/src/aggregate/strategies/synth.tspackages/1-framework/3-tooling/migration/src/aggregate/types.tspackages/1-framework/3-tooling/migration/src/aggregate/unclaimed-elements.tspackages/1-framework/3-tooling/migration/src/aggregate/verifier.tspackages/1-framework/3-tooling/migration/src/exports/aggregate.tspackages/1-framework/3-tooling/migration/src/gather-disk-contract-space-state.tspackages/1-framework/3-tooling/migration/src/integrity-violation.tspackages/1-framework/3-tooling/migration/src/verify-contract-spaces.tspackages/1-framework/3-tooling/migration/test/aggregate/check-integrity.test.tspackages/1-framework/3-tooling/migration/test/aggregate/contract-at.test.tspackages/1-framework/3-tooling/migration/test/aggregate/loader.test.tspackages/1-framework/3-tooling/migration/test/aggregate/planner.test.tspackages/1-framework/3-tooling/migration/test/aggregate/strategies/graph-walk.test.tspackages/1-framework/3-tooling/migration/test/aggregate/strategies/synth.test.tspackages/1-framework/3-tooling/migration/test/aggregate/unclaimed-elements.test.tspackages/1-framework/3-tooling/migration/test/aggregate/verifier.test.tspackages/1-framework/3-tooling/migration/test/deletable-node-modules.test.tspackages/1-framework/3-tooling/migration/test/fixtures.tspackages/2-sql/1-core/schema-ir/src/ir/sql-schema-ir-node.tspackages/2-sql/9-family/src/core/migrations/types.tspackages/2-sql/9-family/src/exports/control.tspackages/3-extensions/supabase/test/classification.e2e.test.tspackages/3-mongo-target/1-mongo-target/src/core/control-target.tspackages/3-mongo-target/1-mongo-target/src/core/scope-verify-result.tspackages/3-mongo-target/1-mongo-target/test/scope-verify-result.test.tspackages/3-targets/3-targets/postgres/src/core/migrations/diff-database-schema.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner.tspackages/3-targets/3-targets/postgres/src/core/schema-ir/schema-node-kinds.tspackages/3-targets/3-targets/sqlite/src/core/migrations/planner.ts
💤 Files with no reviewable changes (2)
- packages/2-sql/9-family/src/exports/control.ts
- packages/1-framework/3-tooling/cli/src/utils/combine-schema-results.ts
✅ Files skipped from review due to trivial changes (13)
- packages/1-framework/3-tooling/cli/src/utils/extension-pack-inputs.ts
- packages/1-framework/3-tooling/migration/src/gather-disk-contract-space-state.ts
- packages/1-framework/3-tooling/cli/src/control-api/operations/db-init.ts
- packages/1-framework/3-tooling/migration/src/integrity-violation.ts
- packages/1-framework/3-tooling/cli/src/control-api/operations/db-run.ts
- packages/1-framework/3-tooling/cli/src/control-api/operations/run-migration.ts
- packages/1-framework/3-tooling/cli/src/commands/migration-graph.ts
- docs/architecture docs/subsystems/7. Migration System.md
- packages/3-extensions/supabase/test/classification.e2e.test.ts
- packages/1-framework/3-tooling/migration/src/aggregate/planner-types.ts
- packages/1-framework/3-tooling/cli/src/control-api/types.ts
- packages/1-framework/3-tooling/migration/test/aggregate/loader.test.ts
- packages/1-framework/3-tooling/migration/src/verify-contract-spaces.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/2-sql/1-core/schema-ir/src/ir/sql-schema-ir-node.ts
- packages/1-framework/3-tooling/migration/test/deletable-node-modules.test.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/diff-database-schema.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/planner.ts
| const exitCode = handleResult(result, flags, ui, (combined) => { | ||
| if (flags.json) { | ||
| ui.output(formatSchemaVerifyJson(schemaVerifyResult)); | ||
| ui.output(formatSchemaVerifyJson(combined.result, combined.unclaimed)); | ||
| } else { | ||
| const output = formatSchemaVerifyOutput(schemaVerifyResult, flags); | ||
| const output = formatSchemaVerifyOutput(combined.result, flags, combined.unclaimed); | ||
| if (output) { | ||
| ui.log(output); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| if (result.ok && !result.value.ok) { | ||
| if (result.ok && !result.value.result.ok) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Schema-only failures can exit silently under --quiet.
Unlike the full-mode failure branch below (Lines 583-591) which explicitly forces { ...flags, quiet: false } so that "exiting 1 without diagnostics is unhelpful" never happens, this schema-only success-callback passes flags unmodified into formatSchemaVerifyOutput. Since that formatter returns '' immediately when flags.quiet is true, running db verify --schema-only --quiet against a database that fails schema verification will print nothing and simply exit with code 1 (via the result.ok && !result.value.result.ok check just below), leaving the user with no diagnostic output.
The line-range change summary for this segment describes "a forced quiet: false flags object," but the actual code doesn't apply that override here — only the full-mode failure path does.
🐛 Proposed fix to force diagnostics on schema-only failures
const exitCode = handleResult(result, flags, ui, (combined) => {
if (flags.json) {
ui.output(formatSchemaVerifyJson(combined.result, combined.unclaimed));
} else {
- const output = formatSchemaVerifyOutput(combined.result, flags, combined.unclaimed);
+ const outputFlags = combined.result.ok ? flags : { ...flags, quiet: false };
+ const output = formatSchemaVerifyOutput(combined.result, outputFlags, combined.unclaimed);
if (output) {
ui.log(output);
}
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const exitCode = handleResult(result, flags, ui, (combined) => { | |
| if (flags.json) { | |
| ui.output(formatSchemaVerifyJson(schemaVerifyResult)); | |
| ui.output(formatSchemaVerifyJson(combined.result, combined.unclaimed)); | |
| } else { | |
| const output = formatSchemaVerifyOutput(schemaVerifyResult, flags); | |
| const output = formatSchemaVerifyOutput(combined.result, flags, combined.unclaimed); | |
| if (output) { | |
| ui.log(output); | |
| } | |
| } | |
| }); | |
| if (result.ok && !result.value.ok) { | |
| if (result.ok && !result.value.result.ok) { | |
| const exitCode = handleResult(result, flags, ui, (combined) => { | |
| if (flags.json) { | |
| ui.output(formatSchemaVerifyJson(combined.result, combined.unclaimed)); | |
| } else { | |
| const outputFlags = combined.result.ok ? flags : { ...flags, quiet: false }; | |
| const output = formatSchemaVerifyOutput(combined.result, outputFlags, combined.unclaimed); | |
| if (output) { | |
| ui.log(output); | |
| } | |
| } | |
| }); | |
| if (result.ok && !result.value.result.ok) { |
🤖 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/db-verify.ts` around lines
545 - 556, Schema-only verification can exit with no diagnostics when `--quiet`
is set because `handleResult`’s callback in `db-verify.ts` passes `flags`
straight into `formatSchemaVerifyOutput`, which suppresses output for quiet
mode. Update the schema-only success path to mirror the full-mode failure branch
by forcing a non-quiet flags object before calling `formatSchemaVerifyOutput`,
using the existing `flags`, `handleResult`, and `formatSchemaVerifyOutput` flow
so `--schema-only --quiet` still prints the failure details.
| // Prefer a failing space's family phrasing; else, when only the unclaimed list | ||
| // fails the verdict, say so; else keep the app space's phrasing. | ||
| const summary = okAll | ||
| ? unclaimedFails | ||
| ? `Database schema has ${unclaimed.length} unclaimed element${unclaimed.length === 1 ? '' : 's'} (not in any contract)` | ||
| : appResult.summary | ||
| : appResult.ok | ||
| ? (firstFailure as VerifyDatabaseSchemaResult).summary | ||
| : appResult.summary; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Bare as cast in production code.
(firstFailure as VerifyDatabaseSchemaResult).summary is a bare as cast on a non-test, non-as const production file. As per coding guidelines, **/*.{ts,tsx}: "No bare as in production code. Use blindCast<T, "Reason"> or castAs<T> from @prisma-next/utils/casts". The cast is logically safe (guarded by !okAll), so narrow it via blindCast with a short reason documenting the invariant.
♻️ Proposed fix
+import { blindCast } from '`@prisma-next/utils/casts`';
import type { VerifyDatabaseSchemaResult } from '`@prisma-next/framework-components/control`';
...
const summary = okAll
? unclaimedFails
? `Database schema has ${unclaimed.length} unclaimed element${unclaimed.length === 1 ? '' : 's'} (not in any contract)`
: appResult.summary
: appResult.ok
- ? (firstFailure as VerifyDatabaseSchemaResult).summary
+ ? blindCast<
+ VerifyDatabaseSchemaResult,
+ 'firstFailure is set whenever !okAll (some space failed)'
+ >(firstFailure).summary
: appResult.summary;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Prefer a failing space's family phrasing; else, when only the unclaimed list | |
| // fails the verdict, say so; else keep the app space's phrasing. | |
| const summary = okAll | |
| ? unclaimedFails | |
| ? `Database schema has ${unclaimed.length} unclaimed element${unclaimed.length === 1 ? '' : 's'} (not in any contract)` | |
| : appResult.summary | |
| : appResult.ok | |
| ? (firstFailure as VerifyDatabaseSchemaResult).summary | |
| : appResult.summary; | |
| // Prefer a failing space's family phrasing; else, when only the unclaimed list | |
| // fails the verdict, say so; else keep the app space's phrasing. | |
| const summary = okAll | |
| ? unclaimedFails | |
| ? `Database schema has ${unclaimed.length} unclaimed element${unclaimed.length === 1 ? '' : 's'} (not in any contract)` | |
| : appResult.summary | |
| : appResult.ok | |
| ? blindCast< | |
| VerifyDatabaseSchemaResult, | |
| 'firstFailure is set whenever !okAll (some space failed)' | |
| >(firstFailure).summary | |
| : appResult.summary; |
🤖 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/combine-verify-results.ts`
around lines 73 - 81, The summary logic in combineVerifyResults uses a bare cast
on firstFailure, which violates the production casting guideline. Replace the
`(firstFailure as VerifyDatabaseSchemaResult).summary` access with a safe cast
helper from `@prisma-next/utils/casts`, using blindCast or castAs with a short
reason that documents the okAll/appResult.ok invariant. Keep the existing
branching in combineVerifyResults unchanged and apply the cast only where
firstFailure is read.
Source: Coding guidelines
| * Collect the union of every namespace declared across all contract spaces of an | ||
| * aggregate (app + extensions) and return a minimal object with the shape | ||
| * `{ storage: { namespaces } }` suitable for passing to | ||
| * `familyInstance.introspect`. | ||
| * | ||
| * Callers invoke this after the integrity gate (`buildContractSpaceAggregate` | ||
| * with `checkContracts: true`), so every `member.contract()` call is safe — | ||
| * with `checkContracts: true`), so every `space.contract()` call is safe — | ||
| * no try/catch is needed here. | ||
| */ | ||
| export function collectAggregateNamespaces(aggregate: ContractSpaceAggregate): { | ||
| readonly storage: { readonly namespaces: Readonly<Record<string, StorageNamespace>> }; | ||
| } { | ||
| const merged: Record<string, StorageNamespace> = {}; | ||
| for (const member of aggregate.spaces()) { | ||
| for (const [key, ns] of Object.entries(member.contract().storage.namespaces)) { | ||
| for (const space of aggregate.spaces()) { | ||
| for (const [key, ns] of Object.entries(space.contract().storage.namespaces)) { | ||
| merged[key] = ns; | ||
| } | ||
| } | ||
| return { storage: { namespaces: merged } }; | ||
| } | ||
|
|
||
| /** | ||
| * Assemble a {@link ContractSpaceAggregate} value from its members and a | ||
| * Assemble a {@link ContractSpaceAggregate} value from its contract spaces and a |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether namespace-level (not just element-level) disjointness across spaces is enforced anywhere
rg -n -C3 'namespaceId' packages/1-framework/3-tooling/migration/src/aggregate/check-integrity.tsRepository: prisma/prisma-next
Length of output: 633
🏁 Script executed:
#!/bin/bash
sed -n '200,310p' packages/1-framework/3-tooling/migration/src/aggregate/aggregate.ts
printf '\n---\n'
sed -n '180,260p' packages/1-framework/3-tooling/migration/src/aggregate/check-integrity.ts
printf '\n---\n'
rg -n -C3 'introspect\(|collectAggregateNamespaces|storage:\s*\{\s*namespaces' packages/1-framework/3-tooling/migration/srcRepository: prisma/prisma-next
Length of output: 8771
🏁 Script executed:
rg -n -C3 'type StorageNamespace|interface StorageNamespace|namespaceId|namespaces\s*:' packages/1-framework/3-tooling/migration/src packages/1-framework/3-tooling/migration -g '!**/dist/**'Repository: prisma/prisma-next
Length of output: 18469
🏁 Script executed:
rg -n -C3 'collectAggregateNamespaces\(' packages/1-framework/3-tooling/migration/src packages/1-framework/3-tooling/migration/testRepository: prisma/prisma-next
Length of output: 1001
🏁 Script executed:
rg -n -C3 'StorageNamespace|elementCoordinates\(|namespaceId|disjointness|unclaimed|extra' packages/1-framework/3-tooling/migration/src packages/1-framework/3-tooling/migration/testRepository: prisma/prisma-next
Length of output: 50374
Merge namespace contents instead of replacing them
merged[key] = ns is last-write-wins. Spaces are allowed to share a namespaceId as long as their primitives are disjoint, so this can silently drop earlier tables from the object passed to familyInstance.introspect and skew the extra/unclaimed results. Merge the namespace entries (or reject conflicting namespace shapes) instead of overwriting the whole namespace.
🤖 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/aggregate/aggregate.ts` around
lines 241 - 263, The namespace collection logic in collectAggregateNamespaces
currently overwrites same-key entries with last-write-wins, which can drop
earlier tables from shared namespaceIds. Update the aggregation in
collectAggregateNamespaces to merge namespace contents per key instead of
assigning merged[key] = ns, or explicitly detect and reject incompatible
namespace shapes before building the object passed to familyInstance.introspect.
Keep the fix localized to collectAggregateNamespaces and its merged namespace
accumulation.
| /** The bare entity name an extra `SchemaDiffIssue` addresses, read off its actual (live-DB) node. */ | ||
| function schemaDiffIssueEntityName(issue: SchemaDiffIssue): string | undefined { | ||
| const actual = issue.actual; | ||
| if (actual === undefined) return undefined; | ||
| const name = (actual as { readonly tableName?: unknown }).tableName; | ||
| return typeof name === 'string' ? name : undefined; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Replace the bare as cast with blindCast.
Line 34 uses a bare as in production code to read the optional tableName off the diff node. The parallel helper issueEntityName in strategies/synth.ts already reads the same field via blindCast; mirror it here for consistency and guideline compliance.
Proposed fix
+import { blindCast } from '`@prisma-next/utils/casts`';
+
/** The bare entity name an extra `SchemaDiffIssue` addresses, read off its actual (live-DB) node. */
function schemaDiffIssueEntityName(issue: SchemaDiffIssue): string | undefined {
const actual = issue.actual;
if (actual === undefined) return undefined;
- const name = (actual as { readonly tableName?: unknown }).tableName;
+ const name = blindCast<
+ { readonly tableName?: unknown },
+ 'entity-name scoping reads the optional target-specific tableName off a diff node'
+ >(actual).tableName;
return typeof name === 'string' ? name : undefined;
}As per coding guidelines: "No bare as in production code. Use blindCast<T, "Reason"> or castAs<T> from @prisma-next/utils/casts… as const and test files are exempt."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** The bare entity name an extra `SchemaDiffIssue` addresses, read off its actual (live-DB) node. */ | |
| function schemaDiffIssueEntityName(issue: SchemaDiffIssue): string | undefined { | |
| const actual = issue.actual; | |
| if (actual === undefined) return undefined; | |
| const name = (actual as { readonly tableName?: unknown }).tableName; | |
| return typeof name === 'string' ? name : undefined; | |
| } | |
| import { blindCast } from '`@prisma-next/utils/casts`'; | |
| /** The bare entity name an extra `SchemaDiffIssue` addresses, read off its actual (live-DB) node. */ | |
| function schemaDiffIssueEntityName(issue: SchemaDiffIssue): string | undefined { | |
| const actual = issue.actual; | |
| if (actual === undefined) return undefined; | |
| const name = blindCast< | |
| { readonly tableName?: unknown }, | |
| 'entity-name scoping reads the optional target-specific tableName off a diff node' | |
| >(actual).tableName; | |
| return typeof name === 'string' ? name : undefined; | |
| } |
🤖 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/aggregate/unclaimed-elements.ts`
around lines 30 - 36, The helper schemaDiffIssueEntityName still uses a bare
cast to read tableName from issue.actual, which violates the production casting
guideline. Update this function to match the pattern used by issueEntityName in
strategies/synth.ts by replacing the inline as cast with blindCast from
`@prisma-next/utils/casts`, while keeping the same undefined and string checks for
the extracted name.
Source: Coding guidelines
| /** | ||
| * Counts the pass/warn/fail statuses over a verification tree (root included). | ||
| * Used only when scoping actually dropped a node — the pruned tree is then | ||
| * self-consistent, so the recomputed `fail` is the honest verdict signal. | ||
| */ | ||
| function countTree(node: SchemaVerificationNode): Counts { | ||
| let pass = 0; | ||
| let warn = 0; | ||
| let fail = 0; | ||
| let totalNodes = 0; | ||
| const visit = (n: SchemaVerificationNode): void => { | ||
| totalNodes += 1; | ||
| if (n.status === 'pass') pass += 1; | ||
| else if (n.status === 'warn') warn += 1; | ||
| else fail += 1; | ||
| for (const child of n.children) visit(child); | ||
| }; | ||
| visit(node); | ||
| return { pass, warn, fail, totalNodes }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
countTree should skip the synthetic root.
When scoping drops a node, this recomputation counts the root too, so pass/warn/fail/totalNodes end up off by one versus Mongo’s “root not counted” basis. Iterate over node.children instead.
🤖 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/3-mongo-target/1-mongo-target/src/core/scope-verify-result.ts`
around lines 61 - 80, The countTree helper is currently including the synthetic
root in its pass/warn/fail/totalNodes totals, which makes the recomputed
verification counts off by one. Update countTree in scope-verify-result.ts to
traverse and count only node.children from the provided root instead of visiting
the root itself, while keeping the existing counting logic for
SchemaVerificationNode statuses unchanged.
| /** | ||
| * Caller-supplied keep-predicate the planner applies to its schema diff | ||
| * (via `SchemaDiff.filter`) before building operations. The orchestration | ||
| * constructs it so the diff findings reaching op-building are exactly the | ||
| * contract space's own — e.g. dropping the `extra` findings for elements | ||
| * a sibling contract space declares, so the planner never emits DROP ops | ||
| * against another space's tables. The planner applies it blindly and | ||
| * holds no ownership logic. Absent for single-space plans. | ||
| */ | ||
| readonly keepDiffIssue?: (issue: DiffIssue) => boolean; |
There was a problem hiding this comment.
This is a weird inversion of control. Why not just give it the list of issues?
| function isEntityNode(node: SchemaVerificationNode): boolean { | ||
| return node.kind === 'table' || node.kind === 'collection'; | ||
| } |
There was a problem hiding this comment.
WHAT THE FUCK IS THIS DOING IN THE FRAMEWORK DOMAIN
| function isExtraIssue(issue: SchemaIssue): issue is BaseSchemaIssue { | ||
| return ( | ||
| issue.kind === 'extra_table' || | ||
| issue.kind === 'extra_column' || | ||
| issue.kind === 'extra_primary_key' || | ||
| issue.kind === 'extra_foreign_key' || | ||
| issue.kind === 'extra_unique_constraint' || | ||
| issue.kind === 'extra_index' || | ||
| issue.kind === 'extra_validator' || | ||
| issue.kind === 'extra_default' | ||
| ); |
There was a problem hiding this comment.
This cannot live in the framework domain
| function isExtraTableNode(node: SchemaVerificationNode): boolean { | ||
| return node.code === 'extra_table' || node.code === 'EXTRA_COLLECTION'; | ||
| } |
| } | ||
|
|
||
| /** | ||
| * Part 2 (per-space contribution) — the bare names of every live element this |
| readonly orphanElements: readonly OrphanElement[]; | ||
| readonly perSpace: ReadonlyMap<string, VerifyDatabaseSchemaResult>; | ||
| /** | ||
| * Part 2 — one deduplicated, sorted list of live element names no contract |
| * enumerable, so it survives a spread that flattens a node into a plain | ||
| * object. | ||
| */ | ||
| readonly nodeKind?: string; |
What this delivers
Retires the conflated
PostgresSchemaIR(a diff-tree node, a Postgres schema, and the tree root at once) for a single-purposedatabase → namespace → table → policyschema-node tree, and — through three rounds of review — restructures schema diffing anddb verify/planaround it. Foundation for slice 3 (@@rls, managed/external grading, policy rename).The architecture (as landed)
Authoritative design:
design-diff-and-verify.md. Four actors, cleanly separated:SchemaDiff<SqlSchemaDiffNode>; the planner builds ops from the typed nodes, no casts). Internals private.declaresEntity/declaringSpaces); it diffs, verifies, and classifies nothing.keepDiffIssuepredicate (built over the passive aggregate); the planner applies it verbatim before op-building. No schema pruning, no "other spaces' names", no post-scoping —scope-schema-result,entitiesOwnedByOtherSpaces, and the set-subtraction helper are deleted.…SchemaNodeclasses with definednodeKinds; staticis/assertguards (noinstanceof, no node-constructingensure());isEqualTois identity; the vocabulary is contract space throughout the aggregate.Review response
nodeKindguards.SchemaDifferSPI +SchemaDiffresult; node-guard consistency; file dedup (one diff module; diff-SPI types out of the catch-alltypes.ts); planner cleanups; transient IDs removed.5e3673318); the two-part verify (9e2e80f0f+8c3d4a10b+9309fd00d); the plan collapse deleting the cross-space plumbing (14f5b7a73); the contract-space rename retiring "member" and "schema result" (9d56e8eba).Confirmed non-duplicates from review:
contract-to-postgres-database-schema-nodebuilds the Postgres tree atop the shared per-namespace table conversion (contractToSchemaIR's flat map is SQLite-only);verify-postgres-namespacesis live (itsmissing_schemaissues drive multi-schemaCREATE SCHEMAplanning).Verification
Planner ops byte-identical throughout (
fixtures:checkclean at every unit);contract inferunchanged; the runner's post-apply verify and single-space family verify preserved; multi-space guards green (cross-namespace-fk, supabaseclassification.e2e+cross-contract-fk,multi-namespace-runtime).lint:depsclean;lint:castsnet −11. The deliberate output change is confined todb verify's unclaimed-element reporting (two-part model above). CI on the merge ref arbitrates the full suites.Follow-ons (out of scope)
SchemaIssuenode-typing and liftsroot/countsoff the relational walk.annotations.pgretirement.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes