fix(sea): address PR review comments + fix failing unit tests

Failing unit test (TS2737): ArrowResultConverter.test.ts used BigInt
literals (`123n`), which don't compile at the tsconfig ES2018 target the
unit-test job typechecks against. Replaced with `BigInt('123')` calls
(the BigInt function is available at ES2018; only the literal syntax is
gated).

Review comments:

- C1 (rich-status propagation untested): the kernel-statement fakes
  returned null for all four rich-status accessors, so the propagation
  through `op.status()` was never exercised with a real value.
  Parameterized the fakes and added a sync-DML test asserting
  numModifiedRows / displayMessage / diagnosticInfo / errorDetailsJson
  all surface, plus an all-null SELECT case. (Verified live: the SEA
  REST server delivers DML counts as a `num_affected_rows` result
  column rather than on the status envelope, so the kernel accessor
  returns null against current SEA warehouses by design; Thrift on the
  same warehouse surfaces Int64(4) on status. The wiring is correct and
  will surface values when a server populates the status field.)

- C2 (synthesizeThriftStatus rich fields untested): added tests for the
  Int64 re-boxing of numModifiedRows (including 0 vs absent), the
  null -> undefined mapping, and the string passthrough fields.

- C3 (header injection): customHeaders values/names were forwarded
  verbatim; CR/LF/NUL enable HTTP header injection and the kernel only
  rejects them at connect time with an opaque error. Added early
  validation in buildSeaHttpOptions throwing a clear HiveDriverError
  that names the offending header (validated before the reserved-name
  drop). Verified live against pecotesting.

- C4 (dead conjunct): simplified `if (this.asyncStatement &&
  !this.cancellableExecution)` to `if (this.asyncStatement)` — the
  constructor already guarantees the handle kinds are mutually
  exclusive.

- C5 (redundant FFI reads): memoized readRichStatusFields so a
  re-status() of a terminal operation reuses the read instead of
  re-hitting the four kernel accessors. Covered by the C1 read-count
  assertion.

Co-authored-by: Isaac

Author: msrathore-db

lib/sea/SeaAuth.ts

