diff --git a/.github/workflows/pull-request-policy.yml b/.github/workflows/pull-request-policy.yml new file mode 100644 index 000000000..66b19cee9 --- /dev/null +++ b/.github/workflows/pull-request-policy.yml @@ -0,0 +1,42 @@ +# The title and description of a pull request are written by whoever opened +# it, so both are read from the environment rather than interpolated into a +# command: a title pasted into a shell is a title that can run as code. +# +# Actions are pinned by commit, never by tag. +name: Pull Request Policy + +on: + pull_request: + types: [opened, edited, synchronize, reopened, ready_for_review] + +permissions: + contents: read + +# A title is often fixed two or three times in a row. Only the last attempt +# is worth reporting on. +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + title_and_description: + name: Title and description + # Renovate writes `chore(deps): …` and dependabot writes `Bump x from y to + # z`. Neither is this project's format, and neither is ours to change -- + # the same allowance verify.commits makes for their commit messages. + if: ${{ !endsWith(github.event.pull_request.user.login, '[bot]') }} + runs-on: ubuntu-latest + steps: + - name: Check out project repository + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - name: Set up Node.js runtime + uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 + with: + node-version-file: 'package.json' + # No install step: the task imports one module from this package and + # nothing from node_modules, and node resolves that by itself. + - name: Verify title and description + run: node build/tasks/verify-pull-request.mts + env: + PR_TITLE: ${{ github.event.pull_request.title }} + PR_BODY: ${{ github.event.pull_request.body }} diff --git a/build/shared/commit-message.mts b/build/shared/commit-message.mts index 65dd3eaf9..bb0f04f1e 100644 --- a/build/shared/commit-message.mts +++ b/build/shared/commit-message.mts @@ -386,3 +386,30 @@ export function validateCommitMessage(message: string) { return problems; } + +/** + * Checks that a human certified the change. Only the person named as author + * can do that: an assistant discloses itself with `Assisted-by` and does not + * sign anything, and a bot certifying on someone's behalf is the thing this + * exists to stop. + * @param {string} message The whole commit message. + * @param {string} author The commit's author, as `Name `. + * @returns {string[]} What is wrong with it, empty if nothing. + */ +export function checkSignOff(message: string, author: string) { + const signed = readTrailers(message) + .filter((line) => /^Signed-off-by:/.test(line)) + .map((line) => line.slice(line.indexOf(':') + 1).trim()); + + if (signed.length === 0) { + return [ + `no “Signed-off-by: ${author}”; the Developer Certificate of Origin is certified by the author, and “Assisted-by:” is what discloses a tool`, + ]; + } + + return signed.includes(author) + ? [] + : [ + `\`Signed-off-by:\` names ${signed.join(', ')}, but the author is ${author}`, + ]; +} diff --git a/build/tasks/verify-pull-request.mts b/build/tasks/verify-pull-request.mts new file mode 100644 index 000000000..b917aafcf --- /dev/null +++ b/build/tasks/verify-pull-request.mts @@ -0,0 +1,50 @@ +/** + * @file Verify a pull request's title and description before it can land. + * @author The OpenINF Authors & Friends + * @license MIT OR Apache-2.0 OR BlueOak-1.0.0 + * @module {type ES6Module} build/tasks/verify/verify-pull-request + */ + +import { validateCommitMessage } from '@openinf/portal/build/commit-message'; + +// Read from the environment rather than interpolated into a command by the +// workflow: a title is written by whoever opened the pull request, and pasting +// one into a shell is how a title comes to run as code. +const title = process.env.PR_TITLE ?? ''; +const body = process.env.PR_BODY ?? ''; + +if (process.env.PR_TITLE === undefined) { + console.error( + 'PR_TITLE is not set. This task reads the pull request from the ' + + 'environment; see .github/workflows/pull-request-policy.yml.' + ); + process.exitCode = 1; +} else { + const problems: string[] = []; + + // The title becomes the subject of the commit that lands, because the + // squash takes it verbatim. So it answers to the same rules, and checking + // it here is the only chance to say so before the subject is history. + for (const problem of validateCommitMessage(title)) { + problems.push(`title: ${problem}`); + } + + // The template is one long HTML comment, so a pull request opened without a + // word written renders as nothing at all. That is what this looks for -- + // not what the description says, only that there is one. + if (body.replaceAll(//g, '').trim() === '') { + problems.push( + 'description: say why the change is needed and what it does. The ' + + 'template is a comment, so a description that is only the template ' + + 'shows a reader nothing.' + ); + } + + if (problems.length > 0) { + console.error(`This pull request is not ready to land:\n`); + for (const problem of problems) console.error(` ${problem}`); + process.exitCode = 1; + } else { + console.log('Title and description are in order.'); + } +} diff --git a/build/tasks/verify/verify-commits.mts b/build/tasks/verify/verify-commits.mts index a18f52b95..5892c676c 100644 --- a/build/tasks/verify/verify-commits.mts +++ b/build/tasks/verify/verify-commits.mts @@ -7,6 +7,7 @@ import { execFileSync } from 'node:child_process'; import { + checkSignOff, readTrailers, validateCommitMessage, } from '@openinf/portal/build/commit-message'; @@ -71,7 +72,10 @@ if (base === '') { } const message = git('log', '-1', '--format=%B', sha); - const problems = validateCommitMessage(message); + const problems = [ + ...validateCommitMessage(message), + ...checkSignOff(message, git('log', '-1', '--format=%an <%ae>', sha)), + ]; // git has the final say on what counts as a trailer, so the rules above // are cross-checked against it rather than trusted on their own. A diff --git a/collections/_docs/handbook/style/commit-messages.md b/collections/_docs/handbook/style/commit-messages.md index a86d50bae..842d343d9 100644 --- a/collections/_docs/handbook/style/commit-messages.md +++ b/collections/_docs/handbook/style/commit-messages.md @@ -109,6 +109,13 @@ throughout rather than about being understood. `PR-URL:` and `Reviewed-by:` are added when the commit lands. The others belong in the commit as you write it. +`Signed-off-by:` is **required**, and has to name the commit's own author — +`git commit -s` writes it for you. It is how you certify the [Developer +Certificate of Origin][]: that you have the right to contribute this change. A +tool cannot do that on your behalf, and neither can a bot, which is why the +check compares the name against the author rather than merely looking for the +line. + ### Disclosing an AI assistant An AI tool that helped write a change is disclosed with `Assisted-by:`, and diff --git a/package-scripts.yml b/package-scripts.yml index 73d79d1ff..c58790260 100644 --- a/package-scripts.yml +++ b/package-scripts.yml @@ -18,6 +18,9 @@ scripts: json: node build/tasks/verify/verify-json.mts liquid: node build/tasks/verify/verify-liquid.mts md: node build/tasks/verify/verify-md.mts + # Outside verify/ on purpose: it needs a pull request in the environment, + # and verify.all runs everything in that directory. + pullRequest: node build/tasks/verify-pull-request.mts svg: node build/tasks/verify/verify-svg.mts toml: node build/tasks/verify/verify-toml.mts ts: node build/tasks/verify/verify-ts.mts