refactor: resolve parameters once and serialize them through a write contract

[arthurschreiber]

Problem

A parameter's handling is spread over five DataType methods and two call sites that each combine them differently. validate runs in Request.validateParameters; resolveLength / resolvePrecision / resolveScale run inside the RPC payload while the request is being written; generateTypeInfo, generateParameterLength and generateParameterData produce a list of small buffers per parameter. Bulk load repeats the resolution logic with slightly different rules in addColumn. There is no single place that says what a parameter's declaration is, and no way for a type to write its bytes directly into the shared buffer that #1773 introduced.

This is the second of the series described in #1773. The third (streaming table-valued parameters) needs parameters resolved before the request starts and types that write into a buffer; this PR provides both.

Change

DataType gains three optional methods, and data-type.ts three helpers that adapt types which do not implement them:

• resolve(parameter, collation, options) → ParameterData: validate the value and determine length, precision, scale and collation. resolveParameter falls back to validate and the resolve* methods; an explicitly specified fact wins, including an explicit 0.
• writeTypeInfo(buffer, data, options): write the TYPE_INFO. writeTypeInfo falls back to generateTypeInfo.
• writeValue(buffer, data, options): write the length prefix and data. writeValue falls back to generateParameterLength and generateParameterData.

Int, NVarChar and VarBinary implement the write methods natively; every other type goes through the adapters unchanged, so migration can continue one type at a time.

Resolution happens once, up front:

