TML-2753: route SQL marker/ledger writes through the control adapter SPI#712
Open
wmadden-electric wants to merge 29 commits into
Open
TML-2753: route SQL marker/ledger writes through the control adapter SPI#712wmadden-electric wants to merge 29 commits into
wmadden-electric wants to merge 29 commits into
Conversation
Refine the dispatch plan (4 dispatches: write SPI, invariant-merge, read/parser consolidation, raw-builder removal); pin the two slice open questions (ON CONFLICT upsert on both dialects; per-family SPI). Record the resumed implementer/reviewer in the registry. Signed-off-by: Will Madden <madden@prisma.io>
…ps-through-adapter
D1 R1 recon surfaced that the contract-free DML builder + TableSource schema-qualification + a Postgres text[] codec do not yet exist (slice 1 shipped DDL only). Intent-validation: these are in-project-scope enabling work (Project DoD + OQ3), not expansion. Pin updated_at to DB-side time (RawExpr), overriding a mirror-Mongo new Date() instinct that would change wire semantics. Signed-off-by: Will Madden <madden@prisma.io>
…ce for control writes (TML-2753)
Enabling surface for migrating SQL marker/ledger writes onto the adapter SPI:
- relational-core: TableSource.schema? threaded through named()/rewriteTableSource; InsertValue widened to admit RawExpr so DB-side time (now()/datetime('now')) can sit in value position; contract-free DML builder (insert/upsert/update with optional RETURNING) beside the DDL builder, no contract dependency.
- Postgres renderer qualifies "schema"."name" when a literal schema is set and no namespaceId is present; SQLite renderer asserts schema absence (control tables are unqualified).
- sql-runtime: createControlCodecRegistry + deriveParamMetadata/encodeParamsWithMetadata exports so contract-free control DML can encode params from AST-bound codecs.
- Postgres pg/text-array@1 codec for the marker invariants column.
Existing DML fixtures stay byte-identical (pnpm fixtures:check).
Signed-off-by: Will Madden <madden@prisma.io>
… SPI (TML-2753)
Add initMarker / updateMarker / writeLedgerEntry to SqlControlAdapter (interface + Postgres and SQLite implementations), mirroring MongoControlAdapter's method shapes. Each builds query-AST DML via the contract-free DML builder, lowers through adapter.lower() with no contract, and encodes params from AST-bound codecs before hitting the driver.
- Marker writes use INSERT ... ON CONFLICT (space) DO UPDATE SET ... (idempotent initMarker); updateMarker is a CAS UPDATE ... WHERE space = ? AND core_hash = expectedFrom RETURNING space, reporting the swap via row presence.
- updated_at is stamped DB-side (now() / datetime('now')) via RawExpr, not an app-side Date.
- Value codecs attached at the value site: pg jsonb for meta/contract_json/operations, pg text[] for invariants, pg timestamptz for updated_at; SQLite uses JSON-as-TEXT codecs.
Tests pin the lowered SQL on both dialects and exercise the writes end-to-end (PGlite for Postgres, node:sqlite in-memory for SQLite) with readback. The old raw-string write builders are left untouched.
Signed-off-by: Will Madden <madden@prisma.io>
…ps-through-adapter
…notes D1 R1 verdict SATISFIED (DC-4 PASS). Record the review carry-forwards: D2 computes invariant union/dedupe in the adapter (TS, uniform across dialects); D4 reconciles the marker column set at cut-over (legacy full-row rewrite vs new subset). F01 (trivial JSDoc) folded into D2. Signed-off-by: Will Madden <madden@prisma.io>
… internally (Option B) Hold the Mongo-symmetric updateMarker signature (project-DoD symmetric SPI + DC-4) by reading the current invariant set inside updateMarker under the existing lock (reusing the readMarker path), rather than threading it in as a param. Negligible cost; no signature leak of the storage difference. Signed-off-by: Will Madden <madden@prisma.io>
…753) updateMarker now reads the current invariant set via the existing readMarker SPI path (under the migration txn + advisory lock), unions it with the incoming set, dedupes, and sorts ascending in TS before writing back. The merge is uniform across dialects: SQLite — which stores invariants as JSON TEXT with no array operators — stops overwriting wholesale and converges on the same accumulate-dedupe behaviour as Postgres. The SPI signature is unchanged, so it stays byte-identical to MongoControlAdapter.updateMarker; the current-invariants source is an internal read, not a new parameter. No new locking is introduced. Also moves the misplaced lowering-helper JSDoc above execute() (it sat above const CONTROL_CODECS) in both marker-ledger-writes.ts files. Signed-off-by: Will Madden <madden@prisma.io>
Legacy SQLite runner already pre-merged invariants client-side, so the convergence is a layer relocation (runner -> updateMarker SPI) that preserves net observable behaviour rather than changing it. PR body to state accurately. Signed-off-by: Will Madden <madden@prisma.io>
…ns (Option 1) Preserve MarkerReadResult (runtime) and the control readMarker | null shape (SQL<->Mongo symmetry) by extracting one canonical read with thin typed projections, rather than widening the control SPI return type (Option 2, which ripples into D2/runner/CLI). Unify on the verify.ts parser superset. Signed-off-by: Will Madden <madden@prisma.io>
Collapse the duplicate SQL marker read paths onto a single canonical read flow and a single row parser, without changing any read semantics. - Add readMarkerResult(queryable, shape) to @prisma-next/family-sql/verify: probe -> select -> optional per-dialect decode -> the verify.ts parser -> MarkerReadResult. The runtime AdapterProfile.readMarker returns it verbatim (3-way no-table/absent/present preserved); the control SqlControlAdapter .readMarker wraps it in withMarkerReadErrorHandling and projects present -> record / else -> null, keeping its ContractMarkerRecord | null signature (and SQL<->Mongo readMarker symmetry). - Unify on the verify.ts parser superset and delete the sql-runtime marker.ts copy + its re-export; repoint both runtime reads. This decodes SQLite TEXT contract_json that the old runtime parser left as a string. - Share each dialect read via postgres/sqliteMarkerReadShape builders, and move decodeSqliteMarkerRow to the sqlite adapter so the runtime and control reads spell the read (and the invariants TEXT decode) exactly once. - Fold the SQLite runner private read into this.family.readMarker (mirrors the Postgres runner) and delete the now-dead readMarkerStatement read builder. Signed-off-by: Will Madden <madden@prisma.io>
…ps-through-adapter
Remove the legacy raw-string marker/ledger write builders and cut every in-scope write call site over to the D1 control-adapter SPI (initMarker / updateMarker / writeLedgerEntry): - Drop buildMergeMarkerStatements (postgres), buildWriteMarkerStatements (sqlite), their buildLedgerInsertStatement siblings, and the sql-runtime writeContractMarker builder, plus the exports/ re-exports and the tests that pinned their string shape. - Postgres and SQLite migration runners now call the family SPI; the upsert collapses to INSERT ... ON CONFLICT (space) DO UPDATE in initMarker, and CAS failures surface as MARKER_CAS_FAILURE. - SQLite runner no longer pre-merges invariants client-side; it passes the plan-provided invariants verbatim and lets updateMarker do the sole TS-side accumulate-dedupe (no double-merge, no overwrite). Net behaviour is preserved. - Marker column-set reconciled to the SPI surface (core_hash/profile_hash/updated_at/invariants), matching the Mongo adapter; contract_json/canonical_version/meta were write-only provenance not read by any typed consumer. Ledger contract_json snapshot columns are likewise no longer written. - Test seeders move to a raw-SQL seedTestMarker helper in sql-runtime test utils; ledger tests assert the now-null snapshot columns. Signed-off-by: Will Madden <madden@prisma.io>
Signed-off-by: Will Madden <madden@prisma.io>
…ps-through-adapter Signed-off-by: Will Madden <madden@prisma.io>
Contributor
|
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 PR moves contract marker and ledger writes from runtime SQL builders to control-plane adapter methods, introduces a composable contract-free SQL query builder surface with raw-expression support, adds target-specific table sources and column helpers, and implements dialect-specific marker persistence with compare-and-swap semantics and delegated ledger recording. ChangesSQL control-plane refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
@prisma-next/extension-author-tools
@prisma-next/mongo-runtime
@prisma-next/family-mongo
@prisma-next/sql-runtime
@prisma-next/family-sql
@prisma-next/extension-arktype-json
@prisma-next/middleware-cache
@prisma-next/mongo
@prisma-next/extension-paradedb
@prisma-next/extension-pgvector
@prisma-next/extension-postgis
@prisma-next/postgres
@prisma-next/sql-orm-client
@prisma-next/sqlite
@prisma-next/target-mongo
@prisma-next/adapter-mongo
@prisma-next/driver-mongo
@prisma-next/contract
@prisma-next/utils
@prisma-next/config
@prisma-next/errors
@prisma-next/framework-components
@prisma-next/operations
@prisma-next/ts-render
@prisma-next/contract-authoring
@prisma-next/ids
@prisma-next/psl-parser
@prisma-next/psl-printer
@prisma-next/cli
@prisma-next/cli-telemetry
@prisma-next/emitter
@prisma-next/migration-tools
prisma-next
@prisma-next/vite-plugin-contract-emit
@prisma-next/mongo-codec
@prisma-next/mongo-contract
@prisma-next/mongo-value
@prisma-next/mongo-contract-psl
@prisma-next/mongo-contract-ts
@prisma-next/mongo-emitter
@prisma-next/mongo-schema-ir
@prisma-next/mongo-query-ast
@prisma-next/mongo-orm
@prisma-next/mongo-query-builder
@prisma-next/mongo-lowering
@prisma-next/mongo-wire
@prisma-next/sql-contract
@prisma-next/sql-errors
@prisma-next/sql-operations
@prisma-next/sql-schema-ir
@prisma-next/sql-contract-psl
@prisma-next/sql-contract-ts
@prisma-next/sql-contract-emitter
@prisma-next/sql-lane-query-builder
@prisma-next/sql-relational-core
@prisma-next/sql-builder
@prisma-next/target-postgres
@prisma-next/target-sqlite
@prisma-next/adapter-postgres
@prisma-next/adapter-sqlite
@prisma-next/driver-postgres
@prisma-next/driver-sqlite
commit: |
size-limit report 📦
|
![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: 4
🧹 Nitpick comments (3)
packages/2-sql/9-family/src/core/control-adapter.ts (1)
91-95: ⚡ Quick winAlign the
updateMarkercontract docs with the new invariant-merge behavior.This comment still says callers must pre-merge invariants and that the adapter writes them verbatim. The PR contract moved union/dedupe into
updateMarker, so leaving this as-is will mislead future adapter implementations into dropping previously recorded invariants.🤖 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 91 - 95, The docstring for updateMarker is inaccurate: it currently claims callers must pre-merge invariants and that destination.invariants are written verbatim; update it to state that updateMarker performs the union/dedupe merge of invariants itself (so callers do not need to pre-merge), that it atomically advances the marker via compare-and-swap on core_hash = expectedFrom, returns true/false as described, and that when destination.invariants is omitted existing invariants are left untouched; also mirror this clarified contract in the comment referencing MongoControlAdapter.updateMarker to avoid misleading future adapter implementations.packages/3-targets/3-targets/postgres/src/core/codecs.ts (1)
188-190: 💤 Low valueConsider stricter validation in
decodeJsonfor data integrity.The current implementation returns an empty array for non-array JSON and coerces all entries to strings. While this provides resilience, it could mask data corruption if the marker's
invariantsJSON is malformed (e.g.,null,{}, or an array with unexpected types).Since this is control-plane data, consider throwing an error for non-array input to surface corruption early, rather than silently recovering with an empty array.
🔍 Stricter alternative
decodeJson(json: JsonValue): readonly string[] { - return Array.isArray(json) ? json.map((entry) => String(entry)) : []; + if (!Array.isArray(json)) { + throw new Error(`PgTextArrayCodec.decodeJson: expected array, got ${typeof json}`); + } + return json.map((entry) => { + if (typeof entry !== 'string') { + throw new Error(`PgTextArrayCodec.decodeJson: expected string entry, got ${typeof entry}`); + } + return entry; + });🤖 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/codecs.ts` around lines 188 - 190, The decodeJson function currently coerces any non-array input to [] and casts entries to String, which can hide corrupted control-plane data; update decodeJson in codecs.ts to validate that the incoming JsonValue is an array and that every entry is a string (or explicitly convertible if you choose) and throw a descriptive error (e.g., TypeError or a custom validation error) when the input is not an array or when any element is not a string, returning readonly string[] only after successful validation so corruption surfaces early.packages/3-targets/3-targets/postgres/src/core/migrations/runner.ts (1)
593-630: ⚡ Quick winAdd a regression test for
MARKER_CAS_FAILURE.This branch is new and safety-critical, but none of the provided Postgres runner tests exercise the
updateMarker() === falsepath or assert that it aborts before ledger writes. A small runner test here would lock down both the failure code/metadata and the rollback behavior. As per coding guidelines,**/*.test.{ts,tsx}: Always write tests before creating or modifying implementation.🤖 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/runner.ts` around lines 593 - 630, The tests are missing a regression that exercises the updateMarker() === false branch in upsertMarker and asserts MARKER_CAS_FAILURE and rollback behavior; add a unit/integration test (matching **/*.test.{ts,tsx}) that sets up a Postgres runner, seeds an existing ContractMarkerRecord, then simulates a concurrent modification so family.updateMarker returns false (or mock family.updateMarker to return false) and verifies upsertMarker returns a runner failure with code 'MARKER_CAS_FAILURE', the failure meta contains space/expectedStorageHash/destinationStorageHash, and that no ledger writes were committed (i.e., migration aborted before ledger operations); target the upsertMarker method and use family.initMarker/family.updateMarker control to reproduce the path.
🤖 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/2-sql/4-lanes/relational-core/src/ast/types.ts`:
- Line 83: InsertValue was extended to include RawExpr but the exported alias
AnyInsertValue still omits RawExpr; update the AnyInsertValue type alias (the
exported public alias) to include RawExpr so the public type surface matches the
AST/runtime behavior—locate the AnyInsertValue alias declaration and add RawExpr
to its union alongside ColumnRef, ParamRef, PreparedParamRef, and
DefaultValueExpr.
- Around line 327-348: The TableSource constructor and its factory
TableSource.named currently allow both schema and namespaceId to be set; add a
validation in the TableSource constructor (and keep it effective for
TableSource.named which calls the constructor) that enforces the mutual
exclusivity: if both schema and namespaceId are non-empty/defined, throw an
Error (or assert) with a clear message identifying the invalid combination
(e.g., "TableSource: schema and namespaceId are mutually exclusive"). This
ensures any caller constructing a TableSource (via new TableSource(...) or
TableSource.named(...)) will fail fast on invalid input.
In `@packages/2-sql/5-runtime/README.md`:
- Line 130: Update the README to reflect the new runtime surface: change the
`readContractMarker` bullet to state it only provides marker read statements
(remove any claim that writes are provided by sql-runtime), and add entries in
the "Codecs" section for the exported functions `createControlCodecRegistry`,
`deriveParamMetadata`, and `encodeParamsWithMetadata` (briefly describe each and
note they are exported from src/exports/index.ts) so the docs match the actual
exports.
In `@packages/2-sql/9-family/src/core/control-instance.ts`:
- Around line 604-608: The sign() logic uses readMarker() then calls
controlAdapter.initMarker(...) which currently upserts and can overwrite a
concurrent signer; change the behavior so initMarker is insert-if-absent (atomic
insert) or make initMarker return a distinct result/error when a row already
exists and have sign() detect that and fall back to updateMarker()/conflict
handling instead of assuming success; update references in sign(),
existingMarker, and controlAdapter.initMarker to implement atomic insert-or-fail
semantics (or propagate “already exists” so sign() can retry/merge).
---
Nitpick comments:
In `@packages/2-sql/9-family/src/core/control-adapter.ts`:
- Around line 91-95: The docstring for updateMarker is inaccurate: it currently
claims callers must pre-merge invariants and that destination.invariants are
written verbatim; update it to state that updateMarker performs the union/dedupe
merge of invariants itself (so callers do not need to pre-merge), that it
atomically advances the marker via compare-and-swap on core_hash = expectedFrom,
returns true/false as described, and that when destination.invariants is omitted
existing invariants are left untouched; also mirror this clarified contract in
the comment referencing MongoControlAdapter.updateMarker to avoid misleading
future adapter implementations.
In `@packages/3-targets/3-targets/postgres/src/core/codecs.ts`:
- Around line 188-190: The decodeJson function currently coerces any non-array
input to [] and casts entries to String, which can hide corrupted control-plane
data; update decodeJson in codecs.ts to validate that the incoming JsonValue is
an array and that every entry is a string (or explicitly convertible if you
choose) and throw a descriptive error (e.g., TypeError or a custom validation
error) when the input is not an array or when any element is not a string,
returning readonly string[] only after successful validation so corruption
surfaces early.
In `@packages/3-targets/3-targets/postgres/src/core/migrations/runner.ts`:
- Around line 593-630: The tests are missing a regression that exercises the
updateMarker() === false branch in upsertMarker and asserts MARKER_CAS_FAILURE
and rollback behavior; add a unit/integration test (matching **/*.test.{ts,tsx})
that sets up a Postgres runner, seeds an existing ContractMarkerRecord, then
simulates a concurrent modification so family.updateMarker returns false (or
mock family.updateMarker to return false) and verifies upsertMarker returns a
runner failure with code 'MARKER_CAS_FAILURE', the failure meta contains
space/expectedStorageHash/destinationStorageHash, and that no ledger writes were
committed (i.e., migration aborted before ledger operations); target the
upsertMarker method and use family.initMarker/family.updateMarker control to
reproduce the path.
🪄 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: 9c8b3e2a-8132-4400-80d4-90d30cbfbc81
⛔ Files ignored due to path filters (3)
projects/migrate-marker-ledger-to-typed-query-ast-commands/slices/sql-marker-ops-through-adapter/plan.mdis excluded by!projects/**projects/migrate-marker-ledger-to-typed-query-ast-commands/slices/sql-marker-ops-through-adapter/spec.mdis excluded by!projects/**projects/migrate-marker-ledger-to-typed-query-ast-commands/subagent-registry.mdis excluded by!projects/**
📒 Files selected for processing (43)
packages/2-sql/4-lanes/relational-core/src/ast/types.tspackages/2-sql/4-lanes/relational-core/src/contract-free/dml.tspackages/2-sql/4-lanes/relational-core/src/exports/contract-free.tspackages/2-sql/5-runtime/README.mdpackages/2-sql/5-runtime/src/codecs/control-codec-registry.tspackages/2-sql/5-runtime/src/exports/index.tspackages/2-sql/5-runtime/src/marker.tspackages/2-sql/5-runtime/src/sql-marker.tspackages/2-sql/5-runtime/test/sql-marker.test.tspackages/2-sql/5-runtime/test/utils.tspackages/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/types.tspackages/2-sql/9-family/src/core/verify.tspackages/2-sql/9-family/src/exports/verify.tspackages/3-targets/3-targets/postgres/src/core/codec-ids.tspackages/3-targets/3-targets/postgres/src/core/codecs.tspackages/3-targets/3-targets/postgres/src/core/migrations/runner.tspackages/3-targets/3-targets/postgres/src/core/migrations/statement-builders.tspackages/3-targets/3-targets/postgres/src/exports/statement-builders.tspackages/3-targets/3-targets/postgres/test/migrations/statement-builders.test.tspackages/3-targets/3-targets/sqlite/src/core/migrations/runner.tspackages/3-targets/3-targets/sqlite/src/core/migrations/statement-builders.tspackages/3-targets/3-targets/sqlite/src/exports/statement-builders.tspackages/3-targets/6-adapters/postgres/src/core/adapter.tspackages/3-targets/6-adapters/postgres/src/core/control-adapter.tspackages/3-targets/6-adapters/postgres/src/core/marker-ledger-writes.tspackages/3-targets/6-adapters/postgres/src/core/sql-renderer.tspackages/3-targets/6-adapters/postgres/test/marker-ledger-writes.test.tspackages/3-targets/6-adapters/postgres/test/migrations/marker-ledger-writes.integration.test.tspackages/3-targets/6-adapters/postgres/test/migrations/runner.errors.integration.test.tspackages/3-targets/6-adapters/postgres/test/migrations/runner.ledger.integration.test.tspackages/3-targets/6-adapters/sqlite/src/core/adapter.tspackages/3-targets/6-adapters/sqlite/src/core/control-adapter.tspackages/3-targets/6-adapters/sqlite/src/core/marker-ledger-writes.tspackages/3-targets/6-adapters/sqlite/test/marker-ledger-writes.test.tspackages/3-targets/6-adapters/sqlite/test/migrations/runner.errors.test.tspackages/3-targets/6-adapters/sqlite/test/migrations/runner.ledger.test.tstest/integration/test/cli.db-verify.aggregate-schema.test.tstest/integration/test/cli.db-verify.e2e.test.tstest/integration/test/family.sign-database.test.tstest/integration/test/family.verify-database.basic.test.tstest/integration/test/family.verify-database.errors.test.ts
💤 Files with no reviewable changes (6)
- packages/2-sql/5-runtime/src/marker.ts
- packages/3-targets/3-targets/sqlite/src/exports/statement-builders.ts
- packages/3-targets/3-targets/postgres/src/exports/statement-builders.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/statement-builders.ts
- packages/2-sql/5-runtime/src/sql-marker.ts
- packages/3-targets/3-targets/sqlite/src/core/migrations/statement-builders.ts
Align AnyInsertValue with InsertValue (RawExpr), enforce TableSource schema/namespaceId exclusivity, refresh sql-runtime README, and route sign() through insert-only insertMarker so concurrent first stamps cannot overwrite via the migration-runner upsert path. Signed-off-by: Will Madden <madden@prisma.io>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/3-targets/6-adapters/postgres/test/marker-ledger-writes.test.ts (1)
26-43: ⚡ Quick winVerify the
invariantsparameter default.The test checks the first three params but doesn't verify
params[7](theinvariantsparameter). Since theinitMarkertest verifies invariants when provided (line 66), this test should verify that invariants defaults to an empty array when omitted.🧪 Suggested assertion to add
expect(params[0]).toBe('app'); expect(params[1]).toBe('sha256:core'); expect(params[2]).toBe('sha256:prof'); + expect(params[7]).toEqual([]); });🤖 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/test/marker-ledger-writes.test.ts` around lines 26 - 43, Add an assertion to verify that when calling adapter.insertMarker (in the test "insertMarker lowers to a plain insert with DB-side updated_at") the invariants parameter defaults to an empty array: after extracting { sql, params } = driver.calls[0]!, assert that params[7] is an empty array (e.g., []). This ensures the insertMarker invocation without provided invariants behaves the same as the initMarker test that verifies provided invariants.packages/3-targets/6-adapters/sqlite/src/core/marker-ledger-writes.ts (1)
170-172: ⚡ Quick winPrefer
ifDefinedfor conditional spreads.The inline ternary spread
...(mergedInvariants !== undefined ? { invariants: ... } : {})can be replaced withifDefined('invariants', mergedInvariants)for consistency with repo patterns.♻️ Refactor to use ifDefined
+import { ifDefined } from '`@prisma-next/utils/defined`'; core_hash: param(destination.storageHash, { codecId: SQLITE_TEXT_CODEC_ID }), profile_hash: param(destination.profileHash, { codecId: SQLITE_TEXT_CODEC_ID }), updated_at: NOW, - ...(mergedInvariants !== undefined - ? { invariants: param(mergedInvariants, { codecId: SQLITE_JSON_CODEC_ID }) } - : {}), + ...ifDefined( + 'invariants', + mergedInvariants !== undefined + ? param(mergedInvariants, { codecId: SQLITE_JSON_CODEC_ID }) + : undefined + ), },Based on learnings: In prisma/prisma-next code, prefer
ifDefinedfromprisma-next/utils/definedfor conditional object spreads instead of inline ternary-based spreads (e.g., when building option objects forParamRef.ofor similar codec/param construction).🤖 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/sqlite/src/core/marker-ledger-writes.ts` around lines 170 - 172, Replace the inline ternary-based conditional spread that adds an invariants param with the repository pattern using ifDefined: when building the object passed to ParamRef.of (where you currently have ...(mergedInvariants !== undefined ? { invariants: param(mergedInvariants, { codecId: SQLITE_JSON_CODEC_ID }) } : {})), import and call ifDefined('invariants', param(mergedInvariants, { codecId: SQLITE_JSON_CODEC_ID })) so the invariants property is only added when mergedInvariants is defined; keep the existing param(...) and SQLITE_JSON_CODEC_ID usage intact and remove the ternary/spread expression.
🤖 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.
Nitpick comments:
In `@packages/3-targets/6-adapters/postgres/test/marker-ledger-writes.test.ts`:
- Around line 26-43: Add an assertion to verify that when calling
adapter.insertMarker (in the test "insertMarker lowers to a plain insert with
DB-side updated_at") the invariants parameter defaults to an empty array: after
extracting { sql, params } = driver.calls[0]!, assert that params[7] is an empty
array (e.g., []). This ensures the insertMarker invocation without provided
invariants behaves the same as the initMarker test that verifies provided
invariants.
In `@packages/3-targets/6-adapters/sqlite/src/core/marker-ledger-writes.ts`:
- Around line 170-172: Replace the inline ternary-based conditional spread that
adds an invariants param with the repository pattern using ifDefined: when
building the object passed to ParamRef.of (where you currently have
...(mergedInvariants !== undefined ? { invariants: param(mergedInvariants, {
codecId: SQLITE_JSON_CODEC_ID }) } : {})), import and call
ifDefined('invariants', param(mergedInvariants, { codecId: SQLITE_JSON_CODEC_ID
})) so the invariants property is only added when mergedInvariants is defined;
keep the existing param(...) and SQLITE_JSON_CODEC_ID usage intact and remove
the ternary/spread expression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: a4db194f-f351-4ebf-8cc2-f0b62fc22863
📒 Files selected for processing (10)
packages/2-sql/4-lanes/relational-core/src/ast/types.tspackages/2-sql/5-runtime/README.mdpackages/2-sql/9-family/src/core/control-adapter.tspackages/2-sql/9-family/src/core/control-instance.tspackages/3-targets/6-adapters/postgres/src/core/control-adapter.tspackages/3-targets/6-adapters/postgres/src/core/marker-ledger-writes.tspackages/3-targets/6-adapters/postgres/test/marker-ledger-writes.test.tspackages/3-targets/6-adapters/sqlite/src/core/control-adapter.tspackages/3-targets/6-adapters/sqlite/src/core/marker-ledger-writes.tspackages/3-targets/6-adapters/sqlite/test/marker-ledger-writes.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/3-targets/6-adapters/postgres/src/core/marker-ledger-writes.ts
- packages/2-sql/5-runtime/README.md
- packages/2-sql/4-lanes/relational-core/src/ast/types.ts
- packages/3-targets/6-adapters/sqlite/test/marker-ledger-writes.test.ts
- packages/2-sql/9-family/src/core/control-adapter.ts
- packages/3-targets/6-adapters/postgres/src/core/control-adapter.ts
wmadden
requested changes
Jun 3, 2026
Comment on lines
+327
to
+336
| /** | ||
| * Literal storage schema this table lives in, used by control-plane DML | ||
| * against fixed tables (e.g. `prisma_contract.marker`) that have no contract | ||
| * namespace coordinate. When set, the Postgres renderer qualifies the table | ||
| * as `schema.name` (each part quoted independently); SQLite asserts it is | ||
| * absent, since its control tables are unqualified. `schema` and | ||
| * `namespaceId` are mutually exclusive — `namespaceId` is the contract-bound | ||
| * coordinate, `schema` the contract-free literal. | ||
| */ | ||
| readonly schema: string | undefined; |
Contributor
There was a problem hiding this comment.
This is invalid here. This is generic SQL core, Postgres-specific AST additions should be modeled as AST node subclasses, not shoved into the generic shared base like this. You even added a comment explaining the layering violation.
| * control-plane tables that have no contract namespace coordinate | ||
| * (`prisma_contract.marker`, `_prisma_ledger`). | ||
| */ | ||
| export function tableRef(name: string, options?: { readonly schema?: string }): TableSource { |
Contributor
There was a problem hiding this comment.
Again, it's not valid to have schema at this layer
| @@ -1,4 +1,5 @@ | |||
| import type { ContractMarkerRecord } from '@prisma-next/contract/types'; | |||
| import type { MarkerReadResult, MarkerStatement } from '@prisma-next/sql-relational-core/ast'; | |||
…d for corrective D5+D6 (TML-2753) Operator caught 3 architectural mistakes in PR #712 that escaped the build loop (implementer + subagent reviewer both SATISFIED): 1. TableSource.schema? added to generic SQL core (Postgres-specific field on shared base); implementer added a code comment acknowledging the layering violation and shipped. 2. Read path is a leaky template-method orchestrator in family-sql/verify.ts (MarkerStatement / MarkerReadShape / readMarkerResult) that takes adapter implementation-detail fragments via an interface, asymmetric with the write SPI we got right in the same slice. 3. sign()+upsert race: collapsing marker write to INSERT…ON CONFLICT DO UPDATE is correct for the migration runner but wrong for sign() (silently overwrites another signer). CodeRabbit caught; fixed at 5da812a via new insertMarker primitive. Retro landings (per drive-run-retro): - projects/migrate-marker-ledger-to-typed-query-ast-commands/retros.md: new retro entry. - drive/calibration/failure-modes.md: F16 (self-acknowledged layering violation), F17 (brief frames win as mechanics), F18 (inverted abstraction over adapter fragments), F19 (single-primitive collapse changes semantics for some callers). - drive/calibration/dor.md: 3 new Dispatch-DoR overlay items (property statement alongside mechanic; per-caller contract enumeration; generic trace-callers reviewer prompt). - drive/code/README.md: 3 new repo-specific code-review smells. - .agents/rules/no-target-branches.mdc: new section "AST class fields are the same violation" with worked PostgresTableSource example. Spec revised with corrective scope section + 3 new done conditions. Plan revised with D5 (revert TableSource.schema?, introduce PostgresTableSource) + D6 (delete MarkerStatement/Shape/Queryable/readMarkerResult; each adapter owns readMarker end-to-end). Both land on the same branch; PR #712 stays open. Signed-off-by: Will Madden <madden@prisma.io>
…ma? from generic core (TML-2753) Signed-off-by: Will Madden <madden@prisma.io>
…footgun, TML-2753) D5 relocated `freeze()` from the `TableSource` constructor into the `named()` static factory so `PostgresTableSource` can assign `schema` after `super()`. All current production callers go through `named()` (which freezes) or construct a subclass (which freezes itself) — but the public constructor remained, leaving unfrozen instances reachable by any future `new TableSource(...)` caller. Making the constructor `protected` closes the footgun structurally — the only callers that need direct construction are `TableSource.named()` itself (inside the class) and `PostgresTableSource` (via `super()`); TS now rejects any external `new TableSource(...)`. The same invariant the freeze-in-constructor pattern gave us, hoisted into the type system rather than relying on runtime audit. D5 R1 reviewer flagged this as F02 (low, recommend tag-along). No runtime change; no semantic change for existing callers; no test changes needed. Signed-off-by: Will Madden <madden@prisma.io>
…ment/MarkerReadShape/readMarkerResult (TML-2753) Move marker read orchestration into per-adapter marker-read.ts (probe → select → decode → parse → tag) so the family layer only shares parseContractMarkerRow. Removes the inverted MarkerReadShape/readMarkerResult abstraction from family-sql verify. Signed-off-by: Will Madden <madden@prisma.io>
…3 close, TML-2753) F03 close from D6 R1. readContractMarker had zero production callers — the adapter-owned readMarker SPI is the canonical read path post-D6. MarkerStatement was shape-identical to SqlStatement with only this one dead consumer. Signed-off-by: Will Madden <madden@prisma.io>
Contributor
Author
Corrective scope landed — re-requesting reviewFollowing the operator's The three findings + their fixes
Validation
Drive lessons landed durably
Optional carry-overs (operator's call — happy to land on this branch if you want)
If you want either folded in before merge, say the word and I'll dispatch. Process noteTwo orchestrator-level calibration misses surfaced this round and are being recorded:
Re-requesting review. |
…tro followup) Caught in this slice when the orchestrator started editing in-place to close F03 (a residual MarkerStatement consumer that the D6 brief had wrongly excluded), instead of dispatching an implementer. Operator interrupted with the role-reminder; edits were reverted and F03 was re-dispatched cleanly as D7 (composer-2.5-fast), landing at e3c4c44 with a proper closed-file-list brief. F20 records the symptom, the self-justifications that license the drift, the detection signals (no implementer dispatch transcript preceding an orchestrator-authored production commit), the mitigation (the split is a role constraint, not an efficiency tradeoff; calibration docs are inside the role, production source is not), and the revert-before-redispatch protocol when caught in-flight. Signed-off-by: Will Madden <madden@prisma.io>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
drive/calibration/failure-modes.md (1)
285-399:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDuplicate failure-mode numbering creates ambiguous references.
### F15appears twice (first at Line 267, then again at Line 285), so anchors and cross-references to “F15” are ambiguous. Please renumber the second occurrence and its subsequent entries to keep the catalogue strictly monotonic.
As per coding guidelines: “**/*.md: Keep docs current (READMEs, rules, links).”🤖 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 `@drive/calibration/failure-modes.md` around lines 285 - 399, The file has a duplicated section heading "### F15" (the Behavioural reports-all entry starting at the second appearance) which makes references ambiguous; renumber that second "### F15" and every subsequent "### F16"→"### F17", "### F17"→"### F18", etc., incrementing each failure-mode heading after the duplicate so the catalogue numbers are strictly monotonic and update any internal cross-references that mention those symbols to match the new numbers.packages/3-targets/6-adapters/sqlite/src/core/adapter.ts (1)
208-236:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject Postgres schema-qualified table sources in the SQLite renderer.
PostgresTableSourcestill flows through this path because it inheritskind === 'table-source'. After removing the old schema check here, passing apgTableRef({ schema: 'x', name: 'y' })into SQLite now lowers to just"y"whennamespaceIdis unset, so the adapter silently drops the Postgres-only coordinate instead of failing fast.Suggested fix
+function hasSchemaCoordinate( + table: Pick<TableSource, 'name' | 'namespaceId'>, +): table is Pick<TableSource, 'name' | 'namespaceId'> & { readonly schema: string | undefined } { + return 'schema' in table; +} + function qualifyTableFromNamespaceCoordinate( table: Pick<TableSource, 'name' | 'namespaceId'>, contract: SqliteContract, ): string { + if (hasSchemaCoordinate(table) && table.schema !== undefined) { + throw new Error('SQLite renderer does not support schema-qualified table sources'); + } if (table.namespaceId === undefined) { return quoteIdentifier(table.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/3-targets/6-adapters/sqlite/src/core/adapter.ts` around lines 208 - 236, The SQLite renderer currently accepts Postgres schema-qualified table sources (e.g. pgTableRef with a schema) because PostgresTableSource shares kind === 'table-source', causing qualifyTableFromNamespaceCoordinate / renderTableSource to silently drop the schema; update the renderer to detect and reject Postgres-style schema-qualified sources and throw a clear error instead. Specifically, in renderTableSource (or at the start of qualifyTableFromNamespaceCoordinate) add a check for PostgresTableSource (e.g. a schema property or the Postgres-specific discriminant on the source) and if a schema is present call throw new Error("Postgres schema-qualified table sources are not supported by the SQLite renderer") (mentioning pgTableRef or PostgresTableSource in the message), otherwise proceed with the existing logic that uses qualifyTableFromNamespaceCoordinate and quoteIdentifier. Ensure the new check runs before returning the unqualified table name so schema-qualified Postgres references fail fast.
🧹 Nitpick comments (1)
packages/3-targets/6-adapters/postgres/src/core/control-adapter.ts (1)
343-365: ⚡ Quick winClarify
currentInvariantshandling in PostgresupdateMarker
markerLedgerWrites.updateMarkeromitsinvariantsfrom the UPDATESETclause whendestination.invariants === undefined(it setsmergedInvariantstoundefinedand only adds{ invariants: ... }whenmergedInvariants !== undefined). So passing[]forcurrentInvariantscan’t wipe existing invariants.Optional: add a short comment near the
currentInvariantscalculation incontrol-adapter.tsnoting thatcurrentInvariantsis only used whendestination.invariantsis provided.🤖 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 343 - 365, The current logic in updateMarker computes currentInvariants as [] when destination.invariants is undefined which prevents markerLedgerWrites.updateMarker from clearing existing invariants; change the computation so currentInvariants is undefined when destination.invariants === undefined (i.e., only call await this.readMarker(...) and supply invariants when destination.invariants is provided), and add a short inline comment by the currentInvariants calculation in control-adapter.ts noting that currentInvariants is only used when destination.invariants is present and must be undefined to allow the downstream update to remove invariants.
🤖 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 `@drive/code/README.md`:
- Around line 13-20: Update the README links that point to the mirror path
".cursor/rules/as-contract-cast-smell.mdc" to use the canonical
".agents/rules/as-contract-cast-smell.mdc" path instead; locate the reference in
the paragraph that starts with "`as Contract` cast bypasses the family
`ContractSerializer` seam" and replace the .cursor link target with the
.agents/rules equivalent so the documentation follows the repo policy about
canonical rule locations.
---
Outside diff comments:
In `@drive/calibration/failure-modes.md`:
- Around line 285-399: The file has a duplicated section heading "### F15" (the
Behavioural reports-all entry starting at the second appearance) which makes
references ambiguous; renumber that second "### F15" and every subsequent "###
F16"→"### F17", "### F17"→"### F18", etc., incrementing each failure-mode
heading after the duplicate so the catalogue numbers are strictly monotonic and
update any internal cross-references that mention those symbols to match the new
numbers.
In `@packages/3-targets/6-adapters/sqlite/src/core/adapter.ts`:
- Around line 208-236: The SQLite renderer currently accepts Postgres
schema-qualified table sources (e.g. pgTableRef with a schema) because
PostgresTableSource shares kind === 'table-source', causing
qualifyTableFromNamespaceCoordinate / renderTableSource to silently drop the
schema; update the renderer to detect and reject Postgres-style schema-qualified
sources and throw a clear error instead. Specifically, in renderTableSource (or
at the start of qualifyTableFromNamespaceCoordinate) add a check for
PostgresTableSource (e.g. a schema property or the Postgres-specific
discriminant on the source) and if a schema is present call throw new
Error("Postgres schema-qualified table sources are not supported by the SQLite
renderer") (mentioning pgTableRef or PostgresTableSource in the message),
otherwise proceed with the existing logic that uses
qualifyTableFromNamespaceCoordinate and quoteIdentifier. Ensure the new check
runs before returning the unqualified table name so schema-qualified Postgres
references fail fast.
---
Nitpick comments:
In `@packages/3-targets/6-adapters/postgres/src/core/control-adapter.ts`:
- Around line 343-365: The current logic in updateMarker computes
currentInvariants as [] when destination.invariants is undefined which prevents
markerLedgerWrites.updateMarker from clearing existing invariants; change the
computation so currentInvariants is undefined when destination.invariants ===
undefined (i.e., only call await this.readMarker(...) and supply invariants when
destination.invariants is provided), and add a short inline comment by the
currentInvariants calculation in control-adapter.ts noting that
currentInvariants is only used when destination.invariants is present and must
be undefined to allow the downstream update to remove invariants.
🪄 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: 1057064f-332b-4907-b60f-c03b333ce5b2
⛔ Files ignored due to path filters (3)
projects/migrate-marker-ledger-to-typed-query-ast-commands/retros.mdis excluded by!projects/**projects/migrate-marker-ledger-to-typed-query-ast-commands/slices/sql-marker-ops-through-adapter/plan.mdis excluded by!projects/**projects/migrate-marker-ledger-to-typed-query-ast-commands/slices/sql-marker-ops-through-adapter/spec.mdis excluded by!projects/**
📒 Files selected for processing (23)
.agents/rules/no-target-branches.mdcdrive/calibration/dor.mddrive/calibration/failure-modes.mddrive/code/README.mdpackages/2-sql/4-lanes/relational-core/src/ast/adapter-types.tspackages/2-sql/4-lanes/relational-core/src/ast/types.tspackages/2-sql/4-lanes/relational-core/src/contract-free/dml.tspackages/2-sql/5-runtime/README.mdpackages/2-sql/5-runtime/src/exports/index.tspackages/2-sql/5-runtime/src/sql-marker.tspackages/2-sql/5-runtime/test/sql-marker.test.tspackages/3-targets/3-targets/postgres/src/contract-free/dml.tspackages/3-targets/3-targets/postgres/src/core/ast/table-source.tspackages/3-targets/3-targets/postgres/src/exports/contract-free.tspackages/3-targets/3-targets/postgres/test/contract-free/dml.test.tspackages/3-targets/6-adapters/postgres/src/core/adapter.tspackages/3-targets/6-adapters/postgres/src/core/control-adapter.tspackages/3-targets/6-adapters/postgres/src/core/marker-ledger-writes.tspackages/3-targets/6-adapters/postgres/src/core/marker-read.tspackages/3-targets/6-adapters/postgres/src/core/sql-renderer.tspackages/3-targets/6-adapters/sqlite/src/core/adapter.tspackages/3-targets/6-adapters/sqlite/src/core/control-adapter.tspackages/3-targets/6-adapters/sqlite/src/core/marker-read.ts
💤 Files with no reviewable changes (3)
- packages/2-sql/4-lanes/relational-core/src/ast/adapter-types.ts
- packages/2-sql/5-runtime/test/sql-marker.test.ts
- packages/2-sql/5-runtime/src/sql-marker.ts
✅ Files skipped from review due to trivial changes (1)
- packages/3-targets/3-targets/postgres/test/contract-free/dml.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/3-targets/6-adapters/postgres/src/core/sql-renderer.ts
- packages/2-sql/5-runtime/src/exports/index.ts
- packages/2-sql/4-lanes/relational-core/src/contract-free/dml.ts
- packages/3-targets/6-adapters/postgres/src/core/marker-ledger-writes.ts
Comment on lines
13
to
+20
| - **`as Contract` cast bypasses the family `ContractSerializer` seam.** Any `JSON.parse(...) as Contract` (or `as Contract<…>`) in `packages/**/src/**` outside the allowlist is a serializer-bypass smell — see [`.cursor/rules/as-contract-cast-smell.mdc`](../../.cursor/rules/as-contract-cast-smell.mdc). The replacement idiom is `familyInstance.deserializeContract(JSON.parse(raw) as unknown)`. Originally surfaced by `TML-2536`. | ||
|
|
||
| - **Self-acknowledged layering violation in source.** A code comment that explicitly admits to a known anti-pattern (`layering violation`, `branch on target`, `leaky abstraction`, `bypasses the seam`, `we shouldn't do this but`, `temporary`, `TODO: this is wrong`) is *itself* the must-fix finding — flag it as `must-fix` and stop reviewing the surrounding diff until the operator confirms whether the shape is correct. Reviewers should grep the diff for these phrases as part of every review pass. The implementer's acknowledgment is the symptom; the underlying structural problem is the actual finding. See [F16 in `drive/calibration/failure-modes.md`](../calibration/failure-modes.md#f16-self-acknowledged-layering-violation-shipped-through-review). Originally surfaced by `TML-2753`. | ||
|
|
||
| - **Inverted abstraction: shared template-method orchestrator over adapter fragments.** A type named `<Operation>Shape` / `<Operation>Statements` / `<Operation>Spec` in `family-sql` (or any shared layer) carrying SQL fragments + row decoders populated by each adapter, plus a function that takes a queryable + that shape and runs the operation — that's the adapter exporting its implementation details upstream instead of owning the operation. The adapter exists to hide those details. Right shape: each adapter owns `<operation>(driver, args)` end-to-end; only *pure* helpers (parsers, row-shape schemas) are shared. 10–20 lines of "duplicated" orchestration between two adapters is the right kind of duplication — it's what the adapter pattern is for. Symmetry check: if the *write* side or the sibling *Mongo* family owns the operation end-to-end, the read side or SQL side must too. See [F18 in `drive/calibration/failure-modes.md`](../calibration/failure-modes.md#f18-inverted-abstraction-shared-orchestrator-in-family-layer-takes-adapter-implementation-detail-fragments-via-an-interface). Originally surfaced by `TML-2753`. | ||
|
|
||
| - **Single-primitive collapse without per-caller contract trace.** A diff that collapses two distinct operations into one primitive (`initMarker` becomes upsert, replacing separate insert + update; `merge` replaces separate overwrite + accumulate) must explicitly enumerate every caller of either pre-collapse operation and confirm the post-collapse semantic still satisfies each caller's contract. Look for: caller A's tests pinning idempotent-re-apply (passes under upsert), caller B's tests pinning fail-loudly-on-duplicate (might still pass sequentially while failing under concurrency). The mitigation is either keeping both operations or adding an explicit variant (e.g. `insertMarker` alongside upsert-`initMarker`). See [F19 in `drive/calibration/failure-modes.md`](../calibration/failure-modes.md#f19-single-primitive-collapse-changes-semantics-for-some-callers-but-not-others). Originally surfaced by `TML-2753`. | ||
|
|
Contributor
There was a problem hiding this comment.
Link rule references to canonical .agents/rules paths, not mirror paths.
This section links to .cursor/rules/as-contract-cast-smell.mdc; per repo policy, .agents/rules/<rule>.mdc is the canonical home and mirror paths are presentation-only. Please switch the link target to the .agents/rules path.
As per coding guidelines: “Rules canonical home lives in .agents/rules/<rule-name>.mdc with symlink presentation mirrors at .cursor/rules/ and .claude/rules/.”
🤖 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 `@drive/code/README.md` around lines 13 - 20, Update the README links that
point to the mirror path ".cursor/rules/as-contract-cast-smell.mdc" to use the
canonical ".agents/rules/as-contract-cast-smell.mdc" path instead; locate the
reference in the paragraph that starts with "`as Contract` cast bypasses the
family `ContractSerializer` seam" and replace the .cursor link target with the
.agents/rules equivalent so the documentation follows the repo policy about
canonical rule locations.
wmadden
reviewed
Jun 3, 2026
| - [ ] If touching `packages/0-shared` or `packages/1-framework-core`, downstream package builds named as "done when" gates. | ||
| - [ ] If the dispatch adds a new public type, the dependent packages' typecheck is named. | ||
| - [ ] Brief's "Outcome" section includes a *property statement* alongside the mechanical instruction (*"such that <invariant the change preserves>"*). If the brief authorises adding a field / collapsing operations / consolidating implementations, it must also name the architectural property the change preserves (target-agnostic core, adapter ownership of an operation, caller's contract semantics, etc.) — not just the mechanic. See [F17 in `failure-modes.md`](./failure-modes.md#f17-dispatch-brief-frames-the-win-as-mechanics-implementer--reviewer-ship-wrong-shape-work-that-satisfies-it). (Added by `TML-2753` retro.) | ||
| - [ ] If the dispatch **collapses two distinct operations into one primitive**, brief enumerates every caller of either pre-collapse operation and the contract each caller depends on (idempotent / fail-loudly / CAS / etc.). The dispatch closes only when every contract is satisfied; otherwise the brief keeps both operations or adds a deliberate variant. See [F19 in `failure-modes.md`](./failure-modes.md#f19-single-primitive-collapse-changes-semantics-for-some-callers-but-not-others). (Added by `TML-2753` retro.) |
Contributor
There was a problem hiding this comment.
This seems over-prescriptive. Our Drive principles say we don't enumerate all files a dispatch will touch. We should be able to assert that the implementer must remove all references of the collapsed operations instead.
Contributor
There was a problem hiding this comment.
This is incorrectly named "control". This doesn't rely on the contract but it can be in theory utilized in either plane
Comment on lines
+56
to
+57
| import * as markerLedgerWrites from './marker-ledger-writes'; | ||
| import * as markerRead from './marker-read'; |
Contributor
There was a problem hiding this comment.
Why are these in separate files?
Comment on lines
+48
to
+51
| /** A single-row `INSERT INTO <table> (…) VALUES (…)`. */ | ||
| export function insert(table: TableSource, row: Readonly<Record<string, InsertValue>>): InsertAst { | ||
| return InsertAst.into(table).withRows([row]); | ||
| } |
Contributor
There was a problem hiding this comment.
Wtf is the point of this insert() function? It just hides the query builder this module is supposed to be providing
Contributor
There was a problem hiding this comment.
This code is unreadable crap. Why isn't it using the query builder we just created
wmadden
requested changes
Jun 3, 2026
Comment on lines
+15
to
+32
| const exists = await driver.query( | ||
| 'select 1 from information_schema.tables where table_schema = $1 and table_name = $2', | ||
| ['prisma_contract', 'marker'], | ||
| ); | ||
| if (exists.rows.length === 0) { | ||
| return { kind: 'no-table' }; | ||
| } | ||
|
|
||
| const result = await driver.query( | ||
| 'select core_hash, profile_hash, contract_json, canonical_version, updated_at, app_tag, meta, invariants from prisma_contract.marker where space = $1', | ||
| [space], | ||
| ); | ||
| const row = result.rows[0]; | ||
| if (!row) { | ||
| return { kind: 'absent' }; | ||
| } | ||
|
|
||
| return { kind: 'present', record: parseContractMarkerRow(row) }; |
Contributor
There was a problem hiding this comment.
WTF is this? Why isn't this using the query builder we just wrote?:
…ive D8/D9/D10 (TML-2753)
After D5/D6/D7 closed SATISFIED, operator review surfaced a deeper architectural miss: the contract-free authoring surface (relational-core/src/contract-free/dml.ts) is not actually a query builder. Current insert(table, row) / update({...}) / upsert({...}) helpers are option-bag wrappers around the AST class chain that already exists; marker writes that use them thread codec IDs / table names / column refs at every leaf; marker reads use raw SQL strings. Net: the slice migrated marker/ledger to typed AST literally (the AST is used) but not in spirit (the authoring is unreadable).
Root cause: D1 brief framed the win as "use the typed AST" instead of "deliver a fluent authoring surface analogous to sql-builder in spirit". composer-2.5-fast pattern-cloned Slice 1 atom-constructors (col/lit/fn, correct for DDL) into DML (which needs a chainable builder). F17 property-statement discipline did not catch this because the property statement named the transport, not the ergonomics.
Landed: F19 DoR overlay rewritten property-based per operator dor.md:45 critique (no more file enumeration; assert "no reference to pre-collapse operations survives" instead); F21 added to failure-modes (implementer ships AST construction by hand wrapped in option-bag factories instead of building the fluent surface); retros.md round-2 entry; slice spec + plan revised with D8/D9/D10 corrective scope and 5 new done conditions; implementer slot for D8 onwards upgraded from composer-2.5-fast to claude-4.6-sonnet-high-thinking.
Signed-off-by: Will Madden <madden@prisma.io>
…ML (D8, TML-2753) Replace the option-bag factory functions in relational-core/contract-free/dml.ts with a genuine fluent authoring surface. The old exports (insert/update/upsert/tableRef/ excludedColumn/dbExpr) are kept as @deprecated re-exports so the build stays green during D8; D9 will delete the call sites and remove them. Design decisions ---------------- **Chain shape.** Each operation starts from a TableHandle and chains immutably: marker.update() .set({ core_hash: newHash }) .where(marker.space.eq(space).and(marker.core_hash.eq(expectedFrom))) .returning(marker.space) .build() .set/.where/.returning all return new builder instances. .build() produces the existing InsertAst/UpdateAst/SelectAst node the lower pipeline already consumes. **Column proxies.** ColumnProxy objects are exposed as keyed properties of TableHandle (marker.space, marker.core_hash). Each proxy carries its codecId and nullable flag baked in at declaration time. Expression methods (.eq, .neq, .isNull, .isNotNull) produce CfExpr — a thin composable wrapper over AnyExpression with .and/.or/.not — without any per-call-site codec or column-name threading. **Value encoding.** Plain JS values passed to .set() or .insert() are automatically wrapped in ParamRef with the column's known codec. Existing AnyExpression values (ColumnRef, RawExpr) are detected via a toExpr() type guard and passed through without re-wrapping. **Table declaration.** The generic table(source, schema) factory in relational-core accepts a TableSource and a ColumnSchema (Record<string, ColumnDescriptor>). Target packages (postgres, sqlite) provide dialect-specific column-type helpers (text(), int4(), timestamptz() etc.) that return ColumnDescriptor objects with the correct codec ID, plus pgTable()/sqliteTable() wrappers that supply the right TableSource subclass. **ExcludedProxy.** The .onConflict().doUpdate(callback) overload passes an ExcludedProxy<Schema> to the callback, giving type-safe access to excluded.<column> ColumnRefs for DO UPDATE SET clauses. **Spirit vs literal alignment.** sql-builder uses a JS Proxy for table resolution because the table set is only known at contract-bind time. Here the table is declared once as a module-level const; a plain object spread with a blindCast at the return boundary is correct by construction, no Proxy needed. What survived from old dml.ts ------------------------------- param() — re-exported unchanged as a non-deprecated primitive. tableRef/excludedColumn/dbExpr/insert/upsert/update — re-exported with @deprecated JSDoc; D9 removes their call sites. Signed-off-by: Will Madden <madden@prisma.io>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/2-sql/4-lanes/relational-core/src/contract-free/table.ts`:
- Around line 255-256: The bug is that table() always passes source.name into
makeColumnProxy so aliased tables (e.g., pgTable(..., { alias })) produce
ColumnRef entries with the base table name; update the proxies creation in
table() (and the similar block around the 278-296 range) to use the source alias
when present (e.g., source.alias or similar property) instead of source.name
when calling makeColumnProxy so generated ColumnRef qualifiers reflect the
alias; locate the loop that assigns proxies[col] = makeColumnProxy(source.name,
col, desc) and change the first argument to the effective qualifier (alias if
defined, otherwise source.name) consistently wherever column proxies are
created.
- Around line 289-290: The eq and neq branches currently convert null into a
ParamRef via toSetExpression and produce BinaryExpr.eq/ref, which yields SQL
like "column = NULL" instead of proper IS NULL semantics; update the eq and neq
handlers in the table contract so that when value is null (or explicitly a
nullable sentinel) they return a CfExpr wrapping an IsNullExpression or
IsNotNullExpression (rather than BinaryExpr.eq/neq), otherwise keep the existing
new CfExpr(BinaryExpr.eq(ref, toSetExpression(...))) behaviour; reference the
eq, neq functions, CfExpr, BinaryExpr, ref and toSetExpression symbols when
making this change.
In `@packages/2-sql/4-lanes/relational-core/src/exports/contract-free.ts`:
- Around line 13-24: Remove the deprecated DML shims from this module's public
export list: delete dbExpr, excludedColumn, insert, tableRef, update, and upsert
from the exported identifiers in contract-free.ts so they are no longer
re-exported from this package; leave the other type exports (ExcludedProxy,
TableHandle, TableInsertRow, TableSetValues) and helpers (param, table) intact
and do not add new backwards-compat exports pointing at ../contract-free/dml.
🪄 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: 0d70b7f8-149f-4dfb-ad79-89ac76a5c189
⛔ Files ignored due to path filters (3)
projects/migrate-marker-ledger-to-typed-query-ast-commands/retros.mdis excluded by!projects/**projects/migrate-marker-ledger-to-typed-query-ast-commands/slices/sql-marker-ops-through-adapter/plan.mdis excluded by!projects/**projects/migrate-marker-ledger-to-typed-query-ast-commands/slices/sql-marker-ops-through-adapter/spec.mdis excluded by!projects/**
📒 Files selected for processing (12)
drive/calibration/dor.mddrive/calibration/failure-modes.mdpackages/2-sql/4-lanes/relational-core/src/contract-free/dml.tspackages/2-sql/4-lanes/relational-core/src/contract-free/table.tspackages/2-sql/4-lanes/relational-core/src/exports/contract-free.tspackages/2-sql/4-lanes/relational-core/test/contract-free/table.test.tspackages/3-targets/3-targets/postgres/src/contract-free/columns.tspackages/3-targets/3-targets/postgres/src/exports/contract-free.tspackages/3-targets/3-targets/postgres/test/contract-free/columns.test.tspackages/3-targets/3-targets/sqlite/src/contract-free/columns.tspackages/3-targets/3-targets/sqlite/src/exports/contract-free.tspackages/3-targets/3-targets/sqlite/test/contract-free/columns.test.ts
✅ Files skipped from review due to trivial changes (4)
- packages/3-targets/3-targets/sqlite/src/exports/contract-free.ts
- packages/3-targets/3-targets/sqlite/test/contract-free/columns.test.ts
- drive/calibration/dor.md
- drive/calibration/failure-modes.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/3-targets/3-targets/postgres/src/exports/contract-free.ts
Comment on lines
+255
to
+256
| for (const [col, desc] of Object.entries(schema)) { | ||
| proxies[col] = makeColumnProxy(source.name, col, desc); |
Contributor
There was a problem hiding this comment.
Preserve aliases when creating column proxies.
table() always feeds source.name into makeColumnProxy, so an aliased source still emits ColumnRefs against the base table name. This is reachable through pgTable(..., { alias }), and any WHERE / RETURNING built from those proxies will reference the wrong qualifier.
Suggested fix
export function table<Schema extends ColumnSchema>(
source: TableSource,
schema: Schema,
): TableHandle<Schema> {
const proxies: Record<string, ColumnProxy> = {};
+ const refTableName = source.alias ?? source.name;
for (const [col, desc] of Object.entries(schema)) {
- proxies[col] = makeColumnProxy(source.name, col, desc);
+ proxies[col] = makeColumnProxy(source.name, refTableName, col, desc);
}
@@
function makeColumnProxy(
tableName: string,
+ refTableName: string,
columnName: string,
desc: ColumnDescriptor,
): ColumnProxy {
- const ref = ColumnRef.of(tableName, columnName);
+ const ref = ColumnRef.of(refTableName, columnName);
return {
codecId: desc.codecId,
nullable: desc.nullable,Also applies to: 278-296
🤖 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/4-lanes/relational-core/src/contract-free/table.ts` around
lines 255 - 256, The bug is that table() always passes source.name into
makeColumnProxy so aliased tables (e.g., pgTable(..., { alias })) produce
ColumnRef entries with the base table name; update the proxies creation in
table() (and the similar block around the 278-296 range) to use the source alias
when present (e.g., source.alias or similar property) instead of source.name
when calling makeColumnProxy so generated ColumnRef qualifiers reflect the
alias; locate the loop that assigns proxies[col] = makeColumnProxy(source.name,
col, desc) and change the first argument to the effective qualifier (alias if
defined, otherwise source.name) consistently wherever column proxies are
created.
Comment on lines
+289
to
+290
| eq: (value) => new CfExpr(BinaryExpr.eq(ref, toSetExpression(value, desc))), | ||
| neq: (value) => new CfExpr(BinaryExpr.neq(ref, toSetExpression(value, desc))), |
Contributor
There was a problem hiding this comment.
Don't lower eq(null) and neq(null) to binary operators.
These branches currently turn null into a ParamRef, so the AST becomes column = NULL / column <> NULL. In SQL that does not behave like IS NULL / IS NOT NULL, so nullable filters will quietly return the wrong rows.
Suggested fix
- eq: (value) => new CfExpr(BinaryExpr.eq(ref, toSetExpression(value, desc))),
- neq: (value) => new CfExpr(BinaryExpr.neq(ref, toSetExpression(value, desc))),
+ eq: (value) =>
+ value === null
+ ? new CfExpr(NullCheckExpr.isNull(ref))
+ : new CfExpr(BinaryExpr.eq(ref, toSetExpression(value, desc))),
+ neq: (value) =>
+ value === null
+ ? new CfExpr(NullCheckExpr.isNotNull(ref))
+ : new CfExpr(BinaryExpr.neq(ref, toSetExpression(value, desc))),📝 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.
Suggested change
| eq: (value) => new CfExpr(BinaryExpr.eq(ref, toSetExpression(value, desc))), | |
| neq: (value) => new CfExpr(BinaryExpr.neq(ref, toSetExpression(value, desc))), | |
| eq: (value) => | |
| value === null | |
| ? new CfExpr(NullCheckExpr.isNull(ref)) | |
| : new CfExpr(BinaryExpr.eq(ref, toSetExpression(value, desc))), | |
| neq: (value) => | |
| value === null | |
| ? new CfExpr(NullCheckExpr.isNotNull(ref)) | |
| : new CfExpr(BinaryExpr.neq(ref, toSetExpression(value, desc))), |
🤖 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/4-lanes/relational-core/src/contract-free/table.ts` around
lines 289 - 290, The eq and neq branches currently convert null into a ParamRef
via toSetExpression and produce BinaryExpr.eq/ref, which yields SQL like "column
= NULL" instead of proper IS NULL semantics; update the eq and neq handlers in
the table contract so that when value is null (or explicitly a nullable
sentinel) they return a CfExpr wrapping an IsNullExpression or
IsNotNullExpression (rather than BinaryExpr.eq/neq), otherwise keep the existing
new CfExpr(BinaryExpr.eq(ref, toSetExpression(...))) behaviour; reference the
eq, neq functions, CfExpr, BinaryExpr, ref and toSetExpression symbols when
making this change.
…se files; delete deprecated shims (D9, TML-2753) Call-site delta: all six marker/ledger functions (insertMarker, initMarker, updateMarker, writeLedgerEntry, readMarker x2 — probe + row fetch) are now expressed entirely through the D8 pgTable()/sqliteTable() API. Every codec, table reference, and column reference lives at the table declaration; leaves hold plain values and column-proxy expressions only. The information_schema.tables and sqlite_master catalog probes are routed through the same execute() helper as the DML, so there are zero raw driver.query() strings in the new files. File collapse: marker-ledger-writes.ts + marker-read.ts merged into marker-ledger.ts per adapter. The split was a round-1 dispatch boundary artefact, not a real seam. Shim deletion: tableRef, excludedColumn, dbExpr, insert, upsert, update, param removed from relational-core/src/contract-free/dml.ts and its contract-free export; pgTableRef and its postgres/contract-free/dml.ts wrapper deleted. Nothing outside the files being rewritten imported these symbols. Signed-off-by: Will Madden <madden@prisma.io>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/3-targets/6-adapters/sqlite/src/core/marker-ledger.ts (1)
251-265: 💤 Low valueReplace bare
ascast with utility function.Line 255 uses a bare
ascast which violates the coding guideline requiringblindCast<T, 'Reason'>orcastAs<T>from@prisma-next/utils/castsfor production code.♻️ Suggested fix
+import { castAs } from '`@prisma-next/utils/casts`'; + export function decodeSqliteMarkerRow(row: unknown): unknown { if (typeof row !== 'object' || row === null || !('invariants' in row)) { return row; } - const record = row as { invariants: unknown }; + const record = castAs<{ invariants: unknown }>(row); if (typeof record.invariants !== 'string') return row;As per coding guidelines: "No bare
asin production code. Use blindCast<T, 'Reason'> or castAs from@prisma-next/utils/casts."🤖 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/sqlite/src/core/marker-ledger.ts` around lines 251 - 265, In decodeSqliteMarkerRow replace the bare TypeScript cast "as { invariants: unknown }" with the project's casting utility: import and use either blindCast<{ invariants: unknown }, 'DecodeSqliteMarkerRow'> or castAs<{ invariants: unknown }> from '`@prisma-next/utils/casts`' and apply it to the row before accessing record.invariants so the cast follows the coding guideline and preserves the same runtime behavior.packages/3-targets/6-adapters/postgres/src/core/marker-ledger.ts (1)
105-127: ⚡ Quick winUse
table.columns.*for these contract-free column refs.This file is reading columns directly off the table handle (
infoSchemaTables.table_schema,marker.space, etc.) instead oftable.columns.*. Please switch the pattern consistently here and in the later DML builders so it stays aligned with the contract-free API convention and avoids future table-property collisions.Suggested change
const probe = infoSchemaTables - .select(infoSchemaTables.table_schema) + .select(infoSchemaTables.columns.table_schema) .where( - infoSchemaTables.table_schema + infoSchemaTables.columns.table_schema .eq('prisma_contract') - .and(infoSchemaTables.table_name.eq('marker')), + .and(infoSchemaTables.columns.table_name.eq('marker')), ) .build(); @@ const fetch = marker .select( - marker.core_hash, - marker.profile_hash, - marker.contract_json, - marker.canonical_version, - marker.updated_at, - marker.app_tag, - marker.meta, - marker.invariants, + marker.columns.core_hash, + marker.columns.profile_hash, + marker.columns.contract_json, + marker.columns.canonical_version, + marker.columns.updated_at, + marker.columns.app_tag, + marker.columns.meta, + marker.columns.invariants, ) - .where(marker.space.eq(space)) + .where(marker.columns.space.eq(space)) .build();As per coding guidelines, "Column access: Use table.columns.fieldName to avoid conflicts with table properties."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/3-targets/6-adapters/postgres/src/core/marker-ledger.ts` around lines 105 - 127, The SQL builders are accessing columns directly off table handles (e.g., infoSchemaTables.table_schema, infoSchemaTables.table_name, marker.space, marker.core_hash, marker.profile_hash, marker.contract_json, marker.canonical_version, marker.updated_at, marker.app_tag, marker.meta, marker.invariants); update all such references to use the contract-free API by replacing them with the table.columns.* form (e.g., infoSchemaTables.columns.table_schema, marker.columns.space, marker.columns.core_hash, etc.) throughout the probe, fetch and any later DML builders so column access consistently uses table.columns.fieldName.
🤖 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.
Nitpick comments:
In `@packages/3-targets/6-adapters/postgres/src/core/marker-ledger.ts`:
- Around line 105-127: The SQL builders are accessing columns directly off table
handles (e.g., infoSchemaTables.table_schema, infoSchemaTables.table_name,
marker.space, marker.core_hash, marker.profile_hash, marker.contract_json,
marker.canonical_version, marker.updated_at, marker.app_tag, marker.meta,
marker.invariants); update all such references to use the contract-free API by
replacing them with the table.columns.* form (e.g.,
infoSchemaTables.columns.table_schema, marker.columns.space,
marker.columns.core_hash, etc.) throughout the probe, fetch and any later DML
builders so column access consistently uses table.columns.fieldName.
In `@packages/3-targets/6-adapters/sqlite/src/core/marker-ledger.ts`:
- Around line 251-265: In decodeSqliteMarkerRow replace the bare TypeScript cast
"as { invariants: unknown }" with the project's casting utility: import and use
either blindCast<{ invariants: unknown }, 'DecodeSqliteMarkerRow'> or castAs<{
invariants: unknown }> from '`@prisma-next/utils/casts`' and apply it to the row
before accessing record.invariants so the cast follows the coding guideline and
preserves the same runtime behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: d957d7f6-9620-466b-8914-07038c44d2c2
📒 Files selected for processing (12)
packages/2-sql/4-lanes/relational-core/src/contract-free/dml.tspackages/2-sql/4-lanes/relational-core/src/contract-free/table.tspackages/2-sql/4-lanes/relational-core/src/exports/contract-free.tspackages/3-targets/3-targets/postgres/src/exports/contract-free.tspackages/3-targets/6-adapters/postgres/src/core/adapter.tspackages/3-targets/6-adapters/postgres/src/core/control-adapter.tspackages/3-targets/6-adapters/postgres/src/core/marker-ledger.tspackages/3-targets/6-adapters/postgres/test/adapter.test.tspackages/3-targets/6-adapters/postgres/test/control-adapter.test.tspackages/3-targets/6-adapters/sqlite/src/core/adapter.tspackages/3-targets/6-adapters/sqlite/src/core/control-adapter.tspackages/3-targets/6-adapters/sqlite/src/core/marker-ledger.ts
💤 Files with no reviewable changes (3)
- packages/2-sql/4-lanes/relational-core/src/exports/contract-free.ts
- packages/2-sql/4-lanes/relational-core/src/contract-free/dml.ts
- packages/3-targets/3-targets/postgres/src/exports/contract-free.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/3-targets/6-adapters/sqlite/src/core/control-adapter.ts
- packages/3-targets/6-adapters/postgres/src/core/adapter.ts
- packages/3-targets/6-adapters/sqlite/src/core/adapter.ts
- packages/2-sql/4-lanes/relational-core/src/contract-free/table.ts
The registry dispatches codecs purely from AST-supplied CodecRefs; it has no contract dependency and is not specific to the control plane. Naming it "control" leaked the intent of its first caller into the registry's identity. Rename to ast-codec-registry / createAstCodecRegistry to match the adjacent ast-codec-resolver it wraps and to name the mechanism (AST CodecRef dispatch) rather than the caller. Files changed: - packages/2-sql/5-runtime/src/codecs/control-codec-registry.ts → ast-codec-registry.ts (git mv + symbol rename + JSDoc rewrite) - packages/2-sql/5-runtime/src/exports/index.ts (re-export updated) - packages/3-targets/6-adapters/postgres/src/core/marker-ledger.ts (import + call site updated) - packages/3-targets/6-adapters/sqlite/src/core/marker-ledger.ts (import + call site updated) - packages/2-sql/5-runtime/README.md (Exports section updated) No behaviour change; same parameters, return type, and internal logic. Signed-off-by: Will Madden <madden@prisma.io>
…ules-footprint cap (TML-2753) Lands two calibration deltas surfaced during slice 2 round-2 reviews: - F22 added to drive/calibration/failure-modes.md. D8 R1 reviewer ran `git stash` to isolate a stale-`dist` typecheck red and popped an unrelated worktree`s stash entry (stash is repository-global, not worktree-scoped). The mitigation is to use `git worktree add` for any base-ref / pre-existing-flake verification; reviewer briefs must carry the no-stash rule explicitly. D9 R1 reviewer honored the new rule via a sibling-worktree verification with no incident. - .cursor/rules-footprint.config.json totalRulesBytes raised from 200000 to 205000. The legitimate growth came from round-1 F16 tightening of `.agents/rules/no-target-branches.mdc` (+48 lines, +2 KB) to forbid cross-layer field additions on shared AST classes. The bump leaves ~4 KB of headroom; trim before the next bump. Signed-off-by: Will Madden <madden@prisma.io>
…round-2 corrective)
Contributor
Author
Round-2 corrective scope landed — re-requesting reviewOperator review on round-1 (D5/D6/D7) cleared the architectural-mistake set but surfaced a deeper miss: the "contract-free" module was an option-bag wrapper around AST construction, marker writes threaded codec IDs at every leaf, marker reads went out as raw SQL strings, the per-adapter marker files were split, and the Round 2 — three corrective dispatches
Representative call site (post-round-2
|
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.
Linked issue
Refs TML-2753 — Slice 2 of the marker/ledger typed-query-AST project (Slice 1: #672). Related: TML-2754 (planner adoption), TML-2253 (project root).
At a glance
No raw INSERT/UPDATE marker SQL in production
srcoutside adapter lowering; SQL/MongoSqlControlAdapter/MongoControlAdapterare now signature-symmetric on the marker/ledger surface.Decision
Migrate all SQL marker/ledger DML (init, advance, ledger append) off raw-string builders and onto a typed-AST
SqlControlAdapterSPI symmetric with Mongo, with invariant-merge converged into a single TS implementation insideupdateMarkerand the read path consolidated onto one helper + one parser.Four dispatches, one merge each:
SqlControlAdapterwrite SPI (initMarker/updateMarker/writeLedgerEntry) + enabling surface: contract-free DML builder (insert/upsert/update),TableSource.schema?for schema-qualified DML, Postgrestext[]codec for invariants.mergeInvariants(union + dedupe + stable sort) lives insideupdateMarker, which reads current invariants internally — both dialects now accumulate-dedupe via one shared TS implementation.readMarkerResult(queryable, shape)helper infamily-sql/verify.ts+ oneparseContractMarkerRow; the duplicatesql-runtime/marker.tsparser and the SQLite runner's private read are deleted.buildMergeMarkerStatements,buildWriteMarkerStatements,writeContractMarker) plus dead ledger-insert builders; route PG + SQLite runners and the familysign()path through the SPI; drop the SQLite runner's client-side invariant pre-merge; collapse upsert toINSERT … ON CONFLICT (space) DO UPDATE; addMARKER_CAS_FAILURErunner code.Reviewer notes — the consequential change
Column-set reduction (deliberate cross-family parity, not a silent drop). The legacy PG merge
updaterefreshedcore_hash/profile_hash/contract_json/canonical_version/app_tag/meta/invariants/updated_aton every advance. The new SPIupdateMarkerwrites onlycore_hash/profile_hash/updated_at(+invariantswhen supplied). The other columns are initialised byinitMarkerand then stale-after-advance — they become write-only provenance.Verified safe before cut-over: zero production decision-paths read any dropped column.
verify()branches only onmarker.storageHash(control-instance.ts:498) andmarker.profileHash(:516); the rest ofmarkeris passed through into the diagnostic result only.ContractMarkerRecordLikeexposes onlystorageHash/invariants/profileHash?.migration-logformatter reads ledger fields the new write set covers (migrationName/migrationHash/from/to/appliedAt/operationCount).Mongo parity is genuine: Mongo
updateMarker$sets the same reduced field set; MongowriteLedgerEntryalready omits the same legacy snapshot fields (origin_profile_hash/destination_profile_hash/contract_json_before/contract_json_after). The DDL columns are retained for storage-shape compatibility — the SPI just stops writing them. If a future feature needs any of these refreshed on advance, the right move is to add it to both family SPIs (preserving DC-4 symmetry).Other reviewer notes:
runner.ts:605-607) already pre-merged invariants client-side before its overwrite statement, so today's net SQLite behaviour already accumulate-deduped — only the SQL statement overwrote. D2 relocates the merge intoupdateMarker; D4 drops the runner pre-merge. Convergence is at the SPI/statement layer.core_hash = expectedFrom+RETURNING spacerow-presence) and lock primitives (pg_advisory_xact_lock(hashtext($1)),BEGIN EXCLUSIVE) are byte-identical to pre-D4.MARKER_CAS_FAILUREis a clean addition toSqlMigrationRunnerErrorCode, symmetric with Mongo's existing code; no caller in tree branched on the prior generic-error path.updated_atstays DB-side (now()for PG,datetime('now')for SQLite viaRawExpr) — no wire-semantics change; the dispatch brief overruled an early instinct to switch to a JSDateparameter for Mongo symmetry.seedTestMarkerhelper — explicitly out of scope for DC-1, because fixtures hold a rawpg.Clientand can't reach the SPI'sControlDriverInstancewithout a layer violation.Behavior changes & evidence
family.initMarker/family.updateMarker—packages/3-targets/3-targets/postgres/src/core/migrations/runner.ts,packages/3-targets/3-targets/sqlite/src/core/migrations/runner.ts,packages/2-sql/9-family/src/core/control-instance.ts. Evidence:packages/3-targets/6-adapters/postgres/test/marker-ledger-writes.test.ts, sqlite sibling, and the PGlite + node:sqlite integration tests.family.writeLedgerEntry— same runner files. Evidence:runner.ledger.test.ts+runner.ledger.integration.test.ts(snapshot columns asserttoBeNull()).packages/3-targets/6-adapters/postgres/src/core/marker-ledger-writes.ts, sqlite sibling. Evidence: D2 unit + integration tests pinningunion({a,b},{b,c}) → [a,b,c]on both dialects.packages/2-sql/9-family/src/core/verify.ts(readMarkerResult). The duplicatesql-runtime/marker.tsparser and the SQLite runner's private read are deleted, not wrapped.Testing performed
pnpm build(65/65 cached, post-merge incremental)pnpm typecheck(135/135)pnpm test:packages(9535 passed, 4 expected fail, 3 skipped)pnpm test:integration(1012/1012 under--maxWorkers=4; default parallelism shows intermittent contention on the journey / SQL-ORM suites — same class as the documentedinit-skill-distribution/init-journeyparallelism flake, not slice-induced)pnpm test:e2e(105/105)pnpm fixtures:check(byte-identical)pnpm lint:deps(1023 modules, clean)git grepfor removed symbols (writeContractMarker,buildMergeMarkerStatements,buildWriteMarkerStatements,buildLedgerInsertStatement): zero matches outsideprojects/Follow-ups
SqlMigrationRunnerErrorCodeinpackages/2-sql/9-family/README.mdandpackages/3-targets/3-targets/postgres/README.mddon't yet listMARKER_CAS_FAILURE— small follow-up tidy ticket; not gating this PR.Alternatives considered
currentInvariantsas a parameter toupdateMarker(Option A) — rejected; would break DC-4 SQL/Mongo SPI signature symmetry. Internal read insideupdateMarker(Option B) preserves the symmetric surface and hides dialect-specific merge concerns.SqlControlAdapter.readMarkerreturnMarkerReadResultdirectly — rejected; would force the control SPI to carry a 3-way result its consumers don't want and breakreadMarker | nullsymmetry with Mongo. SharedreadMarkerResulthelper + two thin typed projections (control returns| null, runtime returns the 3-way) keeps "one read home + one parser" without polluting either consumer's signature.updated_atto a JSDateparameter for Mongo symmetry — rejected; would change observable wire semantics. DB-sideRawExpr(now()/datetime('now')) keeps the existing timestamps' clock-source.WHERE-builder into the contract-free DML builder — rejected; predicate construction already has a typed expression layer (AndExpr/BinaryExpr/ColumnRef). Reusing it keeps the DML builder focused on insert/upsert/update structure.Skill update
n/a — internal SPI surface only; no user-facing CLI or config change.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes