Skip to content

learn: retrospective learnings #1018

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
peco-engineer-bot wants to merge 2 commits into
base: main
Choose a base branch
from
Open

learn: retrospective learnings #1018

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e47c5b8
learn: retrospective learnings
peco-engineer-bot[bot] Nov 10, 2026
1d8e3bd
learn: retrospective learnings
peco-engineer-bot[bot] Nov 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions .claude/knowledge/learning-log.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,4 +178,16 @@ 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.

@peco-review-bot peco-review-bot Bot Nov 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Low — The Rule lists sets next to bytes as types that "used to fail loudly", then says "Exclude those types on purpose". In the same PR (#1013), sets were deliberately moved onto the new path: _renders_as_in_list accepts AbstractSet, and render_inline sorts the items. Read literally, the rule tells a future author to exclude any type that used to fail loudly. That conflicts with the code this entry describes. Suggest rewording so the instruction is to check each previously-failing type and exclude it only when the new output would be wrong (as with bytes/bytearray), not to exclude all of them by reflex.

- **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.* ---
Loading