From b90c27a7efe212526684963e6e910b1e67fb932c Mon Sep 17 00:00:00 2001 From: Derek Lewis Date: Thu, 13 Aug 2026 11:42:48 +0000 Subject: [PATCH 1/2] =?UTF-8?q?=F0=9F=8F=97=EF=B8=8F=E2=9C=A8=EF=BC=9Achec?= =?UTF-8?q?k=20a=20pull=20request's=20title=20and=20description?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The title becomes the subject of the commit that lands, because the squash takes it verbatim, and until now nothing checked it until the commits already existed. It answers to the same rules as a subject now, through the same module, so the two cannot drift. The description has to say something. 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 words say. Both arrive through the environment rather than the command line, which is how nodejs.org does it and for the reason they do it: a title is written by whoever opened the pull request, and one pasted into a shell can run as code. Bots are left to their own formats, as with commits. Signed-off-by: Derek Lewis Assisted-by: Claude-Code:claude-opus-5 --- .github/workflows/pull-request-policy.yml | 42 +++++++++++++++++++ build/shared/commit-message.mts | 27 ++++++++++++ build/tasks/verify-pull-request.mts | 50 +++++++++++++++++++++++ build/tasks/verify/verify-commits.mts | 6 ++- package-scripts.yml | 3 ++ 5 files changed, 127 insertions(+), 1 deletion(-) create mode 100644 .github/workflows/pull-request-policy.yml create mode 100644 build/tasks/verify-pull-request.mts 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/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 From a92a126e2fc82b2d44d67d4f9d65fed7d62e05d2 Mon Sep 17 00:00:00 2001 From: Derek Lewis Date: Thu, 13 Aug 2026 11:43:34 +0000 Subject: [PATCH 2/2] =?UTF-8?q?=F0=9F=93=96=E2=9C=A8=EF=BC=9Awrite=20down?= =?UTF-8?q?=20that=20the=20sign-off=20is=20required?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This repository has been signing off for a long time -- 1055 commits carry the trailer, and 19 of the 40 before this session. The handbook described it as one trailer among several without ever saying it was expected. It says so now, and says why the name has to be the author's: a tool cannot certify the DCO for you, and neither can a bot. Signed-off-by: Derek Lewis Assisted-by: Claude-Code:claude-opus-5 --- collections/_docs/handbook/style/commit-messages.md | 7 +++++++ 1 file changed, 7 insertions(+) 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