• Request.validateParameters(collation, options) resolves every parameter and keeps the result in request.resolvedParameters. It still writes the validated value back to parameter.value, which makeParamsParameter relies on.
• RpcRequestPayload takes ResolvedParameter[] (name, output flag, type, resolved data) and only serializes. It writes each parameter's header, TYPE_INFO and value into one WritableTrackingBuffer and yields its chunks, so a large value written by reference stays by reference.
• Connection.execSql, callProcedure, prepare, unprepare, execute and the Always Encrypted sp_describe_parameter_encryption request build their payloads from resolved parameters. execute resolves each parameter with the value supplied for that execution, as it validated before. The wrapper parameters these methods add (statement, params, handle, stmt, tsql) now pass through validate like every other parameter; their values are always well-formed strings and integers, so nothing observable changes.
• Bulk load writes COLMETADATA through writeTypeInfo and each row's cells through writeValue into the payload's buffer (perf: serialize bulk load rows with an async generator, coalesced into packet-sized chunks #1779). Its error handling is unchanged.

validate is still called as validate(value, collation), without the connection options, as every caller did before. The useUTC-dependent range checks in the date and time types' validators have therefore never been active; enabling them is a behaviour change to make deliberately, not as a side effect here. A unit test pins the call shape.

Behaviour changes

All fall out of sharing one resolution path; all are covered by tests.

• Lengths resolve for every type that can resolve one, in both the RPC payload and BulkLoad.addColumn, not only for type ids matching (id & 0x30) === 0x20. refactor: resolve parameter lengths for all types that can resolve one #1771 made the same change on master in the meantime; this branch carries it through resolveParameter, so the two agree.
• An explicit 0 for length, precision or scale is kept rather than treated as unspecified. Every existing resolve* implementation already re-checks for an explicit value, so no bytes change for existing types; this removes the trap for a future type whose resolver does not.
• TYPE_INFO errors are attributed. An error thrown while writing a parameter's TYPE_INFO now surfaces as the same InputError naming the parameter that an error from its value did. This is the RPC half of fix: wrap bulk load serialization errors in InputError #1772; the bulk load half is not included.

Validation

• test/unit/rpcrequest-payload-test.ts serializes 40 parameter cases across every input type (int, string, binary, decimal, date/time, GUID, TVP, output and unnamed parameters, null values, max values over 8000 bytes) on TDS 7.4 and 7.2, with and without a collation, through the new payload and through an inline copy of the previous serialization algorithm, and asserts the bytes are identical. It also checks that a 1 MB value is passed through by reference and that a failing type surfaces as InputError.
• test/unit/parameter-contract-test.ts covers resolveParameter (explicit facts win, explicit zero kept, native resolve delegation, modern-id lengths, validation errors, the validate call shape) and byte equivalence of the three native types against their legacy methods across 19 value/length combinations. test/unit/bulk-load-test.ts covers modern-id length resolution in addColumn, and its byte-equivalence test now runs the bulk load payload's cells through writeValue.
• Unit suite: 525 tests. Full integration suite against SQL Server 2022 passes except the pre-existing environment-only should not leave any dangling sockets after connection timeout.
• Lint and typecheck clean.
• Also in this PR, two bulk load integration tests made safe for parallel runs against one database, as the Azure CI jobs are: the checkConstraints test no longer names its CHECK constraint, and the fireTriggers test names its trigger after its randomized table instead of the fixed bulkLoadTest. Constraint and trigger names are scoped to the schema, so concurrent jobs collided on both.

Measurements

Measured on this head against current master, same machine, same run. benchmarks/parameters/scalar-params.js is new (20 scalar parameters per request, resolved once per request as validateParameters does).

RPC serialization only, median of 5 runs. On master each parameter is validated and then serialized by the old payload; here each is resolved and then serialized by the new one.

request	master	this PR
20 scalar parameters	~50k req/s	~55k req/s
one 1 MB varbinary(max)	~50k req/s	~52k req/s
one 10 MB varbinary(max)	~57k req/s	~52k req/s

Scalar requests gain about 8%; the large-value cases pass the buffer through by reference on both sides and the difference is inside run-to-run noise (min/max spread of 2x on both).

Bulk load, where this PR routes every cell through writeValue and the three native types skip the per-cell generateParameterLength/generateParameterData allocations. Serializer in isolation, 200,000 rows from an array, median of 5 runs:

rows	master	this PR
int, nvarchar(50), float	~1.08M rows/s	~1.71M rows/s
40 × int	~174k rows/s	~759k rows/s

End to end against SQL Server 2022, 50 bulk loads of 10,000 rows each:

source	master	this PR
one int column, array	~210k rows/s	~212k rows/s
three columns, array	~167k rows/s	~186k rows/s
three columns, async generator	~157k rows/s	~171k rows/s

The single-int case is server-bound on both sides. The wide-row serializer gain (4.4x) is the compounding #1779 anticipated: that PR removed the per-row stream plumbing, this one removes the per-cell allocations.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Fxv5h4UMCGJEpjgCKcAxug

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-03T12:55:59.763444Z	9c5d24c	New commits

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[claude]

Review

Solid refactor — the single resolve → writeTypeInfo/writeValue contract is a clean way to let types migrate incrementally, and the byte-identical reference tests (re-implementing the pre-refactor serialization inline and diffing against it across 40 parameter combinations, two TDS versions, with/without collation) are a genuinely strong way to de-risk a change like this. The buffer-by-reference behavior for large values is preserved and covered by an explicit test. I confirmed by hand that every type currently implementing resolveLength (char, binary, ntext, image, uniqueidentifier, text, nchar, nvarchar, varchar, varbinary) has a legacy-pattern id, so the "lengths resolve for every type now" change is indeed byte-identical for existing types as claimed.

One thing worth fixing before merge: bulk load isn't actually part of the #1771 fix

The PR says "#1771 can be closed once this lands," but #1771 explicitly scopes the fix to both src/rpcrequest-payload.ts and src/bulk-load.ts ("it broke RPC parameters and bulk load columns... Resolve lengths... in both src/rpcrequest-payload.ts and src/bulk-load.ts").

This PR only changes the RPC path. BulkLoad.addColumn (src/bulk-load.ts:434) still gates length resolution behind the old legacy-id check, unchanged by this diff:

if ((type.id & 0x30) === 0x20) {
  if (column.length == null && type.resolveLength) {
    column.length = type.resolveLength(column);
  }
}

So a bulk load column using a modern-id type with a resolvable length (e.g. the upcoming vector/json types #1771 was written against) will still get column.length == null, and writeTypeInfo/generateTypeInfo will still emit the same bad zero-length TYPE_INFO the issue describes. Since writeTypeInfo's fallback path doesn't do any length resolution itself, this bug survives for bulk load specifically.

Recommend either applying the same unconditional-resolveLength treatment to addColumn, or adjusting the PR description so it doesn't claim to close #1771 (which would presumably get re-opened against bulk load specifically).

Minor notes, non-blocking

• resolveParameter's if (parameter.length) / if (parameter.precision) / if (parameter.scale) checks (src/data-type.ts:145-161, carried over unchanged from the old rpcrequest-payload.ts logic) treat an explicit 0 the same as "unset," falling through to the type's own resolve*. This is harmless today only because every current resolve* implementation re-checks with != null internally (e.g. DateTime2.resolveScale), so an explicit scale: 0 still round-trips correctly. It's a bit of a trap for a future type whose resolve* doesn't do that inner re-check, though — might be worth tightening to != null while this code is already being touched.
• get-parameter-encryption-metadata.ts's tsql/params wrapper parameters, and the statement/params/handle/stmt wrapper parameters built inline in connection.ts, now flow through resolveParameter, which calls type.validate() on them. Previously RpcRequestPayload never validated — it used parameter.value as given. Harmless today since these are always well-formed values, but it's a small, probably-unintentional widening of what gets validated; worth a quick sanity check that no caller relies on the old bypass.

Not verified

I wasn't able to run npm ci/tsc/the test suite in this sandbox (network/tooling restricted), so I've relied on static reading rather than confirming "495 tests" / "lint and typecheck clean" firsthand. Everything I traced by hand (call sites of validateParameters, resolvedParameters usage ordering, error-wrapping in RpcRequestPayload.generateParameterData, the PLP-terminator logic in the new writeValue implementations) checked out against the described behavior.

[arthurschreiber]

One Azure job on c6342d9 (Azure SQL Server / Node.js 22.x, one of the three authentication configurations) failed in BulkLoad checks constraints if the checkConstraints option is set to true with an AggregateError from the CREATE TABLE step, while another Azure configuration passed the same test on the same commit.

Cause, reproduced locally against SQL Server 2022 with two sessions: the test creates #tmpTestTable3 with a named constraint (CONSTRAINT chk_id CHECK ...). Temp table names are per session, but constraint names are unique per database, so when two sessions run this test at the same time the second gets 2714: There is already an object named 'chk_id' in the database followed by 1750: Could not create constraint or index, two errors, hence the AggregateError. The three Azure jobs run concurrently against one database, so this is a pre-existing race in the test, unrelated to this PR's changes.

Fixed in the test by leaving the constraint unnamed, which gets a unique generated name; pushed as the next commit.

---

Generated by Claude Code

[arthurschreiber]

Addressed the review:

• Bulk load and refactor: resolve parameter lengths for all types that can resolve one #1771: correct, addColumn still had the legacy id gate. It now resolves lengths by capability like the RPC path, with a unit test for a modern-id type, so the PR covers both places refactor: resolve parameter lengths for all types that can resolve one #1771 changes. Description updated.
• Explicit 0 facts: resolveParameter now uses != null, so an explicit length, precision or scale of 0 is kept. Every existing resolver re-checked for an explicit value itself, so no bytes change for existing types (the equivalence tests still pass); added a test for scale: 0 on DateTime2.
• Wrapper parameters now validated: checked each one. statement / params / stmt / tsql are strings through NVarChar.validate, which returns them unchanged; handle goes through Int.validate, which returns the integer, or null for an unset handle, which serializes to the same NULL int the old path produced from undefined. Noted in the description.

---

Generated by Claude Code

[claude]

Nicely scoped refactor — the resolve / writeTypeInfo / writeValue split is a clean way to migrate types incrementally, and the byte-equivalence testing (rpcrequest-payload-test.ts's 40-case comparison against an inlined copy of the old algorithm, parameter-contract-test.ts's native-vs-legacy comparisons) is a genuinely strong way to guard a refactor like this. I also verified by hand that every current type implementing resolveLength has a legacy-pattern id ((id & 0x30) === 0x20), which confirms the claim that behavior change #1 is byte-identical for all existing types.

One behavior change that isn't called out in the "Behaviour changes" section, and doesn't look intentional:

validate() now receives real connection options, where it previously always got undefined.

Before this PR, every call site that invoked type.validate(...) (Request.validateParameters, Connection.execute) passed only (value, collation) — two arguments. resolveParameter in src/data-type.ts now calls type.validate(parameter.value, collation, options), passing the real InternalConnectionOptions.

Five types key their out-of-range validation on options.useUTC: Date, DateTime, DateTime2, DateTimeOffset, and SmallDateTime all do:

if (options && options.useUTC) {
  year = value.getUTCFullYear(); ...
} else {
  year = value.getFullYear(); ...
}

Since options was always undefined at the old call sites, this branch was always falsy — validation always range-checked using local time getters, regardless of the useUTC connection option. useUTC defaults to true, and the actual serialization (generateParameterData) already honors it. So this PR quietly makes validation consistent with serialization for the default configuration — which is arguably a bug fix — but it's a real behavior change: a Date whose local-time year/month/day was previously accepted (or rejected) at a range boundary (e.g. SmallDateTime's 1900–2079 window, DateTime's 1753 floor, or the 2079-06-06 cutoff) can now be rejected (or accepted) differently once its UTC fields are used instead, for any caller in a non-UTC timezone with a date near one of those boundaries.

This isn't mentioned in the PR description's two enumerated behaviour changes, and none of the new tests exercise it — the date values used (e.g. Date.UTC(2024, 1, 29, ...)) are far from any boundary, and useUTC: true is hardcoded in the test options, so both call paths would agree regardless. Worth either:

• confirming this is intentional and adding it as a third documented behaviour change with a boundary-case test (e.g. a SmallDateTime value that's in range in UTC but out of range locally, or vice versa), or
• passing undefined for options in resolveParameter's call to validate if the goal is strictly no-behavior-change for this PR, deferring the fix to a follow-up.

Everything else looked solid:

• RpcRequestPayload, Request, Connection, bulk load, and the Always Encrypted metadata request are all threaded through consistently — I didn't find a call site still building an unresolved Parameter[] where a ResolvedParameter[] was expected.
• The TYPE_INFO error attribution fix in generateParameterData (wrapping both writeTypeInfo and writeValue in the same try/catch) looks correct and is tested.
• Large-value pass-by-reference is preserved through WritableTrackingBuffer (chunks ≥ 8 KB are referenced, not copied), matching the benchmark numbers in the description.
• Minor nit, non-blocking: Connection.resolveParameter (the new private method) has the same name as the module-level resolveParameter it wraps (src/connection.ts). It's unambiguous since one is always called as this.resolveParameter(...), but a quick skim can misread resolveParameter(...) inside that method as recursion rather than the imported free function.

🤖 Generated with Claude Code

[claude]

Review

I read through the full diff (data-type.ts, request.ts, connection.ts, rpcrequest-payload.ts, bulk-load.ts, the three migrated data types, and the new/updated tests) and checked the new native writeTypeInfo/writeValue implementations byte-for-byte against the generateTypeInfo/generateParameterLength/generateParameterData they replace. Overall this is a clean, well-scoped refactor with strong test coverage (the legacy-vs-contract byte-equivalence tests in particular give good confidence).

Code quality / design

• The resolve / writeTypeInfo / writeValue split with adapter fallbacks (resolveParameter, writeTypeInfo, writeValue in data-type.ts) is a nice incremental-migration pattern — types that haven't been ported keep working unmodified via generate*.
• Moving from truthy checks (if (parameter.length), if (parameter.precision)) to != null checks in the resolution path is a real correctness fix (explicit 0 now survives), and it's called out explicitly in the PR description with test coverage (parameter-contract-test.ts, "keeps an explicitly specified zero").
• Dropping the (type.id & 0x30) === 0x20 gate before resolving length (both in resolveParameter and BulkLoad.addColumn) is verified byte-identical for all current types since only legacy-id types implement resolveLength — good forward-compatibility fix for TDS 7.2+ ids, with a dedicated regression test in bulk-load-test.ts.
• RowTransform._transform in bulk-load.ts now accumulates a row's bytes into one WritableTrackingBuffer and only pushes at the end of the loop, instead of pushing header/value chunks as they're produced. As a side effect, if a later column fails to serialize, no partial-row bytes get pushed downstream before callback(error) — that's actually a nice improvement over the previous behavior (which could emit a truncated row before erroring).

Minor observations (non-blocking)

• Connection.resolveParameter (the new private method in connection.ts) shares its name with the imported top-level resolveParameter function from data-type.ts. It resolves correctly (a class method name isn't a lexical binding inside its own body, so the bare call inside the method still hits the module-level function, not infinite recursion), but it reads ambiguously at a glance — worth a distinct name (e.g. resolveRequestParameter) or a one-line comment.
• Several of the synthetic wrapper parameters (handle in execute/unprepare, stmt/params in prepare, statement/params in execSql) now go through this.resolveParameter(...), which calls validate(), outside of any try/catch (unlike the per-parameter loops, which are wrapped). The PR description explains why this is safe today (these values are always well-formed strings/integers), and that holds up — just flagging it as an invariant that isn't enforced by the type system, so a future change to one of these validate() implementations could turn into an uncaught exception instead of surfacing through the request's error callback.
• test/integration/bulk-load-test.ts's unnamed-constraint change is an unrelated CI-flakiness fix bundled into this PR. It's harmless and has a clear comment explaining the collision issue, but it's technically out of scope for this refactor — might be worth splitting into its own PR next time for a cleaner history.

Bugs
I didn't find any correctness issues. The native Int/NVarChar/VarBinary writeTypeInfo/writeValue implementations match their legacy generate* counterparts exactly, including the trickier PLP/MAX-length branches and the collation-buffer padding in NVarChar, and WritableTrackingBuffer.writeBuffer's reference-vs-copy threshold (CHUNK_SIZE) confirms the "large values pass through by reference" claim holds for both natively-migrated and adapter-fallback types.

Test coverage
Coverage for this change is thorough: rpcrequest-payload-test.ts cross-checks the new payload against an inlined copy of the previous algorithm across TDS 7.2/7.4, with/without collation, named/id procedures, and ~40 parameter shapes (including TVPs, output params, max values, and a 1MB by-reference check); parameter-contract-test.ts covers resolveParameter's precedence rules and native-vs-adapted byte equivalence; bulk-load-test.ts covers the modern-id length resolution fix. This is a solid amount of test coverage for a refactor of this size.

Security / performance
No security concerns — this is internal wire-serialization plumbing with no new external input handling. Performance-wise, batching a row/parameter's bytes into one WritableTrackingBuffer and yielding its chunks (instead of yielding many small buffers) should reduce the number of stream chunks produced, and the reference-vs-copy threshold means large values still avoid an extra copy — net neutral-to-positive, consistent with the new benchmarks added under benchmarks/parameters/.

[arthurschreiber]

Good catch on validate receiving the connection options: confirmed that no caller on master passes them (Request.validateParameters, Connection.execute and bulk load all call validate(value, collation)), so the useUTC branch in the five date/time validators has never run. Enabling it would change which boundary dates are accepted for callers in non-UTC timezones, which is a change to make on its own, not as a side effect of this refactor. resolveParameter now calls validate(value, collation) like before, with a unit test pinning the call shape, and the description says so.

Also renamed Connection.resolveParameter to resolveRequestParameter so it does not read as recursion into the free function.

---

Generated by Claude Code

[claude]

Review

I read through the diff in full (data-type.ts, the Int/NVarChar/VarBinary write methods, request.ts, connection.ts, bulk-load.ts, rpcrequest-payload.ts, and the new tests) and traced the two documented behaviour changes against every existing resolve* implementation. Solid piece of work — a few notes below, nothing blocking.

Code quality / design

• The resolve/writeTypeInfo/writeValue split with adapter fallbacks (resolveParameter, writeTypeInfo, writeValue in data-type.ts) is a clean way to let types migrate incrementally without a flag day. The doc comment above the DataType interface explaining the split is genuinely useful context, not filler.
• Connection.resolveRequestParameter is a nice, minimal wrapper — good call renaming it from the free function resolveParameter to avoid the recursion-looking name clash mentioned in the commit message.
• RpcRequestPayload dropping the separate collation constructor argument (now baked into each ResolvedParameter.data.collation ahead of time) simplifies its contract nicely and removes a footgun (collation now can't drift from what was used to resolve length/precision).

Correctness

• I checked every existing resolve* implementation (char, binary, image, nchar, ntext, text, uniqueidentifier, varchar, nvarchar, varbinary, decimal, numeric, time, datetime2, datetimeoffset) against the new "explicit 0 wins" logic in resolveParameter. They all already re-check parameter.{length,precision,scale} != null internally, so the claim in the PR description that this is byte-identical for existing types holds up — I couldn't find a counterexample.
• The removal of the (type.id & 0x30) === 0x20 gate in both resolveParameter and BulkLoad.addColumn looks correct and is covered by tests in both places (parameter-contract-test.ts and the new bulk-load-test.ts case using a synthetic 0xF5 id).
• The writeTypeInfo/writeValue wrapping change in RpcRequestPayload.generateParameterData — now covering TYPE_INFO errors too, not just value errors — is intentional and documented, and matches the new InputError test.
• Connection.execute() rebuilding parameters via { ...parameter, value: ... } before calling resolveRequestParameter correctly reproduces the old two-step (validate-then-resolve-downstream) behavior in one step; I verified the wrapper-parameter call sites (prepare, unprepare, execSql's statement/params) now going through validate is safe given Int.validate/NVarChar.validate both treat == null as "return null" rather than throwing, so the previously-unvalidated undefined handle values still degrade to SQL NULL as before.
• WritableTrackingBuffer.writeBuffer's copy-vs-reference threshold (CHUNK_SIZE) is respected correctly by the new native writeValue implementations for large buffers, and the "by reference" test assertions back that up.

Minor observations (not blocking)

• NVarChar.writeValue / VarBinary.writeValue carry over the same dead typeof value === 'string' branch that exists in their legacy generateParameterData counterparts (since validate guarantees a Buffer for VarBinary and a string for NVarChar, the other branch can never execute for those two types). Not a new issue — it's copied faithfully from the existing code for byte-parity — but now that these are freshly written functions it might be worth a follow-up to drop the unreachable branch rather than propagate it forward.
• get-parameter-encryption-metadata.ts now resolves metadataRequest.parameters inline via .map(resolveParameter(...)) rather than through Request.validateParameters/resolvedParameters like the other RPC call sites. That's fine functionally (nothing reads metadataRequest.resolvedParameters or relies on parameter.value being written back for this throwaway request), but it does mean there are now two slightly different idioms for "resolve this request's parameters" in the codebase. Worth a one-line comment if that asymmetry is intentional (e.g. because this request's parameters are wrapper-only and never need the mutation-back behavior).

Test coverage

• rpcrequest-payload-test.ts is thorough — 40 parameter cases × 2 TDS versions × collation on/off × named/id procedure, checked byte-for-byte against an inline copy of the pre-refactor algorithm, plus explicit large-value-by-reference and InputError-attribution tests.
• parameter-contract-test.ts covers the resolution contract itself (explicit-fact precedence, explicit-zero, native resolve delegation, modern-id length resolution, the validate(value, collation) call-shape pin, and legacy-adapter equivalence for a non-migrated type).
• The bulk-load integration test fix (unnamed CHECK constraint) is a reasonable, narrowly-scoped fix for the flaky concurrent-CI-job collision, unrelated to the main refactor but good to have bundled given it was found along the way.

Performance / security

• Benchmarks in the PR description show throughput within noise of master, consistent with this being a pure restructuring; nothing here looks like it would regress serialization performance, and the buffer-consolidation actually reduces the number of small stream chunks per parameter.
• No security concerns — this is internal wire-protocol serialization refactoring with no new external input surface.

Overall this is a well-scoped, well-tested refactor that keeps byte-for-byte compatibility while opening the door for streaming TVP support. Nice work.

[arthurschreiber]

On the two minor notes from the latest review:

• The Buffer branch in NVarChar.writeValue and the string branch in VarBinary.writeValue are reachable, not dead: bulk load only validates values on the first row (RowTransform._transform checks firstRowWritten), so later rows reach writeValue unvalidated, and the legacy generate* methods handled both shapes for that reason. They stay.
• The Always Encrypted metadata request resolves its parameters inline because it is a throwaway wrapper request whose two parameters never need the write-back to parameter.value that validateParameters does for user requests. I'll add a one-line comment saying so with the next push rather than spend a CI cycle on it alone.

---

Generated by Claude Code

[claude]

Review

This is a large, carefully-scoped refactor, and it reads that way: the PR description states the exact behavior changes up front, and the new tests (parameter-contract-test.ts, rpcrequest-payload-test.ts) diff the new code against an inline copy of the old serialization logic byte-for-byte across a good spread of types/lengths/collation combinations. I traced through the core pieces by hand and didn't find a correctness bug. Notes below are mostly confirmations and a couple of minor points worth a second look.

Correctness checks I did

• resolveParameter's "explicit fact wins, including 0" fix (src/data-type.ts): confirmed the existing resolve* implementations (Decimal, Numeric, ...) already re-check != null internally, so this only changes behavior for a future type that doesn't — matches the PR description, no regression risk for existing types.
• Int/NVarChar/VarBinary native writeTypeInfo/writeValue vs. the legacy generateTypeInfo/generateParameterLength/generateParameterData they're replacing: walked through the PLP (varchar(max)-style) branches, the null-length branches, and the string-vs-Buffer branches side by side — they match. The value.length * 2 shortcut for UCS-2 byte length in the new code is equivalent to the old Buffer.byteLength(value, 'ucs2') since Node's ucs2/utf16le encoding is 2 bytes per UTF-16 code unit, same unit JS string.length counts.
• TYPE_INFO error attribution (src/rpcrequest-payload.ts): the old code's try/catch around InputError only wrapped generateParameterData, not generateTypeInfo/generateParameterLength — so a throwing resolveLength/generateTypeInfo previously escaped unwrapped. The new code wraps both writeTypeInfo and writeValue, which is a real (and correctly documented) fix, not an accidental behavior change.
• Large-value by-reference passthrough: WritableTrackingBuffer.writeBuffer only copies buffers under CHUNK_SIZE (8KB); RpcRequestPayload.generateParameterData and bulk load's RowTransform._transform both build one buffer per parameter/row and call getBuffers() once, so a varbinary(max) value written via writeBuffer stays a reference all the way to the yielded chunk. Matches the "1 MB buffer identity-included in output" tests.
• Modern type-id length resolution ((id & 0x30) === 0x20 removed from both bulk-load.ts and the old rpcrequest-payload.ts): confirmed this is now unconditional in both places, fixing refactor: resolve parameter lengths for all types that can resolve one #1771 as described, and is a no-op for all legacy ids since every type with resolveLength already has a legacy id.

Minor observations (not blockers)

1. bulk-load.ts RowTransform._transform: previously each column's bytes were this.push()'d as soon as generated; now the whole row is accumulated into one WritableTrackingBuffer and flushed via getBuffers() at the end of the row. Since _transform is synchronous either way (no await between pushes), I don't think this changes observable behavior or backpressure handling — just flagging it as a shape change in case it wasn't intentional for rows with multiple large blob columns.
2. Request.validateParameters now only assigns this.resolvedParameters after the whole loop succeeds (previously parameter.value was mutated in place per-parameter as it went). That preserves the existing partial-mutation-on-failure behavior for parameter.value, but it's worth double-checking there's no code path that reads request.resolvedParameters after a failed validateParameters call expecting a partial/stale-but-valid array — from what I saw, callers only read it after a successful call, so this looks fine.
3. Nice touch: execute()'s per-execution parameter resolution ({...parameter, value: parameters ? parameters[parameter.name] : null} piped through resolveRequestParameter) preserves the old explicit type.validate(...) call there instead of accidentally routing it through the same path as request.parameters, which matters since execute() intentionally validates a different value per call.

Test coverage

Good — byte-equivalence tests for the three natively-migrated types across null/empty/short/max/explicit-length/collation cases, a dedicated test for the modern-type-id length regression, an InputError-attribution test, and a reference-passthrough test for large buffers. The unit test for BulkLoad#addColumn with a synthetic 0xF5 id is a good way to pin the #1771 fix without needing a real TDS 7.2+ type.

Didn't find security concerns — this is wire-serialization code operating on already-validated/typed values, no injection surface here.

I wasn't able to run the suite in this sandbox (no network access to install dependencies), so this is a static review; the PR description's own numbers (498 unit tests, full integration suite passing, lint/typecheck clean) cover that ground.

[arthurschreiber]

The SQL Server 2022 Windows / Node.js 24.x job on d64db5d failed, but not in the test suite: the Install SQL Server 2022 step died with setup.exe exit code 2226847745, so every integration test in that job was skipped — no test body ran. This is a runner-side installer flake, unrelated to the diff.

Corroboration on the same commit:

• SQL Server 2022 Windows / Node.js 26.x installed SQL Server fine and passed the full suite, including Encrypt Test > with strict encryption enabled (TDS 8.0) > opens an encrypted connection — the test that flaked on earlier commits did not recur here.
• All Linux and Azure jobs are green; SQL Server 2022 Windows / Node.js 22.x is still completing.
• The parent commit f1795e8 (identical code apart from a doc comment) went fully green, including every 2022 Windows job.

There's nothing to fix in the PR — re-running the single failed job clears it. This integration can't trigger a re-run (the API returns 403), so a maintainer re-run or the next push will resolve it. Keeping the PR watched until CI is green.

---

Generated by Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 92.17391% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.50%. Comparing base (03afa14) to head (d8557b8).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/data-types/nvarchar.ts	84.00%	1 Missing and 3 partials ⚠️
src/data-types/varbinary.ts	80.95%	1 Missing and 3 partials ⚠️
src/connection.ts	95.45%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1774      +/-   ##
==========================================
+ Coverage   81.27%   82.50%   +1.22%     
==========================================
  Files          92       92              
  Lines        4967     5035      +68     
  Branches      938      961      +23     
==========================================
+ Hits         4037     4154     +117     
+ Misses        649      592      -57     
- Partials      281      289       +8     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9c5d24ccc4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Review

This is a well-executed refactor. The resolve / writeTypeInfo / writeValue contract is a clean seam for migrating types incrementally, the adapters (resolveParameter, writeTypeInfo, writeValue in data-type.ts) correctly fall back to the legacy validate/resolve*/generate* methods, and the three intentional behaviour changes (dropping the (id & 0x30) === 0x20 gate, keeping explicit 0 facts, attributing TYPE_INFO errors to InputError) are each backed by dedicated tests and clearly called out in the PR description rather than slipped in silently. I traced the byte-level equivalence claims for Int, NVarChar, and VarBinary against their generate* counterparts and they check out.

Correctness

• resolveParameter's explicit-fact check (parameter.length != null etc.) is a real improvement over the old truthy checks (if (parameter.length)) used in the previous RpcRequestPayload.generateParameterData, and is verified by the "keeps an explicitly specified zero" test in parameter-contract-test.ts.
• Confirmed all existing types with resolveLength (Char, Binary, NText, Image, NVarChar, Text, UniqueIdentifier, VarBinary, VarChar, NChar) have ids that already satisfy (id & 0x30) === 0x20, so removing that gate is indeed byte-identical for shipped types, as claimed.
• Request.validateParameters / Connection.execSql / callProcedure ordering is preserved correctly — parameter.value is still written back before makeParamsParameter builds the params string, so declaration() still sees the validated value.
• The new writeTypeInfo/writeValue split correctly reproduces the previous error-handling gap: the old code only wrapped generateParameterData in try/catch, not generateTypeInfo/generateParameterLength. Now both are wrapped uniformly, which is the intended fix for fix: wrap bulk load serialization errors in InputError #1772 — nice catch.

Minor observations (non-blocking)

1. Connection.execute() — the synthetic handle parameter ({ type: TYPES.Int, name: '', value: request.handle, ... }) is now pushed via resolveRequestParameter(...) outside the surrounding try block (connection.ts ~2985-2996), whereas before it was pushed as a raw, unvalidated Parameter. It now runs through Int.validate() unguarded. In practice this is harmless (Int.validate treats null/undefined as valid and only throws on genuinely invalid values), and there's already a TODO: Abort if request.handle is not set acknowledging this parameter isn't fully guarded — just flagging that a thrown error here would now propagate synchronously instead of being silently skipped.
2. RowTransform._transform (bulk load) — previously each column's bytes were pushed to the readable stream as soon as they were generated; now the whole row is accumulated into one WritableTrackingBuffer and only pushed once the row is fully serialized. Functionally equivalent (and arguably safer — a mid-row error no longer leaves partially-written row bytes already pushed downstream), but it does mean peak per-row memory is now "whole row" rather than "single column" before a chunk is flushed. Given WritableTrackingBuffer.writeBuffer still references (not copies) buffers ≥ CHUNK_SIZE, this shouldn't matter for large values, only for rows with many small columns — likely a non-issue, just noting the shift.
3. get-parameter-encryption-metadata.ts — getParameterEncryptionMetadata doesn't appear to be called anywhere else in src/, nor exercised by any test in test/. The change here (piping its parameters through resolveParameter) is reasonable and consistent with the rest of the PR, but it's effectively verified only by the type checker, not by any runtime test, since the function seems to be dead/unwired code already (pre-existing, not introduced by this PR).

Test coverage

Excellent — rpcrequest-payload-test.ts diffing the new payload against an inlined copy of the previous algorithm across 40 parameter shapes × TDS versions × collation presence is a strong regression guard, and parameter-contract-test.ts isolates resolveParameter behavior nicely (explicit zero, native resolve delegation, the validate call-shape pin, modern-id lengths). The unrelated chk_id constraint-name fix for concurrent CI runs is a sensible, well-explained drive-by fix.

Security

No concerns — this is internal wire-format serialization logic with no new external input handling; validation behavior for user-supplied values is preserved or made stricter (never weaker).

Nothing here blocks merging; the minor points above are observations for awareness rather than requested changes.

[claude]

Review

This is a well-scoped, carefully executed refactor. The split between resolve (validate + determine declaration facts once) and writeTypeInfo/writeValue (pure serialization) is a clean contract, and the adapter functions (resolveParameter/writeTypeInfo/writeValue in data-type.ts) let existing types keep working unmodified while Int, NVarChar and VarBinary migrate natively. The test/unit/rpcrequest-payload-test.ts and parameter-contract-test.ts suites are a real strength here — cross-checking the new path against an inlined copy of the old algorithm across 40 parameter shapes (including collation, TDS 7.2/7.4, max values, output/unnamed params) is exactly the kind of test that makes a pure-refactor PR trustworthy.

Correctness

• resolveParameter's != null checks for length/precision/scale correctly preserve an explicit 0 (e.g. DateTime2 scale: 0), fixing the trap in the old if (parameter.length) truthiness checks in rpcrequest-payload.ts/bulk-load.ts. This is a no-op for existing types since every resolve* implementation (NVarChar.resolveLength, DateTime2.resolveScale, etc.) already re-checks != null internally — pinned by the test at parameter-contract-test.ts:932.
• Dropping the (type.id & 0x30) === 0x20 gate on length resolution (in both resolveParameter and BulkLoad.addColumn) is correctly scoped: every current type with resolveLength already has a matching legacy id, so this changes no bytes for existing types, and is covered by the "outside the legacy variable-length id bit pattern" tests in both bulk-load-test.ts and parameter-contract-test.ts.
• Byte-for-byte parity for Int/NVarChar/VarBinary's new writeTypeInfo/writeValue vs. the legacy generate* methods checks out on manual trace-through (null handling, PLP zero-length short-circuit, buffer vs. string branches, MAX-length threshold), backed by the parity tests.
• The per-parameter WritableTrackingBuffer plus yield * buffer.getBuffers() in RpcRequestPayload.generateParameterData is a nice side benefit: a failure partway through serializing one parameter's TYPE_INFO/value can no longer leak partial bytes for that parameter onto the wire (previously TYPE_INFO and the length prefix were yielded immediately, before the try/catch that only wrapped generateParameterData).

Minor / worth a second look

• Inconsistent error containment for the synthetic wrapper parameters. Request.validateParameters wraps resolveParameter in try/catch and turns failures into a clean RequestError (request.ts:513). But the internally-constructed statement/params/handle/stmt parameters built in Connection.execSql/prepare/unprepare (and the first handle push in execute, before its own try block) call this.resolveRequestParameter(...) outside any try/catch (connection.ts:2745, 2756, 2914, 2924, 2934, 2968, 2994). Previously these values were passed straight through without ever calling type.validate(), so this path could not throw; now that they go through resolveParameter → type.validate(), a thrown error (e.g. an unexpectedly non-numeric request.handle) would propagate synchronously out of execSql/prepare/unprepare/execute instead of surfacing through the request's error callback the way every other validation failure does. In practice these values are always well-formed as the PR description notes, so this is low risk today — but it's a latent inconsistency, and it becomes more real once a type implements resolve() with less forgiving checks than today's plain validate(). Might be worth wrapping resolveRequestParameter itself (or these call sites) the same way validateParameters is wrapped, for defense in depth.
• Same gap applies to always-encrypted/get-parameter-encryption-metadata.ts:102 — the .map(parameter => resolveParameter(...)) there isn't guarded either; worth checking whether its caller already treats a throw from this function as a normal request failure.

Test coverage

Coverage is thorough for the refactor's core claims (byte parity, explicit-zero handling, modern type-id length resolution, TYPE_INFO error attribution, by-reference passthrough for large values). I didn't find gaps beyond the error-containment point above, which isn't really testable as "bytes produced" — it'd need a test that deliberately makes a wrapper parameter's validate() throw and asserts the request fails cleanly rather than throwing synchronously.

Not verified

I reviewed the diff and affected source files in detail but did not execute the build/lint/test suite in this environment (sandbox restrictions on running npm/tsc). The PR description reports a clean lint/typecheck and full unit + integration suite (498 unit tests, integration pass against SQL Server 2022), which reads as plausible given how the tests are written, but is worth letting CI confirm rather than taking on faith.

Nice work overall — the contract is easy to reason about and the migration path for the remaining types (future work in #1773's series) looks straightforward to follow.

[claude]

Review

This is a well-designed refactor that consolidates parameter resolution and serialization into a single contract, and I traced through the mechanics in detail before commenting. Overall the change is solid; comments below are mostly confirmations of subtle points and a couple of minor nits rather than blockers.

Correctness

• Explicit-zero handling (resolveParameter's != null checks for length/precision/scale). I checked this against every current DataType implementation: for every type where parameter.length/precision/scale is actually consumed downstream (binary, varbinary, varchar, char, nvarchar, nchar, text, ntext, image, decimal, numeric, datetime2, datetimeoffset, time), the type also defines the matching resolve* method, and every one of those already re-checks != null internally. So the new "explicit 0 wins" behavior is provably a no-op for existing types, exactly as the PR description claims — nice bit of verification that would be easy to get wrong.
• Large-value pass-by-reference through RpcRequestPayload.generateParameterData. Header bytes, TYPE_INFO, and value are now written into one WritableTrackingBuffer and flushed via getBuffers(), instead of being yielded as separate pieces. I confirmed in writable-tracking-buffer.ts that writeBuffer only copies below CHUNK_SIZE (8 KiB) and references buffers at or above it directly even when interleaved with small writes to the same buffer object — so a large varbinary(max)/nvarchar(max) value still avoids a copy despite being written into a buffer that also holds small header bytes. This is exercised by the "passes large values through by reference" tests in both rpcrequest-payload-test.ts and parameter-contract-test.ts.
• TYPE_INFO error attribution. Wrapping both writeTypeInfo and writeValue in the same try/catch (vs. only the value-generation call previously) is a deliberate, well-tested improvement, not an accidental behavior change — confirmed by the new "reports serialization errors as InputError naming the parameter" test, which specifically deletes the native writeValue/writeTypeInfo on a stub type to exercise the fallback path too.
• Connection.execute. Moving from type.validate(...) directly to resolveRequestParameter({ ...parameter, value: ... }) looks behavior-preserving: length/precision/scale resolution for execute() parameters happened inside the RPC payload before (using the freshly-validated value), and now happens up front using the same value — same inputs, same outputs.

Minor nits (non-blocking)

• writeTypeInfo/writeValue in data-type.ts use slightly different fallback styles (if/else vs. if (...) { ...; return; } followed by fallback code). Purely cosmetic, but making them consistent would read a hair cleaner.
• NVarChar.writeValue/VarBinary.writeValue preserve the instanceof Buffer branch from the legacy generateParameterData, even though both types' validate() only ever returns a string or null for NVarChar (and buffer-or-null for VarBinary, so that branch is live there but effectively dead for NVarChar). Pre-existing behavior faithfully ported, not introduced by this PR — just flagging in case it's worth pruning in a future cleanup pass now that the code is being touched anyway.

Test coverage

Excellent — the 40-case byte-parity matrix in rpcrequest-payload-test.ts (across TDS 7.2/7.4, with/without collation, named/id procedures) against an inline copy of the previous algorithm is a strong way to guarantee zero wire-format drift, and parameter-contract-test.ts cleanly separates out the resolveParameter contract tests from the byte-equivalence tests. The validate call-shape pinning test is a good guard against accidentally activating the dormant useUTC date/time range checks as a side effect of this refactor.

Security

No concerns — this is an internal serialization refactor with no new untrusted-input parsing, no string-built SQL, and no changes to credential/encryption handling. The Always Encrypted metadata request path is updated consistently and its inline resolution is now explained with a comment.

Nice work — the byte-for-byte test discipline here made this refactor genuinely easy to verify.

[claude]

Review

I went through the diff against master (data-type.ts, request.ts, connection.ts, rpcrequest-payload.ts, bulk-load*.ts, get-parameter-encryption-metadata.ts, the migrated types, and both new/updated test suites). Overall this is a clean, well-scoped refactor with unusually strong verification (byte-for-byte comparison against an inline copy of the legacy algorithm). A few notes below, nothing blocking.

Strengths

• Verification approach is excellent. parameter-contract-test.ts and rpcrequest-payload-test.ts both keep an inline copy of the previous serialization logic and assert new-vs-old byte equality across a wide matrix of types/lengths/collation/TDS-version combinations, including the specific edge cases that matter (zero-length PLP values, values that straddle maximumLength, large by-reference buffers). That's a much stronger safety net than typical "does it produce roughly the right bytes" tests for a wire-protocol change.
• Hand-verified Int, NVarChar, and VarBinary's new writeTypeInfo/writeValue against their legacy generate* counterparts (including the zero-length PLP branch, where the legacy code omits the length prefix entirely and only emits the terminator) — they match exactly.
• The two behavior changes called out in the description (explicit 0 for length/precision/scale now wins over resolution, and TYPE_INFO write errors now get wrapped in InputError the same as value-write errors) are both real, intentional, and match what the diff does. Good that validate's call shape (no options passed) was deliberately preserved and pinned with a test rather than silently changed.
• WritableTrackingBuffer's reference-vs-copy behavior for large values is preserved through the new path and explicitly tested (passes large values through by reference).
• Splitting resolution (resolveParameter) from serialization (writeTypeInfo/writeValue) is a sensible seam for the TVP work this sets up, and letting types migrate one at a time via optional methods keeps the diff's blast radius small.

Things worth a look

1. Wrapper-parameter resolution now happens eagerly and outside any try/catch in a few places. In connection.ts, the resolveRequestParameter(...) calls that build the statement/params parameters in execSql, all three parameters in prepare, the handle parameter in unprepare, and the handle parameter push in execute (the one before the per-parameter loop) all sit outside a try/catch. Same for the .map(resolveParameter) in get-parameter-encryption-metadata.ts. Previously these values were never validated at push time (validation/resolution happened lazily inside the RPC payload's generator, wrapped in InputError); now resolveParameter calls type.validate synchronously at construction time. Since these values are internally generated well-formed strings/numbers this should never actually throw, as the PR description notes — but if it ever did (e.g. a future bug that hands a bad sqlTextOrProcedure), it would now surface as an uncaught synchronous exception out of execSql/prepare/unprepare/execute/getParameterEncryptionMetadata rather than a controlled RequestError/callback(error). Might be worth a defensive try/catch around these, or at least a short comment noting the invariant being relied on (some of this is already explained in the PR description, but not in the code itself for prepare/unprepare).

2. resolveParameter's "explicit fact wins, including explicit zero" rule isn't structurally enforced for types that implement resolve(). For the generic (adapter) path, resolveParameter itself checks parameter.length/precision/scale != null before falling back to the type's resolver. But if a type implements resolve() natively, that responsibility moves entirely into the type's own implementation — resolveParameter just returns whatever the type computes. None of the three migrated types (Int, NVarChar, VarBinary) implement resolve() yet, so this isn't exercised today, but it's a contract that future migrations (e.g. TVP, per the PR description) will need to remember to replicate themselves rather than getting it for free. Might be worth a note in the resolve? JSDoc in data-type.ts calling this out explicitly for implementers.

3. Minor test-coverage gap: NVarChar.writeValue has a Buffer-instance branch (mirroring VarBinary's), but parameter-contract-test.ts's NVarChar cases only ever pass strings, so that branch isn't included in the byte-equivalence matrix (unlike VarBinary, which does test Buffer values explicitly). Small, but since the whole point of that test file is byte-for-byte assurance across value shapes, it'd be nice to have parity.

Other

• No security concerns — this is a pure serialization-path refactor, no new external input handling.
• Performance numbers in the description look plausible given the change (avoiding per-parameter/per-cell small-buffer allocations in favor of writing into one shared tracking buffer), and are backed by both isolated serializer and end-to-end bulk load measurements.
• The two unrelated integration-test fixes (unnamed CHECK constraint, table-scoped trigger name) are reasonable, low-risk fixes for the stated CI flakiness cause (schema-scoped name collisions across parallel Azure jobs).

Nice work — the byte-equivalence testing strategy here is a good pattern for wire-protocol refactors in general.

[github-actions]

🎉 This PR is included in version 20.3.0 🎉

The release is available on:

• npm package (@next dist-tag)
• GitHub release

Your semantic-release bot 📦🚀