[peco-engineer-bot]

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-11-07: 3 new learning(s) since 2026-11-06T17:24:26Z.

[peco-review-bot]

✅ No issues identified by the review bot.

[peco-review-bot]

✅ No issues identified by the review bot.

[peco-review-bot]

✅ No issues identified by the review bot.

[peco-review-bot]

✅ No issues identified by the review bot.

[peco-review-bot]

✅ No issues identified by the review bot.

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[peco-review-bot]

✅ No issues identified by the review bot.

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[peco-review-bot]

✅ No issues identified by the review bot.

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[peco-review-bot]

✅ No issues identified by the review bot.

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[peco-review-bot]

✅ No issues identified by the review bot.

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[peco-review-bot]

Verdict: 1 Low

Documentation-only PR: it appends dated retrospective learning sections to .claude/knowledge/learning-log.md, with no executable code to review. One low-severity doc-consistency issue — the appended ## Entries section leaves the stale "No learnings have been recorded yet" preamble in place, now contradicted by the content below it (also missing a blank line before the heading). Otherwise looks good.

[peco-review-bot]

🔵 Low — This PR appends the first ## Entries section directly beneath the preamble paragraph that still reads "No learnings have been recorded yet — the first retrospective PR will append the initial dated section below." Since this is that first retrospective PR and it now adds many dated sections, the sentence immediately above ## Entries is false the moment this merges — a reader sees "no learnings recorded yet" followed by 20+ learning sections.

When the retrospective flow writes the initial section, it should also replace (or drop) that placeholder sentence so the preamble no longer contradicts the content below it.

Minor secondary nit on the same line: ## Entries is appended with no blank line separating it from the preceding paragraph. CommonMark lets an ATX heading interrupt a paragraph so it still renders, but a blank line before the heading is the house style elsewhere in this file and avoids renderer-specific surprises.

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[peco-review-bot]

Verdict: 1 Low

Looks good. This PR only adds learning-log entries, plus one low-severity fix: the file's preamble is out of date. I checked the new 2026-10-31 KEQ_SEQ claim, and it matches lib/thrift-backend/primaryKeysSchema.ts:5-10.

[peco-review-bot]

🔵 Low — The preamble right above the new ## Entries heading still says "No learnings have been recorded yet". This PR adds 17 dated sections, so that sentence is now wrong. The author flow reads this file back as knowledge, so it will see a statement that contradicts the rest of the file. Also, ## Entries comes straight after the paragraph with no blank line in between. Most Markdown renderers still treat it as a heading, but markdownlint (MD022) flags it, and it doesn't match the spacing used elsewhere in the file. Replace the stale sentence (for example, "Dated sections are appended below.") and add a blank line before ## Entries.

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[peco-review-bot]

Verdict: 1 Medium · 1 Low · 1 Nit

The learning log is mostly fine as a docs-only update, with one medium issue: the 2026-09-24 entry contradicts the later 2026-10-06 entry on how to handle empty-string catalog arguments. Separately, the lockfile resolved-URL rule now appears three times (2026-09-01, 2026-09-09, 2026-10-07); merging those into one entry would keep the log tidier (nit).

Other findings

• ⚪ Nit — This rule tells bot workflows to set queue: max on the concurrency group. I couldn't verify queue against GitHub's documented concurrency keys (group, cancel-in-progress), and nothing in this diff shows where it comes from. If the key isn't supported, a workflow written from this rule could fail validation or quietly ignore it. Either confirm the key exists and cite the workflow that uses it, or remove it from the rule.

[peco-review-bot]

🟡 Medium — This rule from 2026-09-24 says to normalize ''→undefined on the kernel metadata path, and its context says getPrimaryKeys "explicitly rejects ''". The 2026-10-06 entry (line 129) says the opposite: PR #537 removed that guard, and the rule there is to forward empty strings unchanged and never coerce them in just one backend. The author flow reads this log back as rules, so the bot gets two contradictory instructions for the same code path. Mark the older entry as superseded, or reword it so it matches the later decision. For example: note that empty-catalog semantics differ, and say to document that rather than normalize it.

[peco-review-bot]

🔵 Low — The preamble still says "No learnings have been recorded yet — the first retrospective PR will append the initial dated section below". Directly after it, this PR adds ~170 lines of learnings. There is also no blank line before ## Entries, so the heading comes right after a paragraph. Remove or reword the stale sentence and add a blank line before the heading.

(Anchored to the nearest changed line — see the description for the exact location.)

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[peco-review-bot]

Verdict: 1 Medium · 1 Low · 2 Nit

This PR only adds to the learning log; there is one medium concern. The 09-24 and 10-06 sections give opposite rules for empty-string catalog handling, so the author bot reading the log could follow either. The other findings are low: a stale placeholder sentence, one rule repeated three times, and one rule that needs a qualifier.

Other findings

• ⚪ Nit — The intro still says "No learnings have been recorded yet — the first retrospective PR will append the initial dated section below." That stops being true once this PR adds entries. Also, ## Entries (L13) comes straight after the paragraph with no blank line. Please delete or reword the placeholder sentence and add a blank line before the heading.
• ⚪ Nit — The same rule appears three times: lockfile resolved URLs must point at registry.npmjs.org, not an internal proxy or mirror (2026-09-01 L87, 2026-09-09 L95, 2026-10-07 L133). The repeats make the log longer without adding anything, and they make it harder to spot rules that actually differ. Please merge them into one rule and list all three PRs (#516/#522/#538) in its context.

[peco-review-bot]

🟡 Medium — These two rules contradict each other. The 2026-09-24 rule says to normalize ''→undefined on kernel metadata calls, and its context (L116) says getPrimaryKeys "explicitly rejects ''". The 2026-10-06 rule (L129) says the opposite: PR #537 removed that guard, and identifiers, including the empty string, should be forwarded unchanged with no coercion in either backend.

The author flow reads this log back as guidance (author.knowledge_log), so it can follow whichever rule it reads first. Please mark the 09-24 rule as superseded by the 10-06 one, or reword it so it no longer recommends normalizing and no longer says getPrimaryKeys rejects ''.

[peco-review-bot]

🔵 Low — These two rules partly conflict. The 2026-08-25 rule says to "treat the committed napi .d.ts as the source of truth for which modes the binding declares". The 2026-08-28 rule says the same file has been hand-edited and can drift from the real native ABI, and that it says nothing about what the shipped binding actually supports. Please qualify the 08-25 rule: the .d.ts is the source of truth only when it was regenerated from the pinned KERNEL_REV, and even then it doesn't show what the published npm binary supports.