From 73db4e2e93ee975d7abdf51774ed38746515879f Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Mon, 21 Sep 2026 19:56:51 +0300 Subject: [PATCH 01/18] Open sync pull requests when a consumer's automation.yml drifts 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. --- .../workflows/sync-automation-template.yml | 43 +++++ automation-registry.yml | 41 +++++ docs/automation.md | 30 ++++ scripts/sync-automation-template.js | 159 ++++++++++++++++++ 4 files changed, 273 insertions(+) create mode 100644 .github/workflows/sync-automation-template.yml create mode 100644 scripts/sync-automation-template.js diff --git a/.github/workflows/sync-automation-template.yml b/.github/workflows/sync-automation-template.yml new file mode 100644 index 0000000..3f1d90f --- /dev/null +++ b/.github/workflows/sync-automation-template.yml @@ -0,0 +1,43 @@ +name: Sync automation template +run-name: Open a pull request in every consumer repo whose automation.yml has drifted +on: + schedule: + - cron: "17 6 * * 1" + workflow_dispatch: + inputs: + dry_run: + description: 'Report drift without opening pull requests' + type: boolean + default: false + push: + branches: + - main + paths: + - automation-template.yml +permissions: + contents: read +jobs: + sync: + name: Sync consumers + runs-on: ubuntu-latest + steps: + - name: Generate App Token + id: generate-token + uses: actions/create-github-app-token@v3 + with: + app-id: ${{ secrets.LE_BOT_APP_ID }} + private-key: ${{ secrets.LE_BOT_PRIVATE_KEY }} + owner: learningequality + - name: Checkout + uses: actions/checkout@v6 + - name: Setup Node.js + uses: actions/setup-node@v6 + with: + node-version: '20' + cache: 'yarn' + - name: Install dependencies + run: yarn install --frozen-lockfile + - name: Sync consumers + env: + GITHUB_TOKEN: ${{ steps.generate-token.outputs.token }} + run: node scripts/sync-automation-template.js ${{ inputs.dry_run && '--dry-run' || '' }} diff --git a/automation-registry.yml b/automation-registry.yml index 4e5d72f..fcf7b88 100644 --- a/automation-registry.yml +++ b/automation-registry.yml @@ -14,7 +14,48 @@ # Run `node scripts/generate-automation.js` after editing this file to regenerate # automation.yml and automation-template.yml. `node scripts/generate-automation.js --check` # fails if the generated files are out of date (enforced by pre-commit + CI). +# +# `consumers` lists the repos that copy automation-template.yml. Each entry gives: +# repo - the repo name inside the learningequality org +# base - the branch the sync pull request targets +# body_prefix - optional text placed above the sync pull request body, for repos +# whose checks require something in the description +# +# scripts/sync-automation-template.js opens one pull request per repo whose copy has drifted. +# Archived repos are left out, because Actions do not run on them and their copies are inert. + +consumers: + - repo: kolibri + base: develop + - repo: studio + base: unstable + - repo: ricecooker + base: main + - repo: kolibri-design-system + base: develop + # check-description fails unless the body carries a Changelog block whose + # Description line is not the template placeholder. + body_prefix: | + ## Changelog + + + - **Description:** Internal: refresh the copied automation.yml so it matches the current shared template + - **Products impact:** none + - **Addresses:** - + - **Components:** - + - **Breaking:** - + - **Impacts a11y:** - + - **Guidance:** - + + - repo: le-utils + base: main + - repo: kolibri-data-portal + base: develop + - repo: morango + base: release-v0.9.x + - repo: kolibri-installer-debian + base: main automations: - name: review-requested enabled: true diff --git a/docs/automation.md b/docs/automation.md index 3f07e8d..0b75a53 100644 --- a/docs/automation.md +++ b/docs/automation.md @@ -68,3 +68,33 @@ leaf workflow as an empty string, and every automation that needs it fails at ru picks up the wider `on:` block the next time they re-copy `automation-template.yml` - existing copies keep running on their current `on:` block until then, since GitHub workflow triggers are evaluated from the file checked into the consumer repo itself, not from this repo. + +## Keeping the copies in sync + +Most registry changes reach consumers on their own, because their copied file only says +`uses: learningequality/.github/.github/workflows/automation.yml@main`. A consumer must copy the +template again only when the template itself changes, which happens when a new event or activity +type enters the `on:` union, when the permissions widen, or when the secret list changes. + +`.github/workflows/sync-automation-template.yml` handles that. It runs weekly, on manual dispatch, +and whenever `automation-template.yml` changes on `main`. For each repo in the `consumers` list in +`automation-registry.yml`, it compares that repo's `.github/workflows/automation.yml` against the +template and opens a pull request where the two differ. A repo already in sync gets nothing, and a +repo with a sync pull request already open gets that pull request updated rather than a second one. + +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. Someone in each consumer repo reviews and merges, +under that repo's own rules. + +Run it with `dry_run` to see which repos have drifted without opening anything. + +Two results need a person rather than a merge: + +- `toolchain-conflict` means a sync pull request merged before and the file has drifted again. The + repo's own tooling rewrites the copy, so the template is not stable under that toolchain. Fix the + template rather than reopening the pull request. +- `error` means the repo could not be read or written. Check that the bot app is installed there and + holds `contents: write` and `pull-requests: write`. + +To onboard a repo, add it to `consumers` with the branch its pull requests must target. Leave +archived repos out, because Actions do not run on them. diff --git a/scripts/sync-automation-template.js b/scripts/sync-automation-template.js new file mode 100644 index 0000000..6f193c8 --- /dev/null +++ b/scripts/sync-automation-template.js @@ -0,0 +1,159 @@ +#!/usr/bin/env node +/** + * Opens one pull request per consumer repo whose copy of automation.yml has + * drifted from automation-template.yml. + * + * Usage: + * node scripts/sync-automation-template.js open or update pull requests + * node scripts/sync-automation-template.js --dry-run report only, change nothing + * + * Requires GITHUB_TOKEN with contents:write and pull-requests:write on each + * consumer repo. It never commits to a default branch and never merges. + */ +const fs = require('fs'); +const path = require('path'); +const yaml = require('js-yaml'); + +const ROOT = path.join(__dirname, '..'); +const REGISTRY_PATH = path.join(ROOT, 'automation-registry.yml'); +const TEMPLATE_PATH = path.join(ROOT, 'automation-template.yml'); + +const ORG = 'learningequality'; +const TARGET_PATH = '.github/workflows/automation.yml'; +const BRANCH = 'automation-template-sync'; +const TITLE = 'Refresh automation.yml from the shared template'; +const API = 'https://api.github.com'; + +const dryRun = process.argv.includes('--dry-run'); +const token = process.env.GITHUB_TOKEN; + +async function api(method, url, body) { + const res = await fetch(url.startsWith('http') ? url : `${API}${url}`, { + method, + headers: { + authorization: `Bearer ${token}`, + accept: 'application/vnd.github+json', + 'x-github-api-version': '2022-11-28', + 'content-type': 'application/json', + }, + body: body ? JSON.stringify(body) : undefined, + }); + const text = await res.text(); + const data = text ? JSON.parse(text) : null; + return { ok: res.ok, status: res.status, data }; +} + +function body(consumer) { + const explanation = [ + `This replaces \`${TARGET_PATH}\` with the current \`automation-template.yml\` from`, + `[${ORG}/.github](https://github.com/${ORG}/.github).`, + '', + 'The file is generated. Do not edit this copy: edit `automation-registry.yml` upstream', + 'and regenerate, or the next sync will overwrite the change.', + '', + 'Opened automatically. A person reviews and merges it.', + ].join('\n'); + return consumer.body_prefix ? `${consumer.body_prefix.trimEnd()}\n\n${explanation}\n` : `${explanation}\n`; +} + +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 }; + if (!r.ok) return { error: `read failed (${r.status}) ${r.data && r.data.message}` }; + return { sha: r.data.sha, content: Buffer.from(r.data.content, 'base64').toString('utf8') }; +} + +async function openSyncPr(repo) { + const r = await api('GET', `/repos/${ORG}/${repo}/pulls?state=open&head=${ORG}:${BRANCH}`); + return r.ok && r.data.length ? r.data[0] : null; +} + +// A sync pull request that merged, followed by the file drifting again, means the +// repo's own tooling rewrites the copy. Reopening would loop, so report instead. +async function revertedAfterMerge(repo) { + const r = await api('GET', `/repos/${ORG}/${repo}/pulls?state=closed&head=${ORG}:${BRANCH}&per_page=1`); + if (!r.ok || !r.data.length) return false; + return Boolean(r.data[0].merged_at); +} + +async function syncRepo(consumer, template) { + const { repo, base } = consumer; + const copy = await currentCopy(repo, base); + + if (copy.error) return { repo, state: 'error', detail: copy.error }; + if (copy.missing) return { repo, state: 'not-migrated' }; + if (copy.content === template) return { repo, state: 'in-sync' }; + + const existing = await openSyncPr(repo); + if (!existing && (await revertedAfterMerge(repo))) { + return { repo, state: 'toolchain-conflict' }; + } + if (dryRun) return { repo, state: existing ? 'would-update' : 'would-open', pr: existing && existing.number }; + + const baseRef = await api('GET', `/repos/${ORG}/${repo}/git/ref/heads/${base}`); + if (!baseRef.ok) return { repo, state: 'error', detail: `base ${base} not found` }; + + if (!existing) { + const made = await api('POST', `/repos/${ORG}/${repo}/git/refs`, { + ref: `refs/heads/${BRANCH}`, + sha: baseRef.data.object.sha, + }); + if (!made.ok && made.status !== 422) { + return { repo, state: 'error', detail: `branch failed (${made.status}) ${made.data && made.data.message}` }; + } + } + + const onBranch = await currentCopy(repo, BRANCH); + const put = await api('PUT', `/repos/${ORG}/${repo}/contents/${TARGET_PATH}`, { + message: TITLE, + content: Buffer.from(template, 'utf8').toString('base64'), + branch: BRANCH, + ...(onBranch.sha ? { sha: onBranch.sha } : {}), + }); + if (!put.ok) { + return { repo, state: 'error', detail: `write failed (${put.status}) ${put.data && put.data.message}` }; + } + + if (existing) return { repo, state: 'updated', pr: existing.number, url: existing.html_url }; + + const pr = await api('POST', `/repos/${ORG}/${repo}/pulls`, { + title: TITLE, + head: BRANCH, + base, + body: body(consumer), + }); + if (!pr.ok) { + return { repo, state: 'error', detail: `pull request failed (${pr.status}) ${pr.data && pr.data.message}` }; + } + return { repo, state: 'opened', pr: pr.data.number, url: pr.data.html_url }; +} + +async function main() { + if (!token) { + console.error('GITHUB_TOKEN is not set.'); + process.exit(1); + } + const registry = yaml.load(fs.readFileSync(REGISTRY_PATH, 'utf8')); + const template = fs.readFileSync(TEMPLATE_PATH, 'utf8'); + + const results = []; + for (const consumer of registry.consumers) { + results.push(await syncRepo(consumer, template)); + } + + for (const r of results) { + const extra = r.url || r.detail || (r.pr ? `#${r.pr}` : ''); + console.log(`${r.repo.padEnd(26)} ${r.state.padEnd(20)} ${extra}`); + } + + const problems = results.filter((r) => r.state === 'error' || r.state === 'toolchain-conflict'); + const drifted = results.filter((r) => r.state !== 'in-sync'); + + console.log(`\n${results.length} consumers, ${drifted.length} not in sync, ${problems.length} needing attention.`); + for (const p of problems) { + console.log(`::error title=${p.repo}::${p.state}${p.detail ? `: ${p.detail}` : ''}`); + } + process.exit(problems.length ? 1 : 0); +} + +main(); From a4ebeda97a19398a24585c7fe90128f49843478c Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Mon, 21 Sep 2026 20:06:44 +0300 Subject: [PATCH 02/18] Request a pre-review from rtibblesbot on sync pull requests 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. --- docs/automation.md | 9 ++++++--- scripts/sync-automation-template.js | 18 +++++++++++++++--- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/docs/automation.md b/docs/automation.md index 0b75a53..fa5f1c6 100644 --- a/docs/automation.md +++ b/docs/automation.md @@ -82,13 +82,16 @@ and whenever `automation-template.yml` changes on `main`. For each repo in the ` template and opens a pull request where the two differ. A repo already in sync gets nothing, and a repo with a sync pull request already open gets that pull request updated rather than a second one. +Each new sync pull request requests a review from `rtibblesbot`, so it gets a first pass before a +person looks at it, the same way a dependabot pull request does. + 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. Someone in each consumer repo reviews and merges, -under that repo's own rules. +and never merges, approves, or enables auto-merge. A core maintainer in each consumer repo gives the +final review and merges, under that repo's own rules. Run it with `dry_run` to see which repos have drifted without opening anything. -Two results need a person rather than a merge: +Two results need a core maintainer rather than a merge: - `toolchain-conflict` means a sync pull request merged before and the file has drifted again. The repo's own tooling rewrites the copy, so the template is not stable under that toolchain. Fix the diff --git a/scripts/sync-automation-template.js b/scripts/sync-automation-template.js index 6f193c8..2882c47 100644 --- a/scripts/sync-automation-template.js +++ b/scripts/sync-automation-template.js @@ -22,6 +22,7 @@ const ORG = 'learningequality'; const TARGET_PATH = '.github/workflows/automation.yml'; const BRANCH = 'automation-template-sync'; const TITLE = 'Refresh automation.yml from the shared template'; +const REVIEWER = 'rtibblesbot'; const API = 'https://api.github.com'; const dryRun = process.argv.includes('--dry-run'); @@ -51,7 +52,7 @@ function body(consumer) { 'The file is generated. Do not edit this copy: edit `automation-registry.yml` upstream', 'and regenerate, or the next sync will overwrite the change.', '', - 'Opened automatically. A person reviews and merges it.', + 'Opened automatically. A core maintainer reviews and merges it.', ].join('\n'); return consumer.body_prefix ? `${consumer.body_prefix.trimEnd()}\n\n${explanation}\n` : `${explanation}\n`; } @@ -125,7 +126,17 @@ async function syncRepo(consumer, template) { if (!pr.ok) { return { repo, state: 'error', detail: `pull request failed (${pr.status}) ${pr.data && pr.data.message}` }; } - return { repo, state: 'opened', pr: pr.data.number, url: pr.data.html_url }; + + const review = await api('POST', `/repos/${ORG}/${repo}/pulls/${pr.data.number}/requested_reviewers`, { + reviewers: [REVIEWER], + }); + return { + repo, + state: 'opened', + pr: pr.data.number, + url: pr.data.html_url, + reviewerFailed: !review.ok, + }; } async function main() { @@ -143,7 +154,8 @@ async function main() { for (const r of results) { const extra = r.url || r.detail || (r.pr ? `#${r.pr}` : ''); - console.log(`${r.repo.padEnd(26)} ${r.state.padEnd(20)} ${extra}`); + const note = r.reviewerFailed ? ` (could not request ${REVIEWER})` : ''; + console.log(`${r.repo.padEnd(26)} ${r.state.padEnd(20)} ${extra}${note}`); } const problems = results.filter((r) => r.state === 'error' || r.state === 'toolchain-conflict'); From 06b3ee1af0be88ce0d21de8b023342aca982a5a9 Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Mon, 21 Sep 2026 20:21:28 +0300 Subject: [PATCH 03/18] Fix the sync state machine and cover it with tests 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. --- .../workflows/sync-automation-template.yml | 3 + scripts/sync-automation-template.js | 203 +++++++++++------- scripts/sync-automation-template.test.js | 160 ++++++++++++++ 3 files changed, 291 insertions(+), 75 deletions(-) create mode 100644 scripts/sync-automation-template.test.js diff --git a/.github/workflows/sync-automation-template.yml b/.github/workflows/sync-automation-template.yml index 3f1d90f..d1fd0a5 100644 --- a/.github/workflows/sync-automation-template.yml +++ b/.github/workflows/sync-automation-template.yml @@ -16,6 +16,9 @@ on: - automation-template.yml permissions: contents: read +concurrency: + group: sync-automation-template + cancel-in-progress: false jobs: sync: name: Sync consumers diff --git a/scripts/sync-automation-template.js b/scripts/sync-automation-template.js index 2882c47..50b55d5 100644 --- a/scripts/sync-automation-template.js +++ b/scripts/sync-automation-template.js @@ -25,26 +25,33 @@ const TITLE = 'Refresh automation.yml from the shared template'; const REVIEWER = 'rtibblesbot'; const API = 'https://api.github.com'; -const dryRun = process.argv.includes('--dry-run'); -const token = process.env.GITHUB_TOKEN; - -async function api(method, url, body) { - const res = await fetch(url.startsWith('http') ? url : `${API}${url}`, { - method, - headers: { - authorization: `Bearer ${token}`, - accept: 'application/vnd.github+json', - 'x-github-api-version': '2022-11-28', - 'content-type': 'application/json', - }, - body: body ? JSON.stringify(body) : undefined, - }); - const text = await res.text(); - const data = text ? JSON.parse(text) : null; - return { ok: res.ok, status: res.status, data }; +const PROBLEM_STATES = ['error', 'toolchain-conflict', 'not-migrated']; + +function httpApi(token) { + return async function api(method, url, body) { + const res = await fetch(url.startsWith('http') ? url : `${API}${url}`, { + method, + headers: { + authorization: `Bearer ${token}`, + accept: 'application/vnd.github+json', + 'x-github-api-version': '2022-11-28', + 'content-type': 'application/json', + }, + body: body ? JSON.stringify(body) : undefined, + }); + const text = await res.text(); + let data = null; + // A gateway error answers with an HTML page, so parsing has to be able to fail. + try { + data = text ? JSON.parse(text) : null; + } catch { + data = { message: text.slice(0, 200) }; + } + return { ok: res.ok, status: res.status, data }; + }; } -function body(consumer) { +function prBody(consumer) { const explanation = [ `This replaces \`${TARGET_PATH}\` with the current \`automation-template.yml\` from`, `[${ORG}/.github](https://github.com/${ORG}/.github).`, @@ -57,115 +64,161 @@ function body(consumer) { return consumer.body_prefix ? `${consumer.body_prefix.trimEnd()}\n\n${explanation}\n` : `${explanation}\n`; } -async function currentCopy(repo, ref) { +function detail(r) { + return `${r.status} ${(r.data && r.data.message) || ''}`.trim(); +} + +async function readCopy(api, repo, ref) { const r = await api('GET', `/repos/${ORG}/${repo}/contents/${TARGET_PATH}?ref=${ref}`); - if (r.status === 404) return { missing: true }; - if (!r.ok) return { error: `read failed (${r.status}) ${r.data && r.data.message}` }; - return { sha: r.data.sha, content: Buffer.from(r.data.content, 'base64').toString('utf8') }; + 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 + // whether the repo itself is readable before calling the file missing. + if (r.status === 404) { + const probe = await api('GET', `/repos/${ORG}/${repo}`); + return probe.ok ? { missing: true } : { error: `no access to ${repo} (${detail(probe)})` }; + } + return { error: `read failed (${detail(r)})` }; +} + +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; + return r.data[0].commit.committer.date; } -async function openSyncPr(repo) { +async function openSyncPr(api, repo) { const r = await api('GET', `/repos/${ORG}/${repo}/pulls?state=open&head=${ORG}:${BRANCH}`); - return r.ok && r.data.length ? r.data[0] : null; + if (!r.ok) return { error: `could not list pull requests (${detail(r)})` }; + return { pr: r.data.length ? r.data[0] : null }; +} + +async function lastClosedSyncPr(api, repo) { + const r = await api( + 'GET', + `/repos/${ORG}/${repo}/pulls?state=closed&head=${ORG}:${BRANCH}&sort=updated&direction=desc&per_page=1` + ); + if (!r.ok) return { error: `could not list closed pull requests (${detail(r)})` }; + return { pr: r.data.length ? r.data[0] : null }; } -// A sync pull request that merged, followed by the file drifting again, means the -// repo's own tooling rewrites the copy. Reopening would loop, so report instead. -async function revertedAfterMerge(repo) { - const r = await api('GET', `/repos/${ORG}/${repo}/pulls?state=closed&head=${ORG}:${BRANCH}&per_page=1`); - if (!r.ok || !r.data.length) return false; - return Boolean(r.data[0].merged_at); +/** + * Decides what a drifted copy means, given the last closed sync pull request. + * A template change after that pull request closed is ordinary drift. With no + * such change, a merged pull request means the consumer reverted the file, and a + * closed one means a maintainer declined it. + */ +function classifyDrift(closedPr, templateChangedAt) { + if (!closedPr) return 'drift'; + const closedAt = closedPr.merged_at || closedPr.closed_at; + if (templateChangedAt && closedAt && new Date(templateChangedAt) > new Date(closedAt)) return 'drift'; + return closedPr.merged_at ? 'toolchain-conflict' : 'declined'; } -async function syncRepo(consumer, template) { +async function syncRepo(api, consumer, template, { dryRun, templateChangedAt }) { const { repo, base } = consumer; - const copy = await currentCopy(repo, base); + const copy = await readCopy(api, repo, base); if (copy.error) return { repo, state: 'error', detail: copy.error }; if (copy.missing) return { repo, state: 'not-migrated' }; if (copy.content === template) return { repo, state: 'in-sync' }; - const existing = await openSyncPr(repo); - if (!existing && (await revertedAfterMerge(repo))) { - return { repo, state: 'toolchain-conflict' }; + const open = await openSyncPr(api, repo); + if (open.error) return { repo, state: 'error', detail: open.error }; + + if (!open.pr) { + const closed = await lastClosedSyncPr(api, repo); + if (closed.error) return { repo, state: 'error', detail: closed.error }; + const verdict = classifyDrift(closed.pr, templateChangedAt); + if (verdict !== 'drift') return { repo, state: verdict, pr: closed.pr.number }; } - if (dryRun) return { repo, state: existing ? 'would-update' : 'would-open', pr: existing && existing.number }; - const baseRef = await api('GET', `/repos/${ORG}/${repo}/git/ref/heads/${base}`); - if (!baseRef.ok) return { repo, state: 'error', detail: `base ${base} not found` }; + if (dryRun) { + return { repo, state: open.pr ? 'would-update' : 'would-open', pr: open.pr && open.pr.number }; + } - if (!existing) { - const made = await api('POST', `/repos/${ORG}/${repo}/git/refs`, { - ref: `refs/heads/${BRANCH}`, - sha: baseRef.data.object.sha, - }); - if (!made.ok && made.status !== 422) { - return { repo, state: 'error', detail: `branch failed (${made.status}) ${made.data && made.data.message}` }; + const baseRef = await api('GET', `/repos/${ORG}/${repo}/git/ref/heads/${base}`); + if (!baseRef.ok) return { repo, state: 'error', detail: `base ${base} not found (${detail(baseRef)})` }; + const baseSha = baseRef.data.object.sha; + + if (!open.pr) { + const made = await api('POST', `/repos/${ORG}/${repo}/git/refs`, { ref: `refs/heads/${BRANCH}`, sha: baseSha }); + if (!made.ok && made.status === 422) { + // The branch outlives a closed pull request, so start it again from base + // rather than carrying commits a maintainer already saw. + const reset = await api('PATCH', `/repos/${ORG}/${repo}/git/refs/heads/${BRANCH}`, { sha: baseSha, force: true }); + if (!reset.ok) return { repo, state: 'error', detail: `branch reset failed (${detail(reset)})` }; + } else if (!made.ok) { + return { repo, state: 'error', detail: `branch failed (${detail(made)})` }; } } - const onBranch = await currentCopy(repo, BRANCH); + const onBranch = await readCopy(api, repo, BRANCH); + if (onBranch.error) return { repo, state: 'error', detail: onBranch.error }; + const put = await api('PUT', `/repos/${ORG}/${repo}/contents/${TARGET_PATH}`, { message: TITLE, content: Buffer.from(template, 'utf8').toString('base64'), branch: BRANCH, ...(onBranch.sha ? { sha: onBranch.sha } : {}), }); - if (!put.ok) { - return { repo, state: 'error', detail: `write failed (${put.status}) ${put.data && put.data.message}` }; - } + if (!put.ok) return { repo, state: 'error', detail: `write failed (${detail(put)})` }; - if (existing) return { repo, state: 'updated', pr: existing.number, url: existing.html_url }; + if (open.pr) return { repo, state: 'updated', pr: open.pr.number, url: open.pr.html_url }; const pr = await api('POST', `/repos/${ORG}/${repo}/pulls`, { title: TITLE, head: BRANCH, base, - body: body(consumer), + body: prBody(consumer), }); - if (!pr.ok) { - return { repo, state: 'error', detail: `pull request failed (${pr.status}) ${pr.data && pr.data.message}` }; - } + if (!pr.ok) return { repo, state: 'error', detail: `pull request failed (${detail(pr)})` }; const review = await api('POST', `/repos/${ORG}/${repo}/pulls/${pr.data.number}/requested_reviewers`, { reviewers: [REVIEWER], }); - return { - repo, - state: 'opened', - pr: pr.data.number, - url: pr.data.html_url, - reviewerFailed: !review.ok, - }; + return { repo, state: 'opened', pr: pr.data.number, url: pr.data.html_url, reviewerFailed: !review.ok }; } -async function main() { - if (!token) { - console.error('GITHUB_TOKEN is not set.'); - process.exit(1); - } - const registry = yaml.load(fs.readFileSync(REGISTRY_PATH, 'utf8')); - const template = fs.readFileSync(TEMPLATE_PATH, 'utf8'); - +async function run(api, registry, template, options) { + const templateChangedAt = await lastTemplateChange(api); const results = []; for (const consumer of registry.consumers) { - results.push(await syncRepo(consumer, template)); + try { + results.push(await syncRepo(api, consumer, template, { ...options, templateChangedAt })); + } catch (err) { + results.push({ repo: consumer.repo, state: 'error', detail: err.message }); + } } + return results; +} +function report(results) { for (const r of results) { const extra = r.url || r.detail || (r.pr ? `#${r.pr}` : ''); const note = r.reviewerFailed ? ` (could not request ${REVIEWER})` : ''; console.log(`${r.repo.padEnd(26)} ${r.state.padEnd(20)} ${extra}${note}`); } - - const problems = results.filter((r) => r.state === 'error' || r.state === 'toolchain-conflict'); + const problems = results.filter((r) => PROBLEM_STATES.includes(r.state)); const drifted = results.filter((r) => r.state !== 'in-sync'); - console.log(`\n${results.length} consumers, ${drifted.length} not in sync, ${problems.length} needing attention.`); for (const p of problems) { console.log(`::error title=${p.repo}::${p.state}${p.detail ? `: ${p.detail}` : ''}`); } - process.exit(problems.length ? 1 : 0); + return problems.length; } -main(); +async function main() { + const token = process.env.GITHUB_TOKEN; + if (!token) { + console.error('GITHUB_TOKEN is not set.'); + process.exit(1); + } + const registry = yaml.load(fs.readFileSync(REGISTRY_PATH, 'utf8')); + const template = fs.readFileSync(TEMPLATE_PATH, 'utf8'); + const results = await run(httpApi(token), registry, template, { dryRun: process.argv.includes('--dry-run') }); + process.exit(report(results) ? 1 : 0); +} + +if (require.main === module) main(); + +module.exports = { run, syncRepo, classifyDrift, report, PROBLEM_STATES, BRANCH, REVIEWER, TARGET_PATH }; diff --git a/scripts/sync-automation-template.test.js b/scripts/sync-automation-template.test.js new file mode 100644 index 0000000..f2106f8 --- /dev/null +++ b/scripts/sync-automation-template.test.js @@ -0,0 +1,160 @@ +const test = require('node:test'); +const assert = require('node:assert/strict'); + +const { run, classifyDrift, BRANCH } = require('./sync-automation-template'); + +const TEMPLATE = 'name: Automation\non: {}\n'; +const STALE = 'name: Automation\non: {old: true}\n'; +const REGISTRY = { consumers: [{ repo: 'demo', base: 'main' }] }; + +const encode = (s) => Buffer.from(s, 'utf8').toString('base64'); +const ok = (data) => ({ ok: true, status: 200, data }); +const fail = (status, message) => ({ ok: false, status, data: { message } }); + +/** + * Routes calls by method and a fragment of the path. Later entries win, so a + * test can override one route and inherit the rest. + */ +function makeApi(routes, calls = []) { + const ordered = [...routes].reverse(); + return async function api(method, url, body) { + calls.push({ method, url, body }); + const route = ordered.find(([match]) => { + const [m, fragment] = match.split(' '); + if (m !== method) return false; + // A leading "=" matches the whole path, so a repo route cannot also swallow + // the longer URLs underneath it. + return fragment.startsWith('=') ? url === fragment.slice(1) : url.includes(fragment); + }); + if (!route) return fail(404, 'unrouted'); + return typeof route[1] === 'function' ? route[1](url, body) : route[1]; + }; +} + +const baseRoutes = (copy) => [ + ['GET /repos/learningequality/.github/commits', ok([{ commit: { committer: { date: '2026-01-02T00:00:00Z' } } }])], + ['GET contents/.github/workflows/automation.yml', ok({ sha: 'file-sha', content: encode(copy) })], + ['GET /repos/learningequality/demo/pulls?state=open', ok([])], + ['GET /repos/learningequality/demo/pulls?state=closed', ok([])], + ['GET /repos/learningequality/demo/git/ref/heads/main', ok({ object: { sha: 'base-sha' } })], + ['POST /repos/learningequality/demo/git/refs', ok({})], + ['PUT /repos/learningequality/demo/contents', ok({})], + ['POST /repos/learningequality/demo/pulls', ok({ number: 7, html_url: 'https://example.test/7' })], + ['POST /repos/learningequality/demo/pulls/7/requested_reviewers', ok({})], +]; + +const only = async (routes, options = {}) => (await run(makeApi(routes), REGISTRY, TEMPLATE, options))[0]; + +test('a matching copy is in sync', async () => { + assert.equal((await only(baseRoutes(TEMPLATE))).state, 'in-sync'); +}); + +test('a drifted copy opens a pull request and requests the reviewer', async () => { + const calls = []; + const results = await run(makeApi(baseRoutes(STALE), calls), REGISTRY, TEMPLATE, {}); + assert.equal(results[0].state, 'opened'); + assert.equal(results[0].pr, 7); + assert.ok(calls.some((c) => c.url.endsWith('/pulls/7/requested_reviewers'))); +}); + +test('an open sync pull request is updated, not duplicated', async () => { + const calls = []; + const routes = [ + ...baseRoutes(STALE), + ['GET /repos/learningequality/demo/pulls?state=open', ok([{ number: 3, html_url: 'https://example.test/3' }])], + ]; + const results = await run(makeApi(routes, calls), REGISTRY, TEMPLATE, {}); + assert.equal(results[0].state, 'updated'); + assert.equal(calls.filter((c) => c.method === 'POST' && c.url.endsWith('/pulls')).length, 0); +}); + +test('a missing file in a readable repo is not-migrated', async () => { + const routes = [ + ...baseRoutes(STALE), + ['GET contents/.github/workflows/automation.yml', fail(404, 'Not Found')], + ['GET =/repos/learningequality/demo', ok({ name: 'demo' })], + ]; + assert.equal((await only(routes)).state, 'not-migrated'); +}); + +test('a repo the token cannot see is an error, not not-migrated', async () => { + const routes = [ + ...baseRoutes(STALE), + ['GET contents/.github/workflows/automation.yml', fail(404, 'Not Found')], + ['GET =/repos/learningequality/demo', fail(404, 'Not Found')], + ]; + const result = await only(routes); + assert.equal(result.state, 'error'); + assert.match(result.detail, /no access/); +}); + +test('a failed pull request listing is an error, not a missing pull request', async () => { + const routes = [...baseRoutes(STALE), ['GET /repos/learningequality/demo/pulls?state=open', fail(403, 'Forbidden')]]; + const result = await only(routes); + assert.equal(result.state, 'error'); + assert.match(result.detail, /could not list/); +}); + +test('a thrown request is contained and reported per repo', async () => { + const api = async (method, url) => { + if (url.includes('contents/')) throw new Error('socket hang up'); + return ok([]); + }; + const results = await run(api, REGISTRY, TEMPLATE, {}); + assert.equal(results[0].state, 'error'); + assert.equal(results[0].detail, 'socket hang up'); +}); + +test('dry run reports without writing', async () => { + const calls = []; + const results = await run(makeApi(baseRoutes(STALE), calls), REGISTRY, TEMPLATE, { dryRun: true }); + assert.equal(results[0].state, 'would-open'); + assert.equal( + calls.filter((c) => c.method !== 'GET').length, + 0 + ); +}); + +test('classifyDrift: no prior pull request is ordinary drift', () => { + assert.equal(classifyDrift(null, '2026-01-02T00:00:00Z'), 'drift'); +}); + +test('classifyDrift: a template change after a merge is ordinary drift', () => { + const merged = { merged_at: '2026-01-01T00:00:00Z', closed_at: '2026-01-01T00:00:00Z' }; + assert.equal(classifyDrift(merged, '2026-02-01T00:00:00Z'), 'drift'); +}); + +test('classifyDrift: drift with no template change after a merge is a toolchain conflict', () => { + const merged = { merged_at: '2026-03-01T00:00:00Z', closed_at: '2026-03-01T00:00:00Z' }; + assert.equal(classifyDrift(merged, '2026-01-01T00:00:00Z'), 'toolchain-conflict'); +}); + +test('classifyDrift: a pull request closed unmerged is declined until the template moves', () => { + const closed = { merged_at: null, closed_at: '2026-03-01T00:00:00Z' }; + assert.equal(classifyDrift(closed, '2026-01-01T00:00:00Z'), 'declined'); + assert.equal(classifyDrift(closed, '2026-04-01T00:00:00Z'), 'drift'); +}); + +test('a repo that reverted a merged sync is reported, and no pull request is opened', async () => { + const calls = []; + const routes = [ + ...baseRoutes(STALE), + [ + 'GET /repos/learningequality/demo/pulls?state=closed', + ok([{ number: 9, merged_at: '2026-03-01T00:00:00Z', closed_at: '2026-03-01T00:00:00Z' }]), + ], + ]; + const results = await run(makeApi(routes, calls), REGISTRY, TEMPLATE, {}); + assert.equal(results[0].state, 'toolchain-conflict'); + assert.equal(calls.filter((c) => c.method !== 'GET').length, 0); +}); + +test('a stale branch is reset to base when no pull request is open', async () => { + const calls = []; + const routes = [...baseRoutes(STALE), ['POST /repos/learningequality/demo/git/refs', fail(422, 'Reference exists')]]; + await run(makeApi(routes, calls), REGISTRY, TEMPLATE, {}); + const reset = calls.find((c) => c.method === 'PATCH' && c.url.endsWith(`/git/refs/heads/${BRANCH}`)); + assert.ok(reset, 'expected the branch to be reset'); + assert.equal(reset.body.sha, 'base-sha'); + assert.equal(reset.body.force, true); +}); From 3a7e81d3426ebb8c4a1ba7448e9bcb29ce1aa434 Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Mon, 21 Sep 2026 20:46:08 +0300 Subject: [PATCH 04/18] Fail toward drift when the template history cannot be read 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. --- docs/automation.md | 14 ++- scripts/sync-automation-template.js | 25 ++++-- scripts/sync-automation-template.test.js | 104 ++++++++++++++++++++++- 3 files changed, 129 insertions(+), 14 deletions(-) diff --git a/docs/automation.md b/docs/automation.md index fa5f1c6..4864390 100644 --- a/docs/automation.md +++ b/docs/automation.md @@ -91,13 +91,19 @@ final review and merges, under that repo's own rules. Run it with `dry_run` to see which repos have drifted without opening anything. -Two results need a core maintainer rather than a merge: +Four results need a core maintainer rather than a merge. The first three turn the run red: -- `toolchain-conflict` means a sync pull request merged before and the file has drifted again. The - repo's own tooling rewrites the copy, so the template is not stable under that toolchain. Fix the - template rather than reopening the pull request. - `error` means the repo could not be read or written. Check that the bot app is installed there and holds `contents: write` and `pull-requests: write`. +- `not-migrated` means the repo has no `.github/workflows/automation.yml` at all. Either it has not + been onboarded yet, or it belongs in `consumers` by mistake. Copy the template in, or remove the + entry. +- `toolchain-conflict` means a sync pull request merged before, the template has not changed since, + and the file has drifted again. The repo's own tooling rewrites the copy, so the template is not + stable under that toolchain. Fix the template rather than reopening the pull request. +- `declined` means a core maintainer closed the last sync pull request without merging it. The + workflow leaves that repo alone until the template changes again, so it stays drifted while the + run stays green. To restore it sooner, copy the template in by hand. To onboard a repo, add it to `consumers` with the branch its pull requests must target. Leave archived repos out, because Actions do not run on them. diff --git a/scripts/sync-automation-template.js b/scripts/sync-automation-template.js index 50b55d5..b42792d 100644 --- a/scripts/sync-automation-template.js +++ b/scripts/sync-automation-template.js @@ -81,9 +81,14 @@ async function readCopy(api, repo, ref) { } 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; - return r.data[0].commit.committer.date; + try { + const r = await api('GET', `/repos/${ORG}/.github/commits?path=automation-template.yml&per_page=1`); + if (!r.ok) return { date: null, failed: true, detail: detail(r) }; + if (!r.data.length) return { date: null, failed: false }; + return { date: r.data[0].commit.committer.date, failed: false }; + } catch (err) { + return { date: null, failed: true, detail: err.message }; + } } async function openSyncPr(api, repo) { @@ -106,11 +111,15 @@ async function lastClosedSyncPr(api, repo) { * A template change after that pull request closed is ordinary drift. With no * such change, a merged pull request means the consumer reverted the file, and a * closed one means a maintainer declined it. + * + * An unknown template date resolves to drift. A needless pull request costs one + * review, where a wrong toolchain-conflict stops syncing the repo entirely. */ function classifyDrift(closedPr, templateChangedAt) { - if (!closedPr) return 'drift'; + if (!closedPr || !templateChangedAt) return 'drift'; const closedAt = closedPr.merged_at || closedPr.closed_at; - if (templateChangedAt && closedAt && new Date(templateChangedAt) > new Date(closedAt)) return 'drift'; + if (!closedAt) return 'drift'; + if (new Date(templateChangedAt) > new Date(closedAt)) return 'drift'; return closedPr.merged_at ? 'toolchain-conflict' : 'declined'; } @@ -180,7 +189,11 @@ async function syncRepo(api, consumer, template, { dryRun, templateChangedAt }) } async function run(api, registry, template, options) { - const templateChangedAt = await lastTemplateChange(api); + const change = await lastTemplateChange(api); + if (change.failed) { + console.log(`::warning::could not read the template history (${change.detail}); treating drift as ordinary`); + } + const templateChangedAt = change.date; const results = []; for (const consumer of registry.consumers) { try { diff --git a/scripts/sync-automation-template.test.js b/scripts/sync-automation-template.test.js index f2106f8..98d8aa2 100644 --- a/scripts/sync-automation-template.test.js +++ b/scripts/sync-automation-template.test.js @@ -1,11 +1,15 @@ const test = require('node:test'); const assert = require('node:assert/strict'); -const { run, classifyDrift, BRANCH } = require('./sync-automation-template'); +const { run, classifyDrift, report, PROBLEM_STATES, BRANCH, REVIEWER } = require('./sync-automation-template'); const TEMPLATE = 'name: Automation\non: {}\n'; const STALE = 'name: Automation\non: {old: true}\n'; -const REGISTRY = { consumers: [{ repo: 'demo', base: 'main' }] }; +const PREFIX = '## Changelog\n\n - **Description:** test\n'; +// Not "main": five of the eight real consumers target something else, so a +// fixture on main cannot catch a hardcoded base. +const BASE = 'develop'; +const REGISTRY = { consumers: [{ repo: 'demo', base: BASE }] }; const encode = (s) => Buffer.from(s, 'utf8').toString('base64'); const ok = (data) => ({ ok: true, status: 200, data }); @@ -36,7 +40,7 @@ const baseRoutes = (copy) => [ ['GET contents/.github/workflows/automation.yml', ok({ sha: 'file-sha', content: encode(copy) })], ['GET /repos/learningequality/demo/pulls?state=open', ok([])], ['GET /repos/learningequality/demo/pulls?state=closed', ok([])], - ['GET /repos/learningequality/demo/git/ref/heads/main', ok({ object: { sha: 'base-sha' } })], + [`GET /repos/learningequality/demo/git/ref/heads/${BASE}`, ok({ object: { sha: 'base-sha' } })], ['POST /repos/learningequality/demo/git/refs', ok({})], ['PUT /repos/learningequality/demo/contents', ok({})], ['POST /repos/learningequality/demo/pulls', ok({ number: 7, html_url: 'https://example.test/7' })], @@ -57,7 +61,32 @@ test('a drifted copy opens a pull request and requests the reviewer', async () = assert.ok(calls.some((c) => c.url.endsWith('/pulls/7/requested_reviewers'))); }); -test('an open sync pull request is updated, not duplicated', async () => { +test('the write targets the sync branch and carries the template', async () => { + const calls = []; + await run(makeApi(baseRoutes(STALE), calls), REGISTRY, TEMPLATE, {}); + const put = calls.find((c) => c.method === 'PUT'); + assert.equal(put.body.branch, BRANCH, 'must never write to a default branch'); + assert.equal(Buffer.from(put.body.content, 'base64').toString('utf8'), TEMPLATE); +}); + +test('the pull request targets the consumer base and names the reviewer', async () => { + const calls = []; + await run(makeApi(baseRoutes(STALE), calls), REGISTRY, TEMPLATE, {}); + const pr = calls.find((c) => c.method === 'POST' && c.url.endsWith('/pulls')); + assert.equal(pr.body.base, BASE); + const review = calls.find((c) => c.url.endsWith('/requested_reviewers')); + assert.deepEqual(review.body.reviewers, [REVIEWER]); +}); + +test('a body_prefix reaches the pull request body', async () => { + const calls = []; + const registry = { consumers: [{ repo: 'demo', base: BASE, body_prefix: PREFIX }] }; + await run(makeApi(baseRoutes(STALE), calls), registry, TEMPLATE, {}); + const pr = calls.find((c) => c.method === 'POST' && c.url.endsWith('/pulls')); + assert.ok(pr.body.body.startsWith('## Changelog'), 'KDS check-description needs the prefix'); +}); + +test('an open sync pull request is updated in place, keeping the file sha', async () => { const calls = []; const routes = [ ...baseRoutes(STALE), @@ -66,6 +95,9 @@ test('an open sync pull request is updated, not duplicated', async () => { const results = await run(makeApi(routes, calls), REGISTRY, TEMPLATE, {}); assert.equal(results[0].state, 'updated'); assert.equal(calls.filter((c) => c.method === 'POST' && c.url.endsWith('/pulls')).length, 0); + const put = calls.find((c) => c.method === 'PUT'); + assert.equal(put.body.sha, 'file-sha', 'an update without the sha fails with a 422'); + assert.equal(put.body.branch, BRANCH); }); test('a missing file in a readable repo is not-migrated', async () => { @@ -149,6 +181,70 @@ test('a repo that reverted a merged sync is reported, and no pull request is ope assert.equal(calls.filter((c) => c.method !== 'GET').length, 0); }); +test('an unreadable template history proposes rather than stopping the repo', async () => { + const calls = []; + const routes = [ + ...baseRoutes(STALE), + ['GET /repos/learningequality/.github/commits', fail(502, 'Bad Gateway')], + [ + 'GET /repos/learningequality/demo/pulls?state=closed', + ok([{ number: 9, merged_at: '2026-03-01T00:00:00Z', closed_at: '2026-03-01T00:00:00Z' }]), + ], + ]; + const results = await run(makeApi(routes, calls), REGISTRY, TEMPLATE, {}); + assert.equal(results[0].state, 'opened', 'a 502 must not read as a toolchain conflict'); +}); + +test('a thrown template history request does not kill the run', async () => { + const routes = [ + ...baseRoutes(STALE), + [ + 'GET /repos/learningequality/.github/commits', + () => { + throw new Error('socket hang up'); + }, + ], + ]; + const results = await run(makeApi(routes), REGISTRY, TEMPLATE, {}); + assert.equal(results[0].state, 'opened'); +}); + +test('report counts only the states that stop a merge', () => { + const quiet = console.log; + console.log = () => {}; + try { + const problems = report([ + { repo: 'a', state: 'in-sync' }, + { repo: 'b', state: 'opened', pr: 1 }, + { repo: 'c', state: 'declined', pr: 2 }, + { repo: 'd', state: 'not-migrated' }, + { repo: 'e', state: 'error', detail: 'no access' }, + { repo: 'f', state: 'toolchain-conflict', pr: 3 }, + ]); + assert.equal(problems, 3, 'not-migrated, error and toolchain-conflict each need attention'); + } finally { + console.log = quiet; + } +}); + +test('declined is deliberately not a problem state', () => { + assert.ok(!PROBLEM_STATES.includes('declined')); + assert.deepEqual([...PROBLEM_STATES].sort(), ['error', 'not-migrated', 'toolchain-conflict']); +}); + +test('a failed reviewer request is noted but does not fail the run', () => { + const lines = []; + const quiet = console.log; + console.log = (line) => lines.push(line); + try { + const problems = report([{ repo: 'a', state: 'opened', pr: 1, reviewerFailed: true }]); + assert.equal(problems, 0); + assert.ok(lines.some((l) => l.includes(REVIEWER))); + } finally { + console.log = quiet; + } +}); + test('a stale branch is reset to base when no pull request is open', async () => { const calls = []; const routes = [...baseRoutes(STALE), ['POST /repos/learningequality/demo/git/refs', fail(422, 'Reference exists')]]; From 4cc6deb293fbf21823887a3370a42f54d970ee96 Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Mon, 21 Sep 2026 21:03:33 +0300 Subject: [PATCH 05/18] Say what the problem-state test guards against 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. --- scripts/sync-automation-template.test.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/scripts/sync-automation-template.test.js b/scripts/sync-automation-template.test.js index 98d8aa2..d3c87da 100644 --- a/scripts/sync-automation-template.test.js +++ b/scripts/sync-automation-template.test.js @@ -227,9 +227,12 @@ test('report counts only the states that stop a merge', () => { } }); -test('declined is deliberately not a problem state', () => { - assert.ok(!PROBLEM_STATES.includes('declined')); - assert.deepEqual([...PROBLEM_STATES].sort(), ['error', 'not-migrated', 'toolchain-conflict']); +test('the set of problem states is closed', () => { + assert.deepEqual( + [...PROBLEM_STATES].sort(), + ['error', 'not-migrated', 'toolchain-conflict'], + 'adding a state here turns the weekly run red for repos that were passing, so change it deliberately and cover it in the report test above' + ); }); test('a failed reviewer request is noted but does not fail the run', () => { From ecf5a25188b65398d41e15e496c46a504fe22122 Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Mon, 21 Sep 2026 21:34:05 +0300 Subject: [PATCH 06/18] Let a consumer supply the whole sync pull request body 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. --- automation-registry.yml | 25 +++++++++++++++++++----- docs/automation.md | 5 +++++ scripts/sync-automation-template.js | 3 ++- scripts/sync-automation-template.test.js | 22 ++++++++++++++++----- 4 files changed, 44 insertions(+), 11 deletions(-) diff --git a/automation-registry.yml b/automation-registry.yml index fcf7b88..e0b014e 100644 --- a/automation-registry.yml +++ b/automation-registry.yml @@ -18,8 +18,8 @@ # `consumers` lists the repos that copy automation-template.yml. Each entry gives: # repo - the repo name inside the learningequality org # base - the branch the sync pull request targets -# body_prefix - optional text placed above the sync pull request body, for repos -# whose checks require something in the description +# body_template - optional pull request body for repos with their own template or +# description checks; {{explanation}} is replaced by the generated text # # scripts/sync-automation-template.js opens one pull request per repo whose copy has drifted. # Archived repos are left out, because Actions do not run on them and their copies are inert. @@ -33,9 +33,14 @@ consumers: base: main - repo: kolibri-design-system base: develop - # check-description fails unless the body carries a Changelog block whose - # Description line is not the template placeholder. - body_prefix: | + # Follows this repo's own pull request template. check-description fails + # unless the Changelog block holds a Description line that is not the + # placeholder text. + body_template: | + ## Description + + {{explanation}} + ## Changelog @@ -48,6 +53,16 @@ consumers: - **Guidance:** - + + ## Steps to test + + 1. Confirm the diff touches only `.github/workflows/automation.yml`. + 2. Confirm the Automation workflow runs here and every job skips or succeeds. + + ## Comments + + Opened by the sync workflow in `learningequality/.github`. The file is generated from + `automation-registry.yml`, so no part of this change was written by hand. - repo: le-utils base: main - repo: kolibri-data-portal diff --git a/docs/automation.md b/docs/automation.md index 4864390..6af65a6 100644 --- a/docs/automation.md +++ b/docs/automation.md @@ -107,3 +107,8 @@ Four results need a core maintainer rather than a merge. The first three turn th To onboard a repo, add it to `consumers` with the branch its pull requests must target. Leave archived repos out, because Actions do not run on them. + +A repo with its own pull request template, or a check on the description, can also carry a +`body_template`. The workflow puts the generated text where `{{explanation}}` appears, so the body +follows that repo's own section order. `kolibri-design-system` needs one, because its +`check-description` job fails unless the body holds a Changelog block. diff --git a/scripts/sync-automation-template.js b/scripts/sync-automation-template.js index b42792d..5ed78c8 100644 --- a/scripts/sync-automation-template.js +++ b/scripts/sync-automation-template.js @@ -61,7 +61,8 @@ function prBody(consumer) { '', 'Opened automatically. A core maintainer reviews and merges it.', ].join('\n'); - return consumer.body_prefix ? `${consumer.body_prefix.trimEnd()}\n\n${explanation}\n` : `${explanation}\n`; + if (!consumer.body_template) return `${explanation}\n`; + return `${consumer.body_template.replaceAll('{{explanation}}', explanation).trimEnd()}\n`; } function detail(r) { diff --git a/scripts/sync-automation-template.test.js b/scripts/sync-automation-template.test.js index d3c87da..b6f00d4 100644 --- a/scripts/sync-automation-template.test.js +++ b/scripts/sync-automation-template.test.js @@ -5,7 +5,7 @@ const { run, classifyDrift, report, PROBLEM_STATES, BRANCH, REVIEWER } = require const TEMPLATE = 'name: Automation\non: {}\n'; const STALE = 'name: Automation\non: {old: true}\n'; -const PREFIX = '## Changelog\n\n - **Description:** test\n'; +const TEMPLATE_BODY = '## Description\n\n{{explanation}}\n\n## Changelog\n\n - **Description:** test\n'; // Not "main": five of the eight real consumers target something else, so a // fixture on main cannot catch a hardcoded base. const BASE = 'develop'; @@ -78,12 +78,24 @@ test('the pull request targets the consumer base and names the reviewer', async assert.deepEqual(review.body.reviewers, [REVIEWER]); }); -test('a body_prefix reaches the pull request body', async () => { +test('a body_template keeps its own shape and receives the explanation', async () => { const calls = []; - const registry = { consumers: [{ repo: 'demo', base: BASE, body_prefix: PREFIX }] }; + const registry = { consumers: [{ repo: 'demo', base: BASE, body_template: TEMPLATE_BODY }] }; await run(makeApi(baseRoutes(STALE), calls), registry, TEMPLATE, {}); - const pr = calls.find((c) => c.method === 'POST' && c.url.endsWith('/pulls')); - assert.ok(pr.body.body.startsWith('## Changelog'), 'KDS check-description needs the prefix'); + const { body } = calls.find((c) => c.method === 'POST' && c.url.endsWith('/pulls')).body; + assert.ok(body.startsWith('## Description'), 'the repo template decides the order, not the script'); + assert.ok(body.includes('## Changelog'), 'KDS check-description needs the Changelog block'); + assert.ok(body.includes('automation-template.yml'), 'the explanation must replace the placeholder'); + assert.ok(!body.includes('{{explanation}}')); + assert.ok(body.indexOf('## Description') < body.indexOf('## Changelog')); +}); + +test('a consumer with no template gets the plain explanation', async () => { + const calls = []; + await run(makeApi(baseRoutes(STALE), calls), REGISTRY, TEMPLATE, {}); + const { body } = calls.find((c) => c.method === 'POST' && c.url.endsWith('/pulls')).body; + assert.ok(!body.includes('## ')); + assert.ok(body.includes('automation-template.yml')); }); test('an open sync pull request is updated in place, keeping the file sha', async () => { From f25c4608578914c2f0f47e14ee39b3de3da22b42 Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Mon, 21 Sep 2026 22:50:47 +0300 Subject: [PATCH 07/18] Drop the bot pre-review from sync pull requests 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. --- docs/automation.md | 3 --- scripts/sync-automation-template.js | 12 +++-------- scripts/sync-automation-template.test.js | 27 +++++------------------- 3 files changed, 8 insertions(+), 34 deletions(-) diff --git a/docs/automation.md b/docs/automation.md index 6af65a6..44b9dc1 100644 --- a/docs/automation.md +++ b/docs/automation.md @@ -82,9 +82,6 @@ and whenever `automation-template.yml` changes on `main`. For each repo in the ` template and opens a pull request where the two differ. A repo already in sync gets nothing, and a repo with a sync pull request already open gets that pull request updated rather than a second one. -Each new sync pull request requests a review from `rtibblesbot`, so it gets a first pass before a -person looks at it, the same way a dependabot pull request does. - 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. A core maintainer in each consumer repo gives the final review and merges, under that repo's own rules. diff --git a/scripts/sync-automation-template.js b/scripts/sync-automation-template.js index 5ed78c8..d916586 100644 --- a/scripts/sync-automation-template.js +++ b/scripts/sync-automation-template.js @@ -22,7 +22,6 @@ const ORG = 'learningequality'; const TARGET_PATH = '.github/workflows/automation.yml'; const BRANCH = 'automation-template-sync'; const TITLE = 'Refresh automation.yml from the shared template'; -const REVIEWER = 'rtibblesbot'; const API = 'https://api.github.com'; const PROBLEM_STATES = ['error', 'toolchain-conflict', 'not-migrated']; @@ -182,11 +181,7 @@ async function syncRepo(api, consumer, template, { dryRun, templateChangedAt }) body: prBody(consumer), }); if (!pr.ok) return { repo, state: 'error', detail: `pull request failed (${detail(pr)})` }; - - const review = await api('POST', `/repos/${ORG}/${repo}/pulls/${pr.data.number}/requested_reviewers`, { - reviewers: [REVIEWER], - }); - return { repo, state: 'opened', pr: pr.data.number, url: pr.data.html_url, reviewerFailed: !review.ok }; + return { repo, state: 'opened', pr: pr.data.number, url: pr.data.html_url }; } async function run(api, registry, template, options) { @@ -209,8 +204,7 @@ async function run(api, registry, template, options) { function report(results) { for (const r of results) { const extra = r.url || r.detail || (r.pr ? `#${r.pr}` : ''); - const note = r.reviewerFailed ? ` (could not request ${REVIEWER})` : ''; - console.log(`${r.repo.padEnd(26)} ${r.state.padEnd(20)} ${extra}${note}`); + console.log(`${r.repo.padEnd(26)} ${r.state.padEnd(20)} ${extra}`); } const problems = results.filter((r) => PROBLEM_STATES.includes(r.state)); const drifted = results.filter((r) => r.state !== 'in-sync'); @@ -235,4 +229,4 @@ async function main() { if (require.main === module) main(); -module.exports = { run, syncRepo, classifyDrift, report, PROBLEM_STATES, BRANCH, REVIEWER, TARGET_PATH }; +module.exports = { run, syncRepo, classifyDrift, report, PROBLEM_STATES, BRANCH, TARGET_PATH }; diff --git a/scripts/sync-automation-template.test.js b/scripts/sync-automation-template.test.js index b6f00d4..c4cb505 100644 --- a/scripts/sync-automation-template.test.js +++ b/scripts/sync-automation-template.test.js @@ -1,7 +1,7 @@ const test = require('node:test'); const assert = require('node:assert/strict'); -const { run, classifyDrift, report, PROBLEM_STATES, BRANCH, REVIEWER } = require('./sync-automation-template'); +const { run, classifyDrift, report, PROBLEM_STATES, BRANCH } = require('./sync-automation-template'); const TEMPLATE = 'name: Automation\non: {}\n'; const STALE = 'name: Automation\non: {old: true}\n'; @@ -44,7 +44,6 @@ const baseRoutes = (copy) => [ ['POST /repos/learningequality/demo/git/refs', ok({})], ['PUT /repos/learningequality/demo/contents', ok({})], ['POST /repos/learningequality/demo/pulls', ok({ number: 7, html_url: 'https://example.test/7' })], - ['POST /repos/learningequality/demo/pulls/7/requested_reviewers', ok({})], ]; const only = async (routes, options = {}) => (await run(makeApi(routes), REGISTRY, TEMPLATE, options))[0]; @@ -53,12 +52,10 @@ test('a matching copy is in sync', async () => { assert.equal((await only(baseRoutes(TEMPLATE))).state, 'in-sync'); }); -test('a drifted copy opens a pull request and requests the reviewer', async () => { - const calls = []; - const results = await run(makeApi(baseRoutes(STALE), calls), REGISTRY, TEMPLATE, {}); +test('a drifted copy opens a pull request', async () => { + const results = await run(makeApi(baseRoutes(STALE)), REGISTRY, TEMPLATE, {}); assert.equal(results[0].state, 'opened'); assert.equal(results[0].pr, 7); - assert.ok(calls.some((c) => c.url.endsWith('/pulls/7/requested_reviewers'))); }); test('the write targets the sync branch and carries the template', async () => { @@ -69,13 +66,12 @@ test('the write targets the sync branch and carries the template', async () => { assert.equal(Buffer.from(put.body.content, 'base64').toString('utf8'), TEMPLATE); }); -test('the pull request targets the consumer base and names the reviewer', async () => { +test('the pull request targets the consumer base, not a default of main', async () => { const calls = []; await run(makeApi(baseRoutes(STALE), calls), REGISTRY, TEMPLATE, {}); const pr = calls.find((c) => c.method === 'POST' && c.url.endsWith('/pulls')); assert.equal(pr.body.base, BASE); - const review = calls.find((c) => c.url.endsWith('/requested_reviewers')); - assert.deepEqual(review.body.reviewers, [REVIEWER]); + assert.equal(pr.body.head, BRANCH); }); test('a body_template keeps its own shape and receives the explanation', async () => { @@ -247,19 +243,6 @@ test('the set of problem states is closed', () => { ); }); -test('a failed reviewer request is noted but does not fail the run', () => { - const lines = []; - const quiet = console.log; - console.log = (line) => lines.push(line); - try { - const problems = report([{ repo: 'a', state: 'opened', pr: 1, reviewerFailed: true }]); - assert.equal(problems, 0); - assert.ok(lines.some((l) => l.includes(REVIEWER))); - } finally { - console.log = quiet; - } -}); - test('a stale branch is reset to base when no pull request is open', async () => { const calls = []; const routes = [...baseRoutes(STALE), ['POST /repos/learningequality/demo/git/refs', fail(422, 'Reference exists')]]; From 91ad2c102cc616e81b13af0ed142ff2cad03d156 Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Wed, 23 Sep 2026 15:30:54 +0300 Subject: [PATCH 08/18] Correct what the sync states mean and what onboarding needs 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. --- docs/automation.md | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/docs/automation.md b/docs/automation.md index 44b9dc1..90f4373 100644 --- a/docs/automation.md +++ b/docs/automation.md @@ -72,9 +72,12 @@ leaf workflow as an empty string, and every automation that needs it fails at ru ## Keeping the copies in sync Most registry changes reach consumers on their own, because their copied file only says -`uses: learningequality/.github/.github/workflows/automation.yml@main`. A consumer must copy the -template again only when the template itself changes, which happens when a new event or activity -type enters the `on:` union, when the permissions widen, or when the secret list changes. +`uses: learningequality/.github/.github/workflows/automation.yml@main`. A consumer needs the file +again whenever its copy stops matching the template, which happens two ways. + +The template changes here, when a new event or activity type enters the `on:` union, when the +permissions widen, or when the secret list changes. Or the consumer's own tooling rewrites its copy, +which is what a `toolchain-conflict` below reports. `.github/workflows/sync-automation-template.yml` handles that. It runs weekly, on manual dispatch, and whenever `automation-template.yml` changes on `main`. For each repo in the `consumers` list in @@ -88,22 +91,26 @@ final review and merges, under that repo's own rules. Run it with `dry_run` to see which repos have drifted without opening anything. -Four results need a core maintainer rather than a merge. The first three turn the run red: +Three results turn the run red and need someone to act: -- `error` means the repo could not be read or written. Check that the bot app is installed there and - holds `contents: write` and `pull-requests: write`. +- `error` means the repo could not be read or written. The app already holds the permissions the + sync needs, so this is usually a transient API failure, or the app not being installed on that + repo. Check the installation first on a repo that was added recently. - `not-migrated` means the repo has no `.github/workflows/automation.yml` at all. Either it has not been onboarded yet, or it belongs in `consumers` by mistake. Copy the template in, or remove the entry. - `toolchain-conflict` means a sync pull request merged before, the template has not changed since, and the file has drifted again. The repo's own tooling rewrites the copy, so the template is not stable under that toolchain. Fix the template rather than reopening the pull request. -- `declined` means a core maintainer closed the last sync pull request without merging it. The - workflow leaves that repo alone until the template changes again, so it stays drifted while the - run stays green. To restore it sooner, copy the template in by hand. -To onboard a repo, add it to `consumers` with the branch its pull requests must target. Leave -archived repos out, because Actions do not run on them. +A fourth result, `declined`, keeps the run green and asks for nothing. It means a core maintainer +closed the last sync pull request without merging it, so the workflow leaves that repo alone until +the template changes again. The repo stays drifted in the meantime. To restore it sooner, copy the +template in by hand. + +To onboard a repo, add it to `consumers` with the branch its pull requests must target, and make +sure that the bot app is installed on it. The app already holds the permissions the sync needs, so +installation is the only per-repo step. Leave archived repos out, because Actions do not run on them. A repo with its own pull request template, or a check on the description, can also carry a `body_template`. The workflow puts the generated text where `{{explanation}}` appears, so the body From d4714c7938a8dadaca0082c01e096b5885bba7c9 Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Wed, 23 Sep 2026 16:00:14 +0300 Subject: [PATCH 09/18] Name the bot app and tidy the sync section 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. --- docs/automation.md | 69 ++++++++++++++++++++++++---------------------- 1 file changed, 36 insertions(+), 33 deletions(-) diff --git a/docs/automation.md b/docs/automation.md index 90f4373..7743569 100644 --- a/docs/automation.md +++ b/docs/automation.md @@ -47,8 +47,9 @@ No edits required. Then set the secrets: | `CONTRIBUTIONS_SHEET_NAME` | no | Sheet name within the spreadsheet | | `GH_UPLOADER_GCP_SA_CREDENTIALS` | no | GCP service account credentials for Sheets access | -Every automation except `resolve-bot-pr-threads` authenticates as the bot, so the two required -secrets must be set. `resolve-bot-pr-threads` uses the default `GITHUB_TOKEN` instead. +Every automation except `resolve-bot-pr-threads` authenticates as `learning-equality-bot[bot]`, the +GitHub App behind `LE_BOT_APP_ID`, so the two required secrets must be set. `resolve-bot-pr-threads` +uses the default `GITHUB_TOKEN` instead. Optional means that you accept losing the automations that use the secret. It does not mean that they degrade gracefully. The generated caller forwards every key, so an absent secret reaches the @@ -71,48 +72,50 @@ leaf workflow as an empty string, and every automation that needs it fails at ru ## Keeping the copies in sync -Most registry changes reach consumers on their own, because their copied file only says -`uses: learningequality/.github/.github/workflows/automation.yml@main`. A consumer needs the file -again whenever its copy stops matching the template, which happens two ways. +Most registry changes reach consumers automatically because their copied file only says: +`uses: learningequality/.github/.github/workflows/automation.yml@main`. A consumer needs its file +updated whenever its copy no longer matches the template, which happens in two ways. -The template changes here, when a new event or activity type enters the `on:` union, when the -permissions widen, or when the secret list changes. Or the consumer's own tooling rewrites its copy, -which is what a `toolchain-conflict` below reports. +The template changes here, such as when a new event or activity type is added to the `on:` union, +permissions are widened, or the secret list changes. Or the consumer's own tooling rewrites its +copy, which is reported as a `toolchain-conflict` below. -`.github/workflows/sync-automation-template.yml` handles that. It runs weekly, on manual dispatch, +`.github/workflows/sync-automation-template.yml` handles this. It runs weekly, on manual dispatch, and whenever `automation-template.yml` changes on `main`. For each repo in the `consumers` list in -`automation-registry.yml`, it compares that repo's `.github/workflows/automation.yml` against the -template and opens a pull request where the two differ. A repo already in sync gets nothing, and a -repo with a sync pull request already open gets that pull request updated rather than a second one. +`automation-registry.yml`, it compares that repo's `.github/workflows/automation.yml` with the +template and opens a pull request when they differ. A repo that is already in sync gets nothing, +while a repo with an existing sync pull request has that pull request updated rather than a second +one opened. -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. A core maintainer in each consumer repo gives the -final review and merges, under that repo's own rules. +The workflow only proposes changes. It opens pull requests on a branch, never commits to a default +branch, and never merges, approves, or enables auto-merge. A core maintainer in each consumer repo +gives the final review and merges under that repo's own rules. -Run it with `dry_run` to see which repos have drifted without opening anything. +Run it with `dry_run` to see which repos have drifted without opening any pull requests. -Three results turn the run red and need someone to act: +Three results turn the run red and require action: -- `error` means the repo could not be read or written. The app already holds the permissions the - sync needs, so this is usually a transient API failure, or the app not being installed on that - repo. Check the installation first on a repo that was added recently. -- `not-migrated` means the repo has no `.github/workflows/automation.yml` at all. Either it has not - been onboarded yet, or it belongs in `consumers` by mistake. Copy the template in, or remove the +- `error` means the repo could not be read or written. The app already has the permissions required + for syncing, so this is usually a transient API failure or the app not being installed on that + repo. Check the installation first for a recently added repo. +- `not-migrated` means the repo has no `.github/workflows/automation.yml`. Either it has not been + onboarded yet, or it was added to `consumers` by mistake. Copy the template in, or remove the entry. -- `toolchain-conflict` means a sync pull request merged before, the template has not changed since, - and the file has drifted again. The repo's own tooling rewrites the copy, so the template is not +- `toolchain-conflict` means a sync pull request was merged, the template has not changed since, and + the file has drifted again. The repo's own tooling is rewriting the copy, so the template is not stable under that toolchain. Fix the template rather than reopening the pull request. -A fourth result, `declined`, keeps the run green and asks for nothing. It means a core maintainer +A fourth result, `declined`, keeps the run green and requires no action. It means a core maintainer closed the last sync pull request without merging it, so the workflow leaves that repo alone until -the template changes again. The repo stays drifted in the meantime. To restore it sooner, copy the +the template changes again. The repo remains drifted in the meantime. To restore it sooner, copy the template in by hand. To onboard a repo, add it to `consumers` with the branch its pull requests must target, and make -sure that the bot app is installed on it. The app already holds the permissions the sync needs, so -installation is the only per-repo step. Leave archived repos out, because Actions do not run on them. - -A repo with its own pull request template, or a check on the description, can also carry a -`body_template`. The workflow puts the generated text where `{{explanation}}` appears, so the body -follows that repo's own section order. `kolibri-design-system` needs one, because its -`check-description` job fails unless the body holds a Changelog block. +sure the `learning-equality-bot[bot]` app is installed on it. The app already has the permissions +required for syncing, so installation is the only per-repo step. Leave archived repos out because +Actions do not run on them. + +A repo with its own pull request template, or a check on the description, can also specify a +`body_template`. The workflow inserts the generated text wherever `{{explanation}}` appears, so the +body follows that repo's own section order. `kolibri-design-system` needs one because its +`check-description` job fails unless the body contains a Changelog block. From 042cb6d85aa60551738228cd71eb5d6f52ca754e Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Wed, 23 Sep 2026 18:05:21 +0300 Subject: [PATCH 10/18] Drop the redundant comment on the kolibri-design-system entry 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. --- automation-registry.yml | 3 --- 1 file changed, 3 deletions(-) diff --git a/automation-registry.yml b/automation-registry.yml index e0b014e..fc8b2e6 100644 --- a/automation-registry.yml +++ b/automation-registry.yml @@ -33,9 +33,6 @@ consumers: base: main - repo: kolibri-design-system base: develop - # Follows this repo's own pull request template. check-description fails - # unless the Changelog block holds a Description line that is not the - # placeholder text. body_template: | ## Description From 1df6628d5d8220dc053dde95f8219f808f4eb6c6 Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Wed, 23 Sep 2026 19:09:04 +0300 Subject: [PATCH 11/18] Discover the consumers instead of listing them 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. --- automation-registry.yml | 30 ++--- docs/automation.md | 38 +++--- scripts/sync-automation-template.js | 54 ++++++-- scripts/sync-automation-template.test.js | 149 +++++++++++++---------- 4 files changed, 154 insertions(+), 117 deletions(-) diff --git a/automation-registry.yml b/automation-registry.yml index fc8b2e6..dacd44e 100644 --- a/automation-registry.yml +++ b/automation-registry.yml @@ -15,24 +15,18 @@ # automation.yml and automation-template.yml. `node scripts/generate-automation.js --check` # fails if the generated files are out of date (enforced by pre-commit + CI). # -# `consumers` lists the repos that copy automation-template.yml. Each entry gives: -# repo - the repo name inside the learningequality org -# base - the branch the sync pull request targets -# body_template - optional pull request body for repos with their own template or -# description checks; {{explanation}} is replaced by the generated text +# scripts/sync-automation-template.js finds the consumers itself: every repo in the org holding a +# .github/workflows/automation.yml that calls this repo's automation.yml. It targets each repo's +# default branch, which is the only branch GitHub evaluates workflow triggers from. Archived repos +# and forks are skipped. # -# scripts/sync-automation-template.js opens one pull request per repo whose copy has drifted. -# Archived repos are left out, because Actions do not run on them and their copies are inert. +# `consumers` holds per-repo overrides only, so a repo belongs here only when it needs one: +# repo - the repo name inside the learningequality org +# body_template - pull request body for repos with their own template or a description +# check; {{explanation}} is replaced by the generated text consumers: - - repo: kolibri - base: develop - - repo: studio - base: unstable - - repo: ricecooker - base: main - repo: kolibri-design-system - base: develop body_template: | ## Description @@ -60,14 +54,6 @@ consumers: Opened by the sync workflow in `learningequality/.github`. The file is generated from `automation-registry.yml`, so no part of this change was written by hand. - - repo: le-utils - base: main - - repo: kolibri-data-portal - base: develop - - repo: morango - base: release-v0.9.x - - repo: kolibri-installer-debian - base: main automations: - name: review-requested enabled: true diff --git a/docs/automation.md b/docs/automation.md index 7743569..4e440cc 100644 --- a/docs/automation.md +++ b/docs/automation.md @@ -81,11 +81,16 @@ permissions are widened, or the secret list changes. Or the consumer's own tooli copy, which is reported as a `toolchain-conflict` below. `.github/workflows/sync-automation-template.yml` handles this. It runs weekly, on manual dispatch, -and whenever `automation-template.yml` changes on `main`. For each repo in the `consumers` list in -`automation-registry.yml`, it compares that repo's `.github/workflows/automation.yml` with the -template and opens a pull request when they differ. A repo that is already in sync gets nothing, -while a repo with an existing sync pull request has that pull request updated rather than a second -one opened. +and whenever `automation-template.yml` changes on `main`. + +It discovers the consumers by walking the org's repos, skipping archived ones and forks, and keeping +every repo whose `.github/workflows/automation.yml` calls this repo's `automation.yml`. Each pull +request targets that repo's default branch, which is the only branch GitHub evaluates workflow +triggers from. + +For each consumer it compares the copy with the template and opens a pull request when they differ. +A repo that is already in sync gets nothing, while a repo with an existing sync pull request has +that pull request updated rather than a second one opened. The workflow only proposes changes. It opens pull requests on a branch, never commits to a default branch, and never merges, approves, or enables auto-merge. A core maintainer in each consumer repo @@ -93,29 +98,26 @@ gives the final review and merges under that repo's own rules. Run it with `dry_run` to see which repos have drifted without opening any pull requests. -Three results turn the run red and require action: +Two results turn the run red and require action: - `error` means the repo could not be read or written. The app already has the permissions required for syncing, so this is usually a transient API failure or the app not being installed on that repo. Check the installation first for a recently added repo. -- `not-migrated` means the repo has no `.github/workflows/automation.yml`. Either it has not been - onboarded yet, or it was added to `consumers` by mistake. Copy the template in, or remove the - entry. - `toolchain-conflict` means a sync pull request was merged, the template has not changed since, and the file has drifted again. The repo's own tooling is rewriting the copy, so the template is not stable under that toolchain. Fix the template rather than reopening the pull request. -A fourth result, `declined`, keeps the run green and requires no action. It means a core maintainer +A third result, `declined`, keeps the run green and requires no action. It means a core maintainer closed the last sync pull request without merging it, so the workflow leaves that repo alone until the template changes again. The repo remains drifted in the meantime. To restore it sooner, copy the template in by hand. -To onboard a repo, add it to `consumers` with the branch its pull requests must target, and make -sure the `learning-equality-bot[bot]` app is installed on it. The app already has the permissions -required for syncing, so installation is the only per-repo step. Leave archived repos out because -Actions do not run on them. +To onboard a repo, copy the template in and make sure the `learning-equality-bot[bot]` app is +installed on it. The next run picks it up. A repo on which the app is not installed stays invisible +to the sync, so the installation is what enrols it. -A repo with its own pull request template, or a check on the description, can also specify a -`body_template`. The workflow inserts the generated text wherever `{{explanation}}` appears, so the -body follows that repo's own section order. `kolibri-design-system` needs one because its -`check-description` job fails unless the body contains a Changelog block. +A repo with its own pull request template, or a check on the description, needs an entry under +`consumers` in `automation-registry.yml` carrying a `body_template`. The workflow inserts the +generated text wherever `{{explanation}}` appears, so the body follows that repo's own section +order. `kolibri-design-system` needs one because its `check-description` job fails unless the body +contains a Changelog block. Every other consumer needs no entry at all. diff --git a/scripts/sync-automation-template.js b/scripts/sync-automation-template.js index d916586..edd16af 100644 --- a/scripts/sync-automation-template.js +++ b/scripts/sync-automation-template.js @@ -23,8 +23,11 @@ const TARGET_PATH = '.github/workflows/automation.yml'; const BRANCH = 'automation-template-sync'; const TITLE = 'Refresh automation.yml from the shared template'; const API = 'https://api.github.com'; +// A consumer copy calls the shared workflow. This repo's own reusable +// automation.yml sits at the same path and does not, so the marker excludes it. +const CONSUMER_MARKER = 'workflows/automation.yml@'; -const PROBLEM_STATES = ['error', 'toolchain-conflict', 'not-migrated']; +const PROBLEM_STATES = ['error', 'toolchain-conflict']; function httpApi(token) { return async function api(method, url, body) { @@ -71,15 +74,39 @@ function detail(r) { 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 - // whether the repo itself is readable before calling the file missing. - if (r.status === 404) { - const probe = await api('GET', `/repos/${ORG}/${repo}`); - return probe.ok ? { missing: true } : { error: `no access to ${repo} (${detail(probe)})` }; - } + if (r.status === 404) return { missing: true }; return { error: `read failed (${detail(r)})` }; } +/** + * Finds every repo in the org holding a copy of the template. Archived repos are + * skipped because Actions do not run on them, and forks because their copy + * belongs to the upstream repo. + */ +async function findConsumers(api, overrides) { + const repos = []; + for (let page = 1; ; page += 1) { + const r = await api('GET', `/orgs/${ORG}/repos?per_page=100&type=all&page=${page}`); + if (!r.ok) throw new Error(`could not list the org's repos (${detail(r)})`); + repos.push(...r.data); + if (r.data.length < 100) break; + } + + const consumers = []; + for (const repo of repos) { + if (repo.archived || repo.fork) continue; + const copy = await readCopy(api, repo.name, repo.default_branch); + if (copy.missing) continue; + if (copy.error) { + consumers.push({ repo: repo.name, base: repo.default_branch, unreadable: copy.error }); + continue; + } + if (!copy.content.includes(CONSUMER_MARKER)) continue; + consumers.push({ repo: repo.name, base: repo.default_branch, ...(overrides[repo.name] || {}) }); + } + return consumers; +} + async function lastTemplateChange(api) { try { const r = await api('GET', `/repos/${ORG}/.github/commits?path=automation-template.yml&per_page=1`); @@ -125,10 +152,11 @@ function classifyDrift(closedPr, templateChangedAt) { async function syncRepo(api, consumer, template, { dryRun, templateChangedAt }) { const { repo, base } = consumer; - const copy = await readCopy(api, repo, base); + if (consumer.unreadable) return { repo, state: 'error', detail: consumer.unreadable }; + const copy = await readCopy(api, repo, base); if (copy.error) return { repo, state: 'error', detail: copy.error }; - if (copy.missing) return { repo, state: 'not-migrated' }; + if (copy.missing) return { repo, state: 'error', detail: 'the copy disappeared during the run' }; if (copy.content === template) return { repo, state: 'in-sync' }; const open = await openSyncPr(api, repo); @@ -190,8 +218,12 @@ async function run(api, registry, template, options) { console.log(`::warning::could not read the template history (${change.detail}); treating drift as ordinary`); } const templateChangedAt = change.date; + + const overrides = Object.fromEntries((registry.consumers || []).map((c) => [c.repo, c])); + const consumers = await findConsumers(api, overrides); + const results = []; - for (const consumer of registry.consumers) { + for (const consumer of consumers) { try { results.push(await syncRepo(api, consumer, template, { ...options, templateChangedAt })); } catch (err) { @@ -229,4 +261,4 @@ async function main() { if (require.main === module) main(); -module.exports = { run, syncRepo, classifyDrift, report, PROBLEM_STATES, BRANCH, TARGET_PATH }; +module.exports = { run, syncRepo, classifyDrift, findConsumers, report, PROBLEM_STATES, BRANCH, TARGET_PATH }; diff --git a/scripts/sync-automation-template.test.js b/scripts/sync-automation-template.test.js index c4cb505..a0adea2 100644 --- a/scripts/sync-automation-template.test.js +++ b/scripts/sync-automation-template.test.js @@ -1,19 +1,20 @@ const test = require('node:test'); const assert = require('node:assert/strict'); -const { run, classifyDrift, report, PROBLEM_STATES, BRANCH } = require('./sync-automation-template'); +const { run, classifyDrift, findConsumers, report, PROBLEM_STATES, BRANCH } = require('./sync-automation-template'); -const TEMPLATE = 'name: Automation\non: {}\n'; -const STALE = 'name: Automation\non: {old: true}\n'; +const USES = 'jobs:\n automation:\n uses: learningequality/.github/.github/workflows/automation.yml@main\n'; +const TEMPLATE = `name: Automation\non: {}\n${USES}`; +const STALE = `name: Automation\non: {old: true}\n${USES}`; const TEMPLATE_BODY = '## Description\n\n{{explanation}}\n\n## Changelog\n\n - **Description:** test\n'; -// Not "main": five of the eight real consumers target something else, so a -// fixture on main cannot catch a hardcoded base. +// Not "main": a default branch that differs is the only way to catch a hardcoded base. const BASE = 'develop'; -const REGISTRY = { consumers: [{ repo: 'demo', base: BASE }] }; +const NO_OVERRIDES = { consumers: [] }; const encode = (s) => Buffer.from(s, 'utf8').toString('base64'); const ok = (data) => ({ ok: true, status: 200, data }); const fail = (status, message) => ({ ok: false, status, data: { message } }); +const repo = (over = {}) => ({ name: 'demo', default_branch: BASE, archived: false, fork: false, ...over }); /** * Routes calls by method and a fragment of the path. Later entries win, so a @@ -35,8 +36,9 @@ function makeApi(routes, calls = []) { }; } -const baseRoutes = (copy) => [ +const baseRoutes = (copy, repos = [repo()]) => [ ['GET /repos/learningequality/.github/commits', ok([{ commit: { committer: { date: '2026-01-02T00:00:00Z' } } }])], + ['GET /orgs/learningequality/repos', ok(repos)], ['GET contents/.github/workflows/automation.yml', ok({ sha: 'file-sha', content: encode(copy) })], ['GET /repos/learningequality/demo/pulls?state=open', ok([])], ['GET /repos/learningequality/demo/pulls?state=closed', ok([])], @@ -46,49 +48,48 @@ const baseRoutes = (copy) => [ ['POST /repos/learningequality/demo/pulls', ok({ number: 7, html_url: 'https://example.test/7' })], ]; -const only = async (routes, options = {}) => (await run(makeApi(routes), REGISTRY, TEMPLATE, options))[0]; +const only = async (routes, options = {}, registry = NO_OVERRIDES) => + (await run(makeApi(routes), registry, TEMPLATE, options))[0]; test('a matching copy is in sync', async () => { assert.equal((await only(baseRoutes(TEMPLATE))).state, 'in-sync'); }); test('a drifted copy opens a pull request', async () => { - const results = await run(makeApi(baseRoutes(STALE)), REGISTRY, TEMPLATE, {}); - assert.equal(results[0].state, 'opened'); - assert.equal(results[0].pr, 7); + const result = await only(baseRoutes(STALE)); + assert.equal(result.state, 'opened'); + assert.equal(result.pr, 7); }); test('the write targets the sync branch and carries the template', async () => { const calls = []; - await run(makeApi(baseRoutes(STALE), calls), REGISTRY, TEMPLATE, {}); + await run(makeApi(baseRoutes(STALE), calls), NO_OVERRIDES, TEMPLATE, {}); const put = calls.find((c) => c.method === 'PUT'); assert.equal(put.body.branch, BRANCH, 'must never write to a default branch'); assert.equal(Buffer.from(put.body.content, 'base64').toString('utf8'), TEMPLATE); }); -test('the pull request targets the consumer base, not a default of main', async () => { +test('the pull request targets the discovered default branch', async () => { const calls = []; - await run(makeApi(baseRoutes(STALE), calls), REGISTRY, TEMPLATE, {}); + await run(makeApi(baseRoutes(STALE), calls), NO_OVERRIDES, TEMPLATE, {}); const pr = calls.find((c) => c.method === 'POST' && c.url.endsWith('/pulls')); assert.equal(pr.body.base, BASE); assert.equal(pr.body.head, BRANCH); }); -test('a body_template keeps its own shape and receives the explanation', async () => { +test('an override supplies the body template for that repo only', async () => { const calls = []; - const registry = { consumers: [{ repo: 'demo', base: BASE, body_template: TEMPLATE_BODY }] }; + const registry = { consumers: [{ repo: 'demo', body_template: TEMPLATE_BODY }] }; await run(makeApi(baseRoutes(STALE), calls), registry, TEMPLATE, {}); const { body } = calls.find((c) => c.method === 'POST' && c.url.endsWith('/pulls')).body; assert.ok(body.startsWith('## Description'), 'the repo template decides the order, not the script'); assert.ok(body.includes('## Changelog'), 'KDS check-description needs the Changelog block'); - assert.ok(body.includes('automation-template.yml'), 'the explanation must replace the placeholder'); assert.ok(!body.includes('{{explanation}}')); - assert.ok(body.indexOf('## Description') < body.indexOf('## Changelog')); }); -test('a consumer with no template gets the plain explanation', async () => { +test('a consumer with no override gets the plain explanation', async () => { const calls = []; - await run(makeApi(baseRoutes(STALE), calls), REGISTRY, TEMPLATE, {}); + await run(makeApi(baseRoutes(STALE), calls), NO_OVERRIDES, TEMPLATE, {}); const { body } = calls.find((c) => c.method === 'POST' && c.url.endsWith('/pulls')).body; assert.ok(!body.includes('## ')); assert.ok(body.includes('automation-template.yml')); @@ -100,32 +101,47 @@ test('an open sync pull request is updated in place, keeping the file sha', asyn ...baseRoutes(STALE), ['GET /repos/learningequality/demo/pulls?state=open', ok([{ number: 3, html_url: 'https://example.test/3' }])], ]; - const results = await run(makeApi(routes, calls), REGISTRY, TEMPLATE, {}); + const results = await run(makeApi(routes, calls), NO_OVERRIDES, TEMPLATE, {}); assert.equal(results[0].state, 'updated'); assert.equal(calls.filter((c) => c.method === 'POST' && c.url.endsWith('/pulls')).length, 0); const put = calls.find((c) => c.method === 'PUT'); assert.equal(put.body.sha, 'file-sha', 'an update without the sha fails with a 422'); - assert.equal(put.body.branch, BRANCH); }); -test('a missing file in a readable repo is not-migrated', async () => { - const routes = [ - ...baseRoutes(STALE), - ['GET contents/.github/workflows/automation.yml', fail(404, 'Not Found')], - ['GET =/repos/learningequality/demo', ok({ name: 'demo' })], - ]; - assert.equal((await only(routes)).state, 'not-migrated'); +test('discovery skips archived repos and forks', async () => { + const repos = [repo({ name: 'old', archived: true }), repo({ name: 'mirror', fork: true }), repo()]; + const results = await run(makeApi(baseRoutes(TEMPLATE, repos)), NO_OVERRIDES, TEMPLATE, {}); + assert.deepEqual( + results.map((r) => r.repo), + ['demo'] + ); }); -test('a repo the token cannot see is an error, not not-migrated', async () => { +test('a repo with no copy is not a consumer', async () => { + const routes = [...baseRoutes(TEMPLATE), ['GET contents/.github/workflows/automation.yml', fail(404, 'Not Found')]]; + const results = await run(makeApi(routes), NO_OVERRIDES, TEMPLATE, {}); + assert.deepEqual(results, []); +}); + +test('a file that does not call the shared workflow is not a consumer', async () => { const routes = [ - ...baseRoutes(STALE), - ['GET contents/.github/workflows/automation.yml', fail(404, 'Not Found')], - ['GET =/repos/learningequality/demo', fail(404, 'Not Found')], + ...baseRoutes(TEMPLATE), + ['GET contents/.github/workflows/automation.yml', ok({ sha: 'x', content: encode('name: Something else\n') })], ]; + const results = await run(makeApi(routes), NO_OVERRIDES, TEMPLATE, {}); + assert.deepEqual(results, []); +}); + +test('a repo that cannot be read is reported, not skipped', async () => { + const routes = [...baseRoutes(TEMPLATE), ['GET contents/.github/workflows/automation.yml', fail(500, 'Server Error')]]; const result = await only(routes); assert.equal(result.state, 'error'); - assert.match(result.detail, /no access/); + assert.match(result.detail, /read failed/); +}); + +test('a failed repo listing stops the run loudly', async () => { + const routes = [...baseRoutes(TEMPLATE), ['GET /orgs/learningequality/repos', fail(403, 'Forbidden')]]; + await assert.rejects(() => run(makeApi(routes), NO_OVERRIDES, TEMPLATE, {}), /could not list the org's repos/); }); test('a failed pull request listing is an error, not a missing pull request', async () => { @@ -136,18 +152,23 @@ test('a failed pull request listing is an error, not a missing pull request', as }); test('a thrown request is contained and reported per repo', async () => { - const api = async (method, url) => { - if (url.includes('contents/')) throw new Error('socket hang up'); - return ok([]); - }; - const results = await run(api, REGISTRY, TEMPLATE, {}); - assert.equal(results[0].state, 'error'); - assert.equal(results[0].detail, 'socket hang up'); + const routes = [ + ...baseRoutes(STALE), + [ + 'GET /repos/learningequality/demo/pulls?state=open', + () => { + throw new Error('socket hang up'); + }, + ], + ]; + const result = await only(routes); + assert.equal(result.state, 'error'); + assert.equal(result.detail, 'socket hang up'); }); test('dry run reports without writing', async () => { const calls = []; - const results = await run(makeApi(baseRoutes(STALE), calls), REGISTRY, TEMPLATE, { dryRun: true }); + const results = await run(makeApi(baseRoutes(STALE), calls), NO_OVERRIDES, TEMPLATE, { dryRun: true }); assert.equal(results[0].state, 'would-open'); assert.equal( calls.filter((c) => c.method !== 'GET').length, @@ -184,13 +205,12 @@ test('a repo that reverted a merged sync is reported, and no pull request is ope ok([{ number: 9, merged_at: '2026-03-01T00:00:00Z', closed_at: '2026-03-01T00:00:00Z' }]), ], ]; - const results = await run(makeApi(routes, calls), REGISTRY, TEMPLATE, {}); + const results = await run(makeApi(routes, calls), NO_OVERRIDES, TEMPLATE, {}); assert.equal(results[0].state, 'toolchain-conflict'); assert.equal(calls.filter((c) => c.method !== 'GET').length, 0); }); test('an unreadable template history proposes rather than stopping the repo', async () => { - const calls = []; const routes = [ ...baseRoutes(STALE), ['GET /repos/learningequality/.github/commits', fail(502, 'Bad Gateway')], @@ -199,22 +219,8 @@ test('an unreadable template history proposes rather than stopping the repo', as ok([{ number: 9, merged_at: '2026-03-01T00:00:00Z', closed_at: '2026-03-01T00:00:00Z' }]), ], ]; - const results = await run(makeApi(routes, calls), REGISTRY, TEMPLATE, {}); - assert.equal(results[0].state, 'opened', 'a 502 must not read as a toolchain conflict'); -}); - -test('a thrown template history request does not kill the run', async () => { - const routes = [ - ...baseRoutes(STALE), - [ - 'GET /repos/learningequality/.github/commits', - () => { - throw new Error('socket hang up'); - }, - ], - ]; - const results = await run(makeApi(routes), REGISTRY, TEMPLATE, {}); - assert.equal(results[0].state, 'opened'); + const result = await only(routes); + assert.equal(result.state, 'opened', 'a 502 must not read as a toolchain conflict'); }); test('report counts only the states that stop a merge', () => { @@ -225,11 +231,10 @@ test('report counts only the states that stop a merge', () => { { repo: 'a', state: 'in-sync' }, { repo: 'b', state: 'opened', pr: 1 }, { repo: 'c', state: 'declined', pr: 2 }, - { repo: 'd', state: 'not-migrated' }, - { repo: 'e', state: 'error', detail: 'no access' }, - { repo: 'f', state: 'toolchain-conflict', pr: 3 }, + { repo: 'd', state: 'error', detail: 'no access' }, + { repo: 'e', state: 'toolchain-conflict', pr: 3 }, ]); - assert.equal(problems, 3, 'not-migrated, error and toolchain-conflict each need attention'); + assert.equal(problems, 2, 'error and toolchain-conflict each need attention'); } finally { console.log = quiet; } @@ -238,7 +243,7 @@ test('report counts only the states that stop a merge', () => { test('the set of problem states is closed', () => { assert.deepEqual( [...PROBLEM_STATES].sort(), - ['error', 'not-migrated', 'toolchain-conflict'], + ['error', 'toolchain-conflict'], 'adding a state here turns the weekly run red for repos that were passing, so change it deliberately and cover it in the report test above' ); }); @@ -246,9 +251,21 @@ test('the set of problem states is closed', () => { test('a stale branch is reset to base when no pull request is open', async () => { const calls = []; const routes = [...baseRoutes(STALE), ['POST /repos/learningequality/demo/git/refs', fail(422, 'Reference exists')]]; - await run(makeApi(routes, calls), REGISTRY, TEMPLATE, {}); + await run(makeApi(routes, calls), NO_OVERRIDES, TEMPLATE, {}); const reset = calls.find((c) => c.method === 'PATCH' && c.url.endsWith(`/git/refs/heads/${BRANCH}`)); assert.ok(reset, 'expected the branch to be reset'); assert.equal(reset.body.sha, 'base-sha'); assert.equal(reset.body.force, true); }); + +test('findConsumers pages through the org listing', async () => { + const calls = []; + const many = Array.from({ length: 100 }, (_, i) => repo({ name: `r${i}` })); + const routes = [ + ...baseRoutes(TEMPLATE, many), + ['GET /orgs/learningequality/repos', (url) => ok(url.includes('page=2') ? [repo()] : many)], + ]; + const consumers = await findConsumers(makeApi(routes, calls), {}); + assert.equal(consumers.length, 101); + assert.ok(calls.some((c) => c.url.includes('page=2'))); +}); From 82b1c72f93b49ce424f2fd301155d7e74512a05c Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Wed, 23 Sep 2026 19:52:06 +0300 Subject: [PATCH 12/18] Build the pull request body from the consumer's own template 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. --- automation-registry.yml | 39 ------------ docs/automation.md | 13 ++-- scripts/sync-automation-template.js | 69 ++++++++++++++------- scripts/sync-automation-template.test.js | 76 ++++++++++++++++-------- 4 files changed, 105 insertions(+), 92 deletions(-) diff --git a/automation-registry.yml b/automation-registry.yml index dacd44e..4e5d72f 100644 --- a/automation-registry.yml +++ b/automation-registry.yml @@ -14,46 +14,7 @@ # Run `node scripts/generate-automation.js` after editing this file to regenerate # automation.yml and automation-template.yml. `node scripts/generate-automation.js --check` # fails if the generated files are out of date (enforced by pre-commit + CI). -# -# scripts/sync-automation-template.js finds the consumers itself: every repo in the org holding a -# .github/workflows/automation.yml that calls this repo's automation.yml. It targets each repo's -# default branch, which is the only branch GitHub evaluates workflow triggers from. Archived repos -# and forks are skipped. -# -# `consumers` holds per-repo overrides only, so a repo belongs here only when it needs one: -# repo - the repo name inside the learningequality org -# body_template - pull request body for repos with their own template or a description -# check; {{explanation}} is replaced by the generated text - -consumers: - - repo: kolibri-design-system - body_template: | - ## Description - - {{explanation}} - - ## Changelog - - - - **Description:** Internal: refresh the copied automation.yml so it matches the current shared template - - **Products impact:** none - - **Addresses:** - - - **Components:** - - - **Breaking:** - - - **Impacts a11y:** - - - **Guidance:** - - - - - ## Steps to test - - 1. Confirm the diff touches only `.github/workflows/automation.yml`. - 2. Confirm the Automation workflow runs here and every job skips or succeeds. - - ## Comments - Opened by the sync workflow in `learningequality/.github`. The file is generated from - `automation-registry.yml`, so no part of this change was written by hand. automations: - name: review-requested enabled: true diff --git a/docs/automation.md b/docs/automation.md index 4e440cc..d1b5fb5 100644 --- a/docs/automation.md +++ b/docs/automation.md @@ -116,8 +116,11 @@ To onboard a repo, copy the template in and make sure the `learning-equality-bot installed on it. The next run picks it up. A repo on which the app is not installed stays invisible to the sync, so the installation is what enrols it. -A repo with its own pull request template, or a check on the description, needs an entry under -`consumers` in `automation-registry.yml` carrying a `body_template`. The workflow inserts the -generated text wherever `{{explanation}}` appears, so the body follows that repo's own section -order. `kolibri-design-system` needs one because its `check-description` job fails unless the body -contains a Changelog block. Every other consumer needs no entry at all. +The pull request body comes from the consumer's own pull request template when it has one, so it +follows that repo's sections and stays current as they change it. A repo with no template gets a +short explanation instead. + +Template fields are answered rather than copied, because a template ships each one with +instructions for a human author. `Description` gets a line naming the change, `Products impact` +gets `none`, and anything else gets a dash. This also satisfies a repo that checks the description +is no longer the placeholder, as `kolibri-design-system` does. diff --git a/scripts/sync-automation-template.js b/scripts/sync-automation-template.js index edd16af..1df8ef3 100644 --- a/scripts/sync-automation-template.js +++ b/scripts/sync-automation-template.js @@ -12,10 +12,8 @@ */ const fs = require('fs'); const path = require('path'); -const yaml = require('js-yaml'); const ROOT = path.join(__dirname, '..'); -const REGISTRY_PATH = path.join(ROOT, 'automation-registry.yml'); const TEMPLATE_PATH = path.join(ROOT, 'automation-template.yml'); const ORG = 'learningequality'; @@ -53,24 +51,52 @@ function httpApi(token) { }; } -function prBody(consumer) { - const explanation = [ - `This replaces \`${TARGET_PATH}\` with the current \`automation-template.yml\` from`, - `[${ORG}/.github](https://github.com/${ORG}/.github).`, - '', - 'The file is generated. Do not edit this copy: edit `automation-registry.yml` upstream', - 'and regenerate, or the next sync will overwrite the change.', - '', - 'Opened automatically. A core maintainer reviews and merges it.', - ].join('\n'); - if (!consumer.body_template) return `${explanation}\n`; - return `${consumer.body_template.replaceAll('{{explanation}}', explanation).trimEnd()}\n`; +const EXPLANATION = [ + `This replaces \`${TARGET_PATH}\` with the current \`automation-template.yml\` from`, + `[${ORG}/.github](https://github.com/${ORG}/.github).`, + '', + 'The file is generated. Do not edit this copy: edit `automation-registry.yml` upstream', + 'and regenerate, or the next sync will overwrite the change.', + '', + 'Opened automatically. A core maintainer reviews and merges it.', +].join('\n'); + +// A pull request template ships each field with instructions for a human author. +// Left alone they read as an unanswered form, and a repo can check that the +// description in particular is no longer the placeholder. +const SUMMARY = 'Internal: refresh the copied automation.yml so it matches the current shared template'; +const FIELD = /^([ \t]*-[ \t]*\*\*([^*]+?):\*\*).*$/gm; +const ANSWERS = { description: SUMMARY, 'products impact': 'none' }; + +function prBody(prTemplate) { + if (!prTemplate) return `${EXPLANATION}\n`; + const filled = prTemplate.replace(FIELD, (line, prefix, field) => { + const answer = ANSWERS[field.trim().toLowerCase()]; + return `${prefix} ${answer === undefined ? '-' : answer}`; + }); + return `${filled.trimEnd()}\n\n---\n\n${EXPLANATION}\n`; } function detail(r) { return `${r.status} ${(r.data && r.data.message) || ''}`.trim(); } +const PR_TEMPLATE_PATHS = [ + '.github/pull_request_template.md', + '.github/PULL_REQUEST_TEMPLATE.md', + 'PULL_REQUEST_TEMPLATE.md', + 'pull_request_template.md', + 'docs/PULL_REQUEST_TEMPLATE.md', +]; + +async function readPrTemplate(api, repo, ref) { + for (const p of PR_TEMPLATE_PATHS) { + const r = await api('GET', `/repos/${ORG}/${repo}/contents/${p}?ref=${ref}`); + if (r.ok) return Buffer.from(r.data.content, 'base64').toString('utf8'); + } + return null; +} + 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') }; @@ -83,7 +109,7 @@ async function readCopy(api, repo, ref) { * skipped because Actions do not run on them, and forks because their copy * belongs to the upstream repo. */ -async function findConsumers(api, overrides) { +async function findConsumers(api) { const repos = []; for (let page = 1; ; page += 1) { const r = await api('GET', `/orgs/${ORG}/repos?per_page=100&type=all&page=${page}`); @@ -102,7 +128,7 @@ async function findConsumers(api, overrides) { continue; } if (!copy.content.includes(CONSUMER_MARKER)) continue; - consumers.push({ repo: repo.name, base: repo.default_branch, ...(overrides[repo.name] || {}) }); + consumers.push({ repo: repo.name, base: repo.default_branch }); } return consumers; } @@ -206,21 +232,19 @@ async function syncRepo(api, consumer, template, { dryRun, templateChangedAt }) title: TITLE, head: BRANCH, base, - body: prBody(consumer), + body: prBody(await readPrTemplate(api, repo, base)), }); if (!pr.ok) return { repo, state: 'error', detail: `pull request failed (${detail(pr)})` }; return { repo, state: 'opened', pr: pr.data.number, url: pr.data.html_url }; } -async function run(api, registry, template, options) { +async function run(api, template, options) { const change = await lastTemplateChange(api); if (change.failed) { console.log(`::warning::could not read the template history (${change.detail}); treating drift as ordinary`); } const templateChangedAt = change.date; - - const overrides = Object.fromEntries((registry.consumers || []).map((c) => [c.repo, c])); - const consumers = await findConsumers(api, overrides); + const consumers = await findConsumers(api); const results = []; for (const consumer of consumers) { @@ -253,9 +277,8 @@ async function main() { console.error('GITHUB_TOKEN is not set.'); process.exit(1); } - const registry = yaml.load(fs.readFileSync(REGISTRY_PATH, 'utf8')); const template = fs.readFileSync(TEMPLATE_PATH, 'utf8'); - const results = await run(httpApi(token), registry, template, { dryRun: process.argv.includes('--dry-run') }); + const results = await run(httpApi(token), template, { dryRun: process.argv.includes('--dry-run') }); process.exit(report(results) ? 1 : 0); } diff --git a/scripts/sync-automation-template.test.js b/scripts/sync-automation-template.test.js index a0adea2..f19ff2f 100644 --- a/scripts/sync-automation-template.test.js +++ b/scripts/sync-automation-template.test.js @@ -6,11 +6,22 @@ const { run, classifyDrift, findConsumers, report, PROBLEM_STATES, BRANCH } = re const USES = 'jobs:\n automation:\n uses: learningequality/.github/.github/workflows/automation.yml@main\n'; const TEMPLATE = `name: Automation\non: {}\n${USES}`; const STALE = `name: Automation\non: {old: true}\n${USES}`; -const TEMPLATE_BODY = '## Description\n\n{{explanation}}\n\n## Changelog\n\n - **Description:** test\n'; +// Shaped like kolibri-design-system's, including the placeholder its own +// check-description job rejects. +const PR_TEMPLATE = [ + '## Description', + '', + '', + '', + '## Changelog', + '', + ' - **Description:** Summary of change(s)', + ' - **Products impact:** Choose from - none / bugfix / new API', + ' - **Breaking:** Choose from: yes / no', + '', +].join('\n'); // Not "main": a default branch that differs is the only way to catch a hardcoded base. const BASE = 'develop'; -const NO_OVERRIDES = { consumers: [] }; - const encode = (s) => Buffer.from(s, 'utf8').toString('base64'); const ok = (data) => ({ ok: true, status: 200, data }); const fail = (status, message) => ({ ok: false, status, data: { message } }); @@ -48,8 +59,7 @@ const baseRoutes = (copy, repos = [repo()]) => [ ['POST /repos/learningequality/demo/pulls', ok({ number: 7, html_url: 'https://example.test/7' })], ]; -const only = async (routes, options = {}, registry = NO_OVERRIDES) => - (await run(makeApi(routes), registry, TEMPLATE, options))[0]; +const only = async (routes, options = {}) => (await run(makeApi(routes), TEMPLATE, options))[0]; test('a matching copy is in sync', async () => { assert.equal((await only(baseRoutes(TEMPLATE))).state, 'in-sync'); @@ -63,7 +73,7 @@ test('a drifted copy opens a pull request', async () => { test('the write targets the sync branch and carries the template', async () => { const calls = []; - await run(makeApi(baseRoutes(STALE), calls), NO_OVERRIDES, TEMPLATE, {}); + await run(makeApi(baseRoutes(STALE), calls), TEMPLATE, {}); const put = calls.find((c) => c.method === 'PUT'); assert.equal(put.body.branch, BRANCH, 'must never write to a default branch'); assert.equal(Buffer.from(put.body.content, 'base64').toString('utf8'), TEMPLATE); @@ -71,25 +81,41 @@ test('the write targets the sync branch and carries the template', async () => { test('the pull request targets the discovered default branch', async () => { const calls = []; - await run(makeApi(baseRoutes(STALE), calls), NO_OVERRIDES, TEMPLATE, {}); + await run(makeApi(baseRoutes(STALE), calls), TEMPLATE, {}); const pr = calls.find((c) => c.method === 'POST' && c.url.endsWith('/pulls')); assert.equal(pr.body.base, BASE); assert.equal(pr.body.head, BRANCH); }); -test('an override supplies the body template for that repo only', async () => { +const withPrTemplate = (copy) => [ + ...baseRoutes(copy), + ['GET contents/.github/pull_request_template.md', ok({ content: encode(PR_TEMPLATE) })], +]; + +const bodyOf = (calls) => calls.find((c) => c.method === 'POST' && c.url.endsWith('/pulls')).body.body; + +test("the body uses the repo's own pull request template when it has one", async () => { + const calls = []; + await run(makeApi(withPrTemplate(STALE), calls), TEMPLATE, {}); + const body = bodyOf(calls); + assert.ok(body.startsWith('## Description'), 'the repo template decides the shape'); + assert.ok(body.includes(''), 'prose outside the fields is left alone'); + assert.ok(body.includes('automation-template.yml'), 'our explanation is still there'); +}); + +test('every field is answered, so the body is not a half-filled form', async () => { const calls = []; - const registry = { consumers: [{ repo: 'demo', body_template: TEMPLATE_BODY }] }; - await run(makeApi(baseRoutes(STALE), calls), registry, TEMPLATE, {}); - const { body } = calls.find((c) => c.method === 'POST' && c.url.endsWith('/pulls')).body; - assert.ok(body.startsWith('## Description'), 'the repo template decides the order, not the script'); - assert.ok(body.includes('## Changelog'), 'KDS check-description needs the Changelog block'); - assert.ok(!body.includes('{{explanation}}')); + await run(makeApi(withPrTemplate(STALE), calls), TEMPLATE, {}); + const body = bodyOf(calls); + assert.ok(!body.includes('Summary of change(s)'), 'the placeholder fails check-description'); + assert.match(body, /- \*\*Description:\*\* Internal: refresh the copied automation\.yml/); + assert.ok(body.includes('- **Breaking:** -'), 'a field we have no answer for gets a dash'); + assert.ok(!body.includes('yes / no'), 'instructions for a human author do not survive'); }); -test('a consumer with no override gets the plain explanation', async () => { +test('a repo with no pull request template gets the plain explanation', async () => { const calls = []; - await run(makeApi(baseRoutes(STALE), calls), NO_OVERRIDES, TEMPLATE, {}); + await run(makeApi(baseRoutes(STALE), calls), TEMPLATE, {}); const { body } = calls.find((c) => c.method === 'POST' && c.url.endsWith('/pulls')).body; assert.ok(!body.includes('## ')); assert.ok(body.includes('automation-template.yml')); @@ -101,7 +127,7 @@ test('an open sync pull request is updated in place, keeping the file sha', asyn ...baseRoutes(STALE), ['GET /repos/learningequality/demo/pulls?state=open', ok([{ number: 3, html_url: 'https://example.test/3' }])], ]; - const results = await run(makeApi(routes, calls), NO_OVERRIDES, TEMPLATE, {}); + const results = await run(makeApi(routes, calls), TEMPLATE, {}); assert.equal(results[0].state, 'updated'); assert.equal(calls.filter((c) => c.method === 'POST' && c.url.endsWith('/pulls')).length, 0); const put = calls.find((c) => c.method === 'PUT'); @@ -110,7 +136,7 @@ test('an open sync pull request is updated in place, keeping the file sha', asyn test('discovery skips archived repos and forks', async () => { const repos = [repo({ name: 'old', archived: true }), repo({ name: 'mirror', fork: true }), repo()]; - const results = await run(makeApi(baseRoutes(TEMPLATE, repos)), NO_OVERRIDES, TEMPLATE, {}); + const results = await run(makeApi(baseRoutes(TEMPLATE, repos)), TEMPLATE, {}); assert.deepEqual( results.map((r) => r.repo), ['demo'] @@ -119,7 +145,7 @@ test('discovery skips archived repos and forks', async () => { test('a repo with no copy is not a consumer', async () => { const routes = [...baseRoutes(TEMPLATE), ['GET contents/.github/workflows/automation.yml', fail(404, 'Not Found')]]; - const results = await run(makeApi(routes), NO_OVERRIDES, TEMPLATE, {}); + const results = await run(makeApi(routes), TEMPLATE, {}); assert.deepEqual(results, []); }); @@ -128,7 +154,7 @@ test('a file that does not call the shared workflow is not a consumer', async () ...baseRoutes(TEMPLATE), ['GET contents/.github/workflows/automation.yml', ok({ sha: 'x', content: encode('name: Something else\n') })], ]; - const results = await run(makeApi(routes), NO_OVERRIDES, TEMPLATE, {}); + const results = await run(makeApi(routes), TEMPLATE, {}); assert.deepEqual(results, []); }); @@ -141,7 +167,7 @@ test('a repo that cannot be read is reported, not skipped', async () => { test('a failed repo listing stops the run loudly', async () => { const routes = [...baseRoutes(TEMPLATE), ['GET /orgs/learningequality/repos', fail(403, 'Forbidden')]]; - await assert.rejects(() => run(makeApi(routes), NO_OVERRIDES, TEMPLATE, {}), /could not list the org's repos/); + await assert.rejects(() => run(makeApi(routes), TEMPLATE, {}), /could not list the org's repos/); }); test('a failed pull request listing is an error, not a missing pull request', async () => { @@ -168,7 +194,7 @@ test('a thrown request is contained and reported per repo', async () => { test('dry run reports without writing', async () => { const calls = []; - const results = await run(makeApi(baseRoutes(STALE), calls), NO_OVERRIDES, TEMPLATE, { dryRun: true }); + const results = await run(makeApi(baseRoutes(STALE), calls), TEMPLATE, { dryRun: true }); assert.equal(results[0].state, 'would-open'); assert.equal( calls.filter((c) => c.method !== 'GET').length, @@ -205,7 +231,7 @@ test('a repo that reverted a merged sync is reported, and no pull request is ope ok([{ number: 9, merged_at: '2026-03-01T00:00:00Z', closed_at: '2026-03-01T00:00:00Z' }]), ], ]; - const results = await run(makeApi(routes, calls), NO_OVERRIDES, TEMPLATE, {}); + const results = await run(makeApi(routes, calls), TEMPLATE, {}); assert.equal(results[0].state, 'toolchain-conflict'); assert.equal(calls.filter((c) => c.method !== 'GET').length, 0); }); @@ -251,7 +277,7 @@ test('the set of problem states is closed', () => { test('a stale branch is reset to base when no pull request is open', async () => { const calls = []; const routes = [...baseRoutes(STALE), ['POST /repos/learningequality/demo/git/refs', fail(422, 'Reference exists')]]; - await run(makeApi(routes, calls), NO_OVERRIDES, TEMPLATE, {}); + await run(makeApi(routes, calls), TEMPLATE, {}); const reset = calls.find((c) => c.method === 'PATCH' && c.url.endsWith(`/git/refs/heads/${BRANCH}`)); assert.ok(reset, 'expected the branch to be reset'); assert.equal(reset.body.sha, 'base-sha'); @@ -265,7 +291,7 @@ test('findConsumers pages through the org listing', async () => { ...baseRoutes(TEMPLATE, many), ['GET /orgs/learningequality/repos', (url) => ok(url.includes('page=2') ? [repo()] : many)], ]; - const consumers = await findConsumers(makeApi(routes, calls), {}); + const consumers = await findConsumers(makeApi(routes, calls)); assert.equal(consumers.length, 101); assert.ok(calls.some((c) => c.url.includes('page=2'))); }); From 2a700c65612fe46f366ef440bbcc2de74b26dfe4 Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Wed, 23 Sep 2026 20:48:02 +0300 Subject: [PATCH 13/18] Treat kolibri-design-system as the one template exception 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. --- docs/automation.md | 15 +++++----- scripts/sync-automation-template.js | 32 ++++++++++++--------- scripts/sync-automation-template.test.js | 36 ++++++++++++++++-------- 3 files changed, 50 insertions(+), 33 deletions(-) diff --git a/docs/automation.md b/docs/automation.md index d1b5fb5..ed516b0 100644 --- a/docs/automation.md +++ b/docs/automation.md @@ -116,11 +116,10 @@ To onboard a repo, copy the template in and make sure the `learning-equality-bot installed on it. The next run picks it up. A repo on which the app is not installed stays invisible to the sync, so the installation is what enrols it. -The pull request body comes from the consumer's own pull request template when it has one, so it -follows that repo's sections and stays current as they change it. A repo with no template gets a -short explanation instead. - -Template fields are answered rather than copied, because a template ships each one with -instructions for a human author. `Description` gets a line naming the change, `Products impact` -gets `none`, and anything else gets a dash. This also satisfies a repo that checks the description -is no longer the placeholder, as `kolibri-design-system` does. +The pull request body is a short explanation of what changed and why the file is generated. + +`kolibri-design-system` is the exception. Its `check-description` job fails unless the body carries +a Changelog section whose Description 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 and fills each +field, so the body is not a half-filled form. Nothing about that template is stored here, so it +stays current as they change it. diff --git a/scripts/sync-automation-template.js b/scripts/sync-automation-template.js index 1df8ef3..593e406 100644 --- a/scripts/sync-automation-template.js +++ b/scripts/sync-automation-template.js @@ -61,17 +61,13 @@ const EXPLANATION = [ 'Opened automatically. A core maintainer reviews and merges it.', ].join('\n'); -// A pull request template ships each field with instructions for a human author. -// Left alone they read as an unanswered form, and a repo can check that the -// description in particular is no longer the placeholder. const SUMMARY = 'Internal: refresh the copied automation.yml so it matches the current shared template'; const FIELD = /^([ \t]*-[ \t]*\*\*([^*]+?):\*\*).*$/gm; -const ANSWERS = { description: SUMMARY, 'products impact': 'none' }; -function prBody(prTemplate) { +function prBody(prTemplate, answers) { if (!prTemplate) return `${EXPLANATION}\n`; const filled = prTemplate.replace(FIELD, (line, prefix, field) => { - const answer = ANSWERS[field.trim().toLowerCase()]; + const answer = answers[field.trim().toLowerCase()]; return `${prefix} ${answer === undefined ? '-' : answer}`; }); return `${filled.trimEnd()}\n\n---\n\n${EXPLANATION}\n`; @@ -81,15 +77,23 @@ function detail(r) { return `${r.status} ${(r.data && r.data.message) || ''}`.trim(); } -const PR_TEMPLATE_PATHS = [ - '.github/pull_request_template.md', - '.github/PULL_REQUEST_TEMPLATE.md', - 'PULL_REQUEST_TEMPLATE.md', - 'pull_request_template.md', - 'docs/PULL_REQUEST_TEMPLATE.md', -]; +// kolibri-design-system's check-description job needs a Changelog section whose +// Description is not the placeholder its template ships with. +const KDS_REPO = 'kolibri-design-system'; +const KDS_TEMPLATE_ANSWERS = { + description: SUMMARY, + 'products impact': 'none', + addresses: '-', + components: '-', + breaking: '-', + 'impacts a11y': '-', + guidance: '-', +}; + +const PR_TEMPLATE_PATHS = ['.github/pull_request_template.md', '.github/PULL_REQUEST_TEMPLATE.md']; async function readPrTemplate(api, repo, ref) { + if (repo !== KDS_REPO) return null; for (const p of PR_TEMPLATE_PATHS) { const r = await api('GET', `/repos/${ORG}/${repo}/contents/${p}?ref=${ref}`); if (r.ok) return Buffer.from(r.data.content, 'base64').toString('utf8'); @@ -232,7 +236,7 @@ async function syncRepo(api, consumer, template, { dryRun, templateChangedAt }) title: TITLE, head: BRANCH, base, - body: prBody(await readPrTemplate(api, repo, base)), + body: prBody(await readPrTemplate(api, repo, base), KDS_TEMPLATE_ANSWERS), }); if (!pr.ok) return { repo, state: 'error', detail: `pull request failed (${detail(pr)})` }; return { repo, state: 'opened', pr: pr.data.number, url: pr.data.html_url }; diff --git a/scripts/sync-automation-template.test.js b/scripts/sync-automation-template.test.js index f19ff2f..05750e1 100644 --- a/scripts/sync-automation-template.test.js +++ b/scripts/sync-automation-template.test.js @@ -87,30 +87,44 @@ test('the pull request targets the discovered default branch', async () => { assert.equal(pr.body.head, BRANCH); }); -const withPrTemplate = (copy) => [ - ...baseRoutes(copy), +// The repo whose check-description job forces the template path. +const CHECKED = 'kolibri-design-system'; + +const checkedRoutes = (copy) => [ + ...baseRoutes(copy, [repo({ name: CHECKED })]), + [`GET /repos/learningequality/${CHECKED}/pulls?state=open`, ok([])], + [`GET /repos/learningequality/${CHECKED}/pulls?state=closed`, ok([])], + [`GET /repos/learningequality/${CHECKED}/git/ref/heads/${BASE}`, ok({ object: { sha: 'base-sha' } })], + [`POST /repos/learningequality/${CHECKED}/git/refs`, ok({})], + [`PUT /repos/learningequality/${CHECKED}/contents`, ok({})], + [`POST /repos/learningequality/${CHECKED}/pulls`, ok({ number: 7, html_url: 'https://example.test/7' })], ['GET contents/.github/pull_request_template.md', ok({ content: encode(PR_TEMPLATE) })], ]; const bodyOf = (calls) => calls.find((c) => c.method === 'POST' && c.url.endsWith('/pulls')).body.body; -test("the body uses the repo's own pull request template when it has one", async () => { +test('a repo that checks the description gets its own template, with every field answered', async () => { const calls = []; - await run(makeApi(withPrTemplate(STALE), calls), TEMPLATE, {}); + await run(makeApi(checkedRoutes(STALE), calls), TEMPLATE, {}); const body = bodyOf(calls); assert.ok(body.startsWith('## Description'), 'the repo template decides the shape'); - assert.ok(body.includes(''), 'prose outside the fields is left alone'); + assert.ok(!body.includes('Summary of change(s)'), 'the placeholder fails check-description'); + assert.match(body, /- \*\*Description:\*\* Internal: refresh the copied automation\.yml/); + assert.ok(body.includes('- **Breaking:** -'), 'a field we have no answer for gets a dash'); + assert.ok(!body.includes('yes / no'), 'instructions for a human author do not survive'); assert.ok(body.includes('automation-template.yml'), 'our explanation is still there'); }); -test('every field is answered, so the body is not a half-filled form', async () => { +test('any other repo gets the plain explanation, template or not', async () => { const calls = []; - await run(makeApi(withPrTemplate(STALE), calls), TEMPLATE, {}); + const routes = [ + ...baseRoutes(STALE), + ['GET contents/.github/pull_request_template.md', ok({ content: encode(PR_TEMPLATE) })], + ]; + await run(makeApi(routes, calls), TEMPLATE, {}); const body = bodyOf(calls); - assert.ok(!body.includes('Summary of change(s)'), 'the placeholder fails check-description'); - assert.match(body, /- \*\*Description:\*\* Internal: refresh the copied automation\.yml/); - assert.ok(body.includes('- **Breaking:** -'), 'a field we have no answer for gets a dash'); - assert.ok(!body.includes('yes / no'), 'instructions for a human author do not survive'); + assert.ok(!body.includes('## Description'), 'a template is not fetched for a repo that does not need it'); + assert.ok(body.includes('automation-template.yml')); }); test('a repo with no pull request template gets the plain explanation', async () => { From 26388607c095aa302515af615e9d4fb734f6182d Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Wed, 23 Sep 2026 20:53:43 +0300 Subject: [PATCH 14/18] Move the marker comment to the filter that uses 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. --- scripts/sync-automation-template.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/sync-automation-template.js b/scripts/sync-automation-template.js index 593e406..a173a4f 100644 --- a/scripts/sync-automation-template.js +++ b/scripts/sync-automation-template.js @@ -21,8 +21,6 @@ const TARGET_PATH = '.github/workflows/automation.yml'; const BRANCH = 'automation-template-sync'; const TITLE = 'Refresh automation.yml from the shared template'; const API = 'https://api.github.com'; -// A consumer copy calls the shared workflow. This repo's own reusable -// automation.yml sits at the same path and does not, so the marker excludes it. const CONSUMER_MARKER = 'workflows/automation.yml@'; const PROBLEM_STATES = ['error', 'toolchain-conflict']; @@ -131,6 +129,8 @@ async function findConsumers(api) { consumers.push({ repo: repo.name, base: repo.default_branch, unreadable: copy.error }); continue; } + // This repo's own reusable automation.yml sits at the same path and does not + // call the shared workflow, so this excludes it without a special case. if (!copy.content.includes(CONSUMER_MARKER)) continue; consumers.push({ repo: repo.name, base: repo.default_branch }); } From c2f51eafc5008657378cadf6cb0a67e62dda65c3 Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Wed, 23 Sep 2026 21:51:12 +0300 Subject: [PATCH 15/18] Contain a thrown read during discovery, and trim the kolibri-design-system 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. --- scripts/sync-automation-template.js | 60 +++++++++++++++++------- scripts/sync-automation-template.test.js | 38 ++++++++++++++- 2 files changed, 81 insertions(+), 17 deletions(-) diff --git a/scripts/sync-automation-template.js b/scripts/sync-automation-template.js index a173a4f..892315e 100644 --- a/scripts/sync-automation-template.js +++ b/scripts/sync-automation-template.js @@ -62,13 +62,30 @@ const EXPLANATION = [ const SUMMARY = 'Internal: refresh the copied automation.yml so it matches the current shared template'; const FIELD = /^([ \t]*-[ \t]*\*\*([^*]+?):\*\*).*$/gm; -function prBody(prTemplate, answers) { +// Splits on level-two headings only, so a deeper heading stays inside its section. +function sections(markdown) { + const out = []; + for (const line of markdown.split('\n')) { + if (/^## [^#]/.test(line)) out.push({ heading: line.trim(), lines: [] }); + else if (out.length) out[out.length - 1].lines.push(line); + } + return out; +} + +function prBody(prTemplate, { keep, describe, answers }) { if (!prTemplate) return `${EXPLANATION}\n`; - const filled = prTemplate.replace(FIELD, (line, prefix, field) => { - const answer = answers[field.trim().toLowerCase()]; - return `${prefix} ${answer === undefined ? '-' : answer}`; - }); - return `${filled.trimEnd()}\n\n---\n\n${EXPLANATION}\n`; + const body = sections(prTemplate) + .filter((s) => keep.includes(s.heading)) + .map((s) => { + if (s.heading === describe) return `${s.heading}\n\n${EXPLANATION}`; + const filled = s.lines.join('\n').replace(FIELD, (line, prefix, field) => { + const answer = answers[field.trim().toLowerCase()]; + return `${prefix} ${answer === undefined ? '-' : answer}`; + }); + return `${s.heading}\n${filled.trimEnd()}`; + }) + .join('\n\n'); + return `${body.trimEnd()}\n`; } function detail(r) { @@ -77,15 +94,21 @@ function detail(r) { // kolibri-design-system's check-description job needs a Changelog section whose // Description is not the placeholder its template ships with. +// Its template opens with "Please remove any unused sections", so only the two +// that carry content are kept. const KDS_REPO = 'kolibri-design-system'; -const KDS_TEMPLATE_ANSWERS = { - description: SUMMARY, - 'products impact': 'none', - addresses: '-', - components: '-', - breaking: '-', - 'impacts a11y': '-', - guidance: '-', +const KDS_TEMPLATE = { + keep: ['## Description', '## Changelog'], + describe: '## Description', + answers: { + description: SUMMARY, + 'products impact': 'none', + addresses: '-', + components: '-', + breaking: '-', + 'impacts a11y': '-', + guidance: '-', + }, }; const PR_TEMPLATE_PATHS = ['.github/pull_request_template.md', '.github/PULL_REQUEST_TEMPLATE.md']; @@ -123,7 +146,12 @@ async function findConsumers(api) { const consumers = []; for (const repo of repos) { if (repo.archived || repo.fork) continue; - const copy = await readCopy(api, repo.name, repo.default_branch); + let copy; + try { + copy = await readCopy(api, repo.name, repo.default_branch); + } catch (err) { + copy = { error: err.message }; + } if (copy.missing) continue; if (copy.error) { consumers.push({ repo: repo.name, base: repo.default_branch, unreadable: copy.error }); @@ -236,7 +264,7 @@ async function syncRepo(api, consumer, template, { dryRun, templateChangedAt }) title: TITLE, head: BRANCH, base, - body: prBody(await readPrTemplate(api, repo, base), KDS_TEMPLATE_ANSWERS), + body: prBody(await readPrTemplate(api, repo, base), KDS_TEMPLATE), }); if (!pr.ok) return { repo, state: 'error', detail: `pull request failed (${detail(pr)})` }; return { repo, state: 'opened', pr: pr.data.number, url: pr.data.html_url }; diff --git a/scripts/sync-automation-template.test.js b/scripts/sync-automation-template.test.js index 05750e1..7379932 100644 --- a/scripts/sync-automation-template.test.js +++ b/scripts/sync-automation-template.test.js @@ -9,16 +9,27 @@ const STALE = `name: Automation\non: {old: true}\n${USES}`; // Shaped like kolibri-design-system's, including the placeholder its own // check-description job rejects. const PR_TEMPLATE = [ + '', + '', '## Description', '', '', '', + '#### Issue addressed', + '', + 'Addresses #*PR# HERE*', + '', '## Changelog', '', ' - **Description:** Summary of change(s)', ' - **Products impact:** Choose from - none / bugfix / new API', ' - **Breaking:** Choose from: yes / no', '', + '## Steps to test', + '', + '1. Step 1', + '2. Step 2', + '', ].join('\n'); // Not "main": a default branch that differs is the only way to catch a hardcoded base. const BASE = 'develop'; @@ -115,6 +126,16 @@ test('a repo that checks the description gets its own template, with every field assert.ok(body.includes('automation-template.yml'), 'our explanation is still there'); }); +test('sections that would arrive unfilled are dropped', async () => { + const calls = []; + await run(makeApi(checkedRoutes(STALE), calls), TEMPLATE, {}); + const body = bodyOf(calls); + assert.ok(!body.includes('## Steps to test'), 'their template asks for unused sections to go'); + assert.ok(!body.includes('1. Step 1')); + assert.ok(!body.includes('Addresses #')); + assert.deepEqual(body.match(/^## .*/gm), ['## Description', '## Changelog']); +}); + test('any other repo gets the plain explanation, template or not', async () => { const calls = []; const routes = [ @@ -191,7 +212,7 @@ test('a failed pull request listing is an error, not a missing pull request', as assert.match(result.detail, /could not list/); }); -test('a thrown request is contained and reported per repo', async () => { +test('a thrown request during the sync is contained and reported per repo', async () => { const routes = [ ...baseRoutes(STALE), [ @@ -206,6 +227,21 @@ test('a thrown request is contained and reported per repo', async () => { assert.equal(result.detail, 'socket hang up'); }); +test('a thrown request during discovery is contained, not fatal', async () => { + const routes = [ + ...baseRoutes(STALE), + [ + 'GET contents/.github/workflows/automation.yml', + () => { + throw new Error('socket hang up'); + }, + ], + ]; + const result = await only(routes); + assert.equal(result.state, 'error', 'discovery runs before the per-repo try, so it needs its own'); + assert.equal(result.detail, 'socket hang up'); +}); + test('dry run reports without writing', async () => { const calls = []; const results = await run(makeApi(baseRoutes(STALE), calls), TEMPLATE, { dryRun: true }); From cfc4adcb08df33c16a0ed1499fd5135347d8e552 Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Wed, 23 Sep 2026 22:07:10 +0300 Subject: [PATCH 16/18] Keep the explanation when a kept heading is renamed 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. --- scripts/sync-automation-template.js | 24 +++++++++++------------ scripts/sync-automation-template.test.js | 25 ++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 12 deletions(-) diff --git a/scripts/sync-automation-template.js b/scripts/sync-automation-template.js index 892315e..0dff3b7 100644 --- a/scripts/sync-automation-template.js +++ b/scripts/sync-automation-template.js @@ -74,18 +74,18 @@ function sections(markdown) { function prBody(prTemplate, { keep, describe, answers }) { if (!prTemplate) return `${EXPLANATION}\n`; - const body = sections(prTemplate) - .filter((s) => keep.includes(s.heading)) - .map((s) => { - if (s.heading === describe) return `${s.heading}\n\n${EXPLANATION}`; - const filled = s.lines.join('\n').replace(FIELD, (line, prefix, field) => { - const answer = answers[field.trim().toLowerCase()]; - return `${prefix} ${answer === undefined ? '-' : answer}`; - }); - return `${s.heading}\n${filled.trimEnd()}`; - }) - .join('\n\n'); - return `${body.trimEnd()}\n`; + const kept = sections(prTemplate).filter((s) => keep.includes(s.heading)); + const parts = kept.map((s) => { + if (s.heading === describe) return `${s.heading}\n\n${EXPLANATION}`; + const filled = s.lines.join('\n').replace(FIELD, (line, prefix, field) => { + const answer = answers[field.trim().toLowerCase()]; + return `${prefix} ${answer === undefined ? '-' : answer}`; + }); + return `${s.heading}\n${filled.trimEnd()}`; + }); + // A renamed heading would otherwise leave the pull request unexplained, or empty. + if (!kept.some((s) => s.heading === describe)) parts.unshift(EXPLANATION); + return `${parts.join('\n\n').trimEnd()}\n`; } function detail(r) { diff --git a/scripts/sync-automation-template.test.js b/scripts/sync-automation-template.test.js index 7379932..cace5f6 100644 --- a/scripts/sync-automation-template.test.js +++ b/scripts/sync-automation-template.test.js @@ -126,6 +126,31 @@ test('a repo that checks the description gets its own template, with every field assert.ok(body.includes('automation-template.yml'), 'our explanation is still there'); }); +test('a renamed description heading still leaves the change explained', async () => { + const calls = []; + const renamed = PR_TEMPLATE.replace('## Description', '## Overview'); + const routes = [ + ...checkedRoutes(STALE), + ['GET contents/.github/pull_request_template.md', ok({ content: encode(renamed) })], + ]; + await run(makeApi(routes, calls), TEMPLATE, {}); + const body = bodyOf(calls); + assert.ok(body.startsWith('This replaces'), 'the explanation must not go missing'); + assert.ok(body.includes('## Changelog'), 'a section that survived is kept, so their check still passes'); +}); + +test('a template with none of the kept headings still explains itself', async () => { + const calls = []; + const routes = [ + ...checkedRoutes(STALE), + ['GET contents/.github/pull_request_template.md', ok({ content: encode('## Notes\n\nnothing here\n') })], + ]; + await run(makeApi(routes, calls), TEMPLATE, {}); + const body = bodyOf(calls); + assert.ok(body.includes('automation-template.yml'), 'the body must never be empty'); + assert.ok(body.trim().length, 'a bare newline is not a pull request body'); +}); + test('sections that would arrive unfilled are dropped', async () => { const calls = []; await run(makeApi(checkedRoutes(STALE), calls), TEMPLATE, {}); From 8c5d4e93b9e43f567903244f32e7d581267efd43 Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Thu, 24 Sep 2026 15:43:56 +0300 Subject: [PATCH 17/18] Let SYNC_ORG point the sync at another org The org name was fixed, so the write path could only be exercised against production. Set SYNC_ORG to run the whole thing against a test org instead, with a token scoped to the repos there. Discovery only sees what the token can see, so the blast radius is set by the credential rather than by the code being correct. The default is unchanged, so the workflow behaves exactly as before. --- scripts/sync-automation-template.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/scripts/sync-automation-template.js b/scripts/sync-automation-template.js index 0dff3b7..275da86 100644 --- a/scripts/sync-automation-template.js +++ b/scripts/sync-automation-template.js @@ -16,7 +16,9 @@ const path = require('path'); const ROOT = path.join(__dirname, '..'); const TEMPLATE_PATH = path.join(ROOT, 'automation-template.yml'); -const ORG = 'learningequality'; +// SYNC_ORG points the whole run at another org, so the write path can be +// exercised somewhere other than production. +const ORG = process.env.SYNC_ORG || 'learningequality'; const TARGET_PATH = '.github/workflows/automation.yml'; const BRANCH = 'automation-template-sync'; const TITLE = 'Refresh automation.yml from the shared template'; From 92e2dceab1e52b2264b2ecf5dcf6c3f9888007eb Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Thu, 24 Sep 2026 19:32:02 +0300 Subject: [PATCH 18/18] Classify drift by content rather than by commit dates A commit carries the date it was written, not the date it reached main. The commit that last touched automation-template.yml reads 2026-09-16, but it landed on 2026-09-21 when #88 merged. Any consumer that merged a sync pull request in those five days would, on its next real drift, be told the template had not changed since, reported as a toolchain conflict, and stop syncing. The comparison is now against what the last closed sync pull request left behind: the file at its merge commit once merged, or at its head when it was closed unmerged. Matching the current template means nothing new has been published, so the difference came from the consumer. Anything else means the template moved on and this is ordinary drift. That removes the template history read, and with it the fallback for when that read failed. Reported by rtibbles on #97. --- scripts/sync-automation-template.js | 56 +++++++++---------- scripts/sync-automation-template.test.js | 68 +++++++++++++++--------- 2 files changed, 67 insertions(+), 57 deletions(-) diff --git a/scripts/sync-automation-template.js b/scripts/sync-automation-template.js index 275da86..58b1ac0 100644 --- a/scripts/sync-automation-template.js +++ b/scripts/sync-automation-template.js @@ -167,17 +167,6 @@ async function findConsumers(api) { return consumers; } -async function lastTemplateChange(api) { - try { - const r = await api('GET', `/repos/${ORG}/.github/commits?path=automation-template.yml&per_page=1`); - if (!r.ok) return { date: null, failed: true, detail: detail(r) }; - if (!r.data.length) return { date: null, failed: false }; - return { date: r.data[0].commit.committer.date, failed: false }; - } catch (err) { - return { date: null, failed: true, detail: err.message }; - } -} - async function openSyncPr(api, repo) { const r = await api('GET', `/repos/${ORG}/${repo}/pulls?state=open&head=${ORG}:${BRANCH}`); if (!r.ok) return { error: `could not list pull requests (${detail(r)})` }; @@ -194,23 +183,28 @@ async function lastClosedSyncPr(api, repo) { } /** - * Decides what a drifted copy means, given the last closed sync pull request. - * A template change after that pull request closed is ordinary drift. With no - * such change, a merged pull request means the consumer reverted the file, and a - * closed one means a maintainer declined it. + * Decides what a drifted copy means, given what the last closed sync pull + * request left behind. `proposed` is the file as that pull request had it. + * + * Matching the template means nothing new has been published since, so the + * difference came from the consumer: a merge they then reverted, or a pull + * request a maintainer declined. Otherwise the template has moved on. * - * An unknown template date resolves to drift. A needless pull request costs one - * review, where a wrong toolchain-conflict stops syncing the repo entirely. + * Content rather than dates, because a commit carries the date it was written + * rather than the date it reached main, and a branch can be merged long after. */ -function classifyDrift(closedPr, templateChangedAt) { - if (!closedPr || !templateChangedAt) return 'drift'; - const closedAt = closedPr.merged_at || closedPr.closed_at; - if (!closedAt) return 'drift'; - if (new Date(templateChangedAt) > new Date(closedAt)) return 'drift'; +function classifyDrift(closedPr, proposed, template) { + if (!closedPr || proposed !== template) return 'drift'; return closedPr.merged_at ? 'toolchain-conflict' : 'declined'; } -async function syncRepo(api, consumer, template, { dryRun, templateChangedAt }) { +// The reference the pull request left behind: its merge commit once merged, +// otherwise its head, which stays readable after the branch is reset. +function closedPrRef(closedPr) { + return closedPr.merged_at ? closedPr.merge_commit_sha : closedPr.head && closedPr.head.sha; +} + +async function syncRepo(api, consumer, template, { dryRun }) { const { repo, base } = consumer; if (consumer.unreadable) return { repo, state: 'error', detail: consumer.unreadable }; @@ -225,8 +219,13 @@ async function syncRepo(api, consumer, template, { dryRun, templateChangedAt }) if (!open.pr) { const closed = await lastClosedSyncPr(api, repo); if (closed.error) return { repo, state: 'error', detail: closed.error }; - const verdict = classifyDrift(closed.pr, templateChangedAt); - if (verdict !== 'drift') return { repo, state: verdict, pr: closed.pr.number }; + if (closed.pr) { + const ref = closedPrRef(closed.pr); + const proposed = ref ? await readCopy(api, repo, ref) : { error: 'no reference on the closed pull request' }; + if (proposed.error) return { repo, state: 'error', detail: proposed.error }; + const verdict = classifyDrift(closed.pr, proposed.content, template); + if (verdict !== 'drift') return { repo, state: verdict, pr: closed.pr.number }; + } } if (dryRun) { @@ -273,17 +272,12 @@ async function syncRepo(api, consumer, template, { dryRun, templateChangedAt }) } async function run(api, template, options) { - const change = await lastTemplateChange(api); - if (change.failed) { - console.log(`::warning::could not read the template history (${change.detail}); treating drift as ordinary`); - } - const templateChangedAt = change.date; const consumers = await findConsumers(api); const results = []; for (const consumer of consumers) { try { - results.push(await syncRepo(api, consumer, template, { ...options, templateChangedAt })); + results.push(await syncRepo(api, consumer, template, options)); } catch (err) { results.push({ repo: consumer.repo, state: 'error', detail: err.message }); } diff --git a/scripts/sync-automation-template.test.js b/scripts/sync-automation-template.test.js index cace5f6..0d99dc4 100644 --- a/scripts/sync-automation-template.test.js +++ b/scripts/sync-automation-template.test.js @@ -277,51 +277,67 @@ test('dry run reports without writing', async () => { ); }); +const MERGED = { merged_at: '2026-03-01T00:00:00Z', merge_commit_sha: 'm1' }; +const CLOSED = { merged_at: null, head: { sha: 'h1' } }; + test('classifyDrift: no prior pull request is ordinary drift', () => { - assert.equal(classifyDrift(null, '2026-01-02T00:00:00Z'), 'drift'); + assert.equal(classifyDrift(null, undefined, TEMPLATE), 'drift'); }); -test('classifyDrift: a template change after a merge is ordinary drift', () => { - const merged = { merged_at: '2026-01-01T00:00:00Z', closed_at: '2026-01-01T00:00:00Z' }; - assert.equal(classifyDrift(merged, '2026-02-01T00:00:00Z'), 'drift'); +test('classifyDrift: a template published since the merge is ordinary drift', () => { + assert.equal(classifyDrift(MERGED, STALE, TEMPLATE), 'drift'); }); -test('classifyDrift: drift with no template change after a merge is a toolchain conflict', () => { - const merged = { merged_at: '2026-03-01T00:00:00Z', closed_at: '2026-03-01T00:00:00Z' }; - assert.equal(classifyDrift(merged, '2026-01-01T00:00:00Z'), 'toolchain-conflict'); +test('classifyDrift: drift with the template unchanged since the merge is a toolchain conflict', () => { + assert.equal(classifyDrift(MERGED, TEMPLATE, TEMPLATE), 'toolchain-conflict'); }); test('classifyDrift: a pull request closed unmerged is declined until the template moves', () => { - const closed = { merged_at: null, closed_at: '2026-03-01T00:00:00Z' }; - assert.equal(classifyDrift(closed, '2026-01-01T00:00:00Z'), 'declined'); - assert.equal(classifyDrift(closed, '2026-04-01T00:00:00Z'), 'drift'); + assert.equal(classifyDrift(CLOSED, TEMPLATE, TEMPLATE), 'declined'); + assert.equal(classifyDrift(CLOSED, STALE, TEMPLATE), 'drift'); }); +const mergedSync = (sha) => [ + 'GET /repos/learningequality/demo/pulls?state=closed', + ok([{ number: 9, merged_at: '2026-03-01T00:00:00Z', merge_commit_sha: sha }]), +]; + +// The copy at a given ref, so a test can say what the closed pull request left. +const atRef = (sha, content) => [ + `GET contents/.github/workflows/automation.yml?ref=${sha}`, + ok({ sha: 'x', content: encode(content) }), +]; + test('a repo that reverted a merged sync is reported, and no pull request is opened', async () => { const calls = []; - const routes = [ - ...baseRoutes(STALE), - [ - 'GET /repos/learningequality/demo/pulls?state=closed', - ok([{ number: 9, merged_at: '2026-03-01T00:00:00Z', closed_at: '2026-03-01T00:00:00Z' }]), - ], - ]; + const routes = [...baseRoutes(STALE), mergedSync('m1'), atRef('m1', TEMPLATE)]; const results = await run(makeApi(routes, calls), TEMPLATE, {}); - assert.equal(results[0].state, 'toolchain-conflict'); + assert.equal(results[0].state, 'toolchain-conflict', 'the merge left the template, so the consumer changed it'); assert.equal(calls.filter((c) => c.method !== 'GET').length, 0); }); -test('an unreadable template history proposes rather than stopping the repo', async () => { +test('a template published after the merge is drift, whatever the commit dates say', async () => { + const calls = []; + // The case dates get wrong: a template whose commits predate the sync merge, + // but which reached main after it, because the branch was merged later. + const routes = [...baseRoutes(STALE), mergedSync('m1'), atRef('m1', STALE)]; + const results = await run(makeApi(routes, calls), TEMPLATE, {}); + assert.equal(results[0].state, 'opened', 'the merge left something older, so the template moved on'); +}); + +test('a closed unmerged pull request is read at its head', async () => { + const calls = []; const routes = [ ...baseRoutes(STALE), - ['GET /repos/learningequality/.github/commits', fail(502, 'Bad Gateway')], - [ - 'GET /repos/learningequality/demo/pulls?state=closed', - ok([{ number: 9, merged_at: '2026-03-01T00:00:00Z', closed_at: '2026-03-01T00:00:00Z' }]), - ], + ['GET /repos/learningequality/demo/pulls?state=closed', ok([{ number: 9, merged_at: null, head: { sha: 'h1' } }])], + atRef('h1', TEMPLATE), ]; - const result = await only(routes); - assert.equal(result.state, 'opened', 'a 502 must not read as a toolchain conflict'); + const results = await run(makeApi(routes, calls), TEMPLATE, {}); + assert.equal(results[0].state, 'declined'); + assert.ok( + calls.some((c) => c.url.includes('ref=h1')), + 'the head stays readable after the branch is reset' + ); }); test('report counts only the states that stop a merge', () => {