From 3b4db69741ec5bfbfc09781be942888f6aecfd42 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 21 Sep 2026 14:16:07 +0000 Subject: [PATCH] ci: check the pull request title, which a squash merge turns into a commit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A squash merge discards every subject on the branch and uses the pull request title instead, composed by GitHub after all local hooks have run. So the `commit-msg` hook cannot see the subject that actually lands on `main`, and `release-please` parses exactly that subject. #45 is what this costs. Squash-merged as "Make the station selection mean what the config says: radius, and one instrument per station (#45)", the release job recorded: commit could not be parsed: 2af57e8 Make the station selection mean ... error message: Error: unexpected token ' ' at 1:5, valid tokens [(, !, :] Considering: 0 commits No commits for path: ., skipping A radius fix, a WGS84 cut and two new configuration fields went out with no version bump and no changelog entry; both had to be repaired by hand in the release pull request afterwards. Written as a script rather than a third-party action so the accepted types are the ones `release-please-config.json` declares — a test asserts the two lists are identical, since a type this accepts but the release configuration does not know lands in no changelog section, which is the same silent failure reached from the other side. The failure message names one thing to change rather than printing the grammar. The title is attacker-chosen text, so it reaches the script through the environment, never interpolated into a shell command, and the workflow runs on `pull_request` with no permissions rather than `pull_request_target`. Merging with a merge commit remains the safer default and keeps the individual subjects; this makes a squash safe when one is used anyway. Co-Authored-By: Claude Opus 5 --- .github/workflows/pr-title.yml | 30 ++++++++ AGENTS.md | 7 ++ tests/test_pr_title.py | 87 ++++++++++++++++++++++ tools/check_pr_title.py | 131 +++++++++++++++++++++++++++++++++ 4 files changed, 255 insertions(+) create mode 100644 .github/workflows/pr-title.yml create mode 100644 tests/test_pr_title.py create mode 100644 tools/check_pr_title.py diff --git a/.github/workflows/pr-title.yml b/.github/workflows/pr-title.yml new file mode 100644 index 0000000..0ce6404 --- /dev/null +++ b/.github/workflows/pr-title.yml @@ -0,0 +1,30 @@ +name: pr-title + +# A squash merge replaces the branch's commit subjects with this title, so the +# title is what release-please parses for the version bump and the changelog. +# `edited` is in the list because a title is most often fixed after the check +# has already failed once. +on: + pull_request: + types: [opened, edited, reopened] + +# `pull_request`, not `pull_request_target`: this reads a title an outside +# contributor controls, and must never do so with a write token. +permissions: {} + +concurrency: + group: pr-title-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + pr-title: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + - name: The title must be a Conventional Commit + # Through the environment rather than as an argument: the title is + # attacker-chosen text, and `${{ }}` interpolation into a shell command + # would execute it. + env: + PR_TITLE: ${{ github.event.pull_request.title }} + run: python tools/check_pr_title.py diff --git a/AGENTS.md b/AGENTS.md index 58f1ea2..8bfddea 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -41,6 +41,13 @@ to generate `CHANGELOG.md`. See - Say *why*, with the measurement if there was one. The history is the record of what was checked; a message that only restates the diff wastes it. +**The pull request title is one of them.** A squash merge discards every +subject on the branch and uses the title instead, composed by GitHub after all +local hooks have run, so the `commit-msg` hook never sees it. `pr-title.yml` +checks it against the same types `release-please-config.json` declares. Merging +with a merge commit keeps the individual subjects and is the safer default; the +check is what makes a squash safe when one is used anyway. + ## Workflow files need a permission you may not have `.github/workflows/` is editable directly, but only when the session's GitHub diff --git a/tests/test_pr_title.py b/tests/test_pr_title.py new file mode 100644 index 0000000..390e1f2 --- /dev/null +++ b/tests/test_pr_title.py @@ -0,0 +1,87 @@ +"""The pull request title check, which stands in for a hook that cannot exist. + +`release-please` computes the version bump and the changelog from commit +subjects on ``main``. A squash merge composes that subject from the pull +request title, server-side, after every local hook has run — so the +``commit-msg`` hook cannot see it, and the one time it mattered the work +shipped with no changelog entry at all. + +These are the cases that actually occurred or are one keystroke away, not an +exhaustive grammar: the title as #45 carried it, a capitalised type, a missing +space, an empty scope. +""" + +from __future__ import annotations + +import json +import sys +from pathlib import Path + +import pytest + +ROOT = Path(__file__).resolve().parent.parent +sys.path.insert(0, str(ROOT / "tools")) + +from check_pr_title import TYPES, problem_with # noqa: E402 + +VALID = [ + "feat(acquire): rank co-sited instruments", + "fix: ask for waveforms by name", + "docs(roadmap): file v0.4.0 as shipped", + "feat!: make the package installable and importable", + "fix(multitaper)!: default adaptive weighting off", + "chore(main): release 0.5.0", + # What a squash merge appends; the title itself never carries it, but a + # person copying a merged subject back into a title will. + "feat(acquire): rank co-sited instruments (#45)", +] + +INVALID = [ + # The one that happened. + "Make the station selection mean what the config says: radius, and one " + "instrument per station (#45)", + "Roadmap: file v0.4.0 as shipped, and the acquire work as merged", + "Feat(acquire): rank co-sited instruments", + "feat(acquire):no space after the colon", + "feat(): an empty scope", + "feat(acquire): ", + "update the acquire module", + "", + " ", +] + + +@pytest.mark.parametrize("title", VALID) +def test_a_conventional_title_passes(title: str) -> None: + assert problem_with(title) is None, title + + +@pytest.mark.parametrize("title", INVALID) +def test_an_unparseable_title_is_refused(title: str) -> None: + assert problem_with(title) is not None, title + + +@pytest.mark.parametrize("type_", TYPES) +def test_every_known_type_is_accepted(type_: str) -> None: + assert problem_with(f"{type_}: a description") is None + + +def test_the_types_are_the_ones_release_please_knows() -> None: + """Two lists of types that must not drift apart. + + A type this check accepts but `release-please-config.json` does not + declare passes review and then lands in no changelog section — the same + silent outcome the check exists to prevent, arrived at from the other side. + """ + config = json.loads((ROOT / "release-please-config.json").read_text()) + declared = { + section["type"] for section in config["packages"]["."]["changelog-sections"] + } + assert set(TYPES) == declared + + +def test_the_failure_says_what_to_do() -> None: + """The message is the whole interface: it is read on a red check.""" + problem = problem_with("Make the station selection mean what the config says") + assert problem is not None + assert "type" in problem diff --git a/tools/check_pr_title.py b/tools/check_pr_title.py new file mode 100644 index 0000000..f65d66f --- /dev/null +++ b/tools/check_pr_title.py @@ -0,0 +1,131 @@ +#!/usr/bin/env python +"""Check that a pull request title is a Conventional Commit. + +Run as:: + + PR_TITLE='feat(acquire): rank co-sited instruments' python tools/check_pr_title.py + +The title matters because of how a squash merge composes the commit that lands +on ``main``. GitHub replaces the branch's subjects with the pull request title, +server-side, after every local hook has run. `release-please` then parses that +subject to compute the version bump and the changelog section — and a subject +it cannot parse contributes neither. + +That is not hypothetical. #45 was squash-merged as "Make the station selection +mean what the config says: radius, and one instrument per station (#45)", and +the release job recorded: + +.. code-block:: text + + commit could not be parsed: 2af57e8 Make the station selection mean what ... + error message: Error: unexpected token ' ' at 1:5, valid tokens [(, !, :] + Considering: 0 commits + No commits for path: ., skipping + +A radius fix, a WGS84 cut and two new configuration fields went out with no +entry anywhere, and the changelog had to be repaired by hand afterwards. A +local ``commit-msg`` hook cannot prevent this: the squash subject does not +exist until GitHub builds it. + +The title is untrusted input — anyone who can open a pull request chooses it — +so it arrives through the environment rather than as a shell argument, and +nothing here interpolates it into a command. + +Standard library only: this runs before any environment is set up. +""" + +from __future__ import annotations + +import os +import re +import sys + +#: The types `release-please-config.json` declares, and nothing else. Keeping +#: the two lists identical is the point: a type this accepts but the release +#: configuration does not know lands in no changelog section at all. +TYPES = ( + "feat", + "fix", + "perf", + "refactor", + "docs", + "build", + "revert", + "test", + "ci", + "style", + "chore", +) + +#: ``type(scope)!: description``. The scope is optional and, when given, may +#: not be empty. ``!`` marks a breaking change, which bumps the minor while +#: this is ``0.x`` — see `bump-minor-pre-major` in the release configuration. +PATTERN = re.compile( + r"^(?P" + "|".join(TYPES) + r")" + r"(?:\((?P[^()\s][^()]*)\))?" + r"(?P!)?" + r": (?P\S.*)$" +) + +#: What GitHub appends to a squash subject. Present in the commit but not in +#: the title, so it is stripped before matching rather than rejected. +TRAILING_NUMBER = re.compile(r"\s*\(#\d+\)$") + + +def problem_with(title: str) -> str | None: + """Why ``title`` is not a Conventional Commit, or ``None`` if it is.""" + subject = TRAILING_NUMBER.sub("", title.strip()) + if not subject: + return "the title is empty" + if PATTERN.match(subject): + return None + return _diagnosis(subject) + + +def _diagnosis(subject: str) -> str: + """Which part of a subject that did not match to point the author at. + + One message, naming one thing to change. "Does not match + ``^(feat|fix|...)(\\(...\\))?!?: .+$``" is accurate and useless. + """ + if ":" not in subject: + return ( + f"there is no `type: ` prefix. The title begins {subject[:24]!r}, " + f"which release-please reads as a description with no type" + ) + prefix = re.sub(r"[(!].*$", "", subject.split(":", 1)[0]).strip() + if prefix not in TYPES: + if prefix.lower() in TYPES: + return f"the type {prefix!r} must be lower case" + return f"{prefix!r} is not one of the known types" + if re.match(r"^[^:]*\(\s*\)", subject): + return "the scope is empty; write `type: ` or `type(scope): `" + return "there is no space after the colon, or no description after it" + + +def main() -> int: + title = os.environ.get("PR_TITLE") + if title is None: + print("PR_TITLE is not set", file=sys.stderr) + return 2 + + problem = problem_with(title) + if problem is None: + print(f"ok: {title}") + return 0 + + # `::error::` renders the message on the pull request itself, which is + # where the person who has to fix it is looking. + print( + f"::error::The pull request title is not a Conventional Commit: " + f"{problem}. A squash merge makes this title the commit subject on " + f"main, and release-please computes the version and the changelog " + f"from it, so an unparseable title ships the work with no changelog " + f"entry. Known types: {', '.join(TYPES)}. Example: " + f"'fix(acquire): ask for waveforms by name'." + ) + return 1 + + +if __name__ == "__main__": + sys.exit(main())