Skip to content

fix(docs-sync): address review findings on the learnings step - #12834

Merged
iscekic merged 1 commit into
mainfrom
docs-learn-review-9c21
Aug 3, 2026
Merged

fix(docs-sync): address review findings on the learnings step#12834
iscekic merged 1 commit into
mainfrom
docs-learn-review-9c21

Conversation

@iscekic

@iscekic iscekic commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

Follow-up to #12823. It fixes the three review findings from @pandemicsyn.

All three reproduce. Each fix carries one test that fails without it.

1. A failed API call disabled every learned rule

The learn step is continue-on-error, and triage.mjs and edit.mjs read only
docs-sync-out/learnings-triage.md and learnings-edit.md. A throw anywhere in
extraction left both files absent, so the whole run ran with no learned rule and
no signal.

learn.mjs now writes both artifacts from the checked-out LEARNINGS.md before
any fallible work, and replaces them once the rolling branch copy loads. An empty
block now removes the file, so a later replacement can shrink a seeded block.

2. The marker PATCH overwrote a concurrent body edit

The direct PATCH sent the body read at step 1. The extraction call runs for up to
10 minutes between the read and the write, so any edit in that window was lost.

patchOrLogMarker now re-reads the body immediately before the PATCH. GitHub has
no conditional update for a pull request body, so a short fetch-to-PATCH race
remains. The comment states it.

3. Duplicate ids and rules inside one response

validateDelta compared each addition against the existing entries only. Two
additions in one response could carry one id or one rule text, and both rendered.
It now rejects a duplicate of an earlier accepted addition.

Tests

  • 10s asserts both prompt artifacts survive a failed API call and carry the
    checked-out rule. It also asserts an empty file clears a stale block.
  • 10t drives learn.mjs against a stub GitHub API. The second read of the pull
    request returns a maintainer edit. The test asserts the edit survives the
    PATCH, the marker holds the new tip SHA, and one marker remains.
  • 10u asserts both duplicate rejections.

DOCS_SYNC_API_BASE is a new selftest-only hook that points lib.mjs at the
stub server. The workflow never sets it.

Local proof

node .github/docs-sync/selftest.mjs passes.

Every new test was mutation-checked. With the fix reverted:

  • 10s fails with learnings-triage.md must survive the failure.
  • 10t fails with the concurrent body edit must survive the marker PATCH.
  • 10u fails with a repeated id must be rejected.

Visual Changes

N/A

E2E: skipped — this change is a GitHub Action and has no local runtime surface.

Three defects from the review of #12823.

- A throw after the learn step started left no prompt artifact, so triage and
  edit ran with no learned rule at all. The step is continue-on-error, so the
  run continued and the failure was silent. learn.mjs now writes both prompt
  artifacts from the checked-out file before any fallible work, and replaces
  them once the rolling branch copy loads.
- The direct marker PATCH sent the body read before the extraction call, so it
  overwrote any body edit made in the minutes since. learn.mjs now re-reads the
  body immediately before the PATCH.
- Two additions in one model response could carry one id or one rule text.
  validateDelta now rejects a duplicate of an earlier accepted addition.

Tests: 10s pins the prompt artifacts across a failed API call and the removal of
a stale block. 10t drives learn.mjs against a stub GitHub API whose second read
returns a maintainer edit, and asserts the edit survives the PATCH. 10u pins
both duplicate rejections. DOCS_SYNC_API_BASE is the new selftest-only hook that
points lib.mjs at the stub server.
@iscekic iscekic added the human-ready PR is ready for a human review. label Aug 3, 2026
@iscekic iscekic self-assigned this Aug 3, 2026
@iscekic
iscekic requested a review from pandemicsyn August 3, 2026 21:42
@vercel

vercel Bot commented Aug 3, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
docs (docs-staging) Ready Ready Preview Aug 3, 2026 9:44pm

Request Review

@kilo-code-bot

kilo-code-bot Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (3 files)
  • .github/docs-sync/learn.mjs
  • .github/docs-sync/lib.mjs
  • .github/docs-sync/selftest.mjs

lgtm — all three follow-up fixes are correct: the prompt-artifact seed runs before any fallible work and every success path rewrites it, the marker PATCH re-reads the body with a warned fallback, and validateDelta now rejects duplicate ids/rule text within one response without blocking valid re-adds. Tests 10s/10t/10u drive the real script (stub API server, no mock-of-mocks) and pin each regression.


Reviewed by kimi-k3 · Input: 95.2K · Output: 19.6K · Cached: 812.9K

Review guidance: REVIEW.md from base branch main

@iscekic
iscekic enabled auto-merge (squash) August 3, 2026 21:49
@iscekic
iscekic merged commit 6ec20f2 into main Aug 3, 2026
33 checks passed
@iscekic
iscekic deleted the docs-learn-review-9c21 branch August 3, 2026 21:51
t7tran pushed a commit to t7tran/kilocode that referenced this pull request Aug 14, 2026
…rg#12834)

Three defects from the review of Kilo-Org#12823.

- A throw after the learn step started left no prompt artifact, so triage and
  edit ran with no learned rule at all. The step is continue-on-error, so the
  run continued and the failure was silent. learn.mjs now writes both prompt
  artifacts from the checked-out file before any fallible work, and replaces
  them once the rolling branch copy loads.
- The direct marker PATCH sent the body read before the extraction call, so it
  overwrote any body edit made in the minutes since. learn.mjs now re-reads the
  body immediately before the PATCH.
- Two additions in one model response could carry one id or one rule text.
  validateDelta now rejects a duplicate of an earlier accepted addition.

Tests: 10s pins the prompt artifacts across a failed API call and the removal of
a stale block. 10t drives learn.mjs against a stub GitHub API whose second read
returns a maintainer edit, and asserts the edit survives the PATCH. 10u pins
both duplicate rejections. DOCS_SYNC_API_BASE is the new selftest-only hook that
points lib.mjs at the stub server.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

human-ready PR is ready for a human review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants