From 1153dae75b7634bc630436d1fa57cddf27bdcabf Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 29 Oct 2026 19:11:01 +0400 Subject: [PATCH 01/17] Test what a changelog fragment is before the definition exists Cover the shared definition the PR check and the version bump will both use: #4084's changelog.d/added/4083.md, unknown and uppercase types, other name shapes, empty and whitespace-only files, symbolic links and a valid fragment, with the types read from [tool.towncrier] in pyproject.toml. Check that the fragments the relock and bundle-update pull requests write are accepted, and add the rule proof fixtures: #4084's file and #4106's rename of it. These fail because .github/changelog_fragments.py does not exist yet. Co-Authored-By: Claude Opus 5.5 --- .../bad/changelog.d/added/4083.md | 1 + .../changelog.d/report-live-since.added.md | 1 + tests/unit/changelog_fragment_support.py | 52 +++ tests/unit/test_bundle_update_pins.py | 18 + tests/unit/test_changelog_fragments.py | 425 ++++++++++++++++++ tests/unit/test_relock_pull_request_script.py | 17 + 6 files changed, 514 insertions(+) create mode 100644 rules/fixtures/api.process.changelog-fragment/bad/changelog.d/added/4083.md create mode 100644 rules/fixtures/api.process.changelog-fragment/good/changelog.d/report-live-since.added.md create mode 100644 tests/unit/changelog_fragment_support.py create mode 100644 tests/unit/test_changelog_fragments.py diff --git a/rules/fixtures/api.process.changelog-fragment/bad/changelog.d/added/4083.md b/rules/fixtures/api.process.changelog-fragment/bad/changelog.d/added/4083.md new file mode 100644 index 000000000..18289a059 --- /dev/null +++ b/rules/fixtures/api.process.changelog-fragment/bad/changelog.d/added/4083.md @@ -0,0 +1 @@ +`GET /deployment` reports the policyengine.py version the API serves and when it went live; the candidate deploy keeps that date across redeploys of the same version. diff --git a/rules/fixtures/api.process.changelog-fragment/good/changelog.d/report-live-since.added.md b/rules/fixtures/api.process.changelog-fragment/good/changelog.d/report-live-since.added.md new file mode 100644 index 000000000..18289a059 --- /dev/null +++ b/rules/fixtures/api.process.changelog-fragment/good/changelog.d/report-live-since.added.md @@ -0,0 +1 @@ +`GET /deployment` reports the policyengine.py version the API serves and when it went live; the candidate deploy keeps that date across redeploys of the same version. diff --git a/tests/unit/changelog_fragment_support.py b/tests/unit/changelog_fragment_support.py new file mode 100644 index 000000000..4db012db0 --- /dev/null +++ b/tests/unit/changelog_fragment_support.py @@ -0,0 +1,52 @@ +"""Load .github/changelog_fragments.py and .github/bump_version.py for tests. + +``python .github/bump_version.py`` puts .github/ first on ``sys.path``, so the +bump script imports ``changelog_fragments`` from the file beside it. These +helpers load both files by path and register each under its module name, so +the bump script's import finds the fragment module without a change to +``sys.path``. +""" + +from __future__ import annotations + +import importlib.util +import sys +from pathlib import Path +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from types import ModuleType + +REPO = Path(__file__).resolve().parents[2] +GITHUB_DIRECTORY = REPO / ".github" + + +def load_github_module(module_name: str) -> ModuleType: + """Return the module in .github/.py, loading it on first use.""" + loaded_module = sys.modules.get(module_name) + if loaded_module is not None: + return loaded_module + spec = importlib.util.spec_from_file_location( + module_name, GITHUB_DIRECTORY / f"{module_name}.py" + ) + assert spec is not None + assert spec.loader is not None + module = importlib.util.module_from_spec(spec) + sys.modules[module_name] = module + try: + spec.loader.exec_module(module) + except BaseException: + del sys.modules[module_name] + raise + return module + + +def changelog_fragments() -> ModuleType: + """Return .github/changelog_fragments.py.""" + return load_github_module("changelog_fragments") + + +def bump_version() -> ModuleType: + """Return .github/bump_version.py, after the fragment module it imports.""" + changelog_fragments() + return load_github_module("bump_version") diff --git a/tests/unit/test_bundle_update_pins.py b/tests/unit/test_bundle_update_pins.py index 2ccfb19e6..0f1cfd990 100644 --- a/tests/unit/test_bundle_update_pins.py +++ b/tests/unit/test_bundle_update_pins.py @@ -113,6 +113,24 @@ def test_automatic_update_changes_and_stages_both_image_pins(update_checkout): ] in calls +@pytest.mark.parametrize("target_version", [TARGET_VERSION, "4.1.0rc1", "4.1.0.post1"]) +def test_the_pull_request_adds_a_fragment_the_release_reads( + update_checkout, target_version +): + from tests.unit.changelog_fragment_support import changelog_fragments + + result, _ = run_update( + update_checkout, + LATEST_OVERRIDE=target_version, + BUNDLE_TEST_REPORTED_VERSION=target_version, + ) + + assert result.returncode == 0, result.stderr + fragment = f"changelog.d/update-policyengine-bundle-{target_version}.changed.md" + definition = changelog_fragments().FragmentDefinition.load(REPO) + assert definition.problem_with(fragment, update_checkout / fragment) is None + + @pytest.mark.parametrize("pin", ["3.9.0", "4.0.01", "4.0.0rc1"]) def test_mismatched_image_pin_fails_before_file_changes(update_checkout, pin): image = update_checkout / "docker/Dockerfile" diff --git a/tests/unit/test_changelog_fragments.py b/tests/unit/test_changelog_fragments.py new file mode 100644 index 000000000..f88f8ac01 --- /dev/null +++ b/tests/unit/test_changelog_fragments.py @@ -0,0 +1,425 @@ +"""Tests for .github/changelog_fragments.py: what a changelog fragment is. + +The PR check and the release's version bump both read fragments through +``FragmentDefinition`` (rule api.process.changelog-fragment), so these cases +pin down what both of them accept. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +import pytest + +from tests.unit.changelog_fragment_support import REPO, changelog_fragments + +if TYPE_CHECKING: + from pathlib import Path + +fragments_module = changelog_fragments() + +CONFIGURED_TYPES = ("breaking", "added", "changed", "fixed", "removed") +LISTED_TYPES = "breaking, added, changed, fixed or removed" +RULE_FIXTURES = REPO / "rules/fixtures/api.process.changelog-fragment" +# The fragment #4084 added, and the text #4106 moved to changelog.d/. +PULL_REQUEST_4084_FRAGMENT = "changelog.d/added/4083.md" +FRAGMENT_TEXT = "Report when the deployed version went live.\n" +EMPTY_ENTRY = ( + "Write a sentence or two in it saying what changed: Towncrier would " + "publish an empty entry." +) +FORM_TERMS = ( + f" is {LISTED_TYPES}, and is letters and digits, in groups " + "joined by single dots, hyphens or underscores" +) +GENERIC_RENAME = f"Rename it to changelog.d/..md, where {FORM_TERMS}." + + +def _definition(): + return fragments_module.FragmentDefinition.load(REPO) + + +def _write(root: Path, relative_path: str, text: str) -> Path: + file_path = root / relative_path + file_path.parent.mkdir(parents=True, exist_ok=True) + file_path.write_text(text, encoding="utf-8") + return file_path + + +@pytest.fixture +def repository(tmp_path: Path) -> Path: + """Return a root holding the repository's pyproject.toml and changelog.d/.""" + (tmp_path / "pyproject.toml").write_text( + (REPO / "pyproject.toml").read_text(encoding="utf-8"), encoding="utf-8" + ) + _write(tmp_path, "changelog.d/.gitkeep", "") + return tmp_path + + +def _towncrier_config(directory: str, *type_directories: str) -> str: + type_tables = "".join( + f'\n[[tool.towncrier.type]]\ndirectory = "{type_directory}"\n' + f'name = "{type_directory.title()}"\n' + for type_directory in type_directories + ) + return f'[tool.towncrier]\ndirectory = "{directory}"\n{type_tables}' + + +def test_the_directory_and_types_come_from_tool_towncrier_in_pyproject(): + definition = _definition() + + assert definition.directory == "changelog.d" + assert definition.types == CONFIGURED_TYPES + + +def test_the_types_are_read_from_the_given_pyproject(tmp_path: Path): + (tmp_path / "pyproject.toml").write_text( + _towncrier_config("notes", "added", "security"), encoding="utf-8" + ) + + definition = fragments_module.FragmentDefinition.load(tmp_path) + + assert definition.directory == "notes" + assert definition.types == ("added", "security") + + +@pytest.mark.parametrize( + ("config", "expected_error"), + [ + ('[project]\nname = "x"\n', "Could not read the fragment directory"), + ('[tool.towncrier]\ndirectory = "changelog.d"\n', "Could not read"), + ( + '[tool.towncrier]\ndirectory = "changelog.d"\ntype = []\n', + "must list each fragment type once", + ), + ( + _towncrier_config("changelog.d", "added", "added"), + "must list each fragment type once", + ), + (_towncrier_config("notes/changelog.d", "added"), "is not one folder"), + (_towncrier_config("../changelog.d", "added"), "is not one folder"), + (_towncrier_config("changelog.d", "Added"), 'type "Added"'), + (_towncrier_config("changelog.d", "md"), 'type "md"'), + ], + ids=[ + "no-towncrier-table", + "no-types", + "empty-type-list", + "duplicate-type", + "nested-directory", + "parent-directory", + "uppercase-type", + "extension-as-type", + ], +) +def test_load_refuses_a_configuration_the_release_cannot_follow( + tmp_path: Path, config: str, expected_error: str +): + (tmp_path / "pyproject.toml").write_text(config, encoding="utf-8") + + with pytest.raises(fragments_module.ConfigurationError, match=expected_error): + fragments_module.FragmentDefinition.load(tmp_path) + + +@pytest.mark.parametrize( + ("file_name", "expected_name", "expected_type"), + [ + ("4083.added.md", "4083", "added"), + ("report-live-since.added.md", "report-live-since", "added"), + ("relock-dependencies.changed.md", "relock-dependencies", "changed"), + ( + "update-policyengine-bundle-6.11.1.changed.md", + "update-policyengine-bundle-6.11.1", + "changed", + ), + ("v2_reports.breaking.md", "v2_reports", "breaking"), + ("fixed.removed.md", "fixed", "removed"), + ], +) +def test_a_file_named_name_type_md_directly_in_changelog_d_is_a_fragment( + repository: Path, file_name: str, expected_name: str, expected_type: str +): + _write(repository, f"changelog.d/{file_name}", FRAGMENT_TEXT) + + scan = _definition().scan(repository) + + assert scan.problems == {} + assert scan.fragments == ( + fragments_module.Fragment( + path=f"changelog.d/{file_name}", + name=expected_name, + fragment_type=expected_type, + ), + ) + + +def test_pull_request_4084s_fragment_in_a_type_folder_is_not_a_fragment( + repository: Path, +): + _write(repository, PULL_REQUEST_4084_FRAGMENT, FRAGMENT_TEXT) + + scan = _definition().scan(repository) + + assert scan.fragments == () + assert scan.problems == { + PULL_REQUEST_4084_FRAGMENT: ( + "changelog.d/added/4083.md is in a folder inside changelog.d/. The " + "release reads only files directly in changelog.d/ and stops at " + "anything else there. Move it to changelog.d/4083.added.md." + ) + } + + +@pytest.mark.parametrize( + ("relative_path", "expected_problem"), + [ + ( + "changelog.d/fixed/4083.fixed.md", + "is in a folder inside changelog.d/. The release reads only files " + "directly in changelog.d/ and stops at anything else there. Move it " + "to changelog.d/4083.fixed.md.", + ), + ( + "changelog.d/notes/today.txt", + f"Move it to changelog.d/..md, where {FORM_TERMS}, or " + "delete it.", + ), + ( + "changelog.d/added/.gitkeep", + "changelog.d/added/.gitkeep is in a folder inside changelog.d/.", + ), + ], +) +def test_every_file_in_a_folder_inside_changelog_d_is_a_problem( + repository: Path, relative_path: str, expected_problem: str +): + _write(repository, relative_path, FRAGMENT_TEXT) + + scan = _definition().scan(repository) + + assert scan.fragments == () + assert list(scan.problems) == [relative_path] + assert expected_problem in scan.problems[relative_path] + + +def test_an_empty_folder_inside_changelog_d_is_a_problem(repository: Path): + (repository / "changelog.d/added").mkdir() + + scan = _definition().scan(repository) + + assert scan.problems == { + "changelog.d/added": ( + "changelog.d/added is an empty folder inside changelog.d/. The " + "release reads only files directly in changelog.d/ and stops at " + "anything else there. Delete it." + ) + } + + +@pytest.mark.parametrize( + ("file_name", "expected_problem"), + [ + ( + "4083.feature.md", + 'changelog.d/4083.feature.md has the type "feature", which is not ' + "one of the types in [tool.towncrier] in pyproject.toml: " + f"{LISTED_TYPES}. Rename it to changelog.d/4083..md with one " + "of those types.", + ), + ( + "4083.Added.md", + 'changelog.d/4083.Added.md has the type "Added", but types are ' + "lowercase. Rename it to changelog.d/4083.added.md.", + ), + ( + "4083.added.extra.md", + 'changelog.d/4083.added.extra.md has the type "extra", which is not ' + "one of the types", + ), + ], +) +def test_a_fragment_with_an_unknown_type_is_a_problem( + repository: Path, file_name: str, expected_problem: str +): + _write(repository, f"changelog.d/{file_name}", FRAGMENT_TEXT) + + scan = _definition().scan(repository) + + assert scan.fragments == () + assert expected_problem in scan.problems[f"changelog.d/{file_name}"] + + +@pytest.mark.parametrize( + ("file_name", "expected_fix"), + [ + ("notes.md", GENERIC_RENAME), + ("4083.added", "Rename it to changelog.d/4083.added.md."), + ("4083.added.rst", "Rename it to changelog.d/4083.added.md."), + ("4083.added.MD", "Rename it to changelog.d/4083.added.md."), + ("4083.added.md ", "Rename it to changelog.d/4083.added.md."), + (".added.md", GENERIC_RENAME), + ("added.md", GENERIC_RENAME), + ("fix login.added.md", GENERIC_RENAME), + ("4083..added.md", GENERIC_RENAME), + ("-4083.added.md", GENERIC_RENAME), + ], +) +def test_a_file_not_named_name_type_md_is_a_problem( + repository: Path, file_name: str, expected_fix: str +): + relative_path = f"changelog.d/{file_name}" + _write(repository, relative_path, FRAGMENT_TEXT) + + scan = _definition().scan(repository) + + assert scan.fragments == () + problem = scan.problems[relative_path] + assert problem.startswith( + f"{relative_path} is not named ..md: its file name is " + f"{file_name!r}. The release can't read it." + ) + assert problem.endswith(expected_fix) + + +@pytest.mark.parametrize( + ("text", "expected_problem"), + [ + ("", f"changelog.d/4083.added.md is empty. {EMPTY_ENTRY}"), + ( + " \n\t\n", + f"changelog.d/4083.added.md holds only whitespace. {EMPTY_ENTRY}", + ), + ( + "\n", + f"changelog.d/4083.added.md holds only whitespace. {EMPTY_ENTRY}", + ), + ], + ids=["empty", "whitespace-only", "byte-order-mark-only"], +) +def test_a_fragment_without_text_is_a_problem( + repository: Path, text: str, expected_problem: str +): + _write(repository, "changelog.d/4083.added.md", text) + + scan = _definition().scan(repository) + + assert scan.fragments == () + assert scan.problems == {"changelog.d/4083.added.md": expected_problem} + + +def test_a_fragment_that_is_not_utf8_is_a_problem(repository: Path): + (repository / "changelog.d/4083.added.md").write_bytes(b"caf\xe9\n") + + scan = _definition().scan(repository) + + assert scan.problems == { + "changelog.d/4083.added.md": ( + "changelog.d/4083.added.md is not UTF-8 text. Save it as UTF-8." + ) + } + + +@pytest.mark.parametrize( + "link_target", ["../pyproject.toml", "missing.md"], ids=["file", "dangling"] +) +def test_a_symbolic_link_is_not_a_fragment(repository: Path, link_target: str): + (repository / "changelog.d/4083.added.md").symlink_to(link_target) + + scan = _definition().scan(repository) + + assert scan.fragments == () + assert scan.problems == { + "changelog.d/4083.added.md": ( + "changelog.d/4083.added.md is a symbolic link. A fragment is a " + "regular file that holds its text; replace the link with one." + ) + } + + +def test_a_symbolic_link_to_a_folder_is_not_a_fragment(repository: Path): + (repository / "elsewhere").mkdir() + _write(repository, "elsewhere/4083.added.md", FRAGMENT_TEXT) + (repository / "changelog.d/added").symlink_to("../elsewhere") + + scan = _definition().scan(repository) + + assert scan.fragments == () + assert list(scan.problems) == ["changelog.d/added"] + assert "is a symbolic link" in scan.problems["changelog.d/added"] + + +def test_gitkeep_is_the_only_other_file_allowed_and_must_be_a_regular_file( + repository: Path, +): + _write(repository, "changelog.d/4083.added.md", FRAGMENT_TEXT) + + assert _definition().scan(repository).problems == {} + + (repository / "changelog.d/.gitkeep").unlink() + (repository / "changelog.d/.gitkeep").symlink_to("4083.added.md") + + assert list(_definition().scan(repository).problems) == ["changelog.d/.gitkeep"] + + +def test_a_missing_fragment_directory_holds_no_fragments(tmp_path: Path): + scan = _definition().scan(tmp_path) + + assert scan.fragments == () + assert scan.problems == {} + + +def test_a_fragment_directory_that_is_a_file_is_a_problem(tmp_path: Path): + (tmp_path / "changelog.d").write_text(FRAGMENT_TEXT, encoding="utf-8") + + scan = _definition().scan(tmp_path) + + assert scan.fragments == () + assert scan.problems == { + "changelog.d": ( + "changelog.d is not a folder. The release reads fragments from the " + "folder changelog.d/; make it one." + ) + } + + +def test_scan_lists_fragments_and_problems_in_path_order(repository: Path): + _write(repository, "changelog.d/b.fixed.md", FRAGMENT_TEXT) + _write(repository, "changelog.d/a.added.md", FRAGMENT_TEXT) + _write(repository, PULL_REQUEST_4084_FRAGMENT, FRAGMENT_TEXT) + _write(repository, "changelog.d/z.feature.md", FRAGMENT_TEXT) + + scan = _definition().scan(repository) + + assert [fragment.path for fragment in scan.fragments] == [ + "changelog.d/a.added.md", + "changelog.d/b.fixed.md", + ] + assert list(scan.problems) == [ + PULL_REQUEST_4084_FRAGMENT, + "changelog.d/z.feature.md", + ] + + +def test_the_accepted_form_names_the_directory_types_and_documentation(): + assert _definition().accepted_form() == ( + "Add a file changelog.d/..md, directly in changelog.d/, " + "that says in a sentence or two what changed for people who use or " + f"run the Public API. {FORM_TERMS}, such as an issue number. See " + "docs/engineering/skills/github-prs.md#changelog-fragments (rule " + "api.process.changelog-fragment)." + ) + + +def test_rule_proof_changelog_fragment(): + """The definition rejects #4084's fragment and accepts #4106's rename of it.""" + definition = _definition() + + bad_scan = definition.scan(RULE_FIXTURES / "bad") + good_scan = definition.scan(RULE_FIXTURES / "good") + + assert bad_scan.fragments == () + assert list(bad_scan.problems) == [PULL_REQUEST_4084_FRAGMENT] + assert good_scan.problems == {} + assert [fragment.path for fragment in good_scan.fragments] == [ + "changelog.d/report-live-since.added.md" + ] diff --git a/tests/unit/test_relock_pull_request_script.py b/tests/unit/test_relock_pull_request_script.py index dc6241d20..3d7b49c10 100644 --- a/tests/unit/test_relock_pull_request_script.py +++ b/tests/unit/test_relock_pull_request_script.py @@ -154,6 +154,23 @@ def test_opens_a_pull_request_naming_a_new_tracking_issue(checkout: Path) -> Non assert not _calls_to(calls, "pr", "edit") +def test_the_pull_request_adds_a_fragment_the_release_reads(checkout: Path) -> None: + from tests.unit.changelog_fragment_support import changelog_fragments + + result, _ = _open_pull_request(checkout) + + assert result.returncode == 0, result.stderr + pushed = _remote_branch(checkout).split()[0] + added_paths = _git( + checkout, "diff", "--name-only", "--diff-filter=A", "origin/master", pushed + ).splitlines() + assert added_paths == [FRAGMENT] + pushed_fragment = checkout.parent / "pushed-fragment.md" + pushed_fragment.write_text(_git(checkout, "show", f"{pushed}:{FRAGMENT}") + "\n") + definition = changelog_fragments().FragmentDefinition.load(REPO) + assert definition.problem_with(FRAGMENT, pushed_fragment) is None + + def test_reuses_the_open_tracking_issue(checkout: Path) -> None: result, calls = _open_pull_request(checkout, RELOCK_TEST_OPEN_ISSUE="41") From 6c5189cab7589aabdd1dc982d5fb93bc302b0be5 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 29 Oct 2026 19:13:09 +0400 Subject: [PATCH 02/17] Define a changelog fragment once, from [tool.towncrier] Add .github/changelog_fragments.py, which the PR check and the version bump will both read fragments through. It reads the fragment directory and types from [tool.towncrier] in pyproject.toml. A fragment is a regular file directly in changelog.d/ named ..md with a configured type and text that isn't empty or only whitespace; .gitkeep is the only other file allowed. Every other file, including files in folders such as #4084's changelog.d/added/4083.md, gets a message that says what is wrong and how to fix it. Co-Authored-By: Claude Opus 5.5 --- .github/changelog_fragments.py | 365 +++++++++++++++++++++++++++++++++ 1 file changed, 365 insertions(+) create mode 100644 .github/changelog_fragments.py diff --git a/.github/changelog_fragments.py b/.github/changelog_fragments.py new file mode 100644 index 000000000..dbce66b7d --- /dev/null +++ b/.github/changelog_fragments.py @@ -0,0 +1,365 @@ +"""What a changelog fragment is, for the PR check and the release's version bump. + +Rule api.process.changelog-fragment. Every pull request adds a changelog +fragment. After a merge to master, the versioning job in +.github/workflows/push.yml runs .github/bump_version.py, which raises the +version in pyproject.toml from the pending fragments' types, and Towncrier then +builds the fragments into CHANGELOG.md. The PR check +(.github/scripts/check_changelog_fragment.sh) and the version bump both read +fragments through ``FragmentDefinition``, so the check accepts only what the +release reads. + +A fragment is a regular file directly in the fragment directory (``directory`` +in ``[tool.towncrier]`` in pyproject.toml: changelog.d) named +``..md``, where: + +- ```` is letters and digits, in groups joined by single dots, hyphens or + underscores, such as ``4083`` or ``update-policyengine-bundle-6.11.1``; +- ```` is the ``directory`` of one of the ``[[tool.towncrier.type]]`` + entries: breaking, added, changed, fixed or removed; +- its text, read as UTF-8, is not empty or only whitespace. + +``.gitkeep``, which keeps the directory in git once a release has removed every +fragment, is the only other file allowed there. Pull request #4084 added +changelog.d/added/4083.md: the old check counted every file under changelog.d/, +so it passed, and the release, which reads only files directly in changelog.d/, +found no fragment. +""" + +from __future__ import annotations + +import re +import tomllib +from dataclasses import dataclass, field +from pathlib import Path, PurePosixPath + +RULE_ID = "api.process.changelog-fragment" +CONFIG_FILE = "pyproject.toml" +CONFIG_TABLE = "[tool.towncrier] in pyproject.toml" +DOCUMENTATION = "docs/engineering/skills/github-prs.md#changelog-fragments" +# Keeps the fragment directory in git once a release has removed every fragment. +KEEP_FILE = ".gitkeep" +FRAGMENT_EXTENSION = "md" +NAME_PATTERN = r"[A-Za-z0-9]+(?:[._-][A-Za-z0-9]+)*" +FRAGMENT_NAME = re.compile(NAME_PATTERN) +FRAGMENT_FILE_NAME = re.compile( + rf"(?P{NAME_PATTERN})\.(?P[^.]+)\.{FRAGMENT_EXTENSION}" +) +TYPE_NAME = re.compile(r"[a-z]+") +BYTE_ORDER_MARK = "" +NAME_TERMS = ( + " is letters and digits, in groups joined by single dots, hyphens or " + "underscores" +) + + +class ConfigurationError(Exception): + """[tool.towncrier] doesn't say where fragments live or which types they have.""" + + +@dataclass(frozen=True) +class Fragment: + """A changelog fragment the release reads: its path, name and type.""" + + path: str + name: str + fragment_type: str + + +@dataclass(frozen=True) +class Scan: + """What the fragment directory holds: its fragments, and every other file. + + ``problems`` maps each other path, from the repository root, to why the + release can't read it. + """ + + fragments: tuple[Fragment, ...] + problems: dict[str, str] = field(default_factory=dict) + + +def _listed(names: tuple[str, ...]) -> str: + """Return ``names`` as "a, b or c".""" + if len(names) == 1: + return names[0] + return f"{', '.join(names[:-1])} or {names[-1]}" + + +def _nested_entries(folder: Path) -> list[Path]: + """Return every file and symbolic link under ``folder``, and each empty folder. + + Symbolic links to folders are returned, not followed. + """ + children = sorted(folder.iterdir()) + if not children: + return [folder] + entries: list[Path] = [] + for child in children: + if child.is_dir() and not child.is_symlink(): + entries.extend(_nested_entries(child)) + else: + entries.append(child) + return entries + + +def _file_kind_problem(path: str, file: Path) -> str | None: + """Return why ``file``, at ``path``, is not a regular file, or None.""" + if file.is_symlink(): + return ( + f"{path} is a symbolic link. A fragment is a regular file that holds " + "its text; replace the link with one." + ) + if not file.is_file(): + return ( + f"{path} is not a regular file. A fragment is a regular file that " + "holds its text; replace it with one." + ) + return None + + +def _content_problem(path: str, file: Path) -> str | None: + """Return why ``file``, at ``path``, holds no fragment text, or None.""" + kind_problem = _file_kind_problem(path, file) + if kind_problem: + return kind_problem + try: + text = file.read_text(encoding="utf-8") + except UnicodeDecodeError: + return f"{path} is not UTF-8 text. Save it as UTF-8." + if text.removeprefix(BYTE_ORDER_MARK).strip(): + return None + emptiness = "is empty" if not text else "holds only whitespace" + return ( + f"{path} {emptiness}. Write a sentence or two in it saying what changed: " + "Towncrier would publish an empty entry." + ) + + +@dataclass(frozen=True) +class FragmentDefinition: + """Where fragments live and which types they may have, from [tool.towncrier].""" + + directory: str + types: tuple[str, ...] + + @classmethod + def load(cls, root: Path) -> FragmentDefinition: + """Read the fragment directory and types from ``root``'s pyproject.toml.""" + config_path = root / CONFIG_FILE + try: + config = tomllib.loads(config_path.read_text(encoding="utf-8")) + towncrier = config["tool"]["towncrier"] + directory = towncrier["directory"] + types = tuple(entry["directory"] for entry in towncrier["type"]) + except (OSError, tomllib.TOMLDecodeError, KeyError, TypeError) as error: + msg = ( + f"Could not read the fragment directory and types from " + f"{CONFIG_TABLE} ({config_path}): {error!r}" + ) + raise ConfigurationError(msg) from error + if not isinstance(directory, str) or not FRAGMENT_NAME.fullmatch(directory): + msg = ( + f"{CONFIG_TABLE} names a fragment directory that is not one " + f"folder: {directory!r}" + ) + raise ConfigurationError(msg) + if not types or len(set(types)) != len(types): + msg = f"{CONFIG_TABLE} must list each fragment type once: {types!r}" + raise ConfigurationError(msg) + for fragment_type in types: + if ( + not isinstance(fragment_type, str) + or not TYPE_NAME.fullmatch(fragment_type) + or fragment_type == FRAGMENT_EXTENSION + ): + msg = ( + f'{CONFIG_TABLE} lists the fragment type "{fragment_type}"; a ' + f"type is lowercase letters, and not " + f'"{FRAGMENT_EXTENSION}", the fragment extension.' + ) + raise ConfigurationError(msg) + return cls(directory=directory, types=types) + + def listed_types(self) -> str: + """Return the fragment types as "a, b or c".""" + return _listed(self.types) + + def form_terms(self) -> str: + """Return what ```` and ```` stand for in a fragment name.""" + return f" is {self.listed_types()}, and {NAME_TERMS}" + + def accepted_form(self) -> str: + """Return the sentences that say how to add a fragment.""" + return ( + f"Add a file {self.directory}/..md, directly in " + f"{self.directory}/, that says in a sentence or two what changed for " + f"people who use or run the Public API. {self.form_terms()}, such as " + f"an issue number. See {DOCUMENTATION} (rule {RULE_ID})." + ) + + def _stops_the_release(self) -> str: + return ( + f"The release reads only files directly in {self.directory}/ and " + "stops at anything else there." + ) + + def _suggested_name(self, file_name: str) -> str | None: + """Return the fragment name ``file_name`` was probably meant to have. + + It takes the last dot-separated part that is a type, ignoring case, as + Towncrier reads a type, and the parts before it as the name. + """ + parts = file_name.strip().split(".") + for index in range(len(parts) - 1, 0, -1): + candidate_type = parts[index].strip().lower() + if candidate_type in self.types: + name = ".".join(parts[:index]) + if FRAGMENT_NAME.fullmatch(name): + return f"{name}.{candidate_type}.{FRAGMENT_EXTENSION}" + return None + return None + + def _moved_path(self, path: PurePosixPath) -> str | None: + """Return where a fragment filed in a folder belongs, if that is clear. + + ``changelog.d/added/4083.md`` belongs at ``changelog.d/4083.added.md``. + """ + match = FRAGMENT_FILE_NAME.fullmatch(path.name) + if match and match.group("type") in self.types: + return f"{self.directory}/{path.name}" + folders = path.relative_to(self.directory).parts[:-1] + name = path.name.removesuffix(f".{FRAGMENT_EXTENSION}") + if ( + len(folders) == 1 + and folders[0] in self.types + and FRAGMENT_NAME.fullmatch(name) + ): + return f"{self.directory}/{name}.{folders[0]}.{FRAGMENT_EXTENSION}" + return None + + def _nested_problem(self, path: PurePosixPath) -> str: + """Return why a file in a folder inside the fragment directory isn't read.""" + problem = ( + f"{path} is in a folder inside {self.directory}/. " + f"{self._stops_the_release()}" + ) + moved_path = self._moved_path(path) + if moved_path: + return f"{problem} Move it to {moved_path}." + return ( + f"{problem} Move it to {self.directory}/..md, where " + f"{self.form_terms()}, or delete it." + ) + + def _name_problem(self, path: str) -> str | None: + """Return why ``path`` is not where and how a fragment is named, or None.""" + location = PurePosixPath(path) + if not location.is_relative_to(self.directory) or location == PurePosixPath( + self.directory + ): + return f"{path} is not in {self.directory}/." + if location.parent != PurePosixPath(self.directory): + return self._nested_problem(location) + match = FRAGMENT_FILE_NAME.fullmatch(location.name) + if match is None: + suggested_name = self._suggested_name(location.name) + fix = ( + f"Rename it to {self.directory}/{suggested_name}." + if suggested_name + else ( + f"Rename it to {self.directory}/..md, where " + f"{self.form_terms()}." + ) + ) + return ( + f"{path} is not named ..md: its file name is " + f"{location.name!r}. The release can't read it. {fix}" + ) + fragment_type = match.group("type") + if fragment_type in self.types: + return None + if fragment_type.lower() in self.types: + return ( + f'{path} has the type "{fragment_type}", but types are lowercase. ' + f"Rename it to {self.directory}/{match.group('name')}." + f"{fragment_type.lower()}.{FRAGMENT_EXTENSION}." + ) + return ( + f'{path} has the type "{fragment_type}", which is not one of the types ' + f"in {CONFIG_TABLE}: {self.listed_types()}. Rename it to " + f"{self.directory}/{match.group('name')}..md with one of those " + "types." + ) + + def problem_with(self, path: str, file: Path) -> str | None: + """Return why ``file``, at ``path`` from the repository root, isn't a fragment. + + Return None when it is one. + """ + return ( + _file_kind_problem(path, file) + or self._name_problem(path) + or _content_problem(path, file) + ) + + def _fragment(self, path: str) -> Fragment: + """Return the fragment at ``path``, a name ``problem_with`` accepted.""" + match = FRAGMENT_FILE_NAME.fullmatch(PurePosixPath(path).name) + if match is None: + msg = f"{path} is not named ..md" + raise ValueError(msg) + return Fragment( + path=path, name=match.group("name"), fragment_type=match.group("type") + ) + + def _folder_problems(self, root: Path, folder: Path) -> dict[str, str]: + """Return a problem for each file, link and empty folder under ``folder``.""" + problems = {} + for entry in _nested_entries(folder): + path = entry.relative_to(root).as_posix() + if entry.is_dir() and not entry.is_symlink(): + problems[path] = ( + f"{path} is an empty folder inside {self.directory}/. " + f"{self._stops_the_release()} Delete it." + ) + else: + problems[path] = self._nested_problem(PurePosixPath(path)) + return problems + + def scan(self, root: Path) -> Scan: + """Return the fragments in ``root``'s fragment directory, and every other file. + + Every file, symbolic link and empty folder in a folder inside the + directory is a problem: Towncrier stops at a folder there. + """ + directory = root / self.directory + if not directory.exists() and not directory.is_symlink(): + return Scan(fragments=()) + if directory.is_symlink() or not directory.is_dir(): + return Scan( + fragments=(), + problems={ + self.directory: ( + f"{self.directory} is not a folder. The release reads " + f"fragments from the folder {self.directory}/; make it one." + ) + }, + ) + fragments: list[Fragment] = [] + problems: dict[str, str] = {} + for entry in sorted(directory.iterdir()): + path = f"{self.directory}/{entry.name}" + if entry.is_dir() and not entry.is_symlink(): + problems.update(self._folder_problems(root, entry)) + continue + if entry.name == KEEP_FILE: + kind_problem = _file_kind_problem(path, entry) + if kind_problem: + problems[path] = kind_problem + continue + problem = self.problem_with(path, entry) + if problem is None: + fragments.append(self._fragment(path)) + else: + problems[path] = problem + return Scan(fragments=tuple(fragments), problems=problems) From 3567c22f1a94e5b0e044ea7d7e3471ea45427c6f Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 29 Oct 2026 19:15:04 +0400 Subject: [PATCH 03/17] Test that the PR check needs the pull request's own readable fragment Run .github/scripts/check_changelog_fragment.sh in temporary repositories whose master holds a pending fragment. A pull request that adds no fragment, or only edits, deletes or renames a pending one, must fail; so must #4084's changelog.d/added/4083.md, other unreadable files, and an unreadable file already on master. Moving a file the release couldn't read into place counts as adding a fragment. On GitHub's test merge commit, only the pull request's own changes count, and a shallow clone or an unknown base fails with what to fix. The job must fetch the history and pass the event's base commit. These fail because the check still counts every file under changelog.d/. Co-Authored-By: Claude Opus 5.5 --- tests/unit/test_changelog_fragment_check.py | 489 ++++++++++++++++++++ 1 file changed, 489 insertions(+) create mode 100644 tests/unit/test_changelog_fragment_check.py diff --git a/tests/unit/test_changelog_fragment_check.py b/tests/unit/test_changelog_fragment_check.py new file mode 100644 index 000000000..cccb297fc --- /dev/null +++ b/tests/unit/test_changelog_fragment_check.py @@ -0,0 +1,489 @@ +"""Tests for the changelog fragment PR check (rule api.process.changelog-fragment). + +``.github/scripts/check_changelog_fragment.sh`` compares the checked-out +commit with the pull request's base commit (``BASE_SHA``) and passes the +changes in changelog.d/ to ``.github/changelog_fragments.py``. Each test builds +a temporary git repository with a master branch, holding one pending fragment +as master does between releases, and a pull request branch. +""" + +from __future__ import annotations + +import os +import subprocess +import sys +from typing import TYPE_CHECKING + +import pytest +import yaml + +from tests.unit.changelog_fragment_support import REPO, changelog_fragments + +if TYPE_CHECKING: + from pathlib import Path + +fragments_module = changelog_fragments() + +CHECK_SCRIPT = REPO / ".github/scripts/check_changelog_fragment.sh" +PR_WORKFLOW = REPO / ".github/workflows/pr.yml" +PENDING_FRAGMENT = "changelog.d/telemetry-rank-cause-copies.fixed.md" +OWN_FRAGMENT = "changelog.d/4114.fixed.md" +PULL_REQUEST_4084_FRAGMENT = "changelog.d/added/4083.md" +FRAGMENT_TEXT = "Report when the deployed version went live.\n" +NO_OWN_FRAGMENT = "::error::This pull request adds no changelog fragment of its own." +ON_THE_BASE_BRANCH = ( + "It is already on the base branch, so the next release fails until a pull " + "request fixes it." +) + + +def _git_environment() -> dict[str, str]: + """Return an environment that ignores the user's and system git settings.""" + return { + **os.environ, + "GIT_CONFIG_GLOBAL": os.devnull, + "GIT_CONFIG_NOSYSTEM": "1", + "GIT_AUTHOR_NAME": "Test", + "GIT_AUTHOR_EMAIL": "test@example.com", + "GIT_COMMITTER_NAME": "Test", + "GIT_COMMITTER_EMAIL": "test@example.com", + } + + +def _git(directory: Path, *arguments: str) -> str: + return subprocess.run( + ["git", *arguments], + cwd=directory, + env=_git_environment(), + text=True, + capture_output=True, + check=True, + ).stdout.strip() + + +def _write(repository: Path, relative_path: str, text: str = FRAGMENT_TEXT) -> None: + file_path = repository / relative_path + file_path.parent.mkdir(parents=True, exist_ok=True) + file_path.write_text(text, encoding="utf-8") + + +def _commit(repository: Path, message: str) -> str: + _git(repository, "add", "--all") + _git(repository, "commit", "--quiet", "--allow-empty", "-m", message) + return _git(repository, "rev-parse", "HEAD") + + +@pytest.fixture +def repository(tmp_path: Path) -> Path: + """Return a repository on a pull request branch cut from master. + + Master holds the repository's pyproject.toml, changelog.d/.gitkeep and one + pending fragment from an earlier pull request. + """ + repository = tmp_path / "repository" + repository.mkdir() + _git(repository, "init", "--quiet", "--initial-branch=master") + (repository / "pyproject.toml").write_text( + (REPO / "pyproject.toml").read_text(encoding="utf-8"), encoding="utf-8" + ) + _write(repository, "changelog.d/.gitkeep", "") + _write(repository, PENDING_FRAGMENT, "Copy each telemetry rank cause.\n") + _write(repository, "README.md", "The Public API.\n") + _commit(repository, "Master with a pending fragment") + _git(repository, "checkout", "--quiet", "-b", "pull-request") + return repository + + +def _master(repository: Path) -> str: + return _git(repository, "rev-parse", "master") + + +def _run_check( + repository: Path, base_revision: str | None +) -> subprocess.CompletedProcess[str]: + environment = {**_git_environment(), "PYTHON": sys.executable} + environment.pop("BASE_SHA", None) + if base_revision is not None: + environment["BASE_SHA"] = base_revision + return subprocess.run( + ["bash", str(CHECK_SCRIPT)], + cwd=repository, + env=environment, + text=True, + capture_output=True, + check=False, + ) + + +def _merge_into_master(repository: Path) -> str: + """Check out a merge of the pull request into master, as GitHub's checkout does. + + GitHub checks out refs/pull//merge, a merge commit whose first parent + is the base commit and whose second is the pull request's head. + """ + _git(repository, "checkout", "--quiet", "--detach", "master") + _git( + repository, + "merge", + "--quiet", + "--no-ff", + "-m", + "Merge pull-request into master", + "pull-request", + ) + return _git(repository, "rev-parse", "HEAD") + + +def test_a_pull_request_with_no_fragment_fails_while_master_has_a_pending_one( + repository: Path, +): + _write(repository, "README.md", "The Public API, changed.\n") + _commit(repository, "Change the README") + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 1, result.stdout + assert NO_OWN_FRAGMENT in result.stdout + assert ( + "Fragments already on the base branch belong to the pull requests that " + f"added them: {PENDING_FRAGMENT}." in result.stdout + ) + assert "Add a file changelog.d/..md" in result.stdout + + +def test_a_pull_request_that_only_edits_a_pending_fragment_fails(repository: Path): + _write(repository, PENDING_FRAGMENT, "Copy each telemetry rank cause, edited.\n") + _commit(repository, "Edit the pending fragment") + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 1, result.stdout + assert NO_OWN_FRAGMENT in result.stdout + + +def test_a_pull_request_that_only_deletes_a_pending_fragment_fails(repository: Path): + (repository / PENDING_FRAGMENT).unlink() + _commit(repository, "Delete the pending fragment") + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 1, result.stdout + assert NO_OWN_FRAGMENT in result.stdout + + +def test_renaming_a_pending_fragment_does_not_add_one(repository: Path): + _git(repository, "mv", PENDING_FRAGMENT, "changelog.d/renamed.fixed.md") + _commit(repository, "Rename the pending fragment") + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 1, result.stdout + assert NO_OWN_FRAGMENT in result.stdout + assert ( + f"Renaming a fragment that is already on the base branch " + f"({PENDING_FRAGMENT} to changelog.d/renamed.fixed.md) doesn't add one." + in result.stdout + ) + + +def test_a_pull_request_that_adds_a_fragment_passes(repository: Path): + _write(repository, OWN_FRAGMENT) + _commit(repository, "Add a fragment") + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 0, result.stdout + assert result.stdout == ( + f"This pull request adds 1 changelog fragment: {OWN_FRAGMENT} (fixed).\n" + ) + + +def test_a_pull_request_that_adds_a_fragment_and_edits_a_pending_one_passes( + repository: Path, +): + _write(repository, OWN_FRAGMENT) + _write(repository, PENDING_FRAGMENT, "Copy each telemetry rank cause, edited.\n") + _commit(repository, "Add a fragment and edit the pending one") + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 0, result.stdout + + +def test_moving_a_file_the_release_could_not_read_into_place_adds_a_fragment( + repository: Path, +): + _git(repository, "checkout", "--quiet", "master") + _write(repository, PULL_REQUEST_4084_FRAGMENT) + _commit(repository, "Master with #4084's fragment") + _git(repository, "checkout", "--quiet", "-B", "pull-request") + _git( + repository, + "mv", + PULL_REQUEST_4084_FRAGMENT, + "changelog.d/report-live-since.added.md", + ) + _commit(repository, "Move #4084's fragment to the name the release reads") + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 0, result.stdout + assert "changelog.d/report-live-since.added.md (added)" in result.stdout + + +def test_pull_request_4084s_fragment_fails_and_says_where_it_belongs( + repository: Path, +): + _write(repository, PULL_REQUEST_4084_FRAGMENT) + _commit(repository, "Add a fragment in a folder") + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 1, result.stdout + assert ( + f"::error file={PULL_REQUEST_4084_FRAGMENT}::{PULL_REQUEST_4084_FRAGMENT} " + "is in a folder inside changelog.d/. The release reads only files " + "directly in changelog.d/ and stops at anything else there. Move it to " + "changelog.d/4083.added.md.\n" in result.stdout + ) + assert NO_OWN_FRAGMENT in result.stdout + + +@pytest.mark.parametrize( + ("relative_path", "text", "expected_problem"), + [ + ("changelog.d/4114.feature.md", FRAGMENT_TEXT, 'has the type "feature"'), + ("changelog.d/4114.md", FRAGMENT_TEXT, "is not named ..md"), + ("changelog.d/4114.fixed.md", "", "is empty."), + ("changelog.d/4114.fixed.md", " \n\n", "holds only whitespace."), + ], + ids=["unknown-type", "no-type", "empty", "whitespace-only"], +) +def test_a_pull_request_whose_only_fragment_is_unreadable_fails( + repository: Path, relative_path: str, text: str, expected_problem: str +): + _write(repository, relative_path, text) + _commit(repository, "Add an unreadable fragment") + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 1, result.stdout + assert f"::error file={relative_path}::{relative_path} " in result.stdout + assert expected_problem in result.stdout + assert NO_OWN_FRAGMENT in result.stdout + + +def test_an_unreadable_file_beside_a_valid_fragment_fails(repository: Path): + _write(repository, OWN_FRAGMENT) + _write(repository, "changelog.d/notes.md", "Notes.\n") + _commit(repository, "Add a fragment and a notes file") + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 1, result.stdout + assert "::error file=changelog.d/notes.md::" in result.stdout + assert NO_OWN_FRAGMENT not in result.stdout + + +def test_emptying_a_pending_fragment_fails_even_with_a_fragment_of_its_own( + repository: Path, +): + _write(repository, OWN_FRAGMENT) + _write(repository, PENDING_FRAGMENT, "") + _commit(repository, "Add a fragment and empty the pending one") + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 1, result.stdout + assert f"::error file={PENDING_FRAGMENT}::{PENDING_FRAGMENT} is empty." in ( + result.stdout + ) + assert ON_THE_BASE_BRANCH not in result.stdout + + +def test_an_unreadable_file_already_on_master_fails_and_says_so(repository: Path): + _git(repository, "checkout", "--quiet", "master") + _write(repository, PULL_REQUEST_4084_FRAGMENT) + _commit(repository, "Master with #4084's fragment") + _git(repository, "checkout", "--quiet", "-B", "pull-request") + _write(repository, OWN_FRAGMENT) + _commit(repository, "Add a fragment") + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 1, result.stdout + assert f"Move it to changelog.d/4083.added.md. {ON_THE_BASE_BRANCH}" in ( + result.stdout + ) + + +def test_on_the_merge_commit_only_the_pull_requests_own_changes_count( + repository: Path, +): + _write(repository, "README.md", "The Public API, changed.\n") + _commit(repository, "Change the README") + _git(repository, "checkout", "--quiet", "master") + _write(repository, "changelog.d/4110.added.md") + _commit(repository, "Merge another pull request, with its fragment") + _merge_into_master(repository) + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 1, result.stdout + assert NO_OWN_FRAGMENT in result.stdout + assert "changelog.d/4110.added.md" in result.stdout + + +def test_on_the_merge_commit_the_pull_requests_fragment_counts(repository: Path): + _write(repository, OWN_FRAGMENT) + _commit(repository, "Add a fragment") + _git(repository, "checkout", "--quiet", "master") + _write(repository, "changelog.d/4110.added.md") + _commit(repository, "Merge another pull request, with its fragment") + merge_commit = _merge_into_master(repository) + + result = _run_check(repository, _master(repository)) + + assert _git(repository, "rev-parse", "HEAD^1") == _master(repository) + assert _git(repository, "rev-parse", "HEAD") == merge_commit + assert result.returncode == 0, result.stdout + assert result.stdout == ( + f"This pull request adds 1 changelog fragment: {OWN_FRAGMENT} (fixed).\n" + ) + + +def test_a_pull_request_stacked_on_another_needs_its_own_fragment( + repository: Path, +): + _write(repository, OWN_FRAGMENT) + lower_head = _commit(repository, "The lower pull request, with its fragment") + _git(repository, "checkout", "--quiet", "-b", "upper-pull-request") + _write(repository, "README.md", "The Public API, changed.\n") + _commit(repository, "The upper pull request, without a fragment") + + result = _run_check(repository, lower_head) + + assert result.returncode == 1, result.stdout + assert NO_OWN_FRAGMENT in result.stdout + + +def test_a_shallow_clone_fails_and_says_to_fetch_the_history( + repository: Path, tmp_path: Path +): + _write(repository, OWN_FRAGMENT) + _commit(repository, "Add a fragment") + _merge_into_master(repository) + shallow_clone = tmp_path / "shallow" + _git( + tmp_path, + "clone", + "--quiet", + "--depth=1", + f"file://{repository}", + str(shallow_clone), + ) + + result = _run_check(shallow_clone, _master(repository)) + + assert result.returncode == 1, result.stdout + assert result.stdout.startswith("::error::This clone is shallow") + assert "fetch-depth: 0" in result.stdout + + +def test_a_base_commit_the_clone_lacks_fails(repository: Path): + _write(repository, OWN_FRAGMENT) + _commit(repository, "Add a fragment") + + result = _run_check(repository, "0" * 40) + + assert result.returncode == 1, result.stdout + assert f"::error::Could not find a common ancestor of {'0' * 40} and HEAD" in ( + result.stdout + ) + + +def test_the_base_commit_is_required(repository: Path): + result = _run_check(repository, None) + + assert result.returncode != 0 + assert "BASE_SHA is required" in result.stderr + + +@pytest.mark.parametrize( + ("name_status", "expected_added_paths"), + [ + (b"A\0changelog.d/a.added.md\0", {"changelog.d/a.added.md"}), + (b"M\0changelog.d/a.added.md\0D\0changelog.d/b.fixed.md\0", set()), + (b"R100\0changelog.d/a.added.md\0changelog.d/b.added.md\0", set()), + ( + b"R087\0changelog.d/added/4083.md\0changelog.d/4083.added.md\0", + {"changelog.d/4083.added.md"}, + ), + ( + b"C100\0changelog.d/a.added.md\0changelog.d/b.added.md\0", + {"changelog.d/b.added.md"}, + ), + (b"T\0changelog.d/a.added.md\0", set()), + ], + ids=[ + "added", + "modified-and-deleted", + "renamed-fragment", + "renamed-unreadable-file", + "copied", + "type-changed", + ], +) +def test_the_paths_a_pull_request_adds( + name_status: bytes, expected_added_paths: set[str] +): + definition = fragments_module.FragmentDefinition.load(REPO) + changes = fragments_module.parse_name_status(name_status) + + assert definition.added_paths(changes) == expected_added_paths + + +def test_name_status_output_that_ends_mid_record_is_refused(): + with pytest.raises(ValueError, match="ends in the middle of a change"): + fragments_module.parse_name_status(b"R100\0changelog.d/a.added.md\0") + + +def test_annotation_properties_escape_the_characters_the_runner_reads(): + assert fragments_module.annotation_property("changelog.d/a,b:c%.md") == ( + "changelog.d/a%2Cb%3Ac%25.md" + ) + assert fragments_module.annotation_message("line one\nline two 100%") == ( + "line one%0Aline two 100%25" + ) + + +def _check_changelog_job() -> dict: + workflow = yaml.safe_load(PR_WORKFLOW.read_text(encoding="utf-8")) + return workflow["jobs"]["check-changelog"] + + +def test_the_job_checks_out_the_history_and_passes_the_base_commit(): + job = _check_changelog_job() + checkout_step, python_step, check_step = job["steps"] + + assert checkout_step["uses"] == "actions/checkout@v4" + assert checkout_step["with"] == {"fetch-depth": 0} + assert python_step["uses"] == "actions/setup-python@v5" + assert python_step["with"] == {"python-version": "3.12"} + assert check_step["env"] == { + "BASE_SHA": "${{ github.event.pull_request.base.sha }}" + } + assert check_step["run"] == "bash .github/scripts/check_changelog_fragment.sh" + assert job["permissions"] == {"contents": "read"} + + +def test_the_job_keeps_the_stack_condition_and_is_needed_by_ci_complete(): + workflow = yaml.safe_load(PR_WORKFLOW.read_text(encoding="utf-8")) + job = workflow["jobs"]["check-changelog"] + + assert job["if"] == ( + "${{ !github.event.pull_request.stack || " + "github.event.pull_request.stack.base.ref == " + "github.event.pull_request.base.ref }}" + ) + assert "check-changelog" in workflow["jobs"]["ci-complete"]["needs"] From 7a54bf5a02ef4d06178469b9578d066d86b298f5 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 29 Oct 2026 19:17:30 +0400 Subject: [PATCH 04/17] Test that empty folders and submodules are handled as git checks them out Git can't commit an empty folder, so one in a checkout is either left behind by a local git mv or is a submodule. List empty folders apart from the other problems, so the PR check can ignore a leftover one, and fail a pull request that adds a submodule to changelog.d/. Also write the byte-order mark in a test case as an escape. These fail because the definition still reports every empty folder as a problem and the check still counts every file under changelog.d/. Co-Authored-By: Claude Opus 5.5 --- tests/unit/test_changelog_fragment_check.py | 23 ++++++++++++++++ tests/unit/test_changelog_fragments.py | 29 ++++++++++++++------- 2 files changed, 42 insertions(+), 10 deletions(-) diff --git a/tests/unit/test_changelog_fragment_check.py b/tests/unit/test_changelog_fragment_check.py index cccb297fc..32abea027 100644 --- a/tests/unit/test_changelog_fragment_check.py +++ b/tests/unit/test_changelog_fragment_check.py @@ -301,6 +301,29 @@ def test_emptying_a_pending_fragment_fails_even_with_a_fragment_of_its_own( assert ON_THE_BASE_BRANCH not in result.stdout +def test_a_submodule_in_changelog_d_fails(repository: Path): + _write(repository, OWN_FRAGMENT) + _git( + repository, + "update-index", + "--add", + "--cacheinfo", + f"160000,{_master(repository)},changelog.d/vendored.added.md", + ) + _git(repository, "add", OWN_FRAGMENT) + # Commit the index as it stands: git add --all would stage the submodule's + # removal, since the working tree has no folder for it. + _git(repository, "commit", "--quiet", "-m", "Add a fragment and a submodule") + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 1, result.stdout + assert ( + "::error file=changelog.d/vendored.added.md::changelog.d/vendored.added.md " + "is an empty folder or a submodule inside changelog.d/." in result.stdout + ) + + def test_an_unreadable_file_already_on_master_fails_and_says_so(repository: Path): _git(repository, "checkout", "--quiet", "master") _write(repository, PULL_REQUEST_4084_FRAGMENT) diff --git a/tests/unit/test_changelog_fragments.py b/tests/unit/test_changelog_fragments.py index f88f8ac01..de14ec437 100644 --- a/tests/unit/test_changelog_fragments.py +++ b/tests/unit/test_changelog_fragments.py @@ -202,18 +202,27 @@ def test_every_file_in_a_folder_inside_changelog_d_is_a_problem( assert expected_problem in scan.problems[relative_path] -def test_an_empty_folder_inside_changelog_d_is_a_problem(repository: Path): +def test_empty_folders_are_listed_apart_from_the_problems(repository: Path): + """Git can't commit an empty folder, so one is in a checkout only by accident. + + A local ``git mv`` out of changelog.d/added/ leaves one behind; in a fresh + checkout, only a submodule makes one. The version bump treats each as a + problem, as Towncrier does; the PR check reports one only if the pull + request changed that path. + """ (repository / "changelog.d/added").mkdir() + (repository / "changelog.d/notes/drafts").mkdir(parents=True) - scan = _definition().scan(repository) + definition = _definition() + scan = definition.scan(repository) - assert scan.problems == { - "changelog.d/added": ( - "changelog.d/added is an empty folder inside changelog.d/. The " - "release reads only files directly in changelog.d/ and stops at " - "anything else there. Delete it." - ) - } + assert scan.problems == {} + assert scan.empty_folders == ("changelog.d/added", "changelog.d/notes/drafts") + assert definition.empty_folder_problem("changelog.d/added") == ( + "changelog.d/added is an empty folder or a submodule inside changelog.d/. " + "The release reads only files directly in changelog.d/ and stops at " + "anything else there. Delete it." + ) @pytest.mark.parametrize( @@ -290,7 +299,7 @@ def test_a_file_not_named_name_type_md_is_a_problem( f"changelog.d/4083.added.md holds only whitespace. {EMPTY_ENTRY}", ), ( - "\n", + "\ufeff\n", f"changelog.d/4083.added.md holds only whitespace. {EMPTY_ENTRY}", ), ], From dd1d2f776a8aa8879c728a668866af3944902129 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 29 Oct 2026 19:19:10 +0400 Subject: [PATCH 05/17] Check the pull request's own fragments against the shared definition check_changelog_fragment.sh now compares HEAD with its merge base with BASE_SHA, the event's base commit, and passes git's name-status output for changelog.d/ to changelog_fragments.py check-pull-request. The check fails unless the pull request adds a fragment of its own and every file in changelog.d/ is a fragment, with one error annotation per problem. A rename of a fragment already on the base branch adds no release note, so it doesn't count; moving a file the release couldn't read into place does. A shallow clone or a base commit the clone lacks fails with how to fetch the history. On GitHub's test merge commit, the first parent is BASE_SHA, so the comparison is exactly what merging the pull request adds. The job now fetches the full history, sets up Python 3.12, passes github.event.pull_request.base.sha as BASE_SHA and reads with contents: read only. Empty folders are listed apart from the other problems, so a folder a local git mv left behind doesn't fail the check, while a submodule does. Co-Authored-By: Claude Opus 5.5 --- .github/changelog_fragments.py | 253 ++++++++++++++++++-- .github/scripts/check_changelog_fragment.sh | 53 +++- .github/workflows/pr.yml | 19 +- 3 files changed, 298 insertions(+), 27 deletions(-) diff --git a/.github/changelog_fragments.py b/.github/changelog_fragments.py index dbce66b7d..a97c258c2 100644 --- a/.github/changelog_fragments.py +++ b/.github/changelog_fragments.py @@ -24,11 +24,22 @@ changelog.d/added/4083.md: the old check counted every file under changelog.d/, so it passed, and the release, which reads only files directly in changelog.d/, found no fragment. + +Commands, run from the repository root: + + directory print the fragment directory + check-pull-request read ``git diff --find-renames --name-status -z`` output + for the fragment directory on stdin, from the pull + request's base to the checked-out commit, and exit 1 + unless the pull request adds a fragment of its own and + every file in the directory is a fragment """ from __future__ import annotations +import argparse import re +import sys import tomllib from dataclasses import dataclass, field from pathlib import Path, PurePosixPath @@ -46,11 +57,18 @@ rf"(?P{NAME_PATTERN})\.(?P[^.]+)\.{FRAGMENT_EXTENSION}" ) TYPE_NAME = re.compile(r"[a-z]+") -BYTE_ORDER_MARK = "" +BYTE_ORDER_MARK = "\ufeff" NAME_TERMS = ( " is letters and digits, in groups joined by single dots, hyphens or " "underscores" ) +# git diff --name-status statuses that name two paths: the source, then the +# destination. Every other status names one. +TWO_PATH_STATUSES = frozenset({"R", "C"}) +ON_THE_BASE_BRANCH = ( + "It is already on the base branch, so the next release fails until a pull " + "request fixes it." +) class ConfigurationError(Exception): @@ -66,16 +84,33 @@ class Fragment: fragment_type: str +@dataclass(frozen=True) +class Change: + """One file a pull request changes, from ``git diff --name-status``. + + ``status`` is git's status letter: A added, C copied, D deleted, M + modified, R renamed, T changed type. ``source`` is the path a renamed or + copied file came from. + """ + + status: str + path: str + source: str | None = None + + @dataclass(frozen=True) class Scan: - """What the fragment directory holds: its fragments, and every other file. + """What the fragment directory holds: its fragments, and everything else. - ``problems`` maps each other path, from the repository root, to why the - release can't read it. + ``problems`` maps each other file and symbolic link, by its path from the + repository root, to why the release can't read it. ``empty_folders`` lists + the empty folders, which git can't commit: a checkout has one only when a + local command left it behind or a submodule is there. """ fragments: tuple[Fragment, ...] problems: dict[str, str] = field(default_factory=dict) + empty_folders: tuple[str, ...] = () def _listed(names: tuple[str, ...]) -> str: @@ -302,6 +337,30 @@ def problem_with(self, path: str, file: Path) -> str | None: or _content_problem(path, file) ) + def names_a_fragment(self, path: str) -> bool: + """Return whether ``path`` is where and how a fragment is named. + + This looks only at the path, not at a file's text. + """ + return self._name_problem(path) is None + + def added_paths(self, changes: list[Change]) -> set[str]: + """Return the paths a pull request adds a file at, by its ``changes``. + + A renamed file counts only when its old path was not a fragment name: + renaming a fragment already on the base branch adds no release note, + but moving a file the release couldn't read into place does. + """ + added = set() + for change in changes: + if change.status in {"A", "C"} or ( + change.status == "R" + and change.source is not None + and not self.names_a_fragment(change.source) + ): + added.add(change.path) + return added + def _fragment(self, path: str) -> Fragment: """Return the fragment at ``path``, a name ``problem_with`` accepted.""" match = FRAGMENT_FILE_NAME.fullmatch(PurePosixPath(path).name) @@ -312,25 +371,18 @@ def _fragment(self, path: str) -> Fragment: path=path, name=match.group("name"), fragment_type=match.group("type") ) - def _folder_problems(self, root: Path, folder: Path) -> dict[str, str]: - """Return a problem for each file, link and empty folder under ``folder``.""" - problems = {} - for entry in _nested_entries(folder): - path = entry.relative_to(root).as_posix() - if entry.is_dir() and not entry.is_symlink(): - problems[path] = ( - f"{path} is an empty folder inside {self.directory}/. " - f"{self._stops_the_release()} Delete it." - ) - else: - problems[path] = self._nested_problem(PurePosixPath(path)) - return problems + def empty_folder_problem(self, path: str) -> str: + """Return why the release can't build with an empty folder at ``path``.""" + return ( + f"{path} is an empty folder or a submodule inside {self.directory}/. " + f"{self._stops_the_release()} Delete it." + ) def scan(self, root: Path) -> Scan: """Return the fragments in ``root``'s fragment directory, and every other file. - Every file, symbolic link and empty folder in a folder inside the - directory is a problem: Towncrier stops at a folder there. + Every file and symbolic link in a folder inside the directory is a + problem: Towncrier stops at a folder there. """ directory = root / self.directory if not directory.exists() and not directory.is_symlink(): @@ -347,10 +399,18 @@ def scan(self, root: Path) -> Scan: ) fragments: list[Fragment] = [] problems: dict[str, str] = {} + empty_folders: list[str] = [] for entry in sorted(directory.iterdir()): path = f"{self.directory}/{entry.name}" if entry.is_dir() and not entry.is_symlink(): - problems.update(self._folder_problems(root, entry)) + for nested_entry in _nested_entries(entry): + nested_path = nested_entry.relative_to(root).as_posix() + if nested_entry.is_dir() and not nested_entry.is_symlink(): + empty_folders.append(nested_path) + else: + problems[nested_path] = self._nested_problem( + PurePosixPath(nested_path) + ) continue if entry.name == KEEP_FILE: kind_problem = _file_kind_problem(path, entry) @@ -362,4 +422,155 @@ def scan(self, root: Path) -> Scan: fragments.append(self._fragment(path)) else: problems[path] = problem - return Scan(fragments=tuple(fragments), problems=problems) + return Scan( + fragments=tuple(fragments), + problems=problems, + empty_folders=tuple(empty_folders), + ) + + +def parse_name_status(output: bytes) -> list[Change]: + """Return the changes in ``git diff --name-status -z`` output. + + Paths that aren't UTF-8 keep their bytes as surrogate escapes, as + ``os.fsdecode`` decodes them on Linux. + """ + fields = [ + output_field.decode("utf-8", errors="surrogateescape") + for output_field in output.split(b"\0") + ] + if fields and not fields[-1]: + fields.pop() + changes = [] + index = 0 + while index < len(fields): + status = fields[index][:1] + path_count = 2 if status in TWO_PATH_STATUSES else 1 + paths = fields[index + 1 : index + 1 + path_count] + if not status or len(paths) != path_count: + msg = ( + f"git diff --name-status output ends in the middle of a change: " + f"{fields[index:]!r}" + ) + raise ValueError(msg) + if path_count == 1: + changes.append(Change(status=status, path=paths[0])) + else: + changes.append(Change(status=status, path=paths[1], source=paths[0])) + index += 1 + path_count + return changes + + +def annotation_message(message: str) -> str: + """Escape ``message`` so the Actions runner reads it as one annotation message.""" + return message.replace("%", "%25").replace("\r", "%0D").replace("\n", "%0A") + + +def annotation_property(value: str) -> str: + """Escape ``value`` for an annotation property such as ``file=``.""" + return annotation_message(value).replace(":", "%3A").replace(",", "%2C") + + +def _print_error(message: str, path: str | None = None) -> None: + """Print ``message`` as an error annotation, on ``path`` when one is given.""" + location = f" file={annotation_property(path)}" if path else "" + print(f"::error{location}::{annotation_message(message)}") + + +def _no_own_fragment_message( + definition: FragmentDefinition, scan: Scan, changes: list[Change] +) -> str: + """Return the error for a pull request that adds no fragment of its own.""" + sentences = ["This pull request adds no changelog fragment of its own."] + pending = [fragment.path for fragment in scan.fragments] + if pending: + sentences.append( + "Fragments already on the base branch belong to the pull requests " + f"that added them: {', '.join(pending)}." + ) + sentences.extend( + "Renaming a fragment that is already on the base branch " + f"({change.source} to {change.path}) doesn't add one." + for change in changes + if change.status == "R" + and change.source is not None + and definition.names_a_fragment(change.source) + ) + sentences.append(definition.accepted_form()) + return " ".join(sentences) + + +def check_pull_request(root: Path, changes: list[Change]) -> int: + """Check the checkout in ``root`` of a pull request that makes ``changes``. + + Print an error annotation for each file in the fragment directory that is + not a fragment, and one if the pull request adds no fragment of its own; + return the exit status. + """ + definition = FragmentDefinition.load(root) + scan = definition.scan(root) + changed_paths = {change.path for change in changes if change.status != "D"} + problems = { + path: problem if path in changed_paths else f"{problem} {ON_THE_BASE_BRANCH}" + for path, problem in scan.problems.items() + } + # A path the pull request changed that is neither a fragment, the keep + # file nor a problem holds no file in the checkout: it is a submodule. An + # empty folder the pull request didn't change was left by a local command, + # since git can't commit one. + read_paths = {fragment.path for fragment in scan.fragments} | set(problems) + keep_path = f"{definition.directory}/{KEEP_FILE}" + if (root / keep_path).is_file() and not (root / keep_path).is_symlink(): + read_paths.add(keep_path) + problems.update( + (path, definition.empty_folder_problem(path)) + for path in sorted(changed_paths - read_paths) + ) + for path, problem in problems.items(): + _print_error(problem, path) + + added_paths = definition.added_paths(changes) + own_fragments = [ + fragment for fragment in scan.fragments if fragment.path in added_paths + ] + if not own_fragments: + _print_error(_no_own_fragment_message(definition, scan, changes)) + if problems or not own_fragments: + return 1 + + count = ( + "1 changelog fragment" + if len(own_fragments) == 1 + else f"{len(own_fragments)} changelog fragments" + ) + listed = ", ".join( + f"{fragment.path} ({fragment.fragment_type})" for fragment in own_fragments + ) + print(f"This pull request adds {count}: {listed}.") + return 0 + + +def main(arguments: list[str] | None = None) -> int: + """Run the command named in ``arguments``; return the exit status.""" + parser = argparse.ArgumentParser(description=__doc__.split("\n", 1)[0]) + commands = parser.add_subparsers(dest="command", required=True) + commands.add_parser("directory", help="print the fragment directory") + commands.add_parser( + "check-pull-request", + help="check a pull request; git diff --name-status -z output on stdin", + ) + options = parser.parse_args(arguments) + root = Path.cwd() + try: + if options.command == "directory": + print(FragmentDefinition.load(root).directory) + return 0 + changes = parse_name_status(sys.stdin.buffer.read()) + return check_pull_request(root, changes) + except (ConfigurationError, ValueError) as error: + _print_error(str(error)) + return 1 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/.github/scripts/check_changelog_fragment.sh b/.github/scripts/check_changelog_fragment.sh index 280e9eb0e..9eceee793 100644 --- a/.github/scripts/check_changelog_fragment.sh +++ b/.github/scripts/check_changelog_fragment.sh @@ -1,11 +1,54 @@ #!/usr/bin/env bash +# Require the pull request to add a changelog fragment the release can read +# (rule api.process.changelog-fragment). The "Check changelog fragment" job in +# .github/workflows/pr.yml runs this on the commit GitHub checks out. +# +# It lists what the pull request changes in the fragment directory +# (changelog.d/) relative to its base, and passes that to +# .github/changelog_fragments.py. That fails unless the pull request adds a +# fragment of its own and every file in the directory is a fragment. Fragments +# already on the base branch belong to the pull requests that added them, so +# editing, deleting or renaming one doesn't count; moving a file the release +# couldn't read into place does. +# +# The comparison starts at the merge base of BASE_SHA and HEAD. For a pull +# request event, GitHub checks out its test merge commit, whose first parent +# is BASE_SHA, so this is exactly what merging the pull request adds to its +# base. Checked out at the pull request's own head, it is what the branch has +# added since it left the base branch. +# +# Run from the repository root, in a clone with the base branch's history +# (actions/checkout with fetch-depth: 0). +# +# Environment: +# BASE_SHA the commit the pull request merges into +# (github.event.pull_request.base.sha); locally, for example, +# BASE_SHA=origin/master +# PYTHON the Python 3.11+ interpreter to run the check with +# (default: python3) + set -euo pipefail -fragments="$(find changelog.d -type f ! -name '.gitkeep' | wc -l)" -if [[ "${fragments}" -eq 0 ]]; then - echo "::error::No changelog fragment found in changelog.d/" - echo "Add one with: echo 'Description.' > changelog.d/\$(git branch --show-current)..md" - echo "Types: added, changed, fixed, removed, breaking" +base_revision="${BASE_SHA:?BASE_SHA is required: the commit the pull request merges into, such as origin/master}" +python_bin="${PYTHON:-python3}" +fragments_module="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)/changelog_fragments.py" + +if [[ "$(git rev-parse --is-shallow-repository)" == "true" ]]; then + echo "::error::This clone is shallow, so it may lack the base commit or the history that leads to it. Check out with fetch-depth: 0." + exit 1 +fi + +if ! base_commit="$(git merge-base "${base_revision}" HEAD)"; then + echo "::error::Could not find a common ancestor of ${base_revision} and HEAD. The clone needs the base branch's history: check out with fetch-depth: 0." exit 1 fi + +# The module prints its own error annotation when it can't read the directory. +if ! fragment_directory="$("${python_bin}" "${fragments_module}" directory)"; then + echo "${fragment_directory}" + exit 1 +fi + +git diff --find-renames --name-status -z "${base_commit}" HEAD -- "${fragment_directory}" \ + | "${python_bin}" "${fragments_module}" check-pull-request diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index a673dc34c..6e63b92ba 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -157,13 +157,30 @@ jobs: secrets: CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} check-changelog: + # Rule api.process.changelog-fragment: every pull request adds a changelog + # fragment the release can read. See + # docs/engineering/skills/github-prs.md#changelog-fragments. name: Check changelog fragment # Full CI runs only on the lowest unmerged pull request in a stack. if: ${{ !github.event.pull_request.stack || github.event.pull_request.stack.base.ref == github.event.pull_request.base.ref }} runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + contents: read steps: - - uses: actions/checkout@v4 + - name: Checkout repo + uses: actions/checkout@v4 + with: + # The check compares the checked-out merge commit with the pull + # request's base commit, so it needs the base branch's history. + fetch-depth: 0 + - name: Setup Python + uses: actions/setup-python@v5 + with: + python-version: "3.12" - name: Check for changelog fragment + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} run: bash .github/scripts/check_changelog_fragment.sh test_container_builds: name: Docker From e63b7c03e469b3aff252a7fd4670c474c7fb2e09 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 29 Oct 2026 19:20:10 +0400 Subject: [PATCH 06/17] Test that the version bump reads fragments through the shared definition infer_bump now takes the repository root and must read changelog.d/ through changelog_fragments.py, keep its rule (breaking gives a major bump, added or removed a minor one, changed or fixed a patch), refuse to run when its bump table and the types in [tool.towncrier] differ, and stop with status 1 at any file it can't read, such as #4084's changelog.d/added/4083.md or an empty fragment, instead of skipping it. Run the script as the versioning job does, from a copy of .github/. These fail because bump_version.py still reads every top-level file, takes a type from any suffix and skips the rest. Co-Authored-By: Claude Opus 5.5 --- tests/unit/test_bump_version.py | 268 +++++++++++++++++++++++++++----- 1 file changed, 228 insertions(+), 40 deletions(-) diff --git a/tests/unit/test_bump_version.py b/tests/unit/test_bump_version.py index 4929f3c6a..177d123fc 100644 --- a/tests/unit/test_bump_version.py +++ b/tests/unit/test_bump_version.py @@ -1,66 +1,254 @@ -"""Characterization tests for the changelog-driven version bump inference.""" +"""Tests for .github/bump_version.py, which raises the version at each release. -import importlib.util -from pathlib import Path -from types import ModuleType +The versioning job in .github/workflows/push.yml runs it after a merge to +master, before Towncrier builds CHANGELOG.md. It reads the pending fragments +through .github/changelog_fragments.py, the definition the PR check uses (rule +api.process.changelog-fragment), and stops at any file it can't read. +""" + +from __future__ import annotations + +import re +import shutil +import subprocess +import sys +from typing import TYPE_CHECKING import pytest -BUMP_VERSION_PATH = Path(__file__).parents[2] / ".github" / "bump_version.py" +from tests.unit.changelog_fragment_support import ( + GITHUB_DIRECTORY, + REPO, + bump_version, + changelog_fragments, +) + +if TYPE_CHECKING: + from pathlib import Path +bump_module = bump_version() + +FRAGMENT_TEXT = "A change.\n" +UNREADABLE_SUMMARY = ( + "The release reads every file in changelog.d/, and 1 of them is not a " + "fragment. Fix it in a pull request; the release stops until then.\n" +) -def _load_bump_version() -> ModuleType: - spec = importlib.util.spec_from_file_location("bump_version", BUMP_VERSION_PATH) - assert spec is not None - assert spec.loader is not None - module = importlib.util.module_from_spec(spec) - spec.loader.exec_module(module) - return module +def _write(root: Path, relative_path: str, text: str = FRAGMENT_TEXT) -> None: + file_path = root / relative_path + file_path.parent.mkdir(parents=True, exist_ok=True) + file_path.write_text(text, encoding="utf-8") -def _changelog_dir(tmp_path: Path, *fragment_names: str) -> Path: - changelog_dir = tmp_path / "changelog.d" - changelog_dir.mkdir() - for name in fragment_names: - (changelog_dir / name).write_text("A change.\n") - return changelog_dir + +@pytest.fixture +def repository(tmp_path: Path) -> Path: + """Return a root holding the repository's pyproject.toml and changelog.d/.""" + (tmp_path / "pyproject.toml").write_text( + (REPO / "pyproject.toml").read_text(encoding="utf-8"), encoding="utf-8" + ) + _write(tmp_path, "changelog.d/.gitkeep", "") + return tmp_path @pytest.mark.parametrize( ("fragment_names", "expected_bump"), [ (("1.fixed.md",), "patch"), - (("1.fixed",), "patch"), - (("notes",), "patch"), + (("1.changed.md",), "patch"), + (("1.changed.md", "2.fixed.md"), "patch"), (("1.added.md",), "minor"), - (("1.added",), "minor"), + (("1.removed.md",), "minor"), (("1.removed.md", "2.fixed.md"), "minor"), + (("1.breaking.md",), "major"), (("a.b.breaking.md", "1.added.md"), "major"), - (("1.breaking",), "major"), - (("breaking.md",), "patch"), - ((".added.md",), "minor"), - (("1.changed.md", ".gitkeep"), "patch"), + (("update-policyengine-bundle-6.11.1.changed.md",), "patch"), ], ) -def test_infer_bump_reads_the_category_from_the_suffix_or_the_stem( - tmp_path: Path, fragment_names: tuple[str, ...], expected_bump: str -) -> None: - bump_version = _load_bump_version() - - assert ( - bump_version.infer_bump(_changelog_dir(tmp_path, *fragment_names)) - == expected_bump - ) +def test_infer_bump_takes_the_largest_bump_a_pending_fragment_calls_for( + repository: Path, fragment_names: tuple[str, ...], expected_bump: str +): + for fragment_name in fragment_names: + _write(repository, f"changelog.d/{fragment_name}") + assert bump_module.infer_bump(repository) == expected_bump -def test_infer_bump_exits_when_only_gitkeep_is_present( + +def test_the_bump_table_has_a_bump_for_exactly_the_configured_types(): + definition = changelog_fragments().FragmentDefinition.load(REPO) + + assert bump_module.BUMP_BY_TYPE == { + "breaking": "major", + "added": "minor", + "changed": "patch", + "fixed": "patch", + "removed": "minor", + } + assert set(bump_module.BUMP_BY_TYPE) == set(definition.types) + + +def test_infer_bump_refuses_types_the_bump_table_does_not_list( tmp_path: Path, capsys: pytest.CaptureFixture[str] -) -> None: - bump_version = _load_bump_version() - changelog_dir = _changelog_dir(tmp_path, ".gitkeep") +): + type_tables = "".join( + f'\n[[tool.towncrier.type]]\ndirectory = "{fragment_type}"\n' + for fragment_type in ("added", "fixed", "security") + ) + (tmp_path / "pyproject.toml").write_text( + f'[tool.towncrier]\ndirectory = "changelog.d"\n{type_tables}', + encoding="utf-8", + ) + _write(tmp_path, "changelog.d/1.security.md") with pytest.raises(SystemExit) as exit_info: - bump_version.infer_bump(changelog_dir) + bump_module.infer_bump(tmp_path) assert exit_info.value.code == 1 - assert capsys.readouterr().err == "No changelog fragments found\n" + assert capsys.readouterr().err == ( + "The fragment types in [tool.towncrier] in pyproject.toml are added, " + "fixed and security, but BUMP_BY_TYPE in .github/bump_version.py has a " + "bump for breaking, added, changed, fixed and removed. Types with no " + "bump: security. Bumps with no type: breaking, changed and removed. " + "Change both together.\n" + ) + + +@pytest.mark.parametrize( + ("relative_path", "text", "expected_problem"), + [ + ( + "changelog.d/added/4083.md", + FRAGMENT_TEXT, + "changelog.d/added/4083.md is in a folder inside changelog.d/. The " + "release reads only files directly in changelog.d/ and stops at " + "anything else there. Move it to changelog.d/4083.added.md.", + ), + ("changelog.d/notes", FRAGMENT_TEXT, "is not named ..md"), + ("changelog.d/1.added", FRAGMENT_TEXT, "is not named ..md"), + ("changelog.d/.added.md", FRAGMENT_TEXT, "is not named ..md"), + ("changelog.d/breaking.md", FRAGMENT_TEXT, "is not named ..md"), + ("changelog.d/1.feature.md", FRAGMENT_TEXT, 'has the type "feature"'), + ("changelog.d/1.added.md", "", "changelog.d/1.added.md is empty."), + ( + "changelog.d/1.added.md", + " \n", + "changelog.d/1.added.md holds only whitespace.", + ), + ], + ids=[ + "pull-request-4084", + "no-extension", + "no-md-extension", + "no-name", + "no-type", + "unknown-type", + "empty", + "whitespace-only", + ], +) +def test_infer_bump_stops_at_a_file_it_cannot_read( + repository: Path, + capsys: pytest.CaptureFixture[str], + relative_path: str, + text: str, + expected_problem: str, +): + _write(repository, "changelog.d/2.fixed.md") + _write(repository, relative_path, text) + + with pytest.raises(SystemExit) as exit_info: + bump_module.infer_bump(repository) + + assert exit_info.value.code == 1 + error_output = capsys.readouterr().err + assert expected_problem in error_output + assert error_output.endswith(UNREADABLE_SUMMARY) + + +def test_infer_bump_stops_at_an_empty_folder( + repository: Path, capsys: pytest.CaptureFixture[str] +): + _write(repository, "changelog.d/2.fixed.md") + (repository / "changelog.d/vendored").mkdir() + + with pytest.raises(SystemExit) as exit_info: + bump_module.infer_bump(repository) + + assert exit_info.value.code == 1 + assert capsys.readouterr().err == ( + "changelog.d/vendored is an empty folder or a submodule inside " + "changelog.d/. The release reads only files directly in changelog.d/ " + "and stops at anything else there. Delete it.\n" + UNREADABLE_SUMMARY + ) + + +def test_infer_bump_exits_when_only_gitkeep_is_present( + repository: Path, capsys: pytest.CaptureFixture[str] +): + with pytest.raises(SystemExit) as exit_info: + bump_module.infer_bump(repository) + + assert exit_info.value.code == 1 + assert capsys.readouterr().err == ( + "No changelog fragments found in changelog.d/: there is nothing to release.\n" + ) + + +def _release_checkout(tmp_path: Path) -> Path: + """Return a copy of the files the versioning job's bump step reads.""" + (tmp_path / ".github").mkdir() + for script_name in ("bump_version.py", "changelog_fragments.py"): + shutil.copy(GITHUB_DIRECTORY / script_name, tmp_path / ".github" / script_name) + (tmp_path / "pyproject.toml").write_text( + (REPO / "pyproject.toml").read_text(encoding="utf-8"), encoding="utf-8" + ) + _write(tmp_path, "changelog.d/.gitkeep", "") + return tmp_path + + +def _project_version(root: Path) -> tuple[int, int, int]: + match = re.search( + r'^version = "(\d+)\.(\d+)\.(\d+)"', + (root / "pyproject.toml").read_text(encoding="utf-8"), + re.MULTILINE, + ) + assert match is not None + major, minor, patch = (int(part) for part in match.groups()) + return major, minor, patch + + +def _run_bump(root: Path) -> subprocess.CompletedProcess[str]: + """Run the copied bump script as push.yml does: python .github/bump_version.py.""" + return subprocess.run( + [sys.executable, ".github/bump_version.py"], + cwd=root, + text=True, + capture_output=True, + check=False, + ) + + +def test_the_script_raises_the_version_as_the_release_runs_it(tmp_path: Path): + root = _release_checkout(tmp_path) + _write(root, "changelog.d/4083.added.md") + major, minor, _patch = _project_version(root) + + result = _run_bump(root) + + assert result.returncode == 0, result.stderr + assert _project_version(root) == (major, minor + 1, 0) + assert result.stdout.startswith("Version: ") + assert result.stdout.splitlines()[0].endswith(f"-> {major}.{minor + 1}.0 (minor)") + + +def test_the_script_stops_at_pull_request_4084s_fragment(tmp_path: Path): + root = _release_checkout(tmp_path) + _write(root, "changelog.d/2.fixed.md") + _write(root, "changelog.d/added/4083.md") + version_before = _project_version(root) + + result = _run_bump(root) + + assert result.returncode == 1, result.stdout + assert "Move it to changelog.d/4083.added.md." in result.stderr + assert _project_version(root) == version_before From a37f0c1ce19d67ce58cf36875715cf9203cba476 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 29 Oct 2026 19:20:52 +0400 Subject: [PATCH 07/17] Bump the version through the shared fragment definition bump_version.py imports changelog_fragments.py from beside it, as python .github/bump_version.py puts .github/ first on sys.path, and reads changelog.d/ through FragmentDefinition. It prints each file it can't read and exits with status 1 before changing pyproject.toml, instead of skipping the file, and refuses to run when BUMP_BY_TYPE and the types in [tool.towncrier] differ. The bump rule is unchanged: breaking gives a major bump, added or removed a minor one, and changed or fixed a patch. Co-Authored-By: Claude Opus 5.5 --- .github/bump_version.py | 130 ++++++++++++++++++++++++++++++---------- 1 file changed, 97 insertions(+), 33 deletions(-) diff --git a/.github/bump_version.py b/.github/bump_version.py index 78dc45bb0..6826f1e8d 100644 --- a/.github/bump_version.py +++ b/.github/bump_version.py @@ -1,12 +1,47 @@ -"""Infer semver bump from towncrier fragment types and update version.""" +"""Raise the version in pyproject.toml from the pending changelog fragments' types. + +The versioning job in .github/workflows/push.yml and ``make changelog`` run +this before Towncrier builds CHANGELOG.md. It reads the fragments through +``FragmentDefinition`` in changelog_fragments.py, beside this file, the +definition the PR check uses too (rule api.process.changelog-fragment). It +exits with status 1, changing nothing, when changelog.d/ holds anything that +isn't a fragment, holds no fragment, or when ``BUMP_BY_TYPE`` and the types in +[tool.towncrier] differ. +""" import re import sys from pathlib import Path -from typing import TextIO - -# A fragment named "..md" has at least two dot-separated stem parts. -STEM_PARTS_WITH_CATEGORY = 2 +from typing import NoReturn, TextIO + +from changelog_fragments import ( + CONFIG_TABLE, + ConfigurationError, + FragmentDefinition, +) + +# The version part each fragment type raises: breaking the major version, added +# and removed the minor version, and changed and fixed the patch version. Its +# keys must be the types in [tool.towncrier]; infer_bump refuses to run when +# they differ. +BUMP_BY_TYPE = { + "breaking": "major", + "added": "minor", + "changed": "patch", + "fixed": "patch", + "removed": "minor", +} +# Version parts from the smallest bump to the largest. +BUMP_SIZES = ("patch", "minor", "major") + + +def _listed(names: list[str]) -> str: + """Return ``names`` as "a, b and c", or "none".""" + if not names: + return "none" + if len(names) == 1: + return names[0] + return f"{', '.join(names[:-1])} and {names[-1]}" def _report(message: str, *, file: TextIO | None = None) -> None: @@ -14,48 +49,78 @@ def _report(message: str, *, file: TextIO | None = None) -> None: print(message, file=file) +def _fail(message: str) -> NoReturn: + """Print ``message`` to stderr and exit with status 1.""" + _report(message, file=sys.stderr) + sys.exit(1) + + def get_current_version(pyproject_path: Path) -> str: """Return the version in pyproject.toml, exiting with status 1 if it is absent.""" text = pyproject_path.read_text() match = re.search(r'^version\s*=\s*"(\d+\.\d+\.\d+)"', text, re.MULTILINE) if not match: - _report( - "Could not find version in pyproject.toml", - file=sys.stderr, - ) - sys.exit(1) + _fail("Could not find version in pyproject.toml") return match.group(1) -def infer_bump(changelog_dir: Path) -> str: - """Return "major", "minor" or "patch" from the pending changelog fragment types. +def _require_a_bump_for_each_type(types: tuple[str, ...]) -> None: + """Exit with status 1 unless ``BUMP_BY_TYPE`` has a bump for exactly ``types``.""" + if set(types) == set(BUMP_BY_TYPE): + return + _fail( + f"The fragment types in {CONFIG_TABLE} are {_listed(list(types))}, but " + f"BUMP_BY_TYPE in .github/bump_version.py has a bump for " + f"{_listed(list(BUMP_BY_TYPE))}. Types with no bump: " + f"{_listed([name for name in types if name not in BUMP_BY_TYPE])}. " + f"Bumps with no type: " + f"{_listed([name for name in BUMP_BY_TYPE if name not in types])}. " + "Change both together." + ) + + +def infer_bump(root: Path) -> str: + """Return "major", "minor" or "patch" from the fragments in ``root``'s changelog.d/. Any breaking fragment makes a major bump and any added or removed fragment a - minor one; exit with status 1 when there are no fragments. + minor one. Exit with status 1 when the directory holds a file that isn't a + fragment, or no fragment. """ - fragments = [ - f for f in changelog_dir.iterdir() if f.is_file() and f.name != ".gitkeep" + try: + definition = FragmentDefinition.load(root) + except ConfigurationError as error: + _fail(str(error)) + _require_a_bump_for_each_type(definition.types) + scan = definition.scan(root) + problems = [ + *scan.problems.values(), + *(definition.empty_folder_problem(path) for path in scan.empty_folders), ] - if not fragments: - _report("No changelog fragments found", file=sys.stderr) - sys.exit(1) - - categories = {f.suffix.lstrip(".") for f in fragments} - for f in fragments: - parts = f.stem.split(".") - if len(parts) >= STEM_PARTS_WITH_CATEGORY: - categories.add(parts[-1]) - - if "breaking" in categories: - return "major" - if "added" in categories or "removed" in categories: - return "minor" - return "patch" + if problems: + for problem in problems: + _report(problem, file=sys.stderr) + if len(problems) == 1: + unreadable = "1 of them is not a fragment. Fix it" + else: + unreadable = f"{len(problems)} of them are not fragments. Fix them" + _fail( + f"The release reads every file in {definition.directory}/, and " + f"{unreadable} in a pull request; the release stops until then." + ) + if not scan.fragments: + _fail( + f"No changelog fragments found in {definition.directory}/: there is " + "nothing to release." + ) + return max( + (BUMP_BY_TYPE[fragment.fragment_type] for fragment in scan.fragments), + key=BUMP_SIZES.index, + ) def bump_version(version: str, bump: str) -> str: """Return ``version`` raised by one ``bump`` level, resetting the lower parts.""" - major, minor, patch = (int(x) for x in version.split(".")) + major, minor, patch = (int(part) for part in version.split(".")) if bump == "major": return f"{major + 1}.0.0" if bump == "minor": @@ -79,10 +144,9 @@ def main() -> None: """Bump the version in pyproject.toml according to the changelog.d fragments.""" root = Path(__file__).resolve().parent.parent pyproject = root / "pyproject.toml" - changelog_dir = root / "changelog.d" current = get_current_version(pyproject) - bump = infer_bump(changelog_dir) + bump = infer_bump(root) new = bump_version(current, bump) _report(f"Version: {current} -> {new} ({bump})") From e9b27ff44816c325d123896db2e811c77db826d7 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 29 Oct 2026 19:21:30 +0400 Subject: [PATCH 08/17] Test that the locked Towncrier stops at files it can't read Run the Towncrier version uv.lock pins on the repository's [tool.towncrier] configuration, as the release's changelog build does. With #4084's changelog.d/added/4083.md, an unknown type or an uppercase type, the build must fail instead of leaving the file out; the fragments the shared definition accepts must still build. These fail because [tool.towncrier] has no ignore setting, so Towncrier 25.8.0 skips such files and builds without them. Co-Authored-By: Claude Opus 5.5 --- tests/unit/test_bump_version.py | 59 +++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/tests/unit/test_bump_version.py b/tests/unit/test_bump_version.py index 177d123fc..e7b174394 100644 --- a/tests/unit/test_bump_version.py +++ b/tests/unit/test_bump_version.py @@ -12,6 +12,7 @@ import shutil import subprocess import sys +import tomllib from typing import TYPE_CHECKING import pytest @@ -252,3 +253,61 @@ def test_the_script_stops_at_pull_request_4084s_fragment(tmp_path: Path): assert result.returncode == 1, result.stdout assert "Move it to changelog.d/4083.added.md." in result.stderr assert _project_version(root) == version_before + + +def _towncrier_draft(root: Path) -> subprocess.CompletedProcess[str]: + """Run the locked Towncrier's draft build in ``root``, as the release builds.""" + (root / "CHANGELOG.md").write_text("# Changelog\n", encoding="utf-8") + return subprocess.run( + [sys.executable, "-m", "towncrier", "build", "--draft", "--version", "9.9.9"], + cwd=root, + text=True, + capture_output=True, + check=False, + ) + + +def test_towncrier_is_set_to_stop_at_files_it_cannot_read(): + towncrier = tomllib.loads((REPO / "pyproject.toml").read_text(encoding="utf-8"))[ + "tool" + ]["towncrier"] + + assert towncrier["ignore"] == [".gitkeep"] + + +@pytest.mark.parametrize( + ("relative_path", "invalid_name"), + [ + ("changelog.d/added/4083.md", "added"), + ("changelog.d/4083.feature.md", "4083.feature.md"), + ("changelog.d/4083.Added.md", "4083.Added.md"), + ], + ids=["pull-request-4084", "unknown-type", "uppercase-type"], +) +def test_the_locked_towncrier_stops_at_a_file_it_cannot_read( + repository: Path, relative_path: str, invalid_name: str +): + _write(repository, "changelog.d/2.fixed.md") + _write(repository, relative_path) + + result = _towncrier_draft(repository) + + assert result.returncode != 0, result.stdout + assert f"Invalid news fragment name: {invalid_name}" in result.stderr + + +def test_the_locked_towncrier_builds_the_fragments_the_definition_accepts( + repository: Path, +): + _write(repository, "changelog.d/2.fixed.md", "Fix the second thing.\n") + _write( + repository, + "changelog.d/update-policyengine-bundle-6.11.1.changed.md", + "Update the bundle.\n", + ) + + result = _towncrier_draft(repository) + + assert result.returncode == 0, result.stderr + assert "### Changed\n\n- Update the bundle.\n" in result.stdout + assert "### Fixed\n\n- Fix the second thing.\n" in result.stdout From 2c252186d4de44c846c08443a5f0156c42775c15 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 29 Oct 2026 19:21:40 +0400 Subject: [PATCH 09/17] Make Towncrier fail on files in changelog.d/ it can't read Set ignore = [".gitkeep"] in [tool.towncrier]. With an ignore list, the locked Towncrier (25.8.0) fails on a folder or a file whose name has no configured type, instead of leaving it out of CHANGELOG.md. It still renders an empty fragment as an empty bullet, which the version bump, run first, now refuses. Note beside the configuration which files read it. Co-Authored-By: Claude Opus 5.5 --- pyproject.toml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/pyproject.toml b/pyproject.toml index 77aebfb1d..f419cfa3d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -176,6 +176,11 @@ warn_return_any = true warn_unused_ignores = true show_error_codes = true +# .github/changelog_fragments.py reads `directory` and the types' `directory` +# names below, so the PR check, the version bump in .github/bump_version.py and +# Towncrier share one list (rule api.process.changelog-fragment). Adding or +# removing a type also needs its entry in BUMP_BY_TYPE in bump_version.py, +# which refuses to run otherwise. [tool.towncrier] package = "policyengine_api" directory = "changelog.d" @@ -183,6 +188,9 @@ filename = "CHANGELOG.md" title_format = "## [{version}] - {project_date}" issue_format = "" underlines = ["", "", ""] +# With an ignore list, Towncrier fails on any other file in changelog.d/, such as +# a folder or a file with an unknown type, instead of skipping it. +ignore = [".gitkeep"] [[tool.towncrier.type]] directory = "breaking" From 0e798824c60bdd71a62186791e42ac81a09b779f Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 29 Oct 2026 19:24:22 +0400 Subject: [PATCH 10/17] Describe the stricter changelog fragment rule and its exact form Update api.process.changelog-fragment: its statement names the accepted form and the shared definition, its rationale points to the new "Changelog fragments" section of the PR skill, its scope adds the definition, the bump and the check script, its CI wiring adds the versioning job's bump step, and its proof runs the definition on #4084's fragment and #4106's rename of it. Regenerate docs/generated/rules.md and the AGENTS.md rules block. State the accepted form, changelog.d/..md, in the PR skill, AGENTS.md, the PR template and the README, whose changelog section still described the retired changelog_entry.yaml. Add this pull request's fragment. Co-Authored-By: Claude Opus 5.5 --- .github/pull_request_template.md | 6 +++ AGENTS.md | 7 ++- README.md | 14 +++-- changelog.d/changelog-fragment-check.fixed.md | 1 + docs/engineering/skills/github-prs.md | 51 ++++++++++++++++++- docs/generated/rules.md | 10 ++-- rules/registry.toml | 22 ++++++-- 7 files changed, 93 insertions(+), 18 deletions(-) create mode 100644 changelog.d/changelog-fragment-check.fixed.md diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index daaae976d..86564102f 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -7,3 +7,9 @@ Fixes # ## Testing - + + diff --git a/AGENTS.md b/AGENTS.md index 3cf239dfd..1f7b8c19e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -55,8 +55,11 @@ code), or splitting a module into smaller ones, read Read `docs/engineering/skills/github-prs.md` before opening, replacing, or sharing any pull request. Every PR needs a tracking issue and -`Fixes #ISSUE_NUMBER` as the first line of its description. Agents must never -bypass branch rules (for example with `gh pr merge --admin`). +`Fixes #ISSUE_NUMBER` as the first line of its description. Every PR also adds +a changelog fragment of its own: a non-empty file directly in `changelog.d/`, +named `changelog.d/..md`, where `` is `breaking`, `added`, +`changed`, `fixed` or `removed` (see "Changelog fragments" in that skill). +Agents must never bypass branch rules (for example with `gh pr merge --admin`). Before creating or sharing a migration PR, run the focused migration guards: diff --git a/README.md b/README.md index 221e2a932..b95c00ee8 100644 --- a/README.md +++ b/README.md @@ -220,18 +220,16 @@ This will automatically format the code for you; no need to do anything else. ### Step 3: Changelogging -Finally, we ask contributors to make it clear for our users what changes have been made by contributing to a changelog. This changelog is formatted in YAML and describes the changes you've made to the code. This should follow the below format: +Finally, we ask contributors to make it clear for our users what changes have been made by adding a changelog fragment. Every PR adds at least one: a file directly in `changelog.d/`, named `changelog.d/..md`, that says in a sentence or two what changed. + +- `` is `breaking`, `added`, `changed`, `fixed` or `removed`. It sets the version bump: `breaking` raises the major version, `added` and `removed` the minor version, and `changed` and `fixed` the patch version. +- `` is letters and digits, in groups joined by single dots, hyphens or underscores, such as the issue number. ``` -- bump: {major, minor, patch} - changes: - {added, removed, changed, fixed}: - - +echo 'Report when the deployed version went live.' > changelog.d/4083.added.md ``` -For more info on the syntax, check out the [semantic versioning docs](https://www.semver.org) and [keep a changelog](https://www.keepachangelog.com). - -Write your changelog info into the empty file called `changelog_entry.yaml`. When you open your PR, this will automatically be added to the overall changelog. +The `Check changelog fragment` PR job fails unless your PR adds such a file, and fails on any other file in `changelog.d/`, such as one in a folder (`changelog.d/added/4083.md`) or an empty one. After the PR merges, the release builds the fragments into `CHANGELOG.md` with [Towncrier](https://towncrier.readthedocs.io) and removes them. See [the PR skill](docs/engineering/skills/github-prs.md#changelog-fragments) for the full rule. ## Opening a Pull Request diff --git a/changelog.d/changelog-fragment-check.fixed.md b/changelog.d/changelog-fragment-check.fixed.md new file mode 100644 index 000000000..140a33989 --- /dev/null +++ b/changelog.d/changelog-fragment-check.fixed.md @@ -0,0 +1 @@ +The changelog fragment check on pull requests accepts only fragments the release reads: the pull request must add a non-empty file directly in `changelog.d/`, named `..md` with one of the types in `[tool.towncrier]`. Fragments other pull requests left on master no longer count, and the version bump and Towncrier stop at any file in `changelog.d/` they can't read instead of leaving it out. diff --git a/docs/engineering/skills/github-prs.md b/docs/engineering/skills/github-prs.md index 2973ae04d..7691c6de1 100644 --- a/docs/engineering/skills/github-prs.md +++ b/docs/engineering/skills/github-prs.md @@ -11,6 +11,54 @@ Before opening a PR, all developers and agents must: 2. Put `Fixes #ISSUE_NUMBER` as the first line of the PR description, using the issue number from the previous step. The PR template starts with this line. +## Changelog fragments + +Every PR adds at least one changelog fragment of its own +(`api.process.changelog-fragment`). A fragment is a file directly in +`changelog.d/`, named `changelog.d/..md`: + +- `` is one of the types in `[tool.towncrier]` in `pyproject.toml`: + `breaking`, `added`, `changed`, `fixed` or `removed`, in lowercase. It sets + the release's version bump: `breaking` raises the major version, `added` and + `removed` the minor version, and `changed` and `fixed` the patch version. +- `` is letters and digits, in groups joined by single dots, hyphens or + underscores: an issue number such as `4083`, or a short label such as + `report-live-since`. A branch name with a `/` in it makes a folder, which + doesn't count. +- The file holds a sentence or two, in UTF-8, saying what changed for people + who use or run the Public API. An empty file, or one with only whitespace, + fails. + +For example: + +```bash +echo 'Report when the deployed version went live.' > changelog.d/4083.added.md +``` + +The `Check changelog fragment` job in `pr.yml` compares the PR with its base +commit (`github.event.pull_request.base.sha`): + +- It fails unless the PR adds such a file. Fragments already on master don't + count: they belong to the PRs that added them until the next release removes + them. Editing, deleting or renaming one of them doesn't count either, but + moving a file the release couldn't read to a fragment name does. +- It fails on every file in `changelog.d/` the release can't read: a file in a + folder (#4084's `changelog.d/added/4083.md` belonged at + `changelog.d/4083.added.md`), another name, an unknown or uppercase type, an + empty file, a symbolic link or a submodule. Each error says how to fix it. + +After a merge to master, the `versioning` job in `push.yml` runs +`.github/bump_version.py` and then Towncrier. The bump reads fragments through +the same definition as the check, `.github/changelog_fragments.py`, and stops +at any file it can't read; `ignore = [".gitkeep"]` makes Towncrier stop too. +A release therefore never leaves out a fragment. + +To run the check locally before pushing: + +```bash +BASE_SHA=origin/master bash .github/scripts/check_changelog_fragment.sh +``` + ## Branch Rules Agents must never bypass branch rules, even when their credentials have admin @@ -45,7 +93,8 @@ declares an API contract change. If a PR changes migration route contracts, route-group metadata, generated migration artifacts, or the cutover guardrails, it should: -1. Include a changelog fragment. +1. Include a changelog fragment (see + [Changelog fragments](#changelog-fragments)). 2. Refresh generated migration artifacts with `python scripts/export_migration_contracts.py`. 3. Run `python scripts/run_quality_guards.py`. diff --git a/docs/generated/rules.md b/docs/generated/rules.md index 5acfc789d..079f41b3f 100644 --- a/docs/generated/rules.md +++ b/docs/generated/rules.md @@ -470,7 +470,7 @@ flowchart LR | `api.agents.adapters-point-to-agents-md` | CLAUDE.md and .github/copilot-instructions.md only point to AGENTS.md; skills and rules are listed in AGENTS.md alone. | mechanical | enforced | yes | quality guard `rules-registry` | `pr.yml:quality-guards` | yes | | `api.process.agents-never-bypass` | Agents never bypass branch rules, even with admin credentials: no gh pr merge --admin and no bypass option; a blocked merge is reported to a human. | mechanical | doc-only | no | nothing | — | — | | `api.process.agents-suppress-as-last-resort` | Agents add an inline ruff suppression (noqa) only when neither a code change nor a ruff setting removes the finding, and a PR lists every inline suppression it adds, with the reason. | judgment | doc-only | no | nothing | — | — | -| `api.process.changelog-fragment` | Every PR adds a changelog.d fragment. | mechanical | enforced | yes | CI job `check-changelog` | `pr.yml:check-changelog` | — | +| `api.process.changelog-fragment` | Every PR adds, relative to its base commit, at least one changelog fragment of its own, a non-empty file directly in changelog.d/ named `..md` with a type from [tool.towncrier] in pyproject.toml, and leaves nothing else there but .gitkeep; the PR check and the release's version bump read fragments through one definition, .github/changelog_fragments.py. | mechanical | enforced | yes | CI job `check-changelog`, `versioning` | `pr.yml:check-changelog`, `push.yml:versioning` | yes | | `api.process.cloud-run-image-builds` | The Cloud Run container image builds. | mechanical | enforced | yes | CI job `test_cloud_run_container_builds` | `pr.yml:test_cloud_run_container_builds` | — | | `api.process.docker-image-builds` | The API Docker image builds. | mechanical | enforced | yes | CI job `test_container_builds` | `pr.yml:test_container_builds` | — | | `api.process.makefile-phony` | Command-only Makefile targets are declared in .PHONY, and .PHONY stays in sync when targets are renamed or removed. | mechanical | doc-only | no | nothing | — | — | @@ -488,8 +488,9 @@ flowchart LR gap_unenforced>"not enforced"] r_api_process_agents_suppress_as_last_resort("api.process.agents-suppress-as-last-resort (doc-only)") r_api_process_changelog_fragment("api.process.changelog-fragment (enforced)") - e_CI_job_check_changelog["CI job check-changelog"] + e_CI_job_check_changelog__versioning["CI job check-changelog, versioning"] j_CI_pr_yml_check_changelog["CI pr.yml:check-changelog"] + j_CI_push_yml_versioning["CI push.yml:versioning"] r_api_process_cloud_run_image_builds("api.process.cloud-run-image-builds (enforced)") e_CI_job_test_cloud_run_container_builds["CI job test_cloud_run_container_builds"] j_CI_pr_yml_test_cloud_run_container_builds["CI pr.yml:test_cloud_run_container_builds"] @@ -508,8 +509,9 @@ flowchart LR e_quality_guard_rules_registry --> j_CI_pr_yml_quality_guards r_api_process_agents_never_bypass -.-> gap_unenforced r_api_process_agents_suppress_as_last_resort -.-> gap_unenforced - r_api_process_changelog_fragment --> e_CI_job_check_changelog - e_CI_job_check_changelog --> j_CI_pr_yml_check_changelog + r_api_process_changelog_fragment --> e_CI_job_check_changelog__versioning + e_CI_job_check_changelog__versioning --> j_CI_pr_yml_check_changelog + e_CI_job_check_changelog__versioning --> j_CI_push_yml_versioning r_api_process_cloud_run_image_builds --> e_CI_job_test_cloud_run_container_builds e_CI_job_test_cloud_run_container_builds --> j_CI_pr_yml_test_cloud_run_container_builds r_api_process_docker_image_builds --> e_CI_job_test_container_builds diff --git a/rules/registry.toml b/rules/registry.toml index a10ccd0ba..37a41e923 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -1401,9 +1401,14 @@ kind = "none" [[rule]] id = "api.process.changelog-fragment" category = "process" -statement = "Every PR adds a changelog.d fragment." -rationale = "docs/engineering/skills/github-prs.md" -scope = ["changelog.d"] +statement = "Every PR adds, relative to its base commit, at least one changelog fragment of its own, a non-empty file directly in changelog.d/ named `..md` with a type from [tool.towncrier] in pyproject.toml, and leaves nothing else there but .gitkeep; the PR check and the release's version bump read fragments through one definition, .github/changelog_fragments.py." +rationale = "docs/engineering/skills/github-prs.md#changelog-fragments" +scope = [ + "changelog.d", + ".github/changelog_fragments.py", + ".github/bump_version.py", + ".github/scripts/check_changelog_fragment.sh", +] tier = "mechanical" status = "enforced" autofix = false @@ -1413,6 +1418,17 @@ kind = "workflow-job" workflow = ".github/workflows/pr.yml" job = "check-changelog" invocation = "check_changelog_fragment.sh" +[[rule.ci]] +workflow = ".github/workflows/push.yml" +job = "versioning" +invocation = "bump_version.py" +[rule.proof] +bad_fixture = "rules/fixtures/api.process.changelog-fragment/bad" +good_fixture = "rules/fixtures/api.process.changelog-fragment/good" +test = "tests/unit/test_changelog_fragments.py::test_rule_proof_changelog_fragment" +workflow = ".github/workflows/pr.yml" +job = "test" +invocation = "make test" [[rule]] id = "api.process.docker-image-builds" From f3b34ed70142fe7c97bfc628c030defcac01c755 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 29 Oct 2026 19:34:50 +0400 Subject: [PATCH 11/17] Test that a fragment of only invisible characters is refused Review finding: a fragment holding only a zero-width space, or other Unicode format characters such as a word joiner or a soft hyphen, passes the str.strip() test, and Towncrier publishes it as a bullet that looks empty. Require at least one visible character, and keep accepting text that has one. These fail because the definition only checks that stripping whitespace leaves something. Co-Authored-By: Claude Opus 5.5 --- tests/unit/test_changelog_fragments.py | 30 +++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/tests/unit/test_changelog_fragments.py b/tests/unit/test_changelog_fragments.py index de14ec437..34bd0cd24 100644 --- a/tests/unit/test_changelog_fragments.py +++ b/tests/unit/test_changelog_fragments.py @@ -32,6 +32,9 @@ f" is {LISTED_TYPES}, and is letters and digits, in groups " "joined by single dots, hyphens or underscores" ) +INVISIBLE_ONLY = ( + "holds no visible text, only invisible characters such as zero-width spaces." +) GENERIC_RENAME = f"Rename it to changelog.d/..md, where {FORM_TERMS}." @@ -302,8 +305,19 @@ def test_a_file_not_named_name_type_md_is_a_problem( "\ufeff\n", f"changelog.d/4083.added.md holds only whitespace. {EMPTY_ENTRY}", ), + ("\u200b\n", f"changelog.d/4083.added.md {INVISIBLE_ONLY} {EMPTY_ENTRY}"), + ( + "\ufeff\u2060\u200d \u00ad\n", + f"changelog.d/4083.added.md {INVISIBLE_ONLY} {EMPTY_ENTRY}", + ), + ], + ids=[ + "empty", + "whitespace-only", + "byte-order-mark-only", + "zero-width-space-only", + "format-characters-only", ], - ids=["empty", "whitespace-only", "byte-order-mark-only"], ) def test_a_fragment_without_text_is_a_problem( repository: Path, text: str, expected_problem: str @@ -316,6 +330,20 @@ def test_a_fragment_without_text_is_a_problem( assert scan.problems == {"changelog.d/4083.added.md": expected_problem} +@pytest.mark.parametrize( + "text", ["\u200bFixed.\n", "\u00e9\n", "\u2014\n", "7\n", "\u4e2d\u6587\n"] +) +def test_a_fragment_with_any_visible_character_is_read(repository: Path, text: str): + _write(repository, "changelog.d/4083.added.md", text) + + scan = _definition().scan(repository) + + assert scan.problems == {} + assert [fragment.path for fragment in scan.fragments] == [ + "changelog.d/4083.added.md" + ] + + def test_a_fragment_that_is_not_utf8_is_a_problem(repository: Path): (repository / "changelog.d/4083.added.md").write_bytes(b"caf\xe9\n") From c8995b0b889fcf5b77cd0bd97cf9bebec58e45fd Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 29 Oct 2026 19:35:07 +0400 Subject: [PATCH 12/17] Refuse a fragment with no visible character The definition now accepts a fragment's text only when it has a character in a visible Unicode category: a letter, mark, number, punctuation mark or symbol. A file of only whitespace, a byte-order mark or format characters such as a zero-width space gets "holds no visible text" or "holds only whitespace", in the PR check and in the version bump alike. Co-Authored-By: Claude Opus 5.5 --- .github/changelog_fragments.py | 26 ++++++++++++++++++++++---- docs/engineering/skills/github-prs.md | 4 ++-- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/.github/changelog_fragments.py b/.github/changelog_fragments.py index a97c258c2..6eebfbacc 100644 --- a/.github/changelog_fragments.py +++ b/.github/changelog_fragments.py @@ -17,7 +17,9 @@ underscores, such as ``4083`` or ``update-policyengine-bundle-6.11.1``; - ```` is the ``directory`` of one of the ``[[tool.towncrier.type]]`` entries: breaking, added, changed, fixed or removed; -- its text, read as UTF-8, is not empty or only whitespace. +- its text, read as UTF-8, has at least one visible character: it is not + empty, only whitespace or only invisible characters such as a zero-width + space. ``.gitkeep``, which keeps the directory in git once a release has removed every fragment, is the only other file allowed there. Pull request #4084 added @@ -41,6 +43,7 @@ import re import sys import tomllib +import unicodedata from dataclasses import dataclass, field from pathlib import Path, PurePosixPath @@ -58,6 +61,10 @@ ) TYPE_NAME = re.compile(r"[a-z]+") BYTE_ORDER_MARK = "\ufeff" +# Unicode general category groups a reader can see: letters, marks, numbers, +# punctuation and symbols. Separators (Z) and other characters (C: controls and +# format characters such as a zero-width space or a byte-order mark) aren't. +VISIBLE_CATEGORY_GROUPS = frozenset("LMNPS") NAME_TERMS = ( " is letters and digits, in groups joined by single dots, hyphens or " "underscores" @@ -161,11 +168,22 @@ def _content_problem(path: str, file: Path) -> str | None: text = file.read_text(encoding="utf-8") except UnicodeDecodeError: return f"{path} is not UTF-8 text. Save it as UTF-8." - if text.removeprefix(BYTE_ORDER_MARK).strip(): + if any( + unicodedata.category(character)[0] in VISIBLE_CATEGORY_GROUPS + for character in text + ): return None - emptiness = "is empty" if not text else "holds only whitespace" + if not text: + emptiness = "is empty." + elif not text.removeprefix(BYTE_ORDER_MARK).strip(): + emptiness = "holds only whitespace." + else: + emptiness = ( + "holds no visible text, only invisible characters such as zero-width " + "spaces." + ) return ( - f"{path} {emptiness}. Write a sentence or two in it saying what changed: " + f"{path} {emptiness} Write a sentence or two in it saying what changed: " "Towncrier would publish an empty entry." ) diff --git a/docs/engineering/skills/github-prs.md b/docs/engineering/skills/github-prs.md index 7691c6de1..03c39736f 100644 --- a/docs/engineering/skills/github-prs.md +++ b/docs/engineering/skills/github-prs.md @@ -26,8 +26,8 @@ Every PR adds at least one changelog fragment of its own `report-live-since`. A branch name with a `/` in it makes a folder, which doesn't count. - The file holds a sentence or two, in UTF-8, saying what changed for people - who use or run the Public API. An empty file, or one with only whitespace, - fails. + who use or run the Public API. An empty file, or one with only whitespace + or invisible characters such as a zero-width space, fails. For example: From a95087afb44041e101f6aef4cfb3affc8f9375bd Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 29 Oct 2026 19:36:03 +0400 Subject: [PATCH 13/17] Test that a fragment the base branch already released doesn't count Review finding: when a lower pull request in a stack is squash-merged and the release removes its fragment, an upper branch that wasn't rebased still holds the lower pull request's commits. GitHub's test merge commit then adds that fragment back, the check counted it as the upper pull request's own, and the next release would publish the entry a second time. Cover the merge commit, the pull request's own head, and a pull request that also adds its own fragment. These fail because the check doesn't look at what the base branch removed after the pull request's branch left it. Co-Authored-By: Claude Opus 5.5 --- tests/unit/test_changelog_fragment_check.py | 80 +++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/tests/unit/test_changelog_fragment_check.py b/tests/unit/test_changelog_fragment_check.py index 32abea027..8c027753b 100644 --- a/tests/unit/test_changelog_fragment_check.py +++ b/tests/unit/test_changelog_fragment_check.py @@ -29,6 +29,15 @@ PENDING_FRAGMENT = "changelog.d/telemetry-rank-cause-copies.fixed.md" OWN_FRAGMENT = "changelog.d/4114.fixed.md" PULL_REQUEST_4084_FRAGMENT = "changelog.d/added/4083.md" +LOWER_FRAGMENT = "changelog.d/4200.added.md" +ALREADY_RELEASED = ( + f"::error file={LOWER_FRAGMENT}::{LOWER_FRAGMENT} is a fragment the base " + "branch has already released: a release removed it after this branch left " + "the base branch, so merging would publish it again. This happens when a " + "pull request lower in a stack merged; rebase this branch onto the base " + "branch without that pull request's commits. If it is a new note that " + "reuses the name, rename it.\n" +) FRAGMENT_TEXT = "Report when the deployed version went live.\n" NO_OWN_FRAGMENT = "::error::This pull request adds no changelog fragment of its own." ON_THE_BASE_BRANCH = ( @@ -390,6 +399,71 @@ def test_a_pull_request_stacked_on_another_needs_its_own_fragment( assert NO_OWN_FRAGMENT in result.stdout +def _squash_merge_and_release_the_lower_pull_request(repository: Path) -> None: + """Leave the pull request branch on top of a lower one that merged and released. + + The lower pull request added LOWER_FRAGMENT in two commits. Master + squash-merged it, then the release commit removed every fragment. The pull + request branch, cut from the lower one and not rebased, still holds the + lower pull request's commits. + """ + _git(repository, "checkout", "--quiet", "-B", "lower", "master") + _write(repository, "code.txt", "The lower change.\n") + _write(repository, LOWER_FRAGMENT, "Lower change.\n") + _commit(repository, "The lower pull request") + _write(repository, LOWER_FRAGMENT, "Lower change, reworded.\n") + _commit(repository, "Reword the lower pull request's fragment") + _git(repository, "checkout", "--quiet", "-B", "pull-request") + _write(repository, "upper.txt", "The upper change.\n") + _commit(repository, "The upper pull request, without a fragment") + _git(repository, "checkout", "--quiet", "master") + _git(repository, "merge", "--quiet", "--squash", "lower") + _commit(repository, "Squash-merge the lower pull request") + _git(repository, "rm", "--quiet", PENDING_FRAGMENT, LOWER_FRAGMENT) + _commit(repository, "Update PolicyEngine API") + _git(repository, "checkout", "--quiet", "pull-request") + + +def test_on_the_merge_commit_a_fragment_already_released_does_not_count( + repository: Path, +): + _squash_merge_and_release_the_lower_pull_request(repository) + _merge_into_master(repository) + + result = _run_check(repository, _master(repository)) + + assert (repository / LOWER_FRAGMENT).is_file() + assert result.returncode == 1, result.stdout + assert ALREADY_RELEASED in result.stdout + assert NO_OWN_FRAGMENT in result.stdout + + +def test_at_the_pull_requests_head_a_fragment_already_released_does_not_count( + repository: Path, +): + _squash_merge_and_release_the_lower_pull_request(repository) + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 1, result.stdout + assert ALREADY_RELEASED in result.stdout + assert NO_OWN_FRAGMENT in result.stdout + + +def test_a_released_fragment_beside_the_pull_requests_own_still_fails( + repository: Path, +): + _squash_merge_and_release_the_lower_pull_request(repository) + _write(repository, OWN_FRAGMENT) + _commit(repository, "Add the upper pull request's fragment") + _merge_into_master(repository) + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 1, result.stdout + assert result.stdout == ALREADY_RELEASED + + def test_a_shallow_clone_fails_and_says_to_fetch_the_history( repository: Path, tmp_path: Path ): @@ -466,6 +540,12 @@ def test_the_paths_a_pull_request_adds( assert definition.added_paths(changes) == expected_added_paths +def test_the_released_paths_are_read_from_git_logs_nul_separated_names(): + assert fragments_module.parse_paths( + b"changelog.d/a.added.md\0\nchangelog.d/b.fixed.md\0\0" + ) == {"changelog.d/a.added.md", "changelog.d/b.fixed.md"} + + def test_name_status_output_that_ends_mid_record_is_refused(): with pytest.raises(ValueError, match="ends in the middle of a change"): fragments_module.parse_name_status(b"R100\0changelog.d/a.added.md\0") From 1770bb9f359fe380565dea18e20bd8c37557af2f Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 29 Oct 2026 19:37:09 +0400 Subject: [PATCH 14/17] Refuse a fragment the base branch has already released check_changelog_fragment.sh now finds the pull request's own head (the second parent of GitHub's test merge commit, or HEAD), and lists the fragments the base branch removed between the point the branch left it and BASE_SHA, with git log --diff-filter=D. changelog_fragments.py check-pull-request reads them with --released-paths: a fragment the pull request adds back is a problem that says to rebase, and it doesn't count as the pull request's own. A fragment the branch inherited from the base and the release removed stays removed in the merge, so it isn't affected. Co-Authored-By: Claude Opus 5.5 --- .github/changelog_fragments.py | 63 +++++++++++++++++---- .github/scripts/check_changelog_fragment.sh | 23 +++++++- docs/engineering/skills/github-prs.md | 7 ++- 3 files changed, 80 insertions(+), 13 deletions(-) diff --git a/.github/changelog_fragments.py b/.github/changelog_fragments.py index 6eebfbacc..a33ad8c6c 100644 --- a/.github/changelog_fragments.py +++ b/.github/changelog_fragments.py @@ -34,7 +34,10 @@ for the fragment directory on stdin, from the pull request's base to the checked-out commit, and exit 1 unless the pull request adds a fragment of its own and - every file in the directory is a fragment + every file in the directory is a fragment. + ``--released-paths FILE`` names the fragments the base + branch removed after the pull request's branch left it, + NUL-separated, as ``git log --name-only -z`` prints them """ from __future__ import annotations @@ -76,6 +79,13 @@ "It is already on the base branch, so the next release fails until a pull " "request fixes it." ) +ALREADY_RELEASED = ( + "is a fragment the base branch has already released: a release removed it " + "after this branch left the base branch, so merging would publish it again. " + "This happens when a pull request lower in a stack merged; rebase this " + "branch onto the base branch without that pull request's commits. If it is " + "a new note that reuses the name, rename it." +) class ConfigurationError(Exception): @@ -479,6 +489,18 @@ def parse_name_status(output: bytes) -> list[Change]: return changes +def parse_paths(output: bytes) -> set[str]: + """Return the NUL-separated paths in ``output``, as ``git log -z`` prints them. + + git separates one commit's paths from the next with a newline as well. + """ + return { + path.strip(b"\n").decode("utf-8", errors="surrogateescape") + for path in output.split(b"\0") + if path.strip(b"\n") + } + + def annotation_message(message: str) -> str: """Escape ``message`` so the Actions runner reads it as one annotation message.""" return message.replace("%", "%25").replace("\r", "%0D").replace("\n", "%0A") @@ -518,12 +540,16 @@ def _no_own_fragment_message( return " ".join(sentences) -def check_pull_request(root: Path, changes: list[Change]) -> int: +def check_pull_request( + root: Path, changes: list[Change], released_paths: set[str] | None = None +) -> int: """Check the checkout in ``root`` of a pull request that makes ``changes``. - Print an error annotation for each file in the fragment directory that is - not a fragment, and one if the pull request adds no fragment of its own; - return the exit status. + ``released_paths`` are the fragments the base branch removed after the pull + request's branch left it: one the pull request adds back is a problem, not + a fragment of its own. Print an error annotation for each file in the + fragment directory that is not a fragment, and one if the pull request + adds no fragment of its own; return the exit status. """ definition = FragmentDefinition.load(root) scan = definition.scan(root) @@ -544,13 +570,18 @@ def check_pull_request(root: Path, changes: list[Change]) -> int: (path, definition.empty_folder_problem(path)) for path in sorted(changed_paths - read_paths) ) - for path, problem in problems.items(): - _print_error(problem, path) - added_paths = definition.added_paths(changes) + resurrected_paths = added_paths & (released_paths or set()) + problems.update( + (path, f"{path} {ALREADY_RELEASED}") for path in sorted(resurrected_paths) + ) own_fragments = [ - fragment for fragment in scan.fragments if fragment.path in added_paths + fragment + for fragment in scan.fragments + if fragment.path in added_paths - resurrected_paths ] + for path, problem in problems.items(): + _print_error(problem, path) if not own_fragments: _print_error(_no_own_fragment_message(definition, scan, changes)) if problems or not own_fragments: @@ -573,10 +604,15 @@ def main(arguments: list[str] | None = None) -> int: parser = argparse.ArgumentParser(description=__doc__.split("\n", 1)[0]) commands = parser.add_subparsers(dest="command", required=True) commands.add_parser("directory", help="print the fragment directory") - commands.add_parser( + check_parser = commands.add_parser( "check-pull-request", help="check a pull request; git diff --name-status -z output on stdin", ) + check_parser.add_argument( + "--released-paths", + type=Path, + help="file of fragments the base branch removed since the branch left it", + ) options = parser.parse_args(arguments) root = Path.cwd() try: @@ -584,7 +620,12 @@ def main(arguments: list[str] | None = None) -> int: print(FragmentDefinition.load(root).directory) return 0 changes = parse_name_status(sys.stdin.buffer.read()) - return check_pull_request(root, changes) + released_paths = ( + parse_paths(options.released_paths.read_bytes()) + if options.released_paths + else set() + ) + return check_pull_request(root, changes, released_paths) except (ConfigurationError, ValueError) as error: _print_error(str(error)) return 1 diff --git a/.github/scripts/check_changelog_fragment.sh b/.github/scripts/check_changelog_fragment.sh index 9eceee793..22144638b 100644 --- a/.github/scripts/check_changelog_fragment.sh +++ b/.github/scripts/check_changelog_fragment.sh @@ -18,6 +18,12 @@ # base. Checked out at the pull request's own head, it is what the branch has # added since it left the base branch. # +# It also lists the fragments the base branch removed after the pull +# request's branch left it; a release removes each fragment it builds. A +# branch that still holds the commits of a lower pull request in a stack, +# squash-merged and released since, would add that fragment back, so it must +# not count. +# # Run from the repository root, in a clone with the base branch's history # (actions/checkout with fetch-depth: 0). # @@ -50,5 +56,20 @@ if ! fragment_directory="$("${python_bin}" "${fragments_module}" directory)"; th exit 1 fi +# The pull request's own head: the second parent of GitHub's test merge +# commit, or HEAD when HEAD is the head itself. +pull_request_head="HEAD" +if git rev-parse --verify --quiet "HEAD^2" >/dev/null \ + && [[ "$(git rev-parse "HEAD^1")" == "$(git rev-parse "${base_revision}^{commit}")" ]]; then + pull_request_head="HEAD^2" +fi +branch_point="$(git merge-base "${base_revision}" "${pull_request_head}")" + +released_paths_file="$(mktemp)" +trap 'rm -f "${released_paths_file}"' EXIT +git log --diff-filter=D --no-renames --name-only -z --format= \ + "${branch_point}..${base_revision}" -- "${fragment_directory}" >"${released_paths_file}" + git diff --find-renames --name-status -z "${base_commit}" HEAD -- "${fragment_directory}" \ - | "${python_bin}" "${fragments_module}" check-pull-request + | "${python_bin}" "${fragments_module}" check-pull-request \ + --released-paths "${released_paths_file}" diff --git a/docs/engineering/skills/github-prs.md b/docs/engineering/skills/github-prs.md index 03c39736f..18658f97e 100644 --- a/docs/engineering/skills/github-prs.md +++ b/docs/engineering/skills/github-prs.md @@ -41,7 +41,12 @@ commit (`github.event.pull_request.base.sha`): - It fails unless the PR adds such a file. Fragments already on master don't count: they belong to the PRs that added them until the next release removes them. Editing, deleting or renaming one of them doesn't count either, but - moving a file the release couldn't read to a fragment name does. + moving a file the release couldn't read to a fragment name does. git decides + what is a rename: a file deleted and one added with mostly the same text. +- A fragment the base branch has already released doesn't count, and fails. + When a lower PR in a stack is squash-merged, the release removes its + fragment; an upper branch that still holds the lower PR's commits would add + it back. Rebase the upper branch onto master without those commits. - It fails on every file in `changelog.d/` the release can't read: a file in a folder (#4084's `changelog.d/added/4083.md` belonged at `changelog.d/4083.added.md`), another name, an unknown or uppercase type, an From 34c9a40df1fc6f3a92fcc5abfad16512f429eeb9 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 29 Oct 2026 19:41:34 +0400 Subject: [PATCH 15/17] Test that only a released fragment's own text counts as released Second review finding: the released-fragment check compared paths only, so a new fragment that reused the name of another pull request's fragment, released after this branch was cut, was refused as if merging would publish the old entry again. The "no fragment of its own" message also listed a released fragment as already on the base branch. A fragment counts as released only when its text is the blob the base branch removed; read those blobs from git log --raw, and compute a file's blob id as git hash-object does. These fail because the check still matches released fragments by path. Co-Authored-By: Claude Opus 5.5 --- tests/unit/test_changelog_fragment_check.py | 61 +++++++++++++++++++-- 1 file changed, 57 insertions(+), 4 deletions(-) diff --git a/tests/unit/test_changelog_fragment_check.py b/tests/unit/test_changelog_fragment_check.py index 8c027753b..2ffea0613 100644 --- a/tests/unit/test_changelog_fragment_check.py +++ b/tests/unit/test_changelog_fragment_check.py @@ -464,6 +464,43 @@ def test_a_released_fragment_beside_the_pull_requests_own_still_fails( assert result.stdout == ALREADY_RELEASED +def test_a_new_fragment_that_reuses_a_released_fragments_name_counts( + repository: Path, +): + _write(repository, "changelog.d/fix-typo.fixed.md", "Fix the household form.\n") + _commit(repository, "This pull request, with its fragment") + _git(repository, "checkout", "--quiet", "-b", "other", "master") + _write(repository, "changelog.d/fix-typo.fixed.md", "Fix the report title.\n") + _commit(repository, "Another pull request, with a fragment of the same name") + _git(repository, "checkout", "--quiet", "master") + _git(repository, "merge", "--quiet", "--no-ff", "-m", "Merge the other", "other") + _git(repository, "rm", "--quiet", PENDING_FRAGMENT, "changelog.d/fix-typo.fixed.md") + _commit(repository, "Update PolicyEngine API") + _merge_into_master(repository) + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 0, result.stdout + assert result.stdout == ( + "This pull request adds 1 changelog fragment: " + "changelog.d/fix-typo.fixed.md (fixed).\n" + ) + + +def test_a_released_fragment_is_not_listed_as_on_the_base_branch( + repository: Path, +): + _squash_merge_and_release_the_lower_pull_request(repository) + _merge_into_master(repository) + + result = _run_check(repository, _master(repository)) + + no_own_fragment = next( + line for line in result.stdout.splitlines() if NO_OWN_FRAGMENT in line + ) + assert "Fragments already on the base branch" not in no_own_fragment + + def test_a_shallow_clone_fails_and_says_to_fetch_the_history( repository: Path, tmp_path: Path ): @@ -540,10 +577,26 @@ def test_the_paths_a_pull_request_adds( assert definition.added_paths(changes) == expected_added_paths -def test_the_released_paths_are_read_from_git_logs_nul_separated_names(): - assert fragments_module.parse_paths( - b"changelog.d/a.added.md\0\nchangelog.d/b.fixed.md\0\0" - ) == {"changelog.d/a.added.md", "changelog.d/b.fixed.md"} +def test_the_released_fragments_are_read_from_git_logs_raw_deletions(): + removed_blob = "212c395e8d8e25de20d96d1d6f24f6a42cdecd1c" + other_blob = "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391" + raw_log = ( + f":100644 000000 {removed_blob} {'0' * 40} D\0changelog.d/a.added.md\0" + f"\n:100644 000000 {other_blob} {'0' * 40} D\0changelog.d/b.fixed.md\0" + ).encode() + + assert fragments_module.parse_removed_blobs(raw_log) == { + "changelog.d/a.added.md": {removed_blob}, + "changelog.d/b.fixed.md": {other_blob}, + } + + +def test_a_files_git_blob_id_is_computed_as_git_hash_object_does(tmp_path: Path): + fragment = tmp_path / "4114.fixed.md" + fragment.write_bytes(b"A change.\n") + expected_blob = _git(tmp_path, "hash-object", str(fragment)) + + assert fragments_module.git_blob_id(fragment, len(expected_blob)) == expected_blob def test_name_status_output_that_ends_mid_record_is_refused(): From 8919c328272e929123ea337da90e8b6d3d3c399d Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 29 Oct 2026 19:42:36 +0400 Subject: [PATCH 16/17] Count a fragment as released only when it holds the released text check_changelog_fragment.sh now passes git log --raw deletions, which carry the blob each removed fragment held, as --released-fragments. changelog_fragments.py computes the blob id of each fragment the pull request adds, as git hash-object does (SHA-1, or SHA-256 when the ids are that long), and refuses it as already released only when it matches a blob the base branch removed at that path. A new fragment that reuses a released name now counts, and a released fragment is no longer listed as already on the base branch. Co-Authored-By: Claude Opus 5.5 --- .github/changelog_fragments.py | 110 ++++++++++++++------ .github/scripts/check_changelog_fragment.sh | 18 ++-- docs/engineering/skills/github-prs.md | 3 +- 3 files changed, 92 insertions(+), 39 deletions(-) diff --git a/.github/changelog_fragments.py b/.github/changelog_fragments.py index a33ad8c6c..eee415c04 100644 --- a/.github/changelog_fragments.py +++ b/.github/changelog_fragments.py @@ -35,14 +35,15 @@ request's base to the checked-out commit, and exit 1 unless the pull request adds a fragment of its own and every file in the directory is a fragment. - ``--released-paths FILE`` names the fragments the base - branch removed after the pull request's branch left it, - NUL-separated, as ``git log --name-only -z`` prints them + ``--released-fragments FILE`` holds the fragments the + base branch removed after the pull request's branch left + it, as ``git log --diff-filter=D --raw -z`` prints them """ from __future__ import annotations import argparse +import hashlib import re import sys import tomllib @@ -75,6 +76,8 @@ # git diff --name-status statuses that name two paths: the source, then the # destination. Every other status names one. TWO_PATH_STATUSES = frozenset({"R", "C"}) +# Hexadecimal digits in a SHA-256 git object id; a SHA-1 id has 40. +SHA256_OBJECT_ID_LENGTH = 64 ON_THE_BASE_BRANCH = ( "It is already on the base branch, so the next release fails until a pull " "request fixes it." @@ -489,16 +492,55 @@ def parse_name_status(output: bytes) -> list[Change]: return changes -def parse_paths(output: bytes) -> set[str]: - """Return the NUL-separated paths in ``output``, as ``git log -z`` prints them. +def parse_removed_blobs(output: bytes) -> dict[str, set[str]]: + """Return the blob ids each path held when deleted, from ``git log --raw -z``. - git separates one commit's paths from the next with a newline as well. + Each deletion is a ``: D`` record and + then its path, NUL-separated; git separates one commit's records from the + next with a newline as well. """ - return { - path.strip(b"\n").decode("utf-8", errors="surrogateescape") - for path in output.split(b"\0") - if path.strip(b"\n") - } + fields = [ + output_field.strip(b"\n") + for output_field in output.split(b"\0") + if output_field.strip(b"\n") + ] + if len(fields) % 2 or not all(record.startswith(b":") for record in fields[::2]): + msg = f"git log --raw output is not records and paths: {fields!r}" + raise ValueError(msg) + removed_blobs: dict[str, set[str]] = {} + for record, path in zip(fields[::2], fields[1::2], strict=True): + removed_blob = record.split()[2].decode("ascii") + decoded_path = path.decode("utf-8", errors="surrogateescape") + removed_blobs.setdefault(decoded_path, set()).add(removed_blob) + return removed_blobs + + +def git_blob_id(file: Path, id_length: int) -> str: + """Return the id git gives ``file``'s bytes as a blob, as ``git hash-object``. + + ``id_length`` is the length of the repository's object ids: 40 for SHA-1, + 64 for SHA-256. + """ + data = file.read_bytes() + blob = b"blob %d\0" % len(data) + data + if id_length == SHA256_OBJECT_ID_LENGTH: + return hashlib.sha256(blob).hexdigest() + return hashlib.sha1(blob, usedforsecurity=False).hexdigest() + + +def _released_paths( + root: Path, added_paths: set[str], removed_blobs: dict[str, set[str]] +) -> set[str]: + """Return the added paths whose text is a blob the base branch removed there.""" + released = set() + for path in added_paths & set(removed_blobs): + file = root / path + if file.is_symlink() or not file.is_file(): + continue + id_length = len(next(iter(removed_blobs[path]))) + if git_blob_id(file, id_length) in removed_blobs[path]: + released.add(path) + return released def annotation_message(message: str) -> str: @@ -518,11 +560,13 @@ def _print_error(message: str, path: str | None = None) -> None: def _no_own_fragment_message( - definition: FragmentDefinition, scan: Scan, changes: list[Change] + definition: FragmentDefinition, pending: list[str], changes: list[Change] ) -> str: - """Return the error for a pull request that adds no fragment of its own.""" + """Return the error for a pull request that adds no fragment of its own. + + ``pending`` lists the fragments already on the base branch. + """ sentences = ["This pull request adds no changelog fragment of its own."] - pending = [fragment.path for fragment in scan.fragments] if pending: sentences.append( "Fragments already on the base branch belong to the pull requests " @@ -541,15 +585,18 @@ def _no_own_fragment_message( def check_pull_request( - root: Path, changes: list[Change], released_paths: set[str] | None = None + root: Path, + changes: list[Change], + removed_blobs: dict[str, set[str]] | None = None, ) -> int: """Check the checkout in ``root`` of a pull request that makes ``changes``. - ``released_paths`` are the fragments the base branch removed after the pull - request's branch left it: one the pull request adds back is a problem, not - a fragment of its own. Print an error annotation for each file in the - fragment directory that is not a fragment, and one if the pull request - adds no fragment of its own; return the exit status. + ``removed_blobs`` maps each fragment the base branch removed after the + pull request's branch left it to the blobs it held. A fragment the pull + request adds back with that text is a problem, not a fragment of its own. + Print an error annotation for each file in the fragment directory that is + not a fragment, and one if the pull request adds no fragment of its own; + return the exit status. """ definition = FragmentDefinition.load(root) scan = definition.scan(root) @@ -571,7 +618,7 @@ def check_pull_request( for path in sorted(changed_paths - read_paths) ) added_paths = definition.added_paths(changes) - resurrected_paths = added_paths & (released_paths or set()) + resurrected_paths = _released_paths(root, added_paths, removed_blobs or {}) problems.update( (path, f"{path} {ALREADY_RELEASED}") for path in sorted(resurrected_paths) ) @@ -583,7 +630,12 @@ def check_pull_request( for path, problem in problems.items(): _print_error(problem, path) if not own_fragments: - _print_error(_no_own_fragment_message(definition, scan, changes)) + pending = [ + fragment.path + for fragment in scan.fragments + if fragment.path not in resurrected_paths + ] + _print_error(_no_own_fragment_message(definition, pending, changes)) if problems or not own_fragments: return 1 @@ -609,9 +661,9 @@ def main(arguments: list[str] | None = None) -> int: help="check a pull request; git diff --name-status -z output on stdin", ) check_parser.add_argument( - "--released-paths", + "--released-fragments", type=Path, - help="file of fragments the base branch removed since the branch left it", + help="git log --raw -z deletions on the base branch since the branch left it", ) options = parser.parse_args(arguments) root = Path.cwd() @@ -620,12 +672,12 @@ def main(arguments: list[str] | None = None) -> int: print(FragmentDefinition.load(root).directory) return 0 changes = parse_name_status(sys.stdin.buffer.read()) - released_paths = ( - parse_paths(options.released_paths.read_bytes()) - if options.released_paths - else set() + removed_blobs = ( + parse_removed_blobs(options.released_fragments.read_bytes()) + if options.released_fragments + else {} ) - return check_pull_request(root, changes, released_paths) + return check_pull_request(root, changes, removed_blobs) except (ConfigurationError, ValueError) as error: _print_error(str(error)) return 1 diff --git a/.github/scripts/check_changelog_fragment.sh b/.github/scripts/check_changelog_fragment.sh index 22144638b..ba621218c 100644 --- a/.github/scripts/check_changelog_fragment.sh +++ b/.github/scripts/check_changelog_fragment.sh @@ -19,10 +19,10 @@ # added since it left the base branch. # # It also lists the fragments the base branch removed after the pull -# request's branch left it; a release removes each fragment it builds. A -# branch that still holds the commits of a lower pull request in a stack, -# squash-merged and released since, would add that fragment back, so it must -# not count. +# request's branch left it, with the blob each one held; a release removes +# each fragment it builds. A branch that still holds the commits of a lower +# pull request in a stack, squash-merged and released since, would add that +# fragment back with the same text, so it must not count. # # Run from the repository root, in a clone with the base branch's history # (actions/checkout with fetch-depth: 0). @@ -65,11 +65,11 @@ if git rev-parse --verify --quiet "HEAD^2" >/dev/null \ fi branch_point="$(git merge-base "${base_revision}" "${pull_request_head}")" -released_paths_file="$(mktemp)" -trap 'rm -f "${released_paths_file}"' EXIT -git log --diff-filter=D --no-renames --name-only -z --format= \ - "${branch_point}..${base_revision}" -- "${fragment_directory}" >"${released_paths_file}" +released_fragments_file="$(mktemp)" +trap 'rm -f "${released_fragments_file}"' EXIT +git log --diff-filter=D --no-renames --raw --no-abbrev -z --format= \ + "${branch_point}..${base_revision}" -- "${fragment_directory}" >"${released_fragments_file}" git diff --find-renames --name-status -z "${base_commit}" HEAD -- "${fragment_directory}" \ | "${python_bin}" "${fragments_module}" check-pull-request \ - --released-paths "${released_paths_file}" + --released-fragments "${released_fragments_file}" diff --git a/docs/engineering/skills/github-prs.md b/docs/engineering/skills/github-prs.md index 18658f97e..ef00824a0 100644 --- a/docs/engineering/skills/github-prs.md +++ b/docs/engineering/skills/github-prs.md @@ -46,7 +46,8 @@ commit (`github.event.pull_request.base.sha`): - A fragment the base branch has already released doesn't count, and fails. When a lower PR in a stack is squash-merged, the release removes its fragment; an upper branch that still holds the lower PR's commits would add - it back. Rebase the upper branch onto master without those commits. + it back with the same text. Rebase the upper branch onto master without + those commits. A new fragment that only reuses a released one's name counts. - It fails on every file in `changelog.d/` the release can't read: a file in a folder (#4084's `changelog.d/added/4083.md` belonged at `changelog.d/4083.added.md`), another name, an unknown or uppercase type, an From 9aeb9d3939b6e5d88f1cd1f2e10ab1ac7bb303c4 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 29 Oct 2026 19:48:23 +0400 Subject: [PATCH 17/17] Say "created from" for a test branch's starting point Two test docstrings described where a pull request branch starts with a word that also means slicing; name the operation instead. Co-Authored-By: Claude Opus 5.5 --- tests/unit/test_changelog_fragment_check.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/test_changelog_fragment_check.py b/tests/unit/test_changelog_fragment_check.py index 2ffea0613..184f8a702 100644 --- a/tests/unit/test_changelog_fragment_check.py +++ b/tests/unit/test_changelog_fragment_check.py @@ -84,7 +84,7 @@ def _commit(repository: Path, message: str) -> str: @pytest.fixture def repository(tmp_path: Path) -> Path: - """Return a repository on a pull request branch cut from master. + """Return a repository on a pull request branch created from master. Master holds the repository's pyproject.toml, changelog.d/.gitkeep and one pending fragment from an earlier pull request. @@ -404,7 +404,7 @@ def _squash_merge_and_release_the_lower_pull_request(repository: Path) -> None: The lower pull request added LOWER_FRAGMENT in two commits. Master squash-merged it, then the release commit removed every fragment. The pull - request branch, cut from the lower one and not rebased, still holds the + request branch, created from the lower one and not rebased, still holds the lower pull request's commits. """ _git(repository, "checkout", "--quiet", "-B", "lower", "master")