fix(scripts): key the absorbed-heading check on the version, not the line (#2327) - #2341
Conversation
…line (#2327) #2290's absorption check diffs whole heading LINES (`changelog_bracket_headings` is `grep -E` without `-o`, fed to `comm -23`), so any edit to an existing release heading reads as a deletion. Both annotations Keep a Changelog itself prescribes red-line this REQUIRED gate while PRESERVING the heading they annotate — verified against the shipped script on main, unmodified: ## [1.0.0] -> ## [1.0.0] - 2026-01-01 [YANKED] EXIT=1 ## [1.0.0] -> ## [1.0.0] - 2026-01-01 EXIT=1 A false positive on a required merge check blocks every in-flight PR that touches a heading line, not only its author's. Keys the comparison on the VERSION each heading names, through the shared `changelog_versions` extractor, so `## [1.0.0]` and `## [1.0.0] - 2026-01-01 [YANKED]` are one release. `changelog_bracket_headings` is dropped: it was a second reader of the same file, which the header's rendered_lines note argues against ("One tracker, not three, so the modes cannot drift"). The failure still PRINTS `## [<version>]`, so nothing downstream of the message changes. Two behaviour changes worth naming: - Reformatting a heading between the bracketed and unbracketed forms is no longer a deletion — that is CHANGELOG FORMAT's concern. - An UNBRACKETED heading is now protected. A bracketed-only matcher could not see `## 0.9.0 — 2025-12-01` at all, so absorbing that section was invisible. A relabel is still caught: renaming `## [0.51.8]` to `## [0.51.9]` deletes the version 0.51.8 whatever the line around it looks like. Out of scope, tracked in #2327: the check lives inside `--check-bump`'s loop, so an absorption in a change set that does not bump the manifest, and one in a `docs/conventions/*/CHANGELOG.md`, are still unpoliced; and the `2>/dev/null` process substitution on `git show` is fail-open. Both are pre-existing from #2290 and out of scope for a surgical false-positive fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Claude finished @kyle-sexton's task in 1m 44s —— View job Security Review
Scope: both changed files — No security issues found. Reasoning, focused on the actual production-code delta (
Nothing else in this diff touches authorization, tokens/secrets, or CI trust boundaries — it's a pure bug fix to a comparison key inside an existing, already-sandboxed shell check. |
|
Claude finished @kyle-sexton's task in 59s —— View job Security Review
Scope: both changed files — No security issues found. Reasoning, focused on the production-code delta at
Nothing in this diff touches authorization, tokens/secrets, or CI trust boundaries — it's a pure bug-fix to a comparison key inside an already-sandboxed shell check, consistent with the earlier automated review on this PR. |
|
Claude finished @kyle-sexton's task in 5m 9s —— View job Code Review
Scope: both changed files, per the code-review lane (correctness, design, error handling, test coverage, maintainability). Security is out of scope here — this repo has a dedicated security lane that already reviewed this PR. Summary: the core fix is correct. Reading through
Verification note: I was not able to independently run No correctness or design issues found beyond the one nit above. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
…2370) Fixes #2342 ## Summary - Add `--check-preserved <ref>` mode that checks every **touched** changelog (plugin and convention) for dropped version headings vs the fork point. - Replace the in-loop absorbed-heading check inside `--check-bump` (which only ran for bumped plugins) with this dedicated mode. - Wire `--check-preserved` into the CI `changelog-parity-gate` job as an additional PR-only step. - Uses `git ls-tree` + command-substitution `git show` (fail-loud on git errors). ## Test plan - [x] `bash scripts/check-changelog-parity.test.sh` (76/0) - Covers: changelog-only absorption, convention changelogs, relabelling, [YANKED], heading reformat, stale-branch false-positive guard, whole-file deletion, SIGPIPE/gawk fixture ## Related - #2264 / #2290 — motivating absorption defect - #2327 / #2341 — false-positive fixes for heading annotations - #2324 — git fail-open hardening (overlapping fix, landed separately in PR #2369) Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
Independent verifier verdict — post-hoc, PR already merged as
|
fixture (head heading for the preserved 1.0.0) |
shipped bb817c6c |
pre-fix bb817c6c^ |
|---|---|---|
(a) ## [1.0.0] - 2026-01-01 [YANKED] |
EXIT=0 Every plugin whose version changed vs fixture-base has a '## [<version>]' CHANGELOG.md entry. |
EXIT=1 ABSORBED CHANGELOG HEADING: demo lost release section heading(s) … ## [1.0.0] |
(b) ## [1.0.0] - 2026-01-01 |
EXIT=0 (same message) | EXIT=1 (same ABSORBED … ## [1.0.0]) |
ITEM 2: CONFIRMED — the gate still discriminates.
- Head deletes
## [1.0.0]and folds its body into## [1.0.1]→ EXIT=1,ABSORBED CHANGELOG HEADING: demo lost release section heading(s) vs the fork point (a merge-forward may have fused two releases into one section):/## [1.0.0]/Restore every '## [<version>]' heading that existed at the fork point; … - Relabel
## [1.0.0]→## [1.0.2](heading count unchanged) → EXIT=1, same message naming## [1.0.0]. - Cross-plugin state was also checked, since every new test in fix(scripts): key the absorbed-heading check on the version, not the line (#2327) #2341 is single-plugin and
head_documentsisdeclare -A'd once outside the manifest loop.a=()clears an associative array cleanly on this bash (count=0 x=unset), and a two-plugin change set (keeper preserves1.0.0, absorber deletes1.0.0, both manifests bumped) reports the absorber only, in both glob orderings. Non-finding.
ITEM 3: REFUTED — two live misbehaviours on the inline-linked heading form (plus two pre-existing symptoms of the same blind spot on the sibling gates). Seventeen fixtures; the inline-linked form is the only one that misbehaves.
Behaves correctly (post-fix): ## [Unreleased] added (EXIT=0) and deleted (EXIT=0) — it carries no version, so it is invisible to changelog_versions; state that as a coverage boundary, not a pass. Trailing whitespace (two spaces + tab) on a preserved heading → EXIT=0 (pre-fix: EXIT=1 — a third false positive #2341 fixed but did not name). Reference-link definitions [1.0.0]: https://… present on both sides → EXIT=0; and the discriminating case — heading ## [1.0.0] deleted while [1.0.0]: https://… is kept — still EXIT=1 ABSORBED, so a link definition can neither satisfy nor trigger the check. CRLF throughout, heading preserved+annotated → EXIT=0 (pre-fix EXIT=1); CRLF with the heading absorbed → EXIT=1. Unbracketed ## 1.0.0 - 2025-12-01 preserved → EXIT=0, absorbed → EXIT=1 (pre-fix EXIT=0 — new coverage, as the PR claims). Two-component ## 0.9 absorbed → EXIT=1 naming ## [0.9].
The two that misbehave, both on ## [<v>](<url>) - <date> — the inline-linked heading emitted by conventional-changelog / release-please tooling (verified against googleapis/release-please's own CHANGELOG.md, whose top heading is ## [17.11.1](https://github.com/googleapis/release-please/compare/v17.11.0...v17.11.1) (2026-07-31); Keep a Changelog's own reference changelog uses the plain form, so this is a widespread in-the-wild variant, not a KAC-prescribed one):
- FALSE NEGATIVE (a coverage regression introduced by this PR). Fork has
## [1.0.0](https://example.com/compare/v0.9.0...v1.0.0) - 2017-06-20; head deletes it entirely and folds the body into## [1.0.1]. Shipped script: EXIT=0 — a genuine absorption passes. Pre-fix script: EXIT=1ABSORBED …. fix(scripts): reject absorbed changelog release headings (#2264) #2290's matcher^## \[<ver>\]was a prefix match and saw linked headings;changelog_versions'…\]?([[:space:]]|$)trailing anchor does not, because(follows the]. Keying on the version was right; routing through this extractor silently narrowed what the check can see. - FALSE POSITIVE (the same class fix(scripts): key the absorbed-heading check on the version, not the line (#2327) #2341 set out to kill, still live). Fork has plain
## [1.0.0]; head preserves the release and reformats that heading to## [1.0.0](https://example.com/compare/v0.9.0...v1.0.0) - 2017-06-20. Shipped script: EXIT=1ABSORBED CHANGELOG HEADING … ## [1.0.0]. Nothing was absorbed. Adding compare links to an existing changelog is ordinary housekeeping, and it red-lines a required gate.
Reachability was checked on all three gates, not just --check-bump, because changelog_versions is shared by all of them. The form is banned by none of them.
| fixture | result |
|---|---|
--check-bump: head documents the bumped version as ## [1.0.1](…) - 2026-01-01 |
EXIT=0 — accepted, no CHANGELOG FORMAT failure |
--check: newest heading linked ## [1.0.1](…), manifest 1.0.1 |
EXIT=0 |
--check: newest heading plain ## [1.0.1], manifest 1.0.0 (control) |
EXIT=1 CHANGELOG AHEAD OF MANIFEST: … documents 1.0.1 but … carries 1.0.0. |
--check: same violation, heading linked ## [1.0.1](…), manifest 1.0.0 |
EXIT=0 — the violation is invisible |
--check-order: ## [1.0.0] / ## [2.0.0](…) / ## [0.9.0] |
EXIT=0 |
--check-order: same order, all plain (control) |
EXIT=1 MISORDERED CHANGELOG: … 2.0.0 (below 1.0.0) |
So --check-bump blesses a heading form at introduction that its own preservation check cannot see afterwards — an internal inconsistency between two checks in the same script, independent of any external spec — and the other two gates admit it too. Two of those rows are additional symptoms, and they are pre-existing, not #2341's doing: --check and --check-order always read through changelog_versions, so a linked heading has always been able to sit above its manifest or out of order unseen. What #2341 changed is that preservation now shares that blindness. They belong in the same fix, but the blame for them does not sit on this PR.
Why no sweep could have caught this. grep -rEn '^## \[[0-9]+\.[0-9]+(\.[0-9]+)?\]\(' plugins/*/CHANGELOG.md docs/conventions/*/CHANGELOG.md → 0 hits. The corpus contains no linked heading, so a corpus sweep is quiet on it — the identical blind spot that let #2290 ship. The only two ^## lines in the whole corpus the extractor drops are plugins/repo-hygiene/CHANGELOG.md: ## [Unreleased] and docs/conventions/config-cascade/CHANGELOG.md: ## Renamed — 2026-07-23; both were examined and neither is a version heading, so dropping them is correct.
ITEM 4: CONFIRMED — both gaps are real, unchanged by #2341, deliberate, and tracked. The 04822fc4 instance was verified from primary source, not taken on the PR's word: git diff --name-only 04822fc4^1 04822fc4 lists .github/workflows/ci.yml, plugins/docs-hygiene/CHANGELOG.md, scripts/check-changelog-parity.sh, scripts/check-changelog-parity.test.sh — no manifest; ## [0.9.7] is present at 04822fc4^1 and absent at 04822fc4. Replayed in a fresh clone (checkout 04822fc4, overwrite only the gate script with the bb817c6c version — safe, since bumped_candidate reads committed state via git diff, not the worktree), --check-bump 04822fc4^1 → EXIT=0. A real deletion, still unpoliced. Gap (b) likewise: manifests=(plugins/*/.claude-plugin/plugin.json), so docs/conventions/*/CHANGELOG.md is structurally outside --check-bump; a fixture where a plugin bumps cleanly and docs/conventions/foo/CHANGELOG.md absorbs ## 1.0 and ## 1.1 → EXIT=0. Judgement: deliberate, not a defect of this PR. Open issue #2342 covers both by name (item 1 cites 04822fc4 verbatim; item 2 covers convention changelogs) plus the fail-open fork-point read. Deliberate-and-tracked.
ITEM 5: CONFIRMED — zero false positives on the live corpus, with numbers.
Static, tree 9fe90860: 65 plugin changelogs, 11 convention changelogs, 65 plugin manifests. --check → EXIT=0. --check-order → EXIT=0, All 76 changelog(s) read newest-first with no duplicate versions.
Replay, selection rule verbatim git log --format=%H -n 100 origin/main -- 'plugins/*/CHANGELOG.md' (tip 9fe90860), each commit C replayed as its own PR (tree at C, base C^1) in a throwaway clone with the gate script replaced by an instrumented copy of the bb817c6c version (one added echo reporting per-plugin fork/head heading counts; bash -n clean):
| commits selected | 100 |
| replayed | 100 |
| skipped | 0 |
| clean (exit 0) | 100 |
| fired (exit ≠ 0) | 0 |
of which ABSORBED CHANGELOG HEADING |
0 |
| in-scope plugin-changelog comparisons | 278 |
| fork-point headings compared (sum) | 10233 |
Caveat carried forward from the PR body: replaying C vs C^1 is not what CI saw at merge time, so fires from --check-bump's other checks would be artifacts — but for the absorbed-heading check C^1 is an ancestor of C, so fork point and parent coincide and these numbers are exact. And per Item 3: a zero here proves the check is quiet on this corpus, not correct on the format.
VERDICT: DEFECT FOUND — #2341 fixes the two false positives it targeted (plus an unnamed third on trailing whitespace) and does not go soft, but by routing the check through changelog_versions it made inline-linked headings ## [<v>](<url>) - <date> invisible to preservation: absorbing one now passes where the pre-fix script failed it, and reformatting a plain heading into that form false-positives on a required gate — a form this same gate accepts for the bumped version.
Suggested remediation (not a revert — #2341 is a net improvement). The two obvious fixes are not equivalent, and the tension should be decided rather than papered over:
- Relax
changelog_versions' trailing anchor so a version is recognised when followed by(. This is the one I'd take: it keeps the single shared reader the file's own header insists on ("Shared by --check-order and --check so the two directions of the parity pair cannot read a changelog differently"), and it fixes all four symptoms at once — the FN, the FP,CHANGELOG AHEAD OF MANIFEST, andMISORDERED CHANGELOG. It does change behaviour for all three consumers, so it needs tests on each. - Give preservation its own extractor. Cheaper to reason about in isolation, but it re-creates the second reader fix(scripts): key the absorbed-heading check on the version, not the line (#2327) #2341 explicitly deleted and leaves
--check/--check-orderstill blind. It also contradicts the design note this PR quoted as its own justification.
Either way the fix needs a test on the linked form in both directions, since the corpus cannot exercise it. Best folded into #2342's "preservation as its own diff mode" work rather than patching the anchor twice.
SCOPE — what I did NOT examine: the 90 lines added to scripts/check-changelog-parity.test.sh (I neither read the new cases nor ran the suite — my fixtures are independent of it); shellcheck / check-shell-portability.sh / the conflict-marker sweep; the CI workflow wiring and required-context names; --check and --check-order semantics beyond the linked-heading fixtures above and one corpus run each; #2342 item 3 (the fail-open 2>/dev/null fork-point read); duplicate-version fork headings (presence-keying means deleting one of two identical ## [1.0.0] sections would pass — --check-order already rejects duplicates, so I did not pursue it); PR review threads and CI history. Platform delta: everything above ran on Cygwin bash 5.3.9(1) with GNU grep/awk/comm; CI runs Linux with different builds, and declare -A reset semantics, mapfile, [[:space:]], and CRLF handling are all places that can diverge. The two Item 3 findings are regex-level and platform-independent; the cross-plugin non-finding is the one most worth re-confirming on the CI image.
1 participant
Summary
#2290's absorbed-heading check diffs whole heading lines:
changelog_bracket_headings()isgrep -Ewithout-o, so it emits the entire line, andcomm -23compares those lines. Any edit to an existing release heading therefore reads as a deletion — including both annotations Keep a Changelog itself prescribes, which preserve the heading they annotate.Measured against the shipped script on
main, copied unmodified into a throwaway repo (#2327 carries the full reproduction):--check-bumponmain## [1.0.0] - 2026-01-01 [YANKED]ABSORBED CHANGELOG HEADING … ## [1.0.0]## [1.0.0] - 2026-01-01ABSORBED CHANGELOG HEADING … ## [1.0.0]1.0.0is documented in both. Nothing was absorbed. This is a required merge check, so a false positive here blocks every in-flight PR that touches a heading line, not just its author's.Fix
Key the comparison on the version each heading names, through the shared
changelog_versionsextractor, instead of on the rendered line.## [1.0.0]and## [1.0.0] - 2014-12-13 [YANKED]are one release, so annotating passes; renaming## [0.51.8]to## [0.51.9]still deletes the version0.51.8, so a relabel is still caught.changelog_bracket_headings()is dropped. It was a second reader of the same file, which the header's ownrendered_linesnote argues against — "One tracker, not three, so the modes cannot drift." The failure message still prints## [<version>], so nothing downstream of it changes.Two behaviour changes worth naming, both consequences of keying on the version:
CHANGELOG FORMAT's concern, not preservation's.## 0.9.0 — 2025-12-01at all, so absorbing that section was invisible. Both directions have a test.Deliberately out of scope
All pre-existing from #2290 and tracked in #2327 — a false positive on a required gate is worth a surgical fix, not a redesign in the same change set:
--check-bump's loop, so an absorption in a change set that does not bump the manifest is unpoliced (verified EXIT=0).docs/conventions/*/CHANGELOG.mdis outside--check-bump's scope entirely, so an absorption there is unpoliced (verified EXIT=0).2>/dev/nullprocess substitution ongit showis fail-open: a git read that genuinely fails yields an empty fork-point set and reads as "nothing to preserve".#2327 sketches the fuller alternative (preservation as its own diff mode over every changed changelog under both roots), with a worked implementation on
fix/2264-changelog-preservation.Test plan
Suite:
PASS=62 FAIL=0(57 pre-existing + 5 new),bash scripts/check-changelog-parity.test.sh.New cases:
dating an existing heading is not an absorbed heading— the FP above.marking a release '[YANKED]' preserves its heading and passes --check-bump— the FP above.a relabelled predecessor heading still fails --check-bump— the check must not go soft.reformatting a heading between the two accepted forms is not an absorbed heading.absorbing an unbracketed release section is caught— coverage a bracketed-only matcher could not have.#2290's own absorption test is untouched and still passes, asserting on## [0.51.8]in the output.The new tests are load-bearing — established before the fix existed, by running
main's shipped script unmodified against the same two fixtures the new tests use (full transcript in #2327):main's script## [1.0.0]→## [1.0.0] - 2026-01-01 [YANKED]ABSORBED CHANGELOG HEADING … ## [1.0.0]## [1.0.0]→## [1.0.0] - 2026-01-01ABSORBED CHANGELOG HEADING … ## [1.0.0]The symmetric in-tree check — restoring only
scripts/check-changelog-parity.shfromorigin/mainand re-running the suite — was not run: the command was refused by a local permission classifier, and it was not reshaped to get around it. The table above is the same evidence by a different route.Zero false positives on real history.
mainis squash-merged, so each commit on it is a merged PR. Every one of the 60 most recent commits touchingplugins/*/CHANGELOG.mdwas replayed as its own PR (tree atC, baseC^1) in a throwaway local clone, running this branch's--check-bump:cf743d61could not be checked out)ABSORBED CHANGELOG HEADINGThe single fire is
78dbb10e, reportingUNDOCUMENTED BUMPand threeVERSION REGRESSIONs — none from the check this PR touches.The zero is honest, and it is itself a finding. The one commit in recent history that really does delete a release heading —
04822fc4, which foldeddocs-hygiene's never-released## [0.9.7]into## [0.10.0]— is in the replay window (verified) and is not caught, because it changed no manifest at all (git diff --name-only 04822fc4^1 04822fc4lists onlyci.yml, that changelog, and the two gate files). Living inside--check-bump's loop, the check never reaches a plugin whose version did not change. That is #2327 item 3, now demonstrated on real history rather than a synthetic fixture. This PR does not change that scoping — it fixes the false positives — so the sweep's role here is to show the fix introduces no new fires across 59 real commits, not to demonstrate detection. Detection is demonstrated by the unit cases.Replaying a squash commit against its parent is not identical to what CI saw at merge time — CI resolved the fork point of the original branch, which for a stale branch sat further back — so fires from
--check-bump's other checks (VERSION REGRESSION,UNDOCUMENTED BUMP) are artifacts of comparing against a parent the branch never saw, plus checks that did not exist when those commits merged. The absorbed-heading check is unaffected:C^1is an ancestor ofC, so the fork point and the parent coincide and the comparison reads exactly the diff that commit introduced.Other gates, locally:
shellcheck --rcfile=.shellcheckrc -xon both files → clean;scripts/check-shell-portability.sh --pathson both files →No unexcused GNU-only constructs in 2 shell file(s).Conflict-marker sweep, both files:
<<<<<<<0,|||||||0,>>>>>>>0,=======4 — all four pre-existing# ==== <section> ====banner comments in the test file, none introduced here (0 marker-shaped lines among this diff's additions).Related
Closes #2327 — the live false positives it reports. Its remaining items (the scoping gaps and the fail-open fork-point read, all pre-existing from #2290 and untouched here) were split into #2342 so this close is honest rather than partial.
#2342 carries the remaining scoping work. #2290 introduced the check being repaired; #2264 is the defect it closed. #2154 / #2159 are the SIGPIPE regression in this same gate — the reading discipline is untouched here, since neither
changelog_versionsnormapfileexits before EOF.