diff --git a/.claude/knowledge/learning-log.md b/.claude/knowledge/learning-log.md index 988c85f4..27ae8612 100644 --- a/.claude/knowledge/learning-log.md +++ b/.claude/knowledge/learning-log.md @@ -10,3 +10,66 @@ 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. + +### 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. + +### 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'. + +### 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. + +### 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). + +### 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.