chore(changelog): one fragment file per entry in changelog.d/, collected at release (LAB-6153) - #80
Conversation
…ted at release (LAB-6153)
|
Warning Review limit reachedNext included review available in 21 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: cachekit-io/protocol/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
Kody Code Review — 1 suggested fix. 🛠️ Open Agent Prompt |
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
| # changelog.d/ (one file each); only a release/* branch rewrites CHANGELOG.md. | ||
| changelog-fragments: | ||
| name: CHANGELOG.md is edited only by releases | ||
| if: github.event_name == 'pull_request' && !startsWith(github.head_ref, 'release/') |
There was a problem hiding this comment.
Authorization bypass in changelog-fragments job condition: the release exemption checks only github.head_ref, which the PR author controls, including on forks where the base repo's branch rules do not apply. When a contributor opens a PR from a fork branch named release/fix that edits CHANGELOG.md directly, the job is skipped, and because a skipped job reports as passing, the required check does not block the merge. Fix: also require github.event.pull_request.head.repo.full_name == github.repository so the exemption applies only to branches in this repository.
if: github.event_name == 'pull_request' && !(startsWith(github.head_ref, 'release/') && github.event.pull_request.head.repo.full_name == github.repository)Prompt for LLM
File .github/workflows/verify.yml:
Line 88:
Authorization bypass in changelog-fragments job condition: the release exemption checks only `github.head_ref`, which the PR author controls, including on forks where the base repo's branch rules do not apply. When a contributor opens a PR from a fork branch named `release/fix` that edits `CHANGELOG.md` directly, the job is skipped, and because a skipped job reports as passing, the required check does not block the merge. Fix: also require `github.event.pull_request.head.repo.full_name == github.repository` so the exemption applies only to branches in this repository.
Suggested Code:
if: github.event_name == 'pull_request' && !(startsWith(github.head_ref, 'release/') && github.event.pull_request.head.repo.full_name == github.repository)
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
…t so scriv collects them (LAB-6153)
… regrouped same-titled entries (LAB-6153)
| changelog.write_text(f"{head}{MARKER}\n\n{section}{rest.lstrip(chr(10))}", encoding="utf-8") | ||
| for p in fragments: | ||
| p.unlink() |
There was a problem hiding this comment.
Partial-failure state in collect(): CHANGELOG.md is written before the fragments are unlinked, so an OSError on any unlink leaves the new version section in CHANGELOG.md while some fragments remain. When a fragment is read-only or its directory denies write permission, main() exits 1, a rerun is refused with 'already has a section', and unless someone removes the leftover fragments by hand, the next release collects them again and duplicates those entries. Fix: delete the fragments first and restore them from their saved contents if the CHANGELOG.md write fails, or verify every fragment is deletable (e.g. os.access on the parent directory) before writing.
new_text = f"{head}{MARKER}\n\n{section}{rest.lstrip(chr(10))}"
for p in fragments:
p.unlink()
try:
changelog.write_text(new_text, encoding="utf-8")
except OSError:
for p, body in zip(fragments, originals):
p.write_text(body, encoding="utf-8")
raisePrompt for LLM
File tools/changelog-collect.py:
Line 49 to 51:
Partial-failure state in collect(): CHANGELOG.md is written before the fragments are unlinked, so an OSError on any unlink leaves the new version section in CHANGELOG.md while some fragments remain. When a fragment is read-only or its directory denies write permission, main() exits 1, a rerun is refused with 'already has a <version> section', and unless someone removes the leftover fragments by hand, the next release collects them again and duplicates those entries. Fix: delete the fragments first and restore them from their saved contents if the CHANGELOG.md write fails, or verify every fragment is deletable (e.g. os.access on the parent directory) before writing.
Suggested Code:
new_text = f"{head}{MARKER}\n\n{section}{rest.lstrip(chr(10))}"
for p in fragments:
p.unlink()
try:
changelog.write_text(new_text, encoding="utf-8")
except OSError:
for p, body in zip(fragments, originals):
p.write_text(body, encoding="utf-8")
raise
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| try: | ||
| collected = collect(args.root, args.version, args.date.isoformat()) | ||
| except (ValueError, OSError) as e: | ||
| print(f"changelog-collect: {e}", file=sys.stderr) |
There was a problem hiding this comment.
Violates team rule 'Replace print statements with logging framework': Use the standard logging module (or your app's logger) instead of print() in committed code.
Also found in:
tools/changelog-collect.py:66-66tools/changelog-collect.py:68-68tools/test_changelog_collect.py:32-32tools/test_changelog_collect.py:108-108
Prompt for LLM
File tools/changelog-collect.py:
Line 64:
Violates team rule 'Replace print statements with logging framework': Use the standard logging module (or your app's logger) instead of print() in committed code.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| _spec.loader.exec_module(cc) | ||
|
|
||
| BASE = f"# Changelog\n\n## [Unreleased]\n\nPointer.\n\n{cc.MARKER}\n\n## [1.0.0] - 2026-03-28\n\nInitial.\n" | ||
| FAILURES: list[str] = [] |
There was a problem hiding this comment.
WHAT: FAILURES is a module-level mutable list, and check() appends to it as a side effect.
WHY: Global mutable state makes results depend on import and call order. It also leaks state if the module is imported or reused, for example by a test runner.
HOW: Keep failures in a local list or a small results object created in main(). Pass it to check() (or have check return a bool that the caller collects), then compute the exit code from that local state.
Kody rule violation: Avoid Using Mutable Global Variables
Prompt for LLM
File tools/test_changelog_collect.py:
Line 28:
WHAT: `FAILURES` is a module-level mutable list, and `check()` appends to it as a side effect.
WHY: Global mutable state makes results depend on import and call order. It also leaks state if the module is imported or reused, for example by a test runner.
HOW: Keep failures in a local list or a small results object created in `main()`. Pass it to `check()` (or have `check` return a bool that the caller collects), then compute the exit code from that local state.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
… skips when nothing is pending (LAB-6153)
| shutil.copytree(REPO / "changelog.d", real / "changelog.d") | ||
| pending = [p.read_text().strip() for p in sorted((real / "changelog.d").glob("*.md")) if p.name != "README.md"] | ||
| if not pending: | ||
| print("skip repo dry run: no pending fragments (fresh after a release)") |
There was a problem hiding this comment.
Team-rule violation in tools/test_changelog_collect.py: the skip message uses print() instead of the logging framework, which violates the team rule 'Replace print statements with logging framework'. When the repo dry run has no pending fragments after a release, the message goes straight to stdout and bypasses configured log levels and handlers. Fix: replace print() with a module-level logger call, such as logger.info(...).
Prompt for LLM
File tools/test_changelog_collect.py:
Line 67:
Team-rule violation in tools/test_changelog_collect.py: the skip message uses print() instead of the logging framework, which violates the team rule 'Replace print statements with logging framework'. When the repo dry run has no pending fragments after a release, the message goes straight to stdout and bypasses configured log levels and handlers. Fix: replace print() with a module-level logger call, such as logger.info(...).
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
1 participant
Changelog entries now go in
changelog.d/, one file per change, andCHANGELOG.mdis rewritten only at release. Every PR used to add its entry right under the one## [Unreleased]heading, so any two open PRs conflicted on the same hunk. Separate files cannot conflict.The red
changelog-fragmentscheck on this PR is expected. This PR moves the existing Unreleased entries out ofCHANGELOG.md, and the new guard fails any non-release/*PR that edits that file. This PR is the guard's first live failure. The guard never runs onpush.What changes
changelog.d/README.mdis the contributor rule. Write the entry, exactly as it should appear, tochangelog.d/<YYYYMMDD>_<ticket-id>.md.changelog.d/20260328_unreleased-since-1.0.0.mdholds every entry merged since 1.0.0, moved byte-for-byte out ofCHANGELOG.md. The name sorts first, so the next release's section starts with it.tools/changelog-collect.py VERSIONruns on arelease/VERSIONbranch. It writes## [VERSION] - DATEat the<!-- changelog-insert-here -->marker, concatenates the fragments verbatim in filename order, and deletes them. It fails closed, writing nothing, on a missing or repeated marker, an existing version, a malformed version or date, no fragments, or an empty fragment.tools/test_changelog_collect.pyruns inverify.yml. It also dry-runs the next release against the realCHANGELOG.mdandchangelog.d/, so a fragment that would break a release fails CI first.verify.ymlalso gets thechangelog-fragmentsjob. It runs onpull_requestonly, skipsrelease/*heads, and diffsCHANGELOG.mdbetween the test-merge commit and its first parent.Why a script and not scriv, release-please or
merge=union###heading as a category. On this changelog it merged the two### SaaS APIsections and reordered entries across the release. These entries are normative prose, so a verbatim concatenation is the only safe transform.maindirectly.merge=uniondoes not help. GitHub's mergeability check ignores custom merge drivers (community discussion).Verification
python3 tools/test_changelog_collect.pypasses 10/10. The cases cover:Mutation testing: 6 of 7 mutants of the collect script fail the suite, across 6 runs:
The survivor, which drops the repeated-marker check, is equivalent: the
splitstill raises on two markers.The moved block is byte-identical to
main's Unreleased section.I took the five open PRs whose only conflict with
mainisCHANGELOG.md, moved each one's entry into a fragment, and merged all five into one branch: no conflicts.The guard's step script, extracted with
yq, fails on simulated test-merges that editCHANGELOG.mdand passes on fragment-only and unrelated changes.actionlintis clean.Open PRs that still edit
CHANGELOG.mdneed one last resolution after this merges. Keepmain'sCHANGELOG.mdand move the PR's entry into a fragment. The guard's error message says the same.Closes LAB-6153
This PR replaces scriv with an in-repo collector for release changelogs. Each changelog entry is now its own file in
changelog.d/, so concurrent PRs no longer conflict on a shared## [Unreleased]heading.CHANGELOG.mditself is only rewritten when a release is cut.Why
scriv treated every
###heading as a category. That caused two problems for these entries:### SaaS API).The entries are curated normative prose, so the new tool concatenates fragments verbatim instead.
Changes
New tool:
tools/changelog-collect.pypython3 tools/changelog-collect.py VERSION [--date YYYY-MM-DD] [--root PATH]collect(root: Path, version: str, date: str) -> list[Path]MARKER = "<!-- changelog-insert-here -->"## [VERSION] - DATEsection at the marker inCHANGELOG.md. The section holds every fragment (excludingREADME.md) verbatim, in filename order. The collected fragments are then deleted.MAJOR.MINOR.PATCH.CHANGELOG.md<!-- scriv-insert-here -->/<!-- scriv-end-here -->) are replaced by the single<!-- changelog-insert-here -->marker.changelog.d/20260328_unreleased-since-1.0.0.md. That file sorts first, so the next release contains it.changelog.d/README.mdtools/changelog-collect.pyin place ofuvx scriv@1.8.0 collect.Tests:
tools/test_changelog_collect.py(stdlib only) cover:CHANGELOG.mdandchangelog.d/.CI (
.github/workflows/verify.yml)This PR makes the real-repo dry-run check in the changelog collection tests future-proof. The previous check stops being valid after the first fragment-based release.
Changes (
tools/test_changelog_collect.py)dry_run_next_release(tmp): Replaces the inline check inmain()that hardcoded version1.1.0and assumed## [1.0.0]was the next heading. The helper:CHANGELOG.mdandchangelog.d/into a temporary directory.## [X.Y.Z]headings with a regex, and targets one minor version above it (X.(Y+1).0).cc.collecton the copy with a fixed date (2026-10-01).cc.MARKERis unchanged,import refor version parsing.<version>) holds every pending fragment verbatim."Impact
Test-only change; no public or production APIs are modified. The only new callable is the test-module-level function
dry_run_next_release. The existingrefuses(...)negative cases are unchanged.