Repository navigation
learn: retrospective learnings - #443
peco-engineer-bot[bot] wants to merge 23 commits into
Conversation
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
peco-engineer-bot
Bot
added
the
engineer-bot-learning
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 2 Low
Documentation-only PR appending dated sections to the retrospective learning log — no executable code. Content looks fine; two minor markdown/doc-consistency nits (missing blank line before the ## Entries heading, and a now-stale "No learnings recorded yet" preamble).
|
|
||
| No learnings have been recorded yet. Dated sections are appended below by the | ||
| retrospective flow. | ||
| ## Entries |
There was a problem hiding this comment.
🔵 Low — The ## Entries heading is appended directly after the paragraph line ...appended below by the\nretrospective flow. with no intervening blank line. In CommonMark (and GitHub's renderer), an ATX heading must be preceded by a blank line to be parsed as a heading; without it the ## Entries text is folded into the preceding paragraph and renders as literal ## Entries rather than a section header. Insert a blank line before ## Entries.
| No learnings have been recorded yet. Dated sections are appended below by the | ||
| retrospective flow. | ||
| ## Entries | ||
|
|
There was a problem hiding this comment.
🔵 Low — The seeded sentence No learnings have been recorded yet. is now stale — the file below it contains two dated ### ... sections with recorded learnings. Since the retrospective flow appends but does not rewrite this preamble, consider having the flow drop or update this line once the first entry lands, otherwise the header contradicts the body on every future run.
(Anchored to the nearest changed line — see the description for the exact location.)
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Low
Documentation-only PR: appends five dated retrospective learning sections to .claude/knowledge/learning-log.md. No executable code touched, so correctness/concurrency/test axes don't apply. Content is well-formed and consistent; one low note that the seeded "No learnings have been recorded yet" intro line is now stale given the entries below it. Nit: there is no blank line between the seed paragraph (line 12) and the new ## Entries heading — harmless but a blank line before the heading matches typical Markdown style.
|
|
||
| No learnings have been recorded yet. Dated sections are appended below by the | ||
| retrospective flow. | ||
| ## Entries |
There was a problem hiding this comment.
🔵 Low — The seeded intro still asserts "No learnings have been recorded yet" immediately above the new ## Entries section that now contains five dated sections. The statement is now stale/contradictory. Consider dropping (or softening) that sentence when the first real entries land, e.g. leave only "Dated sections are appended below by the retrospective flow." so the header text no longer claims the log is empty.
(Anchored to the nearest changed line — see the description for the exact location.)
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Low
Docs-only learning-log update; looks fine apart from one low-severity issue. The new 2026-10-24 entries are well-scoped and match the PR #516 context they describe.
|
|
||
| No learnings have been recorded yet. Dated sections are appended below by the | ||
| retrospective flow. | ||
| ## Entries |
There was a problem hiding this comment.
🔵 Low — The preamble right above this new ## Entries heading still says "No learnings have been recorded yet." This PR adds many dated sections, so that sentence is now wrong. The author phase reads this file as its knowledge log (author.knowledge_log), so it starts with a statement that contradicts the content below it. Also, ## Entries comes straight after the paragraph with no blank line in between. Most renderers still treat it as a heading, but the formatting is inconsistent. Delete or reword the "No learnings" sentence, for example to "Dated sections are appended below by the retrospective flow.", and put a blank line before ## Entries. It may also be worth fixing the retrospective flow so it updates the seed text when it adds the first section.
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 2 Low
This docs-only learning-log update looks fine. Two low-severity wording problems could mislead the author agent that reads this file: a stale preamble, and a rule that says "floor" when it means "round up". Minor: the 2026-09-23 entry mentions the "600s ceiling" for kernel_statement_execute, but the later 2026-10-20 entry says the v1.2.0 header no longer guarantees that ceiling, so the earlier entry should carry a "historical" qualifier.
|
|
||
| No learnings have been recorded yet. Dated sections are appended below by the | ||
| retrospective flow. | ||
| ## Entries |
There was a problem hiding this comment.
🔵 Low — The seeded preamble still says "No learnings have been recorded yet", but the file now holds many dated sections. The author phase reads this file (author.knowledge_log), so the stale sentence contradicts the content below it. There is also no blank line between retrospective flow. and the new ## Entries heading, which some Markdown renderers need before a heading. Remove or reword the "No learnings" sentence and add a blank line before ## Entries. Ideally the retrospective flow would do this itself when it adds the first entry.
| - **Context:** In PR #457, telemetry's `SocketTimeout` was initially populated with a millisecond value, but reviewers flagged that the receiver proto tags millisecond durations with an explicit `_ms`/`_millis` suffix (`retry_overall_timeout_ms`, `result_set_ready_latency_millis`), whereas the bare `socket_timeout` field is interpreted in seconds — the fix converted to seconds. | ||
| **Rule:** Before populating a telemetry duration field, match its unit to the receiver schema's field-name convention: bare names (e.g. `socket_timeout`) are seconds, only `_ms`/`_millis`-suffixed names are milliseconds — mismatching over/under-reports by 1000×. | ||
| - **Context:** PR #457 had two paired duration converters: the kernel setter rounded positive sub-millisecond values *up* to 1 (so a real timeout never collapses into the `0 = default` sentinel), while the telemetry helper rounded positive sub-second values *down* to 0 — meaning a small nonzero configured timeout was forwarded as a real deadline yet reported as "unset" (the field was `omitempty`). | ||
| **Rule:** When paired converters share a `0 = default/unset` sentinel, round positive sub-unit values consistently (floor them to 1, never down to 0) so a genuinely-configured value is never misreported as absent. |
There was a problem hiding this comment.
🔵 Low — This rule contradicts itself: it says to "floor them to 1, never down to 0". Flooring a positive sub-unit value gives 0, which is exactly the outcome the rule warns against. The Context line just above says the right behavior is rounding up (clamping to a minimum of 1). An author agent reading this literally could build the wrong converter. Change it to "clamp/round them up to 1, never down to 0".
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Low
This PR only changes docs, and it looks good apart from one low-severity note: the file's opening text still says no learnings have been recorded yet. I spot-checked the new 2026-10-30 entries against the code: the WithoutCancel detach is in internal/rows/rows.go:162, and TestE2EArrowBatchesSurviveQueryContextCancellation exists. Both are accurate.
|
|
||
| No learnings have been recorded yet. Dated sections are appended below by the | ||
| retrospective flow. | ||
| ## Entries |
There was a problem hiding this comment.
🔵 Low — The seed preamble still says "No learnings have been recorded yet" right above the new ## Entries section, which now holds many dated sections. The author phase reads this file as its knowledge log (author.knowledge_log), so the sentence now contradicts the content beneath it. Either drop that sentence when the first entries land, or reword it to something like "Dated sections are appended below by the retrospective flow." Also, ## Entries comes straight after the paragraph with no blank line. Markdown still renders it, but adding the blank line keeps it consistent with the rest of the file.
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 2 Low
This PR only appends retrospective learnings to the knowledge log, and the content mostly looks fine. There are 2 low-severity consistency problems: the preamble now contradicts the entries below it, and one older rule conflicts with a newer one about the 600s ceiling.
|
|
||
| No learnings have been recorded yet. Dated sections are appended below by the | ||
| retrospective flow. | ||
| ## Entries |
There was a problem hiding this comment.
🔵 Low — The preamble right above the new ## Entries heading still says "No learnings have been recorded yet." This PR adds about 15 dated sections below it, so the sentence is now wrong. The author phase reads this file (author.knowledge_log), so the claim could mislead it. Since this rolling PR is the one that first fills the log, it should replace that sentence with something like "Dated sections are appended below by the retrospective flow." Also add a blank line before ## Entries so the markdown is cleaner.
| **Rule:** Any Go constant hand-mirrored from a C ABI macro must be pinned with a compile-time drift assertion (the established `uint(a-b)|uint(b-a)` pattern), not a build-tagged runtime test — a test that must be remembered to run lets a KERNEL_REV bump silently validate against a stale bound. | ||
| - **Context:** In PR #484 `WithClientQueryTimeout` stores a pointer by copying into a fresh local (`configured := timeout; c.ClientQueryTimeout = &configured`) inside the option closure, and a dedicated test asserts reusing one option value across two connectors does not alias their configs. | ||
| **Rule:** A functional option that stores a pointer must allocate a fresh copy per application (copy into a local inside the closure, not `¶m`); otherwise reusing the same option value across multiple constructions aliases shared mutable state. Add a test that applies one option to two objects and mutates one. | ||
| - **Context:** PR #484 modeled `ClientQueryTimeout` as `*time.Duration` so that an omitted option (nil → legacy `kernel_statement_execute` with the 600s ceiling) is behaviorally distinct from an explicit zero (non-nil → timeout-aware entry point, unlimited), with a test named "omitted remains distinguishable from explicit zero." |
There was a problem hiding this comment.
🔵 Low — The 2026-09-23 entry still says an omitted ClientQueryTimeout means "nil → legacy kernel_statement_execute with the 600s ceiling". The 2026-10-20 entry in this same PR says the v1.2.0 header no longer guarantees that 600s ceiling. The log is meant to be durable guidance for the author bot, so the older entry now conflicts with the newer one. Either reword the older rule so it doesn't depend on the 600s number (for example, "legacy kernel_statement_execute path"), or add a note pointing to the later entry.
Rolling retrospective learnings
This PR accumulates one dated section of learnings per day (from merged PRs and engineer-bot author runs) until it is merged; merging it starts a fresh one. The bot never writes the canonical log directly.
Latest update 2026-10-31: 1 new learning(s) since 2026-10-30T17:23:39Z.