Skip to content

Commit ba14f62

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

1 file changed

Lines changed: 12 additions & 0 deletions

File tree

.claude/knowledge/learning-log.md

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,3 +43,15 @@ initial dated section below.
4343
**Rule:** Normalize every user-supplied string option forwarded to the native kernel binding with `isBlankOrReserved` (not a bare truthy/`|| undefined` check) so malformed placeholder/whitespace values collapse to the intended default instead of selecting the wrong mode with an opaque native error.
4444
- **Context:** PR #501 exposed `enableTokenFederation`/`federationClientId` on the kernel `static-token` path; review established the kernel has no `enableTokenFederation` flag and always performs a (conditional, same-issuer/non-JWT-skipping) token exchange, whereas Thrift gates federation on the boolean via `FederationProvider` and falls back to the original token on failure.
4545
**Rule:** Kernel and Thrift diverge on token federation — the kernel ignores `enableTokenFederation` (federation is always on) and selects account-wide vs SP-wide WIF solely by `federationClientId` presence; when adding kernel options that mirror Thrift auth flags, verify whether the native binding honors the flag and document any "ignored/always-on" divergence in the docstring and CONNECTION_PARAMETERS.md.
46+
47+
### 2026-08-22: learnings since 2026-08-21T17:33:40Z
48+
- **Context:** In PR #505 (kernel Azure Entra auth), the new `AzureSpM2m` branch `return`ed before the shared `persistence` rejection guard, silently dropping a caller's `persistence` hook that the U2M and workspace-OIDC M2M arms reject — reviewers flagged it as an inconsistent-contract footgun.
49+
**Rule:** When adding a new early-returning auth/config branch in `KernelAuth`, replicate every cross-cutting guard (e.g. `persistence` rejection for refresh-token-less M2M flows) that downstream code would otherwise apply — an early return silently skips shared validation and diverges from the documented contract.
50+
- **Context:** In PR #505 the Azure SP M2M branch initially validated `oauthClientId`/`oauthClientSecret`/`azureTenantId` with bare `=== undefined` checks, so present-but-degenerate values (`''`, whitespace, and the reserved `'undefined'`/`'null'` shell-export artifacts from env-var expansion) passed through and surfaced opaque downstream errors (Entra `invalid_client`, malformed AAD discovery URLs).
51+
**Rule:** For forwarded credential/tenant/secret fields on kernel auth paths that have no Thrift byte-for-byte parity contract, validate with `isBlankOrReserved` (not just `!== undefined`); a blank env-expanded string is as unusable as a missing one and should be rejected up front or treated as omitted so auto-discovery kicks in.
52+
- **Context:** In PR #505, `isAzureHost` claimed to normalize hosts but only stripped scheme/path — a reviewer caught that `https://adb-….azuredatabricks.net:443` or a padded string would fail Azure suffix detection and misroute Azure OAuth down the non-Azure path.
53+
**Rule:** Before matching a host against a domain-suffix set, fully normalize it — `.trim()`, lowercase, strip either scheme, drop the path, and strip an explicit `:port` — otherwise ports, whitespace, or full URLs cause suffix matching to silently fail.
54+
- **Context:** In PR #505 the kernel's Azure-vs-in-house OAuth routing (`isAzureHost && useDatabricksOAuthInAzure !== true`) was deliberately built to mirror the Thrift driver's `OAuthManager.getManager`, keeping `AZURE_HOST_SUFFIXES` in lockstep with Thrift's domain lists; dropping the host guard would misclassify plain AWS/GCP OAuth connections as Entra-direct.
55+
**Rule:** Kernel auth flow-selection must mirror the Thrift `OAuthManager.getManager` domain lists and flag semantics; keep the Azure host-suffix set in lockstep and document any intentional divergence (e.g. accepting `.databricks.azure.us` in modes where Thrift throws) inline.
56+
- **Context:** In PR #505 `KERNEL_REV` was pinned to the tip of an unmerged, stacked kernel branch (#280 on #263) that carried the `AzureSpM2m` native surface, while published `@databricks/databricks-sql-kernel-*` npm pins still lacked it — a reviewer flagged that end users would hit native-side rejection and the pin would break on rebase/force-push/delete.
57+
**Rule:** Do not merge a connector PR while `KERNEL_REV` points at an unmerged kernel branch tip; gate the merge behind the kernel branch merge+release, then re-point `KERNEL_REV` to a `main` SHA and bump the npm kernel pins so published users get the required native surface.

0 commit comments

Comments
 (0)