feat(contract): implement ADR 172 domain-storage separation#275
Merged
wmadden merged 35 commits intoApr 1, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
✅ Files skipped from review due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughThis pull request introduces a major contract schema redesign, replacing relation-level Changes
Sequence Diagram(s)The changes meet diagram criteria: they introduce a new cross-cutting validation feature, span multiple components (contract parser, validator, emitter, runtime), and involve clear sequential operations. However, the changes are highly heterogeneous (spanning documentation updates, type system changes, validation logic, and test fixtures across many packages), making a coherent single diagram impractical. The domain validation flow is the most cohesive new feature, but it is relatively straightforward (parse → validate → emit) and doesn't reveal hidden complexity. Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
@prisma-next/runtime-executor
@prisma-next/mongo-core
@prisma-next/mongo-orm
@prisma-next/mongo-runtime
@prisma-next/sql-runtime
@prisma-next/extension-paradedb
@prisma-next/extension-pgvector
@prisma-next/postgres
@prisma-next/sql-orm-client
@prisma-next/target-mongo
@prisma-next/adapter-mongo
@prisma-next/driver-mongo
@prisma-next/contract-authoring
@prisma-next/contract-ts
@prisma-next/ids
@prisma-next/psl-parser
@prisma-next/psl-printer
@prisma-next/cli
@prisma-next/emitter
@prisma-next/eslint-plugin
@prisma-next/migration-tools
@prisma-next/vite-plugin-contract-emit
@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/family-sql
@prisma-next/sql-lane-query-builder
@prisma-next/sql-relational-core
@prisma-next/sql-builder
@prisma-next/target-postgres
@prisma-next/adapter-postgres
@prisma-next/driver-postgres
@prisma-next/core-control-plane
@prisma-next/core-execution-plane
@prisma-next/config
@prisma-next/contract
@prisma-next/operations
@prisma-next/plan
@prisma-next/utils
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (10)
.agents/skills/multiline-commit-messages/SKILL.md (2)
10-10: Minor wording clarification: "fails silently" may be inaccurate.The heredoc pattern doesn't necessarily "fail silently"—it typically produces garbled or unexpected output (as noted later in the document). Consider rephrasing to "fails to produce the intended result" or "produces garbled output" for accuracy.
✍️ Suggested wording improvement
-The Shell tool sends commands as a single string. Heredoc syntax (`<<'EOF'`) inside `$(cat ...)` is fragile and fails silently — the literal `$(cat <<'EOF' ...` ends up as the commit message instead of the intended text. +The Shell tool sends commands as a single string. Heredoc syntax (`<<'EOF'`) inside `$(cat ...)` is fragile and produces garbled output — the literal `$(cat <<'EOF' ...` ends up as the commit message instead of the intended text.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/multiline-commit-messages/SKILL.md at line 10, Update the wording that currently says the heredoc pattern "fails silently" when used inside the Shell tool; locate the sentence mentioning the heredoc pattern (e.g., the text containing "`$(cat <<'EOF'`" and the phrase "fails silently") and replace it with a more accurate phrase such as "fails to produce the intended result" or "produces garbled output" so the description accurately reflects the observed behavior.
36-36: Optional: Add more technical precision to the explanation.The phrase "may not parse the heredoc correctly" could be more specific. The actual issue is that heredoc delimiters are not recognized or processed as expected when used inside command substitution
$(...), causing the shell to treat the heredoc syntax as literal text rather than interpreting it.✍️ Suggested technical improvement
-The shell may not parse the heredoc correctly in this context — the `EOF` delimiter, newlines, and nested quoting interact unpredictably. The result is the raw `$(cat <<'EOF' ...` text appearing as the commit message. +The shell does not process the heredoc delimiter correctly inside command substitution — the `EOF` delimiter, newlines, and nested quoting are not interpreted as expected. The result is the raw `$(cat <<'EOF' ...` text appearing as the commit message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/multiline-commit-messages/SKILL.md at line 36, Clarify the explanation to state that heredoc delimiters (the EOF marker) can be ignored or treated as literal when used inside command substitution like $(cat <<'EOF' ... EOF), so the shell may not interpret the heredoc and the raw '$(cat <<'EOF' ...' text appears as the commit message; update the sentence to explicitly say "heredoc delimiters are not recognized inside command substitution $(...), causing the heredoc to be emitted verbatim" and add a short remediation suggestion such as using a plain here-doc (cat <<'EOF' >file ... EOF), printf, or writing the content to a temporary file instead of embedding the heredoc inside $().packages/1-framework/1-core/shared/contract/src/ir.ts (1)
125-125: Use sharedifDefined()utility instead of local helper.Line 125 continues a local conditional-spread helper pattern; please switch to
ifDefinedfrom@prisma-next/utils/definedfor consistency across packages.As per coding guidelines:
{packages,examples,test}/**/*.{ts,tsx}: UseifDefined()from@prisma-next/utils/definedfor conditional object spreads instead of inline conditional spread patterns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/1-core/shared/contract/src/ir.ts` at line 125, Replace the local conditional-spread helper with the shared ifDefined utility: import ifDefined from '@prisma-next/utils/defined' and use it where the inline conditional spread is applied (e.g., the expression involving 'roots' and opts.roots) so the object spread becomes ifDefined('roots', opts.roots); remove the local helper definition and any duplicate logic to ensure consistency across packages and satisfy the coding guideline.packages/1-framework/1-core/shared/contract/src/domain-types.ts (1)
1-29: Prefer exported interfaces for these public domain shapes.For this shared public contract surface, convert these object-shape
typeexports tointerfaceexports to match repository API conventions.♻️ Proposed refactor
-export type DomainField = { +export interface DomainField { readonly nullable: boolean; readonly codecId: string; -}; +} -export type DomainRelationOn = { +export interface DomainRelationOn { readonly localFields: readonly string[]; readonly targetFields: readonly string[]; -}; +} -export type DomainRelation = { +export interface DomainRelation { readonly to: string; readonly cardinality: '1:1' | '1:N' | 'N:1'; readonly on?: DomainRelationOn; -}; +} -export type DomainDiscriminator = { +export interface DomainDiscriminator { readonly field: string; -}; +} -export type DomainModel = { +export interface DomainModel { readonly fields: Record<string, DomainField>; readonly relations: Record<string, DomainRelation>; readonly storage: Record<string, unknown>; readonly discriminator?: DomainDiscriminator; readonly variants?: Record<string, unknown>; readonly base?: string; readonly owner?: string; -}; +}As per coding guidelines:
{packages,examples,test}/**/*.{ts,tsx}: Export interfaces and factory functions for public APIs; keep concrete classes as private implementation details.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/1-core/shared/contract/src/domain-types.ts` around lines 1 - 29, The exported object-shape type aliases (DomainField, DomainRelationOn, DomainRelation, DomainDiscriminator, DomainModel) should be converted to exported interfaces to follow the public API convention; replace each "export type X = { ... }" with "export interface X { ... }", preserving readonly modifiers, union literals (e.g. cardinality), optional properties (e.g. on, discriminator, variants, base, owner) and existing property types (Record<string,...>, readonly string[], etc.), and then run a quick project-wide type check to ensure any imports/usages of these symbols still compile (adjust exported references if any import expected a type alias).packages/2-sql/2-authoring/contract-ts/test/contract.structure.test.ts (1)
65-69: Move normalization assertion out of validator-structure tests.This case now tests normalization (
modelsomitted →{}) inside a validator structure suite. Keep validator tests on already-normalized input and assert normalization in the normalization test file.Proposed adjustment
- it('defaults missing models to empty object', () => { - // biome-ignore lint/suspicious/noExplicitAny: testing invalid input - const invalid = { ...validContractInput, models: undefined } as any; - const result = validateContract<SqlContract<SqlStorage>>(invalid); - expect(result.models).toEqual({}); - }); + it('accepts normalized models object', () => { + const normalized = { ...validContractInput, models: {} }; + const result = validateContract<SqlContract<SqlStorage>>(normalized); + expect(result.models).toEqual({}); + });As per coding guidelines:
packages/**/*.test.ts: “Tests for validators, parsers, and emitter hooks should test that they work with already-normalized contracts, NOT that they normalize.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/2-authoring/contract-ts/test/contract.structure.test.ts` around lines 65 - 69, The test currently asserts normalization (that missing models become {}) inside the validator-structure suite; instead, remove the normalization check from this test and ensure the validator is given already-normalized input. Change the test using validateContract/SqlContract/SqlStorage to pass an input where models is explicitly {} (not undefined) and assert validator behavior only, and add a separate test in the normalization test file that verifies missing/undefined models are normalized to {} by the normalization routine.packages/2-sql/1-core/contract/src/validators.ts (1)
136-158: Consider adding'+': 'reject'for consistent strict validation.Other schemas in this file (e.g.,
StorageColumnSchema,ExecutionSchema,StorageSchema) use'+': 'reject'to reject unexpected properties. The model-related schemas (ModelFieldSchema,ModelStorageFieldSchema,ModelStorageSchema,ModelSchema) don't have this constraint, which means extra properties will be silently accepted.This inconsistency could mask contract data issues during validation.
♻️ Suggested fix
const ModelFieldSchema = type({ + '+': 'reject', 'column?': 'string', 'nullable?': 'boolean', 'codecId?': 'string', }); const ModelStorageFieldSchema = type({ + '+': 'reject', column: 'string', }); const ModelStorageSchema = type({ + '+': 'reject', table: 'string', 'fields?': type({ '[string]': ModelStorageFieldSchema }), }); const ModelSchema = type({ + '+': 'reject', storage: ModelStorageSchema, fields: type({ '[string]': ModelFieldSchema }), relations: type({ '[string]': 'unknown' }), 'discriminator?': 'unknown', 'variants?': 'unknown', 'base?': 'string', });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/1-core/contract/src/validators.ts` around lines 136 - 158, The model-related schemas currently allow unexpected properties; update ModelFieldSchema, ModelStorageFieldSchema, ModelStorageSchema, and ModelSchema to be strict by adding the constraint '+': 'reject' to each schema definition so any unknown properties are rejected during validation; ensure you add the '+': 'reject' entry at the top-level of each type(...) call for those four symbols to match the existing pattern used by StorageColumnSchema, ExecutionSchema, and StorageSchema.packages/2-sql/2-authoring/contract-ts/src/contract.ts (1)
286-287: Consider adding a type predicate to avoid the two-step cast.The
as unknown as ModelDefinitioncast is used becausenormalizeContract()produces a union type containing both domain and SQL-specific properties. Per coding guidelines, blind casts should be minimized.Consider extracting a type predicate helper to narrow the type safely:
♻️ Suggested refactor
+function isModelDefinition(value: unknown): value is ModelDefinition { + return ( + typeof value === 'object' && + value !== null && + 'storage' in value && + typeof (value as { storage?: unknown }).storage === 'object' + ); +} + // Validate models for (const [modelName, modelUnknown] of Object.entries(models)) { - // normalizeContract() ensures models have both domain (DomainModel) and SQL-specific (ModelDefinition) properties - const model = modelUnknown as unknown as ModelDefinition; + if (!isModelDefinition(modelUnknown)) { + throw new Error(`Model "${modelName}" is not a valid ModelDefinition`); + } + const model = modelUnknown;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/2-authoring/contract-ts/src/contract.ts` around lines 286 - 287, The code uses a blind double-cast "modelUnknown as unknown as ModelDefinition" after normalizeContract(); instead implement a type predicate to safely narrow the union: add a helper function like isModelDefinition(x: unknown): x is ModelDefinition that checks distinguishing properties (the SQL-specific fields present on ModelDefinition), then replace the double-cast by calling isModelDefinition(modelUnknown) and, when true, assign model = modelUnknown so TypeScript infers ModelDefinition without unsafe casts; reference normalizeContract, ModelDefinition, and the local variable modelUnknown when adding the predicate and using it.packages/2-sql/1-core/contract/test/validate.test.ts (1)
770-989: Move the dual-format bridge coverage into its own test file.
validate.test.tsis now roughly 1,000 lines, and this new block is a naturalvalidate.bridge.test.ts/validate.normalization.test.tssplit. Keeping it separate will make the validator cases easier to scan and keep the file within the repo limit.As per coding guidelines,
**/*.test.ts: "Keep test files under 500 lines to maintain readability and navigability. Split test files when they exceed 500 lines, contain multiple distinct concerns that can be logically separated, or have multiple top-leveldescribeblocks that can be split by functionality."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/1-core/contract/test/validate.test.ts` around lines 770 - 989, The dual-format bridge tests (the describe('dual-format bridge', ...) block exercising validateContract, SqlContract and SqlStorage) should be moved out of validate.test.ts into a new test file (e.g., validate.bridge.test.ts or validate.normalization.test.ts); extract the entire describe block and paste it into the new file, keep the same imports/usings for validateContract/SqlContract/SqlStorage, remove the block from the original file, run tests to ensure no missing imports/exports and update any test-runner config if necessary so the new file is picked up; ensure test names and top-level describe remain unchanged so assertions like expect(result.roots) and mappings checks continue to run identically.test/integration/test/emitter-lanes.test.ts (1)
324-332: Assert the bootstrap emission before the validator normalizes it.Once
bootstrapJsongoes throughvalidateContract(), the rest of this test is exercising the bridge, not the original emitter output. A firstemit()that still returns legacymodels.*.fields = { column }would be repaired here and this test would still pass. Please add one direct ADR 172 assertion onbootstrapJsonbefore normalizing it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/test/emitter-lanes.test.ts` around lines 324 - 332, Add a direct assertion that the raw emitter output (bootstrapJson) conforms to ADR-172 legacy shape before calling validateContract(): after parsing bootstrap.contractJson and before validateContract/bootstrap normalization, assert that bootstrapJson.models exists and that each model has a fields property in the ADR-172 form (i.e., fields is an object mapping column names to column definitions, not the normalized array/bridge shape). This assertion ensures the test verifies the original emitter output produced the legacy models.*.fields structure prior to validateContract().packages/2-sql/3-tooling/emitter/test/emitter-hook.generation.advanced.test.ts (1)
349-418: Split this test file by concern before adding more cases.
emitter-hook.generation.advanced.test.tsis already well over the 500-line cap, and the new relation-shape / parameterized-renderer coverage is a clean boundary for focused...relations.test.tsand...parameterized.test.tsfiles.As per coding guidelines,
**/*.test.ts: "Keep test files under 500 lines to maintain readability and navigability. Split test files when they exceed 500 lines, contain multiple distinct concerns that can be logically separated, or have multiple top-leveldescribeblocks that can be split by functionality."Also applies to: 564-591, 724-827
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/3-tooling/emitter/test/emitter-hook.generation.advanced.test.ts` around lines 349 - 418, The test suite block titled "generates relations type from models" (which calls sqlTargetFamilyHook.generateContractTypes) lives in an oversized file and should be moved into a new focused test file for relations (e.g., relations.test.ts) so the original emitter-hook.generation.advanced.test.ts stays under 500 lines; extract all relation-specific tests (including the assertions checking "export type Relations" and the posts/comments expectation strings) into that new file, wire up the same fixtures/helpers (createContractIR, testHashes, sqlTargetFamilyHook) by importing them, and similarly split parameterized-renderer tests into a separate parameterized.test.ts, updating imports and test exports accordingly so each file addresses a single concern and the test runner still discovers them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/multiline-commit-messages/SKILL.md:
- Around line 1-37: The multiline-commit-messages skill (file SKILL.md for the
multiline-commit-messages skill) is unrelated to the ADR 172 domain-storage
separation PR; remove or revert this file from the current branch and create a
new, focused PR that adds the multiline-commit-messages SKILL.md (or move it to
a docs/tools/dev-guides repo/branch) so the ADR PR only contains domain-storage
changes—ensure the new PR includes the same content and an explanatory
title/description about commit message tooling guidance.
In @.agents/skills/write-architecture-docs/SKILL.md:
- Around line 49-50: The guidance line containing `An owned model declares
owner: "User" — a domain fact about aggregate membership. Its data lives within
the owner's storage. See [ADR 177](...)` uses a non-resolving placeholder link;
replace the `[ADR 177](...)` target with the actual repo-relative path to the
ADR (path-only, no URL scheme or line anchors) so the link resolves in markdown
previews—update the link target in SKILL.md where the string `owner: "User"`
appears to point to the real ADR file path using the format `[ADR
177](path/to/adr-177.md)`.
In `@docs/architecture` docs/adrs/ADR 172 - Contract domain-storage separation.md:
- Line 198: The ADR contains conflicting guidance: the header line references
ADR 177 saying `owner` replaces relation `strategy` while the "Implemented in
this PR" section still describes `model.relations` “with strategy”; update the
"Implemented in this PR" section in ADR 172 to remove any mention of `strategy`
and instead describe how `owner` is used on models (e.g., change descriptions of
`model.relations` to reference `owner` semantics), ensure all examples and text
consistently reference `owner` (not `strategy`), and remove or reword any
remaining phrases that imply `strategy` is still used so the ADR is internally
consistent with ADR 177.
In `@docs/architecture` docs/adrs/ADR 177 - Ownership replaces relation
strategy.md:
- Around line 66-69: The SQL example is inconsistent: the "roots" object lists
"posts": "Post" while no Post model exists and User.relations.posts references
it; either add a matching Post model or remove the posts root and the
User.relations.posts edge from the snippet. Update the snippet so "roots" only
includes defined models (e.g., keep "users": "User" and add a Post model
definition named "Post" matching the User.relations.posts edge, or remove
"posts" and the User.relations.posts reference), and apply the same fix to the
repeated block around lines 76–82.
In `@docs/glossary.md`:
- Line 88: Update the glossary entry to avoid the overly strict phrase "Owned
models have empty `storage` blocks": change the wording to indicate owned models
do not have an independent storage unit but may still include
`storage.relations` for nested owned children; keep references to `roots`,
`storage.relations`, and the example (`"owner": "User"`) and ensure the sentence
reads like “owned models have no independent storage unit (they may still
include `storage.relations` for nested owned children).”
In `@docs/planning/mongo-target/1-design-docs/design-questions.md`:
- Around line 333-335: Add a language specifier (e.g., "text") to the fenced
code blocks that render relationship examples so markdown linting passes;
specifically update the fences around the lines containing "Comment →
commentable → Post | Video" and "User ←→ user_roles ←→ Role" (the code blocks
currently using just ``` ) to use ```text instead.
- Line 354: The table cell for the "Convention field" row contains a raw pipe in
the inline code fragment `commentableType: "Post" | "Video"` which breaks
Markdown table parsing; update that cell by escaping the pipe (e.g., use
`commentableType: "Post" \| "Video"` or replace the pipe with the HTML entity
`|`) so the entire cell remains a single column while keeping the inline
code formatting.
In `@packages/1-framework/1-core/migration/control-plane/src/emission/emit.ts`:
- Around line 22-77: toDomainModel currently bails out when no field has
codecId, so legacy models that only use { column } never get their
storage.fields populated or derive nullable/codecId; modify to always process
fields (not only when hasEnrichedFields) so you still build cleanedFields, set
nullable default (and derive codecId/nullable from the referenced storage column
if available), and always merge/lift any field's column into
mergedStorageFields; keep existing relation handling via
stripStrategyFromRelations and ensure the final result for each model includes
fields: cleanedFields and storage: { ...storage, fields: mergedStorageFields }
(update the control flow around the hasEnrichedFields check in toDomainModel and
reuse mergedStorageFields/cleanedFields logic).
In `@packages/1-framework/1-core/shared/contract/src/validate-domain.ts`:
- Around line 89-92: The validator currently uses the `in` operator (e.g.
`modelName in baseModel.variants`) which walks the prototype chain and can be
spoofed by names like "constructor"; replace those checks with the ES2022
own-property test `Object.hasOwn(baseModel.variants, modelName)` for the variant
membership check and similarly use `Object.hasOwn(...)` for the discriminator
membership checks (the checks around `discriminator` at lines ~120-123) so you
only accept true own properties and satisfy the `noPrototypeBuiltins` guideline.
In `@packages/2-mongo-family/1-core/src/validate-domain.ts`:
- Around line 1-6: This file is just a family-local pass-through re-exporting
DomainContractShape, DomainModelShape, DomainValidationResult and
validateContractDomain from '@prisma-next/contract/validate-domain'; remove the
wrapper by updating every local consumer to import those symbols directly from
'@prisma-next/contract/validate-domain' (replace any imports referencing this
file with direct imports of DomainContractShape, DomainModelShape,
DomainValidationResult, validateContractDomain) and then delete this file to
eliminate the redundant re-export.
In `@packages/2-sql/1-core/contract/src/validate.ts`:
- Around line 406-445: The code overwrites existing relation buckets on
result.topRelations[tableName] when assigning generated relations, dropping
metadata (e.g., through or relations without an on block); instead perform a
deep/merge update: fetch the existing bucket (topRelations[tableName] or
result.topRelations[tableName]) and merge/assign only the relName entry’s
properties into it so existing relation entries are preserved (merge objects
rather than replace), similarly update the other assignment at lines referenced
(the later result.topRelations assignment) to merge into existing buckets; use
the keys modelRels, tableName, relName, topRelations/result.topRelations and
merge existing rel object and new { to, cardinality, on } fields so you don't
discard metadata.
- Around line 237-247: detectFormat currently returns 'old' as soon as it sees a
field with column, which causes enrichNewFormatModels (which re-adds column) to
flip new-format contracts back to old; change detectFormat (and the duplicate at
the other location) to prefer detecting new-format by checking for codecId first
on each field (if ('codecId' in field) return 'new'), and only return 'old' if
no codecId is found but a column is present (or default to 'old' after scanning
all fields); reference functions/values: detectFormat, enrichNewFormatModels,
models, fields, RawModel, RawField, and the field properties column and codecId.
In `@packages/2-sql/1-core/contract/src/validators.ts`:
- Around line 151-158: ModelSchema is missing the optional owner property from
the ModelDefinition type; update the ModelSchema declaration (the const
ModelSchema using type({...})) to include "'owner?': 'string'" alongside the
existing properties (storage: ModelStorageSchema, fields: type({ '[string]':
ModelFieldSchema }), relations, discriminator?, variants?, base?), so the schema
explicitly validates the optional owner field.
In `@packages/2-sql/2-authoring/contract-ts/schemas/data-contract-sql-v1.json`:
- Around line 564-589: The schema for relations[*].on currently allows one-sided
mappings and unknown keys; update the "on" definition to require complete join
pairs by adding an "oneOf" that enforces either both "parentCols" and
"childCols" (arrays of strings) or both "localFields" and "targetFields" (arrays
of strings), and set "additionalProperties": false so extraneous keys (typos)
are rejected; keep the existing property shapes but make the validator require
one of those two complete pairs using the "oneOf" keyword referencing the
property names parentCols, childCols, localFields, and targetFields.
- Around line 489-497: The Model schema currently allows hybrid/partial old/new
formats; change the schema branch that defines model shapes so there is a oneOf
between the legacy shape and the ADR-172 shape: create two alternatives for the
Model object (one requiring legacy per-field shape where "fields"
additionalProperties reference the legacy ModelStorageField column structure,
and the other requiring ADR-172 shape where "fields" additionalProperties allow
the new ModelField form (codecId, nullable) and "storage.fields" contains the
storage column mapping referencing ModelStorageField); update the relevant $defs
(e.g. $defs/ModelField and $defs/ModelStorageField) and replace the existing
plain "fields" object with a oneOf that enforces either legacy-only or
ADR-172-only structure (apply same change to the other occurrence around the
520-538 area).
In `@packages/2-sql/3-tooling/emitter/src/index.ts`:
- Around line 597-600: The emitted model type omits model-level ownership
because `modelParts` does not include an `owner` entry; update the code that
builds `modelParts` (the array used to produce `modelsType`) to append an
`owner` property derived from the model's ownership metadata (e.g., the runtime
contract's model.owner or equivalent metadata object) so that
Contract['models'][M]['owner'] is present in the generated type. Ensure the
`owner` entry uses the same serialization/formatting as the other entries in
`modelParts` so it integrates into the existing `storage`, `fields`, and
`relations` object literal emitted for each model.
- Around line 365-372: generateRootsType currently builds TypeScript object text
by interpolating raw keys/values which can produce invalid/unterminated strings
for keys like "line-items" or values with quotes; update generateRootsType to
call the existing serializeObjectKey(key) for each property name and
serializeValue(value, /*asType*/ true) (or the appropriate serializeValue
overload used elsewhere) for each value when constructing entries so keys are
properly quoted/escaped and values are escaped as TypeScript string literals;
keep the empty/undefined branch returning 'Record<string, string>' unchanged and
replace the mapping of Object.entries(roots).map(...) to use these two helpers
to produce `{ ${entries} }`.
---
Nitpick comments:
In @.agents/skills/multiline-commit-messages/SKILL.md:
- Line 10: Update the wording that currently says the heredoc pattern "fails
silently" when used inside the Shell tool; locate the sentence mentioning the
heredoc pattern (e.g., the text containing "`$(cat <<'EOF'`" and the phrase
"fails silently") and replace it with a more accurate phrase such as "fails to
produce the intended result" or "produces garbled output" so the description
accurately reflects the observed behavior.
- Line 36: Clarify the explanation to state that heredoc delimiters (the EOF
marker) can be ignored or treated as literal when used inside command
substitution like $(cat <<'EOF' ... EOF), so the shell may not interpret the
heredoc and the raw '$(cat <<'EOF' ...' text appears as the commit message;
update the sentence to explicitly say "heredoc delimiters are not recognized
inside command substitution $(...), causing the heredoc to be emitted verbatim"
and add a short remediation suggestion such as using a plain here-doc (cat
<<'EOF' >file ... EOF), printf, or writing the content to a temporary file
instead of embedding the heredoc inside $().
In `@packages/1-framework/1-core/shared/contract/src/domain-types.ts`:
- Around line 1-29: The exported object-shape type aliases (DomainField,
DomainRelationOn, DomainRelation, DomainDiscriminator, DomainModel) should be
converted to exported interfaces to follow the public API convention; replace
each "export type X = { ... }" with "export interface X { ... }", preserving
readonly modifiers, union literals (e.g. cardinality), optional properties (e.g.
on, discriminator, variants, base, owner) and existing property types
(Record<string,...>, readonly string[], etc.), and then run a quick project-wide
type check to ensure any imports/usages of these symbols still compile (adjust
exported references if any import expected a type alias).
In `@packages/1-framework/1-core/shared/contract/src/ir.ts`:
- Line 125: Replace the local conditional-spread helper with the shared
ifDefined utility: import ifDefined from '@prisma-next/utils/defined' and use it
where the inline conditional spread is applied (e.g., the expression involving
'roots' and opts.roots) so the object spread becomes ifDefined('roots',
opts.roots); remove the local helper definition and any duplicate logic to
ensure consistency across packages and satisfy the coding guideline.
In `@packages/2-sql/1-core/contract/src/validators.ts`:
- Around line 136-158: The model-related schemas currently allow unexpected
properties; update ModelFieldSchema, ModelStorageFieldSchema,
ModelStorageSchema, and ModelSchema to be strict by adding the constraint '+':
'reject' to each schema definition so any unknown properties are rejected during
validation; ensure you add the '+': 'reject' entry at the top-level of each
type(...) call for those four symbols to match the existing pattern used by
StorageColumnSchema, ExecutionSchema, and StorageSchema.
In `@packages/2-sql/1-core/contract/test/validate.test.ts`:
- Around line 770-989: The dual-format bridge tests (the describe('dual-format
bridge', ...) block exercising validateContract, SqlContract and SqlStorage)
should be moved out of validate.test.ts into a new test file (e.g.,
validate.bridge.test.ts or validate.normalization.test.ts); extract the entire
describe block and paste it into the new file, keep the same imports/usings for
validateContract/SqlContract/SqlStorage, remove the block from the original
file, run tests to ensure no missing imports/exports and update any test-runner
config if necessary so the new file is picked up; ensure test names and
top-level describe remain unchanged so assertions like expect(result.roots) and
mappings checks continue to run identically.
In `@packages/2-sql/2-authoring/contract-ts/src/contract.ts`:
- Around line 286-287: The code uses a blind double-cast "modelUnknown as
unknown as ModelDefinition" after normalizeContract(); instead implement a type
predicate to safely narrow the union: add a helper function like
isModelDefinition(x: unknown): x is ModelDefinition that checks distinguishing
properties (the SQL-specific fields present on ModelDefinition), then replace
the double-cast by calling isModelDefinition(modelUnknown) and, when true,
assign model = modelUnknown so TypeScript infers ModelDefinition without unsafe
casts; reference normalizeContract, ModelDefinition, and the local variable
modelUnknown when adding the predicate and using it.
In `@packages/2-sql/2-authoring/contract-ts/test/contract.structure.test.ts`:
- Around line 65-69: The test currently asserts normalization (that missing
models become {}) inside the validator-structure suite; instead, remove the
normalization check from this test and ensure the validator is given
already-normalized input. Change the test using
validateContract/SqlContract/SqlStorage to pass an input where models is
explicitly {} (not undefined) and assert validator behavior only, and add a
separate test in the normalization test file that verifies missing/undefined
models are normalized to {} by the normalization routine.
In
`@packages/2-sql/3-tooling/emitter/test/emitter-hook.generation.advanced.test.ts`:
- Around line 349-418: The test suite block titled "generates relations type
from models" (which calls sqlTargetFamilyHook.generateContractTypes) lives in an
oversized file and should be moved into a new focused test file for relations
(e.g., relations.test.ts) so the original
emitter-hook.generation.advanced.test.ts stays under 500 lines; extract all
relation-specific tests (including the assertions checking "export type
Relations" and the posts/comments expectation strings) into that new file, wire
up the same fixtures/helpers (createContractIR, testHashes, sqlTargetFamilyHook)
by importing them, and similarly split parameterized-renderer tests into a
separate parameterized.test.ts, updating imports and test exports accordingly so
each file addresses a single concern and the test runner still discovers them.
In `@test/integration/test/emitter-lanes.test.ts`:
- Around line 324-332: Add a direct assertion that the raw emitter output
(bootstrapJson) conforms to ADR-172 legacy shape before calling
validateContract(): after parsing bootstrap.contractJson and before
validateContract/bootstrap normalization, assert that bootstrapJson.models
exists and that each model has a fields property in the ADR-172 form (i.e.,
fields is an object mapping column names to column definitions, not the
normalized array/bridge shape). This assertion ensures the test verifies the
original emitter output produced the legacy models.*.fields structure prior to
validateContract().
🪄 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: e54f3191-f13c-4914-99c2-85d4810f9f66
⛔ Files ignored due to path filters (4)
projects/contract-domain-extraction/plan.mdis excluded by!projects/**projects/contract-domain-extraction/spec.mdis excluded by!projects/**projects/contract-domain-extraction/tsconfig.jsonis excluded by!projects/**projects/contract-domain-extraction/type-design.tsis excluded by!projects/**
📒 Files selected for processing (95)
.agents/skills/multiline-commit-messages/SKILL.md.agents/skills/write-architecture-docs/SKILL.mddocs/architecture docs/adrs/ADR 172 - Contract domain-storage separation.mddocs/architecture docs/adrs/ADR 173 - Polymorphism via discriminator and variants.mddocs/architecture docs/adrs/ADR 174 - Aggregate roots and relation strategies.mddocs/architecture docs/adrs/ADR 177 - Ownership replaces relation strategy.mddocs/architecture docs/subsystems/1. Data Contract.mddocs/architecture docs/subsystems/10. MongoDB Family.mddocs/glossary.mddocs/planning/mongo-target/1-design-docs/contract-symmetry.mddocs/planning/mongo-target/1-design-docs/design-questions.mddocs/planning/mongo-target/1-design-docs/mongo-poc-plan.mddocs/planning/mongo-target/cross-cutting-learnings.mddocs/reference/mongodb-feature-support-priorities.mddocs/reference/mongodb-user-journey.mddocs/reference/mongodb-user-promise.mdexamples/prisma-next-demo/src/prisma/contract.d.tsexamples/prisma-next-demo/src/prisma/contract.jsonpackages/1-framework/1-core/migration/control-plane/src/emission/canonicalization.tspackages/1-framework/1-core/migration/control-plane/src/emission/emit.tspackages/1-framework/1-core/shared/contract/package.jsonpackages/1-framework/1-core/shared/contract/src/domain-types.tspackages/1-framework/1-core/shared/contract/src/exports/types.tspackages/1-framework/1-core/shared/contract/src/exports/validate-domain.tspackages/1-framework/1-core/shared/contract/src/ir.tspackages/1-framework/1-core/shared/contract/src/types.tspackages/1-framework/1-core/shared/contract/src/validate-domain.tspackages/1-framework/1-core/shared/contract/test/domain-types.test.tspackages/1-framework/1-core/shared/contract/test/validate-domain.test.tspackages/1-framework/1-core/shared/contract/tsdown.config.tspackages/1-framework/3-tooling/emitter/test/emitter.test.tspackages/2-mongo-family/1-core/src/validate-domain.tspackages/2-sql/1-core/contract/src/construct.tspackages/2-sql/1-core/contract/src/exports/types.tspackages/2-sql/1-core/contract/src/types.tspackages/2-sql/1-core/contract/src/validate.tspackages/2-sql/1-core/contract/src/validators.tspackages/2-sql/1-core/contract/test/domain-types.test.tspackages/2-sql/1-core/contract/test/validate.test.tspackages/2-sql/2-authoring/contract-ts/schemas/data-contract-sql-v1.jsonpackages/2-sql/2-authoring/contract-ts/src/contract.tspackages/2-sql/2-authoring/contract-ts/test/contract.normalization.test.tspackages/2-sql/2-authoring/contract-ts/test/contract.structure.test.tspackages/2-sql/3-tooling/emitter/src/index.tspackages/2-sql/3-tooling/emitter/test/emitter-hook.generation.advanced.test.tspackages/2-sql/3-tooling/emitter/test/emitter-hook.generation.basic.test.tspackages/2-sql/3-tooling/emitter/test/emitter-hook.parameterized-types.test.tspackages/2-sql/3-tooling/family/test/contract-to-schema-ir.test.tspackages/2-sql/3-tooling/family/test/schema-verify.basic.test.tspackages/2-sql/4-lanes/kysely-lane/src/client.test.tspackages/2-sql/4-lanes/kysely-lane/src/plan.test.tspackages/2-sql/4-lanes/kysely-lane/src/where-expr.ast.test.tspackages/2-sql/4-lanes/kysely-lane/src/where-expr.test.tspackages/2-sql/4-lanes/kysely-lane/test/build-plan.collect-params.test.tspackages/2-sql/4-lanes/relational-core/test/schema.types.test-d.tspackages/2-sql/5-runtime/test/json-schema-validation.test.tspackages/2-sql/5-runtime/test/mutation-default-generators.test.tspackages/2-sql/5-runtime/test/parameterized-types.test.tspackages/2-sql/5-runtime/test/sql-context.test.tspackages/2-sql/5-runtime/test/sql-family-adapter.test.tspackages/2-sql/5-runtime/test/sql-runtime.test.tspackages/2-sql/5-runtime/test/utils.tspackages/3-extensions/postgres/test/build-only-api.test.tspackages/3-extensions/postgres/test/postgres.test.tspackages/3-targets/3-targets/postgres/test/migrations/fixtures/runner-fixtures.tspackages/3-targets/3-targets/postgres/test/migrations/planner.behavior.test.tspackages/3-targets/3-targets/postgres/test/migrations/planner.case1.test.tspackages/3-targets/3-targets/postgres/test/migrations/planner.contract-to-schema-ir.test.tspackages/3-targets/3-targets/postgres/test/migrations/planner.fk-config.test.tspackages/3-targets/3-targets/postgres/test/migrations/planner.reconciliation-unit.test.tspackages/3-targets/3-targets/postgres/test/migrations/planner.reconciliation.integration.test.tspackages/3-targets/3-targets/postgres/test/migrations/planner.reconciliation.test.tspackages/3-targets/3-targets/postgres/test/migrations/planner.referential-actions.test.tspackages/3-targets/3-targets/postgres/test/migrations/planner.semantic-satisfaction.test.tspackages/3-targets/3-targets/postgres/test/migrations/planner.storage-types.integration.test.tspackages/3-targets/3-targets/postgres/test/migrations/planner.storage-types.test.tspackages/3-targets/3-targets/postgres/test/migrations/schema-verify.after-runner.integration.test.tspackages/3-targets/6-adapters/postgres/test/test-utils.tstest/integration/test/authoring/parity/core-surface/expected.contract.jsontest/integration/test/authoring/parity/default-cuid-2/expected.contract.jsontest/integration/test/authoring/parity/default-dbgenerated/expected.contract.jsontest/integration/test/authoring/parity/default-nanoid-16/expected.contract.jsontest/integration/test/authoring/parity/default-nanoid/expected.contract.jsontest/integration/test/authoring/parity/default-pack-slugid/expected.contract.jsontest/integration/test/authoring/parity/default-ulid/expected.contract.jsontest/integration/test/authoring/parity/default-uuid-v4/expected.contract.jsontest/integration/test/authoring/parity/default-uuid-v7/expected.contract.jsontest/integration/test/authoring/parity/map-attributes/expected.contract.jsontest/integration/test/authoring/parity/pgvector-named-type/expected.contract.jsontest/integration/test/authoring/parity/relation-backrelation-list/expected.contract.jsontest/integration/test/budgets.test.tstest/integration/test/cli.emit-core.test.tstest/integration/test/codecs.test.tstest/integration/test/emitter-lanes.test.tstest/integration/test/sql-dml.test.ts
💤 Files with no reviewable changes (1)
- packages/2-sql/3-tooling/emitter/test/emitter-hook.parameterized-types.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/2-sql/1-core/contract/test/validate.test.ts (3)
1127-1143: Strengthen edge-case assertions beyond existence checks.A few tests only assert
toBeDefined(), which can pass even when normalization behavior regresses. Assert concrete transformed outputs (e.g., derived defaults/relations/fields) tied to each test title.As per coding guidelines:
**/{*.test.ts,*.test-d.ts,test/**,**/test/**}: "Avoid tautological tests that only restate fixture input - tests must verify behavior, not mirror object shape passed by the test itself."Also applies to: 1145-1162, 1671-1694
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/1-core/contract/test/validate.test.ts` around lines 1127 - 1143, The test currently only asserts the User model exists after calling normalizeContract, which is too weak; update the test using normalizeContract and the models['User'] object to assert concrete transformed outputs — e.g., verify that storage.table is absent or a specific default behavior is applied, that derived relations remain an empty object (relations === {} or has expected keys), and that the id field retains its normalized shape (nullable:false and codecId 'pg/int4@1') rather than just being defined; use the normalizeContract result (models['User']) and assert exact expected fields/values for defaults, relations, and field entries to prevent regressions.
859-1005: Keep normalization assertions in normalization-focused tests, not validator tests.These cases assert enrichment/bridge normalization through
validateContract(...). Consider moving those assertions tonormalizeContractsuites, and keepvalidateContracttests focused on validating already-normalized contracts.As per coding guidelines:
packages/**/*.test.ts: "When writing tests for contract normalization, test that the builder normalizes correctly. Tests for validators, parsers, and emitter hooks should test that they work with already-normalized contracts, NOT that they normalize."Also applies to: 1422-1450
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/1-core/contract/test/validate.test.ts` around lines 859 - 1005, The tests under validate.test.ts are asserting normalization/enrichment (fields like nullable/codecId/column, storage.fields, mappings.modelToTable/tableToModel, and top-level relations) using validateContract with oldFormatContract/newFormatContract; move those normalization-focused assertions into the normalization test suite and call normalizeContract (or the normalization helper) there instead, and change validateContract tests to only assert validation behavior against already-normalized inputs; update references accordingly for validateContract, normalizeContract, oldFormatContract, newFormatContract, and the assertions that inspect result.models, result.storage, result.mappings, and result.relations.
786-1696: Split this test file by concern to keep it maintainable.The file now spans far beyond the 500-line limit and contains multiple separable concerns (validateContract semantics, dual-format bridge, normalize edge cases). Please split into focused files (e.g.,
validate.mappings.test.ts,validate.bridge.test.ts,normalize.edge-cases.test.ts).As per coding guidelines:
**/*.test.ts: "Keep test files under 500 lines... Split test files when they exceed 500 lines [or] contain multiple distinct concerns..."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/1-core/contract/test/validate.test.ts` around lines 786 - 1696, The test file is too large and mixes multiple concerns; split the large describe groups into separate focused test files by concern: move the "dual-format bridge" describe block (tests using validateContract and SqlContract/SqlStorage), the "enrichNewFormatModels edge cases" and "normalizeContract edge cases" blocks (tests that call normalizeContract) and the "enrichOldFormatModels edge cases"/"detectFormat"/"normalizeStorage" blocks into their own test files respectively, keeping each file under 500 lines; ensure each new file imports the same helpers under test (validateContract, normalizeContract) and any types (SqlContract, SqlStorage) used in its tests and preserve all test names and expectations so behavior is unchanged and the test runner discovers the new files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/2-sql/1-core/contract/test/validate.test.ts`:
- Around line 1127-1143: The test currently only asserts the User model exists
after calling normalizeContract, which is too weak; update the test using
normalizeContract and the models['User'] object to assert concrete transformed
outputs — e.g., verify that storage.table is absent or a specific default
behavior is applied, that derived relations remain an empty object (relations
=== {} or has expected keys), and that the id field retains its normalized shape
(nullable:false and codecId 'pg/int4@1') rather than just being defined; use the
normalizeContract result (models['User']) and assert exact expected
fields/values for defaults, relations, and field entries to prevent regressions.
- Around line 859-1005: The tests under validate.test.ts are asserting
normalization/enrichment (fields like nullable/codecId/column, storage.fields,
mappings.modelToTable/tableToModel, and top-level relations) using
validateContract with oldFormatContract/newFormatContract; move those
normalization-focused assertions into the normalization test suite and call
normalizeContract (or the normalization helper) there instead, and change
validateContract tests to only assert validation behavior against
already-normalized inputs; update references accordingly for validateContract,
normalizeContract, oldFormatContract, newFormatContract, and the assertions that
inspect result.models, result.storage, result.mappings, and result.relations.
- Around line 786-1696: The test file is too large and mixes multiple concerns;
split the large describe groups into separate focused test files by concern:
move the "dual-format bridge" describe block (tests using validateContract and
SqlContract/SqlStorage), the "enrichNewFormatModels edge cases" and
"normalizeContract edge cases" blocks (tests that call normalizeContract) and
the "enrichOldFormatModels edge cases"/"detectFormat"/"normalizeStorage" blocks
into their own test files respectively, keeping each file under 500 lines;
ensure each new file imports the same helpers under test (validateContract,
normalizeContract) and any types (SqlContract, SqlStorage) used in its tests and
preserve all test names and expectations so behavior is unchanged and the test
runner discovers the new files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 514e58b0-8eb6-4108-a9fd-292a4857b106
📒 Files selected for processing (2)
packages/2-sql/1-core/contract/test/validate.test.tspackages/2-sql/1-core/contract/vitest.config.ts
✅ Files skipped from review due to trivial changes (1)
- packages/2-sql/1-core/contract/vitest.config.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/architecture docs/adrs/ADR 177 - Ownership replaces relation strategy.md (1)
248-248: Prefer a stable architecture reference instead of a planning-doc deep link.This ADR currently points to a planning artifact for core behavior (
Q19). Please replace it with a stable architecture/reference target (or summarize the rule inline) so the ADR remains durable as planning docs evolve.Based on learnings: “Do not reference transient project artifacts (e.g., under projects/ such as specs, plans, milestone documents) from durable system documentation (package READMEs, architecture/docs under docs/).”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/architecture` docs/adrs/ADR 177 - Ownership replaces relation strategy.md at line 248, The ADR currently links to a transient planning doc (design-questions.md § Q19); update the "Self-referential ownership" sentence in "ADR 177 - Ownership replaces relation strategy.md" to remove that deep link and either (a) replace it with a stable architecture reference (e.g., an ADR or docs page under docs/architecture) or (b) inline the rule text summarizing the behavior (e.g., "Self-referential ownership is rejected as circular; the anchor must be a non-self owner") so the ADR is durable; ensure you keep the example (User → Order → LineItem) and the phrase "Each owned model ... uses storage.relations" unchanged and remove the transient planning-doc URL.packages/2-sql/3-tooling/emitter/src/index.ts (1)
573-591: Consider usingserializeValuefor field name escaping consistency.The relation emission correctly implements the new domain format with
to,cardinality, andon.localFields/on.targetFields. However, lines 583-584 use raw string interpolation for field names:const localFields = on.localFields.map((f) => `'${f}'`).join(', '); const targetFields = on.targetFields.map((f) => `'${f}'`).join(', ');While field names typically follow identifier rules, using
serializeValuewould ensure consistent escaping if a field name ever contains special characters (e.g., quotes).That said, this matches the existing pattern used throughout the file (e.g., lines 402, 409, 674-675), so addressing this would require a broader refactor for consistency.
♻️ Optional: Use serializeValue for field names
- const localFields = on.localFields.map((f) => `'${f}'`).join(', '); - const targetFields = on.targetFields.map((f) => `'${f}'`).join(', '); + const localFields = on.localFields.map((f) => this.serializeValue(f)).join(', '); + const targetFields = on.targetFields.map((f) => this.serializeValue(f)).join(', ');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/3-tooling/emitter/src/index.ts` around lines 573 - 591, The current emission builds localFields/targetFields by manually wrapping names in quotes; replace that with calls to serializeValue to ensure proper escaping: in the loop over model.relations where relObj and on are handled, change the mapping for on.localFields and on.targetFields to use serializeValue(f) instead of `'\${f}'`, then join with ', ' and keep the rest of the constructed relParts and relations push intact; ensure serializeValue is imported/available in this module and that the resulting strings fit the existing readonly [...] templating.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/architecture` docs/adrs/ADR 177 - Ownership replaces relation
strategy.md:
- Line 248: Change the phrase "non-self owner" to use a compound hyphen
"non-self-owner" for consistency and readability in the sentence describing
self-referential ownership (the sentence that reads "the anchor must be a
non-self owner"); update that specific occurrence in ADR 177's text (and any
other identical occurrences of "non-self owner" in the same document) so the
wording reads "non-self-owner".
---
Nitpick comments:
In `@docs/architecture` docs/adrs/ADR 177 - Ownership replaces relation
strategy.md:
- Line 248: The ADR currently links to a transient planning doc
(design-questions.md § Q19); update the "Self-referential ownership" sentence in
"ADR 177 - Ownership replaces relation strategy.md" to remove that deep link and
either (a) replace it with a stable architecture reference (e.g., an ADR or docs
page under docs/architecture) or (b) inline the rule text summarizing the
behavior (e.g., "Self-referential ownership is rejected as circular; the anchor
must be a non-self owner") so the ADR is durable; ensure you keep the example
(User → Order → LineItem) and the phrase "Each owned model ... uses
storage.relations" unchanged and remove the transient planning-doc URL.
In `@packages/2-sql/3-tooling/emitter/src/index.ts`:
- Around line 573-591: The current emission builds localFields/targetFields by
manually wrapping names in quotes; replace that with calls to serializeValue to
ensure proper escaping: in the loop over model.relations where relObj and on are
handled, change the mapping for on.localFields and on.targetFields to use
serializeValue(f) instead of `'\${f}'`, then join with ', ' and keep the rest of
the constructed relParts and relations push intact; ensure serializeValue is
imported/available in this module and that the resulting strings fit the
existing readonly [...] templating.
🪄 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: d0141af9-103f-450d-97ef-e24c4c94f72b
📒 Files selected for processing (8)
.agents/skills/write-architecture-docs/SKILL.mddocs/architecture docs/adrs/ADR 172 - Contract domain-storage separation.mddocs/architecture docs/adrs/ADR 177 - Ownership replaces relation strategy.mddocs/glossary.mddocs/planning/mongo-target/1-design-docs/design-questions.mdpackages/1-framework/1-core/shared/contract/src/validate-domain.tspackages/2-sql/1-core/contract/src/validators.tspackages/2-sql/3-tooling/emitter/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/glossary.md
- packages/2-sql/1-core/contract/src/validators.ts
- .agents/skills/write-architecture-docs/SKILL.md
- docs/architecture docs/adrs/ADR 172 - Contract domain-storage separation.md
- packages/1-framework/1-core/shared/contract/src/validate-domain.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/2-sql/3-tooling/emitter/src/index.ts (1)
579-588: Consider using serialization helpers for consistency.The relation emission uses raw string interpolation for
to,cardinality, and field names, whilegenerateRootsTypenow properly escapes viaserializeValue(). Though model/field names are typically safe identifiers, using the helpers would ensure consistency.♻️ Suggested improvement
- if (relObj['to']) relParts.push(`readonly to: '${relObj['to']}'`); - if (relObj['cardinality']) - relParts.push(`readonly cardinality: '${relObj['cardinality']}'`); + if (relObj['to']) relParts.push(`readonly to: ${this.serializeValue(relObj['to'])}`); + if (relObj['cardinality']) + relParts.push(`readonly cardinality: ${this.serializeValue(relObj['cardinality'])}`); const on = relObj['on'] as { localFields?: string[]; targetFields?: string[] } | undefined; if (on?.localFields && on.targetFields) { - const localFields = on.localFields.map((f) => `'${f}'`).join(', '); - const targetFields = on.targetFields.map((f) => `'${f}'`).join(', '); + const localFields = on.localFields.map((f) => this.serializeValue(f)).join(', '); + const targetFields = on.targetFields.map((f) => this.serializeValue(f)).join(', ');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/3-tooling/emitter/src/index.ts` around lines 579 - 588, The relation emission block builds relParts with raw string interpolation for relObj['to'], relObj['cardinality'], and on.* field names which can cause inconsistent escaping; update the code in the emitter where relParts is assembled (the block referencing relObj and relParts and the on variable) to use the existing serialization helper (serializeValue) used by generateRootsType to escape/quote values consistently: call serializeValue(relObj['to']), serializeValue(relObj['cardinality']) and map on.localFields/on.targetFields through serializeValue before joining so emitted readonly to/cardinality/on entries are properly escaped and consistent with generateRootsType.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/2-sql/3-tooling/emitter/src/index.ts`:
- Around line 579-588: The relation emission block builds relParts with raw
string interpolation for relObj['to'], relObj['cardinality'], and on.* field
names which can cause inconsistent escaping; update the code in the emitter
where relParts is assembled (the block referencing relObj and relParts and the
on variable) to use the existing serialization helper (serializeValue) used by
generateRootsType to escape/quote values consistently: call
serializeValue(relObj['to']), serializeValue(relObj['cardinality']) and map
on.localFields/on.targetFields through serializeValue before joining so emitted
readonly to/cardinality/on entries are properly escaped and consistent with
generateRootsType.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 13a8dd57-498b-4019-b8e3-296e0b3337cb
⛔ Files ignored due to path filters (2)
projects/contract-domain-extraction/plan.mdis excluded by!projects/**projects/contract-domain-extraction/spec.mdis excluded by!projects/**
📒 Files selected for processing (2)
packages/2-sql/3-tooling/emitter/src/index.tspackages/2-sql/3-tooling/emitter/test/emitter-hook.generation.basic.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/2-sql/3-tooling/emitter/test/emitter-hook.generation.basic.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/2-sql/1-core/contract/src/validate.ts (2)
366-367: Silent fallback to column name when field mapping is missing.The fallback
sourceColToField[c] ?? creturns the column name as-is when no mapping exists. This could silently produce incorrectlocalFields/targetFieldsif a relation references a column that has no corresponding field (e.g., a FK column not exposed as a model field).Consider logging a warning when the fallback is used, or validating that all referenced columns have field mappings, to surface potential misconfigurations during development.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/1-core/contract/src/validate.ts` around lines 366 - 367, The current mapping silently falls back to the raw column name in localFields/targetFields (localFields = parentCols.map(c => sourceColToField[c] ?? c) and targetFields = childCols.map(c => targetColToField[c] ?? c)), which can hide missing field mappings; change this to validate each parentCols/childCols entry by checking sourceColToField and targetColToField and either throw or at minimum emit a clear warning identifying the unmapped column and the relation being processed (include relation id/name), then only use the mapped field value; ensure this logic is applied where localFields and targetFields are computed so missing mappings are surfaced during development.
440-456: Same silent fallback concern applies here.The
storageFields[f]?.['column'] ?? ... ?? fchain falls back to the field name when storage mapping isn't found. This mirrors the fallback pattern inenrichOldFormatModelsand could similarly mask missing column mappings for new-format contracts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/1-core/contract/src/validate.ts` around lines 440 - 456, The parentCols/childCols mapping silently falls back to the raw field name when no storage mapping exists; change the mapping so that when neither storageFields[f] nor sourceFields[f] (for parent) — and similarly when neither targetStorageFields[f] nor targetFields[f] (for child) — provide a 'column', you surface an explicit validation error (throw or return a ValidationError) that includes the relationship identifier (the `on` object) and the missing field name, instead of returning f; update the mapping logic inside the parentCols and childCols computation to perform this presence check and raise the error with context so missing storage mappings for new-format contracts are not silently masked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/2-sql/1-core/contract/src/validate.ts`:
- Around line 366-367: The current mapping silently falls back to the raw column
name in localFields/targetFields (localFields = parentCols.map(c =>
sourceColToField[c] ?? c) and targetFields = childCols.map(c =>
targetColToField[c] ?? c)), which can hide missing field mappings; change this
to validate each parentCols/childCols entry by checking sourceColToField and
targetColToField and either throw or at minimum emit a clear warning identifying
the unmapped column and the relation being processed (include relation id/name),
then only use the mapped field value; ensure this logic is applied where
localFields and targetFields are computed so missing mappings are surfaced
during development.
- Around line 440-456: The parentCols/childCols mapping silently falls back to
the raw field name when no storage mapping exists; change the mapping so that
when neither storageFields[f] nor sourceFields[f] (for parent) — and similarly
when neither targetStorageFields[f] nor targetFields[f] (for child) — provide a
'column', you surface an explicit validation error (throw or return a
ValidationError) that includes the relationship identifier (the `on` object) and
the missing field name, instead of returning f; update the mapping logic inside
the parentCols and childCols computation to perform this presence check and
raise the error with context so missing storage mappings for new-format
contracts are not silently masked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: fedc8ad2-08f2-45f6-9a08-2965d9718f4d
📒 Files selected for processing (2)
packages/2-sql/1-core/contract/src/validate.tspackages/2-sql/1-core/contract/test/validate.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/2-sql/1-core/contract/test/validate.test.ts
jkomyno
approved these changes
Apr 1, 2026
Spec captures the ADR 172 restructuring: widen ContractBase with domain structure (roots, models with typed fields and relations), update the SQL emitter, and bridge validateContract() so consumers are unaffected until Phase 2. Plan decomposes the work into four milestones: new contract structure, consumer migration, old field removal, and IR alignment.
The Shell tool does not reliably parse heredocs inside $(cat ...) for git commit -m. Use single-quoted strings with embedded newlines instead.
Complete type design covering all phases: new framework-level domain types, widened ContractBase and SqlContract, Phase 1 emitted shape, MongoContract alignment proof, and Phase 3 final types. Includes 25+ type-level assertions that verify structural compatibility with the existing codebase (tsc passes clean).
Prove that ContractBase.models (Record<string, DomainModel>) and SqlContract models (literal M) intersect correctly: domain-level properties are accessible, SQL-specific literal types are preserved, and SqlContract is assignable to ContractBase for cross-family consumers.
Introduce DomainField, DomainRelation, DomainModel in the framework contract package and widen ContractBase to carry roots and models. Add SqlModelStorage, SqlRelation SQL-specific domain types. Type tests verify SqlContract extends ContractBase, domain fields are accessible, and old consumer fields remain unchanged.
Move validateContractDomain and its types to @prisma-next/contract so the SQL family can share the same domain validation logic. Mongo re-exports from the new location; tests ported to framework.
normalizeContract() now detects old vs new (ADR 172) contract JSON format and enriches with the missing fields from the other format. Old-format inputs gain roots, domain fields (nullable, codecId), model.storage.fields and model.relations. New-format inputs gain model.fields[f].column and top-level relations. Integrate validateContractDomain after structural validation. Update arktype schemas to accept the widened model and contract shape.
ContractIR gains optional roots field. emit() includes roots in
the canonical contract and makes relations optional. The SQL emitter
hook now generates model fields with { column, nullable, codecId }
literal types, emits model.storage.fields and model.relations in
the domain format, and adds roots to the ContractBase type.
Include roots in canonicalized contract JSON output. Update parity expected contracts, demo contract/types, JSON schema, and round-trip tests to reflect the enriched ADR 172 format (roots, model.storage.fields, model.fields.codecId, domain-format model.relations). Relations are now optional in the framework emitter since document contracts may not have top-level relations.
Adding models: Record<string, DomainModel> to ContractBase created an intersection with SqlContract's M parameter, propagating an index signature that broke noPropertyAccessFromIndexSignature and caused 267+ type errors in consumers. Instead, only roots lives on ContractBase; domain model conformance is enforced at runtime by validateContractDomain. Also restores generateModelsType to emit JS codec types for model.fields (query builder compat) while keeping domain info in model.storage.fields.
ContractBase now requires roots: Record<string, string>. All test
contracts that construct SqlContract objects inline need roots: {}.
Also regenerates parity fixtures and demo contract to reflect the
corrected emitter output (JS codec types in model.fields, domain
info only in model.storage.fields).
Use a generic TModels parameter (defaulting to Record<string, DomainModel>) to satisfy the ADR 172 acceptance criterion without reintroducing index-signature leakage from noPropertyAccessFromIndexSignature. SqlContract passes its M through to ContractBase, removing the duplicate models property from the intersection.
- Q16: clarify that union field types apply directly to SQL JSON/JSONB columns, not just hypothetically - Q17: note that the SQL contract also has no M:N concept today - Q18: new question on whether 'strategy: embed/reference' violates the facts-not-instructions design principle (naming, not concept)
Introduce `owner` on models as a domain-level fact for aggregate membership, replacing `strategy: "reference" | "embed"` on relations. Relations become plain graph edges; the parent's `storage.relations` maps owned relations to physical locations. This resolves the tension that `strategy` read as an instruction rather than a fact, and that embedding was declared on the wrong object (the relation edge instead of the owned model itself).
Propagate ADR 177 across all architecture docs, design docs, glossary, reference material, and project specs. Replaces strategy: "embed"/"reference" with the owner model property and storage.relations mapping.
Analyzes three cases — self-referential reference (works), recursive embedding (logically sound, needs validation), and mixed root/embedded (correctly prevented by one-canonical-storage-location principle).
Nested ownership chains work structurally. Self-referential ownership is correctly rejected as circular. References Q19 for the full analysis.
Remove strategy from DomainRelation type, add owner to DomainModel, update domain validation requirements to include ownership rules, resolve the model.relations shape open question, and add ADR 177 to references.
Q1: model.storage.fields is { column: string } only — minimal bridge.
Q2: use localFields/targetFields naming from ADR 172.
Q3: emitter derives roots in Phase 1; IR supplies them in Phase 4.
Emitted contract.json now has pure domain fields ({ codecId } or
{ codecId, nullable: true }) with column exclusively in model.storage.fields.
The toDomainFields helper in emit.ts runs after normalizeContract enrichment
and only activates when fields have been enriched (have codecId).
Remove untyped (stripped as Record<string, unknown>)[\"roots\"] cast in constructContract — roots is now typed on ContractBase. Restructure the emitter-lanes round-trip test to bootstrap from raw IR then compare two validated emissions, removing the direct normalizeContract dependency.
… schema Add parentCols/childCols mapping-direction comment in enrichOldFormatModels. Extract extractDomainShape helper to replace inline type cast in validateContract. Add roots to contract-ts SqlContractSchema for runtime validation consistency. Remove verbose comment in cli.emit-core round-trip test.
Per ADR 177, relations are plain graph edges (to, cardinality, on?) with no strategy field. Component membership is declared on the model via owner. This updates DomainRelation, SqlRelation, DomainModelShape, validate-domain (ownership validation), JSON schema, arktype validators, and the emitter to strip strategy from emitted JSON.
Add ownership validation tests (self-ownership, non-existent owner, owned model in roots). Remove strategy from all relation test fixtures and assertions. Update DomainRelation and DomainModel test cases.
Demo contract and all 12 parity expected contracts regenerated to reflect ADR 177 — relations no longer carry strategy in emitted JSON.
Emitted contracts now have domain-format model.fields with codecId and optional nullable — column has moved to model.storage.fields. The schema must accept both old and new formats during the dual-format bridge.
Add edge case tests for normalizeContract/enrichNewFormatModels/ enrichOldFormatModels covering fallback branches for models without fields, relations without on.localFields/targetFields, unknown table relations, column name fallbacks, and storage semantic validation. Exclude pack-types.ts from coverage (pure type definitions).
- Replace placeholder ADR 177 link with real repo-relative path - Remove conflicting strategy reference in ADR 172 Implemented section - Add missing Post model to ADR 177 SQL example - Reword glossary: owned models have no independent storage unit - Add language identifiers to fenced code blocks - Escape pipe in markdown table cell
…sion - Replace in operator with Object.hasOwn() in validate-domain.ts - Add owner to ModelSchema in validators.ts - Use serializeObjectKey/serializeValue in generateRootsType - Emit owner property in generated model type
Insert new Milestone 3 (Mongo emitter hook with shared domain-level generation) as the forcing function for the shared generation API. Bump old M3 (Remove old fields) to M4, M4 (IR alignment) to M5, M5 (Emitter generalization) to M6 (SQL emitter migration). Update spec phases and acceptance criteria to match. Remove Mongo emitter from non-goals since it is now in scope as Phase 3. Linear: renamed project milestones P1/P2 to M1-M9, created M2-M6 milestones, created TML-2176 for M3, updated project description.
- Throw on duplicate column mappings in buildColumnToFieldMap to prevent silent wrong-field resolution in relation conversion - Exclude owned models from auto-derived roots in old-format enrichment to avoid guaranteed domain validation failure - Throw when a field references a non-existent column in a table that has columns defined, catching name mismatches early
Capabilities and profile hashes changed due to sql-builder consolidation (sql.returning capability added).
wmadden
force-pushed
the
tml-2172-contract-domain-extraction-implement-adr-172-domain-storage-separation
branch
from
997b14d to
0094f36
Compare
wmadden
force-pushed
the
tml-2172-contract-domain-extraction-implement-adr-172-domain-storage-separation
branch
from
2196adc to
0094f36
Compare
wmadden
deleted the
tml-2172-contract-domain-extraction-implement-adr-172-domain-storage-separation
branch
This was referenced Apr 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes TML-2172
Walkthrough — Contract Domain-Storage Separation (Milestone 1)
Key snippet (new capability)
Intent
Implement ADR 172's domain-storage separation and ADR 177's ownership model for the SQL contract — making the emitted JSON self-describing at the domain level, widening
ContractBaseto carry shared domain structure across families, adding model ownership as a domain fact, and bridgingvalidateContract()so consumers see both old and new fields without any implementation changes.Change map
Implementation:
ContractBasewidenedModelDefinitiongainsownernormalizeContract,extractDomainShaperootsaccessModelSchemagainsownergenerateRootsType, updatedgenerateModelsTypetoDomainModel,stripStrategyFromRelationsrootsin canonical orderingTests (evidence):
test/integration/test/authoring/parity/The story
Define the domain vocabulary. Introduce
DomainField,DomainRelation,DomainModeltypes in the framework contract package — the family-agnostic representation of what an application models.DomainModelincludes optionalowner(ADR 177) alongside existingbase/discriminator/variants.Widen
ContractBaseto carry domain structure. Addrootsandmodels(via aTModelsgeneric to preserve literal types) toContractBase.SqlContractpasses its model type through the generic, so consumers get precise property access on both domain and SQL-specific fields.Extract domain validation to the framework. Move
validateContractDomain()from mongo-core into the framework contract package. Add ownership validation rules (self-ownership, missing owner model, owned models in roots). Mongo re-exports to maintain its existing API.Build the dual-format normalization bridge. Rewrite
normalizeContract()to detect old vs new JSON format and enrich both directions — old-format contracts gain domain fields (nullable,codecId,model.relations,model.storage.fields,roots), and new-format contracts gain legacy fields (model.fields.column, top-levelrelations).Wire domain validation into SQL
validateContract(). CallvalidateContractDomain()viaextractDomainShape()helper before SQL-specific validation, establishing shared invariant checking across families.Update the SQL emitter to produce ADR 172 JSON. The emitter now:
roots(derived from models withstorage.table)model.fields({ codecId }+ optionalnullable) andmodel.storage.fields({ column })relationswithto,cardinality,on: { localFields, targetFields }— nostrategystrategyfrom IR relations before emission (ADR 177).d.tsgenerator produces both old and new type fields for backward compatibilityIntroduce ADR 177: ownership replaces strategy.
DomainModel.ownerdeclares component membership ("Address belongs to User"). Relations become plain graph edges — nostrategyannotation. The distinction between owned and referenced models is derivable from the target model'sownerproperty.storage.relationsmaps owned relations to physical locations (parallel tostorage.fields).Update emitter-output fixtures and resolve design questions. Parity expected contracts (12 files), the demo contract, JSON schema, and integration test fixtures updated to the new structure. All open questions in the spec resolved. Design questions documented.
Behavior changes & evidence
ContractBasenow carriesrootsandmodelswith domain-level types. Before:ContractBasewas a thin header. After:ContractBasehasroots: Record<string, string>andmodels: TModelswithDomainField,DomainRelation, optionalowner.Emitted JSON follows ADR 172 structure. Before:
model.fields: { column }, top-levelrelationswithparentCols/childCols. After:model.fields: { codecId },model.storage.fields: { column },model.relationswithlocalFields/targetFields,roots.toDomainModelModel ownership (ADR 177). Before:
strategy: "embed" | "reference"on relations. After:owner: "ModelName"on the model, relations are plain graph edges,strategystripped from emitted JSON.owner?: stringShared domain validation with ownership rules. Before: Domain validation only in mongo-core. After: Framework-level
validateContractDomain()validates roots, variants/bases, relations, discriminators, ownership, orphans — called by both SQL and Mongo validation.validateOwnershipDual-format bridge preserves backward compatibility. Before: Only old-format JSON supported. After: Both old and new formats accepted and enriched to produce the same widened runtime type.
normalizeContractCompatibility / migration / risk
src/directories unchanged. Onlyroots: {}additions to inline test contracts.strategydesign. Existing docs/examples updated.Follow-ups / open questions
Non-goals / intentionally out of scope
SqlContract(Milestone 3)Summary by CodeRabbit
Release Notes
Refactor
ownerdeclarations to express aggregate membership and data co-location.Documentation