From 6ec20f23952b94517a106de366c23024a628e0b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Mon, 3 Aug 2026 23:51:45 +0200 Subject: [PATCH] fix(docs-sync): address review findings on the learnings step (#12834) 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. --- .github/docs-sync/learn.mjs | 65 ++++++++-- .github/docs-sync/lib.mjs | 4 +- .github/docs-sync/selftest.mjs | 214 ++++++++++++++++++++++++++++++++- 3 files changed, 269 insertions(+), 14 deletions(-) diff --git a/.github/docs-sync/learn.mjs b/.github/docs-sync/learn.mjs index a6c0dad3d2e..6baa59124e2 100644 --- a/.github/docs-sync/learn.mjs +++ b/.github/docs-sync/learn.mjs @@ -205,6 +205,10 @@ export function validateDelta(delta, { existing, candidateSources, deletedInWind const valid = [] const toRemove = [] const existingIds = new Set(ex.map((e) => e.id)) + // One model response can repeat an id or a rule. Both would render two lines for + // one id, so an accepted addition also blocks the next one. + const acceptedIds = new Set() + const acceptedRules = new Set() // Process remove first so toRemove is populated before the add loop checks // for id collisions with entries listed in remove (criterion 8). @@ -237,6 +241,8 @@ export function validateDelta(delta, { existing, candidateSources, deletedInWind reason = `invalid id format: ${a.id}` } else if (existingIds.has(a.id) && !toRemove.includes(a.id)) { reason = `id ${a.id} collides with an existing entry not listed in remove` + } else if (acceptedIds.has(a.id)) { + reason = `id ${a.id} collides with an earlier addition in this delta` } else if (!/^\d{4}-\d{2}-\d{2}$/.test(a.date)) { reason = `invalid date format: ${a.date}` } else { @@ -261,6 +267,13 @@ export function validateDelta(delta, { existing, candidateSources, deletedInWind continue } + // Duplicate of an earlier addition in the same response. + if (acceptedRules.has(n)) { + reason = `rule text is a duplicate of an earlier addition in this delta` + rejected.push({ entry: a, reason }) + continue + } + // Names a PR, URL, person, or docs page. The URL clause keeps docs-check-links.yml green. if (String(a.rule).match(/#\d{2,}|https?:\/\/|@[A-Za-z0-9-]|packages\/kilo-docs|\.md\b/)) { reason = "rule names a PR, URL, person, or docs page" @@ -275,6 +288,8 @@ export function validateDelta(delta, { existing, candidateSources, deletedInWind continue } + acceptedIds.add(a.id) + acceptedRules.add(n) valid.push({ id: a.id, rule: clean(String(a.rule)).replaceAll("\n", " "), @@ -369,6 +384,13 @@ async function apply() { // --- extraction mode --- async function extract() { + // Step 0: seed the prompt artifacts from the checked-out file before any fallible + // work. Every later step can throw, the workflow step is continue-on-error, and + // triage and edit read only these two files. Without the seed one failed API call + // silently drops every learned rule for the whole run. Later steps replace them + // with the rolling-branch copy and then with the validated delta. + writePromptArtifacts(parseLearnings(readFileOrEmpty(LEARNINGS_FILE))) + // Load fixture when DOCS_SYNC_FIXTURE is set. const fixturePath = process.env.DOCS_SYNC_FIXTURE let fixture = null @@ -428,11 +450,7 @@ async function extract() { let existing = [] let existingText = "" if (fixture) { - try { - existingText = fs.readFileSync(LEARNINGS_FILE, "utf8") - } catch { - // file absent - } + existingText = readFileOrEmpty(LEARNINGS_FILE) existing = parseLearnings(existingText) } else { try { @@ -450,6 +468,10 @@ async function extract() { } log(`existing entries: ${existing.length}`) + // Replace the seed with the rolling-branch copy. Every step below can throw, and + // these two files are all triage and edit read. + writePromptArtifacts(existing) + // Step 3: parse marker. Trust only when authored by github-actions[bot] (like watermark.mjs:35). let commitWm = null let commentWm = null @@ -785,11 +807,24 @@ function writeEmptyStateArtifacts(entries) { writePromptArtifacts(entries) } +// A later call must be able to shrink a seeded block back to nothing, so an empty +// block removes the file instead of leaving the earlier content in place. function writePromptArtifacts(entries) { - const triage = promptBlock(entries, "triage") - if (triage) fs.writeFileSync(`${OUT_DIR}/learnings-triage.md`, triage) - const edit = promptBlock(entries, "edit") - if (edit) fs.writeFileSync(`${OUT_DIR}/learnings-edit.md`, edit) + writeOrRemove(`${OUT_DIR}/learnings-triage.md`, promptBlock(entries, "triage")) + writeOrRemove(`${OUT_DIR}/learnings-edit.md`, promptBlock(entries, "edit")) +} + +function writeOrRemove(file, text) { + if (text) fs.writeFileSync(file, text) + else fs.rmSync(file, { force: true }) +} + +function readFileOrEmpty(file) { + try { + return fs.readFileSync(file, "utf8") + } catch { + return "" + } } async function patchOrLogMarker({ prBody, prNumber, marker, fixture, patchFile }) { @@ -811,8 +846,18 @@ async function patchOrLogMarker({ prBody, prNumber, marker, fixture, patchFile } } // Live PATCH: body-only, one line changed. The job already holds pull-requests: write. + // Re-read the body first. The body in hand was fetched before the extraction call, so + // patching that copy would drop any edit made in the minutes since. GitHub has no + // conditional update for a pull request body, so a short fetch-to-PATCH race remains. const { api, repo } = await import("./lib.mjs") - const newBody = patchMarkerIntoBody(prBody, marker) + let latestBody = prBody + try { + const fresh = await api(`/repos/${repo()}/pulls/${prNumber}`) + latestBody = fresh.body ?? "" + } catch (err) { + warn(`could not re-read PR #${prNumber} before the marker PATCH: ${err.message}. Using the earlier body.`) + } + const newBody = patchMarkerIntoBody(latestBody, marker) await api(`/repos/${repo()}/pulls/${prNumber}`, { method: "PATCH", body: { body: newBody }, diff --git a/.github/docs-sync/lib.mjs b/.github/docs-sync/lib.mjs index 6aaf9dc6c36..f63155adf66 100644 --- a/.github/docs-sync/lib.mjs +++ b/.github/docs-sync/lib.mjs @@ -9,7 +9,9 @@ import { spawnSync } from "node:child_process" import fs from "node:fs" -const API = "https://api.github.com" +// Test hook: DOCS_SYNC_API_BASE points the API at a local stub server. The workflow +// never sets it — only selftests do. +const API = process.env.DOCS_SYNC_API_BASE || "https://api.github.com" const MAX_RETRIES = 3 export function token() { diff --git a/.github/docs-sync/selftest.mjs b/.github/docs-sync/selftest.mjs index d6d552f5b32..8341b0ef9ad 100644 --- a/.github/docs-sync/selftest.mjs +++ b/.github/docs-sync/selftest.mjs @@ -7,12 +7,13 @@ */ import assert from "node:assert/strict" -import { execFileSync, spawnSync } from "node:child_process" +import { execFileSync, spawn, spawnSync } from "node:child_process" import fs from "node:fs" import os from "node:os" import path from "node:path" import { fileURLToPath } from "node:url" +import { sleepSync } from "./lib.mjs" import { mergeOrFallback, DEFAULT_BRANCH } from "./prepare-branch.mjs" import { applyCap } from "./watermark.mjs" import { @@ -3076,7 +3077,13 @@ Just prose, not a rule line. const kiloDir = makeStubKiloDir({ mode: "extraction-delta", callLog: path.join(cwd, "kilo-calls.log") }) writeExtractionDelta(cwd, { add: [ - { id: "dry-suppress", rule: "A rule suppressed under dry run.", scope: "both", source: tipSource, date: "2026-08-03" }, + { + id: "dry-suppress", + rule: "A rule suppressed under dry run.", + scope: "both", + source: tipSource, + date: "2026-08-03", + }, ], remove: [], }) @@ -3120,7 +3127,13 @@ Just prose, not a rule line. const kiloDir = makeStubKiloDir({ mode: "extraction-delta", callLog: path.join(cwd, "kilo-calls.log") }) writeExtractionDelta(cwd, { add: [ - { id: "nopatch-suppress", rule: "A rule suppressed under no-patch.", scope: "both", source: tipSource, date: "2026-08-03" }, + { + id: "nopatch-suppress", + rule: "A rule suppressed under no-patch.", + scope: "both", + source: tipSource, + date: "2026-08-03", + }, ], remove: [], }) @@ -3153,6 +3166,201 @@ Just prose, not a rule line. } } + // 10s — a failed API call must not disable the existing learnings + // The learn step is continue-on-error, and triage and edit read only the two prompt + // artifacts. So learn.mjs must write them before the first call that can throw. + { + console.log(" 10s — prompt artifacts survive an API failure") + const dir = mktemp("docs-sync-learn-s-") + const learningsPath = path.join(dir, "packages", "kilo-docs", "LEARNINGS.md") + fs.mkdirSync(path.dirname(learningsPath), { recursive: true }) + const seeded = [ + { + id: "seeded-rule", + rule: "Do not document features behind experimental flags.", + scope: "both", + source: "commit:aaaaaaa", + date: "2026-08-01", + }, + ] + fs.writeFileSync(learningsPath, renderLearnings(seeded)) + + // No DOCS_SYNC_FIXTURE and an empty GITHUB_REPOSITORY: repo() throws inside + // extract(). It stands for any API failure before the artifacts exist. + const failEnv = { + TRIAGE_MODEL: "test/model", + GITHUB_REPOSITORY: "", + GITHUB_OUTPUT: path.join(dir, "gh-output-s"), + GITHUB_STEP_SUMMARY: path.join(dir, "gh-summary-s"), + DOCS_SYNC_BACKOFF_MS: "0", + } + const result = runNodeScript(LEARN_SCRIPT, { cwd: dir, env: failEnv }) + assert.notEqual(result.status, 0, "extraction must fail without GITHUB_REPOSITORY") + + const triagePath = path.join(dir, "docs-sync-out", "learnings-triage.md") + const editPath = path.join(dir, "docs-sync-out", "learnings-edit.md") + for (const f of [triagePath, editPath]) { + assert.ok(fs.existsSync(f), `${path.basename(f)} must survive the failure`) + assert.ok( + fs.readFileSync(f, "utf8").includes("Do not document features behind experimental flags."), + `${path.basename(f)} must carry the checked-out rule`, + ) + } + + // An empty file must clear the stale block, not leave the earlier rule in place. + fs.writeFileSync(learningsPath, renderLearnings([])) + runNodeScript(LEARN_SCRIPT, { cwd: dir, env: failEnv }) + assert.ok(!fs.existsSync(triagePath), "an empty learnings file must remove learnings-triage.md") + assert.ok(!fs.existsSync(editPath), "an empty learnings file must remove learnings-edit.md") + } + + // 10t — the direct marker PATCH must not overwrite a concurrent body edit + // The body read at step 1 predates the extraction call, so learn.mjs must re-read + // the body immediately before the PATCH. + { + console.log(" 10t — marker PATCH preserves a concurrent body edit") + const dir = mktemp("docs-sync-learn-t-") + initRepoWithIdentity(dir) + fs.writeFileSync(path.join(dir, "base.txt"), "base\n") + gitIn(dir, ["add", "base.txt"]) + gitIn(dir, ["commit", "-m", "base"]) + gitIn(dir, ["checkout", "-b", "docs/auto-sync"]) + + // github-actions[bot] authored the only branch commit, so there is no candidate + // correction and no model call. The run goes straight to the direct marker PATCH. + const learningsPath = path.join(dir, "packages", "kilo-docs", "LEARNINGS.md") + fs.mkdirSync(path.dirname(learningsPath), { recursive: true }) + fs.writeFileSync(learningsPath, renderLearnings([])) + gitIn(dir, ["add", "packages/kilo-docs/LEARNINGS.md"]) + gitIn(dir, ["commit", "-m", "seed learnings", "--author", `github-actions[bot] <${githubBotEmail}>`]) + gitIn(dir, ["remote", "add", "origin", dir]) // learn.mjs fetches origin itself + const cwd = setupLearnRepo(dir) + const tip = gitIn(dir, ["rev-parse", "HEAD"]) + + // Stub GitHub API. The second read of the pull request returns the maintainer edit. + const serverDir = mktemp("docs-sync-api-t-") + const portFile = path.join(serverDir, "port") + const patchFile = path.join(serverDir, "patch.json") + const serverScript = path.join(serverDir, "server.cjs") + fs.writeFileSync( + serverScript, + `const fs = require("node:fs") +const http = require("node:http") +let reads = 0 +const json = (res, data) => { + res.writeHead(200, { "content-type": "application/json" }) + res.end(JSON.stringify(data)) +} +const server = http.createServer((req, res) => { + let raw = "" + req.on("data", (c) => (raw += c)) + req.on("end", () => { + if (req.method === "PATCH") return fs.writeFileSync(process.env.PATCH_FILE, raw), json(res, {}) + if (req.url.startsWith("/search/issues")) return json(res, { items: [{ number: 1 }] }) + if (req.url.includes("/comments")) return json(res, []) + if (req.url.includes("/pulls/1")) { + const body = reads++ === 0 ? process.env.BODY_BEFORE : process.env.BODY_AFTER + return json(res, { + number: 1, + body, + head: { ref: "docs/auto-sync" }, + user: { login: "github-actions[bot]" }, + }) + } + json(res, {}) + }) +}) +server.listen(0, "127.0.0.1", () => fs.writeFileSync(process.env.PORT_FILE, String(server.address().port))) +`, + ) + + const bodyBefore = "Rolling PR body.\n\n" + const humanEdit = "A maintainer edited the body while extraction ran." + const child = spawn(process.execPath, [serverScript], { + stdio: "ignore", + env: { + ...process.env, + PORT_FILE: portFile, + PATCH_FILE: patchFile, + BODY_BEFORE: bodyBefore, + BODY_AFTER: bodyBefore + humanEdit + "\n", + }, + }) + + try { + let port = "" + for (let i = 0; i < 100 && !port; i++) { + if (fs.existsSync(portFile)) port = fs.readFileSync(portFile, "utf8").trim() + else sleepSync(50) + } + assert.ok(port, "the stub API server must report a port") + + const result = runNodeScript(LEARN_SCRIPT, { + cwd, + env: { + TRIAGE_MODEL: "test/model", + GITHUB_REPOSITORY: "acme/repo", + GH_TOKEN: "stub-token", + DOCS_SYNC_API_BASE: `http://127.0.0.1:${port}`, + GITHUB_OUTPUT: path.join(dir, "gh-output-t"), + GITHUB_STEP_SUMMARY: path.join(dir, "gh-summary-t"), + LEARNINGS_BUDGET_MINUTES: "1", + DOCS_SYNC_BACKOFF_MS: "0", + }, + }) + assert.equal(result.status, 0, `learn.mjs must succeed against the stub API: ${result.output}`) + + assert.ok(fs.existsSync(patchFile), "the run must PATCH the pull request body") + const patchedBody = JSON.parse(fs.readFileSync(patchFile, "utf8")).body + assert.ok(patchedBody.includes(humanEdit), "the concurrent body edit must survive the marker PATCH") + assert.ok(patchedBody.includes(tip), "the PATCH must carry the new tip SHA") + assert.ok(!patchedBody.includes("commit=old"), "the old marker must be replaced") + assert.equal( + (patchedBody.match(/