Repository navigation
ci: cite the issue instead of the PR, and stop duplicate reviews - #109
Conversation
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
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
✅ 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 |
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
astubbs
changed the title
|
@claude review please |
|
Claude finished @astubbs's task in 7m 50s —— View job Claude finished reviewing
SummaryBoth halves of the PR are well-reasoned and the concurrency fix is straightforward and correct. There's one significant issue with the section-detection logic in the changelog gate, which I verified concretely against this PR's own diff (see below). 🔴 Section detection likely doesn't work for the common case it needs to
That's the opposite of the fail-closed philosophy the file itself argues for a few lines up (the This PR's own diff of The hunk-header trailing context is Tracing In this specific PR the outcome happens to be harmless, because the new entry lands under Suggested fix: resolve each new entry's section from the full What looks solid
Note on verificationI wasn't able to execute |
# Conflicts: # CHANGELOG.adoc # README.adoc
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
|
Fixed, and the finding was exactly right — the gate was decoration. (Reposting: my previous comment was mangled by shell quoting and lost several passages.) The section detection was inertYou proved it against this PR's own diff, which is what makes it undeniable: git has no funcname pattern for asciidoc, so a hunk header for You also identified why the tests missed it: every synthetic hunk conveniently put Fixed by reading the changelog, not the patchThe job already checks the repo out, so the file is right there: find the added line in Verified end-to-end against the real changelog rather than only synthetic input:
The regression test now models a realistic patch — Also cut it downSeparately, the maintainer's view was that this had got too complex for what it is, and I agree. Removed: Dice-coefficient similarity, edit scoring, content-based pairing of removed against added bullets, 176 → 96 lines of logic, 269 → 147 of tests. The cost is stated in the code rather than left to be discovered: editing an old uncited entry now asks for a citation it never had. Distinguishing edits from additions is precisely the fuzzy matching that was just deleted — the largest and subtlest part of the file, which still mispaired same-template entries, as you found last round. The For a check that exists to remind us, not to withstand an adversary, that is the better trade. On your tooling noteAgreed that granting the test runner would be useful — you could not execute the suite, so "34 tests pass" was author-asserted in exactly the way this workstream keeps trying to eliminate. Not adding it here, since touching 🤖 Generated with Claude Code |
Build & CI entry for the ProducerManagerTest fix. Worth recording because the visible effect is not 'a test got less flaky' - it is that the PIT mutation lane was scoring nothing at all, and its green runs had been measuring suite stability rather than mutation coverage. Build & CI is exempt from the issue-citation gate, per #109. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
1 participant
Description
Two changes, and the second is the cause of the first.
#107 received two bot reviews, 23 seconds apart.
claude-code-review.ymlhad noconcurrencygroup at all — unlikemaven.yml,pr-checklist.yml,copyright.ymland the highcpu lane, which all have one. So the run started by opening the PR was never superseded by the run started by the next push, both completed, and both posted.Both reviews found the same finding, so the duplication cost a full review run and gained nothing.
But why was there a push 23 seconds after opening? Because #104's changelog gate required each new entry to cite its own PR number — which doesn't exist until the PR is opened. So every PR needed a second push purely to add the reference. The duplicate reviews weren't bad luck; they were what our own rules produced, on every PR.
Fixing only the symptom would have left the pointless push in place. So this fixes both ends.
1. One review per PR at a time
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.
2. Changelog entries cite the issue, not the PR
An issue number is known before the work starts, so nothing has to be added after opening. It is also the more useful citation: "which reported problem does this address" is what a changelog reader wants — the PR is an implementation detail reachable from the issue.
Two deliberate constraints:
.../issues/NN[#NN]). A bare#NNcannot 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.Breaking,Improvements,Fixes,Examples.Build & CIis exempt.That exemption is evidence-led, not laziness. Of the 12
Build & CIentries predating any of this, 7 cite nothing at all and the rest cite a PR; only two entries in the whole file cite an issue. This project's tooling work is self-directed and has no issue behind it, so requiring one would mean inventing issues, or writingchangelog-ref: N/Aon every CI PR — the same paperwork this change removes, pointing the other way.Nothing is backfilled. The gate only inspects newly added entries, so existing uncited ones are untouched and no issues need creating retrospectively.
The
changelog-ref: N/A - <reason>opt-out is unchanged for the genuine no-issue case.AGENTS.md's reference convention is updated, since it documented the old PR-citation rule.Verification
34 unit tests (
changelog-ref-gate.test.js), run by the job before the gate. New coverage includes the cases most likely to be got wrong:.../issues/42[#42].../issues/857[#857].../pull/104[#104]#104=== Fixesentry, no issue=== Build & CIentry, no issueThe pairing tests from #104, including the pinned same-template limitation, are unchanged and still pass.
Notes
claude-code-actionrefuses to run when the workflow file differs from the default branch's copy and reports that skip as success, so its greenreviewcheck means nothing; and the checklist job runs the base branch's gate, so the new rule doesn't apply to this PR either. Both take effect for the next PR after merge.Checklist
CHANGELOG.adoc) - Build & CI;README.adocregeneratedAGENTS.md's reference convention rewritten for the issue rule, including theBuild & CIexemption and whychangelog-ref-gate.test.jsnow 34 tests, covering issue-vs-PR links, the section rule and the exemptionubuntu-latest, not the self-hosted box; no permission or grant changes, only run lifecycle and gate logic🤖 Generated with Claude Code