fix(ci): docs-sync bot — no errors, no timeouts, no lost PRs - #12580
Conversation
Code Review SummaryStatus: No Issues Found | Recommendation: Merge The incremental change (commit Files Reviewed (1 file)
Previous Review Summary (commit 3316b3b)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 3316b3b)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
The core fix logic (watermark-holds-back invariant, git-identity-before-merge, Files Reviewed (8 files)
Reviewed by claude-sonnet-5 · Input: 26 · Output: 3.2K · Cached: 547.7K Review guidance: REVIEW.md from base branch |
…g#12580) * fix(ci): configure git identity before the docs-sync merge and classify merge failures * fix(ci): hold the docs-sync watermark back until every PR has an outcome * test(ci): self-check for the docs-sync failure paths * fix(ci): isolate PR selftest concurrency from the daily docs-sync run
What this fixes
The docs-sync bot has been 100% broken since 2026-07-25 — every scheduled run dies at
Prepare rolling branch(runs 30149606876, 30193436440, 30248635269) — and the last run that produced output (30122603016) silently lost 36 of 46 docs-worthy PRs. Three defects, all reproduced before this fix was written:upsert-pr.mjs, afterprepare-branch.mjsmergesorigin/main. On Blacksmith runners (hostname with no domain) identity auto-detection fails, the merge aborts, the failure is misclassified as a conflict, andgit merge --abortthrows because no merge is in progress → exit 1, whole job dead.kilo runcan exit 0 after doing nothing (event stream ends mid-turn).execFileSynccannot recover piped stderr on exit 0, so the diagnostic was discarded; failed PRs were recorded asskippedand rendered under "Considered, no docs change needed" (30 such rows sit in docs: auto-sync with merged PRs (through 2026-07-29) #12521 today); and theprocessed-throughwatermark advanced past them unconditionally — they were never re-collected.kilo runinFix verify failures. Atimeout-minutescancellation discards completed doc edits with no PR and no watermark advance.The fix — one invariant
The watermark never moves past a PR that has no terminal outcome (terminal :=
action !== "pending"; a deliberate agent-writtenskippedis terminal).processed-throughisnowwhen every worthy PR is covered; otherwise the earliest uncovered PR'smerged_at− 1 ms, socollect.mjs's existingmerged:>=sincequery re-collects the uncovered PRs on the next run. No retry queue, no attempt counters, no new state anywhere.Around that invariant:
kilo rungoes through onespawnSyncwrapper so stderr is recoverable even on exit 0; exit 0 is no longer trusted — a missing summary file or unparseable output is a failure logged with the stderr tail and exit code on every attempt; 3 attempts with 60s/300s backoff.Pending — will retrysection (informational only, 60-row display cap).Update docsiscontinue-on-errorso a failed edit step can no longer discard written edits.::warning::, timestamps only) when it abandons PRs, and no longer clamps an explicitsinceoverride (a hidden expiry would have started eating the recovery set on 2026-08-01).since-override replay that strands PRs (uncovered + no body rewrite) emits an explicit::warning::from both producers that can see it (triage.mjswhenworthy == 0,upsert-pr.mjsotherwise) — the scheduled path needs no warning because the untouched marker already holds the window..github/docs-syncfrom the dispatched ref afterprepare-branchswitches branches — without this, a dispatch from a fix branch would silently executemain's scripts (git restore --source=, notcheckout --, so the restored files can't be swept into the docs commit).selftest.mjs(plainnode:assert, no network/LLM/dependencies) proves all of the above offline: merge classification, stderr-on-exit-0 recovery, the watermark invariant six ways plus the unresolvable-merged_atfallback, routing and body round-trip with marker-forgery guards, the legacy-row scrub, re-collection arithmetic, budgets with a call-recording stub, both cap arms, and the triage-side summary + replay warning (including the false-positive arm). It runs as a read-only, secretless job on PRs that touch the bot, and as the first step of the daily job.Two deliberate simpler-shape decisions
merged:>=sincecollection query and the existing 14-day cap. A held-back watermark has none of those failure modes because it adds no mechanism. ThePendingtable is purely informational; the watermark, not the table, is the retry mechanism.kilo rungenuinely reports success after an incomplete run — two distinct paths: the event stream ending beforesession.status: idle(packages/opencode/src/cli/cmd/run.ts:731,833-838), and a mid-stream provider error that never setserrorbecausesession.erroris ignored when its session id doesn't match (run.ts:814);packages/opencode/test/cli/run/run-process.test.ts:49-65locks today's behaviour in deliberately, so flipping it is a debatable repo-wide contract change requiring its own announced decision. It also cannot be observed by this workflow (the bot installs the published@kilocode/cli), and the bot is broken now — bundling the two risks stalling the urgent repair. Nothing here depends on it: the bot no longer trusts exit codes at all.Verification
node .github/docs-sync/selftest.mjs— 8 case groups, all green (also runs as thedocs-sync / selftestcheck on this PR).bun run script/check-workflows.ts— ok;oxlint .github/docs-sync— 0 errors.dry_run=true— green: selftest ran insidesync(branch-code proof), watermark resolved from docs: auto-sync with merged PRs (through 2026-07-29) #12521's marker, 60 PRs collected, triage 60/8 worthy/0 pending.since=2026-07-17replay — success in 76 min (< 120 cap):Prepare rolling branchsucceeded (Defect A's crash step),Restore docs-sync scriptsran,Upsert rolling PRran. The upstreamkilo runstream failures recurred (all 11 edit batches failed) — and this time every failure logged its stderr tail and exit code, the 51 edit failures + 54 triage-pending PRs were recorded pending (never "skipped"), the partial doc edits the dying agents had already written were committed (13 files) instead of discarded, and the watermark was held back to2026-07-17T01:19:29.999Z(earliest uncoveredmerged_at− 1 ms) so the next scheduled run re-collects all 105 uncovered PRs automatically. The 30 misclassified "Considered" rows in docs: auto-sync with merged PRs (through 2026-07-29) #12521 were scrubbed; its body now carries aPending — will retrysection naming each PR with its real cause. The CLI-side root cause (stream ends mid-turn while the CLI still exits 0) is the deliberate follow-up described above — the bot now survives it and reports it.Out of scope (recorded so they are not silently forgotten): normalising inconsistent
Docs changepaths in the PR body; recovering rows already lost toROW_CAP(only the 30 surviving rows are recovered, via the replay); the per-PR fetch-failure drop path incollect.mjs(already visible via::warning::+ counter); the zero-worthy-day marker advance (pre-existing, practically unreachable).