fix: wrap all parameter serialization errors in InputError

[arthurschreiber]

Problem

Parameter serialization errors are wrapped inconsistently, and in some paths not handled at all:

RPC requests (RpcRequestPayload.generateParameterData): only errors thrown while generating a parameter's data are wrapped in an InputError that names the failing parameter:

yield type.generateTypeInfo(param, this.options);
yield type.generateParameterLength(param, this.options);
try {
  yield * type.generateParameterData(param, this.options);
} catch (error) {
  throw new InputError(...);
}

Errors thrown while generating the parameter's type info or length prefix — e.g. a RangeError: The value of "value" is out of range from writing an out-of-range length — escape as raw, unattributed errors with no indication of which parameter caused the failure.

Bulk loads (RowTransform._transform): errors thrown while generating a column's metadata (getColMetaData → generateTypeInfo) escaped the transform with no deliberate handling at all (relying on Node's stream internals to capture a synchronous _transform throw), and errors thrown while serializing row values surfaced raw, without naming the affected column.

Fix

All serialization steps now go through the same InputError wrapping:

• RPC parameters: Input parameter '<name>' could not be validated, original error as cause (unchanged message, now covering all three steps).
• Bulk load columns: Column '<name>' could not be serialized, original error as cause, for both column metadata generation and row value serialization.

Note one deliberate behavior change: bulk load serialization failures that previously surfaced the raw error on the bulk load's callback (e.g. the RangeError for an out-of-range DECIMAL value) now surface the wrapping InputError, with the original error available as cause. The existing integration test covering this path (should not throw in _transform function) was updated accordingly.

Validation

The downstream recovery paths are unchanged and were validated against the spec: when a request payload errors, Connection.makeRequest aborts the partially written request message by setting the packet status IGNORE bit together with EOM ([MS-TDS] v20260617 s2.2.3.1.2: "(From client to server) Ignore this event (0x01 MUST also be set)"), so the connection remains usable and the error surfaces on the request's callback; bulk loads destroy the row/packet streams and surface the error on the bulk load's callback as before. This change only affects error typing and attribution, not the wire behavior.

New unit tests cover errors thrown from each RPC serialization step and both bulk load paths. Full unit suite (450 tests) and the RPC, parameterised-statement, and bulk load integration suites (206 tests, against SQL Server 2022) pass.

🤖 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-01T22:15:31.321988Z	6238640	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]

Reviewed the diff. This is a small, well-targeted fix — feedback below.

Summary

The fix is correct and minimal: moving generateTypeInfo/generateParameterLength inside the existing try/catch in RpcRequestPayload.generateParameterData (src/rpcrequest-payload.ts:114-119) means every parameter serialization failure is now uniformly wrapped in an InputError naming the offending parameter, instead of only failures from the final data-generation step.

Correctness

• The downstream recovery path checks out: Connection.makeRequest (src/connection.ts:3287-3296) attaches a payloadStream.once('error', ...) handler that sets message.ignore = true and ends the message regardless of the error's type, so wrapping the error in InputError doesn't change wire behavior — only what the caller sees in the request callback. Good verification in the PR description.
• InputError extends TypeError (src/errors.ts:30), and cause is passed through correctly via the ErrorOptions second constructor arg, consistent with how it's used elsewhere in this file.

Test coverage

• The new test file (test/unit/rpcrequest-payload-error-test.ts) cleanly covers all three now-wrapped steps (type info, length, data) using a stubbed DataType, which is a nice way to isolate the behavior under test without depending on a real data type's validation quirks. Follows the repo's existing *-test.ts naming convention.
• One gap: there's no test asserting that a successful parameterized RPC call still works end-to-end after this change (i.e., that reordering the yields doesn't affect the happy path). The existing RPC integration/unit suites presumably already exercise this path indirectly, so this is a nice-to-have rather than a blocker.

Minor/optional observations (not blocking)

• src/bulk-load.ts has a very similar pattern: generateParameterLength/generateParameterData are wrapped in try/catch (bulk-load.ts:207-215), but generateTypeInfo for column metadata (bulk-load.ts:566) is not, and its errors would similarly escape unattributed. That's a different code path (column metadata vs. per-row parameter data, and no per-parameter InputError convention there today), so it's out of scope for this PR, but might be worth a follow-up if consistency across bulk load and RPC error attribution is a goal.
• No behavioral or API concerns otherwise — the change is a pure reordering/scope-widening of an existing try/catch, so risk is low.

