From 0ad1af98941c421078435390d628a55370f4af03 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Thu, 13 Aug 2026 17:46:30 +0000 Subject: [PATCH 1/3] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning-log.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/.claude/knowledge/learning-log.md b/.claude/knowledge/learning-log.md index 0e0c2f2e9..2c4541ca2 100644 --- a/.claude/knowledge/learning-log.md +++ b/.claude/knowledge/learning-log.md @@ -8,4 +8,16 @@ until a human merges it. The engineer author phase reads this log (see ## Entries +### 2026-08-13: learnings since 2026-08-04T18:01:30Z +- **Context:** PR #905/#906 repaired an HTTP 400 in the retrospective/bot GitHub Actions cron by fixing the `MODEL_ENDPOINT` value. + **Rule:** In bot workflow YAML, set `MODEL_ENDPOINT` to the concrete `https:///serving-endpoints//invocations` form; do NOT use `.../serving-endpoints/anthropic/invocations`, which hits `sdk_agent.translate_endpoint`'s already-v2 early-return, keeps the trailing `invocations`, and makes the CLI append `/v1/messages` → 400 (unsupported path). +- **Context:** Run #31030595544 fixed TIMESTAMP_NTZ collapsing to 'timestamp' in `cursor.description`; changing `_col_to_description` to emit a new `type_code` would have silently broken the column-based timestamp parser in `utils.py` keyed on `== "timestamp"`. + **Rule:** When you change the DB-API `type_code` string a column reports, grep for every downstream branch that matches that string (e.g. type conversion/parsing in `utils.py`) and extend it too — a new type_code that no existing branch matches leaves values unparsed, a silent regression. +- **Context:** Run #31030595544 diagnosed that Spark `TIMESTAMP` and `TIMESTAMP_NTZ` both arrive over Thrift as `TTypeId.TIMESTAMP_TYPE`, so the enum name alone collapses them (same pattern PR #560 used for VARIANT). + **Rule:** Distinct Spark SQL types that share one Thrift `TTypeId` cannot be disambiguated from the enum name; recover the true type by reading the Arrow field's `Spark:DataType:SqlName` metadata override in `_col_to_description`. +- **Context:** Run #31030595544 verified the TIMESTAMP_NTZ fix; the disambiguating `Spark:DataType:SqlName` Arrow metadata is emitted by the live warehouse, not synthesized client-side. + **Rule:** When a fix depends on server-populated Arrow schema metadata, prove it with an e2e test against the real warehouse (a mocked unit test asserts the metadata you fabricated, not that the server actually emits it); a passing mock is not evidence the wire carries the field. +- **Context:** Run #31030595544's read-only plan phase spent a turn calling `edit_file` (turn 32) before realizing at turn 34 that edit tools are unavailable during planning. + **Rule:** In the read-only plan phase the structured plan is the only deliverable — do not attempt `edit_file`/write tools there; defer all edits to the implementation phase to avoid wasted turns. + --- *Add new entries above this line (oldest→newest); newest sections sort to the bottom.* --- From 41ee2cadcef4f8c9876081ae2824e1b3975fa530 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Sat, 15 Aug 2026 17:33:20 +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 2c4541ca2..077472cd5 100644 --- a/.claude/knowledge/learning-log.md +++ b/.claude/knowledge/learning-log.md @@ -20,4 +20,8 @@ until a human merges it. The engineer author phase reads this log (see - **Context:** Run #31030595544's read-only plan phase spent a turn calling `edit_file` (turn 32) before realizing at turn 34 that edit tools are unavailable during planning. **Rule:** In the read-only plan phase the structured plan is the only deliverable — do not attempt `edit_file`/write tools there; defer all edits to the implementation phase to avoid wasted turns. +### 2026-08-15: learnings since 2026-08-14T17:40:48Z +- **Context:** PR #910 forwarded `identity_federation_client_id` through all three kernel auth-resolution branches (M2M, PAT, U2M) in `auth_bridge.py`, guarded by a truthy `if federation_client_id:` check; the reviewer flagged that the field was uniquely untested even though every other forwarded kwarg was pinned per-path. + **Rule:** When a kwarg/field is forwarded identically across N parallel resolution branches, assert it per-branch (plus a negative case for the omitted/empty guard) — a shared behavior tested on only one branch lets a dropped-field regression on the others pass CI silently. + --- *Add new entries above this line (oldest→newest); newest sections sort to the bottom.* --- From d85e7242e868b80220adbc073dbad20c648728cd Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Tue, 18 Aug 2026 17:37:24 +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 077472cd5..16fbe68a6 100644 --- a/.claude/knowledge/learning-log.md +++ b/.claude/knowledge/learning-log.md @@ -24,4 +24,14 @@ until a human merges it. The engineer author phase reads this log (see - **Context:** PR #910 forwarded `identity_federation_client_id` through all three kernel auth-resolution branches (M2M, PAT, U2M) in `auth_bridge.py`, guarded by a truthy `if federation_client_id:` check; the reviewer flagged that the field was uniquely untested even though every other forwarded kwarg was pinned per-path. **Rule:** When a kwarg/field is forwarded identically across N parallel resolution branches, assert it per-branch (plus a negative case for the omitted/empty guard) — a shared behavior tested on only one branch lets a dropped-field regression on the others pass CI silently. +### 2026-08-18: learnings since 2026-08-16T17:31:37Z +- **Context:** PR #916 (kernel `row_limit`) — a reviewer flagged that treating `row_limit=0` as a hard zero-row cap diverges from SEA (which drops `row_limit<=0` and returns all rows); the maintainer resolved it by stating SEA is incomplete and is not a reference implementation. + **Rule:** When checking cross-backend behavior alignment in this connector, treat Thrift as the authoritative reference; SEA is incomplete/being deprecated and must NOT be used as an alignment target. +- **Context:** PR #914 (kernel OAuth U2M bundle) — reviewers repeatedly caught that forwarding a caller `oauth_redirect_port` without an explicit `oauth_client_id` produces an unregistered redirect URI, because the default `databricks-sql-python` app only registers ports 8020–8024. + **Rule:** OAuth U2M `oauth_redirect_port` is coupled to `oauth_client_id` — only override the default port range when BOTH are supplied (mirroring Thrift's `get_python_sql_connector_auth_provider`); a bare port must fall back to the app's registered range. +- **Context:** PR #914 saw many review rounds (scopes hardcoded vs. honored, stale-tree confusion) because "exact parity with Thrift" was asserted in the CHANGELOG, docstring, and comments while the code actually diverged (e.g. honoring caller `oauth_scopes`). + **Rule:** When a change claims cross-backend parity, keep code, tests, comments, docstring, and CHANGELOG mutually consistent — don't assert "exact parity"/"mirrors X exactly" for behavior that intentionally diverges; scope the parity claim to what actually matches. +- **Context:** PR #913 (connection-parameters doc) — many params show ❌ for the kernel backend because `Session._create_backend` forwards only a curated, named subset of `connect()` kwargs to the kernel rather than splatting `**kwargs`; forwarded retry options use `kwargs.get(...)` with no fallback, so the kernel applies its own Rust defaults. + **Rule:** On the kernel path, a `connect()`/session kwarg is honored only if `Session._create_backend` explicitly forwards it — anything outside that named subset is silently ignored, and options passed as `kwargs.get(...)` without a fallback let the kernel supply its own default rather than the Thrift default. + --- *Add new entries above this line (oldest→newest); newest sections sort to the bottom.* ---