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 one buffer per row. 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. For every existing type this is byte-identical (all types with resolveLength have legacy ids); it fixes the case fix: resolve parameter lengths for all types that can resolve one #1771 describes for TDS 7.2+ ids such as VECTORTYPE and JSONTYPE, in both places fix: resolve parameter lengths for all types that can resolve one #1771 changes. fix: resolve parameter lengths for all types that can resolve one #1771 can be closed once this lands.
• 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.
• Unit suite: 498 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: the bulk load checkConstraints integration test no longer names its CHECK constraint. Constraint names on temp tables are unique per database, so the named one collided when two Azure CI jobs ran the test concurrently.

Measurements

Serialization only, same machine. benchmarks/parameters/scalar-params.js is new (20 scalar parameters per request, resolved once per request as validateParameters does).

	20.0.0	master (f8d5205)	this PR
20 scalar params, req/s	~23k	~47-51k	~50-53k
10 MB varbinary(max), req/s		~9.6-11k	~11.7-12.4k
1 MB varbinary(max), req/s		~14.4-15.9k	~14.1-15.8k

Within noise of master: this PR is a restructuring, the throughput gain over 20.0.0 is #1773's.

🤖 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 fix: 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 fix: 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 fix: 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.68293% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.20%. Comparing base (daa8e77) to head (9c5d24c).

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.89%   82.20%   +0.30%     
==========================================
  Files          92       92              
  Lines        4950     5019      +69     
  Branches      933      954      +21     
==========================================
+ Hits         4054     4126      +72     
+ Misses        601      599       -2     
+ Partials      295      294       -1     

☔ 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.