refactor: resolve parameter lengths for all types that can resolve one

[arthurschreiber]

Problem

RPC request and bulk load parameter serialization only resolve a parameter's length when the type's id matches the legacy variable-length type id bit pattern:

if ((type.id & 0x30) === 0x20) {

Validated against [MS-TDS] v20260617 s2.2.5.4.2 (Variable-Length Data Types): every BYTELEN/USHORTLEN/LONGLEN type id defined up to TDS 7.1 does match that pattern (0x2X/0x6X/0xAX/0xEX — e.g. BIGVARBINARYTYPE 0xA5, NVARCHARTYPE 0xE7, TEXTTYPE 0x23), but the type ids introduced in TDS 7.2 and later do not: UDTTYPE 0xF0, XMLTYPE 0xF1, JSONTYPE 0xF4, and VECTORTYPE 0xF5 all have (id & 0x30) === 0x30.

For a modern-id type that has a length, the length is therefore silently never resolved, and its generateTypeInfo computes with undefined — concretely, Buffer.writeUInt16LE(NaN) writes a zero max length into the TYPE_INFO, which the server rejects with an opaque protocol error (Data type 0xF5 has an invalid data length or metadata length). This was discovered while implementing the SQL Server 2025 vector type (VECTORTYPE 0xF5, the first modern-id type with a length), where it broke RPC parameters and bulk load columns; the json type (JSONTYPE 0xF4) would hit the same landmine.

Change

Resolve lengths based on whether the type can resolve one (type.resolveLength), mirroring how precision and scale are already resolved in the same functions, in both src/rpcrequest-payload.ts and src/bulk-load.ts.

This is a refactor, not a fix, for anything tedious ships today: no existing type is affected. Every type currently implementing resolveLength (Binary, Char, Image, NChar, NText, NVarChar, Text, UniqueIdentifier, VarBinary, VarChar) has a legacy-pattern id, and the three shipped modern-id types (TVP, UDT, Xml) have no resolveLength, so the new condition is false for them exactly as the old one was. It removes the gate ahead of the first type that would trip over it.

Verified two ways:

• No type without resolveLength reads a resolved length (the mask-matching N-types, date/time types, and sql_variant never consume param.length).
• The full unit suite plus the parameter-related integration suites (parameterised statements, RPC, bulk load, prepare/execute, TVP — 215 tests) pass unchanged against SQL Server 2022.

New unit tests cover length resolution for a modern-id type, explicit-length precedence, and the unchanged legacy-type byte output.

#1774 carries the same change in its resolveParameter and BulkLoad.addColumn; whichever lands first, the other's merge is trivial.

🤖 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-01T17:49:25.879270Z	30b7e9c	PR opened

ℹ️ 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, well-scoped bug fix. Summary of what I checked:

Correctness

• Confirmed via MS-TDS type ids in src/data-types/*.ts that the legacy bit-pattern check (type.id & 0x30) === 0x20 indeed excludes TDS 7.2+ ids — udt.ts (0xF0) and xml.ts (0xF1) already exist in-tree and don't implement resolveLength, so they're unaffected by this change either way, consistent with the PR's "behavior-preserving" claim.
• Verified every current type implementing resolveLength (binary, char, image, nchar, ntext, nvarchar, text, uniqueidentifier, varbinary, varchar) has a legacy-pattern id, so no currently-shipped type's behavior changes — only currently-nonexistent/future modern-id types with a length (like the upcoming vector/json types) benefit.
• Searched for other (id & 0x30)-style checks elsewhere in src/ (token parsing, metadata parsing, etc.) — found none, so this fix is complete for the current codebase; good due diligence already reflected in the PR description.
• The fix mirrors the existing, unconditional resolvePrecision/resolveScale handling immediately below each change, so the code is now internally consistent — nice catch.

Style

• The explanatory comments in both files are appropriately used here: they document a non-obvious reason (why the check isn't keyed off the type id) rather than restating what the code does, matching common WHY-not-WHAT comment conventions.

Test coverage

• New unit tests in test/unit/rpcrequest-payload-test.ts cover: a stub modern-id type resolving its length, explicit length taking precedence over resolveLength, and unchanged byte output for a legacy VarBinary parameter. These are well-targeted regression tests for exactly the bug being fixed.
• One gap: src/bulk-load.ts has an equivalent code change but no corresponding new/updated unit test exercising addColumn with a modern-id, length-resolving type. Given rpcrequest-payload and bulk-load now share the same logic and could drift independently in the future, a small parallel test (or a shared test comparing both call sites) would guard against regressions in the bulk-load path specifically. Not blocking, since the logic is trivial and mirrors the tested RPC path, but worth considering.

Minor observation (pre-existing, not introduced by this PR)

• rpcrequest-payload.ts checks if (parameter.length) (truthy) while bulk-load.ts checks if (column.length == null) (nullish) before falling back to resolveLength. That means an explicit length: 0 is treated differently between the two call sites (RPC would fall through to resolveLength, bulk load would keep 0). This inconsistency predates this PR and isn't introduced by it, but since the PR is already touching both call sites for the same reasoning, it might be worth a follow-up ticket if 0-length parameters are a real scenario.

No performance or security concerns — this is a narrow protocol-serialization fix with no new I/O, allocation patterns, or attacker-controlled input handling beyond what already existed.

Nice work tracking this down against the MS-TDS spec and validating against the full integration suite.

[codecov]

Codecov Report

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1771      +/-   ##
==========================================
- Coverage   81.02%   81.01%   -0.01%     
==========================================
  Files          92       92              
  Lines        4948     4946       -2     
  Branches      938      936       -2     
==========================================
- Hits         4009     4007       -2     
  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]

Reviewed the diff. This is a clean, well-scoped refactor.

Correctness
The core change is sound: gating length resolution on type.resolveLength existing, rather than on the legacy (type.id & 0x30) === 0x20 bit-pattern check, is the right fix. I independently verified the claim in the PR description — every type currently implementing resolveLength (binary, char, image, nchar, ntext, nvarchar, text, uniqueidentifier, varbinary, varchar) has a legacy-pattern id, and the shipped modern-id types (tvp, udt, xml) don't implement resolveLength. So the new condition is a strict superset of the old one and is behavior-preserving for every type tedious ships today, while unblocking vector/json-style types with modern ids. The new code also now mirrors the existing precision/scale resolution pattern in both files almost verbatim, which is a nice consistency win.

Test coverage
The new rpcrequest-payload-test.ts suite is solid — it covers the modern-id resolution case, explicit-length precedence over resolveLength, and a regression check that legacy varbinary byte output is unchanged. However, src/bulk-load.ts gets the identical fix but no corresponding test in bulk-load-test.ts. Since the whole point of this PR is that the old gate was silently wrong for a class of types, it'd be good to add an analogous unit test for BulkLoad.addColumn/getColMetaData with a stub modern-id type, mirroring the three cases already covered for RPC. Right now bulk-load's fix is only exercised indirectly (via the "215 integration tests pass unchanged" claim in the description), not verified with a positive test the way RPC is.

Minor

• The explanatory comments duplicated in both files are helpful for future readers who might be tempted to reintroduce the bit-mask check — good call keeping them.
• No security or performance concerns; this only affects TYPE_INFO length metadata construction, not data handling.

Nice catch overall — this closes a real landmine ahead of the vector/json type work.

[github-actions]

🎉 This PR is included in version 20.3.0 🎉

The release is available on:

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

Your semantic-release bot 📦🚀