Skip to content

Commit 3ef3adc

Browse files
learn: retrospective learnings
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
1 parent fddcf13 commit 3ef3adc

1 file changed

Lines changed: 12 additions & 0 deletions

File tree

.claude/knowledge/learning-log.md

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,4 +68,16 @@ until a human merges it. The engineer author phase reads this log (see
6868
- **Context:** In run #32772322538 the author phase called `StructuredOutput` five times in a row (turns 42-50), each resubmission trimming the `root_cause` field (+3252 → +1749 chars).
6969
**Rule:** Structured-output fields have length limits that reject over-long submissions and force silent retries; front-load a concise root_cause/summary the first time rather than emitting a maximal draft and shrinking it across repeated failed `StructuredOutput` calls.
7070

71+
### 2026-08-26: learnings since 2026-08-25T17:35:52Z
72+
- **Context:** In PR #932, `oauth_token_cache_enabled` was forwarded via `bool(opts.get(...))`, which reviewers (Copilot + peco-review-bot) flagged because a DSN/env value like the string `"False"` is truthy and would silently enable on-disk token persistence; the fix used `opts.get(...) is True`.
73+
**Rule:** For a typed `Optional[bool]` connection option that can arrive as a string from a DSN/env, gate enabling on an explicit `is True` identity check, never `bool(...)` — truthy coercion turns `"False"`/`"0"`/`"no"` into an accidental enable; fail safe to disabled.
74+
- **Context:** In PR #932, on-disk OAuth token persistence moved to the kernel path, whose own built-in default is *enabled*; omitting the kwarg would have inherited that default and silently started writing tokens to disk, so the connector forwards an explicit `token_cache_enabled=False` when the option is unset.
75+
**Rule:** When migrating a lifecycle/behavior control to a backend whose native default differs from the connector's prior posture, forward the connector's intended default explicitly rather than omitting the kwarg — omission inherits the backend default and silently changes behavior for existing callers.
76+
- **Context:** In PR #932, the U2M branch unconditionally forwards `token_cache_enabled` into the pyo3 `databricks_sql_kernel.Session(...)`; reviewers noted this raises `TypeError: unexpected keyword argument` on *every* connect against the currently-pinned kernel (^0.2.0) that lacks the field, making it a merge-ordering gate requiring the pin bump to land atomically.
77+
**Rule:** Adding a new kwarg to the native kernel `Session(...)` call is not backward-compatible with the existing `databricks-sql-kernel` pin — either bump the pin atomically (and gate CI on it) or conditionally include the key only when the binding supports it, or U2M/connect breaks entirely, not just for opt-in callers.
78+
- **Context:** In PR #932, the `session.py` line mapping `oauth_token_cache_enabled` into `kernel_auth_options` had no test; a reviewer noted that because the feature fails safe to `False`, a drop/rename in refactor would silently regress with no test failure, so a threading assertion mirroring the azure-sp-m2m test was added.
79+
**Rule:** A mapping/threading line whose feature degrades to a safe default when missing needs its own unit test asserting the value reaches the far side — the safe default masks a silent regression that behavioral tests won't catch.
80+
- **Context:** In PR #931, `_socket_timeout` was forwarded to the kernel where it means the *total HTTP request deadline* (not the Thrift socket send/recv/connect timeout), with divergent defaults (900s Thrift vs 120s kernel) and a sentinel divergence where `0`/unset selects the kernel default rather than meaning unlimited or immediate.
81+
**Rule:** A shared connection parameter can carry different semantics, defaults, and sentinel (`0`/`None`) meanings on the kernel vs Thrift path — document the per-backend contract explicitly and don't assume a value forwards with the same meaning across backends.
82+
7183
--- *Add new entries above this line (oldest→newest); newest sections sort to the bottom.* ---

0 commit comments

Comments
 (0)