Repository navigation
learn: retrospective learnings - #908
peco-engineer-bot[bot] wants to merge 19 commits into
Conversation
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
peco-engineer-bot
Bot
added
the
engineer-bot-learning
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Low
Doc-only change appending dated learning sections to the retrospective knowledge log; content is coherent and no executable code is touched. One low-severity concern: the new 2026-09-28 section duplicates the preceding 2026-09-24 entry (same PR #947, same since 2026-09-18T17:27:19Z window), adding a restated learning rather than a new one.
| **Rule:** A `logging.Filter` that mutates `record.args` must handle all three shapes it can take — `None` (message logged with no args), a tuple, and a dict — and tests must exercise each shape (especially the dict path) since an untested branch masked a NameError. | ||
|
|
||
| ### 2026-09-28: learnings since 2026-09-18T17:27:19Z | ||
| - **Context:** PR #947 fixed `RedactUrlQueryParamsFilter.filter` in `src/databricks/sql/__init__.py`: the non-dict branch assumed `record.args` was always iterable and crashed when it was `None` (log calls with no interpolation args), and a `record.arg[k]` typo in the dict branch had gone unnoticed because that path was never exercised by a test. |
There was a problem hiding this comment.
🔵 Low — This new 2026-09-28 section is a near-duplicate of the immediately-preceding 2026-09-24 section: both cover the same PR #947 fix to RedactUrlQueryParamsFilter.filter in src/databricks/sql/__init__.py, and both carry the identical since 2026-09-18T17:27:19Z window start. The retrospective window evidently did not advance (both say "since 2026-09-18T17:27:19Z"), so the same learning was re-emitted with only slightly reworded Context/Rule text.
The log's own convention comment says each daily run appends "one dated section of 0..N learnings" — a genuinely-new run that surfaced no new source PR should append 0 learnings rather than restate a prior one. As written, a future reader mining this log for durable lessons hits the same record.args-mutation rule twice back-to-back, and the duplicated since timestamp obscures which retrospective actually produced it. Consider dropping this section (or, if a new run genuinely occurred with nothing new, appending an empty-dated section) so the log doesn't accumulate restated duplicates.
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Low
Documentation-only change appending dated sections to the engineer-bot learning log — no executable code, so nothing to review for correctness/security/tests. One low-severity content concern: PR #947's learning is duplicated across the last three sections (2026-09-24, 2026-09-28, 2026-10-07), all sharing the same since 2026-09-18T17:27:19Z watermark, hinting the retrospective's since-cursor isn't advancing between runs.
|
|
||
| ### 2026-09-17: learnings since 2026-09-16T17:28:35Z | ||
| - **Context:** PR #951 hardened mTLS client-identity handling in `SSLOptions`: it added `validate_client_identity()` / `load_client_cert_chain()` and rejects a private key configured without a client certificate. | ||
| **Rule:** Python's `ssl.SSLContext.load_cert_chain` accepts a combined cert+key PEM (so certfile-without-keyfile is valid) but silently ignores a keyfile-without-certfile and downgrades to one-way TLS — explicitly reject key-without-cert, and preflight each identity file (readable, non-empty) separately before calling `load_cert_chain` so the failing input is named instead of an opaque stdlib SSL error. |
There was a problem hiding this comment.
🔵 Low — The same PR #947 RedactUrlQueryParamsFilter.filter learning is emitted three times in a row, in the 2026-09-24, 2026-09-28, and 2026-10-07 sections, each phrased slightly differently but conveying the identical rule (handle record.args as None/tuple/dict, test the dict branch).
Tellingly, all three sections carry the same learnings since 2026-09-18T17:27:19Z watermark, which suggests the retrospective's since-cursor isn't advancing after each run, so it keeps re-distilling the same merged PR into a fresh dated section. Each subsequent run will likely append yet another copy. Worth checking the cursor-advance logic in the retrospective job (databricks_bot_engine.engineer_bot.retrospective) so the window base moves forward once a learning is recorded; otherwise the rolling log accretes duplicates until merge.
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Low
Documentation-only PR appending auto-generated retrospective learning sections — no executable code, APIs, or tests touched, so correctness/security/test axes don't apply. Structure, ordering, and sentinel line are intact. One Low data-quality note: three consecutive sections (2026-09-24, 2026-09-28, 2026-10-07) repeat the same since 2026-09-18T17:27:19Z watermark and re-describe PR #947, suggesting the generator's window cursor didn't advance between those runs.
| - **Context:** PR #947 fixed `RedactUrlQueryParamsFilter.filter` in `src/databricks/sql/__init__.py`, which crashed on log calls with no args (`record.args is None`) and carried a latent `record.arg[k]` NameError typo hidden in the never-exercised dict-args branch. | ||
| **Rule:** A `logging.Filter` that mutates `record.args` must handle all three shapes it can take — `None` (message logged with no args), a tuple, and a dict — and tests must exercise each shape (especially the dict path) since an untested branch masked a NameError. | ||
|
|
||
| ### 2026-09-28: learnings since 2026-09-18T17:27:19Z |
There was a problem hiding this comment.
🔵 Low — Three consecutive sections carry the same watermark learnings since 2026-09-18T17:27:19Z — ### 2026-09-24, ### 2026-09-28, and ### 2026-10-07 — and all three describe the same PR #947 RedactUrlQueryParamsFilter.filter fix in near-identical wording. The log's own convention is that each daily run appends learnings for a new window; a static since timestamp across three dated runs means the retrospective's cursor was not advanced after the 09-24 and 09-28 runs, so the same window (and the same PR) was redistilled and re-appended three times.
This is a generator/data-quality issue, not a code bug, but it leaves three redundant entries that future author-phase reads will treat as three distinct lessons about the same fix. Worth confirming the retrospective advances its knowledge_log cursor to the merge/run time after each run so windows don't overlap. The 2026-10-08 and 2026-10-13 sections correctly advance the watermark, so the issue appears isolated to this run of duplicates.
Rolling retrospective learnings
This PR accumulates one dated section of learnings per day (from merged PRs and engineer-bot author runs) until it is merged; merging it starts a fresh one. The bot never writes the canonical log directly.
Latest update 2026-10-13: 3 new learning(s) since 2026-10-12T17:33:54Z.