fix: wrap bulk load serialization errors in InputError

[arthurschreiber]

Problem

Bulk load serialization errors are not attributed to the column that caused them. An error thrown while writing a column's TYPE_INFO in getColMetaData escapes with no deliberate handling, and an error thrown while serializing a row value (e.g. a RangeError for an out-of-range DECIMAL) surfaces raw, without naming the column.

RPC requests had the same gap for TYPE_INFO and length-prefix errors. That half was delivered by #1774 (now merged), which wraps every serialization step of a parameter in the InputError naming it, so this PR is narrowed to bulk loads.

Fix

Both bulk load serialization steps now go through the same wrapping: Column '<name>' could not be serialized, an InputError with the original error as cause, for column metadata (writeTypeInfo) and row values (writeValue). Only the type's serialization call is wrapped, so errors from the payload's own plumbing are not misattributed to the column. COLMETADATA is written inside the payload's try, so a TYPE_INFO failure closes the row source the way a row failure does (#1779's ownership contract).

InputError is now exported from the package entry point alongside ConnectionError and RequestError. master already hands it to callers from the RPC and TVP paths, and this PR adds the bulk load, but it was only reachable through lib/errors, so err instanceof InputError was not possible from outside the package.

Deliberately out of scope: errors from validate still surface as thrown, unwrapped. Validation errors are a different failure mode (a bad value rather than a value the type cannot serialize) and already carry the type's own message; wrapping them is a separate decision.

One deliberate behaviour change: bulk load serialization failures that previously surfaced the raw error on the bulk load's callback 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) is updated accordingly.

Validation

The downstream recovery path is unchanged: a failed bulk load closes its row source and surfaces the error on the bulk load's callback as before. This change only affects error typing and attribution, not the wire behaviour.

New unit tests cover errors thrown from column metadata generation, from a row value's length prefix and from its data generator, and that a column metadata failure closes the row source. Unit suite (529 tests) and the bulk load integration suite (against SQL Server 2022) pass. Lint and typecheck clean.

Was stacked on #1774; with that merged, this PR targets master and its diff is the bulk load work alone (5 files).

🤖 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-03T13:12:20.093722Z	5b5055c	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 82.51%. Comparing base (563d37b) to head (cdcc847).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1772      +/-   ##
==========================================
+ Coverage   82.50%   82.51%   +0.01%     
==========================================
  Files          92       92              
  Lines        5035     5039       +4     
  Branches      961      961              
==========================================
+ Hits         4154     4158       +4     
  Misses        592      592              
  Partials      289      289              

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

[claude]

Review

Reviewed the diff (src/bulk-load.ts, test/integration/bulk-load-test.ts, test/unit/bulk-load-test.ts) plus the surrounding writeTypeInfo/writeValue contract in src/data-type.ts and InputError in src/errors.ts.

Overall

