fix(sea): address review on #409 (packaging, auth-flow docs, context)

- P0 packaging: the napi router's per-platform npm names were garbled — the
  M0 triple was baked into the prefix for every platform
  (@databricks/sea-native-linux-x64-gnu-<triple>, and the doubled
  ...-gnu-linux-x64-gnu). Corrected to the canonical @databricks/sql-kernel-<triple>
  (matches the loader hint + native/sea/README). Added native-packaging.test
  to lock the naming. The per-platform packages are unpublished, so they remain
  undeclared in optionalDependencies (npm ci can't resolve an unpublished pin);
  README documents the M0 build:native load path.
- Moved @napi-rs/cli out of optionalDependencies (a build tool should not land
  in consumer installs); build:native now fetches it on demand via npx --yes.
- P1 auth-flow: documented honestly that SEA's OAuth flow selection DIVERGES
  from Thrift — Thrift keys off the secret (DBSQLClient.ts:216), SEA keys off
  oauthClientId presence — including the two real gaps (id+no-secret throws;
  U2M has no custom client id). Fixed the stale DBSQLClient.ts:143 ref.
- P1 stale ref: SeaErrorMapping pointed at DBSQLOperation.ts:209; the Canceled
  switch is now ~:374. Updated.
- P2 context: SeaBackendOptions.context is now required (was an `as IClientContext`
  downcast of undefined → latent NPE). Tests pass a shared makeFakeContext().
- P2 prependSlash: dropped the dead lib/utils/prependSlash.ts (nothing imported
  it; SeaAuth and DBSQLClient each keep their own inline copy).

Co-authored-by: Isaac
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>

Author: msrathore-db

lib/sea/SeaAuth.ts

@@ -128,11 +128,23 @@ export function isBlankOrReserved(s: string): boolean {
  *   - OAuth U2M: `authType: 'databricks-oauth'` + NO `oauthClientId` and
  *     NO `oauthClientSecret`. Kernel runs the PKCE auth-code dance (opens
  *     a browser, listens on localhost:8030, exchanges the code, persists
- *     to `~/.config/databricks-sql-kernel/oauth/{sha256}.json`). The flow
- *     selector keys off `oauthClientId` presence: present → M2M, absent →
- *     U2M. (Round-4 NF3-2 fix; previously secret-keyed — that variant
- *     routed a typo'd-secret M2M call to the U2M arm and swallowed the
- *     actionable error.) Mirrors thrift's intent at `DBSQLClient.ts:143`.
+ *     to `~/.config/databricks-sql-kernel/oauth/{sha256}.json`).
+ *
+ *     **Flow selection — DELIBERATE DIVERGENCE FROM THRIFT.** Thrift's
+ *     `DBSQLClient.createAuthProvider` (`DBSQLClient.ts:216`) keys off the
+ *     *secret* (`oauthClientSecret === undefined ? U2M : M2M`), so a custom
+ *     `oauthClientId` with no secret runs U2M with that id. SEA instead keys
+ *     off `oauthClientId` *presence* (id present → M2M, absent → U2M). The
+ *     trade-off: keying off the id means a caller who set an id but
+ *     typoed/forgot the secret gets the actionable M2M "secret is required"
+ *     error instead of being silently routed to U2M (which would hide their
+ *     intent). The cost is two real behavioural gaps vs Thrift:
+ *       1. `oauthClientId` + no secret → Thrift runs U2M; SEA throws
+ *          `AuthenticationError` (M2M secret required).
+ *       2. SEA U2M has NO custom-client-id support — the kernel hardcodes
+ *          `client_id = "databricks-cli"`, and SEA rejects any `oauthClientId`
+ *          on the U2M arm. Thrift U2M honours a custom `clientId`.
+ *     Both are documented limitations of the M0 SEA OAuth surface, not bugs.
  *
  * Out of scope on the OAuth paths (rejected with a clear error):
  *   - `azureTenantId` / `useDatabricksOAuthInAzure` → Microsoft Entra
@@ -204,13 +216,15 @@ export function buildSeaConnectionOptions(options: ConnectionOptions): SeaNative
       );
     }
 
