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 2 commits into
base: main
Choose a base branch
from
Open

learn: retrospective learnings #443

Changes from all commits
Commits
Show all changes
2 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
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
15 changes: 15 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,18 @@ 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.
Loading