[SEA-NodeJS] (2/8) napi-rs binding consumer — TS loader + build script

[msrathore-db]

Stack

Linear stack of 8 PRs landing the M0 + M1 Phase 1 SEA NodeJS work. Merge in order from base ↑ to tip. The tip branch (msrathore/sea-auth-u2m, PR #383) is the single snapshot containing everything in flight — point your test or benchmark harness at it for an end-to-end check.

Pos	PR	Branch	Scope
1/8	#378	sea-abstraction	IBackend / ISessionBackend / IOperationBackend interfaces
2/8	#380	sea-napi-binding	TS loader + build script for the kernel-provided .node artifact
3/8	#377	sea-errors-logging	Kernel ErrorCode → JS error-class mapping (M0 minimum)
4/8	#379	sea-auth	PAT auth via useSEA: true
5/8	#382	sea-execution	executeStatement + openSession (sessionConfig, initialCatalog/Schema)
6/8	#381	sea-results	CloudFetch + Inline Arrow result fetching
7/8	#384	sea-operation	cancel / close / finished lifecycle + INTERVAL parity + napi-relocation acceptance (absorbed sea-integration content)
8/8	#383	sea-auth-u2m ← TIP	M1 Phase 1 OAuth M2M + U2M (5 review rounds, ZERO HIGH at close)

Companion kernel stack (databricks/databricks-sql-kernel): 8 PRs — root #26 (async-public-api) → #27 → #25 → #29 → #28 → #30 → #24 → #23 (tip).

Policy: new PRs always stack on the current tip. No sibling/parallel topology. No force-pushes on existing PRs unless absolutely necessary; if a PR's content is wrong, add a fix-up commit on top of the stack tip rather than rewriting history.

---

This PR is position 2/8.

Summary

Thin TypeScript loader for the napi-rs binding (SeaNativeLoader), generated native/sea/index.d.ts, and the build:native npm script that compiles against the kernel workspace's napi/ sub-crate.

Companion kernel PR

The Rust napi crate itself lives in the kernel repo — see databricks/databricks-sql-kernel#25 (Database / Connection / Statement / ResultStream classes) and its skeleton predecessor #27.

Downstream fixes / reviewer note

• The native package-router/package-name production-load issue is fixed separately in databricks-sql-kernel#93. This loader PR remains the TypeScript consumer/bootstrap layer.
• The larger native-source relocation cleanup is absorbed later in [SEA-NodeJS] (7/8) Operation lifecycle — cancel / close / finished + INTERVAL parity + napi relocation #384, so reviewers do not need to request that cleanup in this lower PR.

Test plan

• ✅ npm run build:native produces native/sea/index.linux-x64-gnu.node
• ✅ Loader correctly require()s the platform-specific binary

Draft until you give the go for review.

Tracking

• PECOBLR-2886 — Multi-platform native packaging + napi-workspace-relocate
• Parent epic: PECOBLR-2666

Co-authored-by: Isaac

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[vikrantpuppala]

High — release-engineering landmine that crashes every consumer the moment PR 3/8+ wires SEA in.

Verified at HEAD 548a14b8:

• git ls-tree native/sea/ shows ONLY README.md and index.d.ts — no index.js.
• .gitignore:17 ignores native/sea/index.js (it's regenerated by npm run build:native).
• .npmignore:10 allow-lists !native/sea/index.js to ship in the tarball.
• package.json scripts has no prepack or prepublishOnly hook that runs build:native.
• The loader at lib/sea/SeaNativeLoader.ts:82 does require('../../native/sea') — Node's resolver looks for index.js in that directory.

Result: any npm publish from a workspace that didn't manually run npm run build:native first ships a tarball missing the napi-rs router. Consumers get MODULE_NOT_FOUND with a misleading hint that tells them to install the optional dep (which is already installed) instead of pointing at the missing router shim.

PR #380 doesn't crash anything today because nothing calls getSeaNative(). The moment PR 3/8 wires SeaBackend.connect() to the loader, every install bricks.

• Suggested fix: either (a) commit native/sea/index.js (analogous to the committed index.d.ts — the file is small and stable), or (b) add a prepack script that runs build:native gated on DATABRICKS_RELEASE_BUILD=1, with an explicit [ -f native/sea/index.js ] || (echo "Missing native router; run npm run build:native first" && exit 1) assertion that fails the pack if it's still missing.

Found by: security, ops, maintainability, devils-advocate (4 reviewers).

---

_{Posted by code-review-squad · /full-review}

[msrathore-db]

Addressed in 3216019 (C1). Committed the napi-rs router native/sea/index.js (un-ignored, analogous to the committed index.d.ts) and added a prepack assertion that hard-fails the pack if the router is missing:

"prepack": "test -f native/sea/index.js || { echo 'ERROR: native/sea/index.js (napi-rs router) is missing …' >&2; exit 1; }"

Companion kernel fix databricks-sql-kernel#93 corrects the base package name so the router’s require paths resolve. A publish can no longer ship a tarball missing the router.

[vikrantpuppala]

Medium — published declaration files reference an unresolvable module specifier.

Verified: tsconfig.build.json extends tsconfig.json, so the paths: { "@sea-native": ["./native/sea/index.d.ts"] } mapping propagates into the production build. TypeScript does NOT rewrite path-aliased specifiers in emitted declarations.

When this loader is re-exported from the public surface (which is the goal in PR 3/8+), every downstream TS consumer of @databricks/sql will see import type { ... } from '@sea-native' in the .d.ts and fail type-check with "Cannot find module '@sea-native'".

Today it doesn't break anything because (a) only import type is used in lib/sea/SeaNativeLoader.ts:26 (erased at runtime) and (b) the loader isn't re-exported from lib/index.ts. But the bug is already in the published surface waiting to be triggered.

• Suggested fix: Drop the paths mapping and use a relative import type { … } from '../../native/sea'. The alias is used by exactly one file — it buys nothing.

Found by: architecture, language, agent-compat, devils-advocate (4 reviewers).

---

_{Posted by code-review-squad · /full-review}

[msrathore-db]

Addressed in 3216019 (C2). Dropped the @sea-native paths alias entirely (0 sea-native refs left in tsconfig.json); SeaNativeLoader.ts uses a relative import(../../native/sea). No unresolvable specifier leaks into the emitted .d.ts.

[vikrantpuppala]

Medium — every customer who hits this error is sent on a wild-goose chase.

Verified: line 55 builds platform = '${process.platform}-${process.arch}'. On linux/x64 that produces linux-x64; on darwin/arm64 it produces darwin-arm64. The error message then says:

Install the matching optional dependency (e.g. @databricks/sea-native-linux-x64).

But the actual published package (per package.json:95) is @databricks/sea-native-linux-x64-**gnu**. The hint's recommended npm install will 404.

Same issue cross-platform:

• linux/x64 → suggests @databricks/sea-native-linux-x64; actual is -linux-x64-gnu or -linux-x64-musl

• linux/arm64 → suggests @databricks/sea-native-linux-arm64; actual is -linux-arm64-gnu

• win32/x64 → suggests @databricks/sea-native-win32-x64; actual is -win32-x64-msvc

• Suggested fix: either compute the napi-rs triple correctly (mirror the router's platform→triple table — easier said than done) or drop the package-name example: "Install the matching @databricks/sea-native-* optional dependency for your platform; see the README for the supported triple list."

Found by: ops, agent-compat.

---

_{Posted by code-review-squad · /full-review}

[msrathore-db]

Addressed in 3216019 (C3). The error hint no longer names a specific (404-ing) package — it points at the README’s supported-triple list instead (// 404. Point at the READMEs supported-triple list instead.).

[vikrantpuppala]

Medium — by far the highest-consensus finding; 7 reviewers flagged it.

Only @databricks/sea-native-linux-x64-gnu: 0.1.0 is in optionalDependencies. macOS-arm64, macOS-x64, linux-arm64-gnu, linux-x64-musl (Alpine), win32-x64-msvc, etc. all have zero installable binding.

Three coupled consequences:

1. Silent degradation — on every non-linux-x64-gnu platform, the loader will hit MODULE_NOT_FOUND. Users may not realize they're stuck on Thrift.
2. Misleading hint (see M2) — the loader tells them to install a package that doesn't exist.
3. Supply-chain squat risk — the unlisted names (@databricks/sea-native-darwin-arm64, …-darwin-x64, etc.) are not on the npm registry. Confirm @databricks org scope is locked to Databricks publishers, OR pre-register empty placeholder packages now, so an external party can't typosquat @databricks/sea-native-darwin-arm64 before Databricks does and have it autoload via the napi-rs router for darwin-arm64 consumers.

• Suggested fix: Decide between two coherent models:
  • (a) Intentional M0 scope — explicitly document "M0 = linux-x64-gnu only; other platforms continue using Thrift exclusively" in the README and update the loader's error message to match.
  • (b) Multi-platform support — list all planned triples in optionalDependencies (npm tolerates missing optionalDeps on the registry — they skip silently with a warning).

Whichever you pick, address the supply-chain squat question separately (lock the scope or pre-register placeholders).

Found by: security, ops, devils-advocate, performance, architecture, agent-compat, language (7 reviewers).

---

_{Posted by code-review-squad · /full-review}

[msrathore-db]

Addressed in 3216019 (C4) via model (a) — intentional M0 scope. native/sea/README.md now has a “Supported platforms (M0)” section documenting that M0 publishes a single triple (linux-x64-gnu); other platforms stay on Thrift. Added an npm scope-lock / placeholder note for the supply-chain-squat concern. Multi-platform (model b) is post-M0.

[vikrantpuppala]

Medium — single-source-of-truth violation.

Lines 39-44 declare:

export interface SeaNativeBinding {
  version(): string;
  openSession(options: ConnectionOptions): Promise<NativeConnection>;
  Connection: typeof NativeConnection;
  Statement: typeof NativeStatement;
}

This is a manual subset of what native/sea/index.d.ts already declares. When the kernel adds a new free function (e.g. cancelStatement, future async APIs), the .d.ts regenerates but this interface stays stale until someone notices. The unchecked cast at line 82 (require('../../native/sea') as SeaNativeBinding) hides the drift.

• Suggested fix: derive the type from the generated module instead of redeclaring it:

import type * as SeaNative from '../../native/sea';
export type SeaNativeBinding = typeof SeaNative;

Then the cast at line 82 stays automatically correct, and the README's claim that "the .d.ts is the contract" is enforced by the type system.

Found by: architecture.

---

_{Posted by code-review-squad · /full-review}

[msrathore-db]

Addressed in 3216019 (C5). SeaNativeBinding is now derived from the generated module — export type SeaNativeBinding = typeof import(../../native/sea) — so it can’t drift from the .d.ts, and the defaultRequire() cast stays correct automatically.

[vikrantpuppala]

Medium — turns every SeaBackend unit test into an integration test once SEA is wired in.

cached and cachedError are module-level let bindings (lines 65-66). getSeaNative() / tryGetSeaNative() are free functions that close over them. There's no constructor, no DI seam, no exported reset hook.

Consequence for PR 3/8: when SeaBackend.connect() stops throwing NOT_IMPLEMENTED and starts calling getSeaNative(), any unit test that touches SeaBackend will require a real .node to be loadable on the test machine — otherwise it skips. sinon.stub(SeaNativeLoader, 'getSeaNative') works once per file but the cached value persists across files in the same mocha run. Order-dependence is baked in.

The lz4 pattern this is "mirroring" gets away with module-state because the existing codebase wraps it (e.g., Lz4Util and adapters at the call sites accept the resolved module). Here SeaNativeLoader IS the wrapper, and the wrapper has no seam.

• Suggested fix: expose the loader as a class with an injectable seam:

export class SeaNativeLoader {
  constructor(private readonly load: () => SeaNativeBinding = defaultRequire) {}
  get(): SeaNativeBinding { /* ... */ }
  tryGet(): SeaNativeBinding | undefined { /* ... */ }
}

Then SeaBackend takes loader: SeaNativeLoader via constructor (matches the existing IClientContext pattern in lib/DBSQLClient.ts). Keep getSeaNative() as a thin convenience over a process-global default instance.

Much cheaper to change now (0 callers) than after PRs 3–8 land.

Found by: architecture, devils-advocate, maintainability adjacent (3 reviewers).

---

_{Posted by code-review-squad · /full-review}

[msrathore-db]

Addressed in 3216019 (C6). SeaNativeLoader is now a class with an injectable load seam (constructor(load = defaultRequire)); getSeaNative()/tryGetSeaNative() are thin wrappers over a process-global default instance. The new tests/unit/sea/loader.test.ts exercises it via the injected seam with no real .node required, so wiring SEA into SeaBackend won’t turn its unit tests into integration tests.

[vikrantpuppala]

Medium — name collisions will surface as IDE autocomplete ambiguity and silent field-shape bugs once SEA is wired in.

Lines 34-37 re-export:

export type { ConnectionOptions, ExecuteOptions, ArrowBatch, ArrowSchema };
export type Connection = NativeConnection;
export type Statement = NativeStatement;

Collisions with existing public types:

• ConnectionOptions (SEA: hostName/httpPath/token) vs ConnectionOptions from lib/contracts/IDBSQLClient.ts (Thrift: host/path/token + AuthOptions union). The token field is shared → silent shape-mismatch bugs when an IDE auto-imports the wrong one.
• ArrowBatch (SEA: {ipcBytes: Buffer}) vs ArrowBatch from lib/result/utils.ts (Thrift: {batches: Buffer[]; rowCount: number}).

With 0 callers today this is harmless. The moment SEA is wired into DBSQLClient and these types are imported anywhere outside lib/sea/, the bare names become ambiguous in autocomplete and in code review.

• Suggested fix: rename the SEA-side exports now, before any production consumer imports them:

export type { ConnectionOptions as SeaConnectionOptions, ExecuteOptions as SeaExecuteOptions, ArrowBatch as SeaArrowBatch, ArrowSchema as SeaArrowSchema };
export type SeaConnection = NativeConnection;
export type SeaStatement = NativeStatement;

The kernel-generated .d.ts can keep the napi-rs default names (it's the kernel's contract). The TS-side wrapper is where disambiguation should happen, and doing it before any consumer imports the bare names is a one-way door.

Found by: agent-compat, architecture, devils-advocate, language, maintainability (5 reviewers).

---

_{Posted by code-review-squad · /full-review}

[msrathore-db]

Addressed in 3216019 (C7). The TS-side re-exports are renamed SeaConnectionOptions / SeaExecuteOptions / SeaArrowBatch / SeaArrowSchema / SeaConnection / SeaStatement. The kernel-generated .d.ts keeps the napi-rs default names. Done now, before any production consumer imports the bare names.

[vikrantpuppala]

Medium — coverage gap that masks broken-binding regressions.

Two coupled problems:

1. tests/unit/sea/version.test.ts:20-30 — silent skip when tryGetSeaNative() returns undefined. No CI escalation. On a linux-x64-gnu CI runner where the optional dep should install, a silently broken @databricks/sea-native-linux-x64-gnu would produce a green build. Compare to tests/e2e/sea/e2e-smoke.test.ts:55-62, which DOES fail loud in CI when env vars are missing — but applies the same logic only to env vars, not to the binding.

2. Line 33 — the only assertion is expect(binding.version()).to.match(/^\d+\.\d+\.\d+$/). No shape-check on binding.openSession, binding.Connection, binding.Statement. If the kernel renames any of these, the test still passes — green.

• Suggested fixes:
  • Mirror the CI fail-loud logic from the e2e test: on a runner whose triple is supposed to have a published .node (e.g., process.env.CI === 'true' and process.platform === 'linux' && process.arch === 'x64'), treat binding === undefined as a hard failure.
  • Add shape checks inside tryLoad() (lib/sea/SeaNativeLoader.ts:82 — verify typeof binding.openSession === 'function', binding.Connection exists, binding.Statement exists) so kernel-side shape drift is caught.
  • Add a tests/unit/sea/loader.test.ts with proxyquire covering the loadFailureHint branches (MODULE_NOT_FOUND, ERR_DLOPEN_FAILED, generic), the Node-version gate, and caching. Pure logic tests; run everywhere regardless of binding availability.

Found by: test, devils-advocate, ops, agent-compat.

---

_{Posted by code-review-squad · /full-review}

[msrathore-db]

Addressed in 3216019 (C8). version.test.ts now fails loud on a linux-x64 CI runner (where the optional dep should install) instead of silently skipping, and adds shape checks for openSession/Connection/Statement. New tests/unit/sea/loader.test.ts covers the hint branches, the Node-version gate, the shape check, and caching — all via the injected seam, so they run everywhere regardless of binding availability.

[vikrantpuppala]

Low — defensive guard works backwards.

if (Number.isFinite(nodeMajor) && nodeMajor < MIN_NODE_MAJOR) evaluates to false when nodeMajor === NaN, so the load proceeds. The intent reads "fail closed if we can't tell what Node we're on" but the code does the opposite.

In practice process.version is always parseable, so the practical risk is near-zero. But it's the kind of guard that future runtime quirks will surface.

Suggested fix:

if (!Number.isFinite(nodeMajor) || nodeMajor < MIN_NODE_MAJOR) {
  // ...
}

---

_{Posted by code-review-squad · /full-review}

[msrathore-db]

Fixed in 3216019 (C9). The guard now fails closed: if (!Number.isFinite(nodeMajor) || nodeMajor < MIN_NODE_MAJOR) — NaN (unparseable version) now also blocks the load.

[vikrantpuppala]

Low — agent-friendliness / signal-to-noise.

The docstring on Connection.close discusses tracing::EnteredSpan, !Send futures, napi-rs's execute_tokio_future glue rejects non-Send futures, etc. This is excellent for a Rust reviewer but counterproductive for JS consumers — an LLM agent answering "why doesn't connection.close() await?" will paste the Rust paragraph into a JS issue/Slack where it has no context.

When the kernel fixes this (via cloned span or similar), the .d.ts is regenerated and the now-stale Rust rationale silently disappears — any agent that scraped it for caching is now serving wrong advice.

• Suggested fix: in the Rust source, move the kernel-internal reasoning into a Rust comment (// SAFETY: style) and keep the #[napi] JSDoc terse and behavior-focused:

Returns immediately after marking the wrapper closed. The server-side close runs in the background; failures are not surfaced to JS. Use Statement.close() (which awaits) if you need to observe close errors per-statement.

Same lens applies to the class-level docs on Connection and Statement — Arc<Mutex<Option<...>>>, result_stream_mut, ExecutedStatementHandle are all Rust-side concerns the JS audience doesn't need.

---

_{Posted by code-review-squad · /full-review}

[msrathore-db]

Partially addressed (C10). index.d.ts was regenerated from merged kernel main, which terser’d most of the Rust-internal prose. The remaining Rust phrasing lives in this generated file, so the real fix is to terse the #[napi] JSDoc in the kernel Rust source rather than hand-edit here — raising that on the kernel side. Not blocking M0.

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).