Nice, focused fix with good test coverage and a well-reasoned validation note on the wire-protocol side.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.09%. Comparing base (170fabc) to head (6238640).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1772      +/-   ##
==========================================
+ Coverage   81.02%   81.09%   +0.06%     
==========================================
  Files          92       92              
  Lines        4948     4966      +18     
  Branches      938      939       +1     
==========================================
+ Hits         4009     4027      +18     
  Misses        640      640              
  Partials      299      299              

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

[claude]

Review

Nice, focused fix — the RPC and bulk-load error paths now attribute failures to the parameter/column that caused them, and the new unit tests do a good job pinning down each of the three serialization steps independently. A few things worth a look:

Inconsistency: validate() errors in bulk load are still unwrapped

In RowTransform._transform (src/bulk-load.ts), the row-value loop now wraps generateParameterLength/generateParameterData errors in InputError (line ~219), but the validate() call just above it does not:

if (!this.bulkLoad.firstRowWritten) {
  try {
    value = c.type.validate(value, c.collation);
  } catch (error: any) {
    return callback(error);   // <-- raw error, no column name, no InputError
  }
}

Since DataType.validate routinely throws (e.g. Int.validate throws TypeError('Invalid number.') for a non-numeric value), an invalid value on the first row of a bulk load will still surface a raw, unattributed error on the bulk load's callback — the exact failure mode this PR sets out to fix, just one call earlier in the same function. It'd be worth wrapping this the same way (Column '${c.name}' could not be serialized) for consistency, especially since the RPC-equivalent (Request.validateParameters in request.ts:500-509) already wraps its validate() errors in a named RequestError.

Possible breaking-change / semver note

The PR description explicitly (and correctly) flags that bulk-load callbacks used to receive raw errors (e.g. a RangeError for an out-of-range DECIMAL) and now receive a wrapping InputError instead. That's a reasonable improvement, but it does change the concrete error type seen by any consumer doing err instanceof RangeError (or similar) on a bulk-load callback. Since the repo uses semantic-release off conventional commits and both commits here use a plain fix: prefix, this change would ship as a patch release. Given it's a user-visible error-type/shape change, it might warrant a BREAKING CHANGE: footer (or at least a mention in the commit body) so it's called out properly in release notes — err.cause does preserve the original error, so most consumers should be fine, but it's worth being explicit for anyone doing type-based error matching.

Minor nit

Inconsistent catch-clause typing style in bulk-load.ts: catch (error: any) in RowTransform._transform/the row loop vs. plain catch (error) in getColMetaData (line 573). Not functionally significant since ErrorOptions.cause accepts unknown, just a small style inconsistency introduced in the same diff.

What's solid

• Reusing the existing InputError (extends TypeError) rather than inventing a new error class keeps this consistent with the rest of the codebase.
• Good coverage: dedicated unit tests for each of the three RPC serialization steps (generateTypeInfo, generateParameterLength, generateParameterData) and both bulk-load paths (column metadata, row values), using minimal stub DataType objects rather than a real type — keeps the tests focused on the wrapping behavior itself.
• The integration test update for should not throw in _transform function correctly asserts on err.cause rather than just changing the top-level assertion, so the original error information is verified as preserved, not just swallowed.
• The PR description's analysis of the downstream recovery path (IGNORE bit / packet abort per [MS-TDS] 2.2.3.1.2) is a nice touch — confirms this is purely an error-typing change with no wire-format impact.

🤖 Generated with Claude Code

[claude]

Reviewed the changes to src/bulk-load.ts, src/rpcrequest-payload.ts, and the tests. Overall this is a solid, well-scoped fix — good problem statement, and the final commit's restructuring (only wrapping the type's own serialization calls, manually iterating the RPC parameter-data generator) nicely avoids misattributing stream/consumer errors to the parameter/column, which is a real subtlety that's easy to get wrong. Test coverage for both new code paths (RPC type-info/length/data, bulk load metadata/row-value) looks thorough.

A few observations, none blocking:

1. Asymmetric protection around generateParameterData() construction (src/rpcrequest-payload.ts)

