ci(pr-checklist): require new changelog entries to cite their PR - #104
Conversation
AGENTS.md already says to link the PR/issue, but the rule was being missed routinely for a structural reason rather than carelessness: the changelog entry is written BEFORE the PR exists, so at authoring time there is no number to cite, and nothing afterwards prompts you to come back. PRs #100, #101, #102 and #103 each shipped an entry with no self-reference until a human spotted it, one at a time. So make it mechanical. When a PR adds a new CHANGELOG.adoc entry line, at least one added entry must reference this PR (pull/NN or #NN, word- bounded so #100 does not match inside #1000). Deliberately narrow, to avoid false failures: - only ADDED asciidoc bullet lines count, so reflows, or edits to an older entry, are ignored; - a PR that touches the changelog without adding an entry of its own can opt out with 'changelog-ref: N/A - <reason>' in the body, mirroring the checklist's existing N/A convention; - bots are exempt, as with the checklist gate. Needs pull-requests: read, to fetch the CHANGELOG.adoc patch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
Dependency ReviewThe following issues were found:
License Issues.github/workflows/pr-checklist.yml
OpenSSF Scorecard
Scanned Files
|
|
Claude finished @astubbs's task in 1m 43s —— View job Code Review
Finding: the new gate didn't actually run its check — it hit its own opt-out by accident
const optOut = body.match(/changelog-ref:\s*N\/?A\b[^\n]*/i);This regex isn't anchored to a line start and isn't scoped to a dedicated declaration — it matches the literal string This PR's own body does exactly that, in the "What it checks" section:
Look at the actual run for this PR (job 91310634762): The new step took the opt-out branch and returned early — it never reached the "does the added entry cite this PR" check at all. That's a real problem beyond just this PR being a bad test case: the added Two consequences:
Suggested fix: anchor the opt-out to its own line, e.g. with const optOut = body.match(/^changelog-ref:\s*N\/?A\b.*$/im);and then this PR would need to either add that opt-out on its own line deliberately, or (better, since this PR does add a real entry) cite Other notes (not blocking)
|
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
|
Claude finished @astubbs's task in 2m 51s —— View job Claude finished the review
SummaryThe mechanism (checking Finding: editing an existing entry is treated as "adding" a new one
const addedEntries = changelog.patch
.split(/\r?\n/)
.filter(l => /^\+\s*\*\s+\S/.test(l))
.map(l => l.replace(/^\+/, ''));The PR description states: "Reflows, or edits to an older entry, are ignored — so a typo fix on last release's notes doesn't demand a self-citation." That isn't what happens, though. Every entry in -* Dependencies and build plugins refreshed ... JUnit 5.10.2 -> 5.14.4 ...(#73)
+* Dependencies and build plugins refreshed ... JUnit 5.10.2 -> 5.14.3 ...(#73)The A fix could pair adjacent Minor / non-blocking observations
|
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
No new clones introduced by this PR. Powered by astubbs/duplicate-code-cross-check |
✅ SpotBugs ReportNo bugs found (new bugs only — baseline from base branch excluded). |
🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its |
…il in changelog gate Review findings on #104, both confirmed against the live run: - The changelog-ref: N/A opt-out regex matched anywhere in the PR body, so a body that merely documented the syntax (this PR's own) silently disabled the gate. Now anchored to its own line with a mandatory reason. - Every '+' bullet in the patch was treated as a NEW entry, so a single-line edit to an old entry (rendered by git as a -/+ pair) demanded a bogus self-citation. '+' bullets now consume unmatched '-' bullets from the same change block; only leftovers count as new. Verified with 13 synthetic-patch cases plus an end-to-end run against this PR's real body and CHANGELOG.adoc patch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both review findings addressed in 0fce24e - thanks, both were real and the first was proven live by this PR's own CI run.
Fixed: the opt-out is now
Fixed with the suggested pairing approach: each
Done - 13 synthetic-patch cases (pure addition, single-line edit, edit + new entry in either order, block resets across hunks, pure deletion, non-bullet header tweaks, and 6 opt-out regex variants) all pass, plus an end-to-end run of the fixed logic against this PR's real body and real
Both now have short comments in the workflow at the relevant spots. Not changed (agreed non-blocking): the duplicated The PR body's opt-out bullet has been updated to state the opt-out must be on its own line with a required reason. |
|
Claude finished @astubbs's task in 3m 59s —— View job Tasks
SummaryBoth previously-reported issues are fixed correctly:
Finding: the edit/add pairing is positional, so it can still misattribute in a mixed block
-* Entry A old text (#10)
-* Entry B old text (#20)
+* Entry A new text (#10)
+* NEW Entry C text (no ref yet)
+* Entry B new text (#20)Traced through the loop:
This is realistic: it just requires one PR to edit ≥2 existing entries' wording and insert a new entry between them in the same hunk — plausible since Given how much care already went into narrowing this gate to avoid false failures, this feels worth at least a documented limitation (alongside the two already-noted ones for large-diff truncation and wrapped continuations), if not a content-aware pairing fix (e.g. matching on a shared trailing Other notes (not blocking)
|
…gate Third review finding on #104: the -/+ bullet pairing was positional, so a block holding two edits with a new entry inserted between them (-A, -B, +A', +NEW, +B') paired off by count into exactly the wrong answer - +NEW consumed as an edit and never checked, +B' falsely flagged as new. Both failure modes at once: the uncited new entry the gate exists to catch sails through, and a plain wording edit is told to self-cite. Pairing is now by CONTENT, strongest match first: a shared (#NN) citation, else word overlap above a threshold. Position is no longer a signal, so ordering and insertions can't misattribute. Writing the tests turned up a fourth case the review didn't mention - a deletion plus an unrelated addition in the same block also silently exempted the new entry. The logic moves to .github/scripts/changelog-ref-gate.js so it can be tested rather than mirrored; the job runs those 25 tests before the gate, matching the existing 'self-test the scanner' step in copyright.yml. The three cases above fail against the old implementation, so they are real regression tests. Net effect on the workflow is less inline JS, not more. AGENTS.md now records that the reference convention is CI-enforced and how to opt out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Third finding addressed in c6846b6. The trace was exactly right - I reproduced it before changing anything, and the old logic reported
Rather than document it, I removed the positional assumption entirely. Pairing is now by content, strongest match first: a shared Writing the tests turned up a fourth case the review didn't call out, same failure class: a deletion plus an unrelated addition in one block (
On testing: a mirrored copy of the logic in a throwaway script would drift from the workflow, so the gate's logic now lives in 25 tests, no dependencies, plain Two notes on the checkout this required: it's the PR's own tree with
|
|
Claude finished @astubbs's task in 3m 31s —— View job Tasks
SummaryAll three previously-reported issues (opt-out anchoring, positional edit/add mispairing, and the follow-up deletion-plus-unrelated-addition case) are fixed correctly in Finding: content-based pairing can still mispair when entries share boilerplate wording but no citation
Concretely, in a single contiguous diff block: -* build(deps): Bump Kafka to 3.6.2
+* build(deps): Bump Testcontainers to 1.21.0
+* build(deps): Bump Kafka to 3.9.0
This is inherently a heuristic-matching limitation rather than a clear-cut bug, and it only bites when two uncited bullets in the same contiguous hunk are textually similar — narrower than the positional bug already fixed, but realistic for exactly this repo's own dependency-bump entries. Given the care already taken to document the other two known limitations (large-diff truncation at line ~111, wrapped continuations at line ~121), this seems worth the same treatment: either a short comment near Other notes (not blocking)
|
Four things #102 does not resolve, so they are not lost at merge: - A green 'review' check can mean the reviewer never ran. claude-code-action refuses to run when the workflow file differs from the default branch's copy, and reports that skip as SUCCESS. Any PR editing a validated workflow therefore gets a green review check that verified nothing - it bit #102 and #104, both workflow changes. The control itself is correct (it stops a PR rewriting the reviewer's own grants and having that version execute), but it is invisible unless you read the job log, so it needs saying out loud. - Credential exposure is unresolved, not cleared: the job runs PR-authored build code alongside CLAUDE_CODE_OAUTH_TOKEN, and #102 widened the blast radius to pull-requests: write. Bounded by fork PRs not getting secrets and by the allowlist - neither of which answers the actual question about token scrubbing. - The reviewer cannot lint workflows: actionlint is not granted, so on a workflow PR it can only eyeball YAML, as it said itself on #102 ('trusted, not verified'). Cheap to grant, but it must land in a non-workflow PR or the validation skip means it is never exercised. - bin/ci-integration-test.sh may not fit the 30-minute cap, and none of #102's setup (cache, Docker, the grants) has actually executed yet - same reason. Filed under CI reliability / gate issues rather than refactoring.md: these are in-flight gate concerns tied to open PRs, not deferred code work. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
…g-pr-reference # Conflicts: # CHANGELOG.adoc # README.adoc
…tion Review finding on #104, reproduced rather than taken on trust. When neither bullet in a candidate pair carries a citation, pairing falls back to plain word overlap - and this changelog is full of same-template entries (build(deps): Bump <lib> to <version>). Two uncited bullets in one diff block can then pair on boilerplate alone. Constructed the failure: removed 'Bump Testcontainers to 1.20.0', added a genuinely new 'Bump Kafka to 3.9.0' plus a reworded edit of the Testcontainers line. The gate pairs the NEW line as the edit, so it escapes the citation check, while the real edit is falsely flagged. Wrong in both directions, and the escaping-new-entry half is exactly the false negative this gate exists to prevent. Not tuned blind - the threshold is someone's considered choice and a guessed number could easily be worse. Instead: - pinned by a regression test, so any future change to EDIT_THRESHOLD or editScore surfaces here rather than silently. The test says outright that it records accepted behaviour, not desired behaviour, and that a failure caused by the gate getting stricter is an improvement. - documented next to EDIT_THRESHOLD with the direction to take if it ever bites: FAIL CLOSED, raising the threshold so an uncertain pair is treated as a new entry needing a citation. That trades a silent miss for a visible false alarm - and the false alarm has an escape hatch (changelog-ref: N/A - ...) whereas the miss has nothing. Also adds two positive pairing tests (shared citation survives a total rewording; distinct bumps pair correctly when the library name breaks the tie), taking the suite to 28. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
|
Claude finished @astubbs's task in 1m 51s —— View job Tasks
SummaryThis round only adds one commit ( Verified by manual trace (couldn't execute
Both traces match the committed expectations, so the test isn't accidentally green-by-construction. This is a reasonable resolution to the finding: rather than tuning the threshold blind (which the author correctly notes could easily make things worse without evidence), it's documented in-place next to No new issues found in this round. Combined with the three earlier fixed findings (opt-out anchoring, positional edit/add mispairing, deletion-plus-unrelated-addition), the gate's logic and its self-documented limitations look solid and consistent with Note on tooling: I was not able to actually execute |
…g-pr-reference # Conflicts: # CHANGELOG.adoc # README.adoc
|
Claude finished @astubbs's task in 1m 23s —— View job Tasks
SummaryNo functional changes since the last full review round (commit Confirmed nothing relevant to the changelog-ref gate changed in the merge:
I was not able to execute No new findings. All previously-reported issues (opt-out anchoring, positional edit/add mispairing, deletion-plus-unrelated-addition, same-template pairing) remain fixed and documented as of |
* ci(review): one review per PR at a time, newest wins PR #107 got two bot reviews 23 seconds apart. claude-code-review.yml had no concurrency group at all - unlike maven.yml, pr-checklist.yml, copyright.yml and the highcpu lane - so the run started by opening the PR was never superseded by the run started by the next push. Both completed, both posted. The triggering sequence is not unusual, it is what this repo's own rules produce: #104's changelog gate requires the entry to cite the PR number, and that number does not exist until the PR is opened. So opening a PR is routinely followed within seconds by a push to add the reference - which is exactly the double-trigger. Keyed on the PR number rather than the ref, so it also covers reopened PRs and cannot collide with anything else keyed on a branch name. Deliberately a separate PR rather than folded into #107: touching claude-code-review.yml makes a PR unreviewable by the bot (claude-code-action refuses to run when the workflow differs from the default branch's copy), and #107's fork-repo and default-branch guards are worth having reviewed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn * docs(changelog): cite this PR (#109) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn * ci(changelog-gate): cite the issue, not this PR Requiring the entry to cite its own PR was a nuisance by construction: the number does not exist until the PR is opened, so every PR needed a second push purely to add the reference. That push is also what double-triggered the review workflow, which is the other half of this PR. An issue number is known before the work starts, and 'which reported problem does this address' is what a changelog reader actually wants - the PR is an implementation detail they can reach from the issue. Two deliberate constraints: - The citation must be an explicit /issues/ link. A bare #NN cannot be told apart from a PR reference without an API call, since GitHub numbers issues and pull requests from one sequence - and 'cite the issue' is the entire point. Fork and upstream issues both count. - Only user-visible sections require one: Breaking, Improvements, Fixes, Examples. Build & CI is exempt, and that is not laziness - of the 12 Build & CI entries predating any of this, 7 cite nothing at all and the rest cite a PR. This project's tooling work is self-directed and has no issue behind it, so requiring one would mean inventing issues or writing changelog-ref: N/A on every CI PR - the same paperwork this change removes, pointing the other way. Existing entries without an issue are left alone. Only newly ADDED entries are checked, so nothing needs backfilling. The opt-out is unchanged for the genuine no-issue case. AGENTS.md's reference convention is updated to match, since it documented the old rule. 34 unit tests, including that a pull link and a bare #NN both correctly fail to count as issue citations. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn * ci(changelog-gate): read the changelog, and drop the clever half Review on #109 found the section detection was inert, and proved it against this PR's own diff: git has no funcname pattern for asciidoc, so a hunk header for CHANGELOG.adoc reads '@@ ... @@ endif::[]' rather than the heading, and entries are one long line each so three lines of context never reach one either. Section resolved to null for essentially every real entry - and since an unknown section counted as exempt, the gate passed everything silently. It was decoration. Fixed by reading the CHANGELOG itself rather than inferring from the patch. The job already checks the repo out, so the file is right there: find the added line, walk back to the nearest '=== ' heading. Verified end to end against the real changelog - a real Build & CI entry resolves to Build & CI and passes, a real 'fix:' entry with no issue link is flagged. Also took the chance to cut this down, per feedback that it had got too complex for what it is. Gone: Dice-coefficient similarity, edit scoring, content-based pairing of removed against added bullets, EDIT_THRESHOLD and its pinned mispairing limitation. 176 -> 96 lines of logic, 269 -> 147 of tests. What that costs, stated in the code rather than discovered later: editing an old uncited entry now asks for a citation it never had. Telling edits from additions needs the fuzzy matching that was just removed - which was the largest and subtlest part of the file and still mispaired same-template entries. The changelog-ref: N/A opt-out covers the rare case. That is a better trade than machinery nobody can follow, for a check that exists to remind us, not to withstand an adversary. The test suite now models a realistic patch - no heading anywhere in the visible context - which is exactly the case the old tests all avoided by putting '=== Fixes' in the synthetic hunk header. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…tinc#893 names itself The changelog-issue gate that master gained in #104 flags two entries on this branch. One is a real omission: upstream PR confluentinc#893 says "issue confluentinc#894" in its own body, and confluentinc#894 is "Offset reset when frequent rebalancing" - precisely the problem the entry describes. It now cites it. upstream-map.yaml recorded issues: [] for this cherry-pick, so the map was wrong rather than merely thin. Corrected to [894], with a note saying what it used to say, and last_checked moved to today. The other flagged entry, the shards.max.size metric, genuinely has no issue: upstream confluentinc#905 opens "I want to add a metric..." and cites nothing. Its related: [71] is "Health-checks", far too general to claim as the request. That one takes the gate's changelog-ref opt-out in the PR body rather than an invented citation - which is the trade the gate's own comments call for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
1 participant
Description
AGENTS.mdalready says changelog entries must link the PR ("a bare#NNrefers to this fork ... and link the PR/issue"). It was being missed anyway — and for a structural reason rather than carelessness.The entry is written before the PR exists. At authoring time there is no number to cite, and nothing afterwards prompts you to come back. PRs #100, #101, #102 and #103 each shipped an entry with no self-reference, and each was caught one at a time by a human reading the diff. That is exactly the kind of "you must remember" rule worth making mechanical.
What it checks
Extends the existing
PR Checklistjob with a second step: when a PR adds a newCHANGELOG.adocentry, at least one added entry must reference this PR (pull/NNor#NN).The logic lives in
.github/scripts/changelog-ref-gate.jsrather than inline in the workflow, and is unit tested (changelog-ref-gate.test.js, 28 tests, plain node, no runner). The workflow runs those tests before the gate, so a regression in the gate fails the job loudly instead of silently mis-judging PRs. That extraction was not gold-plating: this logic has already shipped two real misjudgements (an opt-out regex that a body merely quoting the syntax could trip, and positional add/remove pairing that mis-read a block containing both an edit and a new entry), which is exactly the profile of code that earns tests. It mirrors the existingcopyright.ymlself-test step.Pairing is by content, not position: an added bullet is treated as an edit of a removed one when they share a
(#NN)citation, or failing that when their wording overlaps enough. Position alone turns-A, -B, +A', +NEW, +B'into precisely the wrong answer —+NEWconsumed as an edit and never checked.Deliberately narrow, to avoid false failures:
#100doesn't match inside#1000. Verified against a real patch.changelog-ref: N/A - <reason>on its own line in the body (the reason is required), mirroring the checklist's existingN/Aconvention. Mentions of the syntax mid-prose or in quotes don't count.Needs
pull-requests: readto fetch theCHANGELOG.adocpatch, and anactions/checkout(withpersist-credentials: false, since nothing in that step consumes secrets) so the gate module and its tests are on disk.AGENTS.mdgains a pointer to the extracted module.Known limitation (raised in review, confirmed, documented)
When neither bullet in a candidate pair carries a citation, pairing falls back to plain word overlap — and this changelog is full of same-template entries (
build(deps): Bump <lib> to <version>). Two uncited bullets in one diff block can then pair on boilerplate alone.I reproduced it rather than reasoning about it: given a removed
Bump Testcontainers to 1.20.0, a genuinely newBump Kafka to 3.9.0, and a reworded edit of the Testcontainers line, the gate pairs the new line as the edit — so it escapes the citation check, while the real edit is falsely flagged. Wrong in both directions.Left as-is rather than tuned blind, but pinned by a regression test so any future change to the threshold surfaces here instead of silently, and documented next to
EDIT_THRESHOLDwith the direction I'd take if it ever bites: fail closed — raise the threshold so an uncertain pair is treated as a new entry needing a citation. That trades a silent miss for a visible false alarm, and the false alarm has an escape hatch (changelog-ref: N/A - ...) whereas the miss has nothing.Notes
actionlint; the matching logic was exercised against PR docs: index every TODO/FIXME/XXX marker, triage into the existing refactoring backlog #103's real changelog patch, confirming it detects the added entry, matches#103, and rejects both#100and#1030.Checklist
CHANGELOG.adoc) - Build & CI, citing this PR;README.adocregeneratedAGENTS.md; the workflow's inline comments carry the rationale.github/scripts/changelog-ref-gate.test.js, 28 unit tests run by the job before the gate itself, including a pinned regression test for the known pairing limitation aboveubuntu-latest, not the self-hosted box; adds onlypull-requests: read, and reads the PR diff via the API without executing anything from it🤖 Generated with Claude Code