From e47c5b83294b9af17f57b2f98f0f7686a5260887 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Tue, 10 Nov 2026 17:27:01 +0000 Subject: [PATCH 1/2] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning-log.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.claude/knowledge/learning-log.md b/.claude/knowledge/learning-log.md index abadcb97b..70726a777 100644 --- a/.claude/knowledge/learning-log.md +++ b/.claude/knowledge/learning-log.md @@ -178,4 +178,10 @@ until a human merges it. The engineer author phase reads this log (see - **Context:** PR #1004 bumped OSV-Scanner from v2.3.8 to v2.7.0 by find-and-replacing the version string. That also changed comments claiming "vX doesn't support per-package CVE scoping" without checking whether the claim still holds, and the reviewer flagged it. A stale claim can leave suppressions broader than they need to be. **Rule:** When bumping a tool or dependency version, never find-and-replace the version inside capability claims ("vX does not support Y"). Re-check each claim against the new version's docs, then update the claim or the config it justifies. +### 2026-11-10: learnings since 2026-11-09T17:26:03Z +- **Context:** PR #1013 (#629) made inline params render a sequence directly after `IN` as `(1,2,3)` instead of `ARRAY(1,2,3)`. The reviewer noticed that `_renders_as_array` treats any non-`str` `Sequence` as list-able, and that includes `bytes`/`bytearray`. Before the change, `x IN %s` with bytes rendered as `IN ARRAY(97,98,99)`, which Spark rejected with a loud `PARSE_SYNTAX_ERROR`. After it, the query became valid SQL and quietly matched the wrong rows. The fix left bytes out of the IN-list path. + **Rule:** When you widen which input types take a new rendering or conversion path, list the types that used to fail loudly (bytes, str, sets, empty containers) and check that none of them now produce valid but wrong output. Exclude those types on purpose and pin each one with a unit test, because a loud error that turns into a silent wrong result is a regression. +- **Context:** In PR #1013, `test_probe_shaped_string_value_is_not_a_probe` was meant to guard the nonce check in `_markers_following_in`. It could never fail: an earlier step swaps every escaped string value for `''` during the probe pass, so the probe-shaped text never reached the code the nonce check protects. The test passed whether or not the check existed. + **Rule:** When a test is meant to guard one specific check, remove or break that check and confirm the test fails (a manual mutation check). Make sure another mechanism earlier in the pipeline doesn't neutralize the input first. If one does, feed the input in where it actually reaches the guarded code, or patch the dependency the check relies on (for example `secrets.token_hex`). + --- *Add new entries above this line (oldest→newest); newest sections sort to the bottom.* --- From 1d8e3bd9fd2f5a7a50444c6aeeb288c9527e4923 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Wed, 11 Nov 2026 17:28:37 +0000 Subject: [PATCH 2/2] learn: retrospective learnings Signed-off-by: peco-engineer-bot[bot] --- .claude/knowledge/learning-log.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.claude/knowledge/learning-log.md b/.claude/knowledge/learning-log.md index 70726a777..5e2bf5e85 100644 --- a/.claude/knowledge/learning-log.md +++ b/.claude/knowledge/learning-log.md @@ -184,4 +184,10 @@ until a human merges it. The engineer author phase reads this log (see - **Context:** In PR #1013, `test_probe_shaped_string_value_is_not_a_probe` was meant to guard the nonce check in `_markers_following_in`. It could never fail: an earlier step swaps every escaped string value for `''` during the probe pass, so the probe-shaped text never reached the code the nonce check protects. The test passed whether or not the check existed. **Rule:** When a test is meant to guard one specific check, remove or break that check and confirm the test fails (a manual mutation check). Make sure another mechanism earlier in the pipeline doesn't neutralize the input first. If one does, feed the input in where it actually reaches the guarded code, or patch the dependency the check relies on (for example `secrets.token_hex`). +### 2026-11-11: learnings since 2026-11-10T17:25:56Z +- **Context:** PR #1017 made `Connection.http_client` `Optional` (it is `None` for explicit `use_kernel=True`) and added `assert self.connection.http_client is not None` in the staging PUT/GET/REMOVE handlers in `client.py`. The reviewer pointed out that `python -O` strips `assert`. If a future kernel change made staging reachable, users would get an opaque `AttributeError: 'NoneType' object has no attribute 'request'` instead of a clear error. + **Rule:** Don't use `assert` to enforce a runtime invariant on an Optional attribute, even when it also narrows the type for mypy. `python -O` removes it. Use a small helper that raises a descriptive driver error (`NotSupportedError`/`ProgrammingError`, e.g. "staging operations are not supported with use_kernel=True"). That keeps mypy narrowing and still works under `-O`. +- **Context:** PR #1022 was titled as fixing kernel TLS certificate errors, but it only bumped `KERNEL_REV`. That file only sets which kernel commit `kernel-e2e.yml` builds from source for CI. Users installing the `[kernel]` extra still resolve the PyPI release allowed by `pyproject.toml` (`^1.4.0`), which predates the fix. The reviewer also noted there was no in-repo assertion (e.g. in `tests/e2e/test_kernel_tls.py`) to catch the behavior regressing on a later pin bump. + **Rule:** Bumping `KERNEL_REV` changes CI only and ships nothing to users. A kernel-side fix reaches users only after a kernel PyPI release that includes it plus a matching floor bump in `pyproject.toml`/`poetry.lock`. Don't call a pin bump a user-facing fix, and add an in-repo e2e assertion for the behavior so later pin bumps can't silently regress it. + --- *Add new entries above this line (oldest→newest); newest sections sort to the bottom.* ---