Skip to content

Commit 8053646

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

1 file changed

Lines changed: 14 additions & 0 deletions

File tree

.claude/knowledge/learning-log.md

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,3 +67,17 @@ initial dated section below.
6767
**Rule:** When a connector option depends on a newer native/kernel binding, a `KERNEL_REV` source bump alone does not reach npm consumers: the pinned `optionalDependencies` binding silently ignores unknown fields and falls back to its own (possibly opposite / security-relevant) default. Land the native-router version-check bump and all platform `optionalDependencies` pins together in the same commit that flips the version, and never rely on a connector-supplied default that a not-yet-published binding cannot honor.
6868
- **Context:** In PR #513's kernel U2M e2e suite, the Mocha `before`-all hook called `this.skip()` (gate env vars absent) *before* taking the on-disk cache backup, leaving `backupDir` undefined — but Mocha still runs the `after`-all hook, whose unconditional `fs.rmSync(cacheDir, …)` then deleted the developer's real OAuth token cache with no restore. Fixed by a `suiteActive` flag set only after the gate passes and the backup completes, with `after` early-returning when false.
6969
**Rule:** A `before`-all `this.skip()` does not prevent the `after`-all hook from running, so any destructive cleanup in `after` must be gated behind a flag that is set only after setup/backup has actually completed — otherwise a skipped suite runs teardown against un-backed-up real state.
70+
71+
### 2026-08-28: learnings since 2026-08-27T20:44:22Z
72+
- **Context:** PR #514 routed all kernel bound-parameter forwarding exclusively through a new `rawParams` field, and PR #506 forwarded new `telemetryCircuitBreaker*` options — both fields present only in the source-pinned `KERNEL_REV`, not the published native package. Reviewers repeatedly flagged that napi-rs silently ignores unknown fields on the options object.
73+
**Rule:** On the kernel (`useKernel`) path, forwarding driver state through a napi field that only exists in the source-pinned `KERNEL_REV` is a silent-failure hazard: an older published native binding drops the field with no error (dropped params → wrong results, dropped telemetry → inert config). Keep a fallback to fields the shipped binding understands, add a load-time capability check, or gate the change behind a published-native-package bump — never rely on the field being honored just because TypeScript accepts it.
74+
- **Context:** In PRs #514 and #506, `native/kernel/index.d.ts` (marked `auto-generated by NAPI-RS`) was hand-edited to add new fields/interfaces, and unit tests exercised a hand-written JS fake rather than the real binding, so `tsc` passed regardless of whether the compiled `.node` matched.
75+
**Rule:** `native/kernel/index.d.ts` is a generated, types-only boundary — hand-edits can drift from the real native ABI and neither `tsc` nor the mocked unit suite (which asserts against a JS stub) will catch it. Regenerate it from the pinned `KERNEL_REV` build rather than authoring declarations by hand, and treat green mocked tests as no evidence the shipped binding honors a new field.
76+
- **Context:** PR #514 changed a native-surface dependency (new `rawParams` path in the pinned `KERNEL_REV`) but its CHANGELOG entry omitted the published-native-package caveat that the sibling `getTypeInfo()` entry carried; multiple reviewers asked for parity.
77+
**Rule:** When a kernel-path change depends on a native surface that exists only in the source-pinned `KERNEL_REV` (not yet in the published `@databricks/databricks-sql-kernel-*` optional dependency), the CHANGELOG entry must carry the standard "customer npm installs require a follow-up bump to a published native package" caveat plus the kernel PR/ticket reference.
78+
- **Context:** PR #514's e2e test for INTERVAL MONTH preservation asserted only `expect(caught).to.be.instanceOf(Error)`, which passes for any failure (connect/auth/network/warm-up), not just the intended server-side type rejection.
79+
**Rule:** A negative-path test that asserts only that *some* `Error` was thrown proves nothing about the behavior under test — against a gated/live warehouse it passes on unrelated failures. Assert on the error's shape or message (e.g. that it references the interval literal or a type/parse failure) so the test can only pass for the intended reason.
80+
- **Context:** In PR #506, a kernel `getLocaleName` resolved locale env vars in a different order than the existing Thrift `DBSQLClient.getLocaleName` (`LANG || LC_ALL || LC_MESSAGES`), so the same process reported different `localeName` telemetry per backend; the fix aligned both to POSIX precedence.
81+
**Rule:** Runtime-identity/telemetry metadata (locale, process name, etc.) must be backend-invariant — the kernel and Thrift paths should resolve it identically. Resolve locale env vars in POSIX precedence `LC_ALL || LC_MESSAGES || LANG`, and when adding a second backend's copy of such logic, align (or share) it with the existing one rather than letting the two drift.
82+
- **Context:** In PR #506, `buildKernelTelemetryOptions` used `Number.isFinite(...)` guards documented as "Omitted ⇒ kernel default," but `DBSQLClient.getDefaultConfig()` always populates every telemetry knob from `DEFAULT_TELEMETRY_CONFIG`, so the omit branch is unreachable on the live client path (only reachable from hand-built test configs) — the JS connector defaults silently override the kernel's own tuning.
83+
**Rule:** Before relying on an "omitted ⇒ use downstream default" fallback branch, verify it is actually reachable — an upstream `getDefaultConfig()` that always populates the field makes the fallback dead code and silently substitutes the connector's defaults for the component's own. Guards/docs advertising an opt-in default the code can never take are misleading.

0 commit comments

Comments
 (0)