From e4975dcd879e0fcc0cc8d50479d2b97b8a562487 Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Wed, 21 Oct 2026 16:41:06 +0300 Subject: [PATCH 01/12] Check that a pull request body follows the template Contributors open pull requests that leave required sections empty, and reviewers ask for the missing information by hand. The check resolves the template the way GitHub does, the repo's own copy first and the shared one second, then reads which sections that template marks as required. A section is required when its heading carries a REQUIRED comment, and a template that marks none requires every section, so a repo is strict rather than silently unchecked. It reports a required section that is missing, empty, or still holding the placeholder, and it names every one of them, because the job log is the only channel it has. The template is read from the base branch, so a pull request cannot relax the rules it is judged by. Bot authors are exempt, as the kolibri-design-system changelog check already does. --- .github/workflows/automation-caller.yml | 2 + .../automation-group-pull-request-target.yml | 13 +- .github/workflows/automation.yml | 6 +- .github/workflows/pull-request-template.yml | 41 +++ automation-registry.yml | 10 + automation-template.yml | 2 + scripts/pull-request-template.js | 146 +++++++++++ scripts/pull-request-template.test.js | 244 ++++++++++++++++++ 8 files changed, 459 insertions(+), 5 deletions(-) create mode 100644 .github/workflows/pull-request-template.yml create mode 100644 scripts/pull-request-template.js create mode 100644 scripts/pull-request-template.test.js diff --git a/.github/workflows/automation-caller.yml b/.github/workflows/automation-caller.yml index e465ad5..5b00f1f 100644 --- a/.github/workflows/automation-caller.yml +++ b/.github/workflows/automation-caller.yml @@ -21,8 +21,10 @@ on: # zizmor: ignore[dangerous-triggers] - created pull_request_target: types: + - edited - labeled - opened + - reopened - review_requested schedule: - cron: 1 0 * * 1 diff --git a/.github/workflows/automation-group-pull-request-target.yml b/.github/workflows/automation-group-pull-request-target.yml index 6bb2f97..fcedadd 100644 --- a/.github/workflows/automation-group-pull-request-target.yml +++ b/.github/workflows/automation-group-pull-request-target.yml @@ -8,10 +8,10 @@ on: workflow_call: secrets: LE_BOT_APP_ID: - description: "Forwarded to: review-requested, pull-request-label, dependabot-reviewer, contributor-pr-reply" + description: "Forwarded to: review-requested, pull-request-label, dependabot-reviewer, contributor-pr-reply, pull-request-template" required: true LE_BOT_PRIVATE_KEY: - description: "Forwarded to: review-requested, pull-request-label, dependabot-reviewer, contributor-pr-reply" + description: "Forwarded to: review-requested, pull-request-label, dependabot-reviewer, contributor-pr-reply, pull-request-template" required: true SLACK_COMMUNITY_NOTIFICATIONS_WEBHOOK_URL: description: "Forwarded to: contributor-pr-reply" @@ -54,3 +54,12 @@ jobs: LE_BOT_APP_ID: ${{ secrets.LE_BOT_APP_ID }} LE_BOT_PRIVATE_KEY: ${{ secrets.LE_BOT_PRIVATE_KEY }} SLACK_COMMUNITY_NOTIFICATIONS_WEBHOOK_URL: ${{ secrets.SLACK_COMMUNITY_NOTIFICATIONS_WEBHOOK_URL }} + pull-request-template: + name: Pull Request Template + if: ${{ github.event_name == 'pull_request_target' && contains(fromJSON('["opened","edited","reopened"]'), github.event.action) }} + uses: learningequality/.github/.github/workflows/pull-request-template.yml@main + permissions: + contents: read + secrets: + LE_BOT_APP_ID: ${{ secrets.LE_BOT_APP_ID }} + LE_BOT_PRIVATE_KEY: ${{ secrets.LE_BOT_PRIVATE_KEY }} diff --git a/.github/workflows/automation.yml b/.github/workflows/automation.yml index fea55fd..18d6ccf 100644 --- a/.github/workflows/automation.yml +++ b/.github/workflows/automation.yml @@ -18,10 +18,10 @@ on: description: Unused - accepted until every consumer stops forwarding it required: false LE_BOT_APP_ID: - description: "Forwarded to: review-requested, pull-request-label, dependabot-reviewer, contributor-pr-reply, issue-open, issue-label-header, good-first-issue-comment, community-contribution-label, contributor-issue-comment, unassign-inactive-issues" + description: "Forwarded to: review-requested, pull-request-label, dependabot-reviewer, contributor-pr-reply, pull-request-template, issue-open, issue-label-header, good-first-issue-comment, community-contribution-label, contributor-issue-comment, unassign-inactive-issues" required: true LE_BOT_PRIVATE_KEY: - description: "Forwarded to: review-requested, pull-request-label, dependabot-reviewer, contributor-pr-reply, issue-open, issue-label-header, good-first-issue-comment, community-contribution-label, contributor-issue-comment, unassign-inactive-issues" + description: "Forwarded to: review-requested, pull-request-label, dependabot-reviewer, contributor-pr-reply, pull-request-template, issue-open, issue-label-header, good-first-issue-comment, community-contribution-label, contributor-issue-comment, unassign-inactive-issues" required: true SLACK_COMMUNITY_NOTIFICATIONS_WEBHOOK_URL: description: "Forwarded to: contributor-pr-reply, contributor-issue-comment, unassign-inactive-issues" @@ -32,7 +32,7 @@ on: jobs: pull-request-target: name: Pull Request Target - if: ${{ (github.event_name == 'pull_request_target' && github.event.action == 'review_requested') || (github.event_name == 'pull_request_target' && github.event.action == 'labeled') || (github.event_name == 'pull_request_target' && github.event.action == 'opened' && github.event.pull_request.user.login == 'dependabot[bot]') || (github.event_name == 'pull_request_target' && github.event.action == 'opened') }} + if: ${{ (github.event_name == 'pull_request_target' && github.event.action == 'review_requested') || (github.event_name == 'pull_request_target' && github.event.action == 'labeled') || (github.event_name == 'pull_request_target' && github.event.action == 'opened' && github.event.pull_request.user.login == 'dependabot[bot]') || (github.event_name == 'pull_request_target' && github.event.action == 'opened') || (github.event_name == 'pull_request_target' && contains(fromJSON('["opened","edited","reopened"]'), github.event.action)) }} uses: learningequality/.github/.github/workflows/automation-group-pull-request-target.yml@main permissions: contents: read diff --git a/.github/workflows/pull-request-template.yml b/.github/workflows/pull-request-template.yml new file mode 100644 index 0000000..91610e9 --- /dev/null +++ b/.github/workflows/pull-request-template.yml @@ -0,0 +1,41 @@ +name: Check that a pull request follows the template +on: + workflow_call: + secrets: + LE_BOT_APP_ID: + description: 'GitHub App ID for authentication' + required: true + LE_BOT_PRIVATE_KEY: + description: 'GitHub App Private Key for authentication' + required: true +jobs: + check-template: + name: Check template + 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 }} + - name: Checkout .github repository + uses: actions/checkout@v7 + with: + repository: learningequality/.github + ref: main + token: ${{ steps.generate-token.outputs.token }} + - name: Setup Node.js + uses: actions/setup-node@v7 + with: + node-version: '20' + - name: Install dependencies + run: yarn install --frozen-lockfile + - name: Run script + id: script + uses: actions/github-script@v9 + with: + github-token: ${{ steps.generate-token.outputs.token }} + script: | + const script = require('./scripts/pull-request-template.js'); + return await script({ github, context, core }); diff --git a/automation-registry.yml b/automation-registry.yml index 2080521..27a0ac9 100644 --- a/automation-registry.yml +++ b/automation-registry.yml @@ -64,6 +64,16 @@ automations: LE_BOT_APP_ID: required LE_BOT_PRIVATE_KEY: required SLACK_COMMUNITY_NOTIFICATIONS_WEBHOOK_URL: optional + - name: pull-request-template + enabled: true + leaf: pull-request-template.yml + on: + pull_request_target: + types: [opened, edited, reopened] + if: "github.event_name == 'pull_request_target' && contains(fromJSON('[\"opened\",\"edited\",\"reopened\"]'), github.event.action)" + secrets: + LE_BOT_APP_ID: required + LE_BOT_PRIVATE_KEY: required - name: holiday-message # Seasonal. The leaf has no date guard, so this must be flipped on before # the holidays and off after them. diff --git a/automation-template.yml b/automation-template.yml index 321b23a..bb4e2cf 100644 --- a/automation-template.yml +++ b/automation-template.yml @@ -22,8 +22,10 @@ on: # zizmor: ignore[dangerous-triggers] - created pull_request_target: types: + - edited - labeled - opened + - reopened - review_requested schedule: - cron: 1 0 * * 1 diff --git a/scripts/pull-request-template.js b/scripts/pull-request-template.js new file mode 100644 index 0000000..da27801 --- /dev/null +++ b/scripts/pull-request-template.js @@ -0,0 +1,146 @@ +/** + * Fails the run when a pull request body does not carry the sections that its + * template marks as required. + */ + +const fs = require('fs'); +const path = require('path'); + +const { isBot } = require('./utils'); + +const ORG_TEMPLATE_PATH = path.join(__dirname, '..', '.github', 'PULL_REQUEST_TEMPLATE.md'); + +// The contents API is case sensitive, so both spellings are tried in each of the +// three places GitHub reads a template from. +const REPO_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', + 'docs/PULL_REQUEST_TEMPLATE.md', +]; + +const HEADING = /^## [^#]/; +const REQUIRED_MARKER = /^\s*\s*$/; +const HTML_COMMENT = //g; +const PLACEHOLDER = '…'; + +/** + * Splits markdown on level-two headings, so a deeper heading stays in its section. + */ +function sections(markdown) { + const out = []; + for (const line of (markdown || '').split('\n')) { + if (HEADING.test(line)) out.push({ heading: line.trim(), lines: [] }); + else if (out.length) out[out.length - 1].lines.push(line); + } + return out; +} + +function isMarked(section) { + const first = section.lines.find(line => line.trim() !== ''); + return first !== undefined && REQUIRED_MARKER.test(first); +} + +/** + * The headings a body must carry. A template that marks none requires them all, + * so a repo whose template has no markers is strict rather than unchecked. + */ +function requiredHeadings(template) { + const all = sections(template); + const marked = all.filter(isMarked); + return (marked.length ? marked : all).map(section => section.heading); +} + +function content(section) { + return section.lines.join('\n').replace(HTML_COMMENT, '').trim(); +} + +/** + * One message per required section the body does not satisfy. + */ +function failures(body, required) { + const found = new Map(sections(body).map(section => [section.heading.toLowerCase(), section])); + const problems = []; + for (const heading of required) { + const section = found.get(heading.toLowerCase()); + if (!section) { + problems.push(`\`${heading}\` is missing.`); + continue; + } + const text = content(section); + if (text === '') { + problems.push(`\`${heading}\` is empty.`); + } else if (text.includes(PLACEHOLDER)) { + problems.push(`\`${heading}\` still holds the \`${PLACEHOLDER}\` placeholder.`); + } + } + return problems; +} + +async function readRepoTemplate({ github, context, core }, ref) { + for (const templatePath of REPO_TEMPLATE_PATHS) { + try { + const response = await github.rest.repos.getContent({ + owner: context.repo.owner, + repo: context.repo.repo, + path: templatePath, + ref, + }); + if (response.data?.content) { + core.info(`Checking against ${templatePath} on ${ref}.`); + return Buffer.from(response.data.content, 'base64').toString('utf8'); + } + } catch (error) { + if (error.status !== 404) { + core.warning(`Could not read ${templatePath}: ${error.message}`); + } + } + } + return null; +} + +module.exports = async ({ github, context, core }) => { + try { + const pullRequest = context.payload.pull_request; + const author = pullRequest.user.login; + + if (await isBot(author, { core })) { + core.info(`${author} is a bot, so the template is not checked.`); + return; + } + + const repoTemplate = await readRepoTemplate( + { github, context, core }, + pullRequest.base.ref, + ); + const template = repoTemplate ?? fs.readFileSync(ORG_TEMPLATE_PATH, 'utf8'); + if (!repoTemplate) { + core.info('No template in this repo, so the shared template applies.'); + } + + const required = requiredHeadings(template); + if (!required.length) { + core.warning('The template holds no sections, so there is nothing to check.'); + return; + } + + const problems = failures(pullRequest.body, required); + if (!problems.length) { + core.info(`The body carries all ${required.length} required sections.`); + return; + } + + for (const problem of problems) { + core.error(problem); + } + core.setFailed( + `The description does not follow the pull request template:\n` + + problems.map(problem => `- ${problem}`).join('\n') + + `\n\nRequired: ${required.join(', ')}. Edit the description and the check runs again.`, + ); + } catch (error) { + core.setFailed(`Action failed with error: ${error.message}`); + } +}; diff --git a/scripts/pull-request-template.test.js b/scripts/pull-request-template.test.js new file mode 100644 index 0000000..6957939 --- /dev/null +++ b/scripts/pull-request-template.test.js @@ -0,0 +1,244 @@ +const test = require('node:test'); +const assert = require('node:assert/strict'); + +const pullRequestTemplate = require('./pull-request-template'); +const { LE_BOT_USERNAME } = require('./constants'); + +const MARKED = [ + '## Summary', + '', + '', + '', + '…', + '', + '## Notes', + '', + 'anything', + '', +].join('\n'); + +const UNMARKED = ['## Summary', '', '…', '', '## Notes', '', 'anything', ''].join('\n'); + +const GOOD_BODY = ['## Summary', '', 'What this does.', '', '## Notes', '', 'A note.'].join('\n'); + +// Satisfies the shared template, for the cases that fall back to it. +const SHARED_BODY = [ + '## Summary', + '', + 'What this does.', + '', + '## References', + '', + 'Closes #1.', + '', + '## Reviewer guidance', + '', + 'Read the diff.', + '', + '## AI usage', + '', + 'None.', +].join('\n'); + +function fakeGithub({ templates = {} } = {}) { + const reads = []; + return { + reads, + rest: { + repos: { + getContent: async ({ path, ref }) => { + reads.push({ path, ref }); + if (!(path in templates)) { + const error = new Error('Not Found'); + error.status = 404; + throw error; + } + return { data: { content: Buffer.from(templates[path], 'utf8').toString('base64') } }; + }, + }, + }, + }; +} + +function fakeContext({ body = GOOD_BODY, author = 'contributor', base = 'develop' } = {}) { + return { + repo: { owner: 'learningequality', repo: 'kolibri' }, + payload: { + pull_request: { number: 42, body, user: { login: author }, base: { ref: base } }, + }, + }; +} + +function fakeCore() { + const failures = []; + const errors = []; + const warnings = []; + return { + failures, + errors, + warnings, + info() {}, + warning(message) { + warnings.push(message); + }, + error(message) { + errors.push(message); + }, + setFailed(message) { + failures.push(message); + }, + }; +} + +const repoTemplate = body => ({ '.github/pull_request_template.md': body }); + +test('a body carrying every marked section passes', async () => { + const core = fakeCore(); + await pullRequestTemplate({ + github: fakeGithub({ templates: repoTemplate(MARKED) }), + context: fakeContext(), + core, + }); + + assert.deepEqual(core.failures, []); +}); + +test('a missing marked section fails and is named', async () => { + const core = fakeCore(); + await pullRequestTemplate({ + github: fakeGithub({ templates: repoTemplate(MARKED) }), + context: fakeContext({ body: '## Notes\n\nA note.' }), + core, + }); + + assert.equal(core.failures.length, 1); + assert.match(core.failures[0], /`## Summary` is missing/); +}); + +test('an empty marked section fails', async () => { + const core = fakeCore(); + await pullRequestTemplate({ + github: fakeGithub({ templates: repoTemplate(MARKED) }), + context: fakeContext({ body: '## Summary\n\n\n' }), + core, + }); + + assert.match(core.failures[0], /`## Summary` is empty/); +}); + +test('a marked section still holding the placeholder fails', async () => { + const core = fakeCore(); + await pullRequestTemplate({ + github: fakeGithub({ templates: repoTemplate(MARKED) }), + context: fakeContext({ body: '## Summary\n\n…\n' }), + core, + }); + + assert.match(core.failures[0], /placeholder/); +}); + +test('an unmarked section is not required', async () => { + const core = fakeCore(); + await pullRequestTemplate({ + github: fakeGithub({ templates: repoTemplate(MARKED) }), + context: fakeContext({ body: '## Summary\n\nWhat this does.' }), + core, + }); + + assert.deepEqual(core.failures, []); +}); + +test('a template marking nothing requires every section', async () => { + const core = fakeCore(); + await pullRequestTemplate({ + github: fakeGithub({ templates: repoTemplate(UNMARKED) }), + context: fakeContext({ body: '## Summary\n\nWhat this does.' }), + core, + }); + + assert.equal(core.failures.length, 1); + assert.match(core.failures[0], /`## Notes` is missing/); +}); + +test('every failure is named, not only the first', async () => { + const core = fakeCore(); + await pullRequestTemplate({ + github: fakeGithub({ templates: repoTemplate(UNMARKED) }), + context: fakeContext({ body: '' }), + core, + }); + + assert.equal(core.errors.length, 2); + assert.match(core.failures[0], /## Summary/); + assert.match(core.failures[0], /## Notes/); +}); + +test('an empty body fails rather than throwing', async () => { + const core = fakeCore(); + await pullRequestTemplate({ + github: fakeGithub({ templates: repoTemplate(MARKED) }), + context: fakeContext({ body: null }), + core, + }); + + assert.equal(core.failures.length, 1); +}); + +test('a bot author passes without a template read', async () => { + const core = fakeCore(); + const github = fakeGithub({ templates: repoTemplate(MARKED) }); + await pullRequestTemplate({ + github, + context: fakeContext({ body: '', author: LE_BOT_USERNAME }), + core, + }); + + assert.deepEqual(core.failures, []); + assert.deepEqual(github.reads, []); +}); + +test('the template is read from the base branch, never the head', async () => { + const github = fakeGithub({ templates: repoTemplate(MARKED) }); + await pullRequestTemplate({ github, context: fakeContext({ base: 'release-v1' }), core: fakeCore() }); + + assert.ok(github.reads.length); + for (const read of github.reads) { + assert.equal(read.ref, 'release-v1'); + } +}); + +test('an uppercase template in the repo is found', async () => { + const core = fakeCore(); + const github = fakeGithub({ templates: { '.github/PULL_REQUEST_TEMPLATE.md': MARKED } }); + await pullRequestTemplate({ github, context: fakeContext({ body: '' }), core }); + + assert.equal(core.failures.length, 1); + assert.match(core.failures[0], /## Summary/); +}); + +test('a repo holding no template falls back to the shared one', async () => { + const core = fakeCore(); + await pullRequestTemplate({ + github: fakeGithub(), + context: fakeContext({ body: '## Summary\n\nWhat this does.' }), + core, + }); + + // The shared template asks for References, Reviewer guidance and AI usage too. + assert.equal(core.failures.length, 1); + assert.match(core.failures[0], /## AI usage/); +}); + +test('a read failure other than 404 is reported and does not fail the run', async () => { + const core = fakeCore(); + const github = fakeGithub(); + github.rest.repos.getContent = async () => { + const error = new Error('server error'); + error.status = 500; + throw error; + }; + await pullRequestTemplate({ github, context: fakeContext({ body: SHARED_BODY }), core }); + + assert.equal(core.warnings.length, 6); + assert.deepEqual(core.failures, []); +}); From bd86259893a9abf6484163f90bbebb11410f2520 Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Wed, 21 Oct 2026 17:02:20 +0300 Subject: [PATCH 02/12] Carry the pull request template to consumers GitHub applies an org default template in the web form only. A pull request opened with gh pr create, the API, or a coding agent reads the working copy instead, and the org file is not in it, so a contributor in a kolibri checkout has no template to follow. The sync now carries a list of files rather than one. A consumer is in sync when every file it takes matches, one pull request carries every file that drifted, and the declined and toolchain-conflict guards hold only when every drifted file agrees. A consumer keeping a template of its own keeps it. That is checked in all six places GitHub reads a template from, and a path that cannot be read counts as a template of its own, so a failed read never overwrites a file the run could not see. morango keeps its own template at the path the sync writes to, so existence alone cannot tell a copy of ours from a template of theirs. The shared template opens with a marker comment naming this repo, and at that one path a file without the marker is left alone. --- .github/PULL_REQUEST_TEMPLATE.md | 1 + .../workflows/sync-automation-template.yml | 1 + docs/automation.md | 25 ++- docs/community-automations.md | 23 +++ scripts/sync-automation-template.js | 134 ++++++++++++---- scripts/sync-automation-template.test.js | 149 +++++++++++++++--- 6 files changed, 277 insertions(+), 56 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index af88746..496a94c 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -1,3 +1,4 @@ + ` comment in the template: + +```markdown +## Summary + +``` + +A template that marks no section requires every level-two heading, so a repo is strict rather than silently unchecked. To change what a repo requires, edit the markers in that repo's template. No change here is needed. + +Three states fail, and the log names every one of them rather than stopping at the first: + +- the heading is missing from the description +- the section holds no text, once HTML comments are stripped +- the section still holds the `…` placeholder + +The failed check is the only report. Nothing is posted on the pull request. + # `holiday-message` Sends a holiday message to community pull requests and issue comments. diff --git a/scripts/sync-automation-template.js b/scripts/sync-automation-template.js index 826ff67..198903b 100644 --- a/scripts/sync-automation-template.js +++ b/scripts/sync-automation-template.js @@ -1,7 +1,9 @@ #!/usr/bin/env node /** - * Opens one pull request per consumer repo whose copy of automation.yml has - * drifted from automation-template.yml. + * Opens one pull request per consumer repo whose copy of a shared file has + * drifted from this repo's. The shared files are automation-template.yml, which + * every consumer takes, and the pull request template, which only a consumer + * holding none of its own takes. * * Usage: * node scripts/sync-automation-template.js open or update pull requests @@ -15,7 +17,6 @@ const fs = require('fs'); const path = require('path'); const ROOT = path.join(__dirname, '..'); -const TEMPLATE_PATH = path.join(ROOT, 'automation-template.yml'); // SYNC_ORG points the whole run at another org, so the write path can be // exercised somewhere other than production. @@ -114,7 +115,32 @@ const KDS_TEMPLATE = { }, }; -const PR_TEMPLATE_PATHS = ['.github/pull_request_template.md', '.github/PULL_REQUEST_TEMPLATE.md']; +// The contents API is case sensitive, so both spellings are tried in each of the +// three places GitHub reads a pull request template from. +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', + 'docs/PULL_REQUEST_TEMPLATE.md', +]; + +const PR_TEMPLATE_TARGET = '.github/PULL_REQUEST_TEMPLATE.md'; + +// morango's own template sits at PR_TEMPLATE_TARGET, the path the sync writes to, +// so existence alone cannot tell a copy of ours from a template of their own. +const PR_TEMPLATE_MARKER = 'shared pull request template'; + +/** + * The files carried to a consumer. `mustExist` says the consumer already holds + * one, so a missing file is a fault rather than work to do. `ownStops` says a + * consumer that holds its own copy keeps it. + */ +const SYNCED_FILES = [ + { target: TARGET_PATH, source: 'automation-template.yml', mustExist: true }, + { target: PR_TEMPLATE_TARGET, source: PR_TEMPLATE_TARGET, ownStops: true }, +]; async function readPrTemplate(api, repo, ref) { if (repo !== KDS_REPO) return null; @@ -125,13 +151,34 @@ async function readPrTemplate(api, repo, ref) { return null; } -async function readCopy(api, repo, ref) { - const r = await api('GET', `/repos/${ORG}/${repo}/contents/${TARGET_PATH}?ref=${ref}`); +/** + * Whether the consumer keeps a pull request template of its own, in any of the + * places GitHub reads one from. An unreadable path counts as one, so a failed + * read never overwrites a template this run could not see. At the path the sync + * writes to, a copy of ours carries PR_TEMPLATE_MARKER and their own does not. + */ +async function keepsOwnPrTemplate(api, repo, ref) { + for (const p of PR_TEMPLATE_PATHS) { + const r = await api('GET', `/repos/${ORG}/${repo}/contents/${p}?ref=${ref}`); + if (r.status === 404) continue; + if (!r.ok) return true; + if (p !== PR_TEMPLATE_TARGET) return true; + if (!Buffer.from(r.data.content, 'base64').toString('utf8').includes(PR_TEMPLATE_MARKER)) return true; + } + return false; +} + +async function readFile(api, repo, filePath, ref) { + const r = await api('GET', `/repos/${ORG}/${repo}/contents/${filePath}?ref=${ref}`); if (r.ok) return { sha: r.data.sha, content: Buffer.from(r.data.content, 'base64').toString('utf8') }; if (r.status === 404) return { missing: true }; return { error: `read failed (${detail(r)})` }; } +async function readCopy(api, repo, ref) { + return readFile(api, repo, TARGET_PATH, ref); +} + /** * 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 @@ -219,20 +266,39 @@ function classifyDrift(closedPr, proposed, template) { return closedPr.merged_at ? 'toolchain-conflict' : 'declined'; } +/** + * The files this consumer takes, and the content each one must end up holding. + */ +async function applicableFiles(api, repo, base, sources) { + const files = []; + for (const file of SYNCED_FILES) { + if (file.ownStops && (await keepsOwnPrTemplate(api, repo, base))) continue; + files.push({ ...file, content: sources[file.target] }); + } + return files; +} + // 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 }) { +async function syncRepo(api, consumer, sources, { dryRun }) { const { repo, base } = consumer; 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: 'error', detail: 'the copy disappeared during the run' }; - if (copy.content === template) return { repo, state: 'in-sync' }; + const files = await applicableFiles(api, repo, base, sources); + const pending = []; + for (const file of files) { + const copy = await readFile(api, repo, file.target, base); + if (copy.error) return { repo, state: 'error', detail: copy.error }; + if (copy.missing && file.mustExist) { + return { repo, state: 'error', detail: 'the copy disappeared during the run' }; + } + if (copy.missing || copy.content !== file.content) pending.push(file); + } + if (!pending.length) return { repo, state: 'in-sync' }; const open = await openSyncPr(api, repo); if (open.error) return { repo, state: 'error', detail: open.error }; @@ -242,9 +308,16 @@ async function syncRepo(api, consumer, template, { dryRun }) { if (closed.error) return { repo, state: 'error', detail: closed.error }; 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 (!ref) return { repo, state: 'error', detail: 'no reference on the closed pull request' }; + const verdicts = []; + for (const file of pending) { + const proposed = await readFile(api, repo, file.target, ref); + if (proposed.error) return { repo, state: 'error', detail: proposed.error }; + verdicts.push(classifyDrift(closed.pr, proposed.content, file.content)); + } + // Every pending file has to agree. One file the template has moved on for + // is work to do, whatever the others say. + const verdict = verdicts.every(v => v === verdicts[0]) ? verdicts[0] : 'drift'; if (verdict !== 'drift') return { repo, state: verdict, pr: closed.pr.number }; } } @@ -269,16 +342,18 @@ async function syncRepo(api, consumer, template, { dryRun }) { } } - const onBranch = await readCopy(api, repo, BRANCH); - if (onBranch.error) return { repo, state: 'error', detail: onBranch.error }; + for (const file of pending) { + const onBranch = await readFile(api, repo, file.target, 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 (${detail(put)})` }; + const put = await api('PUT', `/repos/${ORG}/${repo}/contents/${file.target}`, { + message: TITLE, + content: Buffer.from(file.content, 'utf8').toString('base64'), + branch: BRANCH, + ...(onBranch.sha ? { sha: onBranch.sha } : {}), + }); + if (!put.ok) return { repo, state: 'error', detail: `write failed (${detail(put)})` }; + } if (open.pr) return { repo, state: 'updated', pr: open.pr.number, url: open.pr.html_url }; @@ -292,13 +367,13 @@ async function syncRepo(api, consumer, template, { dryRun }) { return { repo, state: 'opened', pr: pr.data.number, url: pr.data.html_url }; } -async function run(api, template, options) { +async function run(api, sources, options) { const consumers = await findConsumers(api, options.only); const results = []; for (const consumer of consumers) { try { - results.push(await syncRepo(api, consumer, template, options)); + results.push(await syncRepo(api, consumer, sources, options)); } catch (err) { results.push({ repo: consumer.repo, state: 'error', detail: err.message }); } @@ -338,8 +413,11 @@ async function main() { console.error('GITHUB_TOKEN is not set.'); process.exit(1); } - const template = fs.readFileSync(TEMPLATE_PATH, 'utf8'); - const results = await run(httpApi(token), template, parseArgs(process.argv.slice(2))); + const sources = {}; + for (const file of SYNCED_FILES) { + sources[file.target] = fs.readFileSync(path.join(ROOT, file.source), 'utf8'); + } + const results = await run(httpApi(token), sources, parseArgs(process.argv.slice(2))); process.exit(report(results) ? 1 : 0); } @@ -360,4 +438,6 @@ module.exports = { PROBLEM_STATES, BRANCH, TARGET_PATH, + PR_TEMPLATE_TARGET, + SYNCED_FILES, }; diff --git a/scripts/sync-automation-template.test.js b/scripts/sync-automation-template.test.js index c92f413..1d2bc16 100644 --- a/scripts/sync-automation-template.test.js +++ b/scripts/sync-automation-template.test.js @@ -9,6 +9,8 @@ const { report, PROBLEM_STATES, BRANCH, + PR_TEMPLATE_TARGET, + TARGET_PATH, } = require('./sync-automation-template'); const USES = 'jobs:\n automation:\n uses: learningequality/.github/.github/workflows/automation.yml@main\n'; @@ -39,6 +41,19 @@ const PR_TEMPLATE = [ '2. Step 2', '', ].join('\n'); +// The shared pull request template, which a consumer keeping none of its own takes. +// The first line is the marker that tells a copy of ours from a template of theirs. +const PR_SHARED = [ + '', + '## Summary', + '', + '', + '…', + '', +].join('\n'); + +const SOURCES = { [TARGET_PATH]: TEMPLATE, [PR_TEMPLATE_TARGET]: PR_SHARED }; + // Not "main": a default branch that differs is the only way to catch a hardcoded base. const BASE = 'develop'; const encode = (s) => Buffer.from(s, 'utf8').toString('base64'); @@ -70,6 +85,7 @@ 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 contents/.github/PULL_REQUEST_TEMPLATE.md', ok({ sha: 'pr-sha', content: encode(PR_SHARED) })], ['GET /repos/learningequality/demo/pulls?state=open', ok([])], ['GET /repos/learningequality/demo/pulls?state=closed', ok([])], [`GET /repos/learningequality/demo/git/ref/heads/${BASE}`, ok({ object: { sha: 'base-sha' } })], @@ -78,7 +94,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 = {}) => (await run(makeApi(routes), TEMPLATE, options))[0]; +const only = async (routes, options = {}) => (await run(makeApi(routes), SOURCES, options))[0]; test('a matching copy is in sync', async () => { assert.equal((await only(baseRoutes(TEMPLATE))).state, 'in-sync'); @@ -92,7 +108,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), TEMPLATE, {}); + await run(makeApi(baseRoutes(STALE), calls), SOURCES, {}); 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); @@ -100,7 +116,7 @@ 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), TEMPLATE, {}); + await run(makeApi(baseRoutes(STALE), calls), SOURCES, {}); const pr = calls.find((c) => c.method === 'POST' && c.url.endsWith('/pulls')); assert.equal(pr.body.base, BASE); assert.equal(pr.body.head, BRANCH); @@ -124,7 +140,7 @@ const bodyOf = (calls) => calls.find((c) => c.method === 'POST' && c.url.endsWit test('a repo that checks the description gets its own template, with every field answered', async () => { const calls = []; - await run(makeApi(checkedRoutes(STALE), calls), TEMPLATE, {}); + await run(makeApi(checkedRoutes(STALE), calls), SOURCES, {}); const body = bodyOf(calls); assert.ok(body.startsWith('## Description'), 'the repo template decides the shape'); assert.ok(!body.includes('Summary of change(s)'), 'the placeholder fails check-description'); @@ -141,7 +157,7 @@ test('a renamed description heading still leaves the change explained', async () ...checkedRoutes(STALE), ['GET contents/.github/pull_request_template.md', ok({ content: encode(renamed) })], ]; - await run(makeApi(routes, calls), TEMPLATE, {}); + await run(makeApi(routes, calls), SOURCES, {}); 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'); @@ -153,7 +169,7 @@ test('a template with none of the kept headings still explains itself', async () ...checkedRoutes(STALE), ['GET contents/.github/pull_request_template.md', ok({ content: encode('## Notes\n\nnothing here\n') })], ]; - await run(makeApi(routes, calls), TEMPLATE, {}); + await run(makeApi(routes, calls), SOURCES, {}); 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'); @@ -161,7 +177,7 @@ test('a template with none of the kept headings still explains itself', async () test('sections that would arrive unfilled are dropped', async () => { const calls = []; - await run(makeApi(checkedRoutes(STALE), calls), TEMPLATE, {}); + await run(makeApi(checkedRoutes(STALE), calls), SOURCES, {}); 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')); @@ -175,27 +191,110 @@ test('any other repo gets the plain explanation, template or not', async () => { ...baseRoutes(STALE), ['GET contents/.github/pull_request_template.md', ok({ content: encode(PR_TEMPLATE) })], ]; - await run(makeApi(routes, calls), TEMPLATE, {}); + await run(makeApi(routes, calls), SOURCES, {}); const body = bodyOf(calls); - assert.ok(!body.includes('## Description'), 'a template is not fetched for a repo that does not need it'); + assert.ok(!body.includes('## Description'), 'only the repo that checks the description gets its template read'); assert.ok(body.includes('automation-template.yml')); }); test('a repo with no pull request template gets the plain explanation', async () => { const calls = []; - await run(makeApi(baseRoutes(STALE), calls), TEMPLATE, {}); + await run(makeApi(baseRoutes(STALE), calls), SOURCES, {}); const { body } = calls.find((c) => c.method === 'POST' && c.url.endsWith('/pulls')).body; assert.ok(!body.includes('## ')); assert.ok(body.includes('automation-template.yml')); }); +const putsTo = (calls, target) => calls.filter((c) => c.method === 'PUT' && c.url.includes(target)); + +// The lowercase spelling, which is a template of the repo's own rather than ours. +const ownTemplate = (content = PR_TEMPLATE) => [ + 'GET contents/.github/pull_request_template.md', + ok({ content: encode(content) }), +]; + +const noSharedTemplate = () => ['GET contents/.github/PULL_REQUEST_TEMPLATE.md', fail(404, 'Not Found')]; + +test('a consumer holding no template of its own takes the shared one', async () => { + const calls = []; + const routes = [...baseRoutes(TEMPLATE), noSharedTemplate()]; + const results = await run(makeApi(routes, calls), SOURCES, {}); + + assert.equal(results[0].state, 'opened'); + const put = putsTo(calls, PR_TEMPLATE_TARGET); + assert.equal(put.length, 1); + assert.equal(Buffer.from(put[0].body.content, 'base64').toString('utf8'), PR_SHARED); + assert.equal(putsTo(calls, TARGET_PATH).length, 0, 'automation.yml already matches'); +}); + +test('a consumer holding its own template keeps it', async () => { + const calls = []; + const routes = [...baseRoutes(TEMPLATE), ownTemplate(), noSharedTemplate()]; + const results = await run(makeApi(routes, calls), SOURCES, {}); + + assert.equal(results[0].state, 'in-sync'); + assert.equal(putsTo(calls, PR_TEMPLATE_TARGET).length, 0); +}); + +test('a candidate path that cannot be read counts as a template of its own', async () => { + const calls = []; + const routes = [ + ...baseRoutes(TEMPLATE), + ['GET contents/docs/pull_request_template.md', fail(500, 'Server Error')], + noSharedTemplate(), + ]; + const results = await run(makeApi(routes, calls), SOURCES, {}); + + assert.equal(results[0].state, 'in-sync', 'a failed read must never overwrite a template it cannot see'); + assert.equal(putsTo(calls, PR_TEMPLATE_TARGET).length, 0); +}); + +test('the shared template is replaced when a copy of ours has drifted', async () => { + const calls = []; + const stale = PR_SHARED.replace('## Summary', '## Overview'); + const routes = [ + ...baseRoutes(TEMPLATE), + ['GET contents/.github/PULL_REQUEST_TEMPLATE.md', ok({ sha: 'pr-sha', content: encode(stale) })], + ]; + const results = await run(makeApi(routes, calls), SOURCES, {}); + + assert.equal(results[0].state, 'opened'); + const put = putsTo(calls, PR_TEMPLATE_TARGET); + assert.equal(put.length, 1); + assert.equal(put[0].body.sha, 'pr-sha', 'an update without the sha fails with a 422'); +}); + +test('a template of their own at our path is left alone', async () => { + const calls = []; + const routes = [ + ...baseRoutes(TEMPLATE), + // morango's shape: its own template sits at the path the sync writes to. + ['GET contents/.github/PULL_REQUEST_TEMPLATE.md', ok({ sha: 'pr-sha', content: encode('## Summary\n') })], + ]; + const results = await run(makeApi(routes, calls), SOURCES, {}); + + assert.equal(results[0].state, 'in-sync'); + assert.equal(putsTo(calls, PR_TEMPLATE_TARGET).length, 0, 'their template must never be overwritten'); +}); + +test('both files drifted are written under one pull request', async () => { + const calls = []; + const routes = [...baseRoutes(STALE), noSharedTemplate()]; + const results = await run(makeApi(routes, calls), SOURCES, {}); + + assert.equal(results[0].state, 'opened'); + assert.equal(putsTo(calls, TARGET_PATH).length, 1); + assert.equal(putsTo(calls, PR_TEMPLATE_TARGET).length, 1); + assert.equal(calls.filter((c) => c.method === 'POST' && c.url.endsWith('/pulls')).length, 1); +}); + test('an open sync pull request is updated in place, keeping the file sha', 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), TEMPLATE, {}); + const results = await run(makeApi(routes, calls), SOURCES, {}); 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'); @@ -204,7 +303,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)), TEMPLATE, {}); + const results = await run(makeApi(baseRoutes(TEMPLATE, repos)), SOURCES, {}); assert.deepEqual( results.map((r) => r.repo), ['demo'] @@ -213,7 +312,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), TEMPLATE, {}); + const results = await run(makeApi(routes), SOURCES, {}); assert.deepEqual(results, []); }); @@ -222,7 +321,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), TEMPLATE, {}); + const results = await run(makeApi(routes), SOURCES, {}); assert.deepEqual(results, []); }); @@ -235,7 +334,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), TEMPLATE, {}), /could not list the org's repos/); + await assert.rejects(() => run(makeApi(routes), SOURCES, {}), /could not list the org's repos/); }); test('a failed pull request listing is an error, not a missing pull request', async () => { @@ -277,7 +376,7 @@ test('a thrown request during discovery is contained, not fatal', async () => { test('dry run reports without writing', async () => { const calls = []; - const results = await run(makeApi(baseRoutes(STALE), calls), TEMPLATE, { dryRun: true }); + const results = await run(makeApi(baseRoutes(STALE), calls), SOURCES, { dryRun: true }); assert.equal(results[0].state, 'would-open'); assert.equal( calls.filter((c) => c.method !== 'GET').length, @@ -319,7 +418,7 @@ const atRef = (sha, content) => [ test('a repo that reverted a merged sync is reported, and no pull request is opened', async () => { const calls = []; const routes = [...baseRoutes(STALE), mergedSync('m1'), atRef('m1', TEMPLATE)]; - const results = await run(makeApi(routes, calls), TEMPLATE, {}); + const results = await run(makeApi(routes, calls), SOURCES, {}); 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); }); @@ -329,7 +428,7 @@ test('a template published after the merge is drift, whatever the commit dates s // 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, {}); + const results = await run(makeApi(routes, calls), SOURCES, {}); assert.equal(results[0].state, 'opened', 'the merge left something older, so the template moved on'); }); @@ -340,7 +439,7 @@ test('a closed unmerged pull request is read at its head', async () => { ['GET /repos/learningequality/demo/pulls?state=closed', ok([{ number: 9, merged_at: null, head: { sha: 'h1' } }])], atRef('h1', TEMPLATE), ]; - const results = await run(makeApi(routes, calls), TEMPLATE, {}); + const results = await run(makeApi(routes, calls), SOURCES, {}); assert.equal(results[0].state, 'declined'); assert.ok( calls.some((c) => c.url.includes('ref=h1')), @@ -376,7 +475,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), TEMPLATE, {}); + await run(makeApi(routes, calls), SOURCES, {}); 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'); @@ -403,7 +502,7 @@ test('a named repo that is not a consumer says why, rather than reporting none', ]; for (const [r, expected] of cases) { const routes = [...baseRoutes(TEMPLATE), ['GET =/repos/learningequality/demo', ok(r)]]; - await assert.rejects(() => run(makeApi(routes), TEMPLATE, { only: 'demo' }), expected); + await assert.rejects(() => run(makeApi(routes), SOURCES, { only: 'demo' }), expected); } }); @@ -413,14 +512,14 @@ test('a named repo without the marker says why', async () => { ['GET =/repos/learningequality/demo', ok(repo())], ['GET contents/.github/workflows/automation.yml', ok({ sha: 'x', content: encode('name: other\n') })], ]; - await assert.rejects(() => run(makeApi(routes), TEMPLATE, { only: 'demo' }), /does not call the shared workflow/); + await assert.rejects(() => run(makeApi(routes), SOURCES, { only: 'demo' }), /does not call the shared workflow/); }); test('only limits the run to that repo, and never lists the org', async () => { const calls = []; const others = [repo({ name: 'demo' }), repo({ name: 'production-repo' })]; const routes = [...baseRoutes(TEMPLATE, others), ['GET =/repos/learningequality/demo', ok(repo())]]; - const results = await run(makeApi(routes, calls), TEMPLATE, { only: 'demo' }); + const results = await run(makeApi(routes, calls), SOURCES, { only: 'demo' }); assert.deepEqual( results.map((r) => r.repo), ['demo'] @@ -433,7 +532,7 @@ test('only limits the run to that repo, and never lists the org', async () => { test('without only, every repo in the listing is considered', async () => { const others = [repo({ name: 'demo' }), repo({ name: 'production-repo' })]; - const results = await run(makeApi(baseRoutes(TEMPLATE, others)), TEMPLATE, {}); + const results = await run(makeApi(baseRoutes(TEMPLATE, others)), SOURCES, {}); assert.deepEqual( results.map((r) => r.repo), ['demo', 'production-repo'] @@ -442,7 +541,7 @@ test('without only, every repo in the listing is considered', async () => { test('only reports an error when the repo cannot be read', async () => { const routes = [...baseRoutes(TEMPLATE), ['GET =/repos/learningequality/missing', fail(404, 'Not Found')]]; - await assert.rejects(() => run(makeApi(routes), TEMPLATE, { only: 'missing' }), /could not read missing/); + await assert.rejects(() => run(makeApi(routes), SOURCES, { only: 'missing' }), /could not read missing/); }); test('findConsumers pages through the org listing', async () => { From 784d984505a20a524389be600c32e44296399a5b Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Wed, 21 Oct 2026 18:32:40 +0300 Subject: [PATCH 03/12] Fix the review findings on the template check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An ellipsis anywhere in a section failed it, so a summary reading "Adds a Loading… spinner" was rejected. Only a line that is the placeholder on its own counts now. A 403 or 5xx reading a repo's own template fell back to the shared one, which would fail a body that follows the template its author was given. The check is skipped instead, matching what the sync does with a path it cannot read. One verdict for every pending file misjudged two cases: a consumer who declined the template was offered it again whenever automation.yml drifted, and a consumer who merged it and then deleted it became a permanent toolchain-conflict. Each file carries its own verdict now, only drifted files are proposed, and the most severe of the rest is reported. A consumer whose only drifted file was the template got a pull request titled "Refresh automation.yml from the shared template", describing a file it did not contain. The title and body are built from the files in the pull request, and the result carries them so the dry run says which ones it would write. The path list lived in both scripts and could drift, so it moved to constants. Three tests were weak or missing. The uppercase-path test passed with that path removed. Nothing checked that the real template carries the sync marker, whose loss would silently stop the sync refreshing copies it placed. Nothing covered verdicts across several pending files. --- scripts/constants.js | 19 ++++ scripts/pull-request-template.js | 58 +++++------ scripts/pull-request-template.test.js | 27 +++++- scripts/sync-automation-template.js | 117 +++++++++++++---------- scripts/sync-automation-template.test.js | 74 +++++++++++++- 5 files changed, 207 insertions(+), 88 deletions(-) diff --git a/scripts/constants.js b/scripts/constants.js index 41479a4..8becc3e 100644 --- a/scripts/constants.js +++ b/scripts/constants.js @@ -167,6 +167,22 @@ const PR_STATS_REPOS = [ // Repositories in which we accept open-source contributions const COMMUNITY_REPOS = [...PR_STATS_REPOS]; +// Every place GitHub reads a pull request template from. The contents API is +// case sensitive, so both spellings are needed. +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', + 'docs/PULL_REQUEST_TEMPLATE.md', +]; + +// The path the template sync writes to, and the marker that tells a copy it +// placed from a template the consumer wrote itself. +const PR_TEMPLATE_TARGET = '.github/PULL_REQUEST_TEMPLATE.md'; +const PR_TEMPLATE_MARKER = 'shared pull request template'; + module.exports = { LE_BOT_USERNAME, BOT_USERNAMES, @@ -195,4 +211,7 @@ module.exports = { HOLIDAY_MESSAGE, PR_STATS_REPOS, COMMUNITY_REPOS, + PR_TEMPLATE_PATHS, + PR_TEMPLATE_TARGET, + PR_TEMPLATE_MARKER, }; diff --git a/scripts/pull-request-template.js b/scripts/pull-request-template.js index da27801..005020d 100644 --- a/scripts/pull-request-template.js +++ b/scripts/pull-request-template.js @@ -7,28 +7,15 @@ const fs = require('fs'); const path = require('path'); const { isBot } = require('./utils'); +const { PR_TEMPLATE_PATHS, PR_TEMPLATE_TARGET } = require('./constants'); -const ORG_TEMPLATE_PATH = path.join(__dirname, '..', '.github', 'PULL_REQUEST_TEMPLATE.md'); - -// The contents API is case sensitive, so both spellings are tried in each of the -// three places GitHub reads a template from. -const REPO_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', - 'docs/PULL_REQUEST_TEMPLATE.md', -]; +const ORG_TEMPLATE_PATH = path.join(__dirname, '..', PR_TEMPLATE_TARGET); const HEADING = /^## [^#]/; const REQUIRED_MARKER = /^\s*\s*$/; const HTML_COMMENT = //g; const PLACEHOLDER = '…'; -/** - * Splits markdown on level-two headings, so a deeper heading stays in its section. - */ function sections(markdown) { const out = []; for (const line of (markdown || '').split('\n')) { @@ -43,10 +30,8 @@ function isMarked(section) { return first !== undefined && REQUIRED_MARKER.test(first); } -/** - * The headings a body must carry. A template that marks none requires them all, - * so a repo whose template has no markers is strict rather than unchecked. - */ +// A template that marks nothing requires every section, so a repo whose template +// has no markers is strict rather than unchecked. function requiredHeadings(template) { const all = sections(template); const marked = all.filter(isMarked); @@ -57,9 +42,6 @@ function content(section) { return section.lines.join('\n').replace(HTML_COMMENT, '').trim(); } -/** - * One message per required section the body does not satisfy. - */ function failures(body, required) { const found = new Map(sections(body).map(section => [section.heading.toLowerCase(), section])); const problems = []; @@ -72,15 +54,23 @@ function failures(body, required) { const text = content(section); if (text === '') { problems.push(`\`${heading}\` is empty.`); - } else if (text.includes(PLACEHOLDER)) { + } else if (text.split('\n').some(line => line.trim() === PLACEHOLDER)) { + // The template puts the placeholder on a line of its own, so an ellipsis + // inside a sentence is prose rather than an unfilled section. problems.push(`\`${heading}\` still holds the \`${PLACEHOLDER}\` placeholder.`); } } return problems; } +/** + * The repo's own template, `null` when it holds none, or `{ unreadable }` when a + * candidate path answered with anything but a 404. The repo may hold a template + * there, and judging the body against the shared one instead would fail a body + * that follows the template the author was given. + */ async function readRepoTemplate({ github, context, core }, ref) { - for (const templatePath of REPO_TEMPLATE_PATHS) { + for (const templatePath of PR_TEMPLATE_PATHS) { try { const response = await github.rest.repos.getContent({ owner: context.repo.owner, @@ -90,15 +80,15 @@ async function readRepoTemplate({ github, context, core }, ref) { }); if (response.data?.content) { core.info(`Checking against ${templatePath} on ${ref}.`); - return Buffer.from(response.data.content, 'base64').toString('utf8'); + return { template: Buffer.from(response.data.content, 'base64').toString('utf8') }; } } catch (error) { if (error.status !== 404) { - core.warning(`Could not read ${templatePath}: ${error.message}`); + return { unreadable: `${templatePath}: ${error.message}` }; } } } - return null; + return { template: null }; } module.exports = async ({ github, context, core }) => { @@ -111,12 +101,14 @@ module.exports = async ({ github, context, core }) => { return; } - const repoTemplate = await readRepoTemplate( - { github, context, core }, - pullRequest.base.ref, - ); - const template = repoTemplate ?? fs.readFileSync(ORG_TEMPLATE_PATH, 'utf8'); - if (!repoTemplate) { + const found = await readRepoTemplate({ github, context, core }, pullRequest.base.ref); + if (found.unreadable) { + core.warning(`Could not read ${found.unreadable}, so the template is not checked.`); + return; + } + + const template = found.template ?? fs.readFileSync(ORG_TEMPLATE_PATH, 'utf8'); + if (!found.template) { core.info('No template in this repo, so the shared template applies.'); } diff --git a/scripts/pull-request-template.test.js b/scripts/pull-request-template.test.js index 6957939..8cff433 100644 --- a/scripts/pull-request-template.test.js +++ b/scripts/pull-request-template.test.js @@ -209,11 +209,15 @@ test('the template is read from the base branch, never the head', async () => { test('an uppercase template in the repo is found', async () => { const core = fakeCore(); - const github = fakeGithub({ templates: { '.github/PULL_REQUEST_TEMPLATE.md': MARKED } }); + // A marked heading the shared template does not carry, so falling back to the + // shared one could not produce this failure. + const own = ['## Evidence', '', '', 'what to paste', ''].join('\n'); + const github = fakeGithub({ templates: { '.github/PULL_REQUEST_TEMPLATE.md': own } }); await pullRequestTemplate({ github, context: fakeContext({ body: '' }), core }); assert.equal(core.failures.length, 1); - assert.match(core.failures[0], /## Summary/); + assert.match(core.failures[0], /## Evidence/); + assert.ok(!core.failures[0].includes('## AI usage'), 'the shared template must not be reached'); }); test('a repo holding no template falls back to the shared one', async () => { @@ -229,7 +233,7 @@ test('a repo holding no template falls back to the shared one', async () => { assert.match(core.failures[0], /## AI usage/); }); -test('a read failure other than 404 is reported and does not fail the run', async () => { +test('an unreadable candidate path skips the check rather than falling back', async () => { const core = fakeCore(); const github = fakeGithub(); github.rest.repos.getContent = async () => { @@ -237,8 +241,21 @@ test('a read failure other than 404 is reported and does not fail the run', asyn error.status = 500; throw error; }; - await pullRequestTemplate({ github, context: fakeContext({ body: SHARED_BODY }), core }); + // An empty body: falling back to the shared template would fail it. + await pullRequestTemplate({ github, context: fakeContext({ body: '' }), core }); + + assert.equal(core.warnings.length, 1); + assert.match(core.warnings[0], /Could not read/); + assert.deepEqual(core.failures, [], 'a body may follow a template this run could not read'); +}); + +test('an ellipsis inside a sentence is prose, not the placeholder', async () => { + const core = fakeCore(); + await pullRequestTemplate({ + github: fakeGithub({ templates: repoTemplate(MARKED) }), + context: fakeContext({ body: '## Summary\n\nAdds a Loading… spinner.\n' }), + core, + }); - assert.equal(core.warnings.length, 6); assert.deepEqual(core.failures, []); }); diff --git a/scripts/sync-automation-template.js b/scripts/sync-automation-template.js index 198903b..e004e1a 100644 --- a/scripts/sync-automation-template.js +++ b/scripts/sync-automation-template.js @@ -16,6 +16,8 @@ const fs = require('fs'); const path = require('path'); +const { PR_TEMPLATE_PATHS, PR_TEMPLATE_TARGET, PR_TEMPLATE_MARKER } = require('./constants'); + const ROOT = path.join(__dirname, '..'); // SYNC_ORG points the whole run at another org, so the write path can be @@ -23,7 +25,6 @@ const ROOT = path.join(__dirname, '..'); 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'; const API = 'https://api.github.com'; const CONSUMER_MARKER = 'workflows/automation.yml@'; @@ -53,17 +54,30 @@ function httpApi(token) { }; } -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'); - -const SUMMARY = 'Internal: refresh the copied automation.yml so it matches the current shared template'; +const UPSTREAM_NOTE = { + [TARGET_PATH]: 'edit `automation-registry.yml` upstream and regenerate `automation-template.yml`', + [PR_TEMPLATE_TARGET]: 'edit the shared template upstream', +}; + +function explanation(targets) { + const named = targets.map(target => `\`${target}\``).join(' and '); + return [ + `This replaces ${named} with the current copy from`, + `[${ORG}/.github](https://github.com/${ORG}/.github).`, + '', + ...targets.map( + target => `Do not edit \`${target}\` here: ${UPSTREAM_NOTE[target]}, or the next sync will overwrite the change.` + ), + '', + 'Opened automatically. A core maintainer reviews and merges it.', + ].join('\n'); +} + +function title(targets) { + return `Refresh ${targets.map((target) => path.basename(target)).join(' and ')} from the shared template`; +} + +const SUMMARY = 'Internal: refresh the copied files so they match the current shared ones'; const FIELD = /^([ \t]*-[ \t]*\*\*([^*]+?):\*\*).*$/gm; // Splits on level-two headings only, so a deeper heading stays inside its section. @@ -76,7 +90,8 @@ function sections(markdown) { return out; } -function prBody(prTemplate, { keep, describe, answers }) { +function prBody(prTemplate, { keep, describe, answers }, targets) { + const EXPLANATION = explanation(targets); if (!prTemplate) return `${EXPLANATION}\n`; const kept = sections(prTemplate).filter((s) => keep.includes(s.heading)); const parts = kept.map((s) => { @@ -115,23 +130,6 @@ const KDS_TEMPLATE = { }, }; -// The contents API is case sensitive, so both spellings are tried in each of the -// three places GitHub reads a pull request template from. -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', - 'docs/PULL_REQUEST_TEMPLATE.md', -]; - -const PR_TEMPLATE_TARGET = '.github/PULL_REQUEST_TEMPLATE.md'; - -// morango's own template sits at PR_TEMPLATE_TARGET, the path the sync writes to, -// so existence alone cannot tell a copy of ours from a template of their own. -const PR_TEMPLATE_MARKER = 'shared pull request template'; - /** * The files carried to a consumer. `mustExist` says the consumer already holds * one, so a missing file is a fault rather than work to do. `ownStops` says a @@ -154,8 +152,11 @@ async function readPrTemplate(api, repo, ref) { /** * Whether the consumer keeps a pull request template of its own, in any of the * places GitHub reads one from. An unreadable path counts as one, so a failed - * read never overwrites a template this run could not see. At the path the sync - * writes to, a copy of ours carries PR_TEMPLATE_MARKER and their own does not. + * read never overwrites a template this run could not see. + * + * morango's own template sits at PR_TEMPLATE_TARGET, the path the sync writes + * to, so at that one path existence cannot tell a copy of ours from theirs. A + * copy of ours carries PR_TEMPLATE_MARKER. */ async function keepsOwnPrTemplate(api, repo, ref) { for (const p of PR_TEMPLATE_PATHS) { @@ -266,9 +267,10 @@ function classifyDrift(closedPr, proposed, template) { return closedPr.merged_at ? 'toolchain-conflict' : 'declined'; } -/** - * The files this consumer takes, and the content each one must end up holding. - */ +// Reported when no file is left to propose. A toolchain conflict needs +// attention, while a decline is the maintainer's answer and needs none. +const SETTLED_STATES = ['toolchain-conflict', 'declined']; + async function applicableFiles(api, repo, base, sources) { const files = []; for (const file of SYNCED_FILES) { @@ -303,27 +305,39 @@ async function syncRepo(api, consumer, sources, { dryRun }) { const open = await openSyncPr(api, repo); if (open.error) return { repo, state: 'error', detail: open.error }; + const settled = []; + let drifted = pending; if (!open.pr) { const closed = await lastClosedSyncPr(api, repo); if (closed.error) return { repo, state: 'error', detail: closed.error }; if (closed.pr) { const ref = closedPrRef(closed.pr); if (!ref) return { repo, state: 'error', detail: 'no reference on the closed pull request' }; - const verdicts = []; + // Per file, because a file a maintainer declined must not be offered again + // every time another file drifts. + drifted = []; for (const file of pending) { const proposed = await readFile(api, repo, file.target, ref); if (proposed.error) return { repo, state: 'error', detail: proposed.error }; - verdicts.push(classifyDrift(closed.pr, proposed.content, file.content)); + const verdict = classifyDrift(closed.pr, proposed.content, file.content); + if (verdict === 'drift') drifted.push(file); + else settled.push({ target: file.target, verdict, pr: closed.pr.number }); } - // Every pending file has to agree. One file the template has moved on for - // is work to do, whatever the others say. - const verdict = verdicts.every(v => v === verdicts[0]) ? verdicts[0] : 'drift'; - if (verdict !== 'drift') return { repo, state: verdict, pr: closed.pr.number }; } } + if (!drifted.length) { + const worst = SETTLED_STATES.find(state => settled.some(s => s.verdict === state)); + return { repo, state: worst, pr: settled[0].pr, files: settled.map(s => s.target) }; + } + if (dryRun) { - return { repo, state: open.pr ? 'would-update' : 'would-open', pr: open.pr && open.pr.number }; + return { + repo, + state: open.pr ? 'would-update' : 'would-open', + pr: open.pr && open.pr.number, + files: drifted.map(file => file.target), + }; } const baseRef = await api('GET', `/repos/${ORG}/${repo}/git/ref/heads/${base}`); @@ -342,12 +356,14 @@ async function syncRepo(api, consumer, sources, { dryRun }) { } } - for (const file of pending) { + const targets = drifted.map(file => file.target); + + for (const file of drifted) { const onBranch = await readFile(api, repo, file.target, BRANCH); if (onBranch.error) return { repo, state: 'error', detail: onBranch.error }; const put = await api('PUT', `/repos/${ORG}/${repo}/contents/${file.target}`, { - message: TITLE, + message: title([file.target]), content: Buffer.from(file.content, 'utf8').toString('base64'), branch: BRANCH, ...(onBranch.sha ? { sha: onBranch.sha } : {}), @@ -355,16 +371,18 @@ async function syncRepo(api, consumer, sources, { dryRun }) { if (!put.ok) return { repo, state: 'error', detail: `write failed (${detail(put)})` }; } - if (open.pr) return { repo, state: 'updated', pr: open.pr.number, url: open.pr.html_url }; + if (open.pr) { + return { repo, state: 'updated', pr: open.pr.number, url: open.pr.html_url, files: targets }; + } const pr = await api('POST', `/repos/${ORG}/${repo}/pulls`, { - title: TITLE, + title: title(targets), head: BRANCH, base, - body: prBody(await readPrTemplate(api, repo, base), KDS_TEMPLATE), + body: prBody(await readPrTemplate(api, repo, base), KDS_TEMPLATE, targets), }); 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 }; + return { repo, state: 'opened', pr: pr.data.number, url: pr.data.html_url, files: targets }; } async function run(api, sources, options) { @@ -384,7 +402,8 @@ async function run(api, sources, options) { function report(results) { 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 files = r.files ? ` (${r.files.join(', ')})` : ''; + console.log(`${r.repo.padEnd(26)} ${r.state.padEnd(20)} ${extra}${files}`); } const problems = results.filter((r) => PROBLEM_STATES.includes(r.state)); const drifted = results.filter((r) => r.state !== 'in-sync'); diff --git a/scripts/sync-automation-template.test.js b/scripts/sync-automation-template.test.js index 1d2bc16..aabcc50 100644 --- a/scripts/sync-automation-template.test.js +++ b/scripts/sync-automation-template.test.js @@ -1,6 +1,10 @@ +const fs = require('fs'); +const path = require('path'); const test = require('node:test'); const assert = require('node:assert/strict'); +const { PR_TEMPLATE_MARKER } = require('./constants'); + const { run, classifyDrift, @@ -144,7 +148,7 @@ test('a repo that checks the description gets its own template, with every field const body = bodyOf(calls); assert.ok(body.startsWith('## Description'), 'the repo template decides the shape'); assert.ok(!body.includes('Summary of change(s)'), 'the placeholder fails check-description'); - assert.match(body, /- \*\*Description:\*\* Internal: refresh the copied automation\.yml/); + assert.match(body, /- \*\*Description:\*\* Internal: refresh the copied files/); 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'); @@ -447,6 +451,74 @@ test('a closed unmerged pull request is read at its head', async () => { ); }); +// The copy of the shared template at a given ref. +const prAtRef = (sha, content) => [ + `GET contents/.github/PULL_REQUEST_TEMPLATE.md?ref=${sha}`, + ok({ sha: 'x', content: encode(content) }), +]; + +test('a declined template is not offered again when automation.yml drifts', async () => { + const calls = []; + const routes = [ + ...baseRoutes(STALE), + noSharedTemplate(), + ['GET /repos/learningequality/demo/pulls?state=closed', ok([{ number: 9, merged_at: null, head: { sha: 'h1' } }])], + atRef('h1', STALE), + prAtRef('h1', PR_SHARED), + ]; + const results = await run(makeApi(routes, calls), SOURCES, {}); + + assert.equal(results[0].state, 'opened', 'automation.yml still moved on'); + assert.deepEqual(results[0].files, [TARGET_PATH]); + assert.equal(putsTo(calls, PR_TEMPLATE_TARGET).length, 0, 'the maintainer already said no to this one'); +}); + +test('a template deleted after the merge is left alone rather than failing every run', async () => { + const calls = []; + const routes = [ + ...baseRoutes(TEMPLATE), + noSharedTemplate(), + mergedSync('m1'), + prAtRef('m1', PR_SHARED), + ]; + const results = await run(makeApi(routes, calls), SOURCES, {}); + + assert.equal(results[0].state, 'toolchain-conflict'); + assert.deepEqual(results[0].files, [PR_TEMPLATE_TARGET]); + assert.equal(calls.filter((c) => c.method !== 'GET').length, 0); +}); + +test('the first run after a merge offers a template the closed pull request never held', async () => { + const calls = []; + const routes = [...baseRoutes(STALE), noSharedTemplate(), mergedSync('m1'), atRef('m1', STALE)]; + const results = await run(makeApi(routes, calls), SOURCES, {}); + + assert.equal(results[0].state, 'opened'); + assert.deepEqual(results[0].files, [TARGET_PATH, PR_TEMPLATE_TARGET]); +}); + +test('the shared template carries the marker the sync recognises its own copies by', () => { + const real = fs.readFileSync(path.join(__dirname, '..', PR_TEMPLATE_TARGET), 'utf8'); + assert.ok( + real.includes(PR_TEMPLATE_MARKER), + 'without it the sync reads every copy it placed as the consumer\'s own and stops refreshing them' + ); +}); + +test('report names the files a pull request carries', () => { + const quiet = console.log; + const lines = []; + console.log = (line) => lines.push(line); + try { + report([{ repo: 'demo', state: 'would-open', files: [TARGET_PATH, PR_TEMPLATE_TARGET] }]); + } finally { + console.log = quiet; + } + + assert.match(lines[0], /automation\.yml/); + assert.match(lines[0], /PULL_REQUEST_TEMPLATE\.md/); +}); + test('report counts only the states that stop a merge', () => { const quiet = console.log; console.log = () => {}; From b1b7bace4c08bbdba7a18fe42c13f666766054a1 Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Wed, 21 Oct 2026 22:42:55 +0300 Subject: [PATCH 04/12] Fail a required section left as the template wrote it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The check knew one placeholder, the lone `…` line that the shared template uses. morango fills its Summary with `*Short description*`, so a body that kept that line passed. kolibri-design-system has the same hole in its Changelog, where its template writes a bullet skeleton. A required section now fails when its text matches the same section in the template. That needs no list of placeholder strings per repo. A part filled section still passes, because the bullet labels a contributor is meant to keep would otherwise fail. --- docs/community-automations.md | 3 ++- scripts/pull-request-template.js | 13 ++++++---- scripts/pull-request-template.test.js | 37 +++++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 6 deletions(-) diff --git a/docs/community-automations.md b/docs/community-automations.md index 72fd2c0..900efa2 100644 --- a/docs/community-automations.md +++ b/docs/community-automations.md @@ -93,11 +93,12 @@ A section is required when its heading carries a `` comment in A template that marks no section requires every level-two heading, so a repo is strict rather than silently unchecked. To change what a repo requires, edit the markers in that repo's template. No change here is needed. -Three states fail, and the log names every one of them rather than stopping at the first: +Four states fail, and the log names every one of them rather than stopping at the first: - the heading is missing from the description - the section holds no text, once HTML comments are stripped - the section still holds the `…` placeholder +- the section still reads word for word as the template wrote it The failed check is the only report. Nothing is posted on the pull request. diff --git a/scripts/pull-request-template.js b/scripts/pull-request-template.js index 005020d..61a6838 100644 --- a/scripts/pull-request-template.js +++ b/scripts/pull-request-template.js @@ -32,10 +32,10 @@ function isMarked(section) { // A template that marks nothing requires every section, so a repo whose template // has no markers is strict rather than unchecked. -function requiredHeadings(template) { +function requiredSections(template) { const all = sections(template); const marked = all.filter(isMarked); - return (marked.length ? marked : all).map(section => section.heading); + return marked.length ? marked : all; } function content(section) { @@ -45,7 +45,7 @@ function content(section) { function failures(body, required) { const found = new Map(sections(body).map(section => [section.heading.toLowerCase(), section])); const problems = []; - for (const heading of required) { + for (const { heading, lines } of required) { const section = found.get(heading.toLowerCase()); if (!section) { problems.push(`\`${heading}\` is missing.`); @@ -58,6 +58,8 @@ function failures(body, required) { // The template puts the placeholder on a line of its own, so an ellipsis // inside a sentence is prose rather than an unfilled section. problems.push(`\`${heading}\` still holds the \`${PLACEHOLDER}\` placeholder.`); + } else if (text === content({ lines })) { + problems.push(`\`${heading}\` still holds the text from the template.`); } } return problems; @@ -112,7 +114,7 @@ module.exports = async ({ github, context, core }) => { core.info('No template in this repo, so the shared template applies.'); } - const required = requiredHeadings(template); + const required = requiredSections(template); if (!required.length) { core.warning('The template holds no sections, so there is nothing to check.'); return; @@ -130,7 +132,8 @@ module.exports = async ({ github, context, core }) => { core.setFailed( `The description does not follow the pull request template:\n` + problems.map(problem => `- ${problem}`).join('\n') + - `\n\nRequired: ${required.join(', ')}. Edit the description and the check runs again.`, + `\n\nRequired: ${required.map(section => section.heading).join(', ')}. ` + + `Edit the description and the check runs again.`, ); } catch (error) { core.setFailed(`Action failed with error: ${error.message}`); diff --git a/scripts/pull-request-template.test.js b/scripts/pull-request-template.test.js index 8cff433..56a44d1 100644 --- a/scripts/pull-request-template.test.js +++ b/scripts/pull-request-template.test.js @@ -19,6 +19,20 @@ const MARKED = [ const UNMARKED = ['## Summary', '', '…', '', '## Notes', '', 'anything', ''].join('\n'); +// Shaped like morango's template, which fills the section with prose rather than +// with the placeholder. +const PROSE_PLACEHOLDER = [ + '## Summary', + '', + '', + '*Short description*', + '', + '## Notes', + '', + 'anything', + '', +].join('\n'); + const GOOD_BODY = ['## Summary', '', 'What this does.', '', '## Notes', '', 'A note.'].join('\n'); // Satisfies the shared template, for the cases that fall back to it. @@ -249,6 +263,29 @@ test('an unreadable candidate path skips the check rather than falling back', as assert.deepEqual(core.failures, [], 'a body may follow a template this run could not read'); }); +test('a section left as the template wrote it fails, placeholder or not', async () => { + const core = fakeCore(); + await pullRequestTemplate({ + github: fakeGithub({ templates: repoTemplate(PROSE_PLACEHOLDER) }), + context: fakeContext({ body: '## Summary\n\n*Short description*\n' }), + core, + }); + + assert.equal(core.failures.length, 1); + assert.match(core.failures[0], /`## Summary` still holds the text from the template/); +}); + +test('rewriting that text passes', async () => { + const core = fakeCore(); + await pullRequestTemplate({ + github: fakeGithub({ templates: repoTemplate(PROSE_PLACEHOLDER) }), + context: fakeContext({ body: '## Summary\n\nRetries the push on a timeout.\n' }), + core, + }); + + assert.deepEqual(core.failures, []); +}); + test('an ellipsis inside a sentence is prose, not the placeholder', async () => { const core = fakeCore(); await pullRequestTemplate({ From 568391ce877f4e821fa78fff2e1265ded38c5c6b Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Wed, 21 Oct 2026 23:25:40 +0300 Subject: [PATCH 05/12] Clarify the pull request template notes in the automation docs The instruction to keep the marker sits next to a paragraph about morango's own file at the same path, so it now names this repo's template and links to it. --- docs/automation.md | 39 +++++++++++++++++------------------ docs/community-automations.md | 4 ++-- 2 files changed, 21 insertions(+), 22 deletions(-) diff --git a/docs/automation.md b/docs/automation.md index 399a12d..aa8012f 100644 --- a/docs/automation.md +++ b/docs/automation.md @@ -5,12 +5,12 @@ contributor replies, issue header management, etc.) copies a single file: `autom There is nothing else to maintain per repo — no per-automation caller, no hand-written `on:` block to keep in sync. -The sync workflow carries one more file: `.github/PULL_REQUEST_TEMPLATE.md`. GitHub applies an org -default template in the web form only, so a pull request opened with `gh pr create`, the API, or a -coding agent never sees it. A copy in the consumer's checkout is where every tool looks. A repo -holding a pull request template of its own keeps it and takes no copy. +The sync workflow also manages `.github/PULL_REQUEST_TEMPLATE.md`. GitHub applies the org-level default +template only in the web form, so pull requests created via `gh pr create`, the API, or coding agents +don't automatically use it. Keeping a copy in the consumer repository ensures these tools can find it +locally. Repositories with their own pull request template retain it and don't receive the synced copy. -The community-contribution spreadsheet is not one of them. `update-pr-spreadsheet.yml` runs daily +The community-contribution spreadsheet is not synced this way. `update-pr-spreadsheet.yml` runs daily in this repo and upserts every public contributor PR in the org updated in the last two days. Run it by hand with `dry_run` to log the rows without writing, or a larger `days` to backfill. @@ -100,21 +100,20 @@ every repo whose `.github/workflows/automation.yml` calls this repo's `automatio request targets that repo's default branch, which is the only branch GitHub evaluates workflow triggers from. -For each consumer it compares each shared file with the copy and opens a pull request when any of -them differ. A repo whose every file matches gets nothing, while a repo with an existing sync pull -request has that pull request updated rather than a second one opened. One pull request carries -every file that drifted. - -A consumer that keeps a pull request template of its own is left alone for that file. The check -covers all six places GitHub reads a template from, in both spellings, and a path it cannot read -counts as a template of its own, so a failed read never overwrites a file this run could not see. - -`morango` keeps its own template at `.github/PULL_REQUEST_TEMPLATE.md`, which is the path the sync -writes to, so existence alone cannot tell a copy of ours from a template of theirs. Our template -opens with a marker comment naming this repo, and at that one path a file without the marker is -treated as the consumer's own. Keep that line when editing -[the template](../.github/PULL_REQUEST_TEMPLATE.md): without it the sync stops refreshing every -copy it has already placed. +For each consumer, the workflow compares every shared file with its local copy and opens a pull +request if any differ. Repositories with no drift are left unchanged, and existing sync pull +requests are updated rather than duplicated. All changed files are included in a single pull request. + +A consumer's own pull request template is preserved. The workflow checks all six locations GitHub +supports, in both filename spellings. If a path cannot be read, it is treated as an existing +template to avoid overwriting a file the workflow could not inspect. + +`morango` keeps its own template at `.github/PULL_REQUEST_TEMPLATE.md`, the same path used for +syncing ours. To distinguish them, our template starts with a marker comment identifying this +repository. At this path, files without that marker are treated as consumer-owned. + +Keep the marker when editing [this repo's template](../.github/PULL_REQUEST_TEMPLATE.md). Without +it, the workflow stops refreshing copies it previously synced. 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 diff --git a/docs/community-automations.md b/docs/community-automations.md index 900efa2..561c198 100644 --- a/docs/community-automations.md +++ b/docs/community-automations.md @@ -82,7 +82,7 @@ In `scripts/constants.js` set: Fails the run when a pull request description does not carry the sections its template requires. It runs on `opened`, `edited` and `reopened`, so a corrected description clears the check. Bot authors are exempt. -The template is resolved the way GitHub resolves it: the repo's own copy first, in all six places GitHub reads one from, and this repo's `.github/PULL_REQUEST_TEMPLATE.md` second. It is read from the pull request's base branch, so a pull request cannot relax the rules it is judged by. +The template is resolved the way GitHub resolves it: the repo's own copy first, in all six places GitHub reads them from, and this repo's `.github/PULL_REQUEST_TEMPLATE.md` next. It is read from the pull request's base branch, so a pull request cannot relax the rules it is judged by. A section is required when its heading carries a `` comment in the template: @@ -91,7 +91,7 @@ A section is required when its heading carries a `` comment in ``` -A template that marks no section requires every level-two heading, so a repo is strict rather than silently unchecked. To change what a repo requires, edit the markers in that repo's template. No change here is needed. +A template that marks no section requires every level-two heading, so that a repo is strict rather than silently unchecked. To change what a repo requires, edit the markers in that repo's template. No change is required here. Four states fail, and the log names every one of them rather than stopping at the first: From 30053d3c6b50acc13b29c19ee7ca2950bea1dbe6 Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Wed, 21 Oct 2026 23:31:55 +0300 Subject: [PATCH 06/12] Put the spreadsheet note back beside the automations it refers to The template sync paragraph landed between the list of shared automations and the note that the spreadsheet is not one of them, so the note read as if it were about the synced files. --- docs/automation.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/automation.md b/docs/automation.md index aa8012f..5b49cfe 100644 --- a/docs/automation.md +++ b/docs/automation.md @@ -5,15 +5,15 @@ contributor replies, issue header management, etc.) copies a single file: `autom There is nothing else to maintain per repo — no per-automation caller, no hand-written `on:` block to keep in sync. +The community-contribution spreadsheet is not one of those automations. `update-pr-spreadsheet.yml` +runs daily in this repo and upserts every public contributor PR in the org updated in the last two +days. Run it by hand with `dry_run` to log the rows without writing, or a larger `days` to backfill. + The sync workflow also manages `.github/PULL_REQUEST_TEMPLATE.md`. GitHub applies the org-level default template only in the web form, so pull requests created via `gh pr create`, the API, or coding agents don't automatically use it. Keeping a copy in the consumer repository ensures these tools can find it locally. Repositories with their own pull request template retain it and don't receive the synced copy. -The community-contribution spreadsheet is not synced this way. `update-pr-spreadsheet.yml` runs daily -in this repo and upserts every public contributor PR in the org updated in the last two days. Run it -by hand with `dry_run` to log the rows without writing, or a larger `days` to backfill. - ## How it fits together ``` From 46964b9c2d47481783a87a5f0e2c3f3dc87fb56a Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Thu, 22 Oct 2026 12:00:07 +0300 Subject: [PATCH 07/12] Reword the template read docstring --- scripts/pull-request-template.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/scripts/pull-request-template.js b/scripts/pull-request-template.js index 61a6838..735f7c3 100644 --- a/scripts/pull-request-template.js +++ b/scripts/pull-request-template.js @@ -66,10 +66,10 @@ function failures(body, required) { } /** - * The repo's own template, `null` when it holds none, or `{ unreadable }` when a - * candidate path answered with anything but a 404. The repo may hold a template - * there, and judging the body against the shared one instead would fail a body - * that follows the template the author was given. + * The repo's own template, `null` if none exists, or `{ unreadable }` if a + * candidate path returns anything other than a 404. An unreadable path may + * contain a repo-owned template, so validating against the shared template + * could reject a body that follows the template the author was given. */ async function readRepoTemplate({ github, context, core }, ref) { for (const templatePath of PR_TEMPLATE_PATHS) { From 2b495ee3df275cbe41d3caf4864ab514dbbe4b5f Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Thu, 22 Oct 2026 12:43:35 +0300 Subject: [PATCH 08/12] Name the source repo in the template marker The marker line said "Edit it there", which left the reader to carry the repo name across a full stop. It now names the repo and the path. The constant the sync matches on follows the new wording. It names the repo too, so another repo's own template cannot be mistaken for a copy of ours. --- .github/PULL_REQUEST_TEMPLATE.md | 2 +- scripts/constants.js | 2 +- scripts/sync-automation-template.test.js | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 496a94c..b3c9a27 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -1,4 +1,4 @@ - + ', + '', '## Summary', '', '', From d017cf16235df9fa2ebec29eb9cfebe87ec80619 Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Thu, 22 Oct 2026 12:44:58 +0300 Subject: [PATCH 09/12] Split the body and the template on CRLF as well as LF The GitHub web form returns a pull request body with CRLF line endings, and the contents API returns the template with LF. Splitting on LF alone left a carriage return on every body line but the last, so a multi line section left as the template wrote it compared as different and passed. Single line sections were unaffected, because the trim removed the lone carriage return. --- scripts/pull-request-template.js | 2 +- scripts/pull-request-template.test.js | 30 +++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/scripts/pull-request-template.js b/scripts/pull-request-template.js index 735f7c3..17f093a 100644 --- a/scripts/pull-request-template.js +++ b/scripts/pull-request-template.js @@ -18,7 +18,7 @@ const PLACEHOLDER = '…'; function sections(markdown) { const out = []; - for (const line of (markdown || '').split('\n')) { + for (const line of (markdown || '').split(/\r?\n/)) { if (HEADING.test(line)) out.push({ heading: line.trim(), lines: [] }); else if (out.length) out[out.length - 1].lines.push(line); } diff --git a/scripts/pull-request-template.test.js b/scripts/pull-request-template.test.js index 56a44d1..73a0b64 100644 --- a/scripts/pull-request-template.test.js +++ b/scripts/pull-request-template.test.js @@ -33,6 +33,17 @@ const PROSE_PLACEHOLDER = [ '', ].join('\n'); +// A required section of more than one line, as kolibri-design-system's Changelog +// is. The GitHub web form returns the body with CRLF line endings. +const MULTILINE = [ + '## Changelog', + '', + '', + '- **Description:** Summary of change(s)', + '- **Breaking:** yes / no', + '', +].join('\n'); + const GOOD_BODY = ['## Summary', '', 'What this does.', '', '## Notes', '', 'A note.'].join('\n'); // Satisfies the shared template, for the cases that fall back to it. @@ -286,6 +297,25 @@ test('rewriting that text passes', async () => { assert.deepEqual(core.failures, []); }); +test('a CRLF body left as the template wrote it fails too', async () => { + const core = fakeCore(); + const body = [ + '## Changelog', + '', + '- **Description:** Summary of change(s)', + '- **Breaking:** yes / no', + '', + ].join('\r\n'); + await pullRequestTemplate({ + github: fakeGithub({ templates: repoTemplate(MULTILINE) }), + context: fakeContext({ body }), + core, + }); + + assert.equal(core.failures.length, 1); + assert.match(core.failures[0], /`## Changelog` still holds the text from the template/); +}); + test('an ellipsis inside a sentence is prose, not the placeholder', async () => { const core = fakeCore(); await pullRequestTemplate({ From 4f4e7cf797d519005a61fa90f36bde6ea753eaee Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Thu, 22 Oct 2026 13:49:55 +0300 Subject: [PATCH 10/12] Drop the unused body fixture from the template tests The test that used it now passes an empty body, which is what makes the case, so the fixture was left unreferenced and eslint flagged it. --- scripts/pull-request-template.test.js | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/scripts/pull-request-template.test.js b/scripts/pull-request-template.test.js index 73a0b64..d688f83 100644 --- a/scripts/pull-request-template.test.js +++ b/scripts/pull-request-template.test.js @@ -46,25 +46,6 @@ const MULTILINE = [ const GOOD_BODY = ['## Summary', '', 'What this does.', '', '## Notes', '', 'A note.'].join('\n'); -// Satisfies the shared template, for the cases that fall back to it. -const SHARED_BODY = [ - '## Summary', - '', - 'What this does.', - '', - '## References', - '', - 'Closes #1.', - '', - '## Reviewer guidance', - '', - 'Read the diff.', - '', - '## AI usage', - '', - 'None.', -].join('\n'); - function fakeGithub({ templates = {} } = {}) { const reads = []; return { From c4940752fa4b4285a8722f4e7eed80b961dcac8f Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Thu, 22 Oct 2026 20:55:25 +0300 Subject: [PATCH 11/12] Revert the pull request template propagation GitHub already applies this repo's template to any repo in the org holding none of its own, so copying the file into each consumer repeated work GitHub does natively. The check stays. It reads a repo's own template when there is one and falls back to this repo's, so it needs no copy in the consumer. --- .github/PULL_REQUEST_TEMPLATE.md | 1 - .../workflows/sync-automation-template.yml | 1 - docs/automation.md | 24 +- scripts/constants.js | 5 +- scripts/sync-automation-template.js | 193 ++++----------- scripts/sync-automation-template.test.js | 223 ++---------------- 6 files changed, 78 insertions(+), 369 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index b3c9a27..af88746 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -1,4 +1,3 @@ - ', - '## Summary', - '', - '', - '…', - '', -].join('\n'); - -const SOURCES = { [TARGET_PATH]: TEMPLATE, [PR_TEMPLATE_TARGET]: PR_SHARED }; - // Not "main": a default branch that differs is the only way to catch a hardcoded base. const BASE = 'develop'; const encode = (s) => Buffer.from(s, 'utf8').toString('base64'); @@ -89,7 +70,6 @@ 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 contents/.github/PULL_REQUEST_TEMPLATE.md', ok({ sha: 'pr-sha', content: encode(PR_SHARED) })], ['GET /repos/learningequality/demo/pulls?state=open', ok([])], ['GET /repos/learningequality/demo/pulls?state=closed', ok([])], [`GET /repos/learningequality/demo/git/ref/heads/${BASE}`, ok({ object: { sha: 'base-sha' } })], @@ -98,7 +78,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 = {}) => (await run(makeApi(routes), SOURCES, 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'); @@ -112,7 +92,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), SOURCES, {}); + 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); @@ -120,7 +100,7 @@ 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), SOURCES, {}); + 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); @@ -144,11 +124,11 @@ const bodyOf = (calls) => calls.find((c) => c.method === 'POST' && c.url.endsWit test('a repo that checks the description gets its own template, with every field answered', async () => { const calls = []; - await run(makeApi(checkedRoutes(STALE), calls), SOURCES, {}); + 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('Summary of change(s)'), 'the placeholder fails check-description'); - assert.match(body, /- \*\*Description:\*\* Internal: refresh the copied files/); + 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'); @@ -161,7 +141,7 @@ test('a renamed description heading still leaves the change explained', async () ...checkedRoutes(STALE), ['GET contents/.github/pull_request_template.md', ok({ content: encode(renamed) })], ]; - await run(makeApi(routes, calls), SOURCES, {}); + 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'); @@ -173,7 +153,7 @@ test('a template with none of the kept headings still explains itself', async () ...checkedRoutes(STALE), ['GET contents/.github/pull_request_template.md', ok({ content: encode('## Notes\n\nnothing here\n') })], ]; - await run(makeApi(routes, calls), SOURCES, {}); + 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'); @@ -181,7 +161,7 @@ test('a template with none of the kept headings still explains itself', async () test('sections that would arrive unfilled are dropped', async () => { const calls = []; - await run(makeApi(checkedRoutes(STALE), calls), SOURCES, {}); + 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')); @@ -195,110 +175,27 @@ test('any other repo gets the plain explanation, template or not', async () => { ...baseRoutes(STALE), ['GET contents/.github/pull_request_template.md', ok({ content: encode(PR_TEMPLATE) })], ]; - await run(makeApi(routes, calls), SOURCES, {}); + await run(makeApi(routes, calls), TEMPLATE, {}); const body = bodyOf(calls); - assert.ok(!body.includes('## Description'), 'only the repo that checks the description gets its template read'); + 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 () => { const calls = []; - await run(makeApi(baseRoutes(STALE), calls), SOURCES, {}); + 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')); }); -const putsTo = (calls, target) => calls.filter((c) => c.method === 'PUT' && c.url.includes(target)); - -// The lowercase spelling, which is a template of the repo's own rather than ours. -const ownTemplate = (content = PR_TEMPLATE) => [ - 'GET contents/.github/pull_request_template.md', - ok({ content: encode(content) }), -]; - -const noSharedTemplate = () => ['GET contents/.github/PULL_REQUEST_TEMPLATE.md', fail(404, 'Not Found')]; - -test('a consumer holding no template of its own takes the shared one', async () => { - const calls = []; - const routes = [...baseRoutes(TEMPLATE), noSharedTemplate()]; - const results = await run(makeApi(routes, calls), SOURCES, {}); - - assert.equal(results[0].state, 'opened'); - const put = putsTo(calls, PR_TEMPLATE_TARGET); - assert.equal(put.length, 1); - assert.equal(Buffer.from(put[0].body.content, 'base64').toString('utf8'), PR_SHARED); - assert.equal(putsTo(calls, TARGET_PATH).length, 0, 'automation.yml already matches'); -}); - -test('a consumer holding its own template keeps it', async () => { - const calls = []; - const routes = [...baseRoutes(TEMPLATE), ownTemplate(), noSharedTemplate()]; - const results = await run(makeApi(routes, calls), SOURCES, {}); - - assert.equal(results[0].state, 'in-sync'); - assert.equal(putsTo(calls, PR_TEMPLATE_TARGET).length, 0); -}); - -test('a candidate path that cannot be read counts as a template of its own', async () => { - const calls = []; - const routes = [ - ...baseRoutes(TEMPLATE), - ['GET contents/docs/pull_request_template.md', fail(500, 'Server Error')], - noSharedTemplate(), - ]; - const results = await run(makeApi(routes, calls), SOURCES, {}); - - assert.equal(results[0].state, 'in-sync', 'a failed read must never overwrite a template it cannot see'); - assert.equal(putsTo(calls, PR_TEMPLATE_TARGET).length, 0); -}); - -test('the shared template is replaced when a copy of ours has drifted', async () => { - const calls = []; - const stale = PR_SHARED.replace('## Summary', '## Overview'); - const routes = [ - ...baseRoutes(TEMPLATE), - ['GET contents/.github/PULL_REQUEST_TEMPLATE.md', ok({ sha: 'pr-sha', content: encode(stale) })], - ]; - const results = await run(makeApi(routes, calls), SOURCES, {}); - - assert.equal(results[0].state, 'opened'); - const put = putsTo(calls, PR_TEMPLATE_TARGET); - assert.equal(put.length, 1); - assert.equal(put[0].body.sha, 'pr-sha', 'an update without the sha fails with a 422'); -}); - -test('a template of their own at our path is left alone', async () => { - const calls = []; - const routes = [ - ...baseRoutes(TEMPLATE), - // morango's shape: its own template sits at the path the sync writes to. - ['GET contents/.github/PULL_REQUEST_TEMPLATE.md', ok({ sha: 'pr-sha', content: encode('## Summary\n') })], - ]; - const results = await run(makeApi(routes, calls), SOURCES, {}); - - assert.equal(results[0].state, 'in-sync'); - assert.equal(putsTo(calls, PR_TEMPLATE_TARGET).length, 0, 'their template must never be overwritten'); -}); - -test('both files drifted are written under one pull request', async () => { - const calls = []; - const routes = [...baseRoutes(STALE), noSharedTemplate()]; - const results = await run(makeApi(routes, calls), SOURCES, {}); - - assert.equal(results[0].state, 'opened'); - assert.equal(putsTo(calls, TARGET_PATH).length, 1); - assert.equal(putsTo(calls, PR_TEMPLATE_TARGET).length, 1); - assert.equal(calls.filter((c) => c.method === 'POST' && c.url.endsWith('/pulls')).length, 1); -}); - test('an open sync pull request is updated in place, keeping the file sha', 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), SOURCES, {}); + 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'); @@ -307,7 +204,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)), SOURCES, {}); + const results = await run(makeApi(baseRoutes(TEMPLATE, repos)), TEMPLATE, {}); assert.deepEqual( results.map((r) => r.repo), ['demo'] @@ -316,7 +213,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), SOURCES, {}); + const results = await run(makeApi(routes), TEMPLATE, {}); assert.deepEqual(results, []); }); @@ -325,7 +222,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), SOURCES, {}); + const results = await run(makeApi(routes), TEMPLATE, {}); assert.deepEqual(results, []); }); @@ -338,7 +235,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), SOURCES, {}), /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 () => { @@ -380,7 +277,7 @@ test('a thrown request during discovery is contained, not fatal', async () => { test('dry run reports without writing', async () => { const calls = []; - const results = await run(makeApi(baseRoutes(STALE), calls), SOURCES, { 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, @@ -422,7 +319,7 @@ const atRef = (sha, content) => [ test('a repo that reverted a merged sync is reported, and no pull request is opened', async () => { const calls = []; const routes = [...baseRoutes(STALE), mergedSync('m1'), atRef('m1', TEMPLATE)]; - const results = await run(makeApi(routes, calls), SOURCES, {}); + const results = await run(makeApi(routes, calls), TEMPLATE, {}); 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); }); @@ -432,7 +329,7 @@ test('a template published after the merge is drift, whatever the commit dates s // 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), SOURCES, {}); + const results = await run(makeApi(routes, calls), TEMPLATE, {}); assert.equal(results[0].state, 'opened', 'the merge left something older, so the template moved on'); }); @@ -443,7 +340,7 @@ test('a closed unmerged pull request is read at its head', async () => { ['GET /repos/learningequality/demo/pulls?state=closed', ok([{ number: 9, merged_at: null, head: { sha: 'h1' } }])], atRef('h1', TEMPLATE), ]; - const results = await run(makeApi(routes, calls), SOURCES, {}); + const results = await run(makeApi(routes, calls), TEMPLATE, {}); assert.equal(results[0].state, 'declined'); assert.ok( calls.some((c) => c.url.includes('ref=h1')), @@ -451,74 +348,6 @@ test('a closed unmerged pull request is read at its head', async () => { ); }); -// The copy of the shared template at a given ref. -const prAtRef = (sha, content) => [ - `GET contents/.github/PULL_REQUEST_TEMPLATE.md?ref=${sha}`, - ok({ sha: 'x', content: encode(content) }), -]; - -test('a declined template is not offered again when automation.yml drifts', async () => { - const calls = []; - const routes = [ - ...baseRoutes(STALE), - noSharedTemplate(), - ['GET /repos/learningequality/demo/pulls?state=closed', ok([{ number: 9, merged_at: null, head: { sha: 'h1' } }])], - atRef('h1', STALE), - prAtRef('h1', PR_SHARED), - ]; - const results = await run(makeApi(routes, calls), SOURCES, {}); - - assert.equal(results[0].state, 'opened', 'automation.yml still moved on'); - assert.deepEqual(results[0].files, [TARGET_PATH]); - assert.equal(putsTo(calls, PR_TEMPLATE_TARGET).length, 0, 'the maintainer already said no to this one'); -}); - -test('a template deleted after the merge is left alone rather than failing every run', async () => { - const calls = []; - const routes = [ - ...baseRoutes(TEMPLATE), - noSharedTemplate(), - mergedSync('m1'), - prAtRef('m1', PR_SHARED), - ]; - const results = await run(makeApi(routes, calls), SOURCES, {}); - - assert.equal(results[0].state, 'toolchain-conflict'); - assert.deepEqual(results[0].files, [PR_TEMPLATE_TARGET]); - assert.equal(calls.filter((c) => c.method !== 'GET').length, 0); -}); - -test('the first run after a merge offers a template the closed pull request never held', async () => { - const calls = []; - const routes = [...baseRoutes(STALE), noSharedTemplate(), mergedSync('m1'), atRef('m1', STALE)]; - const results = await run(makeApi(routes, calls), SOURCES, {}); - - assert.equal(results[0].state, 'opened'); - assert.deepEqual(results[0].files, [TARGET_PATH, PR_TEMPLATE_TARGET]); -}); - -test('the shared template carries the marker the sync recognises its own copies by', () => { - const real = fs.readFileSync(path.join(__dirname, '..', PR_TEMPLATE_TARGET), 'utf8'); - assert.ok( - real.includes(PR_TEMPLATE_MARKER), - 'without it the sync reads every copy it placed as the consumer\'s own and stops refreshing them' - ); -}); - -test('report names the files a pull request carries', () => { - const quiet = console.log; - const lines = []; - console.log = (line) => lines.push(line); - try { - report([{ repo: 'demo', state: 'would-open', files: [TARGET_PATH, PR_TEMPLATE_TARGET] }]); - } finally { - console.log = quiet; - } - - assert.match(lines[0], /automation\.yml/); - assert.match(lines[0], /PULL_REQUEST_TEMPLATE\.md/); -}); - test('report counts only the states that stop a merge', () => { const quiet = console.log; console.log = () => {}; @@ -547,7 +376,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), SOURCES, {}); + 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'); @@ -574,7 +403,7 @@ test('a named repo that is not a consumer says why, rather than reporting none', ]; for (const [r, expected] of cases) { const routes = [...baseRoutes(TEMPLATE), ['GET =/repos/learningequality/demo', ok(r)]]; - await assert.rejects(() => run(makeApi(routes), SOURCES, { only: 'demo' }), expected); + await assert.rejects(() => run(makeApi(routes), TEMPLATE, { only: 'demo' }), expected); } }); @@ -584,14 +413,14 @@ test('a named repo without the marker says why', async () => { ['GET =/repos/learningequality/demo', ok(repo())], ['GET contents/.github/workflows/automation.yml', ok({ sha: 'x', content: encode('name: other\n') })], ]; - await assert.rejects(() => run(makeApi(routes), SOURCES, { only: 'demo' }), /does not call the shared workflow/); + await assert.rejects(() => run(makeApi(routes), TEMPLATE, { only: 'demo' }), /does not call the shared workflow/); }); test('only limits the run to that repo, and never lists the org', async () => { const calls = []; const others = [repo({ name: 'demo' }), repo({ name: 'production-repo' })]; const routes = [...baseRoutes(TEMPLATE, others), ['GET =/repos/learningequality/demo', ok(repo())]]; - const results = await run(makeApi(routes, calls), SOURCES, { only: 'demo' }); + const results = await run(makeApi(routes, calls), TEMPLATE, { only: 'demo' }); assert.deepEqual( results.map((r) => r.repo), ['demo'] @@ -604,7 +433,7 @@ test('only limits the run to that repo, and never lists the org', async () => { test('without only, every repo in the listing is considered', async () => { const others = [repo({ name: 'demo' }), repo({ name: 'production-repo' })]; - const results = await run(makeApi(baseRoutes(TEMPLATE, others)), SOURCES, {}); + const results = await run(makeApi(baseRoutes(TEMPLATE, others)), TEMPLATE, {}); assert.deepEqual( results.map((r) => r.repo), ['demo', 'production-repo'] @@ -613,7 +442,7 @@ test('without only, every repo in the listing is considered', async () => { test('only reports an error when the repo cannot be read', async () => { const routes = [...baseRoutes(TEMPLATE), ['GET =/repos/learningequality/missing', fail(404, 'Not Found')]]; - await assert.rejects(() => run(makeApi(routes), SOURCES, { only: 'missing' }), /could not read missing/); + await assert.rejects(() => run(makeApi(routes), TEMPLATE, { only: 'missing' }), /could not read missing/); }); test('findConsumers pages through the org listing', async () => { From 81286a9db9628d0ad4d76e90e5cb9f26ed0de8d2 Mon Sep 17 00:00:00 2001 From: Samson Akol Date: Thu, 22 Oct 2026 20:55:33 +0300 Subject: [PATCH 12/12] Cover the sectionless template and the heading case --- scripts/pull-request-template.test.js | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/scripts/pull-request-template.test.js b/scripts/pull-request-template.test.js index d688f83..cadfce0 100644 --- a/scripts/pull-request-template.test.js +++ b/scripts/pull-request-template.test.js @@ -166,6 +166,30 @@ test('a template marking nothing requires every section', async () => { assert.match(core.failures[0], /`## Notes` is missing/); }); +test('a template holding no sections warns and fails nothing', async () => { + const core = fakeCore(); + await pullRequestTemplate({ + github: fakeGithub({ templates: repoTemplate('Open a pull request and describe it.\n') }), + context: fakeContext({ body: '' }), + core, + }); + + assert.deepEqual(core.failures, []); + assert.equal(core.warnings.length, 1); + assert.match(core.warnings[0], /holds no sections/); +}); + +test('a heading matches whatever case the body writes it in', async () => { + const core = fakeCore(); + await pullRequestTemplate({ + github: fakeGithub({ templates: repoTemplate(MARKED) }), + context: fakeContext({ body: '## summary\n\nWhat this does.' }), + core, + }); + + assert.deepEqual(core.failures, []); +}); + test('every failure is named, not only the first', async () => { const core = fakeCore(); await pullRequestTemplate({