-    // Flow selector mirrors thrift's `DBSQLClient.createAuthProvider`
-    // (`DBSQLClient.ts:143`): presence of `oauthClientId` indicates M2M
-    // intent, otherwise U2M. Routing decision is based on `oauthClientId`
-    // (the "do I have an id?" signal) rather than the secret, so a
-    // user who set an id but typoed/forgot the secret gets the M2M
-    // "secret is required" error instead of a U2M error that hides
-    // their actual intent. The U2M arm still defends against an id
+    // Flow selector — DELIBERATELY DIFFERENT from thrift's
+    // `DBSQLClient.createAuthProvider` (`DBSQLClient.ts:216`), which keys off
+    // the secret (`oauthClientSecret === undefined ? U2M : M2M`). SEA keys off
+    // `oauthClientId` *presence* (the "do I have an id?" signal) instead, so a
+    // user who set an id but typoed/forgot the secret gets the actionable M2M
+    // "secret is required" error rather than being silently routed to U2M
+    // (which would hide their intent). Cost: `id + no secret` throws here
+    // where thrift would run U2M, and SEA U2M has no custom-client-id support
+    // (see buildSeaConnectionOptions header). The U2M arm still defends against an id
     // sneaking through: fires only when `oauthClientId` is provided as
     // a blank-reserved literal (e.g., whitespace, `"null"`, `"undefined"`)
     // alongside an absent/blank secret — both `idIsBlank` and

lib/sea/SeaBackend.ts

