Repository navigation
learn: retrospective learnings - #443
peco-engineer-bot[bot] wants to merge 21 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".
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-27: 4 new learning(s) since 2026-10-26T17:30:35Z.