fix: 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.

Fix

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.

Behavior-preserving for all existing types, verified two ways:

• Every type currently implementing resolveLength (Binary, Char, Image, NChar, NText, NVarChar, Text, UniqueIdentifier, VarBinary, VarChar) has a legacy-pattern id, and 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.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Fxv5h4UMCGJEpjgCKcAxug

---

Generated by Claude Code

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