fix: make the beta-release changelog merge deterministic so the new section is never absorbed - #678
Conversation
…previous one The 3-way merge of beta/main into a release branch cannot express "append the new section onto beta's accumulated history", so hand-resolved conflicts repeatedly collapsed the new section into the previous one (#648). Add scripts/check-changelog.py: a `build` subcommand that resolves the conflict deterministically (beta/main's published history verbatim, with the new version section inserted after the preamble), and a `check` subcommand that asserts prepend-only, coverage, and no-re-announcing invariants, mapping merged PRs to the issues they reference via the merge-commit body. Wire both into the release skill's step 4/5. Refs #648
|
@claude-bot review |
There was a problem hiding this comment.
Review: PR #678 (fix/issue-648-deterministic-beta-changelog)
Root cause match: Yes. Issue #648 documents the exact failure (3-way merge collapses the new CHANGELOG section into the previous one because trailing boilerplate matches) with concrete evidence (b7/b8, b9/b10 absorption, b8/b10 shipping empty). The issue's proposed fix has three parts — (1) stop merging the file, resolve deterministically, (2) add a script asserting prepend-only / coverage / no-re-announcing, (3) wire it into the release skill — and the PR implements all three, 1:1, no extra scope.
Would the tests catch a regression? Yes. Verified independently (venv unavailable in this environment, so ran against a fresh worktree with vendored black/ruff/mypy/pytest):
.venv/bin/pytest backend/tests/test_check_changelog.py→ 21/21 passed.black --check,ruff check,mypyall clean on both new files (scripts/check-changelog.py,backend/tests/test_check_changelog.py).- The tests assert behavior (function in/out), not implementation, and several are explicitly discriminating:
test_prepend_only_rejects_absorbed_section/test_check_cli_rejects_absorbed_sectionfeed the exact #648 corruption shape through and confirm rejection;test_parse_merged_prs_from_git_log_bodiesreproduces git's real%B%x00padding and the PR body notes this is a regression test against a parser that used to silently drop every commit after the first.DROPPEDcovers the b7#512line-loss case too. This is real RED-provenance, not just green-suite assertion.
Minimal / no scope creep: Yes. Four files: the new script, its test, the CHANGELOG.md entry (correctly placed under ## [Unreleased] on main per CLAUDE.md), and SKILL.md steps 4/5 updated to call it. The stable-release flow (SKILL.md line 110, plain rename, no merge) is untouched — correctly, since the bug is specific to the beta/main merge path. No unrelated edits.
Correctness spot-check (logic walkthrough, not just tests):
insert_new_sectionexplicitly handles the empty-beta-history edge case (first-ever beta), not just the steady-state case.- Merge-conflict stage numbering in the SKILL.md commands (
git show :2:CHANGELOG.md→--new,:3:→--beta) is correct forgit merge beta/mainrun from the release branch (ours=stage 2=release branch with the new section, theirs=stage 3=beta/main). check'smain()requires at least one of--beta-ref/--beta-file/--since/--since-ref, so a mistaken no-op invocation can't silently print "OK" without checking anything.- Subprocess calls to git use list-form args (no
shell=True), no injection surface.
Rule compliance: No Optional[x], no hasattr/getattr-with-default, no exception-string matching, no hardcoded entity IDs (N/A — tooling script, no HA sensor access). Comments in the script are WHY-only (e.g. explaining git's %B record padding, why a bare #N in prose isn't matched) — no WHAT-only comments. The two small support types (ChangelogCheckError(Exception), MergedPR(NamedTuple)) are idiomatic utility types for a standalone script, not a parallel implementation of existing core/bess architecture — I don't read rules.md's "no new classes without approval" as reaching this.
Two non-blocking nits:
docs/agents/rules.mdDebugging Protocol step 9 / checklist item 5 asks the PR description to state an explicit scope assessment (local / structural / needs-a-second-opinion). This PR's "Root cause" + "Fix" sections cover the same ground in substance but don't use that literal framing — worth adopting the label explicitly in future PRs of this shape, not asking for a re-post here.SKILL.mdline 48-52's examplecheckcommand shows--internal <comma-separated internal PRs>as if always populated; the flag is actually optional (empty when step 4 found no internal-only gaps). Cosmetic only — a future reader won't be blocked by it.
No blockers found. Diff is clean, minimal, and directly addresses #648's root cause with real regression coverage.
Summary
The beta release flow's hand-resolved
CHANGELOG.mdmerge repeatedly collapsed the newly-prepended section into the previous one (b7/b8 and b9/b10 on the published beta history, and the reported #648). This PR replaces the hand-merge with a deterministic resolution plus a three-invariant guard.Root cause
CHANGELOG.mdconflicts on every beta release, and git's 3-way merge cannot express "append the new section onto beta's accumulated history" — a hand-fixed conflict folds the new section into the previous one, silently dropping the new section heading and mislabelling its entries.Fix
scripts/check-changelog.py:build— resolves the conflict deterministically: beta/main's published history verbatim, with the new version section inserted after the preamble. Byte-stable by construction.check— asserts three invariants: prepend-only (stripping the new section leaves beta/main's file byte-for-byte), coverage (every PR merged on main since the previous beta's sync point appears in the new section, by PR number or a referenced issue), and no-re-announcing (nothing already shipped is re-listed). The PR→issue mapping comes from the merge-commit body, since GitHub embeds the PR body there and the CHANGELOG links issue numbers..claude/skills/release/SKILL.mdsteps 4 and 5 (buildto resolve the conflict,checkto gate the release), cutting the merged-PR range at the sync point (--since-ref <merge-base>) rather than the previous release's publish timestamp.## [Unreleased].Test plan
backend/tests/test_check_changelog.py— 21 unit tests (section extraction/build determinism, prepend-only accepts/rejects, coverage, no-re-announcing, git-log parsing including git's real%B%x00padding, CLI wiring)../scripts/quality-check.sh— pass.pytest -m slow— 554 passed, 8 skipped.origin/main+beta/mainrefs — output byte-deterministic across runs; the full check passes on a realistic b13 section; the exact Beta release merge silently absorbs the new changelog section into the previous one #648 corruption (new entries absorbed into b12, heading dropped) is rejected with a prepend-only violation.Evidence the test discriminates
test_check_changelog.pyfeeds three variants throughcheck_prepend_only: the correctly-built file (passes), the absorbed-section corruption from #648 (rejected), and a dropped historical line (rejected).test_parse_merged_prs_from_git_log_bodiesreproduces git's real--format=%B%x00padding and fails on the pre-fix parser, which silently dropped every commit after the first.Outcome-level coverage
The guard runs on the next beta release and refuses to proceed on any CHANGELOG state that prepend-only/coverage/no-re-announcing reject; a correct
buildexits 0.Doc check: neither
docs/agents/bess-knowledge.mdnordocs/SOFTWARE_DESIGN.mdmentions the beta release flow or this script; no doc updates needed.Refs #648