diff --git a/.github/scripts/check-pr-coauthors.mjs b/.github/scripts/check-pr-coauthors.mjs new file mode 100644 index 0000000000..13239dd8e1 --- /dev/null +++ b/.github/scripts/check-pr-coauthors.mjs @@ -0,0 +1,135 @@ +#!/usr/bin/env node +/** + * check-pr-coauthors.mjs + * Surfaces the `Co-Authored-By` trailers a squash merge needs to keep + * contributors credited. + * Export: checkCoauthors(commits, prAuthor) → { passed, informational } + * + * This repository squash-merges, so every commit on a branch collapses into + * one commit authored by whoever presses the button. When a branch carries + * someone else's work — a rebase of a stale contributor PR, a port of an + * abandoned branch, a pairing session — their name survives only if the squash + * message carries a `Co-Authored-By` trailer for them. Nothing prompts for it, + * and the PR page keeps showing the original author either way, so the loss is + * invisible at exactly the moment it happens. + * + * Identity matching is a heuristic and is deliberately biased. A commit GitHub + * could not match to an account is credited unless its name or email resolves + * to the PR author, which will occasionally credit someone as a co-author of + * themselves — their git config carrying a real name where the comparison has + * only a login. That error costs a line a human drops while pasting. The + * opposite error costs a contributor their attribution silently, which is the + * failure this gate exists to prevent, so the bias runs towards over-crediting. + * + * Informational rather than a failure, on purpose. The squash message does not + * exist while the PR is open, so this cannot be verified here and cannot be + * fixed here either. Failing the PR would block work on something its author + * has no way to satisfy. What this can do is notice that the situation applies + * and hand over the exact lines to paste. + */ +import { fileURLToPath } from 'node:url'; + +/** + * Fetches every commit on a PR across GitHub pagination. + * + * Capped at the API's own ceiling: `/pulls/{n}/commits` returns at most 250 + * commits and silently stops. A branch that large is not the case this gate is + * about, and a partial list still surfaces the contributors it did see. + */ +export async function fetchAllPullRequestCommits(ghFetchFn, repo, prNumber, token) { + const commits = []; + + for (let page = 1; page <= 3; page += 1) { + const batch = await ghFetchFn( + `/repos/${repo}/pulls/${prNumber}/commits?per_page=100&page=${page}`, + token + ); + commits.push(...batch); + + if (batch.length < 100) break; + } + + return commits; +} + +/** GitHub's own no-reply address for a login, which is what trailers should use. */ +function noReplyEmail(login) { + return `${login}@users.noreply.github.com`; +} + +export function checkCoauthors(commits, prAuthor) { + const author = (prAuthor ?? '').toLowerCase(); + const contributors = new Map(); + // Emails already accounted for under a GitHub login. One person can appear + // both ways in the same branch — some commits matched to their account, some + // authored with an email GitHub does not know — and keying on login alone + // would then emit two trailers for them. + const seenEmails = new Set(); + + for (const entry of commits ?? []) { + const login = entry?.author?.login ?? null; + const gitName = entry?.commit?.author?.name ?? null; + const gitEmail = entry?.commit?.author?.email ?? null; + + // The PR author's own commits need no trailer — the squash is already + // theirs. Compared case-insensitively because GitHub logins are. + if (login && author && login.toLowerCase() === author) continue; + + // Bots author plenty of commits and crediting them is noise. + if (login && /\[bot\]$/.test(login)) continue; + if (!login && !gitName) continue; + + // A commit GitHub could not match to an account may still be the PR + // author's own — their git config carrying an email GitHub does not know. + // Without this they are listed as a co-author of themselves. + if (!login && author) { + const nameMatches = gitName && gitName.toLowerCase() === author; + const emailMatches = gitEmail && gitEmail.toLowerCase().startsWith(`${author}@`); + if (nameMatches || emailMatches) continue; + } + + // Prefer the GitHub identity, so the trailer links to a profile. Fall back + // to the raw git author for a commit GitHub could not match to an account. + const name = login ?? gitName; + const email = login ? noReplyEmail(login) : gitEmail; + if (!email) continue; + + // Keyed on identity, not on the rendered line. One person whose git config + // name changed across commits is still one person, and emitting them twice + // would put two trailers for the same contributor into the squash body. + const key = (login ?? gitEmail ?? name).toLowerCase(); + if (contributors.has(key)) continue; + const emailKey = (gitEmail ?? '').toLowerCase(); + if (emailKey && seenEmails.has(emailKey)) continue; + if (emailKey) seenEmails.add(emailKey); + + const displayName = gitName && login ? gitName : name; + contributors.set(key, { + trailer: `Co-Authored-By: ${displayName} <${email}>`, + name: displayName, + }); + } + + if (contributors.size === 0) return { passed: true, informational: [] }; + + const trailers = [...contributors.values()].map(c => c.trailer).sort(); + const names = [...new Set([...contributors.values()].map(c => c.name))].sort(); + const who = names.length === 1 ? names[0] : `${names.length} other contributors`; + + return { + passed: true, + informational: [ + `This branch carries commits by ${who}. Squash-merging drops that authorship unless ` + + 'the squash message carries their trailers, and nothing else will notice if it does not. ' + + 'Add to the squash body when merging:\n\n' + + trailers.map(line => ` ${line}`).join('\n'), + ], + }; +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) { + const commits = JSON.parse(process.env.PR_COMMITS ?? '[]'); + const result = checkCoauthors(commits, process.env.PR_AUTHOR ?? ''); + console.log(JSON.stringify(result)); + process.exit(0); +} diff --git a/.github/scripts/run-quality-gates.mjs b/.github/scripts/run-quality-gates.mjs index fde29a1a33..31a1a12a00 100644 --- a/.github/scripts/run-quality-gates.mjs +++ b/.github/scripts/run-quality-gates.mjs @@ -17,6 +17,7 @@ import { checkTestCoverage } from './check-pr-test-coverage.mjs'; import { checkLockfile } from './check-pr-lockfile.mjs'; import { checkDependencies } from './check-pr-dependencies.mjs'; import { checkReleaseBootstrap } from './check-pr-release-bootstrap.mjs'; +import { checkCoauthors, fetchAllPullRequestCommits } from './check-pr-coauthors.mjs'; const COMMENT_SIGNATURE = '— commitperclip'; @@ -106,6 +107,17 @@ async function main() { fetchAllPullRequestFiles(ghFetch, GH_REPO, prNumber, GH_TOKEN), ]); + // Separate, and allowed to fail. The co-author note is informational: it + // cannot fail a PR by design, so it must not be able to fail the workflow by + // accident either. Sharing the Promise.all above would let one transient + // 5xx on this request take down every gate, including the ones that block. + let commits = []; + try { + commits = await fetchAllPullRequestCommits(ghFetch, GH_REPO, prNumber, GH_TOKEN); + } catch (error) { + console.error(`co-author lookup skipped: ${error.message}`); + } + const prBody = pr.body ?? ''; const author = PR_AUTHOR ?? pr.user.login; const branch = PR_BRANCH ?? pr.head.ref; @@ -122,6 +134,7 @@ async function main() { checkDependencies(files, GH_TOKEN, GH_REPO, prNumber, pr.base?.ref), checkReleaseBootstrap(files, GH_TOKEN, GH_REPO, prNumber, pr.base?.ref), ]); + const coauthorResult = checkCoauthors(commits, author); const allFailures = [ ...templateResult.failures, @@ -133,6 +146,7 @@ async function main() { const informational = [ ...(depsResult.informational ?? []), ...(bootstrapResult.informational ?? []), + ...coauthorResult.informational, ]; const allPassed = allFailures.length === 0; diff --git a/.github/scripts/tests/check-pr-coauthors.test.mjs b/.github/scripts/tests/check-pr-coauthors.test.mjs new file mode 100644 index 0000000000..8f33953238 --- /dev/null +++ b/.github/scripts/tests/check-pr-coauthors.test.mjs @@ -0,0 +1,179 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { checkCoauthors, fetchAllPullRequestCommits } from '../check-pr-coauthors.mjs'; + +function commit(login, name = null, email = null) { + return { + author: login ? { login } : null, + commit: { author: { name: name ?? login, email: email ?? `${login}@users.noreply.github.com` } }, + }; +} + +test('checkCoauthors: says nothing when every commit is the PR author\'s own', () => { + const result = checkCoauthors( + [commit('tonio-alucema'), commit('tonio-alucema')], + 'tonio-alucema' + ); + + assert.equal(result.passed, true); + assert.deepEqual(result.informational, []); +}); + +test('checkCoauthors: hands over the trailer when the branch carries someone else\'s commit', () => { + // The case this exists for: a stale contributor PR rebased and landed by a + // maintainer. Squash-merging drops the contributor unless the squash body + // carries their trailer. + const result = checkCoauthors( + [commit('stubbi', 'Jannes Stubbemann'), commit('tonio-alucema')], + 'tonio-alucema' + ); + + assert.equal(result.informational.length, 1); + assert.match(result.informational[0], /Jannes Stubbemann/); + assert.match( + result.informational[0], + /Co-Authored-By: Jannes Stubbemann / + ); +}); + +test('checkCoauthors: never fails the PR, because the squash message does not exist yet', () => { + // Informational only. The author of the PR cannot satisfy this from the PR, + // so failing here would block work on something unfixable at that point. + const result = checkCoauthors([commit('stubbi')], 'tonio-alucema'); + + assert.equal(result.passed, true); +}); + +test('checkCoauthors: matches the PR author case-insensitively', () => { + // GitHub logins are case-insensitive, and PR_AUTHOR does not always arrive + // in the same case as the commit author login. + const result = checkCoauthors([commit('Tonio-Alucema')], 'tonio-alucema'); + + assert.deepEqual(result.informational, []); +}); + +test('checkCoauthors: ignores bots', () => { + const result = checkCoauthors( + [commit('github-actions[bot]'), commit('dependabot[bot]')], + 'tonio-alucema' + ); + + assert.deepEqual(result.informational, []); +}); + +test('checkCoauthors: lists each contributor once, however many commits they wrote', () => { + const result = checkCoauthors( + [commit('stubbi', 'Jannes Stubbemann'), commit('stubbi', 'Jannes Stubbemann')], + 'tonio-alucema' + ); + + const trailers = result.informational[0].match(/Co-Authored-By:/g) ?? []; + assert.equal(trailers.length, 1); +}); + +test('checkCoauthors: falls back to the raw git author when GitHub matched no account', () => { + // A commit authored with an email GitHub cannot resolve still deserves a + // trailer — that is precisely the identity most likely to be lost. + const result = checkCoauthors( + [{ author: null, commit: { author: { name: 'Ada Lovelace', email: 'ada@example.com' } } }], + 'tonio-alucema' + ); + + assert.match(result.informational[0], /Co-Authored-By: Ada Lovelace /); +}); + +test('checkCoauthors: skips an unattributable commit rather than emitting a broken trailer', () => { + const result = checkCoauthors( + [{ author: null, commit: { author: { name: 'Nameless', email: null } } }], + 'tonio-alucema' + ); + + assert.deepEqual(result.informational, []); +}); + +test('checkCoauthors: names the count rather than everyone when several contributed', () => { + const result = checkCoauthors( + [commit('stubbi', 'Jannes Stubbemann'), commit('elJayAdvisor', 'LJ')], + 'tonio-alucema' + ); + + assert.match(result.informational[0], /2 other contributors/); + assert.match(result.informational[0], /Jannes Stubbemann/); + assert.match(result.informational[0], /LJ/); +}); + +test('checkCoauthors: tolerates a PR with no commits', () => { + assert.deepEqual(checkCoauthors([], 'tonio-alucema').informational, []); + assert.deepEqual(checkCoauthors(undefined, 'tonio-alucema').informational, []); +}); + +test('fetchAllPullRequestCommits: pages until a short batch', async () => { + const seen = []; + const commits = await fetchAllPullRequestCommits(async (path) => { + seen.push(path); + if (path.endsWith('page=1')) return Array.from({ length: 100 }, () => commit('stubbi')); + return [commit('tonio-alucema')]; + }, 'paperclipai/paperclip', 9900, 'token'); + + assert.equal(commits.length, 101); + assert.equal(seen.length, 2); +}); + +test('fetchAllPullRequestCommits: stops at the API ceiling instead of looping', async () => { + // `/pulls/{n}/commits` caps at 250 and keeps returning full pages of nothing + // new past that. A branch that large is not what this gate is about, but it + // must not spin. + let calls = 0; + const commits = await fetchAllPullRequestCommits(async () => { + calls += 1; + return Array.from({ length: 100 }, () => commit('stubbi')); + }, 'paperclipai/paperclip', 9900, 'token'); + + assert.equal(calls, 3); + assert.equal(commits.length, 300); +}); + +test('checkCoauthors: counts one person once when their git name varies across commits', () => { + // People change their git config. Keying the dedup on the rendered trailer + // would put two lines for the same contributor into the squash body. + const result = checkCoauthors( + [commit('stubbi', 'Jannes Stubbemann'), commit('stubbi', 'J. Stubbemann')], + 'tonio-alucema' + ); + + const trailers = result.informational[0].match(/Co-Authored-By:/g) ?? []; + assert.equal(trailers.length, 1); + assert.doesNotMatch(result.informational[0], /other contributors/); +}); + +test('checkCoauthors: does not credit the PR author as a co-author of themselves', () => { + // Their own commit, authored with an email GitHub could not match to the + // account. Without the guard they appear in their own trailer list. + const byName = checkCoauthors( + [{ author: null, commit: { author: { name: 'tonio-alucema', email: 'tonio@example.com' } } }], + 'tonio-alucema' + ); + const byEmail = checkCoauthors( + [{ author: null, commit: { author: { name: 'Tonio', email: 'tonio-alucema@users.noreply.github.com' } } }], + 'tonio-alucema' + ); + + assert.deepEqual(byName.informational, []); + assert.deepEqual(byEmail.informational, []); +}); + +test('checkCoauthors: counts one person once when some commits matched their account and some did not', () => { + // The mixed case: GitHub resolved one commit to the login and left another + // unmatched, both carrying the same email. Keying on login alone emits two + // trailers for one contributor. + const result = checkCoauthors( + [ + commit('stubbi', 'Jannes Stubbemann', 'jannes@example.com'), + { author: null, commit: { author: { name: 'Jannes Stubbemann', email: 'jannes@example.com' } } }, + ], + 'tonio-alucema' + ); + + const trailers = result.informational[0].match(/Co-Authored-By:/g) ?? []; + assert.equal(trailers.length, 1); +}); diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 591caf04ed..b1f4138959 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -57,6 +57,8 @@ jobs: - name: Test no-git-push check run: node --test ./scripts/check-no-git-push.test.mjs + - name: Test PR quality-gate scripts + run: node --test '.github/scripts/tests/*.test.mjs' - name: Test general-server shard partition run: node --test ./scripts/__tests__/run-vitest-stable-shard.test.mjs