Skip to content

learn: retrospective learnings #443

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
peco-engineer-bot wants to merge 3 commits into
base: main
Choose a base branch
from
Open

learn: retrospective learnings #443

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
77bfd52
learn: retrospective learnings
peco-engineer-bot[bot] Aug 13, 2026
7c711e7
learn: retrospective learnings
peco-engineer-bot[bot] Aug 21, 2026
e4852e9
learn: retrospective learnings
peco-engineer-bot[bot] Aug 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions .claude/knowledge/learning-log.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,28 @@ configured in `.bot/config.yaml`:

No learnings have been recorded yet. Dated sections are appended below by the
retrospective flow.
## Entries

@peco-review-bot peco-review-bot Bot Aug 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Low — The ## Entries heading is appended directly after the paragraph line ...appended below by the\nretrospective flow. with no intervening blank line. In CommonMark (and GitHub's renderer), an ATX heading must be preceded by a blank line to be parsed as a heading; without it the ## Entries text is folded into the preceding paragraph and renders as literal ## Entries rather than a section header. Insert a blank line before ## Entries.


@peco-review-bot peco-review-bot Bot Aug 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Low — The seeded sentence No learnings have been recorded yet. is now stale — the file below it contains two dated ### ... sections with recorded learnings. Since the retrospective flow appends but does not rewrite this preamble, consider having the flow drop or update this line once the first entry lands, otherwise the header contradicts the body on every future run.

(Anchored to the nearest changed line — see the description for the exact location.)

### 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/<model-name>/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.
Loading