From d8d2b3c42af034115797f53335e715197b0bd97b Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Thu, 13 Aug 2026 17:45:12 +0000 Subject: [PATCH 1/4] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning-log.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/.claude/knowledge/learning-log.md b/.claude/knowledge/learning-log.md index b65b6b34..98a815f7 100644 --- a/.claude/knowledge/learning-log.md +++ b/.claude/knowledge/learning-log.md @@ -10,3 +10,20 @@ This file is the shared knowledge log for the databricks-sql-nodejs engineer-bot No learnings have been recorded yet — the first retrospective PR will append the initial dated section below. +## Entries + +### 2026-08-13: learnings since 2026-08-12T17:43:56Z +- **Context:** In PR #499 the daily learning workflow declared a `workflow_dispatch` input named `window-hours` and referenced it as `${{ inputs.window-hours }}`; reviewers noted the hyphen is parsed as subtraction (`inputs.window - hours`), yielding an empty value. + **Rule:** In GitHub Actions `${{ }}` expressions, reference hyphenated input/context names with bracket notation (`inputs['window-hours']`) or rename them to use underscores — a bare hyphen is parsed as the minus operator, not part of the identifier. +- **Context:** PR #499's workflow comment documents that a `type: number` `workflow_dispatch` input fails the whole run at startup ("workflow file issue") when the workflow also has a `schedule` trigger; it was declared `type: string` and coerced to int downstream instead. + **Rule:** Declare `workflow_dispatch` inputs as `type: string` (and coerce downstream) when the same workflow also has a `schedule` trigger — mixing `type: number` inputs with a schedule trigger fails the run at startup. +- **Context:** In PR #499 reviewers found that pointing `retrospective.system_prompt` at a non-existent `prompts/retrospective_system.md` would hard-fail every scheduled run; the fix was to drop the key entirely so the engine used its built-in base prompt. + **Rule:** For databricks-bot-engine config, a set-but-missing prompt-file key (e.g. `system_prompt`) is a hard error; leave the key UNSET to fall back to the engine's built-in base prompt rather than pointing it at a path that may not exist. +- **Context:** PR #499's workflow sets `MODEL_ENDPOINT` to `.../serving-endpoints//invocations`; the comment warns that using `.../serving-endpoints/anthropic/invocations` hits `translate_endpoint`'s already-v2 early return, so the CLI appends `/v1/messages` to `.../anthropic/invocations` and gets HTTP 400. + **Rule:** For these databricks bot workflows, set `MODEL_ENDPOINT` to the concrete `.../serving-endpoints//invocations` form (which `translate_endpoint` strips to the `anthropic` base); do NOT pass `.../serving-endpoints/anthropic/invocations` — it survives translation and produces an unsupported `/anthropic/invocations/v1/messages` path (HTTP 400). +- **Context:** In PR #499 engineer-bot could not apply a valid workflow fix because `.github/` is a denied/non-writable path for its tools (read_file and edit_file returned "Path denied or invalid"), so the finding had to be flagged for a human. + **Rule:** engineer-bot's edit tools cannot touch `.github/` paths — review findings on workflow/action files must be routed to a human; don't expect the bot to self-apply or verify changes under `.github/`. +- **Context:** Across PR #499's review threads, engineer-bot repeatedly reported fixes as pushed (commits c45c237, 5514996) that did not appear at head, and both bots re-read stale snapshots, causing repeated churn on already-resolved threads. + **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. +- **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`). + **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. From 431d8a825b54e5aa46cd97ee5d2759a139bfa94e Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Tue, 18 Aug 2026 17:35:55 +0000 Subject: [PATCH 2/4] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning-log.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.claude/knowledge/learning-log.md b/.claude/knowledge/learning-log.md index 98a815f7..f4874f92 100644 --- a/.claude/knowledge/learning-log.md +++ b/.claude/knowledge/learning-log.md @@ -27,3 +27,11 @@ initial dated section below. **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. - **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`). **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. + +### 2026-08-18: learnings since 2026-08-17T17:33:39Z +- **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). + **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`). +- **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. + **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. +- **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. + **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`. From 762b80baa38e87a2f1d265c5663ce68635ebd4fc Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 21 Aug 2026 17:35:22 +0000 Subject: [PATCH 3/4] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning-log.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.claude/knowledge/learning-log.md b/.claude/knowledge/learning-log.md index f4874f92..b96aaf0a 100644 --- a/.claude/knowledge/learning-log.md +++ b/.claude/knowledge/learning-log.md @@ -35,3 +35,11 @@ initial dated section below. **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. - **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. **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`. + +### 2026-08-21: learnings since 2026-08-20T17:33:58Z +- **Context:** PR #501 added a `static-token` auth branch to `buildKernelConnectionOptions` (lib/kernel/KernelAuth.ts); reviewers repeatedly flagged that it guarded against conflicting `oauthClientId`/`oauthClientSecret` but not a stray `token`, unlike the sibling `access-token`/`databricks-oauth` branches. + **Rule:** When adding a new auth-mode branch in KernelAuth, mirror the sibling branches' "pick one auth mode" ambiguity guards — reject every credential field that belongs to the other modes (`token`, `oauthClientId`, `oauthClientSecret`) so a misconfigured caller gets a clear error instead of a silently-dropped field. +- **Context:** In PR #501 the kernel `static-token` branch forwarded `federationClientId` with a bare truthy check (`federationClientId || undefined`); a reviewer noted values like `'undefined'`, `'null'`, or whitespace (common from unset shell vars) are truthy and leak through as a literal client id, while sibling fields use the module's `isBlankOrReserved` predicate. + **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. +- **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. + **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. From ba14f627db138846dc8908487cd8ef6f042cc9be Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Sat, 22 Aug 2026 17:33:10 +0000 Subject: [PATCH 4/4] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning-log.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/.claude/knowledge/learning-log.md b/.claude/knowledge/learning-log.md index b96aaf0a..d7021b1a 100644 --- a/.claude/knowledge/learning-log.md +++ b/.claude/knowledge/learning-log.md @@ -43,3 +43,15 @@ initial dated section below. **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. - **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. **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. + +### 2026-08-22: learnings since 2026-08-21T17:33:40Z +- **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. + **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. +- **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). + **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. +- **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. + **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. +- **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. + **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. +- **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. + **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.