@@ -359,12 +359,35 @@ export function buildSeaTlsOptions(options: ConnectionOptions): SeaTlsOptions {
  */
 const KERNEL_MANAGED_HEADERS = new Set(['authorization', 'x-databricks-org-id']);
 
+// CR / LF / NUL in a header name or value enable request-splitting / header
+// injection. The kernel's HTTP client (reqwest) does reject these, but only at
+// connect time and with an opaque "Failed to construct HTTP client:
+// InvalidArgument: failed to parse header value" error that names neither the
+// offending header nor the cause. Reject them here, before the FFI hop, with a
+// clear error so a caller gets actionable signal at the point they set the
+// header (verified against pecotesting: the kernel otherwise surfaces the
+// opaque construction error).
+const FORBIDDEN_HEADER_CHARS = /[\r\n\0]/;
+
+function validateHeaderToken(kind: 'name' | 'value', headerName: string, token: string): void {
+  if (FORBIDDEN_HEADER_CHARS.test(token)) {
+    throw new HiveDriverError(
+      `SEA backend: customHeaders ${kind} for \`${headerName}\` contains a forbidden control character ` +
+        `(CR, LF, or NUL). Such characters enable HTTP header injection and are rejected.`,
+    );
+  }
+}
+
 export function buildSeaHttpOptions(options: ConnectionOptions): SeaHttpOptions {
   const { customHeaders, userAgentEntry } = options;
 
   const headers: Array<{ name: string; value: string }> = [];
   if (customHeaders) {
     for (const [name, value] of Object.entries(customHeaders)) {
+      // Reject CR/LF/NUL in either the name or the value before forwarding —
+      // a clear, early error instead of the kernel's opaque connect-time throw.
+      validateHeaderToken('name', name, name);
+      validateHeaderToken('value', name, value);
       // Drop kernel-managed reserved names before the FFI hop — same
       // double-wall as the Python connector's `_KERNEL_MANAGED_HEADERS`.
       if (KERNEL_MANAGED_HEADERS.has(name.toLowerCase())) {

lib/sea/SeaOperationBackend.ts

@@ -192,6 +192,13 @@ export default class SeaOperationBackend implements IOperationBackend {
   // sync-execute path once `cancellableExecution.result()` settles.
   private blockingStatement?: SeaOperationStatement;
 
+  // Memoized rich-status read. `readRichStatusFields()` is only ever invoked
+  // once the operation is terminal (the Succeeded branches of `status()` and
+  // the `seaFinished` progress callback), and the kernel's terminal response is
+  // immutable, so the FFI accessors are read exactly once and the result
+  // reused — re-`status()`-ing a completed operation is then free.
+  private richStatusFieldsPromise?: Promise<SeaRichStatusFields>;
+
   // The cancel/close surface — whichever handle backs this operation. Both
   // `AsyncStatement` and `Statement` expose `cancel()` / `close()`; the
   // sync-execute path uses a composite that routes `cancel()` to the
@@ -474,7 +481,14 @@ export default class SeaOperationBackend implements IOperationBackend {
    * status-field read must not turn a successful operation's status query into
    * a throw. The fields are best-effort metadata, not the operation outcome.
    */
-  private async readRichStatusFields(): Promise<SeaRichStatusFields> {
+  private readRichStatusFields(): Promise<SeaRichStatusFields> {
+    if (!this.richStatusFieldsPromise) {
+      this.richStatusFieldsPromise = this.computeRichStatusFields();
+    }
+    return this.richStatusFieldsPromise;
+  }
+
+  private async computeRichStatusFields(): Promise<SeaRichStatusFields> {
     const empty: SeaRichStatusFields = {
       numModifiedRows: null,
       displayMessage: null,
@@ -483,8 +497,10 @@ export default class SeaOperationBackend implements IOperationBackend {
     };
 
     // The async path never produces a terminal sync `Statement`, so there is
-    // nothing to read these off of.
-    if (this.asyncStatement && !this.cancellableExecution) {
+    // nothing to read these off of. (The constructor guarantees exactly one of
+    // `asyncStatement` / `statement` / `cancellableExecution`, so `asyncStatement`
+    // being set already implies `cancellableExecution` is undefined.)
+    if (this.asyncStatement) {
       return empty;
     }
 

tests/unit/result/ArrowResultConverter.test.ts

@@ -215,7 +215,7 @@ describe('ArrowResultConverter', () => {
   it('preserves BIGINT precision as bigint when preserveBigNumericPrecision is set', async () => {
     // 9007199254740993 = Number.MAX_SAFE_INTEGER + 2 — not exactly
     // representable as a JS number.
-    const table = tableFromArrays({ big_value: BigInt64Array.from([9007199254740993n, 5n]) });
+    const table = tableFromArrays({ big_value: BigInt64Array.from([BigInt('9007199254740993'), BigInt('5')]) });
     const rowSetProvider = new ResultsProviderStub(
       [{ batches: [createSampleArrowBatch(table.batches[0])], rowCount: 2 }],
       emptyItem,
@@ -227,13 +227,13 @@ describe('ArrowResultConverter', () => {
       { preserveBigNumericPrecision: true },
     );
     expect(await result.fetchNext({ limit: 10000 })).to.deep.equal([
-      { big_value: 9007199254740993n },
-      { big_value: 5n },
+      { big_value: BigInt('9007199254740993') },
+      { big_value: BigInt('5') },
     ]);
   });
 
   it('narrows BIGINT to a (lossy) number by default — preserves the Thrift contract', async () => {
-    const table = tableFromArrays({ big_value: BigInt64Array.from([9007199254740993n, 5n]) });
+    const table = tableFromArrays({ big_value: BigInt64Array.from([BigInt('9007199254740993'), BigInt('5')]) });
     const rowSetProvider = new ResultsProviderStub(
       [{ batches: [createSampleArrowBatch(table.batches[0])], rowCount: 2 }],
       emptyItem,
@@ -246,10 +246,10 @@ describe('ArrowResultConverter', () => {
   });
 
   it('formats unscaled decimals to exact strings (bigNumDecimalToString)', () => {
-    expect(bigNumDecimalToString(1234567890n, 5)).to.equal('12345.67890'); // trailing zero kept
-    expect(bigNumDecimalToString(-1234567890123456789n, 4)).to.equal('-123456789012345.6789');
-    expect(bigNumDecimalToString(5n, 2)).to.equal('0.05'); // leading zero synthesized
-    expect(bigNumDecimalToString(-5n, 2)).to.equal('-0.05');
-    expect(bigNumDecimalToString(12345n, 0)).to.equal('12345'); // scale 0 → integer string
+    expect(bigNumDecimalToString(BigInt('1234567890'), 5)).to.equal('12345.67890'); // trailing zero kept
+    expect(bigNumDecimalToString(BigInt('-1234567890123456789'), 4)).to.equal('-123456789012345.6789');
+    expect(bigNumDecimalToString(BigInt('5'), 2)).to.equal('0.05'); // leading zero synthesized
+    expect(bigNumDecimalToString(BigInt('-5'), 2)).to.equal('-0.05');
+    expect(bigNumDecimalToString(BigInt('12345'), 0)).to.equal('12345'); // scale 0 → integer string
   });
 });

tests/unit/sea/connectionOptions.test.ts

@@ -273,6 +273,39 @@ describe('SeaAuth HTTP options (buildSeaHttpOptions)', () => {
     expect(native.customHeaders?.find((h) => h.name === 'X-Trace')?.value).to.equal('abc');
     expect(native.customHeaders?.find((h) => h.name === 'User-Agent')?.value).to.contain('MyApp/2.0');
   });
+
+  describe('rejects header-injection control characters (CR / LF / NUL)', () => {
+    // The kernel HTTP client does reject these, but only at connect time with an
+    // opaque "Failed to construct HTTP client: InvalidArgument" error (verified
+    // against pecotesting). We reject earlier, naming the offending header.
+    const injections: Array<[string, Record<string, string>]> = [
+      ['CRLF in value', { 'X-Evil': 'ok\r\nInjected-Header: pwned' }],
+      ['bare LF in value', { 'X-Evil': 'a\nb' }],
+      ['bare CR in value', { 'X-Evil': 'a\rb' }],
+      ['NUL in value', { 'X-Evil': 'a\0b' }],
+      ['CRLF in name', { 'X-Ev\r\nil': 'v' }],
+      ['NUL in name', { 'X-Ev\0il': 'v' }],
+    ];
+    for (const [label, customHeaders] of injections) {
+      it(`throws HiveDriverError on ${label}`, () => {
+        expect(() => buildSeaHttpOptions(opts({ customHeaders }))).to.throw(HiveDriverError, /forbidden control character/);
+      });
+    }
+
+    it('does not throw on a valid header containing spaces, tabs, and punctuation', () => {
+      expect(() =>
+        buildSeaHttpOptions(opts({ customHeaders: { 'X-Ok': 'Bearer abc.def-123; q=0.9\tfoo' } })),
+      ).to.not.throw();
+    });
+
+    it('validates a reserved header before dropping it (injection via Authorization is still rejected)', () => {
+      // Reserved-name drop must not let a CR/LF-laced reserved header slip past
+      // validation — validate first, then drop.
+      expect(() =>
+        buildSeaHttpOptions(opts({ customHeaders: { Authorization: 'Bearer x\r\nInjected: 1' } })),
+      ).to.throw(HiveDriverError, /forbidden control character/);
+    });
+  });
 });
 
 describe('SeaAuth retry options — buildSeaRetryOptions', () => {

tests/unit/sea/execution.test.ts

@@ -59,24 +59,50 @@ class FakeNativeStatement implements SeaStatement {
     this.closed = true;
   }
 
-  // Status accessors added by the kernel's status-fields surface.
+  // Status accessors added by the kernel's status-fields surface. The values
+  // are configurable so a test can assert non-null rich-status (e.g. a DML
+  // `numModifiedRows`) propagates through `op.status()`; they default to all-
+  // null, matching a SELECT / metadata statement that carries none.
+  public rich: SeaRichStatusValues = {
+    numModifiedRows: null,
+    displayMessage: null,
+    diagnosticInfo: null,
+    errorDetailsJson: null,
+  };
+
+  // Counts every rich-field accessor call so a test can assert the backend
+  // memoizes the read on a terminal statement (re-`status()` must not re-hit
+  // the FFI accessors).
+  public richReads = 0;
+
   public async numModifiedRows(): Promise<number | null> {
-    return null;
+    this.richReads += 1;
+    return this.rich.numModifiedRows;
   }
 
   public async displayMessage(): Promise<string | null> {
-    return null;
+    this.richReads += 1;
+    return this.rich.displayMessage;
   }
 
   public async diagnosticInfo(): Promise<string | null> {
-    return null;
+    this.richReads += 1;
+    return this.rich.diagnosticInfo;
   }
 
   public async errorDetailsJson(): Promise<string | null> {
-    return null;
+    this.richReads += 1;
+    return this.rich.errorDetailsJson;
   }
 }
 
+interface SeaRichStatusValues {
+  numModifiedRows: number | null;
+  displayMessage: string | null;
+  diagnosticInfo: string | null;
+  errorDetailsJson: string | null;
+}
+
 /**
  * Fake `AsyncStatement` (the `submitStatement` return). `status()` reports a
  * configurable state (default Succeeded); `awaitResult()` yields a fetch handle
@@ -223,6 +249,10 @@ class FakeNativeConnection implements SeaConnection {
     return this.statementToReturn;
   }
 
+  // Rich status the next sync-execute's terminal Statement should report
+  // (e.g. a DML `numModifiedRows`). Defaults to all-null (a SELECT).
+  public richStatus?: SeaRichStatusValues;
+
   // Sync (`runAsync: false`, the DEFAULT) query path: records sql + options and
   // returns a pending CancellableExecution whose result() drives the execute.
   public async executeStatementCancellable(sql: string, options?: unknown): Promise<any> {
@@ -231,7 +261,11 @@ class FakeNativeConnection implements SeaConnection {
     }
     this.lastSql = sql;
     this.lastOptions = options;
-    this.lastCancellableExecution = new FakeCancellableExecution();
+    const resultHandle = new FakeNativeStatement();
+    if (this.richStatus) {
+      resultHandle.rich = this.richStatus;
+    }
+    this.lastCancellableExecution = new FakeCancellableExecution(resultHandle);
     return this.lastCancellableExecution;
   }
 
@@ -1148,4 +1182,45 @@ describe('SeaOperationBackend — sync (executeStatementCancellable) path', () =
     expect(status.isSuccess).to.equal(true);
     expect(exec.resultHandle.closed).to.equal(false);
   });
+
+  it('surfaces the kernel rich-status fields (numModifiedRows etc.) through op.status() and memoizes the read', async () => {
+    // A DML statement's terminal kernel `Statement` carries numModifiedRows /
+    // displayMessage / diagnosticInfo / errorDetailsJson. Drive the sync execute
+    // to terminal and assert each non-null field propagates through op.status()
+    // (previously the fakes returned all-null, so this propagation was untested).
+    const resultHandle = new FakeNativeStatement();
+    resultHandle.rich = {
+      numModifiedRows: 42,
+      displayMessage: 'INSERT 0 42',
+      diagnosticInfo: 'stage 1/1 finished',
+      errorDetailsJson: '{"detail":"none"}',
+    };
+    const exec = new FakeCancellableExecution(resultHandle);
+    const op = makeSyncOp(exec);
+    await op.waitUntilReady();
+
+    const status = await op.status(false);
+    expect(status.state).to.equal(OperationState.Succeeded);
+    expect(status.numModifiedRows).to.equal(42);
+    expect(status.displayMessage).to.equal('INSERT 0 42');
+    expect(status.diagnosticInfo).to.equal('stage 1/1 finished');
+    expect(status.errorDetailsJson).to.equal('{"detail":"none"}');
+
+    // C5: re-status()-ing a completed op reuses the memoized read — the four FFI
+    // accessors fire exactly once across both status() calls (4 reads, not 8).
+    await op.status(false);
+    expect(resultHandle.richReads).to.equal(4);
+  });
+
+  it('reports all-null rich-status for a SELECT (no rows modified) — the default', async () => {
+    // A read-only statement carries no numModifiedRows; the backend surfaces
+    // null rather than fabricating a value.
+    const op = makeSyncOp(new FakeCancellableExecution());
+    await op.waitUntilReady();
+    const status = await op.status(false);
+    expect(status.numModifiedRows).to.equal(null);
+    expect(status.displayMessage).to.equal(null);
+    expect(status.diagnosticInfo).to.equal(null);
+    expect(status.errorDetailsJson).to.equal(null);
+  });
 });

tests/unit/thrift-backend/wireSynthesis.test.ts

@@ -1,4 +1,5 @@
 import { expect } from 'chai';
+import Int64 from 'node-int64';
 import { TOperationState, TSparkRowSetType, TStatusCode } from '../../../thrift/TCLIService_types';
 import { OperationState, OperationStatus } from '../../../lib/contracts/OperationStatus';
 import { ResultFormat, ResultMetadata } from '../../../lib/contracts/ResultMetadata';
@@ -68,6 +69,54 @@ describe('wireSynthesis', () => {
       expect(resp.errorMessage).to.equal('should-not-elevate-to-error-but-still-passed-through');
       expect(resp.sqlState).to.equal('01000');
     });
+
+    describe('rich status fields (numModifiedRows / displayMessage / diagnosticInfo / errorDetailsJson)', () => {
+      it('re-boxes numModifiedRows as a Thrift Int64 (matching the Thrift deserializer wire shape)', () => {
+        const resp = synthesizeThriftStatus({ ...baseStatus, numModifiedRows: 5 });
+        expect(resp.numModifiedRows, 'should be a node-int64 Int64').to.be.instanceOf(Int64);
+        expect((resp.numModifiedRows as Int64).toNumber()).to.equal(5);
+      });
+
+      it('re-boxes numModifiedRows: 0 as Int64(0) — a real zero-row DML result, not "absent"', () => {
+        const resp = synthesizeThriftStatus({ ...baseStatus, numModifiedRows: 0 });
+        expect(resp.numModifiedRows, '0 is a value, not a missing field').to.be.instanceOf(Int64);
+        expect((resp.numModifiedRows as Int64).toNumber()).to.equal(0);
+      });
+
+      it('maps a null numModifiedRows (server did not supply) to undefined, matching the Thrift path', () => {
+        const resp = synthesizeThriftStatus({ ...baseStatus, numModifiedRows: null });
+        expect(resp.numModifiedRows).to.equal(undefined);
+      });
+
+      it('maps an absent numModifiedRows to undefined', () => {
+        const resp = synthesizeThriftStatus({ ...baseStatus });
+        expect(resp.numModifiedRows).to.equal(undefined);
+      });
+
+      it('passes displayMessage / diagnosticInfo / errorDetailsJson through as strings', () => {
+        const resp = synthesizeThriftStatus({
+          ...baseStatus,
+          displayMessage: 'INSERT 0 5',
+          diagnosticInfo: 'stage 1/1 finished',
+          errorDetailsJson: '{"detail":"none"}',
+        });
+        expect(resp.displayMessage).to.equal('INSERT 0 5');
+        expect(resp.diagnosticInfo).to.equal('stage 1/1 finished');
+        expect(resp.errorDetailsJson).to.equal('{"detail":"none"}');
+      });
+
+      it('maps null string fields to undefined (absent, not the literal null)', () => {
+        const resp = synthesizeThriftStatus({
+          ...baseStatus,
+          displayMessage: null,
+          diagnosticInfo: null,
+          errorDetailsJson: null,
+        });
+        expect(resp.displayMessage).to.equal(undefined);
+        expect(resp.diagnosticInfo).to.equal(undefined);
+        expect(resp.errorDetailsJson).to.equal(undefined);
+      });
+    });
   });
 
   describe('synthesizeThriftResultSetMetadata', () => {