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 1/3] 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 2/3] 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 3/3] 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.