From 77bfd522cc046918ab6a6be1a444f73aa0e2fd04 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Thu, 13 Aug 2026 17:43:12 +0000 Subject: [PATCH 01/14] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning-log.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/.claude/knowledge/learning-log.md b/.claude/knowledge/learning-log.md index 988c85f4..6ff53d3c 100644 --- a/.claude/knowledge/learning-log.md +++ b/.claude/knowledge/learning-log.md @@ -10,3 +10,14 @@ configured in `.bot/config.yaml`: No learnings have been recorded yet. Dated sections are appended below by the retrospective flow. +## Entries + +### 2026-08-13: learnings since 2026-08-12T17:42:15Z +- **Context:** PR #442 configured a bot's `MODEL_ENDPOINT`; using the generic `.../serving-endpoints/anthropic/invocations` form 400'd every scheduled run (`Unsupported native API path .../anthropic/invocations/v1/messages`), confirmed empirically against the sibling databricks-sql-python cron. + **Rule:** Point Databricks bot `MODEL_ENDPOINT` at the concrete `.../serving-endpoints//invocations` form, never `.../serving-endpoints/anthropic/invocations` — `translate_endpoint` early-returns on URLs already containing `/serving-endpoints/anthropic`, leaving the `/invocations` suffix so the CLI appends `/v1/messages` and hits HTTP 400. +- **Context:** PR #442's learning workflow declared a `workflow_dispatch` input as a string (`window-hours`) with a comment noting that `type: number` breaks the run. + **Rule:** In GitHub Actions, a `workflow_dispatch` input declared `type: number` fails the whole run at startup ("workflow file issue") when the workflow also has a `schedule` trigger — declare numeric dispatch inputs as `type: string` and coerce to int downstream (e.g. via argparse). +- **Context:** PR #442 initially set `retrospective.system_prompt: prompts/retrospective_system.md`, a file that did not exist; the maintainer confirmed the engine treats a set-but-missing prompt path as a hard error, which would have failed the daily cron every run. + **Rule:** For engine config keys that reference a file (e.g. `system_prompt`): an UNSET key falls back to the engine's built-in default, but a SET key pointing at a missing file is a hard error — omit the key entirely rather than point it at a nonexistent path. +- **Context:** PR #442 needed the retrospective flow to commit `.claude/knowledge/learning-log.md`, but the repo `.gitignore` ignored all of `.claude`; the fix scoped the ignore (`.claude/*` + `!.claude/knowledge/` + `.claude/knowledge/*` + `!.claude/knowledge/learning-log.md`) and seeded the file so the author read path never hits a missing file. + **Rule:** When a bot/tool must commit a file under a normally-ignored directory, add scoped `.gitignore` negation for exactly that file (ignoring intermediate dirs still hides children, so re-include each level) and seed the file, so both the write (commit) and read paths resolve. From 7c711e70e9e8ba90f0ec1b52236e79ea332f1445 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 21 Aug 2026 17:33:48 +0000 Subject: [PATCH 02/14] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning-log.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.claude/knowledge/learning-log.md b/.claude/knowledge/learning-log.md index 6ff53d3c..8cf434d5 100644 --- a/.claude/knowledge/learning-log.md +++ b/.claude/knowledge/learning-log.md @@ -21,3 +21,7 @@ retrospective flow. **Rule:** For engine config keys that reference a file (e.g. `system_prompt`): an UNSET key falls back to the engine's built-in default, but a SET key pointing at a missing file is a hard error — omit the key entirely rather than point it at a nonexistent path. - **Context:** PR #442 needed the retrospective flow to commit `.claude/knowledge/learning-log.md`, but the repo `.gitignore` ignored all of `.claude`; the fix scoped the ignore (`.claude/*` + `!.claude/knowledge/` + `.claude/knowledge/*` + `!.claude/knowledge/learning-log.md`) and seeded the file so the author read path never hits a missing file. **Rule:** When a bot/tool must commit a file under a normally-ignored directory, add scoped `.gitignore` negation for exactly that file (ignoring intermediate dirs still hides children, so re-include each level) and seed the file, so both the write (commit) and read paths resolve. + +### 2026-08-21: learnings since 2026-08-20T17:32:23Z +- **Context:** PR #446 corrected CONNECTION_PARAMETERS.md to document how session confs and proxies diverge between the Thrift and kernel (SEA) backends. + **Rule:** The kernel/SEA path is stricter than Thrift: session-conf keys are matched case-insensitively against an allowlist (non-allowlisted keys are dropped with a warning, a few hard-rejected e.g. HTTP 400 INVALID_CONF_VALUE), and only http(s) proxies are accepted (socks* URLs honored on Thrift are rejected at connect) — so a conf/proxy that works on Thrift may be silently inert or rejected on kernel; verify kernel behavior separately when adding or relying on any session parameter or proxy feature. From e4852e957386a70423c1342313fed46c7d3d611e Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Sat, 22 Aug 2026 17:32:32 +0000 Subject: [PATCH 03/14] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning-log.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/.claude/knowledge/learning-log.md b/.claude/knowledge/learning-log.md index 8cf434d5..faed0237 100644 --- a/.claude/knowledge/learning-log.md +++ b/.claude/knowledge/learning-log.md @@ -25,3 +25,13 @@ retrospective flow. ### 2026-08-21: learnings since 2026-08-20T17:32:23Z - **Context:** PR #446 corrected CONNECTION_PARAMETERS.md to document how session confs and proxies diverge between the Thrift and kernel (SEA) backends. **Rule:** The kernel/SEA path is stricter than Thrift: session-conf keys are matched case-insensitively against an allowlist (non-allowlisted keys are dropped with a warning, a few hard-rejected e.g. HTTP 400 INVALID_CONF_VALUE), and only http(s) proxies are accepted (socks* URLs honored on Thrift are rejected at connect) — so a conf/proxy that works on Thrift may be silently inert or rejected on kernel; verify kernel behavior separately when adding or relying on any session parameter or proxy feature. + +### 2026-08-22: learnings since 2026-08-21T17:32:18Z +- **Context:** PR #449 fixed Azure OAuth U2M on the kernel backend — `resolveKernelAuth` had been forwarding the cloud-inferred client id + scopes (`a.U2MClientID()` / `oauth.GetScopes`) that the Thrift path infers from the host, which routed the kernel's browser to a broken AAD authorize URL on Azure. + **Rule:** The kernel/SEA backend runs ONE cloud-blind in-house workspace-federated U2M flow (OIDC discovery against `{host}/oidc`, no Azure branching), so its auth mapping must forward the fixed in-house `databricks-sql-connector` client + `offline_access`+`sql` scopes uniformly across all clouds — do NOT forward the Azure Entra-direct app id / `user_impersonation` scope the Thrift path uses, even though both paths authorize identically on AWS/GCP. +- **Context:** PR #449 replaced `oauth.GetScopes(host, nil)` with a hardcoded scope slice and initially wrote `[]string{"sql", "offline_access"}`, producing a High-severity failure: one test asserted the reversed order and contradicted its sibling test, because `resolveKernelAuth` returns `[]string{"offline_access", "sql"}`. + **Rule:** OAuth scopes are a space-delimited unordered set per spec, but `reflect.DeepEqual` on `[]string` is order-sensitive — when hardcoding a scope slice that replaces `oauth.GetScopes` (which appends `offline_access` first, then `sql`), preserve the exact element order and keep all sibling assertions consistent, or DeepEqual comparisons/tests will fail spuriously. +- **Context:** PR #444 added `WithFederatedTokenProvider*` on the kernel path; for account-wide federation (no client id) the driver hands the kernel the raw un-exchanged external subject token via `set_auth_pat`, unlike the Thrift path which exchanges in-driver via `FederationProvider`. Reviewers repeatedly questioned whether this silently fails; a maintainer confirmed the kernel's behavior. + **Rule:** The kernel performs a mandatory server-side token exchange for tokens presented on the PAT path UNLESS the token is same-issuer or non-JWT — so handing a raw external-IdP JWT subject token to `set_auth_pat` (client id only set for SP-wide federation) correctly federates account-wide without driver-side exchange. Rely on this documented kernel guarantee rather than assuming un-exchanged tokens are treated as literal PATs. +- **Context:** In both PR #449 and PR #444, reviewers (Copilot, peco-review-bot) flagged multiple stale/contradictory doc+comment sites after a kernel-auth behavior change — the behavior is mirrored across `doc.go`, `README.md`, `CONNECTION_PARAMETERS.md`, `internal/backend/kernel/auth.go` (Auth struct + provider-interface docs), `backend.go` (setAuth comment), and `auth/oauth/u2m/authenticator.go`. + **Rule:** This repo documents kernel/Thrift auth semantics redundantly across many files; when changing behavior on one auth path, sweep ALL mirrored doc/comment sites in the same PR (not just the function you edited) or reviewers will flag stale contradictions. From 95737b8f302ece8662d808e838d1f12a05553924 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Mon, 24 Aug 2026 17:34:34 +0000 Subject: [PATCH 04/14] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning-log.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/.claude/knowledge/learning-log.md b/.claude/knowledge/learning-log.md index faed0237..36658b39 100644 --- a/.claude/knowledge/learning-log.md +++ b/.claude/knowledge/learning-log.md @@ -35,3 +35,17 @@ retrospective flow. **Rule:** The kernel performs a mandatory server-side token exchange for tokens presented on the PAT path UNLESS the token is same-issuer or non-JWT — so handing a raw external-IdP JWT subject token to `set_auth_pat` (client id only set for SP-wide federation) correctly federates account-wide without driver-side exchange. Rely on this documented kernel guarantee rather than assuming un-exchanged tokens are treated as literal PATs. - **Context:** In both PR #449 and PR #444, reviewers (Copilot, peco-review-bot) flagged multiple stale/contradictory doc+comment sites after a kernel-auth behavior change — the behavior is mirrored across `doc.go`, `README.md`, `CONNECTION_PARAMETERS.md`, `internal/backend/kernel/auth.go` (Auth struct + provider-interface docs), `backend.go` (setAuth comment), and `auth/oauth/u2m/authenticator.go`. **Rule:** This repo documents kernel/Thrift auth semantics redundantly across many files; when changing behavior on one auth path, sweep ALL mirrored doc/comment sites in the same PR (not just the function you edited) or reviewers will flag stale contradictions. + +### 2026-08-24: learnings since 2026-08-23T17:29:41Z +- **Context:** PR #450 (kernel log-forwarding cgo bridge) — a reviewer flagged a helper that coerced a `cgo.Handle` (an integer token) into a small fabricated `void*` and passed it as the C callback's `user_data`. + **Rule:** Never pass a `cgo.Handle` or any fabricated/Go-managed pointer into a C pointer slot (`void* user_data`); the GC can detect the invalid pointer and abort via `runtime.throw`, which `recover` cannot catch — use NULL `user_data` with a package-global sink, or allocate a real C cell and pass its address. +- **Context:** PR #450 — the kernel invokes the log callback synchronously on the emitting native thread and forbids blocking/re-entry, but the first implementation ran zerolog plus an arbitrary user `SetLogOutput` writer inside the callback. + **Rule:** In a synchronous FFI callback that runs on a foreign thread, do only minimal bounded work: copy borrowed strings into owned memory and non-blockingly enqueue onto a bounded channel, then drain and do the real I/O on a separate goroutine — never run arbitrary user I/O on the callback thread (it can stall the native op or re-enter the ABI). +- **Context:** PR #450 — the logger held its destination-routing mutex while executing the user-owned `Write`, so a blocking writer (or one that called `SetLogOutput`/logged) could deadlock and could not be replaced. + **Rule:** Never invoke arbitrary user-supplied code (a `Write`, callback, or handler) while holding an internal lock; publish the target via an atomic pointer / short-held lock and invoke it only after releasing the lock, so a stuck consumer can never block retargeting or self-deadlock. +- **Context:** PR #450 — `installKernelLogCallback` returned early inside `logCallbackOnce.Do` when the level was OFF; a reviewer noted `sync.Once` still marks itself complete, latching forwarding off permanently for the process. + **Rule:** A conditional early `return` inside `sync.Once.Do` still records the Once as done, so a later attempt becomes a permanent no-op; if a step must be retryable when a precondition later changes, gate the precondition outside `Once.Do` rather than short-circuiting within it. +- **Context:** PR #450 — building the root zerolog instance over a stable proxy that implemented only `io.Writer` permanently downgraded any later `zerolog.LevelWriter` (e.g. `MultiLevelWriter`, syslog) to plain `Write`, losing severity-aware routing. + **Rule:** A long-lived proxy/wrapper placed in front of a swappable destination must implement the richest interface its underlying targets may support (e.g. `WriteLevel` for `zerolog.LevelWriter`) and delegate to it when present; wrapping only the base interface silently strips capabilities of any future richer target. +- **Context:** PR #450 — dropped kernel log records under channel backpressure were counted but the count was only read by tests, so a burst past capacity silently lost lines with no operator-visible signal. + **Rule:** When dropping data under backpressure (bounded queue overflow, sampling, truncation), surface it — at minimum a one-shot/periodic warning through the normal output path — so consumers can distinguish 'nothing was produced' from 'output was silently dropped'. From 3e0ea54cb1e74dd8cfee4cd3dab95bd09315358e Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Tue, 25 Aug 2026 17:35:08 +0000 Subject: [PATCH 05/14] 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 36658b39..9cf79e66 100644 --- a/.claude/knowledge/learning-log.md +++ b/.claude/knowledge/learning-log.md @@ -49,3 +49,11 @@ retrospective flow. **Rule:** A long-lived proxy/wrapper placed in front of a swappable destination must implement the richest interface its underlying targets may support (e.g. `WriteLevel` for `zerolog.LevelWriter`) and delegate to it when present; wrapping only the base interface silently strips capabilities of any future richer target. - **Context:** PR #450 — dropped kernel log records under channel backpressure were counted but the count was only read by tests, so a burst past capacity silently lost lines with no operator-visible signal. **Rule:** When dropping data under backpressure (bounded queue overflow, sampling, truncation), surface it — at minimum a one-shot/periodic warning through the normal output path — so consumers can distinguish 'nothing was produced' from 'output was silently dropped'. + +### 2026-08-25: learnings since 2026-08-24T17:32:28Z +- **Context:** PR #456 added `WithKernelClientCertificate`; a paired credential where an empty cert/key is invalid. It introduced a dedicated `TLSClientCertConfigured` bool alongside the PEM buffers and validated at connect time (`ErrInvalidKernelConfig`). + **Rule:** For optional paired-credential/config options where empty is invalid, carry an explicit "configured" sentinel flag (not just checking for non-empty values) so an explicit call with empty input is rejected rather than silently treated as unset — prevents failing open. +- **Context:** PR #455 corrected the kernel connection docs: the connect-context deadline was described as unhonored only during U2M browser login, but it is actually unhonored mid-connect for ALL auth modes (PAT/M2M included). + **Rule:** On the kernel/SEA path the connect-context deadline is checked only at entry to session-open; the kernel's blocking C-ABI session-open then runs uninterruptibly, so a slow cold-start or network partition can block past the deadline regardless of auth mechanism — not a U2M-specific limitation. +- **Context:** PR #455's new kernel session-conf allowlist table transcribed state living in vendored kernel source (`build/kernel-src/src/config.rs`) not present in the repo checkout; a review-bot finding flagged it as un-guarded and drift-prone, and the fix pinned the source and called out the missing CI guard. + **Rule:** When documenting behavior sourced from vendored/external code that isn't in the repo checkout (so no test or CI guard anchors it), pin the exact source ref/path, name which entries have repo-side anchors, and explicitly warn the rest may lag — treat the external source as authoritative. From 8f0e74f1b5085b8bcc4cdf790b9ccd71380bfabc Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Wed, 26 Aug 2026 18:07:08 +0000 Subject: [PATCH 06/14] 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 9cf79e66..abc15e9f 100644 --- a/.claude/knowledge/learning-log.md +++ b/.claude/knowledge/learning-log.md @@ -57,3 +57,11 @@ retrospective flow. **Rule:** On the kernel/SEA path the connect-context deadline is checked only at entry to session-open; the kernel's blocking C-ABI session-open then runs uninterruptibly, so a slow cold-start or network partition can block past the deadline regardless of auth mechanism — not a U2M-specific limitation. - **Context:** PR #455's new kernel session-conf allowlist table transcribed state living in vendored kernel source (`build/kernel-src/src/config.rs`) not present in the repo checkout; a review-bot finding flagged it as un-guarded and drift-prone, and the fix pinned the source and called out the missing CI guard. **Rule:** When documenting behavior sourced from vendored/external code that isn't in the repo checkout (so no test or CI guard anchors it), pin the exact source ref/path, name which entries have repo-side anchors, and explicitly warn the rest may lag — treat the external source as authoritative. + +### 2026-08-26: learnings since 2026-08-25T17:32:57Z +- **Context:** In PR #458 a reviewer caught an unused local variable (`k declared and not used`) in `internal/backend/kernel/backend.go`, a file behind `//go:build cgo && databricks_kernel`. It is a hard Go compile error, but the default `go test` excludes that tag so ordinary CI would not catch it — only the tagged kernel test environment fails. + **Rule:** Code behind build tags excluded from the default test run (e.g. `cgo && databricks_kernel`) is not compiled or vetted by ordinary `go test`/CI; compile and run the tagged build separately before relying on it, since even trivial compile errors (unused vars) there pass default CI. +- **Context:** In PR #458 the test seam `trySetTokenCacheConfig` originally re-implemented the `set_u2m_token_cache_config` cgo call standalone instead of routing through the production `setAuth`. A reviewer noted that a future edit dropping/mis-wiring the setter inside `setAuth`'s U2M branch would leave every test green; the fix rebuilt a real `KernelBackend` and called `k.setAuth(cfg)`. + **Rule:** Test seams should drive the real production code path (call the actual method under test), not a parallel re-implementation of the same underlying call — a standalone copy asserts the C signature but not that the production path still invokes it, so regressions in the real wiring go uncaught. +- **Context:** In PR #458 the `WithTokenCache(bool)` option calls `kernelExperimental(c)`, which allocates `KernelExperimental` unconditionally — so even `WithTokenCache(false)` makes `KernelExperimental != nil` and is rejected on the Thrift backend with `ErrRequiresKernelBackend`. The DSN carrier `tokenCache=false` deliberately does NOT forward, so only it is a true no-op. Godoc initially (wrongly) claimed disabling was "a no-op on any backend." + **Rule:** `WithKernel*` options allocate the kernel-only `KernelExperimental` struct unconditionally, so passing even the disabling/`false` value opts the connection into the kernel backend and fails on Thrift; only the DSN carrier for such a flag can be a genuine no-op. Scope any "no-op" doc claim to the DSN path, and mind that the option and DSN entry points for the same setting diverge (option always opts in; DSN `false` does not). From 11ed01cf8d47a3248de49c1708977e676c1eef45 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Thu, 27 Aug 2026 20:45:14 +0000 Subject: [PATCH 07/14] 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 abc15e9f..27ae8612 100644 --- a/.claude/knowledge/learning-log.md +++ b/.claude/knowledge/learning-log.md @@ -65,3 +65,11 @@ retrospective flow. **Rule:** Test seams should drive the real production code path (call the actual method under test), not a parallel re-implementation of the same underlying call — a standalone copy asserts the C signature but not that the production path still invokes it, so regressions in the real wiring go uncaught. - **Context:** In PR #458 the `WithTokenCache(bool)` option calls `kernelExperimental(c)`, which allocates `KernelExperimental` unconditionally — so even `WithTokenCache(false)` makes `KernelExperimental != nil` and is rejected on the Thrift backend with `ErrRequiresKernelBackend`. The DSN carrier `tokenCache=false` deliberately does NOT forward, so only it is a true no-op. Godoc initially (wrongly) claimed disabling was "a no-op on any backend." **Rule:** `WithKernel*` options allocate the kernel-only `KernelExperimental` struct unconditionally, so passing even the disabling/`false` value opts the connection into the kernel backend and fails on Thrift; only the DSN carrier for such a flag can be a genuine no-op. Scope any "no-op" doc claim to the DSN path, and mind that the option and DSN entry points for the same setting diverge (option always opts in; DSN `false` does not). + +### 2026-08-27: learnings since 2026-08-26T18:05:10Z +- **Context:** PR #457 forwarded the driver's ClientTimeout to the kernel C ABI (`kernel_session_config_set_request_timeout`). Reviewers noted that passing `0` does not mean unlimited or immediate — the kernel substitutes its own 120s default. + **Rule:** When forwarding a timeout/limit to the kernel C ABI, treat `0` as the "use kernel default" sentinel (120s for request timeout), not as unlimited or zero-wait; document this at every knob and account for it when reporting effective values. +- **Context:** In PR #457, telemetry's `SocketTimeout` was initially populated with a millisecond value, but reviewers flagged that the receiver proto tags millisecond durations with an explicit `_ms`/`_millis` suffix (`retry_overall_timeout_ms`, `result_set_ready_latency_millis`), whereas the bare `socket_timeout` field is interpreted in seconds — the fix converted to seconds. + **Rule:** Before populating a telemetry duration field, match its unit to the receiver schema's field-name convention: bare names (e.g. `socket_timeout`) are seconds, only `_ms`/`_millis`-suffixed names are milliseconds — mismatching over/under-reports by 1000×. +- **Context:** PR #457 had two paired duration converters: the kernel setter rounded positive sub-millisecond values *up* to 1 (so a real timeout never collapses into the `0 = default` sentinel), while the telemetry helper rounded positive sub-second values *down* to 0 — meaning a small nonzero configured timeout was forwarded as a real deadline yet reported as "unset" (the field was `omitempty`). + **Rule:** When paired converters share a `0 = default/unset` sentinel, round positive sub-unit values consistently (floor them to 1, never down to 0) so a genuinely-configured value is never misreported as absent. From b2e1d5173d5c7d7bef6b18fb302c21e2f41c126a Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 28 Aug 2026 21:05:49 +0000 Subject: [PATCH 08/14] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning-log.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/.claude/knowledge/learning-log.md b/.claude/knowledge/learning-log.md index 27ae8612..66cad344 100644 --- a/.claude/knowledge/learning-log.md +++ b/.claude/knowledge/learning-log.md @@ -73,3 +73,17 @@ retrospective flow. **Rule:** Before populating a telemetry duration field, match its unit to the receiver schema's field-name convention: bare names (e.g. `socket_timeout`) are seconds, only `_ms`/`_millis`-suffixed names are milliseconds — mismatching over/under-reports by 1000×. - **Context:** PR #457 had two paired duration converters: the kernel setter rounded positive sub-millisecond values *up* to 1 (so a real timeout never collapses into the `0 = default` sentinel), while the telemetry helper rounded positive sub-second values *down* to 0 — meaning a small nonzero configured timeout was forwarded as a real deadline yet reported as "unset" (the field was `omitempty`). **Rule:** When paired converters share a `0 = default/unset` sentinel, round positive sub-unit values consistently (floor them to 1, never down to 0) so a genuinely-configured value is never misreported as absent. + +### 2026-08-28: learnings since 2026-08-27T20:43:32Z +- **Context:** PR #440 wired per-platform kernel archives as nested Go modules using an in-tree `replace` pointing at a placeholder `require ... v0.0.0`/`v0.0.1`; reviewers flagged it builds in-tree but breaks external consumers. + **Rule:** Go `replace` directives are honored ONLY in the main module — a downstream `go get` of your module ignores them, so never rely on an in-tree `replace` to shadow a placeholder `require` version; publish and pin the dependency at a real tagged version before advertising a `go get`-installable build. +- **Context:** PR #440 review: an unresolvable nested-module `require` would break even pure-Thrift (`CGO_ENABLED=0`, no build tag) consumer builds that never compile a kernel file. + **Rule:** Go module-graph resolution (MVS) is build-tag-independent — every `require`d module's `go.mod` must resolve for ALL consumers regardless of build tags, so an unpublished/placeholder version in `go.mod` fails builds that never touch that code, not just the tag-gated ones. +- **Context:** PR #440 added a `!databricks_kernel_dynlib` term to only the darwin/arm64 cgo link shim while the other shims and the `cgo_unsupported.go` compile guard stayed dynlib-unaware, so building with that tag compiles no link shim and fails with opaque `undefined reference` link errors. + **Rule:** With build-tag-gated cgo link shims plus a companion `unsupported`/compile-time guard, any new build-tag term must be applied consistently across every shim AND the guard's exclusion list — an asymmetric term silently drops all shims and defeats the guard whose job is to turn link failures into a clear compile error. +- **Context:** PR #440 CI sync step used `go mod verify || true`, which reviewers noted swallows genuine module-cache corruption/tampering failures while the surrounding comment framed the step as verifying the module graph. + **Rule:** Do not mask verification/gating commands (`go mod verify`, checksum/lint checks) with `|| true` — it defeats the check's purpose; let the command fail the job, or log an explicit justification if a non-fatal run is truly intended. +- **Context:** PR #451's `applyTelemetry` always forwarded hardcoded Go-side constants (batch size, flush interval, etc.) across the kernel C ABI even when unset, while `CONNECTION_PARAMETERS.md` promised the kernel's own default applied "when unset." + **Rule:** When docs state the downstream/native layer owns a default "when unset," do not hardcode a duplicated copy of that default on the wrapper side and always forward it — forward a sentinel that lets the native layer keep its own default, or change the docs to state the driver owns the value; duplicated defaults silently drift when the native side changes and no test catches it. +- **Context:** PRs #463/#440 documented a `make kernel-lib` → `make build-kernel` → `make test-kernel` contributor flow, but reviewers noted `build-kernel`/`test-kernel` link the published bindings modules, not the local `.a` that `kernel-lib` produces; linking the local build needs a still-unwired manual `go.work` step. + **Rule:** In this repo the kernel dev-loop does not link a locally built archive by default — `make build-kernel`/`test-kernel` link the published `databricks-sql-kernel-bindings` modules; to test a local source build you must manually point a `go.work`/replace at the built `lib/` archive, so verify which archive is actually linked before trusting a source-build change. From fd921453d0a9d5fe210efb7504f10e36fc9f4718 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 4 Sep 2026 17:26:36 +0000 Subject: [PATCH 09/14] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning-log.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.claude/knowledge/learning-log.md b/.claude/knowledge/learning-log.md index 66cad344..d2856d84 100644 --- a/.claude/knowledge/learning-log.md +++ b/.claude/knowledge/learning-log.md @@ -87,3 +87,7 @@ retrospective flow. **Rule:** When docs state the downstream/native layer owns a default "when unset," do not hardcode a duplicated copy of that default on the wrapper side and always forward it — forward a sentinel that lets the native layer keep its own default, or change the docs to state the driver owns the value; duplicated defaults silently drift when the native side changes and no test catches it. - **Context:** PRs #463/#440 documented a `make kernel-lib` → `make build-kernel` → `make test-kernel` contributor flow, but reviewers noted `build-kernel`/`test-kernel` link the published bindings modules, not the local `.a` that `kernel-lib` produces; linking the local build needs a still-unwired manual `go.work` step. **Rule:** In this repo the kernel dev-loop does not link a locally built archive by default — `make build-kernel`/`test-kernel` link the published `databricks-sql-kernel-bindings` modules; to test a local source build you must manually point a `go.work`/replace at the built `lib/` archive, so verify which archive is actually linked before trusting a source-build change. + +### 2026-09-04: learnings since 2026-09-03T17:27:27Z +- **Context:** In PR #469 the kernel-rev sync also rewrote user-facing docs (CONNECTION_PARAMETERS.md/README) to claim the kernel now forwards `WithSessionParams` unchanged with no allowlist, but the review flagged that released-driver consumers link the *published* `databricks-sql-kernel-bindings` module in `go.mod`, whose archive contents live outside this repo checkout — the no-allowlist behavior was only verified in the source-built pinned `KERNEL_REV`. + **Rule:** When a PR documents new kernel behavior, gate the doc change on the shipped/published bindings version that released-driver users actually link — don't describe source-built `KERNEL_REV` behavior as current unless you can confirm the pinned bindings module in `go.mod` contains it (bindings archive contents are not in this checkout, so treat them as unverifiable from the repo alone). From 1dd3d56dac9213482d4da59da303733eb323621c Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Sat, 5 Sep 2026 17:25:41 +0000 Subject: [PATCH 10/14] 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 d2856d84..3a5e813e 100644 --- a/.claude/knowledge/learning-log.md +++ b/.claude/knowledge/learning-log.md @@ -91,3 +91,11 @@ retrospective flow. ### 2026-09-04: learnings since 2026-09-03T17:27:27Z - **Context:** In PR #469 the kernel-rev sync also rewrote user-facing docs (CONNECTION_PARAMETERS.md/README) to claim the kernel now forwards `WithSessionParams` unchanged with no allowlist, but the review flagged that released-driver consumers link the *published* `databricks-sql-kernel-bindings` module in `go.mod`, whose archive contents live outside this repo checkout — the no-allowlist behavior was only verified in the source-built pinned `KERNEL_REV`. **Rule:** When a PR documents new kernel behavior, gate the doc change on the shipped/published bindings version that released-driver users actually link — don't describe source-built `KERNEL_REV` behavior as current unless you can confirm the pinned bindings module in `go.mod` contains it (bindings archive contents are not in this checkout, so treat them as unverifiable from the repo alone). + +### 2026-09-05: learnings since 2026-09-04T17:25:10Z +- **Context:** PR #470 added per-statement query tags on the kernel backend; a reviewer caught that the serialized tags string reaching the length-less `kernel_statement_set_query_tags` C ABI setter had no interior-NUL guard, unlike SQL text (`checkQueryText`/`errQueryNUL`) and bound params (`checkParamValue`/`errParamNUL`). + **Rule:** Any user-supplied string passed to a NUL-terminated (length-less) kernel C ABI setter must be guarded for interior NUL bytes before cgo marshaling — a NUL silently truncates on the kernel path while the length-prefixed Thrift `confOverlay` carries the value whole, breaking Thrift/SEA parity; fail loudly, and place the guard in an untagged pure-Go file so it is unit-tested under CGO_ENABLED=0. +- **Context:** PR #468 bumped the vendored `databricks_kernel.h`, flipping the documented `kernel_session_close` contract from async/best-effort to awaited, but the Go-side `CloseSession` doc comment still described the old behavior and explicitly cited that header. + **Rule:** When a vendored kernel C ABI header's documented contract changes (or KERNEL_REV is bumped), audit and update the Go doc comments that cite or paraphrase that header in lockstep — they go stale silently and can end up contradicting the very ABI they reference. +- **Context:** PR #471 documented numeric defaults in CONNECTION_PARAMETERS.md (e.g. `cloudfetch_link_prefetch_window=10`, `inline_max_chunks_in_memory=4`) that live only in the Rust kernel, not in this repo; a reviewer noted they can drift out of sync silently. + **Rule:** Don't assert exact numeric defaults owned by the Rust kernel in repo docs unless the value is pinned by a cited KERNEL_REV (as the mTLS row does); otherwise soften to "kernel default" to avoid silent doc/kernel drift. From ac862f22c7d932d244ad550cc3a3a3f314b1962d Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Fri, 11 Sep 2026 17:28:19 +0000 Subject: [PATCH 11/14] 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 3a5e813e..a9463a45 100644 --- a/.claude/knowledge/learning-log.md +++ b/.claude/knowledge/learning-log.md @@ -99,3 +99,11 @@ retrospective flow. **Rule:** When a vendored kernel C ABI header's documented contract changes (or KERNEL_REV is bumped), audit and update the Go doc comments that cite or paraphrase that header in lockstep — they go stale silently and can end up contradicting the very ABI they reference. - **Context:** PR #471 documented numeric defaults in CONNECTION_PARAMETERS.md (e.g. `cloudfetch_link_prefetch_window=10`, `inline_max_chunks_in_memory=4`) that live only in the Rust kernel, not in this repo; a reviewer noted they can drift out of sync silently. **Rule:** Don't assert exact numeric defaults owned by the Rust kernel in repo docs unless the value is pinned by a cited KERNEL_REV (as the mTLS row does); otherwise soften to "kernel default" to avoid silent doc/kernel drift. + +### 2026-09-11: learnings since 2026-09-10T17:25:51Z +- **Context:** PR #479 added Reyden→kernel auto-recovery that hinges on `errors.Is(err, ErrReydenThriftUnsupported)` traversing production's wrapping chain (`thrift.Backend.OpenSession` wraps the marker via `NewRequestError` → `WithMessage`/`WithStack`), but the tests injected fakes returning the *bare* marker, so a regression that breaks the unwrap chain would silently disable recovery while every test stayed green. + **Rule:** When a feature depends on `errors.Is`/`errors.As` matching through a multi-layer wrapping chain, add a test that asserts the match against the production-shaped *wrapped* error (or drives the real backend) — fakes returning the bare sentinel do not pin the production error path. +- **Context:** PR #479's `TestReydenDefaultBuildKernelNotCompiled` asserted default-build stub behavior (`newKernelBackend` returning `ErrKernelNotCompiled`) but carried no build constraint, so it also compiled under `-tags databricks_kernel` + `CGO_ENABLED=1` where `newKernelBackend` is the real implementation, breaking `make test` for the kernel build. + **Rule:** A test asserting build-tag-gated stub behavior must carry the same `//go:build` constraint as the stub it exercises (e.g. `!cgo || !databricks_kernel`); an untagged test runs under every build and fails where the real implementation is linked. +- **Context:** PR #479 review flagged that the known-Reyden cache pre-check opened the kernel backend before the `KernelExperimental != nil` guardrail, so identical misconfigured config (a `WithKernel*` option without `WithUseKernel`) produced a hard error on a cold cache but silently succeeded on a warm cache populated by a sibling connection. + **Rule:** Run config-validation guardrails before any cache-based short-circuit/pre-check so behavior for a given config is deterministic and does not depend on process-global cache state. From fe3b0bda0bc17fb3283a118708d1608cfb0f86df Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Thu, 17 Sep 2026 17:27:37 +0000 Subject: [PATCH 12/14] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning-log.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.claude/knowledge/learning-log.md b/.claude/knowledge/learning-log.md index a9463a45..0962fbae 100644 --- a/.claude/knowledge/learning-log.md +++ b/.claude/knowledge/learning-log.md @@ -107,3 +107,7 @@ retrospective flow. **Rule:** A test asserting build-tag-gated stub behavior must carry the same `//go:build` constraint as the stub it exercises (e.g. `!cgo || !databricks_kernel`); an untagged test runs under every build and fails where the real implementation is linked. - **Context:** PR #479 review flagged that the known-Reyden cache pre-check opened the kernel backend before the `KernelExperimental != nil` guardrail, so identical misconfigured config (a `WithKernel*` option without `WithUseKernel`) produced a hard error on a cold cache but silently succeeded on a warm cache populated by a sibling connection. **Rule:** Run config-validation guardrails before any cache-based short-circuit/pre-check so behavior for a given config is deterministic and does not depend on process-global cache state. + +### 2026-09-17: learnings since 2026-09-16T17:26:26Z +- **Context:** PR #485 rewrote `parseHostName` in connector.go — the old code used `strings.HasPrefix(host, "https"/"http")`, which is case-sensitive and also matched non-scheme hostnames like `httpbin.example.com`. The fix uses `strings.Cut` on `:` plus `strings.EqualFold`, and this scheme detection feeds downstream backend validation (e.g. mTLS-over-HTTP must be rejected before the SEA/kernel backend regardless of scheme casing). + **Rule:** When detecting a URL/URI scheme by string matching, compare case-insensitively (URI schemes are case-insensitive per RFC 3986 — use `EqualFold`, not `HasPrefix`) and require the `:` delimiter so ordinary hostnames that merely start with `http` aren't misparsed as a scheme; verify casing variants flow correctly into downstream backend/protocol validation. From aaad986b6dbb2e45d979aa4b77803e026dba87d1 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Wed, 23 Sep 2026 17:31:41 +0000 Subject: [PATCH 13/14] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning-log.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/.claude/knowledge/learning-log.md b/.claude/knowledge/learning-log.md index 0962fbae..ed9dfdad 100644 --- a/.claude/knowledge/learning-log.md +++ b/.claude/knowledge/learning-log.md @@ -111,3 +111,13 @@ retrospective flow. ### 2026-09-17: learnings since 2026-09-16T17:26:26Z - **Context:** PR #485 rewrote `parseHostName` in connector.go — the old code used `strings.HasPrefix(host, "https"/"http")`, which is case-sensitive and also matched non-scheme hostnames like `httpbin.example.com`. The fix uses `strings.Cut` on `:` plus `strings.EqualFold`, and this scheme detection feeds downstream backend validation (e.g. mTLS-over-HTTP must be rejected before the SEA/kernel backend regardless of scheme casing). **Rule:** When detecting a URL/URI scheme by string matching, compare case-insensitively (URI schemes are case-insensitive per RFC 3986 — use `EqualFold`, not `HasPrefix`) and require the `:` delimiter so ordinary hostnames that merely start with `http` aren't misparsed as a scheme; verify casing variants flow correctly into downstream backend/protocol validation. + +### 2026-09-23: learnings since 2026-09-22T17:27:28Z +- **Context:** PR #484 added a detached per-statement cancel watcher on the kernel client-query-timeout path; reviewers repeatedly flagged that the watcher can call `kernel_statement_canceller_cancel`/`_free` concurrently with or after `kernel_statement_close`, and safety hinged on a kernel C ABI guarantee (canceller owns cloned Arcs + its own session ref, no back-pointer into the statement) that is not verifiable from this repo. Resolution came only from checking the pinned kernel revision and its ABI test `canceller_outlives_statement_and_session_close`. + **Rule:** When Go memory safety depends on a cgo/kernel object staying valid independently of another handle's lifetime (use/free after the related statement or session is closed), that independence is NOT verifiable from this repo — confirm it against the pinned KERNEL_REV and its ABI test, and record the load-bearing assumption in a code comment so a future kernel refactor trips a reviewer instead of silently introducing a use-after-free. +- **Context:** PR #484 introduced `MaxClientQueryTimeoutMilliseconds` as a hand-copied mirror of the C macro `DATABRICKS_KERNEL_MAX_CLIENT_QUERY_TIMEOUT_MS`, guarded only by a build-tagged runtime test; a reviewer contrasted this with the `KernelStatusCode` constants, which use a compile-time drift assertion (`_ = uint(a-b) | uint(b-a)`) so a header renumber fails the build. + **Rule:** Any Go constant hand-mirrored from a C ABI macro must be pinned with a compile-time drift assertion (the established `uint(a-b)|uint(b-a)` pattern), not a build-tagged runtime test — a test that must be remembered to run lets a KERNEL_REV bump silently validate against a stale bound. +- **Context:** In PR #484 `WithClientQueryTimeout` stores a pointer by copying into a fresh local (`configured := timeout; c.ClientQueryTimeout = &configured`) inside the option closure, and a dedicated test asserts reusing one option value across two connectors does not alias their configs. + **Rule:** A functional option that stores a pointer must allocate a fresh copy per application (copy into a local inside the closure, not `¶m`); otherwise reusing the same option value across multiple constructions aliases shared mutable state. Add a test that applies one option to two objects and mutates one. +- **Context:** PR #484 modeled `ClientQueryTimeout` as `*time.Duration` so that an omitted option (nil → legacy `kernel_statement_execute` with the 600s ceiling) is behaviorally distinct from an explicit zero (non-nil → timeout-aware entry point, unlimited), with a test named "omitted remains distinguishable from explicit zero." + **Rule:** When an omitted option must select different behavior than an explicit zero/default value, model it as a pointer (nil = omitted) rather than a bare value, and test both the omitted and explicit-zero cases — a plain zero value collapses the two and silently picks one path. From 7459758339cfe6494f334151b96e1850693cdea3 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Tue, 6 Oct 2026 17:30:21 +0000 Subject: [PATCH 14/14] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning-log.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.claude/knowledge/learning-log.md b/.claude/knowledge/learning-log.md index ed9dfdad..cdeaa61a 100644 --- a/.claude/knowledge/learning-log.md +++ b/.claude/knowledge/learning-log.md @@ -121,3 +121,9 @@ retrospective flow. **Rule:** A functional option that stores a pointer must allocate a fresh copy per application (copy into a local inside the closure, not `¶m`); otherwise reusing the same option value across multiple constructions aliases shared mutable state. Add a test that applies one option to two objects and mutates one. - **Context:** PR #484 modeled `ClientQueryTimeout` as `*time.Duration` so that an omitted option (nil → legacy `kernel_statement_execute` with the 600s ceiling) is behaviorally distinct from an explicit zero (non-nil → timeout-aware entry point, unlimited), with a test named "omitted remains distinguishable from explicit zero." **Rule:** When an omitted option must select different behavior than an explicit zero/default value, model it as a pointer (nil = omitted) rather than a bare value, and test both the omitted and explicit-zero cases — a plain zero value collapses the two and silently picks one path. + +### 2026-10-06: learnings since 2026-10-05T17:30:24Z +- **Context:** PR #493 fixed M2M OAuth so a least-privilege service-principal secret (e.g. scoped to `sql`) can authenticate. The old `GetScopes` unconditionally appended `all-apis` to any caller-supplied scope list, broadening access beyond what the caller asked for. + **Rule:** When defaulting an OAuth/credential scope set, only inject a broad default (e.g. `all-apis`) when the caller passed NONE — never append it to a non-empty list, or you silently over-grant and break least-privilege scoped secrets. +- **Context:** PR #493 reversed an earlier decision to reject custom M2M scopes on the SEA/kernel path. The rejection assumed scopes were unforwardable because `set_auth_m2m` has no scopes argument; in fact a separate `set_oauth_scopes` C-ABI call carries them, so the whole `M2MScopesSupported` rejection path was deleted. + **Rule:** Before rejecting a config option as unsupported on a backend because its primary setter can't carry it, check for a complementary API (e.g. a separate `set_oauth_scopes` call) that can — a missing argument on one setter doesn't mean the capability is absent.