Conversation
Consumers hold a copied automation-template.yml, and nothing checked that the copies still matched. A stale copy fails silently: the repo keeps running its old on: block, so a new automation never fires there. Six of the eight drifted within days of migrating, because the template changed after their pull requests were opened. That is the case this addresses. The workflow compares each consumer's copy against the template and opens one pull request per repo that differs. A repo already in sync gets nothing, and a repo with a sync pull request open gets it updated rather than duplicated. It only proposes: it opens pull requests on a branch, never commits to a default branch, and never merges, approves, or enables auto-merge. The consumer list lives in automation-registry.yml, next to the automations it distributes. Each entry carries the branch to target, because most consumers do not use main. kolibri-design-system also carries a body prefix, because its check-description job fails unless the description holds a Changelog block. A repo whose own tooling rewrites the copy would generate a pull request forever, so a merged sync pull request followed by fresh drift is reported as a toolchain conflict instead.
🟡 Waiting for changesLast updated: 2026-09-23 19:13 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #97 is registry-driven and propose-only, as #95 specified. Two script bugs surface only in production. The rest are resilience and test coverage.
- blocking — every consumer that merges a sync PR stops receiving sync PRs (
sync-automation-template.js:88) - blocking — a repo the app cannot read is reported
not-migratedand the run exits 0 (sync-automation-template.js:61) - suggestion — one transient API failure ends the run with no report (
:140) - suggestion — the fixed branch is reused after a maintainer closes a sync PR unmerged (
:97) - suggestion — no tests for the state machine (
:79) - nitpick — no
concurrencygroup on a workflow that writes (sync-automation-template.yml:19)
CI pending. No UI surface in the diff. The frontend lens produced no findings, and UI verification and manual QA did not apply.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran a phased review pipeline over the pull request diff:
- Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
- Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
- Specialized frontend/backend review passes applied framework-specific lenses where those files changed
- For UI changes: manual QA and an accessibility audit against a live dev server, when available
- Checked CI status and linked issue acceptance criteria
- Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence
| if (copy.content === template) return { repo, state: 'in-sync' }; | ||
|
|
||
| const existing = await openSyncPr(repo); | ||
| if (!existing && (await revertedAfterMerge(repo))) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: Every consumer that merges a sync PR stops receiving sync PRs from then on. revertedAfterMerge returns true for any merged prior sync PR (line 76). It never compares the consumer's copy against what that PR installed. So the next upstream template change is classified toolchain-conflict rather than ordinary drift. No PR is opened and the run exits 1 (line 156). #95 also asks for that report only after a revert-and-reopen happens more than once.
Please tell the two cases apart using the template's history. One option: compare the merge time against the last commit touching automation-template.yml (GET /repos/learningequality/.github/commits?path=automation-template.yml&per_page=1), where a later template commit means ordinary drift. The other: record the synced template SHA in the PR body and read it back.
|
|
||
| async function currentCopy(repo, ref) { | ||
| const r = await api('GET', `/repos/${ORG}/${repo}/contents/${TARGET_PATH}?ref=${ref}`); | ||
| if (r.status === 404) return { missing: true }; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: A repo the app cannot access is reported as not-migrated and the run still exits 0. GitHub returns 404, not 403, for a repo the token cannot see. That 404 maps to not-migrated (line 84), which is excluded from problems (line 149). So there is no ::error annotation and the overall verdict reads "in sync" for a repo the workflow never reached. #95 names this case directly: "A repo the workflow cannot write to is reported, not skipped silently."
Please probe with GET /repos/{ORG}/{repo} to disambiguate. Every entry in consumers exists, so a 404 there means no access and should map to error. All eight repos are migrated per #88, so also annotate not-migrated and exit non-zero rather than printing a quiet row.
| const template = fs.readFileSync(TEMPLATE_PATH, 'utf8'); | ||
|
|
||
| const results = []; | ||
| for (const consumer of registry.consumers) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: One transient API failure ends the run with no report at all. There is no per-repo error handling and no catch in main(). api() calls JSON.parse(text) unconditionally (line 42), so GitHub's HTML 502 page throws. The loop then unwinds as an unhandled rejection and the accumulated results never print.
Please wrap syncRepo in a try/catch that records { state: 'error', detail }. That keeps the other seven repos moving and preserves the table. Guarding the JSON.parse with a raw-text fallback makes the detail readable.
Separately, openSyncPr returns null when its list request fails (line 68). A 403 then reads as "no open PR", and the later POST /pulls fails with a 422 that hides the real cause.
| if (!baseRef.ok) return { repo, state: 'error', detail: `base ${base} not found` }; | ||
|
|
||
| if (!existing) { | ||
| const made = await api('POST', `/repos/${ORG}/${repo}/git/refs`, { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: After a maintainer closes a sync PR without merging, the next run re-proposes the same change from the same branch. BRANCH is a constant and ref creation tolerates 422 (line 101), so the branch outlives the closed PR. revertedAfterMerge is false in that case. The new PR therefore carries the stale commits and re-asks weekly for a change a human declined.
Please reset the ref to the base SHA (PATCH /git/refs/heads/{BRANCH} with force: true) whenever no PR is open. For the re-proposal itself, skip repos whose most recent sync PR was closed unmerged.
| return Boolean(r.data[0].merged_at); | ||
| } | ||
|
|
||
| async function syncRepo(consumer, template) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: The state classification in syncRepo has no test coverage. It is pure given a stubbed api. scripts/pr-statistics.test.js already establishes the injected-client pattern that node --test scripts/ runs in CI. Both blocking findings above are reachable only in production because #95's test plan is manual across eight live repos.
Please make api injectable and assert each state.
| - automation-template.yml | ||
| permissions: | ||
| contents: read | ||
| jobs: |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: Two overlapping runs can both try to open the sync PR, and the loser reports a 422. A push to main touching automation-template.yml can start while the weekly cron is still running. Both see no open PR and both POST /pulls.
Please add concurrency: { group: sync-automation-template, cancel-in-progress: false }.
| base: develop | ||
| # check-description fails unless the body carries a Changelog block whose | ||
| # Description line is not the template placeholder. | ||
| body_prefix: | |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: KDS's check-description exception lives in the registry as a data field, not as a repo-name branch in the script. The next consumer needing a description check is a data change.
A sync pull request now requests a review from rtibblesbot when it is opened, so it gets a first pass before a core maintainer looks at it. Dependabot pull requests already work this way. A failed reviewer request does not fail the sync. The pull request is already open at that point, so the run notes it and carries on.
A merged sync pull request made every later template change look like a toolchain conflict, so a repo would have received one sync and then silently stopped receiving them. Drift is now classified against the time the template last changed here. A change after the pull request closed is ordinary drift. Without one, a merged pull request means the consumer reverted the file, and a closed one means a maintainer declined it. A declined repo is left alone until the template moves again. GitHub answers 404 both for a missing file and for a repo the token cannot see, and the second case was reported as not-migrated and exited 0. A 404 now probes the repo itself to tell the two apart, and not-migrated counts as a problem, because every listed consumer is already migrated. One failed request used to end the run with no output at all. Each repo is now wrapped, a response that is not JSON no longer throws, and a failed pull request listing reports an error instead of reading as no open pull request. The sync branch is reset to base when no pull request is open, so a closed one cannot leave commits behind for the next run to carry. The state machine has tests with an injected client, covering each state. Both of the bugs above were reachable only in production.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #97: 4 new blocking findings and 1 suggestion, all in code added since the last round (see inline). All 7 prior findings are resolved or acknowledged.
The state machine is right now, and the suite covers every classification. What is missing sits around it. The one call the classification depends on has no error path. The suite never asserts a write payload or the exit-code function. The operator runbook no longer lists every state that stops a merge.
CI passing. No UI files, so no visual verification or manual QA applied.
Prior-finding status
RESOLVED - scripts/sync-automation-template.js:88 - every consumer that merges a sync PR stops receiving sync PRs
RESOLVED - scripts/sync-automation-template.js:61 - inaccessible repo reported as not-migrated with exit 0
RESOLVED - scripts/sync-automation-template.js:185 - one transient API failure ends the run with no report
RESOLVED - scripts/sync-automation-template.js:97 - closed-unmerged PR re-proposed from a stale branch
RESOLVED - scripts/sync-automation-template.js:79 - no test coverage for the state classification
RESOLVED - .github/workflows/sync-automation-template.yml:22 - overlapping runs both POST /pulls
ACKNOWLEDGED - automation-registry.yml:38 - KDS description exception lives in the registry
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Ran the same phased review passes as a first review (core, frontend/backend lenses, manual QA when required)
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
|
|
||
| async function lastTemplateChange(api) { | ||
| const r = await api('GET', `/repos/${ORG}/.github/commits?path=automation-template.yml&per_page=1`); | ||
| if (!r.ok || !r.data.length) return null; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: A transient API failure here silently stops syncing a repo. lastTemplateChange has no error path, and every consumer's classification depends on its result.
A non-ok response returns null. classifyDrift then skips the timestamp comparison, and every consumer with a merged sync pull request falls through to toolchain-conflict. No pull request opens and the run exits 1. The annotation tells maintainers the template is not stable under that toolchain — for a 502.
A throw (null body, socket hang-up, missing commit) is worse. The call sits outside the per-repo try at line 186, and main() has no .catch(). The process dies with no table and no annotations.
Fail toward drift, not toward a stop. An unknown template date costs a human one review. A wrong toolchain-conflict stops syncing that repo and points the maintainer at the wrong file. Distinguish a failed request from "no commits found" — only the latter justifies null — and wrap the call in the same try/catch shape used at line 186.
| const test = require('node:test'); | ||
| const assert = require('node:assert/strict'); | ||
|
|
||
| const { run, classifyDrift, BRANCH } = require('./sync-automation-template'); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: The prior round's exit-code fix can regress with CI green. report and PROBLEM_STATES are exported (sync-automation-template.js:224) and never exercised by a test.
report is the entire failure signal: main() does process.exit(report(results) ? 1 : 0), and the workflow step is a bare node scripts/sync-automation-template.js. Mutating PROBLEM_STATES to ['error'] leaves all 14 tests passing. That mutation reinstates the prior blocking finding — an inaccessible repo exits 0.
Feed report a hand-built mixed result set (in-sync, declined, not-migrated, error) and assert the returned count. Name declined explicitly there: it is deliberately not a problem state, and that decision should be a test rather than an omission. Add a case that sets reviewerFailed, which no test currently does.
| assert.equal((await only(baseRoutes(TEMPLATE))).state, 'in-sync'); | ||
| }); | ||
|
|
||
| test('a drifted copy opens a pull request and requests the reviewer', async () => { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: No test asserts a write payload. A commit to the consumer's default branch would pass the suite.
Both mutations are green against the current suite (14 pass, 0 fail):
- PUT body
branch: BRANCH->branch: 'main' - PUT body
content: <template>->content: 'WRONG'
The first breaks the hard requirement from #95 — never commit to a default branch. On eight consumers it commits straight to develop, unstable and release-v0.9.x. The second means the suite cannot tell a correct sync from one that writes garbage.
calls is already captured here, so assert both fields:
const put = calls.find((c) => c.method === 'PUT');
assert.equal(put.body.branch, BRANCH);
assert.equal(Buffer.from(put.body.content, 'base64').toString('utf8'), TEMPLATE);Dropping sha: onBranch.sha from that body is also green, and fails for real with a 422 on every update. Assert it too, in the update test at line 60.
|
|
||
| Run it with `dry_run` to see which repos have drifted without opening anything. | ||
|
|
||
| Two results need a core maintainer rather than a merge: |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: This section promises "Two results", but the code now emits four states. Two of them are undocumented.
not-migratedjoinedPROBLEM_STATES(sync-automation-template.js:28). It emits::error title=<repo>::not-migratedand exits 1. A maintainer who sees the weekly run go red on that word finds nothing here.declinedis new. It stops sync for that repo until the template next moves, and exits 0. A repo can sit drifted with a green workflow, and the docs never say that closing a sync pull request has that effect.
Document both and fix the count. For not-migrated, the fix is installing or onboarding the app. For declined, reopening sync means waiting for the next template change or opening the pull request by hand.
|
|
||
| const TEMPLATE = 'name: Automation\non: {}\n'; | ||
| const STALE = 'name: Automation\non: {old: true}\n'; | ||
| const REGISTRY = { consumers: [{ repo: 'demo', base: 'main' }] }; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: The fixture pins base: 'main', the one value that cannot catch a hardcoded base. Only three of the eight real consumers use main; the base field exists because five do not. Today both GET git/ref/heads/${base} -> heads/main and POST /pulls base -> 'wrong' stay green.
Switch the fixture and its route to base: 'develop'. That costs nothing and makes both fail. Assert the POST /pulls body at the same time: reviewers: ['nobody'] and ignoring body_prefix are both green today, and the second silently fails KDS's check-description on every run.
| async function readCopy(api, repo, ref) { | ||
| const r = await api('GET', `/repos/${ORG}/${repo}/contents/${TARGET_PATH}?ref=${ref}`); | ||
| if (r.ok) return { sha: r.data.sha, content: Buffer.from(r.data.content, 'base64').toString('utf8') }; | ||
| // A repo the token cannot see answers 404, exactly like a missing file, so ask |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: The comment names the ambiguity — missing file and invisible repo both answer 404. That makes the extra request read as necessary rather than defensive. Same for the reset comment at line 146.
A failed commits request returned null, which made classifyDrift skip the comparison and label every consumer with a merged sync pull request a toolchain conflict. A transient error would have stopped syncing those repos and told maintainers their tooling rewrites the file. The call also sat outside the per-repo error handling, so a malformed body killed the run with no table. An unknown template date now resolves to ordinary drift. A needless pull request costs one review, where a wrong toolchain conflict stops the repo entirely. The suite now asserts the things that decide correctness rather than only the classification. The write must target the sync branch, carry the template, and keep the file sha on an update. The pull request must target the consumer's own base and name the reviewer, and a body prefix must reach the body, because kolibri-design-system fails its description check without one. The fixture base moved off main, which is the one value that cannot catch a hardcoded base. report and PROBLEM_STATES are tested directly, since they decide whether the workflow goes red. Narrowing PROBLEM_STATES now fails a test rather than quietly reinstating an inaccessible repo exiting 0. The docs listed two states needing attention where the code emits four.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #97 — 12 of 13 prior findings are resolved. One stays open, a nitpick, recorded below. CI is passing. No UI files, so no visual verification.
The new tests hold under mutation. Adding 'declined' to PROBLEM_STATES fails a test. Reverting the null-date handling in classifyDrift fails another. One new suggestion inline: a test that only restates the constant it checks.
.github/workflows/sync-automation-template.yml:22 — nitpick (unchanged since the last round, recorded not re-argued): two overlapping runs can both open the sync PR, and the loser reports a 422. Add a concurrency group to the workflow, or leave it as-is if the duplicate run is rare enough that the 422 is just noise.
Prior-finding status
RESOLVED — scripts/sync-automation-template.js — revertedAfterMerge stopped every merging consumer from receiving further sync PRs
RESOLVED — scripts/sync-automation-template.js — an inaccessible repo was reported not-migrated with exit 0
RESOLVED — scripts/sync-automation-template.js:198 — one transient API failure ended the run with no report
RESOLVED — scripts/sync-automation-template.js — lastTemplateChange had no error path, silently stopping a repo
RESOLVED — scripts/sync-automation-template.js — a declined sync PR was re-proposed on the next run
RESOLVED — scripts/sync-automation-template.test.js — state classification in syncRepo had no test coverage
RESOLVED — scripts/sync-automation-template.test.js — report/PROBLEM_STATES untested, so the exit-code fix could regress green
RESOLVED — scripts/sync-automation-template.test.js:56 — no test asserted the write payload
RESOLVED — scripts/sync-automation-template.test.js — fixture pinned base: 'main', the one value that cannot catch a hardcoded base
RESOLVED — docs/automation.md — the section promised "Two results" while the code emits four
ACKNOWLEDGED — scripts/sync-automation-template.js:74 — praise: the comment names the missing-file/invisible-repo 404 ambiguity
ACKNOWLEDGED — automation-registry.yml:38 — praise: KDS check-description exception lives in the registry as data
UNADDRESSED — .github/workflows/sync-automation-template.yml:22 — overlapping runs both open the sync PR, loser 422s
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Ran the same phased review passes as a first review (core, frontend/backend lenses, manual QA when required)
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| } | ||
| }); | ||
|
|
||
| test('declined is deliberately not a problem state', () => { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Delete this test — it restates the PROBLEM_STATES declaration and catches nothing the report test above already catches.
That test (line 212) feeds report() a declined row alongside all three problem states. Adding 'declined' to PROBLEM_STATES fails it on its own. Removing 'not-migrated' fails it too. I confirmed both by mutation.
The one case unique to this test is adding a genuinely new problem state, which fails the deepEqual even after the report test has been updated to cover it. That leaves a list to edit in two places and no defect caught in between.
If the point is that the set is closed and any addition should force a second look, keep it and say that in an assertion message. With no message it reads as a mirror of the declaration.
| assert.equal(calls.filter((c) => c.method !== 'GET').length, 0); | ||
| }); | ||
|
|
||
| test('an unreadable template history proposes rather than stopping the repo', async () => { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: This one pins the outcome rather than the mock. It stubs the history call to fail while keeping a merged closed PR in the routes. So state === 'opened' can only pass if the unknown date really resolves to drift, not toolchain-conflict. Reverting the null-date branch fails it.
The report test already fails when a state is added to or removed from PROBLEM_STATES, so the membership assertion next to it caught nothing. What is left is the closed-set check, which only fires when someone adds a state the report test does not cover. The assertion message now says that, instead of the test reading as a copy of the declaration.
|
The concurrency group is already in the workflow. It landed in 06b3ee1, two commits before this review: concurrency:
group: sync-automation-template
cancel-in-progress: falseThe previous review listed the same finding as resolved at the same line, so this one looks like a stale status rather than a new observation. The test suggestion is done in 4cc6deb. The membership assertion was redundant, as you showed, so it is gone. The closed-set check stays, now with a message saying it guards against widening |
body_prefix could only put text above the generated explanation, so the
Changelog block led the body. kolibri-design-system#1327 shows that repo's real
shape: Description first, Changelog fifth, Steps to test after it. A prefix
cannot express that, because the explanation belongs inside the first section
and the Changelog in the middle.
body_template replaces it. The generated text goes where {{explanation}}
appears, so the body follows the repo's own section order. A consumer without a
template still gets the plain explanation.
kolibri-design-system now gets a body shaped like its template. The parser in
check-description still finds the Description line, because it only needs that
line between the Changelog heading and the next line holding two hashes.
akolson
changed the title
A sync pull request replaces one generated file with a known upstream file. The copy either matches automation-template.yml on main or it does not, and the script writes those bytes by construction, so there is no content question a reviewer could answer differently. A pre-review would post a generated comment on eight pull requests every time the template changes, saying nothing. The dependabot comparison that justified it does not hold either. A dependabot pull request carries a version jump with changelog and breaking-change risk. This one carries no judgment at all. What the core maintainer decides is whether to accept the file landing in their repo, which is consent rather than review. That step is unchanged.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #97 is approved. All 15 prior findings are resolved or acknowledged. The new commits raise no new findings. The four ACKNOWLEDGED items stay as they are — no action needed before merge.
Prior-finding status
RESOLVED — scripts/sync-automation-template.js — every consumer that merges a sync PR stops receiving sync PRs
RESOLVED — scripts/sync-automation-template.js — an inaccessible repo reported as not-migrated with exit 0
RESOLVED — scripts/sync-automation-template.js:194 — one transient API failure ends the run with no report
RESOLVED — scripts/sync-automation-template.js — a closed-without-merge sync PR is re-proposed every run
RESOLVED — scripts/sync-automation-template.js — syncRepo state classification has no test coverage
RESOLVED — scripts/sync-automation-template.js — lastTemplateChange has no error path
RESOLVED — .github/workflows/sync-automation-template.yml:22 — two overlapping runs race to open the same PR
RESOLVED — scripts/sync-automation-template.test.js — the exit-code fix can regress with CI green
RESOLVED — scripts/sync-automation-template.test.js — no test asserts a write payload
RESOLVED — scripts/sync-automation-template.test.js — the fixture pins base: 'main'
RESOLVED — docs/automation.md — "Two results" promised, four states emitted
ACKNOWLEDGED — scripts/sync-automation-template.test.js — delete the test that restates PROBLEM_STATES
ACKNOWLEDGED — scripts/sync-automation-template.test.js:192 — the history-failure test pins the outcome, not the mock
ACKNOWLEDGED — automation-registry.yml — the KDS exception lives in the registry as a data field
ACKNOWLEDGED — scripts/sync-automation-template.js:74 — the comment names the 404 ambiguity
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Ran the core review pass only
- Specialized frontend/backend lenses and manual QA run only when a review is explicitly requested
- Synthesized one review from the passes
- Chose the verdict from the findings, CI status, and QA evidence
The states section said four results need a core maintainer. Only three do. The fourth, declined, is the record of a maintainer having already acted, it keeps the run green, and it asks for nothing. The error entry sent readers to check permissions the app already holds. Permissions belong to the app and are granted once. What varies per repo is whether the app is installed there, which is what morango was missing, so the troubleshooting and the onboarding step now say that instead. The opening also said a consumer needs the file again only when the template changes, which contradicted the toolchain-conflict entry twenty lines below. A copy also stops matching when the consumer's own tooling rewrites it.
The docs said automations authenticate "as the bot" and told a maintainer to check that "the bot app" is installed. Two bot identities act in these repos, and only one is relevant here, so both places now name learning-equality-bot[bot] and tie it to LE_BOT_APP_ID. That is the app whose missing installation broke morango, and the name is what someone needs when they go looking in the organization's app settings. The section also reads more plainly than before, and its lines now wrap near 100 columns like the rest of the file.
The header already says body_template is for repos with their own template or a description check, and the Changelog block below the comment shows the rest.
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
Chatted on Slack - seems like searching for repos using automation.yml and then using their default branch for the PR will help avoid a hard coding here.
The consumer list had to be edited whenever a repo onboarded, and it carried a base branch per repo that could disagree with reality. The sync now walks the org's repos, skips archived ones and forks, and keeps every repo whose .github/workflows/automation.yml calls this repo's automation.yml. This repo's own reusable workflow sits at the same path and does not call it, so the marker excludes it without a special case. Each pull request targets the repo's default branch. That was never a free choice: GitHub evaluates workflow triggers only from the default branch, so any other value would have been wrong. The registry keeps a consumers block for per-repo overrides, which today is kolibri-design-system and its body template. Every other consumer needs no entry. The not-migrated state is gone. A repo without the file is not a consumer rather than a broken one, so nothing is reported and nothing turns red. A dry run against the org finds the same eight repos the list held, in about a minute for 120 candidates.
The body for kolibri-design-system was a copy of that repo's pull request template, stored here. Their template changes over time and ours would not, so the copy was a second thing to maintain in a change meant to remove exactly that. The sync now reads the consumer's pull request template when it has one, and falls back to a short explanation when it does not. Their template decides the shape, and it stays current on its own. Fields are answered rather than copied, because a template ships each one with instructions for a human author and a body full of "Choose from: yes / no" reads as an unanswered form. Description names the change, Products impact is none, anything else is a dash. That also satisfies a repo checking that the description is no longer the placeholder, which is what check-description does. For kolibri-design-system the result is the same text the stored copy produced, now derived rather than remembered. The consumers block leaves the registry, and the script no longer reads the registry at all.
Reading every consumer's pull request template served one repo. Only kolibri-design-system checks its description, so only it needs a body shaped like its template. The other seven take the plain explanation, which is shorter and says more than a filled form would. Its fields are now answered from a named map rather than a rule that looked general while being tuned to one repo. Products impact is none and the items below carry a dash, which is what that repo's own guidance asks for. A field the map does not name still gets a dash, so a field they add later does not arrive carrying instructions meant for a human author. Its template is still read rather than stored, so it stays current as they change it.
The comment explains why no special case is needed to keep this repo out of its own consumer list, which is a question that arises at the filter rather than at the constant. The constant name and its value already say what the marker is.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #97: all 15 prior findings resolved; 2 new suggestions inline.
CI passing. Discovery matches the old registry's base values.
- suggestion: one thrown
automation.ymlread during discovery ends the run with no report. - suggestion: the KDS sync PR body still contains template placeholders, but the docs say it won't.
Prior-finding status
RESOLVED — scripts/sync-automation-template.js — consumers stop receiving sync PRs after a merge (revertedAfterMerge)
RESOLVED — scripts/sync-automation-template.js:105 — inaccessible repo reported as not-migrated, exit 0
RESOLVED — scripts/sync-automation-template.js — one transient API failure ends the run with no report
RESOLVED — scripts/sync-automation-template.js — re-proposes after a maintainer closes a sync PR
RESOLVED — scripts/sync-automation-template.js — syncRepo state classification untested
RESOLVED — .github/workflows/sync-automation-template.yml:22 — overlapping runs race to open the PR
RESOLVED — automation-registry.yml — praise: KDS exception as registry data
RESOLVED — scripts/sync-automation-template.js — transient failure in lastTemplateChange silently stops syncing
RESOLVED — scripts/sync-automation-template.test.js — exit-code fix can regress with CI green
RESOLVED — scripts/sync-automation-template.test.js — no test asserts a write payload
RESOLVED — docs/automation.md — "Two results" vs four emitted states
RESOLVED — scripts/sync-automation-template.test.js — fixture pins base: 'main'
RESOLVED — scripts/sync-automation-template.js — praise: comment on 404 ambiguity
RESOLVED — scripts/sync-automation-template.test.js — delete test restating PROBLEM_STATES
RESOLVED — scripts/sync-automation-template.test.js:253 — praise: pins outcome, not mock
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Ran the same phased review passes as a first review (core, frontend/backend lenses, manual QA when required)
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| const consumers = []; | ||
| for (const repo of repos) { | ||
| if (repo.archived || repo.fork) continue; | ||
| const copy = await readCopy(api, repo.name, repo.default_branch); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: A thrown read here ends the whole run with no report. This regresses the resolved "one transient API failure ends the run with no report" finding. Discovery runs before the per-repo try (:254-259). One socket hang up on any org repo rejects run. A non-OK response goes through unreadable; a throw does not.
- Catch around
readCopyand return{ error: err.message }. The throw then flows through the existingunreadablepath. - The throw test (
sync-automation-template.test.js:198) now throws onpulls?state=open, not on this read. Make it throw onGET contents/.github/workflows/automation.ymland assertstate: 'error'.
|
|
||
| function prBody(prTemplate, answers) { | ||
| if (!prTemplate) return `${EXPLANATION}\n`; | ||
| const filled = prTemplate.replace(FIELD, (line, prefix, field) => { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: The KDS sync PR body is still a half-filled form. Only the - **Field:** lines get filled. The body still contains:
- an empty
## Descriptionholding only the HTML comment Addresses #*PR# HERE*1. Step 1/2. Step 2- unticked checklists
The explanation sits after a --- at the bottom. docs/automation.md:124 says the body "is not a half-filled form".
Put EXPLANATION under ## Description and drop the other sections. KDS's template says to remove unused sections. check-description only reads the Changelog block. If you keep the sections, reword the docs line instead.
| assert.ok(body.includes('automation-template.yml'), 'our explanation is still there'); | ||
| }); | ||
|
|
||
| test('any other repo gets the plain explanation, template or not', async () => { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: This negative case pins the KDS-only scoping. Without it, fetching templates for every repo would pass silently.
…ystem body Discovery runs before the per-repo error handling, so one socket hang up while reading any org repo rejected the whole run and printed no table. A non-ok response was already handled, a throw was not. It now flows through the same path and reports as an error against that repo. The body sent to kolibri-design-system carried every section of their template, including "Addresses #*PR# HERE*", numbered steps reading "1. Step 1", and unticked checklists. Their template opens by asking for unused sections to be removed, so only Description and Changelog are kept now. Description holds the explanation and Changelog holds the answered fields. Both fixes have a test that fails without them.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #97: all 18 prior findings resolved; one new suggestion inline.
CI passing.
Prior-finding status
RESOLVED — scripts/sync-automation-template.js — revertedAfterMerge stops syncing after a merge
RESOLVED — scripts/sync-automation-template.js:128 — inaccessible repo reported as not-migrated, exit 0
RESOLVED — scripts/sync-automation-template.js — one transient API failure ends the run with no report
RESOLVED — scripts/sync-automation-template.js — closed-unmerged sync PR re-proposed
RESOLVED — scripts/sync-automation-template.js — syncRepo state classification untested
RESOLVED — .github/workflows/sync-automation-template.yml:22 — overlapping runs race to open the PR
RESOLVED — automation-registry.yml — KDS exception as data (praise)
RESOLVED — scripts/sync-automation-template.js — lastTemplateChange transient failure silently stops syncing
RESOLVED — scripts/sync-automation-template.test.js — exit-code fix can regress with CI green
RESOLVED — scripts/sync-automation-template.test.js — no test asserts a write payload
RESOLVED — docs/automation.md — "Two results" vs four states
RESOLVED — scripts/sync-automation-template.test.js — fixture pins base: 'main'
RESOLVED — scripts/sync-automation-template.js — 404 ambiguity comment (praise)
RESOLVED — scripts/sync-automation-template.test.js — delete test restating PROBLEM_STATES
RESOLVED — scripts/sync-automation-template.test.js:289 — history-failure test pins outcome (praise)
RESOLVED — scripts/sync-automation-template.js — thrown read during discovery ends the run
RESOLVED — scripts/sync-automation-template.js — KDS sync PR body is a half-filled form
RESOLVED — scripts/sync-automation-template.test.js:139 — negative case pins KDS-only scoping (praise)
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| }); | ||
| return `${s.heading}\n${filled.trimEnd()}`; | ||
| }) | ||
| .join('\n\n'); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: The KDS sync PR opens with no explanation if KDS renames its ## Description heading. Nothing then matches the section that carries the explanation. If both headings change, the body is just "\n". Please fall back to ${EXPLANATION}\n when the describe heading is missing, and add a test for it.
There was a problem hiding this comment.
Fixed in cfc4adc, but prepending the explanation rather than returning it alone.
Returning ${EXPLANATION}\n also discards a Changelog section that survived, so a renamed ## Description would fail check-description on every sync pull request. Prepending covers both cases: a renamed Description leads with the explanation and keeps the Changelog, and a template with neither heading gives the explanation alone.
Tests for both, and each fails without the guard.
The body is built by keeping named sections of kolibri-design-system's template, and the explanation goes under the Description one. If that heading is renamed the explanation went missing, and if both kept headings changed the body was a single newline. The explanation is now prepended whenever no section matched, so the pull request always says what it is. Sections that did survive are kept, so a body that still carries the Changelog continues to pass their check rather than failing on a heading rename.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #97: all 19 prior findings resolved; no new findings. CI passing.
Prior-finding status
RESOLVED — scripts/sync-automation-template.js — revertedAfterMerge stops syncing after a merge
RESOLVED — scripts/sync-automation-template.js:128 — inaccessible repo reported as not-migrated, exit 0
RESOLVED — scripts/sync-automation-template.js — one transient API failure ends the run with no report
RESOLVED — scripts/sync-automation-template.js — closed-unmerged sync PR re-proposed
RESOLVED — scripts/sync-automation-template.js — syncRepo state classification untested
RESOLVED — .github/workflows/sync-automation-template.yml:22 — overlapping runs race to open the PR
RESOLVED — automation-registry.yml — KDS exception as data (praise)
RESOLVED — scripts/sync-automation-template.js — lastTemplateChange transient failure silently stops syncing
RESOLVED — scripts/sync-automation-template.test.js — exit-code fix can regress with CI green
RESOLVED — scripts/sync-automation-template.test.js — no test asserts a write payload
RESOLVED — docs/automation.md — "Two results" vs four states
RESOLVED — scripts/sync-automation-template.test.js — fixture pins base: 'main'
RESOLVED — scripts/sync-automation-template.js — 404 ambiguity comment (praise)
RESOLVED — scripts/sync-automation-template.test.js — delete test restating PROBLEM_STATES
RESOLVED — scripts/sync-automation-template.test.js:314 — history-failure test pins outcome (praise)
RESOLVED — scripts/sync-automation-template.js — thrown read during discovery ends the run
RESOLVED — scripts/sync-automation-template.js — KDS sync PR body is a half-filled form
RESOLVED — scripts/sync-automation-template.test.js:164 — negative case pins KDS-only scoping (praise)
RESOLVED — scripts/sync-automation-template.js — renamed ## Description heading leaves the PR unexplained
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
3 participants
Summary
Consumer repos hold a copied
automation-template.yml, and nothing checks that the copies stillmatch. A stale copy fails silently: the repo keeps running its old
on:block, so a new automationnever fires there, with no error and no log line.
This adds a workflow that compares each consumer's copy against the template and opens one pull
request per repo that differs, in the same shape dependabot uses. There is no accompanying issue,
because the change is a byte copy of one file with nothing to author.
The change
scripts/sync-automation-template.jsfinds the consumers, compares each copy against thetemplate, and opens or updates a pull request where they differ. A repo in sync gets nothing. A
repo with a sync pull request already open gets it updated, never duplicated.
.github/workflows/sync-automation-template.ymlruns it weekly, on manual dispatch, and whenautomation-template.ymlchanges onmain. Manual dispatch takes adry_runinput. Aconcurrencygroup stops two runs racing to open the same pull request.scripts/sync-automation-template.test.jscovers the state machine with an injected client, andasserts the payloads that decide correctness: the write targets the sync branch, carries the
template, and keeps the file sha on an update.
docs/automation.mddescribes what the workflow opens, who merges it, and each state that needsa core maintainer.
The consumers are discovered, not listed. The workflow walks the org's repos, skips archived ones
and forks, and keeps every repo whose
.github/workflows/automation.ymlcalls this repo'sautomation.yml. This repo's own reusable workflow sits at the same path and does not call it, sothat marker excludes it without a special case. Each pull request targets the repo's default branch,
which is the only branch GitHub evaluates workflow triggers from.
The body is a short explanation of what changed and why the file is generated.
kolibri-design-systemis the exception: its
check-descriptionjob fails unless the body carries a Changelog section whoseDescription is not the placeholder its own template ships with, and a plain explanation has no such
section. For that repo the sync reads its template, keeps the Description and Changelog sections, and
answers the Changelog fields. Their template opens by asking for unused sections to be removed, so
the rest go rather than arriving unfilled. The template is read rather than stored, so it stays
current as they change it.
Drift is classified against the time the template last changed here. A change after the last sync
pull request closed is ordinary drift. Without one, a merged pull request means the consumer
reverted the file, reported as
toolchain-conflict, and a closed one means a maintainer declinedit, reported as
declinedand left alone until the template moves again. An unknown template dateresolves to ordinary drift, because a needless pull request costs one review where a wrong conflict
stops the repo entirely.
The workflow only proposes. It opens pull requests on a branch, never commits to a default branch,
and never merges, approves, or enables auto-merge. Each consumer's own review rules gate the change.
There is no bot pre-review. The copy either matches the template or it does not, and the script
writes those bytes by construction, so there is no content question a reviewer could answer
differently. What the core maintainer decides is whether to accept the file landing in their repo.
References
Refs #95.
Follows #88, which introduced the template and the generator.
Reviewer guidance
A dry run against the live org finds the eight consumers and reports them all in sync. All eight
pre-commit hooks pass, actionlint included, the generator reports no drift, and 65 tests pass. The
write path has never run, because nothing is drifted for it to act on.
GITHUB_TOKEN=$(gh auth token) node scripts/sync-automation-template.js --dry-run. Expecteight rows, all
in-sync, and nothing needing attention.developfor kolibri,kolibri-design-system and kolibri-data-portal,
unstablefor studio,release-v0.9.xformorango, and
mainfor the rest.automation-template-syncbranch, and a test fails if that is changed to a default branch.kolibri-design-systemwould receive. Expect two sections, Description andChangelog, with every Changelog field answered. Its
check-descriptionjob parses thatDescriptionfield, and the value must not be the placeholder its template ships with.than ending the run. Discovery happens before the per-repo error handling, so it carries its own.
AI usage
I used Claude Code to audit the drift across the consumers, to write the script, workflow and tests,
and to draft this description. I directed the design decisions: pull requests rather than issues,
the workflow doing the copy itself rather than delegating it to an agent, discovering the consumers
rather than recording them here, handling
kolibri-design-systemas a named exception rather thangeneralising one repo's form into a rule, no bot pre-review on a change with nothing to judge, and
human review as a hard constraint. I verified the dry run against the live org,
confirmed the hooks and tests pass, and checked that the payload assertions fail when the write is
pointed at a default branch.