Repository navigation
feat(docs-sync): learn from maintainer corrections - #12823
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental review at Files Reviewed (2 files)
Previous Review Summaries (2 snapshots, latest commit 1dd45df)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 1dd45df)Status: No Issues Found | Recommendation: Merge Incremental review at Files Reviewed (2 files)
Previous review (commit 0d89078)Status: 1 Issue Found | Recommendation: Address before merge Overview
Incremental review at Issue Details (click to expand)SUGGESTION
Files Reviewed (9 files)
Reviewed by kimi-k3 · Input: 45.6K · Output: 7.3K · Cached: 305.2K Review guidance: REVIEW.md from base branch |
pandemicsyn
left a comment
pandemicsyn
left a comment
There was a problem hiding this comment.
couple small things inline from the robot
| } | ||
| existing = parseLearnings(existingText) | ||
| } | ||
| log(`existing entries: ${existing.length}`) |
There was a problem hiding this comment.
Extraction errors can disable all existing learnings for the run
Suggested fix: Emit prompt artifacts from the best available existing LEARNINGS.md before any fallible API/model work, and retain them on every error path. Ideally pre-seed from the checked-out file at startup, then replace them after loading the rolling branch and after a validated delta. Add a test where PR/comment fetching throws and assert triage/edit still receive existing rules.
|
|
||
| // Live PATCH: body-only, one line changed. The job already holds pull-requests: write. | ||
| const { api, repo } = await import("./lib.mjs") | ||
| const newBody = patchMarkerIntoBody(prBody, marker) |
There was a problem hiding this comment.
The direct marker PATCH can overwrite concurrent PR-body edits
Suggested fix: Fetch the PR again immediately before patching, apply patchMarkerIntoBody to that latest body, and PATCH the result. Add a test in which the body returned at patch time differs from the initially fetched body and verify the intervening content is preserved. If GitHub exposes a usable conditional-update mechanism, use it to close the remaining fetch/PATCH race.
| reason = `source ${a.source} not in candidate sources` | ||
| } else if (!/^[a-z0-9][a-z0-9-]{2,48}$/.test(a.id)) { | ||
| reason = `invalid id format: ${a.id}` | ||
| } else if (existingIds.has(a.id) && !toRemove.includes(a.id)) { |
There was a problem hiding this comment.
Duplicate IDs within one model response pass validation
Suggested fix: Track IDs accepted from the current add array as well as existing IDs, rejecting a second occurrence. Also compare normalized text against already accepted additions so one response cannot render duplicate rules. Add validation tests for duplicate IDs and duplicate rule text in the same delta.
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.
* feat(docs-sync): learn from maintainer corrections * feat(docs-sync): apply learnings during sync * fix(docs-sync): handle standalone review comments * fix(docs-sync): cover extraction marker routes * test(docs-sync): verify dry-run marker safety * chore: retrigger review * fix(docs-sync): harden extraction input * fix(docs-sync): compare correction timestamps * feat(docs-sync): assign and request review on new rolling PR
…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.
Summary
Add learning from maintainer corrections to the rolling docs pull request.
The workflow reads trusted corrective commits and review comments. It extracts validated rules, injects tagged rules into triage and edit, and commits
LEARNINGS.mdwith the normal rolling pull request update.Design and safety
The implementation keeps the twelve closed design decisions and G5.
LEARNINGS.mdin the non-content draft gate.learned-throughfor empty deltas and uses the upsert route for non-empty deltas.The extractor rejects rules that name a pull request, page, person, or URL. It preserves a marker when extraction omits its output. It writes no marker under
dry_runorLEARNINGS_NO_PATCH.Upsert changes
nonContentFilesexcludes the root learnings file from the draft gate.renderBodyrenderslearned-throughonly when a value exists.resolveLearnedThroughpreserves an existing marker when extraction omits an output.Local proof
The extraction call deliberately omits
--auto. The local proof usedLEARNINGS_NO_PATCH=1, the real9dd2c07correction, andkilo/moonshotai/kimi-k3.It produced:
Do not document features that are behind experimental or unreleased flags.bothcommit:9dd2c07kilo-auto/efficientwas unavailable locally. The workflow model completed the proof with no live pull request write.Review result
The planning reviews raised 31 findings. The plan accepted 29 and rejected two.
The rejected items were a prose-arithmetic budget assertion and a persistent learning tombstone. Both add fragile or unsupported state without preventing a real defect.
Accepted limits
The barren-run lag remains accepted. The conflict path can reset the marker and self-heals on the next run.
Visual Changes
N/A
Remaining: one human-approved dispatch
workflow_dispatchfordocs-syncon this branch.docs/auto-sync.LEARNINGS.mdcommit and advancelearned-through.dry_run: true.E2E: skipped — this change is a GitHub Action and has no local runtime surface.