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())