Skip to content

Commit 431d8a8

Browse files
learn: retrospective learnings
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
1 parent d8d2b3c commit 431d8a8

1 file changed

Lines changed: 8 additions & 0 deletions

File tree

.claude/knowledge/learning-log.md

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,3 +27,11 @@ initial dated section below.
2727
**Rule:** Before claiming a fix has landed, re-fetch and confirm the change is actually present at the branch head — a local edit or a push that reverted/failed can produce false "pushed" claims and wasted re-work on the same thread.
2828
- **Context:** PR #497's `bin/build-native.sh` runs under `set -euo pipefail` and expands a possibly-empty bash array as `"${arr[@]}"`; reviewers noted this raises `unbound variable` and aborts on bash 4.3 and earlier (macOS still ships bash 3.2 at `/usr/bin/bash` under `#!/usr/bin/env bash`).
2929
**Rule:** Under `set -u`, expanding an empty array as `"${arr[@]}"` aborts on bash < 4.4 (incl. stock macOS bash 3.2); guard the expansion (e.g. `${arr[@]+"${arr[@]}"}` or a length check) for portable scripts. Also note `${VAR-default}` substitutes only when unset while `${VAR:-default}` also covers set-but-empty.
30+
31+
### 2026-08-18: learnings since 2026-08-17T17:33:39Z
32+
- **Context:** In PR #457 (connection-parameter reference), reviewers repeatedly caught the doc claiming a Thrift option was "honored" when the wiring silently drops it: `oauthScopes` (never threaded into `DatabricksOAuth` by `createAuthProvider`, so it always falls back to `defaultOAuthScopes`), `noProxy` (not mapped in `getConnectionOptions`, and `createProxyAgent` installs `getProxyForUrl: () => proxyUrl` with no bypass list), and `customHeaders` (only applied to driver-owned out-of-band telemetry/feature-flag requests, not the primary query transport). Conversely, TLS/mTLS options were wrongly marked kernel-only when `getConnectionOptions` does map `customCaCert``ca` (additive), `clientCert``cert`, `clientKey``key` and sets `rejectUnauthorized: options.checkServerCertificate ?? true` (secure-by-default).
33+
**Rule:** Never infer per-backend option support from the `ConnectionOptions` type declaration; trace each option end-to-end through `getConnectionOptions`/`createAuthProvider` into the transport — the Thrift path both silently drops declared options (oauthScopes, noProxy, customHeaders on the query transport) and fully honors others assumed kernel-only (TLS/mTLS, secure-by-default via `?? true`).
34+
- **Context:** In PR #457, the `KernelBackend.ts` class docstring stated OAuth routing "keys off `oauthClientId` presence" and that the kernel throws an M2M "secret required" error, but the actual `KernelAuth.ts` adapter (`buildKernelConnectionOptions`) keys the U2M/M2M flow off `oauthClientSecret === undefined` (mirroring Thrift exactly) and forwards a custom `oauthClientId` verbatim on the U2M arm with no throw.
35+
**Rule:** When documenting or reasoning about kernel auth flow selection, verify against the `KernelAuth.ts` adapter implementation, not class docstrings/comments — the docstrings in this repo can contradict the code they describe.
36+
- **Context:** In PR #457, a gap note attributed the metric-view conf (`spark.sql.thriftserver.metadata.metricview.enabled`) auto-injection to `ThriftBackend.ts` as "Thrift-only." In fact `DBSQLClient.openSession` injects it into `request.configuration` before dispatching to either backend, so `KernelBackend.openSession` also receives and folds it into `sessionConf`; the `ThriftBackend.ts` injection is a redundant second one. Any kernel-side gap comes from the kernel session-conf allowlist filtering the key, not from a missing injection.
37+
**Rule:** Shared request preparation in `DBSQLClient.openSession` runs for both backends before dispatch, so behavior seen on only one backend is often pre-dispatch injection plus downstream allowlist filtering — check `DBSQLClient` before attributing a divergence to a single backend's `openSession`.

0 commit comments

Comments
 (0)