const parameterData = type.generateParameterData(param, this.options)[Symbol.iterator]();
while (true) {
  let result;
  try {
    result = parameterData.next();
  } catch (error) { ... }

The call type.generateParameterData(param, this.options) itself sits outside the try, whereas the equivalent bulk-load path wraps the whole call+spread:

parameterDataBuffers = [...c.type.generateParameterData(parameter, this.mainOptions)];

In practice every real DataType.generateParameterData is a function*, so calling it just builds an iterator without running any body code, and this is safe. But a couple of the internal placeholder types (IntN, Null, DecimalN, etc. in src/data-types/*.ts) implement generateParameterData as a plain function that throws synchronously and immediately — if one of those were ever reached here (they're not user-selectable via TYPES, so today it shouldn't happen), the error would escape unwrapped, unlike the bulk-load version. Might be worth a one-line comment noting the "must be a generator function" assumption, or just wrapping the initial call too for parity with bulk-load.ts.

2. Double-wrapping for TVP parameters

data-types/tvp.ts already wraps per-row column errors in its own InputError (TVP column '...' has invalid data at row index ...). Since RpcRequestPayload.generateParameterData now wraps every error surfacing from parameterData.next() (including TVP's internal generator), a TVP validation failure ends up as InputError('Input parameter ... could not be validated', { cause: InputError('TVP column ... ', { cause: original }) }) — two levels of wrapping, so the more specific TVP/column detail is one .cause further away than before. This isn't a regression introduced by this PR (the same try/catch scope already covered generateParameterData before this change), just worth being aware of — callers inspecting error.cause.message for the specific reason will need error.cause.cause in the TVP case.

3. Minor test duplication

The buildType(...) stub-DataType builder is duplicated verbatim between test/unit/bulk-load-test.ts and test/unit/rpcrequest-payload-error-test.ts. Could live in a shared test helper, but not a big deal at this size.

Other notes

• The integration test update (RangeError → TypeError + .cause check) correctly reflects that InputError extends TypeError, and the PR description is upfront that this is a deliberate, documented behavior change for bulk load callback consumers.
• No security concerns — error messages only interpolate parameter/column names that the caller already supplied, and nothing here changes wire behavior.
• Performance impact is negligible: the bulk-load column-value buffer array is bounded to one column's data per row, and the RPC path's manual .next() loop is equivalent cost to the previous yield* delegation.

Nice work on isolating exactly which calls needed the try/catch in the final commit — that's the trickiest part of this kind of change to get right.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 67fcfd9433

ℹ️ 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

Overall this is a solid, well-scoped fix — consistent InputError wrapping across both RPC and bulk-load serialization paths, with good test coverage for each new failure point and a clear PR description with spec justification for the "recovery path unchanged" claim. A couple of things worth a look:

1. src/rpcrequest-payload.ts:112-152 — .return()/.throw() no longer delegate into generateParameterData

The old code used yield * type.generateParameterData(...), which automatically forwards .return()/.throw() calls made on the outer generator into the inner one. The rewrite replaces this with a manual loop over type.generateParameterData(...)[Symbol.iterator]().next():

```ts
let parameterData;
try {
parameterData = type.generateParameterData(param, this.options)Symbol.iterator;
} catch (error) { ... }

while (true) {
let result;
try {
result = parameterData.next();
} catch (error) { ... }
if (result.done) break;
yield result.value;
}
```

Since there's no yield*, a .return() call on the outer RpcRequestPayload generator (which happens when payloadStream.destroy() is called on request cancellation — see connection.ts around the onCancel handler and Readable.from(payload) in makeRequest) no longer propagates into parameterData. If a DataType.generateParameterData implementation ever relies on try/finally for cleanup (e.g., releasing a resource), that cleanup would silently stop running on cancellation after this change. No built-in type currently does this, so it's not an active bug, but it's a narrowing of the generator contract worth being aware of — might be worth a comment noting the tradeoff, or restoring delegation with something like manually calling .return()/.throw() on parameterData if the outer generator receives them (harder to do cleanly without yield*).

2. test/unit/bulk-load-test.ts — serialization-error tests don't cover generateParameterData throwing

The new describe('serialization errors', ...) block covers generateTypeInfo and generateParameterLength throwing, but not generateParameterData (the RPC test file rpcrequest-payload-error-test.ts does cover all three steps, including data generation and iteration). Since bulk-load.ts's _transform also wraps [...c.type.generateParameterData(...)] in the same try/catch, a matching test would close the gap and guard against a future refactor accidentally moving that call outside the try/catch.

Minor

• The behavior change for bulk loads (raw error → wrapped InputError with cause) is a breaking change for any consumer doing instanceof RangeError-style checks on bulk load callback errors. It's called out in the PR description and the integration test was updated, which is good — just flagging it's worth a mention in a changelog/release notes entry if this repo maintains one, since it's technically not backward compatible for error-handling consumers.

Nice test structure overall (the shared buildType stub helper keeps the new tests concise), and the spec citation for why aborting the payload mid-write is safe is a nice touch.