feat(reflex-release): name orphan news fragments after their pull request - #6928
Conversation
…uest 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 `<pr>.<type>.<n>.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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GPtjZej5GJQSWoGcG2Cxgf
Greptile SummaryThe PR associates orphan Towncrier fragments with the pull request that introduced them before materializing changelogs.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/reflex-release/src/reflex_release/gitutil.py | Adds first-parent and full-history lookup candidates; the first-parent query addresses the previously reported merge-commit association failure. |
| packages/reflex-release/src/reflex_release/discovery.py | Implements orphan parsing, pull-request extraction, collision-safe naming, and staged fragment renames. |
| packages/reflex-release/src/reflex_release/commands.py | Integrates orphan association immediately before each package changelog is built. |
| tests/units/reflex_release/test_discovery.py | Covers merge commits, squash subjects, non-PR merges, reused paths, literal pathspecs, collisions, custom prefixes, and unassociated fragments. |
| tests/units/reflex_release/test_commands.py | Verifies materialization produces linked entries and fully consumes renamed fragments. |
Reviews (3): Last reviewed commit: "fix(reflex-release): read a fragment's h..." | Re-trigger Greptile
Merging this PR will not alter performance
Comparing Footnotes
|
There was a problem hiding this comment.
3 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/reflex-release/src/reflex_release/gitutil.py">
<violation number="1" location="packages/reflex-release/src/reflex_release/gitutil.py:108">
P2: When a fragment enters through a regular merge, this path-limited log returns the branch commit instead of the merge commit. If that branch subject lacks `(#N)`, `pull_request_number` finds nothing and leaves the orphan unlinked; add `--first-parent` so the merge subject is selected.</violation>
<violation number="2" location="packages/reflex-release/src/reflex_release/gitutil.py:112">
P2: When an uncommitted fragment reuses a path that an earlier release already consumed, this command returns the historical add commit even though the path is absent from `HEAD`. Check that the current path exists in `HEAD` before querying its adding commit, otherwise the new fragment can be linked to an unrelated old PR.</violation>
<violation number="3" location="packages/reflex-release/src/reflex_release/gitutil.py:145">
P2: When `git mv` fails for a tracked source, this unconditional fallback performs a filesystem-only rename. The release commit stages only changelogs, so the old fragment deletion remains unstaged; fall back only for untracked sources and propagate tracked move errors.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
FarhanAliRaza
left a comment
There was a problem hiding this comment.
Live-run review finding: merge-commit PR recovery never fires.
adding_commit_message (packages/reflex-release/src/reflex_release/gitutil.py) runs git log --diff-filter=A --max-count=1 -- <fragment>. Without --first-parent, git's history simplification attributes the file's addition to the PR's inner commit, not to the merge commit. The inner commit carries the contributor's own subject, which has no #N. So in a repo that uses merge commits, every orphan fragment gets the no-PR warning, and the ^Merge pull request #(\d+) pattern in discovery.py is unreachable in practice: real GitHub merges never add the file in a commit with that subject.
Fix: add --first-parent to that one git log call. Verified live against a repo scaffolded with the real reflex-release init, running the real materialize with RELEASES_JSON/ACTION as the generated workflow does:
| Topology | Without flag | With flag |
|---|---|---|
Squash merge (feat: x (#101)) |
#101 ✓ |
#101 ✓ (unchanged) |
Real --no-ff PR merge |
inner commit, warning ✗ | Merge pull request #202 ✓ |
| Direct push, no PR | warning ✓ | warning ✓ (unchanged) |
| Stacked branches (merge of merge, outer PR #777) | inner noise, warning ✗ | #777 — the PR that landed on main ✓ |
| Fragment renamed inside the PR branch (#888) | rename commit, warning ✗ | #888 ✓ |
The last row is a bonus: --first-parent also recovers the number for a fragment that was git mv-renamed inside the PR branch, because the merge commit shows the renamed path as added. The end-to-end run with the flag produced linked entries ([#777]...) and ([#888]...) in CHANGELOG.md, with all fragments consumed and their deletions staged cleanly.
Test gap that hid this: test_pull_request_number in tests/units/reflex_release/test_discovery.py tests the Merge pull request #77 string only; no test builds the merge topology. A regression test should commit a fragment on a branch, git merge --no-ff -m "Merge pull request #N from ..." into main, and assert associate_orphan_fragments renames it to N.<type>.md — it fails on the current code.
Verified working as-is: squash recovery, counter collisions (303.feature.1/2/3.md all rendered with #303 links), custom orphan_prefix, disabled orphans, uncommitted/no-PR fragments (warning + unlinked entry, no crash), the bare +.feature.md name, and staging via git mv.
…found 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GPtjZej5GJQSWoGcG2Cxgf
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/reflex-release/src/reflex_release/gitutil.py">
<violation number="1" location="packages/reflex-release/src/reflex_release/gitutil.py:104">
P2: When an orphan filename contains `*`, `?`, or bracket characters, Git interprets `rel_path` as a wildcard and can read another fragment’s adding commit, causing a wrong PR rename. Pass the path as `:(literal){rel_path}` to disable pathspec globbing.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
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:<path>` is an object reference rather than a pathspec, so the log is the only lookup affected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GPtjZej5GJQSWoGcG2Cxgf
An orphan fragment (
+something.feature.md) is meant to be renamed once thepull 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.
materializenow recovers it: for every package it is about to build, eachleftover 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 mergecommit,
... (#N)for a squash). The rename goes throughgit mv, so thedeletion of the old path is staged and towncrier's own
git rmof the renamedfragment leaves nothing behind. A pull request that left two fragments of one
type behind gets towncrier's
<pr>.<type>.<n>.mdcounter form rather thancolliding.
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 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01GPtjZej5GJQSWoGcG2Cxgf