Clean, well-scoped change. It extends the same InputError wrapping pattern used for RPC parameters (#1774) to bulk load columns, and correctly narrows the try/catch to only the type's serialization call in both spots:

• getColMetaData wraps writeTypeInfo (src/bulk-load.ts:574-578)
• RowTransform._transform wraps writeValue (src/bulk-load.ts:217-221)

Because getColMetaData already wraps its own error, the _transform catch around this.bulkLoad.getColMetaData() (bulk-load.ts:176-180) is just a plain pass-through (callback(error), no double-wrapping) — good, that avoids nesting one InputError inside another.

Bugs / correctness

Didn't find any. I checked for:

• Double-wrapping — none, confirmed above.
• Other callers of getColMetaData that might assume the raw error shape — there's only the one caller in this file.
• Anywhere in src/test that pattern-matches on the previously-raw RangeError from this path — only the integration test did, and it's updated in this PR.

Test coverage

Good coverage for the new behavior:

• Unit tests use a minimal stub DataType to independently exercise the generateTypeInfo-throws and generateParameterLength-throws paths, asserting InputError, the message, and .cause.
• The integration test (should not throw in _transform function) is updated to match the new wrapped-error shape rather than just relaxed.

One small gap: the "row values" unit test throws from generateParameterLength, but not from generateParameterData (the per-chunk generator writeValue iterates over). Both are inside the same try in _transform, so it's almost certainly covered by the same code path, but a chunk-generator-throws case would make that explicit rather than implied.

API surface (minor, possibly out of scope for this PR)

InputError isn't exported from the public entry point (src/tedious.ts only exports ConnectionError/RequestError). Consumers can currently only narrow this new error via instanceof TypeError + a message regex, which is fragile (any built-in TypeError would also match instanceof TypeError). Since InputError is now part of the documented failure mode for both RPC parameters and bulk load columns, it'd be worth exporting it from src/tedious.ts — though that may belong in #1774 rather than here, since the class is introduced there.

Style nit

catch (error: any) (bulk-load.ts:178, pre-existing pattern at 196) vs. catch (error) / implicit unknown (bulk-load.ts:576, matching rpcrequest-payload.ts). Not a real issue since ErrorOptions.cause accepts unknown, just a minor inconsistency between the two call sites added/touched in this diff.

Security / performance

No concerns — this only changes error attribution/typing on already-thrown synchronous errors; no new I/O, no change to wire behavior, and no user-controlled data flows into anything new (the column name interpolated into the message is a column identifier already sent over the wire in TDS COLMETADATA, not attacker-controlled free text beyond what a caller of addColumn already provides).

Nice work — the scoping down from the original multi-commit attempt (only wrapping the type's serialization call, not stream/generator-consumer errors) is exactly the right call, and the PR description does a good job explaining that evolution.

[claude]

Review

Nicely scoped fix — the wrapping is applied precisely around the type's serialization call in both getColMetaData (writeTypeInfo) and _transform (writeValue), so stream-internal errors aren't misattributed to a column. I confirmed:

• getColMetaData's inner try/catch (bulk-load.ts:574-577) wraps writeTypeInfo in InputError and throws; _transform's outer catch (bulk-load.ts:176-180) just forwards that already-wrapped error via callback(error) — no double-wrapping. Good.
• writeValue's default path in data-type.ts consumes generateParameterLength and iterates generateParameterData synchronously inside the single writeValue(...) call, so the try/catch around it (bulk-load.ts:217-221) also catches generator-chunk errors, matching the third unit test.
• getColMetaData has only one call site, so there's no other caller left with the old unwrapped-throw behavior.

A couple of minor things worth a look, non-blocking:

1. Integration test asserts the looser TypeError instead of InputError (test/integration/bulk-load-test.ts:1566): assert.instanceOf(err, TypeError) passes today since InputError extends TypeError, but it doesn't actually assert the new type is InputError. The unit tests import and assert InputError directly — worth doing the same here for consistency and a tighter regression check.
2. Validation errors remain unwrapped: c.type.validate(...) errors in _transform (bulk-load.ts:193-198) still call callback(error) directly without column attribution, unlike the two serialization paths. That's presumably intentional/out-of-scope per the PR description (which scopes this to "serialization" specifically), but might be worth a one-line note in the PR body so reviewers don't wonder if it was missed.
3. Breaking change flagged correctly — swapping a raw RangeError/etc. for a wrapping InputError on the bulk load callback is a real behavior change for any consumer pattern-matching on error type/message (e.g. catching RangeError for out-of-range DECIMAL). Good that it's called out explicitly in the description and covered by the updated integration test; just flagging it here too since it's user-visible on master once this lands.

Test coverage looks solid: the three new unit tests cleanly isolate metadata-generation, value-length, and chunk-generation failure points using a minimal stub DataType, and the integration test was updated to match the new wrapping. No security or performance concerns — this only affects the error path, not the wire protocol or hot path.

[claude]

Review of #1772: wrap bulk load serialization errors in InputError

Nicely scoped change — reviewed the diff plus the stacked #1774 context for consistency.

Code quality / best practices

• The try/catch blocks are drawn tightly around just the throwing call (writeTypeInfo in getColMetaData, writeValue in _transform), exactly as the PR description promises — errors from tBuf.writeBVarchar/stream internals stay unwrapped and won't be misattributed to a column. Good discipline.
• _transform's outer try { this.bulkLoad.getColMetaData() } catch (error: any) { return callback(error); } correctly avoids double-wrapping, since getColMetaData() already returns an InputError for the only thing inside it that can throw.
• Minor consistency nit: the sibling RPC wrapping added in refactor: resolve parameters once and serialize them through a write contract #1774 uses the message Input parameter '<name>' could not be validated, while this PR uses Column '<name>' could not be serialized. Both wrap a serialization failure (not a validation failure) in an InputError, so a consumer catching InputError across both paths sees inconsistent wording ("validated" vs "serialized") for the same conceptual event. Worth a follow-up to align the phrasing — "serialized" reads more accurate for both, since validate() errors are explicitly out of scope for wrapping in both PRs.

Potential bugs

• Nothing introduced by this diff looks broken — the wrap sites match exactly the two places bulk load serialization can throw (TYPE_INFO, row value).
• Tangential, pre-existing (not touched by this PR): firstRowWritten is declared and initialized to false in bulk-load.ts but is never set to true anywhere in src/. That means the if (!this.bulkLoad.firstRowWritten) guard around c.type.validate(...) at bulk-load.ts:193 is always true, so validate actually runs unwrapped on every row, not just the first, and addColumn's "cannot add columns after the first row" check can never fire. The PR description's "errors from validate on the first row still surface as thrown, unwrapped" is therefore a bit narrower than actual behavior (it's every row). Not a regression from this change, but since the PR body leans on that distinction to justify scope, flagging it in case it's news.

Performance

No concerns — the added try/catch wraps calls that already happen once per column per row; V8's cost for a non-throwing try block is negligible here.

Security

None. The column name is only interpolated into an Error message string, never evaluated or used in a query, so no injection surface.

Test coverage

Solid:

• Three new unit tests in test/unit/bulk-load-test.ts independently cover all three throw sites: generateTypeInfo (column metadata), generateParameterLength (length prefix), and generateParameterData (row chunks) — matching the three try/catch sites touched.
• The integration test update correctly asserts the new shape (InputError, message naming the column, .cause chain preserving the original RangeError and its message) rather than just checking instanceof.
• One gap: no test asserts that the validate-path error (first row, deliberately out of scope) is still a raw/unwrapped error post-change, as a regression guard for the "deliberately out of scope" boundary. Given the firstRowWritten note above, that path currently runs on every row anyway, so such a test would also incidentally document that behavior.

Overall: a well-targeted, well-tested fix. The items above are optional polish (message-wording alignment with #1774, and maybe a note in the description about validate running on every row rather than just the first) rather than blockers.

[claude]

Review

Nicely scoped change. I read through src/bulk-load.ts, src/bulk-load-payload.ts, and both test files, and compared the wrapping pattern to the existing InputError usage in rpcrequest-payload.ts / data-types/tvp.ts.

Correctness

• The two try/catch additions are placed correctly: getColMetaData()'s writeTypeInfo call in bulk-load.ts:440-444, and writeValue per-column in bulk-load-payload.ts:164-168. Both wrap only the type's serialization call, not the surrounding buffer writes (writeUsVarchar/writeBVarchar, the row-token write, the chunk-flush logic) — matching the stated intent of not misattributing stream/consumer errors to a column.
• Moving buffer.writeBuffer(bulkLoad.getColMetaData()) inside the outer try in bulk-load-payload.ts:122 is the right call — it means a TYPE_INFO failure now goes through the same iterator.return() cleanup path as a row failure, instead of escaping before the row source is ever closed. The new "closes the row source when the column metadata cannot be written" test exercises exactly this.
• No double-wrapping risk: the outer catch (err) in [Symbol.asyncIterator] (bulk-load-payload.ts:181) only does cleanup and rethrows err as-is, so an InputError thrown by getColMetaData() isn't re-wrapped.
• writeValue's default path (data-type.ts:190-200) runs generateParameterLength and iterates generateParameterData synchronously within the same call frame, so wrapping the single writeValue(...) call is sufficient to catch failures from either step — confirmed by the three new unit tests (metadata, length prefix, data generator all throwing).

Minor observations (non-blocking)

• Message wording is slightly inconsistent across the codebase: this PR uses "Column '<name>' could not be serialized", while the already-merged RPC path (rpcrequest-payload.ts:86) uses "Input parameter '<name>' could not be validated" even though it wraps writeTypeInfo/writeValue, not validate. Not introduced by this PR and out of scope, but worth a follow-up for consistency since both are now the same class of error (InputError wrapping a serialization failure).
• The integration test change (RangeError → InputError with .cause) is a real, documented behavior change for consumers who catch specific error types off the bulk load callback. Worth double-checking whether this warrants a note in release notes, since existing user code doing err instanceof RangeError on that callback would silently stop matching (it'd need err.cause instanceof RangeError now).

Test coverage

Good coverage of the three failure points (column metadata, length prefix, data generator) plus the source-cleanup behavior. The integration test update correctly asserts both the new InputError and that the original RangeError is preserved via cause.

Overall this looks correct, well-tested, and appropriately narrow — no issues that should block merging.

[claude]

Review

Reviewed src/bulk-load-payload.ts, src/bulk-load.ts, and the accompanying tests.

Code quality / correctness

• The fix is well-scoped: only the type's writeTypeInfo/writeValue calls sit inside the try, so errors from stream internals (Transform#push, iterator plumbing) are no longer misattributed to a column, per the refactor in the later commits. That distinction is easy to get wrong and this PR clearly reasoned through it.
• Moving buffer.writeBuffer(bulkLoad.getColMetaData()) inside the outer try in bulk-load-payload.ts is the right call — it makes a TYPE_INFO failure close the row source (iterator.return()) the same way a row-value failure does, instead of leaking the source. The new "closes the row source when the column metadata cannot be written" test exercises exactly this, including the eager pre-fetch done in the constructor.
• The scoping decisions are explicit and reasonable: validate() errors stay unwrapped (different failure mode, already has a good message), and the pre-existing firstRowWritten dead code is correctly called out as untouched rather than opportunistically "fixed" in this PR.
• No double-wrapping risk: writeTypeInfo/writeValue in data-type.ts don't wrap anything themselves, so an InputError thrown out of getColMetaData (bulk-load.ts) simply passes through the payload's outer catch unchanged instead of getting re-wrapped.

Potential issue: InputError isn't part of the public API surface

InputError (src/errors.ts) is now a documented, user-visible error type — bulk load callbacks (and RPC requests, via #1774) will hand it to consumers — but it is not re-exported from src/tedious.ts, unlike ConnectionError/RequestError. Since package.json's main/types point only at lib/tedious.js/.d.ts, a consumer can't currently do err instanceof InputError without reaching into an internal build path (tedious/lib/errors). Worth adding InputError to the export { ... } block in src/tedious.ts before this (or #1774) ships, so the new error type is actually usable from outside the package.

Test coverage

• Good coverage of the three failure sites: column metadata (generateTypeInfo), length prefix (generateParameterLength), and value chunks (generateParameterData), each asserting the InputError message, column name, and cause.
• The integration test update correctly reflects the new (intentional) behavior change — the raw RangeError is now wrapped, with the original preserved as cause.
• Minor/optional: the "closes the row source" test only covers the COLMETADATA failure path; the same assertion for a row-value failure (writeValue throwing) isn't explicitly tested, even though the closing logic is shared code. Not blocking, just would round out symmetry.

Security

No concerns — this only changes error typing/attribution for already-local, synchronous serialization failures; it doesn't change what goes on the wire, and the cause chain stays in-process (not serialized or logged automatically).

Overall this is a clean, narrowly-scoped fix with solid test coverage. The main thing I'd want addressed before merge is exporting InputError from the package entrypoint.

[claude]

Review

Solid, well-scoped change. The fix does what it says: getColMetaData's writeTypeInfo call and the per-cell writeValue call are each wrapped narrowly, so only the type's own serialization logic gets attributed to the column — buffer/stream plumbing around those calls is left alone, matching the stated intent in the commit messages.

Correctness

• Moving buffer.writeBuffer(bulkLoad.getColMetaData()) inside the try in bulk-load-payload.ts (src/bulk-load-payload.ts:122) is the key fix — previously a TYPE_INFO failure would throw before the try/finally that closes the row source, leaking the async row iterator instead of calling .return() on it. Now a metadata failure closes the source the same way a row failure does. The added unit test ("closes the row source when the column metadata cannot be written") exercises exactly this.
• No double-wrapping: getColMetaData() already returns an InputError (wrapped in bulk-load.ts), and the payload's outer catch just closes the iterator and rethrows without re-wrapping, so the original InputError/cause chain survives intact.
• writeValue fully drains generateParameterData internally (src/data-type.ts:190-200), so wrapping the single writeValue(...) call also covers errors thrown lazily from the generator body during iteration, not just from generateParameterLength/writeValue itself — the "generates row value chunks" unit test confirms this.

Minor / nit

• test/integration/bulk-load-test.ts:1690 — the test title 'should not throw in _transform function' is now stale: bulk load serialization no longer goes through a _transform/RowTransform implementation (I couldn't find one left in src/; it's BulkLoadPayload's async iterator now). Since this PR already rewrites the test body's assertions, it'd be a good time to rename it to something like 'wraps out-of-range value errors in InputError'.
• Slight asymmetry worth a sentence in the PR description (may already be intentional/covered by refactor: resolve parameters once and serialize them through a write contract #1774): writeValue's wrap also implicitly covers the buffer.writeBuffer(chunk) calls inside writeValue's internal loop, not just the type's generateParameterData/generateParameterLength. In practice WritableTrackingBuffer.writeBuffer is unlikely to throw independent of the chunk it's given, so this seems fine, but it's a slightly larger surface than "only the type's serialization call" implies literally.

Tests
Good coverage: unit tests hit all three failure points (generateTypeInfo, generateParameterLength, generateParameterData) plus the row-source-closing behavior, and the integration test was updated to assert the new InputError/cause shape rather than the raw RangeError. The buildType stub helper is a nice, minimal way to exercise the contract without a real SQL type.

Security/Performance: no concerns — the added try/catch is on a cold path (only taken on error) and the wrapped message only echoes a column name that's already known to the caller (from their own addColumn call).

Nice, focused PR — the scoping decisions (leaving validate errors unwrapped, calling out the pre-existing dead firstRowWritten guard as untouched) are called out clearly and are reasonable.

[claude]

Review

Nice, focused change — the fix is small and the test coverage is thorough (unit tests for TYPE_INFO/length/data-generator failures, an explicit "row source is closed" test, and the integration test updated to match the new wrapping behavior).

Correctness

• The key fix — moving buffer.writeBuffer(bulkLoad.getColMetaData()) inside the try in bulk-load-payload.ts — is correct and well justified: a getColMetaData() failure now closes the row source the same way a row-serialization failure does, which matches the "close row source on any failure" contract described in the PR body. Good catch.
• Wrapping only the writeTypeInfo/writeValue calls (not the surrounding buffer bookkeeping) keeps errors from the payload's own plumbing from being misattributed to a column, which is a sensible boundary.
• Exporting InputError from src/tedious.ts closes a real gap — without it, callers couldn't do err instanceof InputError even though the RPC/TVP paths (from refactor: resolve parameters once and serialize them through a write contract #1774) already handed it out.

Minor observations (non-blocking)

• Both the TYPE_INFO failure (bulk-load.ts) and the value-serialization failure (bulk-load-payload.ts) produce the identical message Column '<name>' could not be serialized. A consumer can't tell from the message alone which serialization step failed — they'd have to inspect .cause. That may be intentional (cause-chaining is the documented mechanism), but a slightly different wording for each site (e.g. "...metadata could not be serialized" vs "...value could not be serialized") would make the top-level message self-descriptive without needing cause. Worth considering, not a blocker.
• If a column's type is itself a TVP whose row validation already throws an InputError (TVP column '...' has invalid data..., from data-types/tvp.ts), the bulk-load wrapping here will wrap that InputError again inside another InputError. That's a reasonable nested-cause chain, but worth double-checking it reads sensibly to a caller (error.cause.cause), since previously it'd have been a single-level InputError.
• The chosen behavior change (raw errors, e.g. RangeError for out-of-range DECIMAL, now surface wrapped instead of raw) is called out explicitly in the PR description and the updated integration test reflects it — good, since this is technically a breaking change for anyone doing assert.instanceOf(err, RangeError) downstream. Worth a mention in the changelog/release notes if not already planned, since it's user-visible for bulk load error handling.

Security / performance

No concerns — this only adds try/catch wrapping around existing serialization calls; no new I/O, parsing, or user-input handling paths introduced.

Test coverage

Solid. The unit tests cover all three failure sites (generateTypeInfo, generateParameterLength, generateParameterData) plus the row-source-closing behavior, and the integration test was updated to assert on the new wrapped error and its cause. I don't see gaps here.

Overall: a clean, well-scoped fix with good test coverage. The only follow-up worth considering is whether the two wrap sites should have distinguishable messages.

[claude]

Review

Reviewed the diff (src/bulk-load-payload.ts, src/bulk-load.ts, src/tedious.ts, and the two test files). This is a small, tightly-scoped change and it's well executed.

Correctness

• The relocation of buffer.writeBuffer(bulkLoad.getColMetaData()) into the try block in bulk-load-payload.ts is the right fix, not just a refactor: previously, if getColMetaData() threw, the call sat outside the try/catch/finally, so the row source's iterator would never have .return() called on it — a real leak for stream-backed row sources. Now a TYPE_INFO failure closes the row source the same way a row-value failure does, and there's a dedicated unit test (closes the row source when the column metadata cannot be written) verifying it.
• Wrapping is scoped precisely to writeTypeInfo/writeValue calls, not the surrounding buffer-management code (chunk flushing, row/done tokens). That means a bug in the payload's own plumbing won't get misattributed to a column, which matches the PR's stated intent and mirrors the equivalent RPC-side contract from refactor: resolve parameters once and serialize them through a write contract #1774.
• done/closed/finally bookkeeping is preserved correctly — the new try around getColMetaData() doesn't introduce a double-close path.

API surface

• Exporting InputError from tedious.ts is a sensible, minimal addition — it was already being handed to callers via bulk load/RPC callbacks, just not instanceof-checkable from outside the package. Placed alphabetically alongside the other error exports.

Test coverage

• Good coverage of all three failure points: column metadata (generateTypeInfo), length prefix (generateParameterLength), and data chunks (generateParameterData), plus the row-source-closes-on-metadata-failure case. Using a minimal stub DataType keeps these unit tests independent of any specific real type's behavior.
• The integration test rename (should not throw in _transform function → wraps a value the column type cannot serialize in an InputError naming the column) accurately reflects both the new behavior and why the old name no longer made sense (referenced a _transform method that doesn't exist anymore per the last commit).
• Nice touch asserting on cause in addition to the outer InputError, so the original RangeError and its message aren't lost.

Minor observations (non-blocking)

• The wrapping in bulk-load.ts's getColMetaData() only covers the writeTypeInfo call, not the subsequent TableName/ColName writes (writeUsVarchar/writeBVarchar). That's consistent with the PR's stated scoping rule ("only the type's serialization call is wrapped"), so not a bug, but worth confirming that a pathologically long table/column name can't itself throw an unattributed error — if it can, that's an existing gap rather than one introduced here.
• The behavior change called out in the PR description (bulk load callback now receives the wrapping InputError instead of the raw error) is a breaking change for any consumer doing err instanceof RangeError on a bulk load callback. Worth a mention in release notes/changelog if not already planned, since it's not obviously discoverable from the diff alone.

No security or performance concerns — this only changes error attribution on failure paths, not the wire format or serialization logic itself. Nice, minimal PR.

[claude]

Review

Went through the diff (src/bulk-load-payload.ts, src/bulk-load.ts, src/tedious.ts, and the test files) plus the surrounding context in data-type.ts and the already-merged RPC counterpart (#1774) for consistency. Overall this is a clean, well-scoped change - the wrapping is narrow, the docstring contract in BulkLoadPayload is honored, and the tests target the right seams.

Code quality / best practices

• The try/catch blocks are scoped tightly around only writeTypeInfo/writeValue, not the surrounding buffer/pointer writes - matches the stated intent ("errors from the payload's own plumbing are not misattributed to the column") and mirrors the pattern already established for RPC params and TVP columns.
• Moving buffer.writeBuffer(bulkLoad.getColMetaData()) inside the outer try is the right call - it makes a COLMETADATA failure close the row source the same way a row failure does, and there's a dedicated unit test (closes the row source when the column metadata cannot be written) proving it.
• Exporting InputError from src/tedious.ts alongside ConnectionError/RequestError is a sensible, minimal fix for the instanceof gap described in the PR body.

Potential bugs

• Nothing stood out as broken. One thing worth double-checking (not a bug, just flagging for reviewers): the message text is identical for both failure sites - Column '<name>' could not be serialized for both a TYPE_INFO failure (bulk-load.ts) and a value failure (bulk-load-payload.ts). That's clearly intentional per the PR description, and cause disambiguates, but it does mean the top-level message alone can't tell you which phase failed - worth a mention in case that's ever surfaced to end users without unwrapping cause.

Performance

• No concerns. The added try/catch per cell/column is negligible next to the actual serialization work, and no extra buffering or copying was introduced - writeValue/writeTypeInfo still write straight into the shared WritableTrackingBuffer.

Security

• No issues. This only affects error typing/attribution, not wire behavior. Interpolating c.name into the error message is the same pattern already used for RPC parameter names, so no new injection-style surface.

Test coverage

• Good coverage of the new behavior: TYPE_INFO failure, length-prefix failure, and data-generator failure are each covered in test/unit/bulk-load-test.ts, plus the row-source-closing case for a COLMETADATA failure. The integration test (test/integration/bulk-load-test.ts) was correctly updated to assert on the new InputError/cause shape instead of the raw RangeError.
• One gap, likely low-risk given the PR's stated scope: there's no unit test asserting the row-source-closes-on-failure behavior specifically for a row value serialization error (only for the COLMETADATA case). The pre-existing hands nothing downstream after a row fails, and closes the row source test already covers "a row failure closes the source" in general, so this is arguably already covered and just not re-verified with the new InputError wrapper.

Note on behavior change

The PR body is upfront that this is a deliberate change: bulk load callbacks that used to receive a raw RangeError (etc.) now receive an InputError with the original error as cause. Worth confirming the release process (semantic-release / commit type) reflects this as more than a patch-level fix:, since a consumer doing err instanceof RangeError on a bulk load callback would now silently stop matching, needing to unwrap cause instead.

Nice work overall - scoping this down from the original combined RPC+bulk-load PR into just the bulk-load half (with the RPC half already merged via #1774) keeps it reviewable, and the commit history shows the error-wrapping was iterated on carefully (narrowing what's caught, handling the non-generator-function edge case, etc.).

[claude]

Review

Reviewed the diff (src/bulk-load-payload.ts, src/bulk-load.ts, src/tedious.ts, and the two test files). This is a well-scoped, focused change and I think it's solid.

Correctness

• Moving buffer.writeBuffer(bulkLoad.getColMetaData()) inside the try in bulk-load-payload.ts is the key fix here: previously a writeTypeInfo failure during COLMETADATA generation threw before entering the try block, so the row source was never closed via iterator.return(). Now it goes through the same close-on-failure path as a row-value error. Good catch, and the new unit test (closes the row source when the column metadata cannot be written) verifies it directly.
• The wrapping is correctly scoped to just the type's writeTypeInfo/writeValue calls, not the surrounding buffer plumbing (TableName/ColName writes, row-token writes, etc.), so a RangeError from, say, tBuf.writeUsVarchar on an oversized table/column name won't be misattributed to the column's data. That matches the RPC precedent in rpcrequest-payload.ts.
• Confirmed c.type.validate(...) in the row loop is intentionally left outside the wrapping (as called out in the PR description) — it's still inside the outer try, so it still closes the row source on failure, it's just not re-typed as InputError. Consistent with the "validation vs. serialization are different failure modes" reasoning.
• No double-wrapping risk: getColMetaData() already wraps its own writeTypeInfo error into InputError in bulk-load.ts; the outer catch in bulk-load-payload.ts just propagates it (throw err) rather than re-wrapping.
• Verified against src/data-types/decimal.ts that the integration test's DECIMAL range check is thrown from generateParameterData (i.e., reached via writeValue), so updating that test's expectation from a raw RangeError to a wrapped InputError (with the original as cause) is accurate.

Test coverage

Good breadth: separate unit tests cover errors from generateTypeInfo (COLMETADATA), generateParameterLength, and generateParameterData (the three sub-steps writeTypeInfo/writeValue can fail at), plus two tests asserting the row source is actually closed (return() observed) for both the metadata and row-value failure cases. The integration test update is consistent with the new behavior described above.

Minor / non-blocking observations

• Message wording differs between the RPC path (Input parameter '<name>' could not be validated, in src/rpcrequest-payload.ts, already on master) and this PR's bulk load path (Column '<name>' could not be serialized), even though both wrap the same writeTypeInfo/writeValue failure. This is out of scope for this PR (per the description, the RPC half landed separately in refactor: resolve parameters once and serialize them through a write contract #1774), but it means code catching InputError and pattern-matching on .message will see different phrasing depending on which path produced it. Worth a follow-up if a consistent message across both paths is desired — not a blocker.
• InputError extends TypeError {} with no added fields — fine, matches the minimal style of ConnectionError/RequestError, and there's no test asserting tedious.ts exports InputError (there's also none for ConnectionError/RequestError, so this is consistent with existing convention rather than a gap introduced here).

Security

No concerns — this only changes error attribution/wrapping, not wire behavior or data handling. Error messages interpolate column names, which come from caller-supplied schema definitions, not untrusted external input.

Overall: clean, well-tested, appropriately narrow fix. Nice work tracing through the async-iterator close semantics.