@@ -24,13 +24,13 @@ import SeaSessionBackend from './SeaSessionBackend';
 
 export interface SeaBackendOptions {
   /**
-   * Optional in the type so unit tests that only exercise the auth-
-   * routing surface (which doesn't touch context) can pass
-   * `{ nativeBinding }`. The constructor downcasts undefined to
-   * `IClientContext` because runtime callers from `DBSQLClient` always
-   * supply one — see `lib/DBSQLClient.ts` SEA seam.
+   * Required. Provides the logger + config the SEA session/operation chain
+   * logs through. `DBSQLClient` supplies it via the SEA seam
+   * (`new SeaBackend({ context: this })`); unit tests pass a stub. Kept
+   * mandatory (rather than an `as IClientContext` downcast of `undefined`)
+   * so a missing context is a compile error, not a latent runtime NPE.
    */
-  context?: IClientContext;
+  context: IClientContext;
   /**
    * Optional injection seam for unit tests. When provided, replaces the
    * default `getSeaNative()` call so tests can swap in a mock napi
@@ -68,9 +68,9 @@ export default class SeaBackend implements IBackend {
 
   private nativeOptions?: SeaNativeConnectionOptions;
 
-  constructor(options?: SeaBackendOptions) {
-    this.context = options?.context as IClientContext;
-    this.binding = options?.nativeBinding ?? getSeaNative();
+  constructor(options: SeaBackendOptions) {
+    this.context = options.context;
+    this.binding = options.nativeBinding ?? getSeaNative();
   }
 
   public async connect(options: ConnectionOptions): Promise<void> {

lib/sea/SeaErrorMapping.ts

@@ -57,8 +57,8 @@ export type KernelErrorCode =
  *
  * `errorCode` is namespaced under `kernelMetadata` rather than placed at
  * the top level because `OperationStateError` already declares a top-level
- * `errorCode: enum` field, and `DBSQLOperation.ts:209` switches on it
- * (`err.errorCode === OperationStateErrorCode.Canceled`). Top-level
+ * `errorCode: enum` field, and `DBSQLOperation.ts` switches on it
+ * (`err.errorCode === OperationStateErrorCode.Canceled`, around `:374`). Top-level
  * defineProperty would clobber that enum with a kernel string and break
  * cancel/close detection.
  */
@@ -210,7 +210,7 @@ function buildKernelMetadata(parsed: Record<string, unknown>): KernelMetadata {
  *    envelope fields under a single non-enumerable `kernelMetadata`
  *    namespace. Namespacing avoids the collision with
  *    `OperationStateError.errorCode` (an enum already switched on at the
- *    JS layer — see `DBSQLOperation.ts:209`).
+ *    JS layer — see `DBSQLOperation.ts` around `:374`).
  *  - Binding-side error (e.g. `napi::Error::new(InvalidArg, "openSession:
  *    \`token\` is required for the requested auth mode")` produced by
  *    the binding's own validation): returned unchanged. These don't

lib/utils/prependSlash.ts

@@ -57,17 +57,24 @@ nodejs repo.
 
 At release time the kernel's CI publishes
 `@databricks/sql-kernel-<triple>` npm packages — one per supported
-platform — each containing a single `.node` binary. The nodejs
-driver lists them as `optionalDependencies`; npm installs only the
-one matching the consumer's `process.platform` / `process.arch`.
-`native/sea/index.js` (the napi-rs router) then `require()`s the
-installed package at load time.
+platform — each containing a single `.node` binary. `native/sea/index.js`
+(the napi-rs router) `require()`s the package matching the consumer's
+`process.platform` / `process.arch` at load time.
+
+> **M0 status:** these per-platform packages are **not yet published**, so
+> they are intentionally **not** declared in the driver's
+> `optionalDependencies`. (npm refuses an `npm ci` against a pinned
+> dependency it cannot resolve from the registry, so declaring an
+> unpublished package would break every install.) Until they ship, the
+> binding is produced locally via `npm run build:native` (which copies
+> `index.<triple>.node` into this directory). Once the packages are
+> published, add `@databricks/sql-kernel-<triple>` back to
+> `optionalDependencies` — npm then installs only the matching one.
 
 ## Supported platforms (M0)
 
-M0 publishes a **single** triple: **`linux-x64-gnu`** (package
-`@databricks/sql-kernel-linux-x64-gnu`). It is the only entry in the
-driver's `optionalDependencies`.
+M0 targets a **single** triple: **`linux-x64-gnu`** (package
+`@databricks/sql-kernel-linux-x64-gnu`, once published).
 
 On every other platform (macOS, Windows, linux-arm64, linux-x64-musl
 / Alpine, …) the SEA binding is simply absent: `SeaNativeLoader`

native/sea/README.md

@@ -17,7 +17,7 @@
     "test": "nyc --report-dir=${NYC_REPORT_DIR:-coverage_unit} mocha --config tests/unit/.mocharc.js",
     "update-version": "node bin/update-version.js && prettier --write ./lib/version.ts",
     "build": "npm run update-version && tsc --project tsconfig.build.json",
-    "build:native": "bash -c 'cd ${DATABRICKS_SQL_KERNEL_REPO:-../../databricks-sql-kernel}/napi && npx --no-install @napi-rs/cli build --platform ${BUILD_PROFILE:---release} && cp index.* $OLDPWD/native/sea/'",
+    "build:native": "bash -c 'cd ${DATABRICKS_SQL_KERNEL_REPO:-../../databricks-sql-kernel}/napi && npx --yes @napi-rs/cli@2.18.4 build --platform ${BUILD_PROFILE:---release} && cp index.* $OLDPWD/native/sea/'",
     "prepack": "test -f native/sea/index.js || { echo 'ERROR: native/sea/index.js (napi-rs router) is missing — the published tarball would fail to load SEA. It is committed to git; run `npm run build:native` if you removed it.' >&2; exit 1; }",
     "watch": "tsc --project tsconfig.build.json --watch",
     "type-check": "tsc --noEmit",
@@ -91,7 +91,6 @@
     "winston": "^3.8.2"
   },
   "optionalDependencies": {
-    "lz4": "^0.6.5",
-    "@napi-rs/cli": "2.18.4"
+    "lz4": "^0.6.5"
   }
 }