From 03d8c9a76d10287bd61e4da3c5318771be34945a Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 22:11:37 +0000 Subject: [PATCH 1/3] feat(reflex-release): name orphan news fragments after their pull request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An orphan fragment (`+something.feature.md`) is meant to be renamed once the pull request number is known. When it isn't, the entry towncrier writes has no link, and the number is lost for good once the fragment is consumed. `materialize` now recovers it: for every package it is about to build, each leftover orphan fragment is renamed after the pull request whose commit added it, read out of that commit's subject (`Merge pull request #N ...` for a merge commit, `... (#N)` for a squash). The rename goes through `git mv`, so the deletion of the old path is staged and towncrier's own `git rm` of the renamed fragment leaves nothing behind. A pull request that left two fragments of one type behind gets towncrier's `...md` counter form rather than colliding. Fragments whose number cannot be recovered — a commit that landed outside a pull request, or a fragment that was never committed — keep their orphan name and render as before, with a warning annotation naming the file. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GPtjZej5GJQSWoGcG2Cxgf --- CONTRIBUTING.md | 2 +- packages/reflex-release/README.md | 16 +- ...+orphan-fragment-pr-association.feature.md | 1 + .../src/reflex_release/actions.py | 9 + .../src/reflex_release/commands.py | 9 +- .../src/reflex_release/discovery.py | 148 +++++++++++- .../src/reflex_release/gitutil.py | 56 +++++ tests/units/reflex_release/test_commands.py | 50 +++++ tests/units/reflex_release/test_discovery.py | 212 +++++++++++++++++- 9 files changed, 492 insertions(+), 11 deletions(-) create mode 100644 packages/reflex-release/news/+orphan-fragment-pr-association.feature.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b6997b99975..53dfce66d53 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -84,7 +84,7 @@ uv run towncrier create --config pyproject.toml --dir packages/reflex-components Drop `--dir` for a fragment against the main `reflex` package. -If you don't yet know the PR number, use an [orphan fragment](https://towncrier.readthedocs.io/en/stable/cli.html#towncrier-create) (`+.feature.md`) and rename it after opening the PR. +If you don't yet know the PR number, use an [orphan fragment](https://towncrier.readthedocs.io/en/stable/cli.html#towncrier-create) (`+.feature.md`). Renaming it after opening the PR is nice, but not required: the release workflow renames any orphan fragment that made it to `main` after the PR that merged it, so the changelog entry still links to it. **Skipping the fragment check:** for PRs that are genuinely not user-facing (CI-only tweaks, script fixes, test-only changes), apply the `skip-changelog` label on the PR to bypass the changelog CI check. diff --git a/packages/reflex-release/README.md b/packages/reflex-release/README.md index 9d6304cae9b..a4bf5ee8078 100644 --- a/packages/reflex-release/README.md +++ b/packages/reflex-release/README.md @@ -241,10 +241,16 @@ uvx reflex-release create 1234.feature.md # root package uvx reflex-release create --package widget-core 1234.bugfix.md # sub-package ``` -Before you know the PR number, use an orphan fragment (`+something.feature.md`) -and rename it later. CI requires a fragment for every package whose source the -PR touches; the `skip-changelog` label waives that for changes that genuinely -are not user-facing. +Before you know the PR number, use an orphan fragment (`+something.feature.md`). +Renaming it once the PR exists is nice but optional: when the release +materializes the changelog, every orphan fragment left over is renamed after the +pull request whose commit added it — read out of that commit's subject, which +GitHub writes as `Merge pull request #N ...` or `... (#N)` — so its entry gets +the usual link. A fragment whose commit +landed outside a pull request keeps its orphan name and its entry gets no link, +with a warning in the job log. CI requires a fragment for every package whose +source the PR touches; the `skip-changelog` label waives that for changes that +genuinely are not user-facing. ## Adding sub-packages @@ -764,7 +770,7 @@ a flag for running the same command by hand. | `create [--package P] NAME` | Create a news fragment. | | `packages` | List releasable packages. | | `plan` | Compute the next version of each selected package. | -| `materialize` | Run towncrier and (for `release-from-prerelease`) collapse alphas. | +| `materialize` | Name orphan fragments after their PR, run towncrier and (for `release-from-prerelease`) collapse alphas. | | `open-release-pr` / `push-prerelease` | Commit the changelogs and deliver them. | | `detect` | List packages whose newest changelog version has no tag. | | `prepare-publish` | Validate a package/version and emit build metadata. | diff --git a/packages/reflex-release/news/+orphan-fragment-pr-association.feature.md b/packages/reflex-release/news/+orphan-fragment-pr-association.feature.md new file mode 100644 index 00000000000..1f279884bfc --- /dev/null +++ b/packages/reflex-release/news/+orphan-fragment-pr-association.feature.md @@ -0,0 +1 @@ +Materializing a changelog now names leftover orphan news fragments (`+something.feature.md`) after the pull request whose commit added them, so their entries get the usual `#`-link instead of shipping unlinked. diff --git a/packages/reflex-release/src/reflex_release/actions.py b/packages/reflex-release/src/reflex_release/actions.py index 545c5464906..e1968c17130 100644 --- a/packages/reflex-release/src/reflex_release/actions.py +++ b/packages/reflex-release/src/reflex_release/actions.py @@ -47,6 +47,15 @@ def notice(message: str) -> None: sys.stdout.write(f"::notice::{message}\n") +def warning(message: str) -> None: + """Emit a GitHub Actions warning annotation (plain line outside Actions). + + Args: + message: The warning text. + """ + sys.stdout.write(f"::warning::{message}\n") + + def error(message: str) -> None: """Emit a GitHub Actions error annotation (plain line outside Actions). diff --git a/packages/reflex-release/src/reflex_release/commands.py b/packages/reflex-release/src/reflex_release/commands.py index 6b6540718f5..bdaa9b0721b 100644 --- a/packages/reflex-release/src/reflex_release/commands.py +++ b/packages/reflex-release/src/reflex_release/commands.py @@ -35,6 +35,7 @@ from .config import POST_RELEASE_INPUTS, POST_RELEASE_WORKFLOW_KEY, Config, is_final from .discovery import ( alpha_train_packages, + associate_orphan_fragments, build_changelog, category_order, changelog_packages, @@ -328,8 +329,10 @@ def cmd_plan(config: Config, action: str, selection: str) -> None: def cmd_materialize(config: Config, action: str, releases_json: str) -> None: """Write the planned versions into the changelogs via towncrier. - For ``release-from-prerelease``, collapses the alpha sections of each - changelog into the single final-version section after building it. + Orphan fragments are first named after the pull request that added them, so + the entries towncrier writes carry a link. For ``release-from-prerelease``, + collapses the alpha sections of each changelog into the single final-version + section after building it. Args: config: The repository configuration. @@ -346,6 +349,8 @@ def cmd_materialize(config: Config, action: str, releases_json: str) -> None: for release in releases: package, version = release["package"], release["next"] + for old_name, new_name in associate_orphan_fragments(config, package): + echo(f"{package}: renamed news fragment {old_name} -> {new_name}") build_changelog(config, package, version, today) if collapse: path = config.changelog_path(package) diff --git a/packages/reflex-release/src/reflex_release/discovery.py b/packages/reflex-release/src/reflex_release/discovery.py index 21b750df8bd..f912a831fae 100644 --- a/packages/reflex-release/src/reflex_release/discovery.py +++ b/packages/reflex-release/src/reflex_release/discovery.py @@ -2,16 +2,27 @@ from __future__ import annotations +import re import subprocess import sys from pathlib import Path from packaging.version import Version -from .actions import fail +from .actions import fail, warning from .changelog import DEFAULT_TITLE_FORMAT, latest_version from .config import Config, load_pyproject -from .gitutil import latest_tag_version +from .gitutil import adding_commit_message, latest_tag_version, move_file + +#: towncrier's default prefix for fragments written before the number is known. +DEFAULT_ORPHAN_PREFIX = "+" + +#: The two shapes GitHub itself writes a pull request number in: the subject of +#: a merge commit, and the suffix a squash merge appends to the subject. +_PR_SUBJECT_PATTERNS = ( + re.compile(r"^Merge pull request #(\d+)\b"), + re.compile(r"\(#(\d+)\)$"), +) def towncrier_table(config: Config) -> dict: @@ -60,6 +71,22 @@ def title_format(config: Config) -> str: return configured or DEFAULT_TITLE_FORMAT +def orphan_prefix(config: Config) -> str: + """Return the prefix marking a fragment as having no issue number yet. + + Args: + config: The repository configuration. + + Returns: + The configured ``orphan_prefix``, or towncrier's default. Empty when the + repository disables orphan fragments. + """ + configured = towncrier_table(config).get("orphan_prefix", DEFAULT_ORPHAN_PREFIX) + if not isinstance(configured, str): + fail(f"[tool.towncrier] orphan_prefix must be a string: {configured!r}") + return configured + + def category_order(config: Config) -> list[str]: """Read the towncrier category display names, in configured order. @@ -108,6 +135,123 @@ def has_pending_fragments(news_dir: Path, types: set[str]) -> bool: ) +def split_fragment_name(basename: str, types: set[str]) -> tuple[str, str] | None: + """Split a fragment filename into its issue part and the rest. + + Mirrors towncrier's own parsing: the fragment type is the last + dot-separated part naming a configured type, everything before it is the + issue and everything from it onwards is the suffix (type, optional counter, + extension). + + Args: + basename: The fragment filename. + types: The configured fragment type keys. + + Returns: + An ``(issue, suffix)`` tuple, or None when no part names a type. + """ + parts = basename.split(".") + for index in reversed(range(1, len(parts))): + if parts[index] in types: + return ".".join(parts[:index]), ".".join(parts[index:]) + return None + + +def pull_request_number(message: str) -> str | None: + """Extract the pull request number a commit message refers to. + + Only the two shapes GitHub writes itself are recognized, and only in the + subject: a number elsewhere in the message is a reference (``Fixes #12``), + not the pull request the commit landed through. + + Args: + message: The full commit message. + + Returns: + The pull request number, or None when the subject names none. + """ + subject = message.strip().partition("\n")[0].strip() + for pattern in _PR_SUBJECT_PATTERNS: + match = pattern.search(subject) + if match: + return match[1] + return None + + +def _unused_fragment_name(taken: set[str], issue: str, suffix: str) -> str: + """Return a fragment filename for an issue that no file in the directory uses. + + towncrier keys fragments by ``(issue, type, counter)``, so a second fragment + for the same pull request and type takes the ``...`` + counter form instead of colliding with the first. + + Args: + taken: The filenames already present in the news directory. + issue: The issue (pull request) number to name the fragment after. + suffix: The fragment suffix (type, optional counter, extension). + + Returns: + A filename that is not in ``taken``. + """ + candidate = f"{issue}.{suffix}" + fragment_type, _, rest = suffix.partition(".") + counter = 0 + while candidate in taken: + counter += 1 + candidate = ".".join(filter(None, (issue, fragment_type, str(counter), rest))) + return candidate + + +def associate_orphan_fragments(config: Config, package: str) -> list[tuple[str, str]]: + """Rename a package's orphan fragments after the pull request that added them. + + A contributor who does not know the number yet writes ``+something.feature.md`` + and is supposed to rename it once the pull request exists; when they don't, + the changelog entry ships with no link. The commit that added the fragment + knows the number, so it is recovered here instead — right before towncrier + consumes the fragments, so the entry gets its link. + + Fragments whose number cannot be recovered (added by a commit that did not + land through a pull request, or not committed at all) are left alone, with a + warning: towncrier renders them as entries without a link. + + Args: + config: The repository configuration. + package: The package name. + + Returns: + The ``(old name, new name)`` pairs that were renamed. + """ + news_dir = config.news_dir(package) + prefix = orphan_prefix(config) + if not prefix or not news_dir.is_dir(): + return [] + types = fragment_types(config) + fragments = sorted(news_dir.iterdir()) + taken = {path.name for path in fragments} + renamed: list[tuple[str, str]] = [] + for path in fragments: + if not path.name.startswith(prefix) or not path.is_file(): + continue + parsed = split_fragment_name(path.name, types) + if parsed is None: + continue + rel_path = path.relative_to(config.root).as_posix() + message = adding_commit_message(config.root, rel_path) + number = pull_request_number(message) if message else None + if number is None: + warning( + f"{rel_path}: no pull request found for the commit that added " + "this orphan fragment; its changelog entry will have no link." + ) + continue + new_name = _unused_fragment_name(taken, number, parsed[1]) + move_file(config.root, path, news_dir / new_name) + taken.add(new_name) + renamed.append((path.name, new_name)) + return renamed + + def changelog_packages(config: Config) -> list[str]: """List the packages that maintain a ``CHANGELOG.md``. diff --git a/packages/reflex-release/src/reflex_release/gitutil.py b/packages/reflex-release/src/reflex_release/gitutil.py index f91bdb45da6..90d66a0a0a3 100644 --- a/packages/reflex-release/src/reflex_release/gitutil.py +++ b/packages/reflex-release/src/reflex_release/gitutil.py @@ -89,6 +89,62 @@ def git_show(root: Path, ref: str, rel_path: str) -> str | None: return result.stdout if result.returncode == 0 else None +def adding_commit_message(root: Path, rel_path: str) -> str | None: + """Return the message of the commit that most recently added a file. + + Args: + root: The repository root. + rel_path: Repo-relative file path. + + Returns: + The full commit message, or None when no commit in the local history + added the path (an uncommitted file, or a shallow checkout that does not + reach the commit). + """ + result = subprocess.run( + [ + "git", + "log", + "--diff-filter=A", + "--max-count=1", + "--format=%B", + "--", + rel_path, + ], + cwd=root, + capture_output=True, + text=True, + check=False, + ) + if result.returncode != 0: + return None + return result.stdout.strip() or None + + +def move_file(root: Path, src: Path, dst: Path) -> None: + """Rename a file, keeping git's index in sync when the source is tracked. + + A tracked file has to move through ``git mv``: a bare rename would leave the + deletion of the old path unstaged, and the commands that commit a release + stage only the changelogs, so the old path would survive the release. + + Args: + root: The repository root. + src: The existing path. + dst: The path to move it to. + """ + if ( + subprocess.run( + ["git", "mv", "--", str(src), str(dst)], + cwd=root, + capture_output=True, + check=False, + ).returncode + != 0 + ): + src.rename(dst) + + def changed_files(root: Path, base_ref: str) -> list[str]: """List the repo-relative paths changed since the merge base with a ref. diff --git a/tests/units/reflex_release/test_commands.py b/tests/units/reflex_release/test_commands.py index d690d641155..8825d7e9265 100644 --- a/tests/units/reflex_release/test_commands.py +++ b/tests/units/reflex_release/test_commands.py @@ -247,6 +247,56 @@ def test_materialize_writes_the_changelog( assert not (config.news_dir("widget-core") / "7.feature.md").exists() +def test_materialize_associates_orphan_fragments_with_their_pull_request( + config: Config, repo: Path, outputs: Outputs +) -> None: + """An orphan fragment that landed is linked to the PR that merged it.""" + fragment(config, "widget-core", "+a-widget.feature.md", "A new widget.") + commit_all(repo, "feat: a new widget (#4242)") + releases = json.dumps([ + { + "package": "widget-core", + "current": "", + "next": "0.2.0", + "tag": "widget-core-v0.2.0", + } + ]) + + commands.cmd_materialize(config, "release-minor", releases) + + text = config.changelog_path("widget-core").read_text(encoding="utf-8") + assert "A new widget. ([#4242]" in text + news = config.news_dir("widget-core") + assert not (news / "+a-widget.feature.md").exists() + assert not (news / "4242.feature.md").exists() + # towncrier consumed the renamed fragment, so the orphan is gone for good. + assert git(repo, "status", "--porcelain", "--", str(news)).split() == [ + "D", + "packages/widget-core/news/+a-widget.feature.md", + ] + + +def test_materialize_keeps_an_unassociated_orphan_entry( + config: Config, repo: Path, outputs: Outputs +) -> None: + fragment(config, "widget-core", "+a-widget.feature.md", "A new widget.") + commit_all(repo, "a commit with no pull request") + releases = json.dumps([ + { + "package": "widget-core", + "current": "", + "next": "0.2.0", + "tag": "widget-core-v0.2.0", + } + ]) + + commands.cmd_materialize(config, "release-minor", releases) + + text = config.changelog_path("widget-core").read_text(encoding="utf-8") + assert "A new widget." in text + assert "#" not in text.split("### Features")[1] + + def test_materialize_collapses_a_prerelease_train( config: Config, repo: Path, outputs: Outputs ) -> None: diff --git a/tests/units/reflex_release/test_discovery.py b/tests/units/reflex_release/test_discovery.py index c9e680a2917..bacd67fd0b4 100644 --- a/tests/units/reflex_release/test_discovery.py +++ b/tests/units/reflex_release/test_discovery.py @@ -2,13 +2,23 @@ from __future__ import annotations +import shutil from pathlib import Path import pytest from reflex_release.actions import ReleaseError from reflex_release.changelog import DEFAULT_TITLE_FORMAT from reflex_release.config import Config, load_config -from reflex_release.discovery import title_format +from reflex_release.discovery import ( + associate_orphan_fragments, + fragment_types, + orphan_prefix, + pull_request_number, + split_fragment_name, + title_format, +) + +from .conftest import commit_all, git def set_title_format(repo: Path, value: str) -> Config: @@ -46,3 +56,203 @@ def test_title_format_rejects_a_non_string(config: Config, repo: Path) -> None: reloaded = set_title_format(repo, "3") with pytest.raises(ReleaseError, match="must be a string"): title_format(reloaded) + + +def set_orphan_prefix(repo: Path, value: str) -> Config: + """Configure the repository's towncrier ``orphan_prefix``. + + Args: + repo: The repository root. + value: The TOML value to write, verbatim. + + Returns: + The reloaded configuration. + """ + pyproject = repo / "pyproject.toml" + pyproject.write_text( + pyproject.read_text(encoding="utf-8").replace( + "\n[tool.towncrier]\n", + f"\n[tool.towncrier]\norphan_prefix = {value}\n", + ), + encoding="utf-8", + ) + return load_config(repo) + + +def commit_fragment(config: Config, package: str, name: str, message: str) -> Path: + """Write a news fragment and commit it with a message. + + Args: + config: The repository configuration. + package: The package name. + name: The fragment filename. + message: The commit message to land it with. + + Returns: + The fragment path. + """ + path = config.news_dir(package) / name + path.write_text("Something.\n", encoding="utf-8") + commit_all(config.root, message) + return path + + +def test_orphan_prefix_defaults_to_towncriers_own(config: Config) -> None: + assert orphan_prefix(config) == "+" + + +def test_orphan_prefix_is_configurable(config: Config, repo: Path) -> None: + assert orphan_prefix(set_orphan_prefix(repo, '"~"')) == "~" + + +def test_orphan_prefix_rejects_a_non_string(config: Config, repo: Path) -> None: + with pytest.raises(ReleaseError, match="must be a string"): + orphan_prefix(set_orphan_prefix(repo, "3")) + + +@pytest.mark.parametrize( + ("message", "expected"), + [ + ("feat: add a thing (#1234)", "1234"), + ("ENG-1 refactor(log): split it up (4/5) (#6866)", "6866"), + ("Merge pull request #77 from someone/branch\n\nfeat: a thing", "77"), + ("feat: add a thing (#1234)\n\nA body.", "1234"), + ("feat: add a thing", None), + # A number in the body references an issue, not the pull request. + ("feat: add a thing\n\nFixes #12", None), + ("feat: add a thing\n\n(#12)", None), + ("feat: add a thing (#12) and more", None), + ("", None), + ], +) +def test_pull_request_number(message: str, expected: str | None) -> None: + assert pull_request_number(message) == expected + + +@pytest.mark.parametrize( + ("basename", "expected"), + [ + ("+something.feature.md", ("+something", "feature.md")), + ("1234.bugfix.md", ("1234", "bugfix.md")), + ("+something.feature.1.md", ("+something", "feature.1.md")), + ("+something.feature", ("+something", "feature")), + # The last part naming a type wins, as in towncrier. + ("+feature.bugfix.md", ("+feature", "bugfix.md")), + ("README.md", None), + ("feature", None), + ], +) +def test_split_fragment_name( + config: Config, basename: str, expected: tuple[str, str] | None +) -> None: + assert split_fragment_name(basename, fragment_types(config)) == expected + + +def test_associate_names_an_orphan_after_its_pull_request( + config: Config, repo: Path +) -> None: + commit_fragment(config, "widget-core", "+a-thing.feature.md", "feat: a thing (#42)") + + assert associate_orphan_fragments(config, "widget-core") == [ + ("+a-thing.feature.md", "42.feature.md") + ] + news = config.news_dir("widget-core") + assert not (news / "+a-thing.feature.md").exists() + assert (news / "42.feature.md").is_file() + + +def test_associate_stages_the_rename(config: Config, repo: Path) -> None: + """Only fragments git knows about get their deletion staged by towncrier.""" + commit_fragment(config, "widget-core", "+a-thing.feature.md", "feat: a thing (#42)") + + associate_orphan_fragments(config, "widget-core") + + staged = git(repo, "diff", "--cached", "--name-status", "--no-renames").split() + assert staged == [ + "D", + "packages/widget-core/news/+a-thing.feature.md", + "A", + "packages/widget-core/news/42.feature.md", + ] + + +def test_associate_leaves_numbered_fragments_alone(config: Config, repo: Path) -> None: + commit_fragment(config, "widget-core", "7.feature.md", "feat: a thing (#42)") + + assert associate_orphan_fragments(config, "widget-core") == [] + assert (config.news_dir("widget-core") / "7.feature.md").is_file() + + +def test_associate_keeps_an_orphan_without_a_pull_request( + config: Config, repo: Path, capsys: pytest.CaptureFixture[str] +) -> None: + commit_fragment( + config, "widget-core", "+a-thing.feature.md", "pushed straight to main" + ) + + assert associate_orphan_fragments(config, "widget-core") == [] + assert (config.news_dir("widget-core") / "+a-thing.feature.md").is_file() + assert "::warning::" in capsys.readouterr().out + + +def test_associate_keeps_an_uncommitted_orphan( + config: Config, repo: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """A fragment that never landed has no commit to read a number from.""" + (config.news_dir("widget-core") / "+a-thing.feature.md").write_text( + "Something.\n", encoding="utf-8" + ) + + assert associate_orphan_fragments(config, "widget-core") == [] + assert (config.news_dir("widget-core") / "+a-thing.feature.md").is_file() + assert "::warning::" in capsys.readouterr().out + + +def test_associate_counts_up_on_a_name_collision(config: Config, repo: Path) -> None: + """A pull request can leave behind more than one fragment of a type.""" + news = config.news_dir("widget-core") + (news / "42.feature.md").write_text("Numbered.\n", encoding="utf-8") + (news / "+one.feature.md").write_text("One.\n", encoding="utf-8") + (news / "+two.feature.md").write_text("Two.\n", encoding="utf-8") + commit_all(repo, "feat: three entries (#42)") + + assert associate_orphan_fragments(config, "widget-core") == [ + ("+one.feature.md", "42.feature.1.md"), + ("+two.feature.md", "42.feature.2.md"), + ] + assert sorted(path.name for path in news.iterdir()) == [ + ".gitkeep", + "42.feature.1.md", + "42.feature.2.md", + "42.feature.md", + ] + + +def test_associate_honors_a_custom_orphan_prefix(config: Config, repo: Path) -> None: + reloaded = set_orphan_prefix(repo, '"~"') + commit_fragment( + reloaded, "widget-core", "~a-thing.feature.md", "feat: a thing (#42)" + ) + + assert associate_orphan_fragments(reloaded, "widget-core") == [ + ("~a-thing.feature.md", "42.feature.md") + ] + + +def test_associate_skips_a_package_without_a_news_directory( + config: Config, repo: Path +) -> None: + shutil.rmtree(config.news_dir("mypkg")) + assert associate_orphan_fragments(config, "mypkg") == [] + + +def test_associate_is_a_no_op_when_orphans_are_disabled( + config: Config, repo: Path +) -> None: + reloaded = set_orphan_prefix(repo, '""') + commit_fragment( + reloaded, "widget-core", "+a-thing.feature.md", "feat: a thing (#42)" + ) + + assert associate_orphan_fragments(reloaded, "widget-core") == [] + assert (reloaded.news_dir("widget-core") / "+a-thing.feature.md").is_file() From e4d5b30b920b8a800dab50258c3fcfb4e7b1651c Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 18:52:19 +0000 Subject: [PATCH 2/3] fix(reflex-release): harden how an orphan fragment's pull request is found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the fragment association: - Non-fast-forward merges: the plain path-limited log selects the branch commit that created the fragment, not the merge commit carrying `Merge pull request #N`, so a repository that lands pull requests as merge commits never associated anything. Two candidates are now consulted, most specific first: the commit that added the path along HEAD's first-parent line, then the commit that created the file anywhere in history. Adding `--first-parent` alone would have regressed the prerelease train, where a fragment arrives through a plain `Merge branch 'main' into r/pre-*` commit that names no pull request; the second candidate is what still recovers the number there. - Reused paths: a fragment that is not committed in HEAD is not the file an old commit added at the same path — a fragment an earlier release already consumed, `+.feature.md` say — so its history is no longer consulted and it cannot be linked to an unrelated old pull request. - `git mv` failures are no longer swallowed by a filesystem rename that would leave the deletion of the old path unstaged, and with it the orphan fragment in the release commit. Only fragments tracked in HEAD are renamed now, so a failing `git mv` means the worktree is not in the state the release assumes and fails the command. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GPtjZej5GJQSWoGcG2Cxgf --- .../src/reflex_release/discovery.py | 34 +++++++++-- .../src/reflex_release/gitutil.py | 57 +++++++++++-------- tests/units/reflex_release/test_discovery.py | 51 +++++++++++++++++ 3 files changed, 115 insertions(+), 27 deletions(-) diff --git a/packages/reflex-release/src/reflex_release/discovery.py b/packages/reflex-release/src/reflex_release/discovery.py index f912a831fae..bb1230d570e 100644 --- a/packages/reflex-release/src/reflex_release/discovery.py +++ b/packages/reflex-release/src/reflex_release/discovery.py @@ -12,7 +12,7 @@ from .actions import fail, warning from .changelog import DEFAULT_TITLE_FORMAT, latest_version from .config import Config, load_pyproject -from .gitutil import adding_commit_message, latest_tag_version, move_file +from .gitutil import adding_commit_messages, git_run, latest_tag_version #: towncrier's default prefix for fragments written before the number is known. DEFAULT_ORPHAN_PREFIX = "+" @@ -202,6 +202,24 @@ def _unused_fragment_name(taken: set[str], issue: str, suffix: str) -> str: return candidate +def _fragment_pull_request(root: Path, rel_path: str) -> str | None: + """Return the pull request number a committed fragment landed through. + + Args: + root: The repository root. + rel_path: Repo-relative path of the fragment. + + Returns: + The number named by the first candidate commit that has one, or None + when none of them does. + """ + for message in adding_commit_messages(root, rel_path): + number = pull_request_number(message) + if number is not None: + return number + return None + + def associate_orphan_fragments(config: Config, package: str) -> list[tuple[str, str]]: """Rename a package's orphan fragments after the pull request that added them. @@ -221,6 +239,12 @@ def associate_orphan_fragments(config: Config, package: str) -> list[tuple[str, Returns: The ``(old name, new name)`` pairs that were renamed. + + Raises: + ReleaseError: When a fragment cannot be renamed. Only a tracked fragment + is ever renamed, so ``git mv`` failing means the worktree is not in + the state the release assumes, and a bare rename in its place would + leave the orphan behind in the release commit. """ news_dir = config.news_dir(package) prefix = orphan_prefix(config) @@ -237,8 +261,7 @@ def associate_orphan_fragments(config: Config, package: str) -> list[tuple[str, if parsed is None: continue rel_path = path.relative_to(config.root).as_posix() - message = adding_commit_message(config.root, rel_path) - number = pull_request_number(message) if message else None + number = _fragment_pull_request(config.root, rel_path) if number is None: warning( f"{rel_path}: no pull request found for the commit that added " @@ -246,7 +269,10 @@ def associate_orphan_fragments(config: Config, package: str) -> list[tuple[str, ) continue new_name = _unused_fragment_name(taken, number, parsed[1]) - move_file(config.root, path, news_dir / new_name) + # git mv, not a bare rename: the release commit stages only the + # changelogs, so an unstaged deletion would leave the orphan behind for + # the next release to materialize a second time. + git_run(["mv", "--", str(path), str(news_dir / new_name)], config.root) taken.add(new_name) renamed.append((path.name, new_name)) return renamed diff --git a/packages/reflex-release/src/reflex_release/gitutil.py b/packages/reflex-release/src/reflex_release/gitutil.py index 90d66a0a0a3..c09eb275db8 100644 --- a/packages/reflex-release/src/reflex_release/gitutil.py +++ b/packages/reflex-release/src/reflex_release/gitutil.py @@ -89,28 +89,19 @@ def git_show(root: Path, ref: str, rel_path: str) -> str | None: return result.stdout if result.returncode == 0 else None -def adding_commit_message(root: Path, rel_path: str) -> str | None: - """Return the message of the commit that most recently added a file. +def _log_message(root: Path, args: list[str], rel_path: str) -> str | None: + """Return the message of the first commit a path-limited ``git log`` selects. Args: root: The repository root. + args: Extra ``git log`` arguments placed before the pathspec. rel_path: Repo-relative file path. Returns: - The full commit message, or None when no commit in the local history - added the path (an uncommitted file, or a shallow checkout that does not - reach the commit). + The commit message, or None when the log selected no commit. """ result = subprocess.run( - [ - "git", - "log", - "--diff-filter=A", - "--max-count=1", - "--format=%B", - "--", - rel_path, - ], + ["git", "log", *args, "--max-count=1", "--format=%B", "--", rel_path], cwd=root, capture_output=True, text=True, @@ -121,28 +112,48 @@ def adding_commit_message(root: Path, rel_path: str) -> str | None: return result.stdout.strip() or None -def move_file(root: Path, src: Path, dst: Path) -> None: - """Rename a file, keeping git's index in sync when the source is tracked. +def adding_commit_messages(root: Path, rel_path: str) -> list[str]: + """Return the messages of the commits that brought a file into ``HEAD``. - A tracked file has to move through ``git mv``: a bare rename would leave the - deletion of the old path unstaged, and the commands that commit a release - stage only the changelogs, so the old path would survive the release. + Two candidates, most specific first, because which one carries the pull + request number depends on the repository's merge strategy: + + 1. the commit that added the path along ``HEAD``'s first-parent line, which + is the merge commit of a non-fast-forward merge (``Merge pull request + #N``) and the squash commit of a squash merge; + 2. the commit that created the file anywhere in history, which is that same + squash commit, or the branch commit of a non-fast-forward merge. + + The second is what recovers the number when a fragment reaches the branch + being released through a merge that is not a pull request — merging ``main`` + into a prerelease branch to pull new work into the train, say, where the + first-parent commit is that plain branch merge. Args: root: The repository root. - src: The existing path. - dst: The path to move it to. + rel_path: Repo-relative file path. + + Returns: + The distinct candidate messages, most specific first. Empty when the + path is not committed in ``HEAD``: an uncommitted file is not the file + an old commit added at the same path (a fragment an earlier release + already consumed), so its history says nothing about it. """ if ( subprocess.run( - ["git", "mv", "--", str(src), str(dst)], + ["git", "rev-parse", "-q", "--verify", f"HEAD:{rel_path}"], cwd=root, capture_output=True, check=False, ).returncode != 0 ): - src.rename(dst) + return [] + messages = [ + _log_message(root, ["--first-parent", "--diff-filter=A"], rel_path), + _log_message(root, ["--diff-filter=A"], rel_path), + ] + return list(dict.fromkeys(message for message in messages if message)) def changed_files(root: Path, base_ref: str) -> list[str]: diff --git a/tests/units/reflex_release/test_discovery.py b/tests/units/reflex_release/test_discovery.py index bacd67fd0b4..20e685b57e4 100644 --- a/tests/units/reflex_release/test_discovery.py +++ b/tests/units/reflex_release/test_discovery.py @@ -176,6 +176,57 @@ def test_associate_stages_the_rename(config: Config, repo: Path) -> None: ] +def test_associate_reads_a_merge_commit_subject(config: Config, repo: Path) -> None: + """A repository that lands pull requests as merge commits names them there.""" + git(repo, "checkout", "-q", "-b", "feature") + commit_fragment(config, "widget-core", "+a-thing.feature.md", "add a fragment") + git(repo, "checkout", "-q", "main") + git( + repo, + "merge", + "--no-ff", + "-q", + "-m", + "Merge pull request #99 from someone/feature", + "feature", + ) + + assert associate_orphan_fragments(config, "widget-core") == [ + ("+a-thing.feature.md", "99.feature.md") + ] + + +def test_associate_looks_past_a_merge_that_is_not_a_pull_request( + config: Config, repo: Path +) -> None: + """A prerelease train pulls new work in by merging main, which names no PR.""" + git(repo, "branch", "r/pre-1") + commit_fragment(config, "widget-core", "+a-thing.feature.md", "feat: a thing (#55)") + git(repo, "checkout", "-q", "r/pre-1") + git( + repo, "merge", "--no-ff", "-q", "-m", "Merge branch 'main' into r/pre-1", "main" + ) + + assert associate_orphan_fragments(config, "widget-core") == [ + ("+a-thing.feature.md", "55.feature.md") + ] + + +def test_associate_ignores_the_history_of_a_reused_path( + config: Config, repo: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """A new fragment is not the one an earlier release consumed at that path.""" + news = config.news_dir("widget-core") + commit_fragment(config, "widget-core", "+.feature.md", "feat: an old thing (#7)") + (news / "+.feature.md").unlink() + commit_all(repo, "release: v0.1.0") + (news / "+.feature.md").write_text("A new thing.\n", encoding="utf-8") + + assert associate_orphan_fragments(config, "widget-core") == [] + assert (news / "+.feature.md").is_file() + assert "::warning::" in capsys.readouterr().out + + def test_associate_leaves_numbered_fragments_alone(config: Config, repo: Path) -> None: commit_fragment(config, "widget-core", "7.feature.md", "feat: a thing (#42)") From 96f5f686b92652d49ed89acc3551801afe90ce91 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 19:00:14 +0000 Subject: [PATCH 3/3] fix(reflex-release): read a fragment's history with a literal pathspec A fragment name holding `*`, `?` or brackets is a glob to git, so the path-limited log could select the commit that added a different fragment and rename the orphan after that pull request. Reproduced: with `+a[b].feature.md` and a later `+ab.feature.md`, the former was named after the latter's pull request. `:(literal)` turns the pathspec back into a path. `git mv` resolves its source literally even with a matching sibling present, and `HEAD:` is an object reference rather than a pathspec, so the log is the only lookup affected. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GPtjZej5GJQSWoGcG2Cxgf --- .../reflex-release/src/reflex_release/gitutil.py | 14 +++++++++++++- tests/units/reflex_release/test_discovery.py | 15 +++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/packages/reflex-release/src/reflex_release/gitutil.py b/packages/reflex-release/src/reflex_release/gitutil.py index c09eb275db8..d7e2d249db1 100644 --- a/packages/reflex-release/src/reflex_release/gitutil.py +++ b/packages/reflex-release/src/reflex_release/gitutil.py @@ -92,6 +92,10 @@ def git_show(root: Path, ref: str, rel_path: str) -> str | None: def _log_message(root: Path, args: list[str], rel_path: str) -> str | None: """Return the message of the first commit a path-limited ``git log`` selects. + The path is passed as a ``:(literal)`` pathspec: a filename holding ``*``, + ``?`` or brackets is a glob to git, which would read some other file's + history. + Args: root: The repository root. args: Extra ``git log`` arguments placed before the pathspec. @@ -101,7 +105,15 @@ def _log_message(root: Path, args: list[str], rel_path: str) -> str | None: The commit message, or None when the log selected no commit. """ result = subprocess.run( - ["git", "log", *args, "--max-count=1", "--format=%B", "--", rel_path], + [ + "git", + "log", + *args, + "--max-count=1", + "--format=%B", + "--", + f":(literal){rel_path}", + ], cwd=root, capture_output=True, text=True, diff --git a/tests/units/reflex_release/test_discovery.py b/tests/units/reflex_release/test_discovery.py index 20e685b57e4..9b50e0613af 100644 --- a/tests/units/reflex_release/test_discovery.py +++ b/tests/units/reflex_release/test_discovery.py @@ -227,6 +227,21 @@ def test_associate_ignores_the_history_of_a_reused_path( assert "::warning::" in capsys.readouterr().out +def test_associate_matches_a_glob_like_filename_literally( + config: Config, repo: Path +) -> None: + """A fragment name holding brackets is a path, not a git pathspec.""" + commit_fragment( + config, "widget-core", "+a[b].feature.md", "feat: the real one (#222)" + ) + commit_fragment(config, "widget-core", "+ab.feature.md", "feat: a sibling (#111)") + + assert associate_orphan_fragments(config, "widget-core") == [ + ("+a[b].feature.md", "222.feature.md"), + ("+ab.feature.md", "111.feature.md"), + ] + + def test_associate_leaves_numbered_fragments_alone(config: Config, repo: Path) -> None: commit_fragment(config, "widget-core", "7.feature.md", "feat: a thing (#42)")