TML-2753: route SQL marker/ledger writes through the control adapter SPI#712
Merged
Merged
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
…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. |
…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
…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
|
…roxies; fix docs link - table(): feed source.alias ?? source.name into makeColumnProxy so ColumnRef and tableName use the alias when one is set. - ColumnProxy.eq/neq: route null to NullCheckExpr (IS NULL / IS NOT NULL) instead of BinaryExpr (= NULL / <> NULL); the latter always evaluates to unknown in SQL. - Add unit tests covering both bug paths. - drive/code/README.md: update .cursor/rules link to canonical .agents/rules. Signed-off-by: Will Madden <madden@prisma.io>
…ps-through-adapter
wmadden
requested changes
Jun 4, 2026
TML-2791 (remove docs/drive, now upstream in ignite) expanded AGENTS.md by ~37 bytes updating the Drive-methodology link to the ignite tree. The lint:rules:footprint CI job was skipped on that docs-only commit; the cap was not adjusted there. Raise agentsBytes 11200 → 11500 to accommodate. Signed-off-by: Will Madden <madden@prisma.io>
…20 follow-up D8/D9/D10 closed SATISFIED but operator review surfaced three further findings on the round-2 work that should land on the same branch before merge: 1. Marker/ledger ops become methods on the control adapter class, not module functions + thin class delegation (Postgres + SQLite + Mongo). 2. Bootstrap-DDL residue (sql-marker.ts, both statement-builders.ts, SqlStatement type) deleted from production source; test helpers rewritten in test/utils.ts against the contract-free DDL builders. 3. Bare `as` casts in slice 2 test files cleaned up (~32 sites across 4 files), with README L26 prose fix + column-helper test simplification as ride-alongs. Driver-bound control SPI + common MarkerReader abstraction filed as TML-2820 (out of slice scope; surfaced from operator's symmetry critique on PostgresAdapterImpl.profile.readMarker wiring). Spec round-3 corrective scope; plan D11/D12/D13 dispatches. Signed-off-by: Will Madden <madden@prisma.io>
…rol-tables.ts (TML-2753) Move MARKER_TABLE_NAME, LEDGER_TABLE_NAME, and CONTROL_TABLE_NAMES into a production module so statement-builders can be deleted without breaking issue-planner. statement-builders re-exports the constants until the bootstrap-DDL residue is removed in a follow-up commit. Signed-off-by: Will Madden <madden@prisma.io>
…ee DDL builders (TML-2753) setupTestDatabase and new bootstrapPostgresSignMarkerTables lower buildSignMarkerBootstrapQueries via the postgres control adapter instead of raw ensureSchemaStatement / ensureTableStatement strings. Signed-off-by: Will Madden <madden@prisma.io>
…6 + column-helper test simplification (TML-2753) Replace bare as T casts with typed assignments and castAs in slice-2 test files, collapse static column-helper assertions, and update runtime README marker prose to reflect reads going through the control adapter SPI. Signed-off-by: Will Madden <madden@prisma.io>
…r class (TML-2753) Move readMarker, insertMarker, initMarker, updateMarker, and writeLedgerEntry implementations onto PostgresControlAdapter, SqliteControlAdapter, and MongoControlAdapterImpl. Each SQL adapter method lowers via this.lower(...) instead of a threaded lower callback. marker-ledger.ts in each SQL adapter package now holds contract-free table declarations, execute(), mergeInvariants (and decodeSqliteMarkerRow for SQLite) only. Mongo marker-ledger.ts keeps wire helpers (executeAggregate, parseMongoMarkerDoc, …) shared by the class. Runtime adapter profiles call readMarkerDiscriminated on a control adapter instance. Mongo package exports keep db-shaped entry points that delegate through a minimal control-driver shim into the default adapter instance. Signed-off-by: Will Madden <madden@prisma.io>
…ML-2753) Remove sql-marker.ts, target statement-builders modules, and SqlStatement. Migration runners and test helpers use LoweredStatement from the relational core AST; postgres integration bootstrap lives in test/integration postgres-bootstrap.ts. Signed-off-by: Will Madden <madden@prisma.io>
- bootstrap fixtures: cast DdlNode -> {Sqlite,Postgres}DdlNode at adapter.lower call sites
(buildControlTableBootstrapQueries returns family-generic DdlNode[], but lower expects
the target-specific subclass with accept())
- control-adapter.test.ts: replace castAs<Row[]> / castAs<CliStructuredError> with
as unknown as ... — castAs<T>(value: T) cannot validate when T is a generic or the
source is unknown
- runner.errors test files: drop unused executeStatement imports left behind by the
bootstrap-DDL deletion
- sqlite/package.json: sort exports map (biome formatter)
Signed-off-by: Will Madden <madden@prisma.io>
…ytes cap to 11200 The canonical-vs-symlink explanation grew over time into 7+ paragraphs that re-stated the same pattern three ways. Collapsed to: name the canonical paths, state the one rule (edit there, not through the symlink), point at the CI checks. Drive-skills-live-upstream paragraph kept (it's genuinely new info). AGENTS.md 11,237 -> 9,149 bytes (-2,088). Reverts the cap bump in c58f7d3 that absorbed TML-2791's ~37-byte growth; the bumped cap is no longer needed. Signed-off-by: Signed-off-by: Will Madden <madden@prisma.io>
…L-2753)
Slice 2 promised that the control adapter speaks typed AST end-to-end:
"Zero driver.query(rawMarkerSql) outside adapter lowering for in-scope
SQL marker/ledger ops (git grep clean)." readMarker / readMarkerResult
were converted in the earlier round, but readAllMarkers and readLedger
still issued raw SELECT strings against information_schema.tables /
sqlite_master and the marker / ledger tables. Operator review caught the
hole; converting them here closes the done-condition for Slice 2.
Both methods now route through the contract-free fluent surface and the
shared execute(lower, driver, query) helper, mirroring readMarkerResult.
To support readLedger.orderBy(id), the relational-core CfSelectQuery
gains an .orderBy(column, dir?) method that constructs an OrderByItem
from the column proxy and threads it through the immutable builder.
To carry the DB-generated id (bigserial / INTEGER PK) and created_at
(default now() / strftime) without polluting the writeable insert shape,
each marker-ledger.ts declares a second handle `ledgerReadShape` with
the full column set; the original `ledger` keeps the writeable subset
the insert path needs. (TableInsertRow has no concept of "defaulted"
columns yet, so two handles is the simplest correct fix; a proper
defaulted-column flag is a separate concern.)
postgres needed an int8() column helper (added alongside int4 in
target-postgres/contract-free/columns.ts and the contract-free export
barrel). sqlite reuses the existing integer() helper.
control-adapter.test.ts: the readAllMarkers corrupt-row test matched
unquoted raw SQL (`information_schema.tables`, `from prisma_contract.marker`);
updated to quoted-identifier matchers consistent with the other lowered-
SQL matchers already in the file.
Done-condition check after this change:
rg "driver\\.query" packages/3-targets/6-adapters/{postgres,sqlite}/src/core/{control-adapter,marker-ledger}.ts | rg -i "marker|ledger|prisma_contract|information_schema|sqlite_master"
→ only the two execute() helper sites that take a *lowered* AST.
Validation: adapter-postgres and adapter-sqlite typecheck + test suites
pass locally (561/561 + 174/174).
Signed-off-by: Will Madden <madden@prisma.io>
wmadden
approved these changes
Jun 4, 2026
wmadden
left a comment
wmadden
left a comment
Contributor
There was a problem hiding this comment.
Merge once my comments are addressed
… bump (TML-2753) Operator review (PR #712, .cursor/rules-footprint.config.json:9) flagged the totalRulesBytes bump from 200000 to 205000 in 745c63a and asked for no-target-branches.mdc to be condensed instead. Trimmed the "AST class fields are the same violation" section (the round-1 addition that triggered the bump) by ~1 KB. Kept the rule statement, one WRONG example, and one CORRECT example; dropped the renderer-dilemma intermediate example and tightened the why-this-matters prose. The pattern reference to the three-layer polymorphic IR survives. Result: no-target-branches.mdc 5759 → 4747 B. Total rules 196.1 KB → 195.3 KB, fits under the restored 200000 cap with ~3.9 KB headroom. Signed-off-by: Will Madden <madden@prisma.io>
Resolve AGENTS.md conflict by keeping main's expanded skills/rules section; auto-merge brings in control-policy PSL authoring work. Signed-off-by: Will Madden <madden@prisma.io>
Import PostgresContract from adapter-postgres/types, narrow bootstrap queries to PostgresDdlNode for lower(), and drop stale executeStatement imports after bootstrap helpers took over statement execution. Signed-off-by: Will Madden <madden@prisma.io>
Probe information_schema.columns when the marker table exists so db init surfaces PN-RUN-3020 instead of PN-RUN-3006 on pre-space marker tables. Skip the probe when a driver returns non-catalog rows (unit-test mocks). Signed-off-by: Will Madden <madden@prisma.io>
This was referenced Jun 5, 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.
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