docs(changelog): generation covers 0.6.0.0 now, not "from the next release on" - #210
Conversation
…lease on" #113 moved the changelog from per-PR entries to release-time generation. The policy is right; the sentence describing it put 0.6.0.0 on the wrong side of the line. "Everything up to and including == 0.6.0.0 is hand-written and now frozen; from the next release on, each section is generated" reads as: v6 is finished, generation starts with v7. Generation is in effect now and 0.6.0.0 is the release it applies to. The misreading is not hypothetical - an agent hit it this week and concluded that whatever currently sits under == 0.6.0.0 is what v6 ships with. The trap is that "frozen" is doing two jobs in one sentence. It is a statement about text in the file (leave it alone) and it reads as a statement about a release (settled, quotable). The rewrite splits them: a section's state follows from whether its release has SHIPPED, and 0.6.0.0 has not, so its contents are working text that release-time generation will replace. Both wrong readings are named and denied explicitly, because a reader who has already formed one will not give it up to an implication. Also states the exception that previously had to be inferred. "Do not add entries in a feature PR" does not obviously permit a correction, and #198 made one - the Kafka client entry said 3.9.1 after the pom moved to 3.9.2. The test is now written down: changing an existing claim to be true is allowed, adding information about a change is the generator's job. Three locations carried the phrase (AGENTS.md's Changelog section and its "where things live" row, and the CHANGELOG header NOTE). A fourth, in docs/inflight/release-0600-blockers.md, had already copied it. README.adoc is unaffected - it stopped embedding the changelog in #113 and only links to it. Separately, the same section claimed the PR Checklist changelog gate "should never fire ... since PRs no longer touch the file". That was false when written: #51, #57, #105 and #106 all modify CHANGELOG.adoc and all predate the policy. The gate also does not enforce the policy in either direction - it only fails UNCITED entries, so #57's cited entries would pass while violating it, and it cannot tell an edit from an addition, so a correction like #198's trips it and needs the changelog-ref: N/A opt-out. The gate is left alone deliberately, with the reasoning in docs/inflight/ci-changelog-gate-scope.md. Rejecting every addition would fail the one edit the policy permits, since "adds an entry" and "corrects an entry" are both +* lines - the gate's own header records that fuzzy-matching removed bullets against added ones was built, and abandoned as its subtlest code. And the strict version's only escape is a self-declared PR-body line, so it would buy no enforcement over the written rule while putting an opt-out on every legitimate correction, and would block four PRs that followed the rule that was live when they were opened. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Claude finished @astubbs's task in 3m 32s —— View job Review complete ✅Tasks
SummaryDocs-only PR, one commit, no behaviour change. It corrects a genuine ambiguity left by #113: the old wording ("frozen ... from the next release on, each section is generated") put Verification performedI independently checked the load-bearing factual claims rather than taking the PR body at face value:
Observations (non-blocking)
No bugs or factual errors found. The rewording is internally consistent (AGENTS.md's two spots, |
✅ 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). |
The note said what the AGENTS.md subsection above it already said. Its one unique contribution - why tightening the gate to reject every addition was rejected - now sits inline where the gate is described, so a reader gets the reasoning without a second hop. docs/inflight/ tracks work that is open and needs picking up. This is a settled decision with its rationale, which is what AGENTS.md is for. A note recording a decision already made is the stale-marker failure that directory's own charter warns about. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
check-review-posted.sh:94 ran `printf | grep -q` under `set -o pipefail`. grep exits the instant it matches, printf then dies with EPIPE (141), and pipefail promotes that to the pipeline's status - so a comment citing the run made the check fail. It only bites when more than one pipe buffer (64 KiB) of comment data follows the match, which is why it looked intermittent. Observed on #198, #199, #204 and #210. On #210 the review comment sat 4.7 KB into the stream with a 127 KB similarity report behind it; the job log carries `printf: write error: Broken pipe` directly above the failure. The emitted error blamed the workflow-validation guard, which none of those PRs had tripped. The fix is the herestring this repo already prescribes: the same bug class is guarded against in bin/test-check-copyright-headers.sh, whose comment records it "seen live in CI". check-review-posted.sh shipped in that same commit without the guard. Two tests, both verified to fail against the old line: - functional: match found, then >64 KiB of further comments. The existing cases dance around this - case 5 buries the match but keeps it small, case 6 puts it last so nothing follows to fill the buffer. - structural: the checker may not pipe into grep -q or awk at all, mirroring the copyright scanner's guard, so the next instance of the class is caught rather than the next occurrence of this one. The error text is left alone: it misdiagnosed those four PRs only because of this bug, and becomes accurate again once the SIGPIPE path is gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
Two follow-ups pushed1. The red
|
|
Claude finished @astubbs's task in 3m 45s —— View job Review complete ✅Tasks
SummaryTwo independent, well-scoped fixes, both docs/tooling with no runtime behavior change to the product itself. 1. Changelog policy wording ( 2. Verification performedI independently checked every load-bearing factual claim rather than trusting the PR body:
Observations (non-blocking)
No bugs or factual errors found. Every checkable claim held up against direct verification. |
#211 added a rule to bin/AGENTS.md - never pipe into `grep -q` under pipefail - while the repo broke it in four places. A rule shipped with known violations is not a rule. `writer | grep -q PATTERN` under `set -o pipefail` reports failure exactly when it MATCHES: grep exits on the first hit, the writer takes EPIPE (141), pipefail promotes that to the pipeline's status. It only fires once the writer still has more than one pipe buffer (64 KiB) to write, so it passes every small fixture and surfaces when real data grows. - check-review-posted.sh:94 - live. Reported "no review posted" on four PRs whose reviews had posted (#198, #199, #204, #210). - check-quarantine-owners.sh:98,110 - latent, and close. `git show` pipes a whole source file into `grep -q` inside an `if`. The largest file in the repo is 65,185 bytes against a 65,536-byte buffer: 351 bytes of headroom, on a file two open PRs are adding lines to. It would fail as "annotation missing", not as a pipe error. - quarantine-lane-report.sh:201 - the `||` makes a SIGPIPE take the wrong branch and silently retarget, rather than shielding it. All four become herestrings, which have no pipeline to fail. Adds bin/check-shell-sigpipe.sh, run in CI beside the copyright self-test (seconds, no JDK) and granted to the reviewer. Verified both directions: clean on this tree, exit 1 when the old line is reinstated. It skips itself, since its failure message necessarily contains the anti-pattern as the "wrong" half of a worked example. shellcheck does NOT detect this - run against the known-bad line, it passed clean. Hence a bespoke grep rather than adopting a linter. Note check-review-posted.sh:94 is also fixed in #210, which adds the functional regression test for it. Conflict expected and cheap; this PR fixes it because it is this PR that states the rule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
bin/test-check-review-posted.sh was missing from --allowedTools while its sibling bin/test-check-copyright-headers.sh was granted. So when #210 changed the review gate, the reviewer could not run the gate's self-test and verified the fix by reading instead - and said so - on the one PR where executing it mattered most. The workflow already runs that same script itself, at its "Self-test the review gate" step. Both spellings granted for each script: these are prefix matches, not globs, so Bash(bin/foo.sh:*) does not cover ./bin/foo.sh. Deliberately NOT widened to bin/*. The comment above the list explains why it is enumerated rather than Bash(*) - the job has no fork guard beyond sender.type != Bot and reads attacker-influencable text - and a directory glob gives that margin away just as effectively. Adds bin/AGENTS.md, because nothing failed to keep the allowlist in sync: no rule existed. `allowedTools` appears in zero docs, scripts or checks, so a new verification script stays invisible to the reviewer until someone remembers by hand. An automated "every script must be granted" check would be wrong - the list is a security boundary and most scripts belong off it - so the rule is written down instead: what earns a grant, what must never get one, that both spellings are needed, and that editing this workflow costs you the review on that PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
`writer | grep -q` under `set -o pipefail` reports failure exactly when it MATCHES: grep exits on the first hit, the writer takes EPIPE (141), pipefail promotes that to the pipeline's status. It only fires once the writer still has more than one pipe buffer (64 KiB) to write, so it passes every small fixture and surfaces when real data grows. - check-review-posted.sh:94 - live. Reported "no review posted" on four PRs whose reviews had posted (#198, #199, #204, #210). - check-quarantine-owners.sh:98,110 - latent, and close. `git show` pipes a whole source file into `grep -q` inside an `if`. The largest file in the repo is 65,185 bytes against a 65,536-byte buffer: 351 bytes of headroom, on a file two open PRs are adding lines to. It would fail as "annotation missing", not as a pipe error, sending the reader nowhere near the cause. - quarantine-lane-report.sh:201 - the `||` makes a SIGPIPE take the wrong branch and silently retarget, rather than shielding it. All four become herestrings, which have no pipeline to fail. Kept separate from the guard that enforces this, so the fixes can be reviewed - and reverted - on their own. shellcheck does NOT detect this: run against the known-bad line, it passed clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
bin/test-check-review-posted.sh was missing from --allowedTools while its sibling bin/test-check-copyright-headers.sh was granted. So when #210 changed the review gate, the reviewer could not run the gate's self-test and verified the fix by reading instead - and said so - on the one PR where executing it mattered most. The workflow already runs that same script itself, at its "Self-test the review gate" step. Both spellings granted for each script: these are prefix matches, not globs, so Bash(bin/foo.sh:*) does not cover ./bin/foo.sh. Deliberately NOT widened to bin/*. The comment above the list explains why it is enumerated rather than Bash(*) - the job has no fork guard beyond sender.type != Bot and reads attacker-influencable text - and a directory glob gives that margin away just as effectively. Adds bin/AGENTS.md, because nothing failed to keep the allowlist in sync: no rule existed. `allowedTools` appears in zero docs, scripts or checks, so a new verification script stays invisible to the reviewer until someone remembers by hand. An automated "every script must be granted" check would be wrong - the list is a security boundary and most scripts belong off it - so the rule is written down instead: what earns a grant, what must never get one, that both spellings are needed, and that editing this workflow costs you the review on that PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
`writer | grep -q` under `set -o pipefail` reports failure exactly when it MATCHES: grep exits on the first hit, the writer takes EPIPE (141), pipefail promotes that to the pipeline's status. It only fires once the writer still has more than one pipe buffer (64 KiB) to write, so it passes every small fixture and surfaces when real data grows. - check-review-posted.sh:94 - live. Reported "no review posted" on four PRs whose reviews had posted (#198, #199, #204, #210). - check-quarantine-owners.sh:98,110 - latent, and close. `git show` pipes a whole source file into `grep -q` inside an `if`. The largest file in the repo is 65,185 bytes against a 65,536-byte buffer: 351 bytes of headroom, on a file two open PRs are adding lines to. It would fail as "annotation missing", not as a pipe error, sending the reader nowhere near the cause. - quarantine-lane-report.sh:201 - the `||` makes a SIGPIPE take the wrong branch and silently retarget, rather than shielding it. All four become herestrings, which have no pipeline to fail. Kept separate from the guard that enforces this, so the fixes can be reviewed - and reverted - on their own. shellcheck does NOT detect this: run against the known-bad line, it passed clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
Picks up the two new required contexts (shell: sigpipe, workflows: action versions) so this PR can report them, and resolves the overlap with #211. check-review-posted.sh:94 - both branches fixed the same SIGPIPE line. The code was identical; only the explanatory comment differed. Took master's wording. test-check-review-posted.sh - this branch's structural guard (no `| grep -q` in the checker) is now redundant: bin/check-shell-sigpipe.sh landed on master with #211 and enforces that across every bin/*.sh, in every flag spelling, with its own self-test. Removed the local copy rather than keep two rules for one thing. Case 13 is KEPT. It is behavioural where the repo-wide guard is static: it drives the checker with a match followed by >64 KiB of comments and asserts exit 0, so it would catch a regression that reintroduced the failure by some route other than a literal `| grep -q` - which is the only shape the static guard sees. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
|
Claude finished @astubbs's task in 4m 7s —— View job Review complete ✅Tasks
SummaryThis 1. Changelog policy wording. The old text from #113 put 2. Verification performed
One thing I could not execute directly: this sandbox required interactive approval to actually run shell scripts ( Observations (non-blocking)
No bugs or factual errors found. Every checkable claim in the PR body held up against direct, independent verification. |
…anch PR Checklist failed with "12 reference(s) below #1000 do not say which repo they mean". The gate arrived in 735b1d3 (#114), which merged 17 minutes AFTER #211 - so this branch was written before the rule existed. The fork's numbers sit inside upstream's range, so a bare number is a coin flip: #114 found 48 numbers that exist in BOTH repos meaning different things. All twelve here are fork PRs, now written astubbs#NN. Pointed, given this PR is about a reference convention. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
|
Claude finished @astubbs's task in 6m 33s —— View job Review complete ✅Tasks
SummaryThe wording fix itself is sound: the old "from the next release on" phrasing did put However, digging into the two specific PRs cited as live evidence for "don't tighten the gate" turned up a real problem: the mechanism described for both examples is wrong, verified against the actual CI logs. Finding: the false-pass/false-fail examples in
|
1 participant
Description
The changelog policy from #113 is correct. Only its stated timing is wrong, and the wrong timing has already misled a reader.
#113 (
e3b1601e) moved the changelog off per-PR entries and onto release-time generation from the commit log. The sentence it wrote to describe that says:That puts
0.6.0.0on the hand-written side of the line and starts generation after it. Generation is in effect now, and0.6.0.0is the release it applies to.git log -S "from the next release"confirmse3b1601eis the sole origin of the phrase; it reached three locations, and a fourth file had already copied the claim.Why the wording, not just the fact, is the bug
An agent read it this week and concluded "0.6.0.0 is frozen, therefore v6 ships with whatever is written under that heading now." Both halves of that follow from the sentence as written.
The trap is that "frozen" is doing two jobs in one word. It is a statement about text in a file - do not add to it - and it reads as a statement about a release - settled, quotable, final. The rewrite separates them and hangs the state on a fact the reader can check: has the release shipped?
== 0.5.xand below== 0.6.0.0- the release being cutBoth wrong readings are then named and denied outright, rather than left to be inferred - a reader who has already formed one will not give it up to an implication.
The exception that had to be inferred, and now doesn't
"Do not add entries in a feature PR" does not obviously permit a correction - yet #198 made one, changing a Dependencies entry that claimed the Kafka client stayed on
3.9.1after the pom had moved to3.9.2. The rule is now explicit, with a test an author can apply:Before / after
AGENTS.md§ Changelog== 0.6.0.0is hand-written and now frozen; from the next release on, each section is generated…"0.6.0.0itself" + the shipped/not-shipped table + the two explicit anti-readings + the correction exceptionAGENTS.md"where things live" row== 0.6.0.0; later sections are generated at release time"== 0.6.0.0included - is regenerated at release time … what is under it today is working text, not the notes v6 will publish"CHANGELOG.adocheaderNOTE::0.6.0.0is hand-written and frozen. From the next release on, each section is generated…"0.6.0.0below: it has not shipped, so what you are reading there is working text and not the notes that release will publish"docs/inflight/release-0600-blockers.md(4th site, already copied)CHANGELOG.adocis frozen up to== 0.6.0.0"== 0.6.0.0section is not final … do not quote it as the release notes"README.adocis untouched and needs no regeneration - #113 stopped it embedding the changelog, and neither it norsrc/docs/README_TEMPLATE.adocrestates the policy (verified). The== 0.6.0.0section's content is deliberately not edited; regenerating it is release-time work.Second defect: the changelog gate note was false when written
The same section claimed:
Four open PRs modify
CHANGELOG.adocright now - #51, #57, #105, #106 - all opened before #113 merged (verified viagh pr view --json files). Every PR predating the policy is in the same position.Worse, the gate never enforced the policy in the first place, and misses in both directions:
changelog-ref-gate.jsonly fails entries citing no issue. fix: unbounded PCMetrics heap growth (#120, upstream #859), offset reset after rebalance (#121, upstream #894), and no hot-shard metric #57's entries all cite issues, so they pass the gate while violating the policy outright.changelog-ref: N/A - <reason>is the intended escape, and the workflow comment already names this exact case.The note now states this, and says plainly that a green gate is not compliance with the rule.
My call on tightening it: don't - documented instead
Reasoning is inlined in the
AGENTS.mdgate subsection itself, so a reader gets it without a second hop. The scope of the gate is unchanged and the citation check is not weakened. Three reasons:+*lines. The gate's own header records that fuzzy-matching removed bullets against added ones was built, became the largest and subtlest part of the file, and still mispaired entries built from a common template. Re-deriving it repeats known-failed work.changelog-ref: N/A - <reason>, a self-declared line in the PR body. The same line that legitimises a correction legitimises a policy-violating addition - so the strict gate buys no enforcement over the written rule, and costs an opt-out line on every legitimate correction.If entries do start reappearing after 0.6.0.0 ships, the mechanically honest version is a warning, not a gate: any
CHANGELOG.adocchange posts a comment asking the author to confirm it is a correction. No false failures, and it teaches the policy at the point of edit rather than at a release-time merge conflict.changelog-ref: N/A - the only
CHANGELOG.adocchange is the headerNOTE::describing how the file is maintained. No entry is added, edited or removed.Checklist
AGENTS.mdN/A, no behaviour change;.github/scripts/changelog-ref-gate.test.jswas run and passes unchangedN/A, no workflow or runner change (the gate is described, not modified)One test kept from the review-gate work
An earlier revision of this PR also fixed
bin/check-review-posted.sh:94-printf | grep -qunderset -o pipefail, which reports failure exactly when it matches, becausegrep -qexits on the first hit andpipefailpromotes the writer'sEPIPE. That fix has since landed on master via #211, so it is no longer part of this diff - merging master resolved it to a no-op.What survives here is case 13 in
bin/test-check-review-posted.sh: a match followed by >64 KiB of further comments must still pass.It is worth keeping alongside #211's guard rather than being redundant with it. That guard is static - it greps
bin/*.shfor a literal| grep -q. This case is behavioural: it drives the checker with a real oversized payload and asserts exit 0, so it catches a regression that reintroduces the failure by any route the static grep does not recognise.The existing suite could not have caught the original bug: case 5 buries the match in a multi-line body but keeps it small, and case 6 puts the match last, so nothing follows it to fill the pipe buffer.
References
Qualified as
astubbs#NNthroughout, per the gate added in #114 (735b1d3a), which merged after this branch was written. The fork's numbers sit inside upstream's range, and #114 found 48 that exist in both repos meaning different things.Known-red check
Check PR Dependenciesdoes not report on this branch - it needed an admin bypass to push. Unrelated to this PR's content.