Conversation
|
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:
📝 WalkthroughWalkthroughParamRefs were changed from positional indexes to value+codec form; parameter collection was moved to AST-level via Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as rgba(52, 152, 219, 0.5)
participant Transformer as rgba(39, 174, 96, 0.5)
participant AST as rgba(241, 196, 15, 0.5)
participant Deriver as rgba(155, 89, 182, 0.5)
participant Planner as rgba(231, 76, 60, 0.5)
participant Adapter as rgba(52, 73, 94, 0.5)
participant Runtime as rgba(22, 160, 133, 0.5)
Client->>Transformer: request transform/build
Transformer->>AST: emit QueryAst with ParamRef.of(value,{codecId})
AST->>Deriver: collectParamRefs()
Deriver->>Planner: return params[] and paramDescriptors[]
Planner->>Adapter: provide finalized AST
Adapter->>AST: collectParamRefs() -> build ParamIndexMap
Adapter->>Runtime: emit SQL with $n placeholders + ordered params
Runtime->>Runtime: encodeParams(ordered params, descriptors)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
@prisma-next/runtime-executor
@prisma-next/sql-runtime
@prisma-next/extension-paradedb
@prisma-next/extension-pgvector
@prisma-next/postgres
@prisma-next/sql-orm-client
@prisma-next/contract-authoring
@prisma-next/contract-ts
@prisma-next/ids
@prisma-next/psl-parser
@prisma-next/cli
@prisma-next/emitter
@prisma-next/eslint-plugin
@prisma-next/migration-tools
@prisma-next/vite-plugin-contract-emit
@prisma-next/sql-contract
@prisma-next/sql-errors
@prisma-next/sql-operations
@prisma-next/sql-schema-ir
@prisma-next/sql-contract-psl
@prisma-next/sql-contract-ts
@prisma-next/sql-contract-emitter
@prisma-next/family-sql
@prisma-next/sql-kysely-lane
@prisma-next/sql-lane-query-builder
@prisma-next/sql-relational-core
@prisma-next/sql-lane-sql-builder-new
@prisma-next/sql-lane
@prisma-next/target-postgres
@prisma-next/adapter-postgres
@prisma-next/driver-postgres
@prisma-next/core-control-plane
@prisma-next/core-execution-plane
@prisma-next/config
@prisma-next/contract
@prisma-next/operations
@prisma-next/plan
@prisma-next/utils
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/2-sql/5-runtime/src/codecs/encoding.ts (1)
59-71:⚠️ Potential issue | 🟠 MajorKeep encode-time validation for schema-backed params.
encodeParams()now sends schema-backed values straight to codec encoding/the driver without consulting any validator registry. That means a contract-invalid JSON payload can be written successfully and only fail later when a read path hitsdecodeRow(), which is a data-integrity regression. Please preserve a write-side validation hook here, even if the newParamRefmodel needs a different lookup mechanism thanrefs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/5-runtime/src/codecs/encoding.ts` around lines 59 - 71, encodeParams currently skips write-time validation for schema-backed parameters, so add a validation step before calling encodeParam: for each paramDescriptor in plan.meta.paramDescriptors look up the associated schema/validator for the ParamRef (or equivalent ref identifier on the descriptor) from whatever validator registry or mapping replaces refs, run the validator against paramValue and throw or propagate a validation error if it fails, then proceed to call encodeParam; ensure you use the existing symbols encodeParams, encodeParam, plan.meta.paramDescriptors and ParamRef (or the new descriptor.ref field) so the lookup integrates with the new ParamRef model and prevents invalid JSON from being encoded and written.packages/2-sql/4-lanes/sql-lane/src/sql/predicate-builder.ts (1)
104-126:⚠️ Potential issue | 🟠 MajorConvert
eq/neqwithnullplaceholders into null checks.If
paramsMap[paramName]isnull, this path currently buildsParamRef.of(null, ...), which later lowers tocol = $Norcol <> $N. That never matchesIS NULL/IS NOT NULL, so nullable runtime filters will return the wrong results.🐛 Proposed fix
if (isParamPlaceholder(where.right)) { const placeholder: ParamPlaceholder = where.right; paramName = placeholder.name; if (!Object.hasOwn(paramsMap, paramName)) { errorMissingParameter(paramName); } const value = paramsMap[paramName]; + if (value === null && (where.op === 'eq' || where.op === 'neq')) { + return { + expr: + where.op === 'eq' + ? NullCheckExprNode.isNull(leftExpr) + : NullCheckExprNode.isNotNull(leftExpr), + codecId: undefined, + paramName: '', + }; + } let nativeType: string | undefined; if (leftExpr instanceof ColumnRef) { const { table, column } = leftExpr; const contractTable = contract.storage.tables[table];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-lane/src/sql/predicate-builder.ts` around lines 104 - 126, When handling a parameter placeholder (isParamPlaceholder) and extracting paramsMap[paramName], if the runtime value is null and the binary operator on the where clause is an equality comparison ('eq' or 'neq'), do not call ParamRefNode.of(...) — instead build a null-check predicate: for 'eq' produce an IS NULL check and for 'neq' produce an IS NOT NULL check (or construct the appropriate NullCheck/IsNull AST node used by the predicate builder) using leftExpr and the node class that represents null checks; keep the existing errorMissingParameter, ParamPlaceholder, paramsMap and ParamRefNode.of logic for non-null values. Ensure this branching lives in the same block that currently creates rightExpr so downstream lowering will emit IS NULL / IS NOT NULL instead of col = $N or col <> $N.
🧹 Nitpick comments (10)
packages/2-sql/5-runtime/test/json-schema-validation.test.ts (1)
372-381: Exercise the name-based codec lookup, not just the fallback.Because
descriptor.codecIdis still set here, this test passes even ifresolveParamCodec()never readsplan.meta.annotations.codecs[descriptor.name]. Please add a case where the descriptor only hasnameand the plan annotation supplies the codec, so the new branch is actually covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/5-runtime/test/json-schema-validation.test.ts` around lines 372 - 381, The test currently always sets descriptor.codecId so it doesn't exercise the name-based lookup; modify or add a test that passes a ParamDescriptor with no codecId (only name: 'metadata') and populate the test plan's meta.annotations.codecs map with an entry for 'metadata' pointing to 'pg/jsonb@1', then call encodeParam (the same as before) and assert the encoded result is '{"age":30}'; this ensures resolveParamCodec/encodeParam actually reads plan.meta.annotations.codecs[descriptor.name] rather than relying on descriptor.codecId.packages/2-sql/4-lanes/relational-core/src/ast/types.ts (1)
1422-1435: Consider documenting or enforcing deterministic parameter ordering for increased robustness.
Object.values(row)andObject.values(this.onConflict.action.set)rely on property insertion order, which is guaranteed by ES2015+ for string-keyed objects. Production code constructs rows deterministically by assigning properties in sequence (transform-dml.ts iterates columns in order; sql-lane builds values incrementally). However, to make the ordering contract explicit and prevent future bugs if construction patterns change, either:
- Sort extracted values by key name in
collectParamRefs()- Add a code comment documenting that row/set ordering must be preserved during construction
The existing test at insert.test.ts:108-119 validates the
[r1Id, r1Email, r2Id, r2Email]order, confirming current behavior is correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/relational-core/src/ast/types.ts` around lines 1422 - 1435, collectParamRefs currently uses Object.values(row) and Object.values(this.onConflict.action.set) which rely on insertion order; change collectParamRefs to explicitly iterate keys in a deterministic order (e.g., Object.keys(row).sort() and Object.keys(this.onConflict.action.set).sort()) and push values[key] so parameter ordering is stable, and do the same for the onConflict branch when this.onConflict?.action instanceof DoUpdateSetConflictAction; alternatively, if you prefer minimal change, add a clear code comment inside collectParamRefs stating that rows and set objects must be constructed in deterministic column order and reference transform-dml.ts and sql-lane as the producers.packages/3-extensions/sql-orm-client/src/where-utils.ts (1)
7-11: Don't export a self-described fragile type guard.Line 11 distinguishes
BoundWhereExprby the absence ofaccept, which couples this helper to currentWhereExprinternals instead of the wrapper's own contract. If either side evolves, this silently flips behavior. I'd rather brand the wrapper or keep this normalization local than rely on a negative shape check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/src/where-utils.ts` around lines 7 - 11, The exported type guard isBoundWhereExpr is fragile because it relies on the absence of WhereExpr.accept(); remove that public contract by making the guard internal or switch to a positive discriminant: either (A) stop exporting isBoundWhereExpr (remove the export) and keep normalization/local checks internal to this module, or (B) add an explicit brand on the wrapper (e.g., a boolean or string property like '__bound' or 'kind: "bound"') on BoundWhereExpr and change isBoundWhereExpr to test that positive property (instead of '!("accept" in value)'). Update any call sites to use the new internal/local helper or the explicit-brand check accordingly.packages/3-extensions/sql-orm-client/src/query-plan-meta.ts (1)
6-20: UseifDefined()for conditional object spreads.The inline conditional spread patterns at lines 16-17 should use
ifDefined()from@prisma-next/utils/definedper coding guidelines.♻️ Suggested refactor
+import { ifDefined } from '@prisma-next/utils/defined'; import type { ParamDescriptor, PlanMeta } from '@prisma-next/contract/types'; import type { SqlContract, SqlStorage } from '@prisma-next/sql-contract/types'; import { type ParamRef, type QueryAst, SelectAst } from '@prisma-next/sql-relational-core/ast'; import type { SqlQueryPlan } from '@prisma-next/sql-relational-core/plan'; export function deriveParamsFromAst(ast: { collectParamRefs(): ParamRef[] }): { params: unknown[]; paramDescriptors: ParamDescriptor[]; } { const collectedParams = ast.collectParamRefs(); return { params: collectedParams.map((p) => p.value), paramDescriptors: collectedParams.map((p) => ({ name: p.name, source: 'dsl' as const, - ...(p.codecId ? { codecId: p.codecId } : {}), - ...(p.nativeType ? { nativeType: p.nativeType } : {}), + ...ifDefined('codecId', p.codecId), + ...ifDefined('nativeType', p.nativeType), })), }; }As per coding guidelines: "Use
ifDefined()from@prisma-next/utils/definedfor conditional object spreads instead of inline conditional spread patterns."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/src/query-plan-meta.ts` around lines 6 - 20, The conditional object spreads in deriveParamsFromAst should use ifDefined from `@prisma-next/utils/defined` instead of inline ternaries; import ifDefined and replace the two inline spreads inside the paramDescriptors mapping (the parts referencing p.codecId and p.nativeType) with ...ifDefined({ codecId: p.codecId }) and ...ifDefined({ nativeType: p.nativeType }) respectively so undefined values are omitted per project conventions.packages/3-targets/6-adapters/postgres/test/rich-adapter.test.ts (1)
150-150: Inconsistent:params: []passed here but removed from otherlower()calls.Other tests in this file (lines 105, 134, 176, 187, 195) call
adapter.lower(ast, { contract })without theparamsargument, but this test still passesparams: []. For consistency with the PR's approach of deriving params from the AST, consider removing the explicitparamsargument.♻️ Suggested fix
- const lowered = adapter.lower(ast, { contract, params: [] }); + const lowered = adapter.lower(ast, { contract });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-targets/6-adapters/postgres/test/rich-adapter.test.ts` at line 150, The test is inconsistent because it calls adapter.lower(ast, { contract, params: [] }) while other tests use adapter.lower(ast, { contract }); remove the explicit params: [] from this invocation so the call becomes adapter.lower(ast, { contract }) and allow adapter.lower to derive params from the AST like the other tests (locate the call that assigns lowered from adapter.lower and remove the params property).packages/2-sql/4-lanes/sql-lane/src/sql/select-builder.ts (1)
64-65: Consider usingifDefined()for conditional spreads.The coding guidelines recommend using
ifDefined()from@prisma-next/utils/definedfor conditional object spreads. This is a minor style preference.♻️ Optional refactor using ifDefined()
+import { ifDefined } from '@prisma-next/utils/defined'; // Inside deriveParamsFromAst: paramDescriptors: collected.map((p) => ({ name: p.name, source: 'dsl' as const, - ...(p.codecId ? { codecId: p.codecId } : {}), - ...(p.nativeType ? { nativeType: p.nativeType } : {}), + ...ifDefined('codecId', p.codecId), + ...ifDefined('nativeType', p.nativeType), })),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-lane/src/sql/select-builder.ts` around lines 64 - 65, Replace the conditional object spreads that check p.codecId and p.nativeType with calls to ifDefined from `@prisma-next/utils/defined`; specifically, in select-builder.ts where the spread uses ...(p.codecId ? { codecId: p.codecId } : {}) and ...(p.nativeType ? { nativeType: p.nativeType } : {}), import ifDefined and use ifDefined({ codecId: p.codecId }) and ifDefined({ nativeType: p.nativeType }) (or equivalent) so the spreads are applied only when values are defined, keeping the surrounding object assembly unchanged.packages/3-extensions/sql-orm-client/test/query-plan-select.test.ts (1)
67-70: Keep thesewhereexpectations independent ofbindWhereExpr.These assertions now build the expected tree with the same binder that
compileSelect*relies on, so a binding regression can still leave the tests green. Prefer constructing the expectedParamRef-bearing ASTs directly here and keepbindWhereExprcovered in its own unit tests.As per coding guidelines, "Avoid tautological tests that only restate fixture input - tests must verify behavior, not mirror object shape passed by the test itself."
Also applies to: 83-86, 111-124, 139-140, 168-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/test/query-plan-select.test.ts` around lines 67 - 70, The test currently uses bindWhereExpr to build the expected where AST (e.g., bindWhereExpr(...).expr), which mirrors the production binder and makes the test tautological; replace those expectations by constructing the expected ASTs directly using ParamRef nodes and concrete AST constructors (e.g., BinaryExpr.eq, ColumnRef.of, LiteralExpr.of, and explicit ParamRef instances) so the test asserts the compiled output against a hand-built ParamRef-bearing tree; keep bindWhereExpr covered in its own unit tests and update the affected assertions (the ones using bindWhereExpr at the referenced locations) to use the direct AST construction instead of calling bindWhereExpr.packages/2-sql/4-lanes/kysely-lane/src/transform/transform.ts (1)
69-76: UseifDefined()for these optional descriptor fields.This file already imports
ifDefined, and the repo guideline prefers it over inline conditional spreads for optional properties. Swapping these two branches keeps the descriptor construction consistent with the rest of the lane code.♻️ Suggested cleanup
const paramDescriptors = collectedParams.map((p, index) => ({ name: p.name, source: 'lane' as const, index: index + 1, - ...(p.codecId ? { codecId: p.codecId } : {}), - ...(p.nativeType ? { nativeType: p.nativeType } : {}), + ...ifDefined('codecId', p.codecId), + ...ifDefined('nativeType', p.nativeType), }));As per coding guidelines, "Use
ifDefined()from@prisma-next/utils/definedfor conditional object spreads instead of inline conditional spread patterns."Also applies to: 155-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/kysely-lane/src/transform/transform.ts` around lines 69 - 76, Replace the inline conditional spreads used when building paramDescriptors with the repo-preferred ifDefined utility: instead of ...(p.codecId ? { codecId: p.codecId } : {}) and ...(p.nativeType ? { nativeType: p.nativeType } : {}), call ifDefined for those optional fields so the object only includes codecId and nativeType when present. Update both occurrences where paramDescriptors is constructed (the collectedParams -> paramDescriptors mapping and the later similar block around the other descriptor construction) to use ifDefined from `@prisma-next/utils/defined` and keep all other keys (name, source, index) unchanged.packages/3-targets/6-adapters/postgres/test/adapter.test.ts (1)
131-138: Assert the derived param arrays here, not just the rendered SQL.This refactor moved parameter collection and ordering into
adapter.lower(). These checks can still pass if the adapter emits the right placeholders but binds the wrong values or order, especially in the DML and typed-JSON cases.🧪 Suggested assertions
- expect(adapter.lower(updateAst, { contract }).body.sql).toBe( + const loweredUpdate = adapter.lower(updateAst, { contract }); + expect(loweredUpdate.body.sql).toBe( 'UPDATE "user" SET "email" = $1 WHERE "user"."id" = $2 RETURNING "user"."email"', ); + expect(loweredUpdate.body.params).toEqual(['b@example.com', 1]);const lowered = adapter.lower(ast, { contract }); expect(lowered.body.sql).toBe( [ 'SELECT DISTINCT ON ("user"."email") "user"."id" AS "id"', @@ ].join(' '), ); + expect(lowered.body.params).toEqual([{ active: true }, { source: 'test' }]);Also applies to: 191-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-targets/6-adapters/postgres/test/adapter.test.ts` around lines 131 - 138, The tests currently only assert rendered SQL from adapter.lower(...) for insertAst, updateAst, deleteAst (and the similar typed-JSON cases later) but not the derived parameter arrays; add assertions that the returned object's body.params (or equivalent param array returned by adapter.lower(insertAst, { contract })) contains the expected bound values in the correct order for insertAst, updateAst, deleteAst and the typed-JSON test group so we verify both placeholders and their actual bound values are correct while keeping the same calls to adapter.lower and using the existing variables insertAst, updateAst, deleteAst, contract (and the corresponding ASTs in the other test block).packages/2-sql/4-lanes/sql-lane/src/sql/mutation-builder.ts (1)
39-43: UseifDefined()for the optional descriptor fields.Lines 40-43 should omit optional keys the same way the rest of the repo does. That also avoids materializing
name: undefinedfor unnamed params.Suggested refactor
+import { ifDefined } from '@prisma-next/utils/defined'; + function deriveParamsFromAst(ast: { collectParamRefs(): ParamRef[] }) { const collected = ast.collectParamRefs(); return { paramValues: collected.map((p) => p.value), paramDescriptors: collected.map((p) => ({ - name: p.name, source: 'dsl' as const, - ...(p.codecId ? { codecId: p.codecId } : {}), - ...(p.nativeType ? { nativeType: p.nativeType } : {}), + ...ifDefined('name', p.name), + ...ifDefined('codecId', p.codecId), + ...ifDefined('nativeType', p.nativeType), })), }; }As per coding guidelines, "Use
ifDefined()from@prisma-next/utils/definedfor conditional object spreads instead of inline conditional spread patterns".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-lane/src/sql/mutation-builder.ts` around lines 39 - 43, Replace the inline conditional spreads in the param descriptor creation with ifDefined() so optional keys are omitted when undefined: inside the collected.map callback that constructs paramDescriptors (the block creating name/source and spreading codecId/nativeType), import and use ifDefined from `@prisma-next/utils/defined` and wrap the optional fields (codecId and nativeType) with ifDefined(...) to avoid materializing undefined keys and follow the repo pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/2-sql/4-lanes/sql-lane/src/sql/mutation-builder.ts`:
- Around line 35-45: deriveParamsFromAst currently maps every occurrence from
ast.collectParamRefs(), causing duplicate entries when the same ParamRef object
is shared in the AST; change it to produce params/paramDescriptors based on
unique ParamRef identity (object reference) in insertion order. Iterate the
collected array, track seen ParamRef objects (e.g., via a Set or Map keyed by
the ParamRef instance), and only push a paramValue and paramDescriptor for the
first occurrence of each unique ParamRef; keep the descriptor fields (name,
source: 'dsl', codecId, nativeType) the same and preserve the original ordering
of first appearances.
In `@packages/2-sql/4-lanes/sql-lane/test/rich-select-and-include.test.ts`:
- Around line 68-70: The test currently expects a ParamRef stamped as pg/int4@1
but uses the string value 'u1' (see the expectation using ParamRef.of('u1', {
name: 'userId', codecId: 'pg/int4@1', nativeType: 'int4' }) and the plan built
earlier with 'u1'), which mismatches the stamped integer type; fix by making the
bound value match the stamped parameter type—replace the string 'u1' with an
integer literal (e.g., 1) in the ParamRef.of call so the value and
codecId/nativeType align with pg/int4@1 (or alternatively change the
codecId/nativeType to a text type if you intend a string).
In `@packages/2-sql/4-lanes/sql-lane/test/sql-dml-vector-ops.test.ts`:
- Around line 137-138: The test is asserting an unbound ParamRef for the vector
arg (left.args === ParamRef.of(undefined, { name: 'other' })) while the runtime
call passed other: [1,2,3]; update the assertions so the vector argument is
carried into the plan before validating the threshold: ensure left.args
references the ParamRef that includes the runtime value (or references the
correct param index) and assert plan.params contains the actual vector [1,2,3]
(in addition to the existing numeric threshold check). Concretely, adjust the
assertions around left.args, ParamRef.of and plan.params so the 'other' vector
is present/bound in plan.params and left.args corresponds to that bound param.
In `@packages/3-extensions/sql-orm-client/src/where-binding.ts`:
- Around line 39-47: The binary(...) builder currently always returns a
BinaryExpr which turns comparisons against null into "col = NULL"/"col <> NULL";
change binary to detect eq/neq cases where bindComparable(...) yields a null
literal (or otherwise represents SQL NULL) and return a NullCheckExpr (with
appropriate isNot flag for "neq") instead of a BinaryExpr; keep using
bindExpression(contract, expr.left) to compute left and pass the left ColumnRef
into bindComparable(contract, expr.right, bindingColumn) to know when the right
is null, and only fall back to new BinaryExpr(expr.op, left, right) for non-null
comparisons.
- Around line 73-79: The code returns caller-provided ParamRef instances
unchanged, which drops contract-derived metadata (codecId/nativeType) needed for
Postgres lowering; update the branch handling comparable instanceof ParamRef
(and the combined conditional using bindingColumn) to enrich the ParamRef with
the bindingColumn's metadata before returning by calling the same enrichment
path used by bindExpression (i.e., merge bindingColumn/contract info into the
ParamRef), while leaving LiteralExpr and ListLiteralExpr behavior unchanged;
locate ParamRef, bindingColumn, bindExpression, LiteralExpr, ListLiteralExpr,
and comparable in where-binding.ts and ensure ParamRef instances are augmented
with codecId/nativeType from the binding column instead of returned raw.
In `@packages/3-targets/6-adapters/postgres/src/core/adapter.ts`:
- Around line 123-131: The current lower method overwrites the index for reused
ParamRef instances because it unconditionally sets paramIndexMap and always
pushes ref.value; change the logic in lower (handling collectedParamRefs,
paramIndexMap, params) to check if the ParamRef instance is already in
paramIndexMap (use paramIndexMap.has(ref)) and only assign a new index and push
ref.value when it is not present, otherwise reuse the existing index so shared
ParamRef nodes keep their original placeholder slot; refer to the lower
function, ParamRef instances, paramIndexMap, and params to locate and update
this behavior.
---
Outside diff comments:
In `@packages/2-sql/4-lanes/sql-lane/src/sql/predicate-builder.ts`:
- Around line 104-126: When handling a parameter placeholder
(isParamPlaceholder) and extracting paramsMap[paramName], if the runtime value
is null and the binary operator on the where clause is an equality comparison
('eq' or 'neq'), do not call ParamRefNode.of(...) — instead build a null-check
predicate: for 'eq' produce an IS NULL check and for 'neq' produce an IS NOT
NULL check (or construct the appropriate NullCheck/IsNull AST node used by the
predicate builder) using leftExpr and the node class that represents null
checks; keep the existing errorMissingParameter, ParamPlaceholder, paramsMap and
ParamRefNode.of logic for non-null values. Ensure this branching lives in the
same block that currently creates rightExpr so downstream lowering will emit IS
NULL / IS NOT NULL instead of col = $N or col <> $N.
In `@packages/2-sql/5-runtime/src/codecs/encoding.ts`:
- Around line 59-71: encodeParams currently skips write-time validation for
schema-backed parameters, so add a validation step before calling encodeParam:
for each paramDescriptor in plan.meta.paramDescriptors look up the associated
schema/validator for the ParamRef (or equivalent ref identifier on the
descriptor) from whatever validator registry or mapping replaces refs, run the
validator against paramValue and throw or propagate a validation error if it
fails, then proceed to call encodeParam; ensure you use the existing symbols
encodeParams, encodeParam, plan.meta.paramDescriptors and ParamRef (or the new
descriptor.ref field) so the lookup integrates with the new ParamRef model and
prevents invalid JSON from being encoded and written.
---
Nitpick comments:
In `@packages/2-sql/4-lanes/kysely-lane/src/transform/transform.ts`:
- Around line 69-76: Replace the inline conditional spreads used when building
paramDescriptors with the repo-preferred ifDefined utility: instead of
...(p.codecId ? { codecId: p.codecId } : {}) and ...(p.nativeType ? {
nativeType: p.nativeType } : {}), call ifDefined for those optional fields so
the object only includes codecId and nativeType when present. Update both
occurrences where paramDescriptors is constructed (the collectedParams ->
paramDescriptors mapping and the later similar block around the other descriptor
construction) to use ifDefined from `@prisma-next/utils/defined` and keep all
other keys (name, source, index) unchanged.
In `@packages/2-sql/4-lanes/relational-core/src/ast/types.ts`:
- Around line 1422-1435: collectParamRefs currently uses Object.values(row) and
Object.values(this.onConflict.action.set) which rely on insertion order; change
collectParamRefs to explicitly iterate keys in a deterministic order (e.g.,
Object.keys(row).sort() and Object.keys(this.onConflict.action.set).sort()) and
push values[key] so parameter ordering is stable, and do the same for the
onConflict branch when this.onConflict?.action instanceof
DoUpdateSetConflictAction; alternatively, if you prefer minimal change, add a
clear code comment inside collectParamRefs stating that rows and set objects
must be constructed in deterministic column order and reference transform-dml.ts
and sql-lane as the producers.
In `@packages/2-sql/4-lanes/sql-lane/src/sql/mutation-builder.ts`:
- Around line 39-43: Replace the inline conditional spreads in the param
descriptor creation with ifDefined() so optional keys are omitted when
undefined: inside the collected.map callback that constructs paramDescriptors
(the block creating name/source and spreading codecId/nativeType), import and
use ifDefined from `@prisma-next/utils/defined` and wrap the optional fields
(codecId and nativeType) with ifDefined(...) to avoid materializing undefined
keys and follow the repo pattern.
In `@packages/2-sql/4-lanes/sql-lane/src/sql/select-builder.ts`:
- Around line 64-65: Replace the conditional object spreads that check p.codecId
and p.nativeType with calls to ifDefined from `@prisma-next/utils/defined`;
specifically, in select-builder.ts where the spread uses ...(p.codecId ? {
codecId: p.codecId } : {}) and ...(p.nativeType ? { nativeType: p.nativeType } :
{}), import ifDefined and use ifDefined({ codecId: p.codecId }) and ifDefined({
nativeType: p.nativeType }) (or equivalent) so the spreads are applied only when
values are defined, keeping the surrounding object assembly unchanged.
In `@packages/2-sql/5-runtime/test/json-schema-validation.test.ts`:
- Around line 372-381: The test currently always sets descriptor.codecId so it
doesn't exercise the name-based lookup; modify or add a test that passes a
ParamDescriptor with no codecId (only name: 'metadata') and populate the test
plan's meta.annotations.codecs map with an entry for 'metadata' pointing to
'pg/jsonb@1', then call encodeParam (the same as before) and assert the encoded
result is '{"age":30}'; this ensures resolveParamCodec/encodeParam actually
reads plan.meta.annotations.codecs[descriptor.name] rather than relying on
descriptor.codecId.
In `@packages/3-extensions/sql-orm-client/src/query-plan-meta.ts`:
- Around line 6-20: The conditional object spreads in deriveParamsFromAst should
use ifDefined from `@prisma-next/utils/defined` instead of inline ternaries;
import ifDefined and replace the two inline spreads inside the paramDescriptors
mapping (the parts referencing p.codecId and p.nativeType) with ...ifDefined({
codecId: p.codecId }) and ...ifDefined({ nativeType: p.nativeType })
respectively so undefined values are omitted per project conventions.
In `@packages/3-extensions/sql-orm-client/src/where-utils.ts`:
- Around line 7-11: The exported type guard isBoundWhereExpr is fragile because
it relies on the absence of WhereExpr.accept(); remove that public contract by
making the guard internal or switch to a positive discriminant: either (A) stop
exporting isBoundWhereExpr (remove the export) and keep normalization/local
checks internal to this module, or (B) add an explicit brand on the wrapper
(e.g., a boolean or string property like '__bound' or 'kind: "bound"') on
BoundWhereExpr and change isBoundWhereExpr to test that positive property
(instead of '!("accept" in value)'). Update any call sites to use the new
internal/local helper or the explicit-brand check accordingly.
In `@packages/3-extensions/sql-orm-client/test/query-plan-select.test.ts`:
- Around line 67-70: The test currently uses bindWhereExpr to build the expected
where AST (e.g., bindWhereExpr(...).expr), which mirrors the production binder
and makes the test tautological; replace those expectations by constructing the
expected ASTs directly using ParamRef nodes and concrete AST constructors (e.g.,
BinaryExpr.eq, ColumnRef.of, LiteralExpr.of, and explicit ParamRef instances) so
the test asserts the compiled output against a hand-built ParamRef-bearing tree;
keep bindWhereExpr covered in its own unit tests and update the affected
assertions (the ones using bindWhereExpr at the referenced locations) to use the
direct AST construction instead of calling bindWhereExpr.
In `@packages/3-targets/6-adapters/postgres/test/adapter.test.ts`:
- Around line 131-138: The tests currently only assert rendered SQL from
adapter.lower(...) for insertAst, updateAst, deleteAst (and the similar
typed-JSON cases later) but not the derived parameter arrays; add assertions
that the returned object's body.params (or equivalent param array returned by
adapter.lower(insertAst, { contract })) contains the expected bound values in
the correct order for insertAst, updateAst, deleteAst and the typed-JSON test
group so we verify both placeholders and their actual bound values are correct
while keeping the same calls to adapter.lower and using the existing variables
insertAst, updateAst, deleteAst, contract (and the corresponding ASTs in the
other test block).
In `@packages/3-targets/6-adapters/postgres/test/rich-adapter.test.ts`:
- Line 150: The test is inconsistent because it calls adapter.lower(ast, {
contract, params: [] }) while other tests use adapter.lower(ast, { contract });
remove the explicit params: [] from this invocation so the call becomes
adapter.lower(ast, { contract }) and allow adapter.lower to derive params from
the AST like the other tests (locate the call that assigns lowered from
adapter.lower and remove the params property).
🪄 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: 2aa0d0c4-9520-4592-9963-7efe7343175c
⛔ Files ignored due to path filters (2)
projects/structural-paramref/plan.mdis excluded by!projects/**projects/structural-paramref/spec.mdis excluded by!projects/**
📒 Files selected for processing (65)
packages/2-sql/4-lanes/kysely-lane/src/internal/build-plan.tspackages/2-sql/4-lanes/kysely-lane/src/plan.test.tspackages/2-sql/4-lanes/kysely-lane/src/transform/transform-context.tspackages/2-sql/4-lanes/kysely-lane/src/transform/transform-expr.tspackages/2-sql/4-lanes/kysely-lane/src/transform/transform-select.tspackages/2-sql/4-lanes/kysely-lane/src/transform/transform.tspackages/2-sql/4-lanes/kysely-lane/src/where-expr.ast.test.tspackages/2-sql/4-lanes/kysely-lane/src/where-expr.test.tspackages/2-sql/4-lanes/kysely-lane/src/where-expr.tspackages/2-sql/4-lanes/kysely-lane/test/build-plan.collect-params.test.tspackages/2-sql/4-lanes/relational-core/src/ast/types.tspackages/2-sql/4-lanes/relational-core/src/operations-registry.tspackages/2-sql/4-lanes/relational-core/test/ast/common.test.tspackages/2-sql/4-lanes/relational-core/test/ast/delete.test.tspackages/2-sql/4-lanes/relational-core/test/ast/insert.test.tspackages/2-sql/4-lanes/relational-core/test/ast/rich-ast.test.tspackages/2-sql/4-lanes/relational-core/test/ast/select.test.tspackages/2-sql/4-lanes/relational-core/test/ast/test-helpers.tspackages/2-sql/4-lanes/relational-core/test/ast/update.test.tspackages/2-sql/4-lanes/relational-core/test/ast/visitors.test.tspackages/2-sql/4-lanes/sql-lane/src/sql/include-builder.tspackages/2-sql/4-lanes/sql-lane/src/sql/mutation-builder.tspackages/2-sql/4-lanes/sql-lane/src/sql/predicate-builder.tspackages/2-sql/4-lanes/sql-lane/src/sql/select-builder.tspackages/2-sql/4-lanes/sql-lane/test/codecs.test.tspackages/2-sql/4-lanes/sql-lane/test/include-builder.test.tspackages/2-sql/4-lanes/sql-lane/test/mutation-builder.test.tspackages/2-sql/4-lanes/sql-lane/test/predicate-builder.test.tspackages/2-sql/4-lanes/sql-lane/test/rich-mutation.test.tspackages/2-sql/4-lanes/sql-lane/test/rich-select-and-include.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-comparison-operators.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-dml-vector-ops.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-dml.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-includes.builder.basic.test.tspackages/2-sql/4-lanes/sql-lane/test/sql.test.tspackages/2-sql/4-lanes/sql-lane/test/utils/guards.test.tspackages/2-sql/5-runtime/src/codecs/encoding.tspackages/2-sql/5-runtime/src/sql-runtime.tspackages/2-sql/5-runtime/test/json-schema-validation.test.tspackages/2-sql/5-runtime/test/lints.test.tspackages/3-extensions/sql-orm-client/src/param-descriptors.tspackages/3-extensions/sql-orm-client/src/query-plan-aggregate.tspackages/3-extensions/sql-orm-client/src/query-plan-meta.tspackages/3-extensions/sql-orm-client/src/query-plan-mutations.tspackages/3-extensions/sql-orm-client/src/query-plan-select.tspackages/3-extensions/sql-orm-client/src/where-binding.tspackages/3-extensions/sql-orm-client/src/where-interop.tspackages/3-extensions/sql-orm-client/src/where-utils.tspackages/3-extensions/sql-orm-client/test/collection.state.test.tspackages/3-extensions/sql-orm-client/test/integration/create.test.tspackages/3-extensions/sql-orm-client/test/query-plan-aggregate.test.tspackages/3-extensions/sql-orm-client/test/query-plan-mutations.test.tspackages/3-extensions/sql-orm-client/test/query-plan-select.test.tspackages/3-extensions/sql-orm-client/test/repository.test.tspackages/3-extensions/sql-orm-client/test/rich-collection.test.tspackages/3-extensions/sql-orm-client/test/rich-filters-and-where.test.tspackages/3-extensions/sql-orm-client/test/rich-query-plans.test.tspackages/3-extensions/sql-orm-client/test/where-binding.test.tspackages/3-extensions/sql-orm-client/test/where-interop.select-includes.test.tspackages/3-extensions/sql-orm-client/test/where-interop.test.tspackages/3-extensions/sql-orm-client/test/where-utils.test.tspackages/3-targets/6-adapters/postgres/src/core/adapter.tspackages/3-targets/6-adapters/postgres/test/adapter.test.tspackages/3-targets/6-adapters/postgres/test/operation-lowering.test.tspackages/3-targets/6-adapters/postgres/test/rich-adapter.test.ts
💤 Files with no reviewable changes (2)
- packages/3-extensions/sql-orm-client/src/param-descriptors.ts
- packages/2-sql/4-lanes/sql-lane/src/sql/include-builder.ts
There was a problem hiding this comment.
♻️ Duplicate comments (3)
packages/3-extensions/sql-orm-client/src/where-binding.ts (1)
36-41:⚠️ Potential issue | 🔴 CriticalHandle
NULLcomparison semantics foreq/neqoperators.When the bound right-hand side resolves to
null(either aLiteralExpr(null)converted toParamRefor an existingParamRefwithvalue === null), creating aBinaryExprproducescol = $Norcol <> $N. In SQL, these comparisons againstNULLalways yield unknown/false — the correct SQL iscol IS NULL/col IS NOT NULL.,
🐛 Proposed fix
binary(expr) { const left = bindExpression(contract, expr.left); const bindingColumn = left instanceof ColumnRef ? left : undefined; + const right = bindComparable(contract, expr.right, bindingColumn); + + if ( + (expr.op === 'eq' || expr.op === 'neq') && + ((right instanceof LiteralExpr && right.value === null) || + (right instanceof ParamRef && right.value === null)) + ) { + return expr.op === 'eq' ? NullCheckExpr.isNull(left) : NullCheckExpr.isNotNull(left); + } - return new BinaryExpr(expr.op, left, bindComparable(contract, expr.right, bindingColumn)); + return new BinaryExpr(expr.op, left, right); },Based on learnings: "When generating SQL for comparison operators in the SQL ORM client, do not emit
col = NULLorcol <> NULL; useNullCheckExprforeq/neqwith null."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/src/where-binding.ts` around lines 36 - 41, The binary() handler in where-binding.ts should detect when the bound right-hand side is a null parameter (e.g., a LiteralExpr(null) converted to a ParamRef or a ParamRef with value===null) and, for eq/neq operators, return a NullCheckExpr instead of constructing a BinaryExpr; update binary(contract, expr) to call bindExpression for left and bindComparable for right (as now), then if expr.op is 'eq' or 'neq' and the resolved right is a ParamRef with a null value (or is a LiteralExpr(null) before conversion) produce new NullCheckExpr(bindingColumn, expr.op === 'neq') otherwise fall back to returning new BinaryExpr(expr.op, left, right) so SQL emits IS NULL / IS NOT NULL correctly.packages/2-sql/4-lanes/sql-lane/src/sql/mutation-builder.ts (1)
35-45:⚠️ Potential issue | 🟠 MajorThis helper still over-counts shared
ParamRefnodes.It still emits one
params/paramDescriptorsentry percollectParamRefs()occurrence. When a mutation AST reuses the sameParamRefobject more than once, lowering will reuse one placeholder but the plan will carry duplicates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-lane/src/sql/mutation-builder.ts` around lines 35 - 45, The deriveParamsFromAst helper currently maps every entry from ast.collectParamRefs(), causing duplicate entries when the same ParamRef object is reused; change deriveParamsFromAst to iterate collected ParamRef objects and deduplicate by identity (use a Map or Set keyed by the ParamRef object) while preserving first-seen order, then build paramValues and paramDescriptors from the unique ParamRef instances; ensure you reference the ParamRef properties (value, name, codecId, nativeType) when creating paramDescriptors so each unique ParamRef yields exactly one value and one descriptor in the returned object.packages/2-sql/4-lanes/sql-lane/src/sql/select-builder.ts (1)
50-68:⚠️ Potential issue | 🟠 MajorDeduplicate repeated
ParamRefidentities before derivingparams.
collectParamRefs()returns every occurrence it walks. Lowering now assigns placeholder slots byParamRefinstance identity, so reusing the same node twice in the AST will produce one placeholder but two entries here. That can makequeryPlan.paramsandmeta.paramDescriptorsdrift from the lowered SQL.Suggested fix
function deriveParamsFromAst(ast: { collectParamRefs(): Array<{ value: unknown; name: string | undefined; codecId: string | undefined; nativeType: string | undefined; }>; }) { const collected = ast.collectParamRefs(); + const unique: typeof collected = []; + const seen = new Set<(typeof collected)[number]>(); + for (const ref of collected) { + if (seen.has(ref)) continue; + seen.add(ref); + unique.push(ref); + } + return { - paramValues: collected.map((p) => p.value), - paramDescriptors: collected.map((p) => ({ + paramValues: unique.map((p) => p.value), + paramDescriptors: unique.map((p) => ({ ...(p.name !== undefined && { name: p.name }), source: 'dsl' as const, ...(p.codecId ? { codecId: p.codecId } : {}), ...(p.nativeType ? { nativeType: p.nativeType } : {}), })), }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-lane/src/sql/select-builder.ts` around lines 50 - 68, deriveParamsFromAst currently maps every entry returned by ast.collectParamRefs(), but collectParamRefs() can include the same ParamRef instance multiple times; you must deduplicate by object identity before building paramValues and paramDescriptors so placeholders align with lowering. Fix deriveParamsFromAst by iterating collected, tracking seen ParamRef instances in a Map/Set keyed by the instance, only adding the first occurrence to the output arrays, and then map those unique entries to paramValues and paramDescriptors (preserving name, source: 'dsl', codecId and nativeType only when present) so queryPlan.params and meta.paramDescriptors stay in sync with the lowered SQL.
🧹 Nitpick comments (6)
packages/2-sql/4-lanes/relational-core/test/ast/test-helpers.ts (1)
28-30: UseifDefined()instead of inline conditional spreads.Please replace these
...(cond && { ... })spreads withifDefined()to match the repository standard for conditional object assembly in TS files.As per coding guidelines: "Use
ifDefined()from@prisma-next/utils/definedfor conditional object spreads instead of inline conditional spread patterns."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/relational-core/test/ast/test-helpers.ts` around lines 28 - 30, Replace the inline conditional object spreads for expr.name, expr.codecId, and expr.nativeType with the repository standard ifDefined() helper: instead of using ...(expr.name !== undefined && { name: expr.name }) etc., import ifDefined from '@prisma-next/utils/defined' (if not already) and use ifDefined(expr.name, () => ({ name: expr.name })) and similarly for codecId and nativeType in the object assembly where these spreads occur (look for variable/return building the expression object in test-helpers.ts).packages/2-sql/4-lanes/kysely-lane/src/transform/transform.ts (1)
28-112: Consider extracting shared logic to reduce duplication.
transformKyselyToPnAstandtransformKyselyToPnAstCollectingParamsshare ~80% of their implementation (query dispatch, paramDescriptor derivation, projection building, and metaAdditions assembly). The only difference is whetherparametersis passed tocreateContext.A helper function could encapsulate the shared post-transform logic:
♻️ Suggested refactor
function buildTransformResult( ast: QueryAst, ctx: TransformContext, ): TransformResult { const refs = extractRefsFromAst(ast); const collectedParams = ast.collectParamRefs(); const paramDescriptors = collectedParams.map((p, index) => ({ ...(p.name !== undefined && { name: p.name }), source: 'lane' as const, index: index + 1, ...(p.codecId ? { codecId: p.codecId } : {}), ...(p.nativeType ? { nativeType: p.nativeType } : {}), })); // ... projection logic ... return { ast, metaAdditions }; }Then both public functions would call this helper after dispatching to the appropriate transform.
Also applies to: 115-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/kysely-lane/src/transform/transform.ts` around lines 28 - 112, transformKyselyToPnAst and transformKyselyToPnAstCollectingParams duplicate most post-transform logic (dispatch, paramDescriptors, projection and metaAdditions); extract that shared logic into a helper like buildTransformResult(ast: QueryAst, ctx: TransformContext) which runs extractRefsFromAst, calls ast.collectParamRefs to build paramDescriptors, computes projection and projectionTypes using SelectAst and ColumnRef, and assembles metaAdditions with ifDefined; keep each public function responsible only for creating the correct ctx (with or without parameters) and dispatching to transformSelect/transformInsert/transformUpdate/transformDelete, then call buildTransformResult and return its result..agents/skills/drive-pr-local-review/SKILL.md (1)
193-193: Consider capitalizing "Markdown" (proper noun)."Markdown" appears twice on this line in lowercase. As a proper noun referring to the markup language, it's typically capitalized.
📝 Suggested capitalization fix
-**Location**: repo-relative path, and line range **as plain text** (not inside a `path:line` markdown link). In **Cursor** (`CURSOR_AGENT`, `CURSOR_TRACE_ID`, or `CURSOR_CLI` set, or user says output is for Cursor): use a path-only markdown link `[path](path)` and put the range after it, e.g. ` — lines 12–34`. Outside Cursor, you may use `[path (L12–L34)](path:12-34)` if links resolve for the reader. +**Location**: repo-relative path, and line range **as plain text** (not inside a `path:line` Markdown link). In **Cursor** (`CURSOR_AGENT`, `CURSOR_TRACE_ID`, or `CURSOR_CLI` set, or user says output is for Cursor): use a path-only Markdown link `[path](path)` and put the range after it, e.g. ` — lines 12–34`. Outside Cursor, you may use `[path (L12–L34)](path:12-34)` if links resolve for the reader.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/drive-pr-local-review/SKILL.md at line 193, Update the two lowercase occurrences of "markdown" on the same line in .agents/skills/drive-pr-local-review/SKILL.md to the proper noun "Markdown" so both instances are capitalized; locate the line containing the guidance about repo-relative paths and replace each "markdown" with "Markdown"..agents/skills/drive-pr-walkthrough/SKILL.md (1)
113-113: Consider capitalizing "Markdown" (proper noun)."Markdown" is typically capitalized as it refers to the markup language by name. The same applies to line 117.
📝 Suggested capitalization fix
Line 113:
-Use **repo-relative markdown links** so they're clickable from within the workspace. +Use **repo-relative Markdown links** so they're clickable from within the workspace.Line 117:
-**Cursor** does not resolve `path:line` / `path:start-end` in markdown link targets to a useful location +**Cursor** does not resolve `path:line` / `path:start-end` in Markdown link targets to a useful location🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/drive-pr-walkthrough/SKILL.md at line 113, Update the two occurrences of the lowercase word "markdown" to the proper noun "Markdown" in the SKILL.md text—specifically change the phrase "repo-relative markdown links" and the other instance on the subsequent line (line 117) so both read "repo-relative Markdown links" and "Markdown" respectively; locate these by searching for the exact phrase "repo-relative markdown links" and the other "markdown" occurrence and replace with a capitalized "Markdown".packages/3-extensions/sql-orm-client/test/query-plan-mutations.test.ts (1)
27-40: MakeusersColParamfail fast on missing metadata.This helper currently tolerates missing table/column metadata via optional chaining and conditional spreads, which can hide fixture drift. In this test, prefer asserting known metadata paths and building options explicitly.
Suggested refactor
function usersColParam( contract: ReturnType<typeof getTestContract>, column: string, value: unknown, ): ParamRef { - const columns = contract.storage.tables.users?.columns as - | Record<string, { codecId?: string; nativeType?: string }> - | undefined; - const columnMeta = columns?.[column]; - return ParamRef.of(value, { - name: column, - ...(columnMeta?.codecId !== undefined && { codecId: columnMeta.codecId }), - ...(columnMeta?.nativeType !== undefined && { nativeType: columnMeta.nativeType }), - }); + const columns = contract.storage.tables.users.columns as Record< + string, + { codecId?: string; nativeType?: string } + >; + const columnMeta = columns[column]!; + const options: { name: string; codecId?: string; nativeType?: string } = { name: column }; + if (columnMeta.codecId !== undefined) options.codecId = columnMeta.codecId; + if (columnMeta.nativeType !== undefined) options.nativeType = columnMeta.nativeType; + return ParamRef.of(value, options); }As per coding guidelines, "Prefer assertions over defensive checks when data is guaranteed to be valid ... Remove optional chaining and conditional property spreading for properties that are guaranteed to exist after validation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/test/query-plan-mutations.test.ts` around lines 27 - 40, usersColParam currently tolerates missing table/column metadata; change it to fail fast by asserting the existence of users table metadata and the specific column metadata from getTestContract before calling ParamRef.of, and construct the options object explicitly (e.g., name, codecId, nativeType) rather than using optional chaining or conditional spreads; throw a clear Error if columns or columnMeta are missing so tests fail fast when fixtures drift.packages/3-extensions/sql-orm-client/src/where-binding.ts (1)
94-99: UseifDefined()for conditional object spreads.The inline conditional spread patterns should use the
ifDefined()utility per coding guidelines.♻️ Proposed refactor
+import { ifDefined } from '@prisma-next/utils/defined'; + function createParamRef( contract: SqlContract<SqlStorage>, columnRef: ColumnRef, value: unknown, ): ParamRef { const columnMeta = contract.storage.tables[columnRef.table]?.columns[columnRef.column]; return ParamRef.of(value, { name: columnRef.column, - ...(columnMeta?.codecId !== undefined && { codecId: columnMeta.codecId }), - ...(columnMeta?.nativeType !== undefined && { nativeType: columnMeta.nativeType }), + ...ifDefined('codecId', columnMeta?.codecId), + ...ifDefined('nativeType', columnMeta?.nativeType), }); }As per coding guidelines: "Use
ifDefined()from@prisma-next/utils/definedfor conditional object spreads instead of inline conditional spread patterns."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/src/where-binding.ts` around lines 94 - 99, The code uses inline conditional object spreads when building the options for ParamRef.of (using columnMeta?.codecId and columnMeta?.nativeType); replace these inline spreads with the ifDefined() utility from `@prisma-next/utils/defined` so the options object passes only defined keys. Specifically, import ifDefined and change the options passed to ParamRef.of(...) to use ifDefined({ codecId: columnMeta?.codecId }, { nativeType: columnMeta?.nativeType }) (or equivalent calls) so the conditional fields are added via ifDefined instead of ...(cond && {...}) while keeping name: columnRef.column and the columnMeta lookup intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/2-sql/4-lanes/sql-lane/src/sql/mutation-builder.ts`:
- Around line 35-45: The deriveParamsFromAst helper currently maps every entry
from ast.collectParamRefs(), causing duplicate entries when the same ParamRef
object is reused; change deriveParamsFromAst to iterate collected ParamRef
objects and deduplicate by identity (use a Map or Set keyed by the ParamRef
object) while preserving first-seen order, then build paramValues and
paramDescriptors from the unique ParamRef instances; ensure you reference the
ParamRef properties (value, name, codecId, nativeType) when creating
paramDescriptors so each unique ParamRef yields exactly one value and one
descriptor in the returned object.
In `@packages/2-sql/4-lanes/sql-lane/src/sql/select-builder.ts`:
- Around line 50-68: deriveParamsFromAst currently maps every entry returned by
ast.collectParamRefs(), but collectParamRefs() can include the same ParamRef
instance multiple times; you must deduplicate by object identity before building
paramValues and paramDescriptors so placeholders align with lowering. Fix
deriveParamsFromAst by iterating collected, tracking seen ParamRef instances in
a Map/Set keyed by the instance, only adding the first occurrence to the output
arrays, and then map those unique entries to paramValues and paramDescriptors
(preserving name, source: 'dsl', codecId and nativeType only when present) so
queryPlan.params and meta.paramDescriptors stay in sync with the lowered SQL.
In `@packages/3-extensions/sql-orm-client/src/where-binding.ts`:
- Around line 36-41: The binary() handler in where-binding.ts should detect when
the bound right-hand side is a null parameter (e.g., a LiteralExpr(null)
converted to a ParamRef or a ParamRef with value===null) and, for eq/neq
operators, return a NullCheckExpr instead of constructing a BinaryExpr; update
binary(contract, expr) to call bindExpression for left and bindComparable for
right (as now), then if expr.op is 'eq' or 'neq' and the resolved right is a
ParamRef with a null value (or is a LiteralExpr(null) before conversion) produce
new NullCheckExpr(bindingColumn, expr.op === 'neq') otherwise fall back to
returning new BinaryExpr(expr.op, left, right) so SQL emits IS NULL / IS NOT
NULL correctly.
---
Nitpick comments:
In @.agents/skills/drive-pr-local-review/SKILL.md:
- Line 193: Update the two lowercase occurrences of "markdown" on the same line
in .agents/skills/drive-pr-local-review/SKILL.md to the proper noun "Markdown"
so both instances are capitalized; locate the line containing the guidance about
repo-relative paths and replace each "markdown" with "Markdown".
In @.agents/skills/drive-pr-walkthrough/SKILL.md:
- Line 113: Update the two occurrences of the lowercase word "markdown" to the
proper noun "Markdown" in the SKILL.md text—specifically change the phrase
"repo-relative markdown links" and the other instance on the subsequent line
(line 117) so both read "repo-relative Markdown links" and "Markdown"
respectively; locate these by searching for the exact phrase "repo-relative
markdown links" and the other "markdown" occurrence and replace with a
capitalized "Markdown".
In `@packages/2-sql/4-lanes/kysely-lane/src/transform/transform.ts`:
- Around line 28-112: transformKyselyToPnAst and
transformKyselyToPnAstCollectingParams duplicate most post-transform logic
(dispatch, paramDescriptors, projection and metaAdditions); extract that shared
logic into a helper like buildTransformResult(ast: QueryAst, ctx:
TransformContext) which runs extractRefsFromAst, calls ast.collectParamRefs to
build paramDescriptors, computes projection and projectionTypes using SelectAst
and ColumnRef, and assembles metaAdditions with ifDefined; keep each public
function responsible only for creating the correct ctx (with or without
parameters) and dispatching to
transformSelect/transformInsert/transformUpdate/transformDelete, then call
buildTransformResult and return its result.
In `@packages/2-sql/4-lanes/relational-core/test/ast/test-helpers.ts`:
- Around line 28-30: Replace the inline conditional object spreads for
expr.name, expr.codecId, and expr.nativeType with the repository standard
ifDefined() helper: instead of using ...(expr.name !== undefined && { name:
expr.name }) etc., import ifDefined from '@prisma-next/utils/defined' (if not
already) and use ifDefined(expr.name, () => ({ name: expr.name })) and similarly
for codecId and nativeType in the object assembly where these spreads occur
(look for variable/return building the expression object in test-helpers.ts).
In `@packages/3-extensions/sql-orm-client/src/where-binding.ts`:
- Around line 94-99: The code uses inline conditional object spreads when
building the options for ParamRef.of (using columnMeta?.codecId and
columnMeta?.nativeType); replace these inline spreads with the ifDefined()
utility from `@prisma-next/utils/defined` so the options object passes only
defined keys. Specifically, import ifDefined and change the options passed to
ParamRef.of(...) to use ifDefined({ codecId: columnMeta?.codecId }, {
nativeType: columnMeta?.nativeType }) (or equivalent calls) so the conditional
fields are added via ifDefined instead of ...(cond && {...}) while keeping name:
columnRef.column and the columnMeta lookup intact.
In `@packages/3-extensions/sql-orm-client/test/query-plan-mutations.test.ts`:
- Around line 27-40: usersColParam currently tolerates missing table/column
metadata; change it to fail fast by asserting the existence of users table
metadata and the specific column metadata from getTestContract before calling
ParamRef.of, and construct the options object explicitly (e.g., name, codecId,
nativeType) rather than using optional chaining or conditional spreads; throw a
clear Error if columns or columnMeta are missing so tests fail fast when
fixtures drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 062a10f5-447e-4fa2-867a-a10e33f2b7dc
📒 Files selected for processing (19)
.agents/skills/drive-pr-local-review/SKILL.md.agents/skills/drive-pr-walkthrough/SKILL.md.cursor/rules/README.md.cursor/rules/cursor-markdown-file-links.mdcpackages/2-sql/4-lanes/kysely-lane/src/transform/transform-expr.tspackages/2-sql/4-lanes/kysely-lane/src/transform/transform.tspackages/2-sql/4-lanes/relational-core/test/ast/test-helpers.tspackages/2-sql/4-lanes/sql-lane/src/sql/include-builder.tspackages/2-sql/4-lanes/sql-lane/src/sql/mutation-builder.tspackages/2-sql/4-lanes/sql-lane/src/sql/predicate-builder.tspackages/2-sql/4-lanes/sql-lane/src/sql/select-builder.tspackages/2-sql/4-lanes/sql-lane/test/include-builder.test.tspackages/2-sql/4-lanes/sql-lane/test/predicate-builder.test.tspackages/3-extensions/sql-orm-client/src/query-plan-meta.tspackages/3-extensions/sql-orm-client/src/query-plan-mutations.tspackages/3-extensions/sql-orm-client/src/where-binding.tspackages/3-extensions/sql-orm-client/src/where-interop.tspackages/3-extensions/sql-orm-client/test/query-plan-mutations.test.tspackages/3-targets/6-adapters/postgres/src/core/adapter.ts
✅ Files skipped from review due to trivial changes (2)
- .cursor/rules/README.md
- .cursor/rules/cursor-markdown-file-links.mdc
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/3-extensions/sql-orm-client/src/query-plan-meta.ts
- packages/3-extensions/sql-orm-client/src/where-interop.ts
- packages/3-extensions/sql-orm-client/src/query-plan-mutations.ts
- packages/2-sql/4-lanes/sql-lane/src/sql/include-builder.ts
- packages/3-targets/6-adapters/postgres/src/core/adapter.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/2-sql/4-lanes/sql-lane/src/sql/select-builder.ts (1)
50-66: Consider consolidating duplicatederiveParamsFromAsthelpers.This helper is duplicated across
select-builder.ts,mutation-builder.ts, andquery-plan-meta.ts. They share nearly identical logic. Consider importing from a single canonical location (e.g.,query-plan-meta.tsor a shared utils module) to reduce maintenance burden.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-lane/src/sql/select-builder.ts` around lines 50 - 66, deriveParamsFromAst is duplicated across select-builder.ts, mutation-builder.ts, and query-plan-meta.ts; extract the single implementation into a shared module (or make query-plan-meta.ts the canonical export) and replace the local copies by exporting deriveParamsFromAst from that module and importing it where needed (select-builder.ts, mutation-builder.ts). Ensure the exported function signature matches the original (collectParamRefs returning {value,name?,codecId}), update imports to use the shared function, and remove the duplicate implementations so only the canonical deriveParamsFromAst remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/2-sql/4-lanes/kysely-lane/src/transform/transform-expr.ts`:
- Around line 31-44: The function resolveParamOptions currently accepts refs?:
but treats refs as required at runtime; change its signature to require refs
(refs: { table: string; column: string }) and update all callers (e.g.,
transformValue, transformRightOperand and call sites in transform-dml.ts) to
pass a valid refs object, or alternatively implement explicit fallback handling
when refs is undefined — pick one approach and make signatures and usages
consistent (prefer making refs required to reflect runtime behavior).
---
Nitpick comments:
In `@packages/2-sql/4-lanes/sql-lane/src/sql/select-builder.ts`:
- Around line 50-66: deriveParamsFromAst is duplicated across select-builder.ts,
mutation-builder.ts, and query-plan-meta.ts; extract the single implementation
into a shared module (or make query-plan-meta.ts the canonical export) and
replace the local copies by exporting deriveParamsFromAst from that module and
importing it where needed (select-builder.ts, mutation-builder.ts). Ensure the
exported function signature matches the original (collectParamRefs returning
{value,name?,codecId}), update imports to use the shared function, and remove
the duplicate implementations so only the canonical deriveParamsFromAst remains.
🪄 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: 444b3b91-217b-4e12-a4f4-f93d7f7bc888
⛔ Files ignored due to path filters (3)
projects/structural-paramref/plan.mdis excluded by!projects/**projects/structural-paramref/reviews/code-review.mdis excluded by!projects/**projects/structural-paramref/reviews/walkthrough.mdis excluded by!projects/**
📒 Files selected for processing (60)
packages/2-sql/4-lanes/kysely-lane/src/plan.test.tspackages/2-sql/4-lanes/kysely-lane/src/transform/transform-expr.tspackages/2-sql/4-lanes/kysely-lane/src/transform/transform.tspackages/2-sql/4-lanes/kysely-lane/src/where-expr.ast.test.tspackages/2-sql/4-lanes/kysely-lane/src/where-expr.test.tspackages/2-sql/4-lanes/kysely-lane/src/where-expr.tspackages/2-sql/4-lanes/kysely-lane/test/build-plan.collect-params.test.tspackages/2-sql/4-lanes/relational-core/README.mdpackages/2-sql/4-lanes/relational-core/src/ast/types.tspackages/2-sql/4-lanes/relational-core/src/operations-registry.tspackages/2-sql/4-lanes/relational-core/src/types.tspackages/2-sql/4-lanes/relational-core/test/ast/common.test.tspackages/2-sql/4-lanes/relational-core/test/ast/rich-ast.test.tspackages/2-sql/4-lanes/relational-core/test/ast/test-helpers.tspackages/2-sql/4-lanes/relational-core/test/operations-registry.test.tspackages/2-sql/4-lanes/sql-lane/src/sql/mutation-builder.tspackages/2-sql/4-lanes/sql-lane/src/sql/predicate-builder.tspackages/2-sql/4-lanes/sql-lane/src/sql/select-builder.tspackages/2-sql/4-lanes/sql-lane/test/codecs.test.tspackages/2-sql/4-lanes/sql-lane/test/include-builder.test.tspackages/2-sql/4-lanes/sql-lane/test/mutation-builder.test.tspackages/2-sql/4-lanes/sql-lane/test/predicate-builder.test.tspackages/2-sql/4-lanes/sql-lane/test/rich-mutation.test.tspackages/2-sql/4-lanes/sql-lane/test/rich-select-and-include.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-comparison-operators.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-dml-vector-ops.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-dml.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-includes.builder.basic.test.tspackages/2-sql/4-lanes/sql-lane/test/sql.test.tspackages/2-sql/4-lanes/sql-lane/test/utils/guards.test.tspackages/2-sql/5-runtime/src/codecs/encoding.tspackages/2-sql/5-runtime/test/json-schema-validation.test.tspackages/2-sql/5-runtime/test/lints.test.tspackages/3-extensions/sql-orm-client/src/collection.tspackages/3-extensions/sql-orm-client/src/grouped-collection.tspackages/3-extensions/sql-orm-client/src/mutation-executor.tspackages/3-extensions/sql-orm-client/src/query-plan-aggregate.tspackages/3-extensions/sql-orm-client/src/query-plan-meta.tspackages/3-extensions/sql-orm-client/src/query-plan-mutations.tspackages/3-extensions/sql-orm-client/src/query-plan-select.tspackages/3-extensions/sql-orm-client/src/types.tspackages/3-extensions/sql-orm-client/src/where-binding.tspackages/3-extensions/sql-orm-client/src/where-interop.tspackages/3-extensions/sql-orm-client/src/where-utils.tspackages/3-extensions/sql-orm-client/test/collection.state.test.tspackages/3-extensions/sql-orm-client/test/integration/create.test.tspackages/3-extensions/sql-orm-client/test/query-plan-aggregate.test.tspackages/3-extensions/sql-orm-client/test/query-plan-mutations.test.tspackages/3-extensions/sql-orm-client/test/query-plan-select.test.tspackages/3-extensions/sql-orm-client/test/repository.test.tspackages/3-extensions/sql-orm-client/test/rich-collection.test.tspackages/3-extensions/sql-orm-client/test/rich-filters-and-where.test.tspackages/3-extensions/sql-orm-client/test/rich-query-plans.test.tspackages/3-extensions/sql-orm-client/test/where-binding.test.tspackages/3-extensions/sql-orm-client/test/where-interop.select-includes.test.tspackages/3-extensions/sql-orm-client/test/where-interop.test.tspackages/3-extensions/sql-orm-client/test/where-utils.test.tspackages/3-targets/6-adapters/postgres/test/adapter.test.tspackages/3-targets/6-adapters/postgres/test/operation-lowering.test.tspackages/3-targets/6-adapters/postgres/test/rich-adapter.test.ts
✅ Files skipped from review due to trivial changes (7)
- packages/2-sql/4-lanes/kysely-lane/src/plan.test.ts
- packages/2-sql/4-lanes/sql-lane/test/sql-comparison-operators.test.ts
- packages/3-extensions/sql-orm-client/test/rich-collection.test.ts
- packages/3-extensions/sql-orm-client/test/query-plan-mutations.test.ts
- packages/3-extensions/sql-orm-client/test/integration/create.test.ts
- packages/3-extensions/sql-orm-client/test/where-utils.test.ts
- packages/2-sql/4-lanes/sql-lane/test/predicate-builder.test.ts
🚧 Files skipped from review as they are similar to previous changes (28)
- packages/2-sql/4-lanes/sql-lane/test/sql.test.ts
- packages/2-sql/4-lanes/sql-lane/test/utils/guards.test.ts
- packages/2-sql/4-lanes/sql-lane/test/rich-select-and-include.test.ts
- packages/2-sql/4-lanes/sql-lane/test/sql-includes.builder.basic.test.ts
- packages/2-sql/4-lanes/kysely-lane/src/where-expr.test.ts
- packages/2-sql/4-lanes/kysely-lane/src/where-expr.ast.test.ts
- packages/3-targets/6-adapters/postgres/test/rich-adapter.test.ts
- packages/2-sql/4-lanes/sql-lane/test/include-builder.test.ts
- packages/2-sql/4-lanes/sql-lane/test/codecs.test.ts
- packages/3-extensions/sql-orm-client/test/repository.test.ts
- packages/2-sql/4-lanes/sql-lane/test/sql-dml-vector-ops.test.ts
- packages/2-sql/4-lanes/sql-lane/test/rich-mutation.test.ts
- packages/3-extensions/sql-orm-client/src/query-plan-meta.ts
- packages/2-sql/5-runtime/test/lints.test.ts
- packages/2-sql/4-lanes/sql-lane/test/sql-dml.test.ts
- packages/3-extensions/sql-orm-client/test/query-plan-aggregate.test.ts
- packages/3-targets/6-adapters/postgres/test/operation-lowering.test.ts
- packages/3-extensions/sql-orm-client/test/where-interop.select-includes.test.ts
- packages/3-extensions/sql-orm-client/src/query-plan-aggregate.ts
- packages/2-sql/4-lanes/relational-core/test/ast/test-helpers.ts
- packages/2-sql/4-lanes/sql-lane/test/mutation-builder.test.ts
- packages/3-extensions/sql-orm-client/test/rich-filters-and-where.test.ts
- packages/2-sql/5-runtime/src/codecs/encoding.ts
- packages/3-extensions/sql-orm-client/test/collection.state.test.ts
- packages/2-sql/4-lanes/relational-core/test/ast/rich-ast.test.ts
- packages/2-sql/4-lanes/kysely-lane/src/transform/transform.ts
- packages/2-sql/4-lanes/kysely-lane/test/build-plan.collect-params.test.ts
- packages/3-extensions/sql-orm-client/src/where-binding.ts
wmadden
force-pushed
the
tml-2103-restructure-paramref-to-use-structural-linkage-instead-of-implicit-1
branch
from
a288daa to
87c4b0f
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
packages/2-sql/4-lanes/sql-lane/src/sql/mutation-builder.ts (1)
35-45:⚠️ Potential issue | 🟠 MajorDeduplicate by
ParamRefidentity to avoid duplicate parameter slots.This helper maps every occurrence from
ast.collectParamRefs()without deduplication. If the sameParamRefinstance is shared in multiple AST positions, this creates duplicate entries inparamValuesandparamDescriptors, which would break the parameter ordering contract.Suggested fix
function deriveParamsFromAst(ast: { collectParamRefs(): ParamRef[] }) { - const collected = ast.collectParamRefs(); + const seen = new Set<ParamRef>(); + const collected: ParamRef[] = []; + for (const ref of ast.collectParamRefs()) { + if (!seen.has(ref)) { + seen.add(ref); + collected.push(ref); + } + } return { paramValues: collected.map((p) => p.value), paramDescriptors: collected.map((p) => ({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-lane/src/sql/mutation-builder.ts` around lines 35 - 45, deriveParamsFromAst currently uses every element from ast.collectParamRefs() which can duplicate entries when the same ParamRef instance appears multiple times; change it to deduplicate by object identity while preserving first-occurrence order: iterate the collected array, maintain a Set of seen ParamRef objects, push only unseen items into a new unique array, then build paramValues and paramDescriptors from that unique array (referencing deriveParamsFromAst, collected, ParamRef, paramValues, and paramDescriptors).packages/3-targets/6-adapters/postgres/src/core/adapter.ts (1)
123-130:⚠️ Potential issue | 🟠 MajorHandle shared
ParamRefinstances to avoid duplicate parameter slots.The current implementation unconditionally assigns a new index and pushes a value for every
ParamRefin the collected array. If the sameParamRefinstance is reused in multiple AST positions (shared reference), this will:
- Overwrite its index in
paramIndexMapwith the last position- Push duplicate values to
paramsThis breaks the structural-linkage model when a single
ParamRefnode is intentionally shared.Suggested fix
lower(ast: QueryAst, context: LowererContext<PostgresContract>) { const collectedParamRefs = ast.collectParamRefs(); const paramIndexMap: ParamIndexMap = new Map(); const params: unknown[] = []; - for (const [i, ref] of collectedParamRefs.entries()) { - paramIndexMap.set(ref, i + 1); - params.push(ref.value); + for (const ref of collectedParamRefs) { + if (paramIndexMap.has(ref)) { + continue; + } + paramIndexMap.set(ref, params.length + 1); + params.push(ref.value); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-targets/6-adapters/postgres/src/core/adapter.ts` around lines 123 - 130, In lower (function lower in adapter.ts) we currently assign a new index and push a value for every entry in collectedParamRefs, which causes duplicate slots when the same ParamRef instance is reused; change the loop over collectedParamRefs so it checks paramIndexMap.has(ref) first and only creates a new index and pushes ref.value when the ref is not already present, otherwise reuse the existing index from paramIndexMap; this preserves identity-sharing of ParamRef instances and prevents duplicate entries in params.
🧹 Nitpick comments (3)
packages/2-sql/4-lanes/kysely-lane/src/transform/transform.ts (1)
67-111: Consider extracting shared post-processing logic.Lines 67–111 and 152–196 are nearly identical—both build
paramDescriptorsfromcollectParamRefs(), computeprojection/projectionTypesforSelectAst, and assemblemetaAdditions. Only the function signature and context creation differ.Extracting a shared helper (e.g.,
buildMetaAdditions(ast, ctx)) would reduce duplication and simplify future maintenance.♻️ Sketch of extraction
function buildMetaAdditions(ast: QueryAst, ctx: TransformContext): TransformResult['metaAdditions'] { const refs = extractRefsFromAst(ast); const collectedParams = ast.collectParamRefs(); const paramDescriptors = collectedParams.map((p, index) => ({ ...(p.name !== undefined && { name: p.name }), source: 'lane' as const, index: index + 1, codecId: p.codecId, })); let projection: Record<string, string> | undefined; let projectionTypes: Record<string, string> | undefined; if (ast instanceof SelectAst) { // ... existing projection logic } return { refs, paramDescriptors, ...ifDefined('projection', projection), ...ifDefined('projectionTypes', projectionTypes && Object.keys(projectionTypes).length > 0 ? projectionTypes : undefined), ...ifDefined('selectAllIntent', ast instanceof SelectAst ? ast.selectAllIntent : undefined), ...ifDefined('limit', ast instanceof SelectAst ? ast.limit : undefined), }; }Then both transform functions simply:
return { ast, metaAdditions: buildMetaAdditions(ast, ctx) };Also applies to: 152-196
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/kysely-lane/src/transform/transform.ts` around lines 67 - 111, The same post-processing logic (using extractRefsFromAst, ast.collectParamRefs(), building paramDescriptors, computing projection/projectionTypes for SelectAst using ColumnRef and ctx.contract.storage, and assembling metaAdditions with ifDefined) is duplicated; extract it into a helper like buildMetaAdditions(ast: QueryAst, ctx: TransformContext) that returns the metaAdditions object, reuse it in both transform functions so they simply return { ast, metaAdditions: buildMetaAdditions(ast, ctx) }, and ensure the helper replicates the existing behavior for selectAllIntent and limit for SelectAst and preserves the 'lane' source and index arithmetic for paramDescriptors.packages/2-sql/4-lanes/sql-lane/test/sql-dml-vector-ops.test.ts (1)
137-138: Consider verifying the vector value inplan.params.The assertion at line 137 confirms the AST carries the value-carrying ParamRef. Since params are now derived from the AST's ParamRefs, verifying that
[1, 2, 3]also appears inplan.paramswould strengthen coverage of the value-carrying behavior.🔧 Suggested addition
expect(left.args).toEqual([ParamRef.of([1, 2, 3], { name: 'arg_0', codecId: 'pg/vector@1' })]); + expect(plan.params).toContainEqual([1, 2, 3]); expect(plan.params).toContain(0.5);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-lane/test/sql-dml-vector-ops.test.ts` around lines 137 - 138, Add an assertion to verify the actual vector value from the AST appears in the execution plan's params: after the existing checks on left.args and ParamRef.of, assert that plan.params includes the array [1, 2, 3] (or otherwise contains an element equal to that vector) so the test confirms that ParamRef values are propagated into plan.params; locate the assertion near the existing references to left.args, ParamRef, and plan.params in sql-dml-vector-ops.test.ts.packages/2-sql/4-lanes/sql-lane/src/sql/select-builder.ts (1)
60-63: UseifDefined()for the optionalnamefield.This adds a new inline conditional spread in a package that standardizes on
ifDefined(), so it would be better to keep the helper aligned with the repo pattern.As per coding guidelines,
Use ifDefined() from@prisma-next/utils/definedfor conditional object spreads instead of inline conditional spread patterns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-lane/src/sql/select-builder.ts` around lines 60 - 63, Replace the inline conditional spread of the optional name in the paramDescriptors mapping with the repository helper: import and use ifDefined from `@prisma-next/utils/defined` and change the collected.map callback to set name: ifDefined(p.name) instead of ...(p.name !== undefined && { name: p.name }); keep source: 'dsl' and codecId: p.codecId unchanged so the paramDescriptors mapping uses the standardized ifDefined() pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/2-sql/4-lanes/sql-lane/src/sql/select-builder.ts`:
- Around line 50-66: The deriveParamsFromAst function currently returns
paramValues and paramDescriptors but not paramCodecs; modify it to also produce
a paramCodecs map by iterating the same collected refs (ast.collectParamRefs())
and, for each ref with a defined name, set paramCodecs[name] = p.codecId so
buildMeta receives a complete name→codec map; update callers (e.g., places that
previously built paramCodecs from the top-level whereResult or in
buildIncludeJoinArtifact) to use the returned paramCodecs from
deriveParamsFromAst, or remove the old separate map logic if no longer needed
(also apply the same change in the other locations mentioned such as the usages
around buildIncludeJoinArtifact and buildMeta).
In `@packages/3-extensions/sql-orm-client/test/query-plan-select.test.ts`:
- Around line 50-61: Rename the test to match the actual observed ordering:
update the it(...) description from "orders include filter params after
top-level params in collectParamRefs" to something like "orders include filter
params before top-level params in collectParamRefs" (or "collects params in AST
traversal order (includes first)") so the test description aligns with the
assertions around plan.params and plan.meta.paramDescriptors produced by
createCollection() and compileSelectWithIncludeStrategy(...) for the
'correlated' include strategy.
In `@packages/3-extensions/sql-orm-client/test/where-binding.test.ts`:
- Around line 45-47: The test currently only checks the .value of the right side
of BinaryExprs and can miss that the expression was not converted to a ParamRef;
update the assertions in where-binding.test.ts (around the bound/and/AndExpr and
BinaryExpr checks) to assert the right-hand expression is a ParamRef (or has
kind 'ParamRef') for each branch before asserting its value, and do the same for
the other occurrence around line 57 so the test verifies type conversion rather
than just matching the literal value.
- Around line 109-111: Consolidate the three related assertions into a single
object matcher: replace the three expects that inspect list.values and ParamRef
(the assertions using list and ParamRef around lines 109-111 and similarly
208-210) with one expect that asserts the array shape, e.g.
expect(list.values).toMatchObject([{ value: 1 }, { value: 2 }]) (or include type
checks with expect.any(ParamRef) if you need instance checks), so the test
verifies both elements in one matcher for readability and consistency.
---
Duplicate comments:
In `@packages/2-sql/4-lanes/sql-lane/src/sql/mutation-builder.ts`:
- Around line 35-45: deriveParamsFromAst currently uses every element from
ast.collectParamRefs() which can duplicate entries when the same ParamRef
instance appears multiple times; change it to deduplicate by object identity
while preserving first-occurrence order: iterate the collected array, maintain a
Set of seen ParamRef objects, push only unseen items into a new unique array,
then build paramValues and paramDescriptors from that unique array (referencing
deriveParamsFromAst, collected, ParamRef, paramValues, and paramDescriptors).
In `@packages/3-targets/6-adapters/postgres/src/core/adapter.ts`:
- Around line 123-130: In lower (function lower in adapter.ts) we currently
assign a new index and push a value for every entry in collectedParamRefs, which
causes duplicate slots when the same ParamRef instance is reused; change the
loop over collectedParamRefs so it checks paramIndexMap.has(ref) first and only
creates a new index and pushes ref.value when the ref is not already present,
otherwise reuse the existing index from paramIndexMap; this preserves
identity-sharing of ParamRef instances and prevents duplicate entries in params.
---
Nitpick comments:
In `@packages/2-sql/4-lanes/kysely-lane/src/transform/transform.ts`:
- Around line 67-111: The same post-processing logic (using extractRefsFromAst,
ast.collectParamRefs(), building paramDescriptors, computing
projection/projectionTypes for SelectAst using ColumnRef and
ctx.contract.storage, and assembling metaAdditions with ifDefined) is
duplicated; extract it into a helper like buildMetaAdditions(ast: QueryAst, ctx:
TransformContext) that returns the metaAdditions object, reuse it in both
transform functions so they simply return { ast, metaAdditions:
buildMetaAdditions(ast, ctx) }, and ensure the helper replicates the existing
behavior for selectAllIntent and limit for SelectAst and preserves the 'lane'
source and index arithmetic for paramDescriptors.
In `@packages/2-sql/4-lanes/sql-lane/src/sql/select-builder.ts`:
- Around line 60-63: Replace the inline conditional spread of the optional name
in the paramDescriptors mapping with the repository helper: import and use
ifDefined from `@prisma-next/utils/defined` and change the collected.map callback
to set name: ifDefined(p.name) instead of ...(p.name !== undefined && { name:
p.name }); keep source: 'dsl' and codecId: p.codecId unchanged so the
paramDescriptors mapping uses the standardized ifDefined() pattern.
In `@packages/2-sql/4-lanes/sql-lane/test/sql-dml-vector-ops.test.ts`:
- Around line 137-138: Add an assertion to verify the actual vector value from
the AST appears in the execution plan's params: after the existing checks on
left.args and ParamRef.of, assert that plan.params includes the array [1, 2, 3]
(or otherwise contains an element equal to that vector) so the test confirms
that ParamRef values are propagated into plan.params; locate the assertion near
the existing references to left.args, ParamRef, and plan.params in
sql-dml-vector-ops.test.ts.
🪄 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: 9ba51669-3fdf-47d2-80d2-378d7b7c7c5d
⛔ Files ignored due to path filters (3)
projects/structural-paramref/plan.mdis excluded by!projects/**projects/structural-paramref/reviews/code-review.mdis excluded by!projects/**projects/structural-paramref/reviews/walkthrough.mdis excluded by!projects/**
📒 Files selected for processing (61)
packages/2-sql/4-lanes/kysely-lane/src/plan.test.tspackages/2-sql/4-lanes/kysely-lane/src/transform/transform-expr.tspackages/2-sql/4-lanes/kysely-lane/src/transform/transform.tspackages/2-sql/4-lanes/kysely-lane/src/where-expr.ast.test.tspackages/2-sql/4-lanes/kysely-lane/src/where-expr.test.tspackages/2-sql/4-lanes/kysely-lane/src/where-expr.tspackages/2-sql/4-lanes/kysely-lane/test/build-plan.collect-params.test.tspackages/2-sql/4-lanes/relational-core/README.mdpackages/2-sql/4-lanes/relational-core/src/ast/types.tspackages/2-sql/4-lanes/relational-core/src/operations-registry.tspackages/2-sql/4-lanes/relational-core/src/types.tspackages/2-sql/4-lanes/relational-core/test/ast/common.test.tspackages/2-sql/4-lanes/relational-core/test/ast/rich-ast.test.tspackages/2-sql/4-lanes/relational-core/test/ast/test-helpers.tspackages/2-sql/4-lanes/relational-core/test/operations-registry.test.tspackages/2-sql/4-lanes/sql-lane/src/sql/mutation-builder.tspackages/2-sql/4-lanes/sql-lane/src/sql/predicate-builder.tspackages/2-sql/4-lanes/sql-lane/src/sql/select-builder.tspackages/2-sql/4-lanes/sql-lane/test/codecs.test.tspackages/2-sql/4-lanes/sql-lane/test/include-builder.test.tspackages/2-sql/4-lanes/sql-lane/test/mutation-builder.test.tspackages/2-sql/4-lanes/sql-lane/test/predicate-builder.test.tspackages/2-sql/4-lanes/sql-lane/test/rich-mutation.test.tspackages/2-sql/4-lanes/sql-lane/test/rich-select-and-include.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-comparison-operators.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-dml-vector-ops.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-dml.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-includes.builder.basic.test.tspackages/2-sql/4-lanes/sql-lane/test/sql.test.tspackages/2-sql/4-lanes/sql-lane/test/utils/guards.test.tspackages/2-sql/5-runtime/src/codecs/encoding.tspackages/2-sql/5-runtime/test/json-schema-validation.test.tspackages/2-sql/5-runtime/test/lints.test.tspackages/3-extensions/sql-orm-client/src/collection.tspackages/3-extensions/sql-orm-client/src/grouped-collection.tspackages/3-extensions/sql-orm-client/src/mutation-executor.tspackages/3-extensions/sql-orm-client/src/query-plan-aggregate.tspackages/3-extensions/sql-orm-client/src/query-plan-meta.tspackages/3-extensions/sql-orm-client/src/query-plan-mutations.tspackages/3-extensions/sql-orm-client/src/query-plan-select.tspackages/3-extensions/sql-orm-client/src/types.tspackages/3-extensions/sql-orm-client/src/where-binding.tspackages/3-extensions/sql-orm-client/src/where-interop.tspackages/3-extensions/sql-orm-client/src/where-utils.tspackages/3-extensions/sql-orm-client/test/collection.state.test.tspackages/3-extensions/sql-orm-client/test/integration/create.test.tspackages/3-extensions/sql-orm-client/test/query-plan-aggregate.test.tspackages/3-extensions/sql-orm-client/test/query-plan-mutations.test.tspackages/3-extensions/sql-orm-client/test/query-plan-select.test.tspackages/3-extensions/sql-orm-client/test/repository.test.tspackages/3-extensions/sql-orm-client/test/rich-collection.test.tspackages/3-extensions/sql-orm-client/test/rich-filters-and-where.test.tspackages/3-extensions/sql-orm-client/test/rich-query-plans.test.tspackages/3-extensions/sql-orm-client/test/where-binding.test.tspackages/3-extensions/sql-orm-client/test/where-interop.select-includes.test.tspackages/3-extensions/sql-orm-client/test/where-interop.test.tspackages/3-extensions/sql-orm-client/test/where-utils.test.tspackages/3-targets/6-adapters/postgres/src/core/adapter.tspackages/3-targets/6-adapters/postgres/test/adapter.test.tspackages/3-targets/6-adapters/postgres/test/operation-lowering.test.tspackages/3-targets/6-adapters/postgres/test/rich-adapter.test.ts
✅ Files skipped from review due to trivial changes (10)
- packages/2-sql/4-lanes/sql-lane/test/utils/guards.test.ts
- packages/2-sql/4-lanes/sql-lane/test/codecs.test.ts
- packages/2-sql/4-lanes/sql-lane/test/sql-dml.test.ts
- packages/2-sql/4-lanes/relational-core/README.md
- packages/2-sql/4-lanes/kysely-lane/src/where-expr.ast.test.ts
- packages/3-extensions/sql-orm-client/test/rich-query-plans.test.ts
- packages/3-extensions/sql-orm-client/test/integration/create.test.ts
- packages/3-extensions/sql-orm-client/src/types.ts
- packages/3-extensions/sql-orm-client/test/where-interop.test.ts
- packages/3-extensions/sql-orm-client/src/where-binding.ts
🚧 Files skipped from review as they are similar to previous changes (27)
- packages/2-sql/4-lanes/sql-lane/test/sql-comparison-operators.test.ts
- packages/2-sql/4-lanes/sql-lane/test/rich-select-and-include.test.ts
- packages/2-sql/4-lanes/sql-lane/test/sql.test.ts
- packages/2-sql/4-lanes/kysely-lane/src/plan.test.ts
- packages/2-sql/4-lanes/sql-lane/test/include-builder.test.ts
- packages/2-sql/4-lanes/kysely-lane/src/where-expr.test.ts
- packages/2-sql/4-lanes/sql-lane/test/sql-includes.builder.basic.test.ts
- packages/3-extensions/sql-orm-client/src/query-plan-meta.ts
- packages/3-extensions/sql-orm-client/test/where-utils.test.ts
- packages/3-extensions/sql-orm-client/test/query-plan-mutations.test.ts
- packages/2-sql/4-lanes/relational-core/test/ast/rich-ast.test.ts
- packages/3-extensions/sql-orm-client/src/grouped-collection.ts
- packages/3-extensions/sql-orm-client/test/repository.test.ts
- packages/3-targets/6-adapters/postgres/test/operation-lowering.test.ts
- packages/3-targets/6-adapters/postgres/test/adapter.test.ts
- packages/2-sql/4-lanes/relational-core/test/ast/test-helpers.ts
- packages/3-extensions/sql-orm-client/test/query-plan-aggregate.test.ts
- packages/2-sql/4-lanes/sql-lane/test/mutation-builder.test.ts
- packages/3-targets/6-adapters/postgres/test/rich-adapter.test.ts
- packages/3-extensions/sql-orm-client/src/collection.ts
- packages/2-sql/4-lanes/sql-lane/test/predicate-builder.test.ts
- packages/2-sql/5-runtime/test/lints.test.ts
- packages/2-sql/4-lanes/kysely-lane/src/transform/transform-expr.ts
- packages/2-sql/5-runtime/src/codecs/encoding.ts
- packages/2-sql/5-runtime/test/json-schema-validation.test.ts
- packages/2-sql/4-lanes/relational-core/src/ast/types.ts
- packages/3-extensions/sql-orm-client/src/query-plan-mutations.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/2-sql/4-lanes/sql-lane/src/sql/mutation-builder.ts (1)
329-337:⚠️ Potential issue | 🟠 MajorUPDATE/DELETE only record one predicate codec in plan meta.
buildWhereExpr()gives you a single{ paramName, codecId }pair here. If the predicate tree materializes more than one namedParamRef,paramsandparamDescriptorswill include all of them from the finalized AST, butmeta.annotations.codecswill keep at most one WHERE entry. Derive the name→codec map from the same AST walk and reuse it forbuildMeta().Also applies to: 349-364, 448-456, 468-483
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-lane/src/sql/mutation-builder.ts` around lines 329 - 337, The code only records one WHERE param codec because it trusts buildWhereExpr()'s single {paramName, codecId} return; instead, walk the finalized WHERE AST (the same walk that produces params and paramDescriptors from ParamRef nodes) to build a complete name→codec map (e.g., iterate ParamRef nodes in wherePredicate using paramsMap) and populate paramCodecs/meta.annotations.codecs from that full map; then pass or reuse that map into buildMeta() so UPDATE/DELETE sites that call buildWhereExpr (and other similar call sites) record all predicate codecs rather than only the last/first one.
♻️ Duplicate comments (1)
packages/2-sql/4-lanes/sql-lane/src/sql/select-builder.ts (1)
50-66:⚠️ Potential issue | 🟠 Major
meta.annotations.codecsis still incomplete after the AST migration.
paramDescriptorsnow come fromcollectParamRefs(), butparamCodecsis still stitched together from the top-levelwhereResult. Named refs introduced by include child filters, projection/order expressions, or additional predicate nodes can make it into the AST andparamDescriptorswhile never reachingbuildMeta(), so the public plan metadata stays partial. Please derive the name→codec map from the same deduped AST walk and pass that through instead.Also applies to: 317-323, 335-335, 373-395
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-lane/src/sql/select-builder.ts` around lines 50 - 66, The current deriveParamsFromAst builds paramDescriptors from the deduped ast.collectParamRefs() but paramCodecs / meta.annotations.codecs are still assembled elsewhere (e.g., in buildMeta/from whereResult), leading to incomplete codec mapping; update deriveParamsFromAst (the function using ast.collectParamRefs()) to also produce a name→codec map by iterating the same deduped collected array, collecting entries where p.name is defined and mapping p.name to p.codecId, and return that map alongside paramValues and paramDescriptors; then update callers (places that previously read paramCodecs from whereResult/buildMeta) to accept and pass through this new codec map so meta.annotations.codecs is derived from the same deduped AST walk (apply the same change to the other instances noted around the other blocks).
🧹 Nitpick comments (3)
packages/2-sql/4-lanes/kysely-lane/src/transform/transform-expr.ts (1)
46-72: ResolvecodecIdonly on theParamRefbranches.
transformValue()now does the contract lookup even when it returns aLiteralExpr. Deferring that lookup keeps literal-only paths independent from codec metadata and avoids extra work on every immediate literal.♻️ Proposed refactor
export function transformValue( node: unknown, ctx: TransformContext, refs: { table: string; column: string }, ): ParamRef | LiteralExpr { - const options = resolveParamOptions(ctx, refs); + const toParamRef = (value: unknown): ParamRef => + ParamRef.of(value, resolveParamOptions(ctx, refs)); if (!isOperationNode(node)) { if (ctx.parameters) { const nextCompiledParam = ctx.parameters[ctx.paramIndex]; if (ctx.paramIndex < ctx.parameters.length && Object.is(nextCompiledParam, node)) { advanceParamCursor(ctx); - return ParamRef.of(node, options); + return toParamRef(node); } return LiteralExpr.of(node); } advanceParamCursor(ctx); - return ParamRef.of(node, options); + return toParamRef(node); } if (ValueNode.is(node)) { if (node.immediate === true) { return LiteralExpr.of(node.value); } advanceParamCursor(ctx); - return ParamRef.of(node.value, options); + return toParamRef(node.value); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/kysely-lane/src/transform/transform-expr.ts` around lines 46 - 72, The function transformValue currently calls resolveParamOptions(ctx, refs) eagerly and thus performs codec/contract lookup even when returning LiteralExpr; change it so resolveParamOptions is invoked only on branches that produce a ParamRef: move the call into the branches that return ParamRef (the branch where ctx.parameters is present and the matched compiled param path that returns ParamRef.of(...), the fallback that calls advanceParamCursor then ParamRef.of(...), and the ValueNode.is branch where immediate is false and you advanceParamCursor then return ParamRef.of(...)). Keep LiteralExpr.of(...) returns (including the ValueNode.immediate === true case) free of any resolveParamOptions/codec lookups. Ensure references to resolveParamOptions, ParamRef.of, LiteralExpr.of, advanceParamCursor, and ValueNode.is are updated accordingly.packages/2-sql/4-lanes/sql-lane/src/sql/mutation-builder.ts (1)
39-42: UseifDefined()instead of the inline conditional spread.This helper is using the conditional-spread pattern the repo has standardized away from.
As per coding guidelines, "Use
ifDefined()from@prisma-next/utils/definedfor conditional object spreads instead of inline conditional spread patterns".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-lane/src/sql/mutation-builder.ts` around lines 39 - 42, Replace the inline conditional spread inside the collected.map for paramDescriptors with the repo-standard ifDefined helper: import ifDefined from '@prisma-next/utils/defined' and use it to conditionally include the name property instead of the pattern ...(p.name !== undefined && { name: p.name }); update the mapping expression in mutation-builder.ts (the collected.map callback that builds paramDescriptors) to call ifDefined('name', p.name) so conditional spreads follow the shared utility convention.packages/2-sql/4-lanes/sql-lane/src/sql/select-builder.ts (1)
60-63: UseifDefined()for the optionalnamefield.The new helper reintroduces an inline conditional spread. Please switch this descriptor construction to the shared
ifDefined()helper to stay consistent with the repo pattern.As per coding guidelines, "Use
ifDefined()from@prisma-next/utils/definedfor conditional object spreads instead of inline conditional spread patterns".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-lane/src/sql/select-builder.ts` around lines 60 - 63, Replace the inline conditional spread used when building paramDescriptors inside the collected.map callback with the shared ifDefined helper from '@prisma-next/utils/defined'; specifically, in the expression currently doing ...(p.name !== undefined && { name: p.name }), call ifDefined(p.name, (name) => ({ name })) (or the repo’s established ifDefined signature) so the object becomes { ...ifDefined(...) , source: 'dsl' as const, codecId: p.codecId }; also add the ifDefined import at the top of the file if it’s not already imported.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/2-sql/4-lanes/sql-lane/src/sql/mutation-builder.ts`:
- Around line 329-337: The code only records one WHERE param codec because it
trusts buildWhereExpr()'s single {paramName, codecId} return; instead, walk the
finalized WHERE AST (the same walk that produces params and paramDescriptors
from ParamRef nodes) to build a complete name→codec map (e.g., iterate ParamRef
nodes in wherePredicate using paramsMap) and populate
paramCodecs/meta.annotations.codecs from that full map; then pass or reuse that
map into buildMeta() so UPDATE/DELETE sites that call buildWhereExpr (and other
similar call sites) record all predicate codecs rather than only the last/first
one.
---
Duplicate comments:
In `@packages/2-sql/4-lanes/sql-lane/src/sql/select-builder.ts`:
- Around line 50-66: The current deriveParamsFromAst builds paramDescriptors
from the deduped ast.collectParamRefs() but paramCodecs /
meta.annotations.codecs are still assembled elsewhere (e.g., in buildMeta/from
whereResult), leading to incomplete codec mapping; update deriveParamsFromAst
(the function using ast.collectParamRefs()) to also produce a name→codec map by
iterating the same deduped collected array, collecting entries where p.name is
defined and mapping p.name to p.codecId, and return that map alongside
paramValues and paramDescriptors; then update callers (places that previously
read paramCodecs from whereResult/buildMeta) to accept and pass through this new
codec map so meta.annotations.codecs is derived from the same deduped AST walk
(apply the same change to the other instances noted around the other blocks).
---
Nitpick comments:
In `@packages/2-sql/4-lanes/kysely-lane/src/transform/transform-expr.ts`:
- Around line 46-72: The function transformValue currently calls
resolveParamOptions(ctx, refs) eagerly and thus performs codec/contract lookup
even when returning LiteralExpr; change it so resolveParamOptions is invoked
only on branches that produce a ParamRef: move the call into the branches that
return ParamRef (the branch where ctx.parameters is present and the matched
compiled param path that returns ParamRef.of(...), the fallback that calls
advanceParamCursor then ParamRef.of(...), and the ValueNode.is branch where
immediate is false and you advanceParamCursor then return ParamRef.of(...)).
Keep LiteralExpr.of(...) returns (including the ValueNode.immediate === true
case) free of any resolveParamOptions/codec lookups. Ensure references to
resolveParamOptions, ParamRef.of, LiteralExpr.of, advanceParamCursor, and
ValueNode.is are updated accordingly.
In `@packages/2-sql/4-lanes/sql-lane/src/sql/mutation-builder.ts`:
- Around line 39-42: Replace the inline conditional spread inside the
collected.map for paramDescriptors with the repo-standard ifDefined helper:
import ifDefined from '@prisma-next/utils/defined' and use it to conditionally
include the name property instead of the pattern ...(p.name !== undefined && {
name: p.name }); update the mapping expression in mutation-builder.ts (the
collected.map callback that builds paramDescriptors) to call ifDefined('name',
p.name) so conditional spreads follow the shared utility convention.
In `@packages/2-sql/4-lanes/sql-lane/src/sql/select-builder.ts`:
- Around line 60-63: Replace the inline conditional spread used when building
paramDescriptors inside the collected.map callback with the shared ifDefined
helper from '@prisma-next/utils/defined'; specifically, in the expression
currently doing ...(p.name !== undefined && { name: p.name }), call
ifDefined(p.name, (name) => ({ name })) (or the repo’s established ifDefined
signature) so the object becomes { ...ifDefined(...) , source: 'dsl' as const,
codecId: p.codecId }; also add the ifDefined import at the top of the file if
it’s not already imported.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 239ffe16-0cbf-4fb7-b01d-d7eee639e5e2
📒 Files selected for processing (6)
packages/2-sql/4-lanes/kysely-lane/src/transform/transform-expr.tspackages/2-sql/4-lanes/sql-lane/src/sql/mutation-builder.tspackages/2-sql/4-lanes/sql-lane/src/sql/select-builder.tspackages/2-sql/4-lanes/sql-lane/test/rich-select-and-include.test.tspackages/3-extensions/sql-orm-client/src/query-plan-meta.tspackages/3-targets/6-adapters/postgres/src/core/adapter.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/3-extensions/sql-orm-client/src/query-plan-meta.ts
- packages/3-targets/6-adapters/postgres/src/core/adapter.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/3-targets/6-adapters/postgres/src/core/adapter.ts (1)
448-454: Consider including ParamRef details in the error message.When the invariant is violated, including the ref's
nameandcodecIdwould help diagnose where the untrackedParamReforiginated.🔧 Suggested improvement
function renderParamRef(ref: ParamRef, pim?: ParamIndexMap): string { const index = pim?.get(ref); if (index === undefined) { - throw new Error('ParamRef not found in index map'); + throw new Error( + `ParamRef not found in index map (name=${ref.name ?? 'unnamed'}, codecId=${ref.codecId ?? 'none'})`, + ); } return renderTypedParam(index, ref.codecId); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-targets/6-adapters/postgres/src/core/adapter.ts` around lines 448 - 454, The error thrown in renderParamRef when a ParamRef is missing from the ParamIndexMap should include identifying details to aid debugging; update the throw to include ref.name and ref.codecId (and any other useful fields from the ParamRef) in the error message, e.g., include the ParamRef's name and codecId alongside the existing message so callers can see which ParamRef (from renderParamRef) was untracked.packages/3-extensions/sql-orm-client/test/where-binding.test.ts (1)
45-47: Add pre-cast runtime type assertions for consistency and clearer failures.A few blocks cast
bound as ...directly. Consider addingexpect(bound).toBeInstanceOf(...)before each cast to match the stronger pattern already used in neighboring tests.Also applies to: 120-120, 134-134, 172-172, 183-183, 218-218
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/test/where-binding.test.ts` around lines 45 - 47, Add explicit runtime assertions before each test cast: e.g., before "const and = bound as AndExpr" add "expect(bound).toBeInstanceOf(AndExpr)"; likewise for other direct casts of bound in this file (the places producing BinaryExpr/InExpr casts) add "expect(bound).toBeInstanceOf(BinaryExpr)" or "expect(bound).toBeInstanceOf(InExpr)" as appropriate so the subsequent casts (symbols: and, andRight0, andRight1 and the other cast sites at the noted locations) fail with a clear message if the type is wrong.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/3-extensions/sql-orm-client/test/where-binding.test.ts`:
- Around line 127-137: The test checks only value/type preservation of the
right-hand ParamRef after bindWhereExpr but should assert reference identity to
ensure the original ParamRef instance is reused; update the assertions in the
'preserves ParamRef on the right side without rebinding' test (and the other
"without rebinding" cases later in the same file) to assert the bound
BinaryExpr.right is the identical ParamRef instance from the fixture (use an
object-identity assertion against the original ParamRef created with
ParamRef.of) instead of only checking instance type or value.
---
Nitpick comments:
In `@packages/3-extensions/sql-orm-client/test/where-binding.test.ts`:
- Around line 45-47: Add explicit runtime assertions before each test cast:
e.g., before "const and = bound as AndExpr" add
"expect(bound).toBeInstanceOf(AndExpr)"; likewise for other direct casts of
bound in this file (the places producing BinaryExpr/InExpr casts) add
"expect(bound).toBeInstanceOf(BinaryExpr)" or
"expect(bound).toBeInstanceOf(InExpr)" as appropriate so the subsequent casts
(symbols: and, andRight0, andRight1 and the other cast sites at the noted
locations) fail with a clear message if the type is wrong.
In `@packages/3-targets/6-adapters/postgres/src/core/adapter.ts`:
- Around line 448-454: The error thrown in renderParamRef when a ParamRef is
missing from the ParamIndexMap should include identifying details to aid
debugging; update the throw to include ref.name and ref.codecId (and any other
useful fields from the ParamRef) in the error message, e.g., include the
ParamRef's name and codecId alongside the existing message so callers can see
which ParamRef (from renderParamRef) was untracked.
🪄 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: ff368bad-ae14-43d2-8750-cc53102728c6
📒 Files selected for processing (8)
packages/2-sql/4-lanes/kysely-lane/src/transform/transform-expr.tspackages/2-sql/4-lanes/sql-lane/src/sql/mutation-builder.tspackages/2-sql/4-lanes/sql-lane/src/sql/select-builder.tspackages/2-sql/4-lanes/sql-lane/test/rich-select-and-include.test.tspackages/3-extensions/sql-orm-client/src/query-plan-meta.tspackages/3-extensions/sql-orm-client/test/query-plan-select.test.tspackages/3-extensions/sql-orm-client/test/where-binding.test.tspackages/3-targets/6-adapters/postgres/src/core/adapter.ts
✅ Files skipped from review due to trivial changes (1)
- packages/2-sql/4-lanes/sql-lane/src/sql/mutation-builder.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/2-sql/4-lanes/sql-lane/test/rich-select-and-include.test.ts
- packages/3-extensions/sql-orm-client/src/query-plan-meta.ts
- packages/3-extensions/sql-orm-client/test/query-plan-select.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/3-extensions/sql-orm-client/test/where-binding.test.ts`:
- Around line 32-35: The tests currently assert that literals become ParamRef
and preserve .value but do not verify the resolved codec identifier; update the
relevant tests (those that create BinaryExpr with ParamRef and the IN/recursive
bind cases) to also assert the ParamRef.codecId equals the expected codec
identifier for the bound value (for example assert (binary.right as
ParamRef).codecId === expectedCodecId for the scalar bind and assert codecId on
each ParamRef returned by ast.collectParamRefs() for the IN/recursive case),
locating the ParamRef instances via the existing bindings (e.g., BinaryExpr ->
binary.right or the array returned by ast.collectParamRefs()) and use the known
expected codec ids for the fixture values.
- Around line 82-84: The test currently only checks the outer node is an
ExistsExpr and not that rebinding recurses into the nested SELECT; modify the
three EXISTS-case assertions (the blocks that assert
expect(bound).toBeInstanceOf(ExistsExpr)) so the nested WHERE contains a
bindable literal and then assert the inner BinaryExpr.right (inside the nested
SELECT's WHERE) becomes a ParamRef after calling bindWhereExpr; specifically,
update the nested query fixture to include a literal on the right-hand side,
locate the bound node via ExistsExpr, drill into its subquery to find the inner
BinaryExpr, and add an assertion that (innerBinaryExpr as BinaryExpr).right is
an instance of ParamRef to prove recursive rebinding by bindWhereExpr.
🪄 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: 7c330523-1e6a-4881-ae57-abedc8af9737
📒 Files selected for processing (1)
packages/3-extensions/sql-orm-client/test/where-binding.test.ts
SevInf
approved these changes
Mar 27, 2026
Contributor
SevInf
left a comment
SevInf
left a comment
There was a problem hiding this comment.
Just a few nits, feel free to ignore any of them.
LGTM
Replace ParamRef's implicit numeric-index linkage with structural value-carrying. Spec covers the problem (silent off-by-one bugs, fragile offset arithmetic, Postgres convention leaking into target-agnostic code) and the chosen approach (Option A: AST carries values directly). Plan breaks the work into 2 milestones and 23 tasks.
…al linkage ParamRef now holds value, codecId, and nativeType directly instead of a numeric index. BoundWhereExpr drops its params/paramDescriptors arrays. DML AST nodes (Insert/Update/Delete) gain collectParamRefs() for deterministic parameter collection.
Remove explicit paramValues/paramDescriptors accumulators from builders. ParamRef nodes now carry values, so buildWhereExpr drops its descriptor and values array params. Query plans derive params at finalization by walking the completed AST.
Drop BindState, offset helpers, and index-tracking from where-binding, where-utils, and where-interop. Value-carrying ParamRef nodes make param accumulation unnecessary. Query plan compilers now derive params from the finished AST via collectParamRefs.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/3-extensions/sql-orm-client/src/query-plan-mutations.ts`:
- Around line 87-90: The code in query-plan-mutations.ts currently uses optional
chaining when resolving codecId and throws "Unknown column" even if the table is
missing; update the function (the same area that mirrors toParamAssignments) to
first verify that contract.storage.tables[tableName] exists and, if not, throw a
distinct "Unknown table \"{tableName}\"" error before iterating rows, then
inside the row loop resolve codecId from
contract.storage.tables[tableName].columns[column]?.codecId and throw "Unknown
column \"{column}\" in table \"{tableName}\"" when codecId is missing; move the
table existence check outside the row loop since tableName is constant to avoid
repeated checks.
🪄 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: a9157df7-319e-4d93-88ab-e0042b29154a
📒 Files selected for processing (2)
packages/3-extensions/sql-orm-client/src/query-plan-mutations.tspackages/3-extensions/sql-orm-client/test/where-binding.test.ts
Remove params/paramDescriptors from TransformContext. transformValue now
creates value-carrying ParamRef directly. buildKyselyWhereExpr returns
only { expr } since params live in the AST. Param descriptors are
derived from collectParamRefs at finalization.
…ering The adapter lower() now calls collectParamRefs() on the AST to build a ParamIndexMap and params array. Render functions thread this map to emit $N placeholders. renderParamRef replaces renderParam, using codecId from the ParamRef itself instead of contract table metadata lookups.
Migrate ~38 test files from ParamRef.of(index) to ParamRef.of(value,
{ name, codecId, nativeType }). Remove assertions on BoundWhereExpr
params/paramDescriptors arrays and withIndex calls. Test helpers updated
to use new param() signature and shiftParamRef() for rewrite tests.
Cover Insert (rows + onConflict, skipping DefaultValueExpr), Update (set + where, skipping ColumnRef), Delete (where + empty), and Select (where + empty) to verify deterministic parameter collection order.
- F01/F05: Extract shared deriveParamsFromAst into query-plan-meta.ts and local helpers in sql-lane builders to eliminate 7+ duplicated collect-and-map patterns. - F03: Remove empty TransformResultWithParams interface. - F06: Rename nextParamIndex to advanceParamCursor for clarity. - F07: Add deterministic ordering tests for SelectAst (clause traversal order) and InsertAst (multi-row order). - F04: Document isBoundWhereExpr fragility (deferred to BoundWhereExpr elimination follow-up).
…ng path encodeParam no longer validates via ParamDescriptor.refs (now always undefined). Remove jsonValidators param from encodeParams, use paramIndex for error labels instead of paramDescriptor.index, and delete unused createColumnParamDescriptor.
Conditionally spread optional ParamRef properties (name, codecId, nativeType) to satisfy exactOptionalPropertyTypes. Remove leftover extra arguments from buildWhereExpr and buildIncludeJoinArtifact call sites. Apply biome formatting fixes.
Add code review (round 2) and walkthrough documents. Update plan with Milestone 3 tasks for remaining review findings: eliminate BoundWhereExpr (F09), remove nativeType (F10), make codecId required and fix OperationExpr sentinel (F11), remove annotations.codecs encoding lookup (F12). F08 (types.ts split) deferred to TML-2173.
The resolveParamCodec function incorrectly used projection-alias-to-codec mappings (annotations.codecs) for parameter encoding. This lookup matched param names against projection aliases — a different namespace — and took priority over the correct paramDescriptor.codecId. With codecId now reliably set on every ParamDescriptor, this coincidence-based lookup is redundant. Also removes unused plan parameter from encodeParam.
nativeType was dead weight on ParamRef — not consumed by any runtime encoding path or adapter. Remove it from the class, all producer call sites (predicate-builder, mutation-builder, where-binding, transform- expr, query-plan-mutations), all deriveParamsFromAst helpers, and all test assertions.
Every ParamRef corresponds to a column binding with a known codec.
Making codecId required:
- Provides compile-time safety for missing codec metadata
- Removes conditional spreading patterns from deriveParamsFromAst
- Fixes the OperationExpr sentinel (ParamRef.of(undefined, {name}))
which was broken — value was undefined and no codecId. Operations
now accept raw values and derive codecId from columnMeta.
- Updates ArgToType for param-kind operation args from ParamPlaceholder
to unknown (raw values accepted directly).
BoundWhereExpr was { readonly expr: WhereExpr } — a wrapper that added
no value after params/paramDescriptors were removed. All code now uses
WhereExpr directly: bindWhereExpr returns WhereExpr, CollectionState
stores WhereExpr[], and combineWhereExprs replaces both
combineWhereFilters and combinePlainWhereExprs.
…imination Test assertions in query-plan-select and query-plan-aggregate still accessed .expr on values that are now WhereExpr directly.
BoundWhereExpr was reduced to { expr: AnyWhereExpr } — a trivial
wrapper that adds no value. ToWhereExpr.toWhereExpr() now returns
AnyWhereExpr directly. All consumers and test helpers updated to
remove the .expr unwrapping.
aqrln
reviewed
Mar 27, 2026
aqrln
reviewed
Mar 27, 2026
| * Maps operation argument specs to TypeScript argument types. | ||
| * - typeId args: ColumnBuilder (accepts base columns or operation results) | ||
| * - param args: ParamPlaceholder | ||
| * - param args: unknown (raw value; converted to ParamRef at operation build time) |
Member
There was a problem hiding this comment.
is ParamPlaceholder ever used anywhere in practice now?
aqrln
reviewed
Mar 27, 2026
aqrln
approved these changes
Mar 27, 2026
Member
aqrln
left a comment
aqrln
left a comment
There was a problem hiding this comment.
LGTM with one question. This is much nicer to work with!
aqrln
deleted the
tml-2103-restructure-paramref-to-use-structural-linkage-instead-of-implicit-1
branch
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes TML-2103
Key snippet — Before / After
Before
After
Intent
Make each SQL AST parameter placeholder carry its bound value and codec metadata directly, so there is no implicit numeric index linking a
ParamRefto a parallelparams[]array. This eliminates a category of silent off-by-one bugs, removes ~150 lines of fragile offset arithmetic, and decouples the target-agnostic AST from PostgreSQL's 1-based$Nconvention.Change map
ParamRefclassQueryAst.collectParamRefs()abstractThe story
Make
ParamRefvalue-carrying: Replaceindex: numberwithvalue: unknownand addcodecIdmetadata. RemovewithIndex(). This is the foundational data model change — every other change cascades from it.Add
collectParamRefs()to all AST types: Make it abstract onQueryAst(forcing implementation onSelectAst,InsertAst,UpdateAst,DeleteAst).SelectAstalready had this method; the DML types gain it. This establishes a deterministic param-collection contract that the adapter depends on.Migrate all producers to embed values directly: SQL lane builders, ORM where-binding, Kysely transforms — all stop maintaining parallel
values[]/paramDescriptors[]accumulators and instead createParamRef.of(value, { name, codecId }). TheBindStatemutable pattern inwhere-binding.tsis eliminated.Derive plan params from the AST: Instead of accumulating params during construction, call
ast.collectParamRefs()after the AST is fully built to deriveparamsandparamDescriptorsarrays. This centralizes the source of truth.Delete the offset machinery:
offsetWhereExprParams,offsetBoundWhereExpr,combineWhereFiltersoffset logic,assertBoundPayloadvalidation, the Kysely where-expr index remapping pass — all existed solely to maintain the implicit index invariant. With values on the nodes, these are unnecessary.Assign target-specific indices at adapter lowering time: The Postgres adapter calls
ast.collectParamRefs()once to build an identity-basedMap<ParamRef, number>, then passes this map through all render functions.renderParamReflooks up the assigned index and usesref.codecIdfor typed casts.Eliminate
BoundWhereExpr: The wrapper was{ expr, params, paramDescriptors }pre-refactoring, reduced to{ expr }in step 5, and now eliminated entirely.ToWhereExpr.toWhereExpr()returnsWhereExprdirectly. AllcreateBoundWhereExpr,isBoundWhereExpr,ensureBoundWhereExprhelpers are deleted.Make
codecIdrequired onParamRef: WithnativeTyperemoved and theOperationExprsentinel fixed (see step 9), everyParamRefhas a legitimatecodecId. Making it required provides compile-time safety and removes conditional spreading patterns from everyderiveParamsFromAstsite.Fix
OperationExprparam handling: ThecreateOperationExprBuildersentinel (ParamRef.of(undefined, { name })) was already broken —value: undefined, nocodecId, yet collected bycollectParamRefs(). Changed to accept raw values and derivecodecIdfromcolumnMeta.codecId.Remove
annotations.codecslookup from encoding: TheresolveParamCodecfunction incorrectly used projection-alias-to-codec mappings (annotations.codecs) for parameter encoding. WithcodecIdnow reliably set on everyParamDescriptor, this coincidence-based lookup is redundant and removed.Behavior changes & evidence
ParamRefdata model: Before —ParamRef(index: number, name?: string)withwithIndex(). After —ParamRef(value: unknown, { name?: string, codecId: string })with no index orwithIndex().codecIdis required;nativeTypeis removed (dead weight — never consumed by any runtime path).Adapter-time index assignment: Before —
ParamRef.indexwas pre-assigned; adapter rendered$${ref.index}directly. After — adapter builds aParamIndexMapviacollectParamRefs()and assigns indices during rendering.BoundWhereExpreliminated: Before —{ expr, params, paramDescriptors }. Intermediate —{ expr }. After — eliminated entirely; consumers acceptWhereExprdirectly.ToWhereExpr.toWhereExpr()returnsWhereExpr. AllcreateBoundWhereExpr/isBoundWhereExpr/ensureBoundWhereExprhelpers deleted.BoundWhereExprdeleted; packages/3-extensions/sql-orm-client/src/where-utils.ts — utilities deleted; packages/3-extensions/sql-orm-client/src/where-interop.ts — returnsWhereExprOffset arithmetic deleted: ~150 lines of code removed. No behavior change — these functions existed only to maintain the implicit index invariant.
remapParamIndexes,collectParamIndexes,findDescriptorByIndexdeleted (~80 lines)Bare
WhereExprwithParamRefnow accepted: Before —normalizeWhereArgthrew on bareWhereExprcontainingParamRef. After — accepted, sinceParamRefcarries its value.WhereExprhad no way to carry values. With value-carryingParamRef, this restriction is unnecessary.Codec resolution moved to AST-construction time: Before — the adapter looked up
codecIdfrom the contract during rendering. After —codecIdis set onParamRefat construction, and the adapter usesref.codecIddirectly.Encoding path simplified:
resolveParamCodecno longer matches param names against projection aliases (annotations.codecs). Codec resolution uses onlyparamDescriptor.codecId. Theplanparameter is removed fromresolveParamCodecandencodeParam.annotations.codecslookup was coincidence-based (matching param names against a different namespace) and took priority over the correctcodecId. WithcodecIdreliably set, it was redundant and potentially harmful.Compatibility / migration / risk
LoweredStatementshape ({ sql, params }) is unchanged.ExecutionPlan/ plan metadata structure is unchanged. External consumers of plans see no difference.ParamRef.of()signature changed (index to value). All internal call sites migrated in this branch.BoundWhereExpreliminated: Any code implementingToWhereExprthat returned{ expr, params, paramDescriptors }must return bareWhereExprnow. The interface no longer exists.ParamDescriptormetadata loss:refs,nullable,indexfields are no longer populated in param descriptors derived from the AST. No downstream consumers existed.Follow-ups / open questions
types.tssplit (F08): Deferred to TML-2173. The 1,600-linetypes.tsshould be split into focused modules, but this is orthogonal to the structural paramref change.Non-goals / intentionally out of scope
instanceofdispatch in the AST (TML-2096, independent)ExecutionPlan/LoweredStatementpublic shapeSummary by CodeRabbit
Refactor
Tests
Chores
Documentation