ci: fix four SIGPIPE-inverted checks, guard the class, and restore action-version automation - #211
Conversation
Dependency ReviewThe following issues were found:
License Issues.github/workflows/repo-hygiene.yml
OpenSSF Scorecard
Scanned Files
|
✅ 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). |
|
@claude review this |
|
Claude finished @astubbs's task in 3m 6s —— View job Review
Note on verification method: this review is running under The diff does what it says.
One non-blocking observation, not a defect in this diff: Minor nit: the workflow comment reflow splits "so the reviewer can" / "independently re-run what a PR" across two lines in a slightly awkward spot — purely cosmetic, no action needed. Everything else — merge-order note, the expectation that |
#211 now fixes the violations of its own ruleFair challenge: this PR added a rule to Could a static scanner have caught it?No.
The four violations
The quarantine one is the find worth knowing about. It pipes an entire source file into if ! git show "FETCH_HEAD:$relpath" 2>/dev/null | grep -qE "$QUARANTINE_ANNOTATION_ERE"; thenThe pipe buffer is 65,536 bytes. 351 bytes of headroom - on a file that #203 and #205 are both adding lines to. The first quarantined test to live in a file that crosses 64 KiB would fail this check, and the message would read "the quarantine is not yet on its base" - a missing annotation, not a pipe error. Someone would have gone looking in entirely the wrong place. The fix, and the guardAll four become herestrings - no pipeline, nothing for
Verified both directions, not just that it passes: clean on this tree, and exit 1 when the old line is reinstated. It skips itself, because its own failure message necessarily contains the anti-pattern as the "wrong" half of a worked example - noted in the code so the exclusion is not mistaken for an oversight. Scope is Overlap with #210
|
#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
|
@claude review this |
|
Claude finished @astubbs's task in 2m 54s —— View job Review
Note on verification method: same gap as the first review pass on this PR — The diff does what the PR body says.
Verified the 64 KiB headroom claim directly, since it's the kind of specific number that's usually rounded in a PR body: Checked the guard script ( One genuine but very minor limitation, not currently live: the violation regex Also non-blocking: Cosmetic, carried over from the first review pass, still unaddressed: Everything else matches the PR body: both grants present in both spellings, the enumerated-list rationale intact (not widened to No blocking issues. |
Both non-blocking findings addressed, plus one they led me to1. Split-flag blind spot - fixed, and it was worth fixingCorrect: You were right that nothing in Now matches 2. No self-test - added, 11 cases
The two new-capability cases were verified to fail against the old regex before being kept: A regression test that has never been red proves nothing. On the self-exclusion specifically - you flagged it as hardcoded by path with no coverage. Rather than test the hardcode, I removed it: the exclusion now matches on basename, so a rename or a move cannot silently switch it off. Case 10 pins that by copying the guard into a fixture directory and confirming it is still skipped. That change also surfaced a second file needing the same treatment: the self-test itself, whose fixtures are literally the shapes being detected. It now skips exactly two files - itself and 3. One I found while doing the aboveThe exclusion was written AlsoThe comment reflow flagged in both passes is fixed.
Verification: guard clean on the tree, all 11 self-test cases green, and Standing caveat on this PR's reviewsBoth passes were static - |
Both findings from the second review pass on #211, plus one I found while addressing them. The violation regex was `grep -[a-zA-Z]*q`, which a space defeats: `grep -v -q` and `grep --quiet` both slipped through. Nothing in bin/ writes them that way today, so this fixed no live bug - but the next author of a check-*.sh would have had no way to know the guard was shape-sensitive, which makes a green check a false reassurance. Now matches -q, -qE, -Eq, split flags and the long forms, while still not matching a bare word like `query` (a leading `-` is required on the q-bearing token). Adds bin/test-check-shell-sigpipe.sh - 11 cases, run in CI before the guard it protects, mirroring how the copyright and review-gate self-tests are wired. The split-flag and long-form cases were both verified to FAIL against the old regex before being kept; a regression test that has never been red proves nothing. The guard's self-exclusion was a hardcoded path, so renaming the file would have silently switched it off. It now matches on basename, and covers its own self-test too - whose fixtures are, necessarily, the shapes being detected. Those are the only two files that may contain the anti-pattern as data; excluding anything else would be hiding a real violation. Case 11 pins both. Also rewrites that exclusion from `[ A ] || [ B ] && continue` to an explicit `if`. Under `set -e` that list returns non-zero when both tests are false - the exact class of bash foot-gun this script exists to discourage, and it would have been an unpleasant thing to leave in the file that lectures about them. Fixes the comment reflow flagged in both review passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
|
@claude review this |
|
Claude finished @astubbs's task in 3m 21s —— View job Review
Note on verification method, same as both prior passes on this PR: The three follow-ups from the second review are addressed, and I confirmed each holds up:
The four SIGPIPE fix sites are all correctly converted, and none reintroduce a pipe:
One claim in the round-up comment doesn't fully hold, cosmetic only: the reflow at No blocking issues. Everything else - the guard's scope decisions (why |
The SIGPIPE checks now have their own workflowThey were bolted onto
name: Shell Hygiene
jobs:
sigpipe:
name: "shell: sigpipe"
steps:
- name: Self-test the SIGPIPE guard # 11 cases - proves the guard CAN fail
run: bash bin/test-check-shell-sigpipe.sh
- name: No SIGPIPE-prone pipes in bin/ # the guard itself
run: bash bin/check-shell-sigpipe.shSelf-test first, matching how Why not rename
|
|
@claude review this |
|
Claude finished @astubbs's task in 1m 54s —— View job Review
Note on verification method, same gap as all three prior passes on this PR: The extraction does what the commit message says.
One small, genuine inconsistency, not blocking: Not independently verifiable from the repo: the claim that No blocking issues. The split, the job naming convention, the self-test-before-guard sequencing, and the grants all match what the commit message and PR body describe. |
Both points addressed
|
astubbs
changed the title
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
Docs do not stop recurrence, and shellcheck does not detect this pattern - verified against the known-bad line, which it passed clean. So the rule the previous commit fixed four instances of gets a check. bin/check-shell-sigpipe.sh fails any bin/*.sh piping into `grep -q` under pipefail. It matches every flag spelling - -q, -qE, -Eq, split flags (`grep -v -q`) and --quiet/--silent - while still not matching a bare word like `query`, since a leading `-` is required on the q-bearing token. It skips exactly two files: itself and its self-test. Both must carry the anti-pattern as DATA - the guard shows the "wrong" half of a worked example in its failure message, and the self-test's fixtures are literally the shapes being detected. Anything else skipped would be a violation in hiding. Matched on basename, not a hardcoded path, so a rename cannot silently switch it off. bin/test-check-shell-sigpipe.sh, 11 cases, runs in CI BEFORE the guard it protects - the same sequencing copyright.yml uses. Every case that asserts a detection was verified to FAIL against a weaker regex before being kept; a regression test that has never been red proves nothing. The exclusion is written as an `if`, not `[ A ] || [ B ] && continue`: under `set -e` that list returns non-zero when both tests are false, which is exactly the class of bash foot-gun this file exists to discourage. New repo-hygiene.yml rather than bolting these onto copyright.yml, which would have left a job called "Copyright header check" doing two unrelated things. These checks are not about copyright and never were. One job per concern, named `<area>: <check>` to match the context convention already in the master ruleset (quarantine: audit, dups: clones, deps: vulnerabilities), so each is its own required-status-check candidate instead of hiding in a shared green tick. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
Dependabot bumps versions that are already there. It has nothing to say about a NEW workflow authored at an old version - which is exactly how this PR's own repo-hygiene.yml shipped at actions/checkout@v4 while 21 other uses were on @v6. A human caught it in review, and not for the first time. bin/check-action-versions.sh fails if any action is used at two versions. The rule is deliberately NOT "checkout must be v6", which rots on the next bump. "Pick one and use it everywhere" survives upgrades, precisely because Dependabot raises every use of an action in a single PR, so a consistent repo stays consistent through them. SHA pins are exempt and are not drift: the astubbs/* forks are pinned to a commit on a BRANCH on purpose, so each use site tracks a different ref by design. See #212 for why that itself wants fixing. The check immediately found drift beyond the file that prompted it: claude.yml and claude-code-review.yml were still on actions/checkout@v4. An earlier review reasoned those away as "a different action template, not a convention to match" - but checkout is checkout, and v4 runs on deprecated Node 20, which is what the Node 20 warnings in job logs are. Exempting them instead would have hollowed out the rule on the day it was written. All 24 uses are now @v6. Writing the check surfaced a bug in itself: it scanned a stray .bak file left by the very experiment testing it. Now scoped to *.yml/*.yaml. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
The github-actions ecosystem has been OFF since a470ab0 (2022), commented out with "Repo doesn't use github actions anymore". True then; the entire CI estate is now Actions across 24 workflow files. Nothing had bumped an action version in four years, which is why drift went unnoticed until a human spotted a new file at checkout@v4 while 21 other uses were on @v6. Weekly, not daily like maven: actions bump a few times a year, and daily polling would re-present the same PR. Deliberately NOT grouped, unlike maven. Grouping buys less noise, and there is little noise here to buy off. What it costs is isolation, and an action has repo-wide reach: a bad checkout or setup-java breaks every workflow at once, and in a grouped PR of five bumps you cannot tell which one broke CI, nor merge the safe four. This does not fragment versions - Dependabot bumps every use of one action in a single PR regardless, so bin/check-action-versions.sh stays green. No open-pull-requests-limit: that cap exists to stop a swarm of Java dependency PRs. Actions are a trickle, and a limit here would silently withhold an update rather than reduce work. astubbs/* ignored, with the reason inline: each is pinned to a SHA AHEAD of that fork's newest tag - unreleased work on a branch - so Dependabot would advance it to newer branch commits, a hand-managed decision. Recorded as a workaround to remove once #212 lands and they reference immutable tags. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
84ced9f to
708b75f
Compare
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
…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
Brings in #198 and #211, and picks up the two new required checks (`shell: sigpipe`, `workflows: action versions`) from .github/workflows/repo-hygiene.yml, which this branch predated - GitHub was blocking on statuses that could never arrive. One conflict, in src/docs/development/upstream-map.yaml, resolved in master's favour: the whole `UPSTREAM ISSUES` block was deleted there, including the issue-162-861-906-build-friction entry this branch had been updating. That is not a collision to split the difference on - it is a policy change that makes this branch's edit obsolete. #211 mirrored all 78 open upstream issues into this fork and shrank the manifest to track upstream PRs ONLY, because issues now live in the mirror. AGENTS.md now says so directly: "If the work maps to an upstream *issue*, the fork mirror is where status goes". This work maps to confluentinc#861, whose mirror is #180, so re-adding the entry would reintroduce exactly the duplication #211 removed. The upstream side of confluentinc#861 is still tracked on master, under the upstream-pr-901-licence-check entry. Follow-up commits handle what the merge cannot: #211 also made bare issue numbers below #1000 a CI failure on added lines, and this branch's added lines are full of them. AGENTS.md merged cleanly - both this branch's "Building and running the tests" section and #211's repo-hygiene CI entry survive. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
…flow entry #211 made a bare `#NN` below 1000 a CI failure on added lines, because the fork's numbers sit entirely inside confluentinc's range - a bare `#200` resolves here to a ManagedTruth issue while an author may well have meant confluentinc#200, shared-nothing architecture. This branch predated that rule and its added lines were full of bare numbers: running .github/scripts/issue-ref-gate.js over the diff flagged seven, across AGENTS.md, bin/build.sh, pom.xml and the parked note. All seven now name their repo, fork first, hyperlinked where the format allows; the gate reports zero. Two of them gained something in the rewrite. #132 / confluentinc#162 is titled "mvn compile fails if test-jar of parallel-consumer-core was not previously installed" - which is precisely the fresh-clone failure this PR spent its last round correcting, filed from the other end. The docs now say so, in AGENTS.md and in the parked note, because a reader hitting the vertx dependency error should find both the explanation and the issue that has been describing it since 2021. In the enforcer's POM comment the reference is spelled out with a URL rather than left as prose: that comment sits beside a <message> body whose indentation is load-bearing, and a bare number there is the least clickable place in the repo. The message body itself is untouched - re-verified that the rule still fires and still prints one [ERROR] per line, left-aligned as intended. Separately, AGENTS.md listed .github/workflows/shell-hygiene.yml, which does not exist: commit 1b040cb folded it into repo-hygiene.yml and left the old bullet behind, so the CI list documented two workflows for one file. Removed the stale bullet and folded into the surviving one the two details only it carried - that the SIGPIPE inversion needs >64 KiB after the match to bite, and that shellcheck does not catch it. Those explain why the check is a bespoke script instead of a linter rule, which is the part worth keeping. Found while checking this branch's text still read correctly next to #211's new entry. Verified: `./mvnw validate` BUILD SUCCESS; `-pl parallel-consumer-core` still fails the enforcer with the intended message; bin/check-shell-sigpipe.sh and bin/check-action-versions.sh both pass. Upstream-Issue: confluentinc#861 Forwarded: not-needed Applied-Upstream: no Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
Brings in the repo-hygiene workflow whose two new required checks (shell: sigpipe, workflows: action versions) this branch predated, so the PR was blocked waiting on contexts it could never produce. Conflict: src/docs/development/upstream-map.yaml. #211 slimmed the schema (dropped `forwarded`, `todo`, `backlink`, the long-tail DEFERRED block and scripts/upstream-backlink.sh) and mirrored every open confluentinc issue into this fork. Resolved by taking master's file whole and re-adding the issue-402-max-load-factor-log-noise entry in the new shape: `forwarded`/`todo` dropped (no tooling reads them now), the backlink action folded into notes, `fork_issue: 155` recorded now that confluentinc#402's mirror exists, and every issue reference qualified by repo per the new house rule. scripts/upstream-map.py validate passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
#211 made a bare `#NN` below #1000 a CI failure on added lines: the fork's numbers sit entirely inside confluentinc's range, so an unqualified reference is a coin flip. Three added lines in docs/inflight/pr-155-load-factor-noise.md tripped the gate; the javadoc in AbstractParallelEoSStreamProcessor and LoadFactorCeilingReportingTest passed only via the "upstream confluentinc#402" escape hatch, which the same commit deprecates in favour of naming the owner. Both now say #155 / confluentinc#402. Also moves the "answer the original reporter when this merges" note out of upstream-map.yaml and into the inflight doc. AGENTS.md is explicit that the manifest has no todo: field and loose ends belong in docs/inflight/ - the field was already dropped when the manifest was slimmed, so leaving the note there would have been an entry no tooling reads. Verified with .github/scripts/issue-ref-gate.js over this branch's diff against master: clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
… of the index Two things #211/#212 surfaced in this PR's own files. #212's lesson - await/assert the thing itself, never a proxy - has an adjacent shape here worth naming. fixedMessageBufferSizeDoesNotWarnOnEveryPass asserts NO warnings, which would pass vacuously if the appender were ever attached to the wrong logger. It is not vacuous today, because the debug assertion below it can only hold when the capture is live and pointed at the code under test - but nothing said so, and the two are separable by a well-meaning edit. Now they are commented as a pair, pointing at the write-up. The test needed no other change: it is fully synchronous, driving 500 checkPipelinePressure() passes on the test thread and reading the appender after, so there is no await to get wrong. Also rewords the upstream-map note that said "this manifest has no todo: field". bin/todo-index.sh read the literal marker and wanted to index it, which is the gate working - the fix is to stop writing a marker in prose, not to regenerate the index around a false entry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
…lease on" (#210) #113 moved release notes from per-PR entries to generation from the commit log at release time. The policy is right; its stated timing was not, and the wording misled a reader into concluding v6 would ship with whatever text already sat under `== 0.6.0.0`. "From the next release on" put 0.6.0.0 on the hand-written side of the line and started generation after it. Generation covers the release being cut - 0.6.0.0 included. The phrasing was written when the freeze and the release looked simultaneous; ten PRs landed in between and they came apart. Restructured around the only question that stays true across releases: HAS THIS RELEASE SHIPPED. 0.5.x is shipped legacy, frozen. 0.6.0.0 has not shipped, so it is regenerated at release time and frozen only once it ships. Later releases inherit the same test rather than needing the text edited again. Both misreadings are now named and denied outright, because a reader who reaches the right conclusion by luck will not reach it twice: 0.6.0.0 is not on the hand-written side, and its current contents are not what v6 will publish - not yours to add to, nor to trust. The bad wording had already been copied into a fourth file, docs/inflight/release-0600-blockers.md, within a day of being written. Corrected there too. States the correction exception, which previously had to be inferred: a PR never ADDS an entry, but may correct a factual error in text already there - as #198 did for the Kafka client version. Without that written down, the next agent either leaves a false statement standing or believes #198 broke the rule. Corrects the claim that the changelog-ref gate "should never fire, since PRs no longer touch the file". Four open PRs modify CHANGELOG.adoc, and the gate only rejects entries citing no issue - so #57's cited entries pass it while still violating the policy. It is neither a subset nor a superset of the rule. Deliberately NOT tightened to reject every addition: "adds" and "corrects" are both `+*` lines and are not mechanically distinguishable - the gate's own header records that fuzzy-matching removed against added bullets was built and then abandoned - and a blanket rule's only escape hatch legitimises a violating addition exactly as easily as a legitimate correction. Adds case 13 to the review gate's self-test: a match followed by >64 KiB of comments must still pass. The SIGPIPE fix itself landed in #211; this case is behavioural where that guard is static, so it catches a regression arriving by some route other than a literal `| grep -q`. References qualified as astubbs#NN per the gate added in #114, which merged after this branch was written. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Description
Started as two lines granting the reviewer one script. Each step exposed the next, so it ended up as a small CI-hygiene pass: fix a class of bug, add the guard that catches it, and turn back on the automation that should have been watching.
1. The reviewer could not run the thing it was reviewing
bin/test-check-review-posted.shwas missing from--allowedToolswhile its siblingbin/test-check-copyright-headers.shwas granted. So when #210 changed the review gate, the reviewer verified the fix by reading and said so - on the one PR where executing it mattered. The workflow already runs that script itself.Granted, both spellings (they are prefix matches, not globs). Not widened to
bin/*: the list is enumerated as an injection-safety margin, and a directory glob gives that away as effectively asBash(*).2. Four scripts inverted their own answer
writer | grep -qunderset -o pipefailreports failure exactly when it matches: grep exits on the first hit, the writer takesEPIPE,pipefailpromotes 141. It needs >64 KiB still to write, so it passes every small fixture and surfaces on real data.check-review-posted.sh:94check-quarantine-owners.sh:98,:110quarantine-lane-report.sh:201The quarantine one is the find worth reading twice. It pipes a whole source file into
grep -qinside anif. Pipe buffer 65,536 bytes;AbstractParallelEoSStreamProcessor.javais 65,185. Two open PRs (#203, #205) are adding lines to that file. When it crosses, the failure reads "the quarantine is not yet on its base" - a missing annotation, nowhere near a pipe.All four become herestrings.
3. Guards, because docs do not stop recurrence
bin/check-shell-sigpipe.sh- fails anybin/*.shpiping intogrep -qunderpipefail. Matches every flag spelling (-q,-qE,-Eq,grep -v -q,--quiet), skips exactly two files - itself and its self-test - because both carry the anti-pattern as data.shellcheckdoes not detect this. Verified against the known-bad line: clean. Hence a bespoke grep rather than adopting a linter.bin/check-action-versions.sh- fails if any action is used at two versions. It found drift immediately:claude.ymlandclaude-code-review.ymlwere still oncheckout@v4, which runs on deprecated Node 20. All 24 uses now@v6.Both have self-tests (11 cases for the SIGPIPE guard), and every regression case was verified red against the pre-fix code before being kept.
4. Dependabot had been off since 2022
.github/dependabot.ymlhad thegithub-actionsecosystem commented out bya470ab0bwith "Repo doesn't use github actions anymore" - true then, and false for years. Nothing had bumped an action since, which is why the drift went unnoticed.Re-enabled weekly, and deliberately not grouped, unlike the maven ecosystem. Grouping buys less noise; actions bump a few times a year, so there is little to buy off. It costs isolation, and an action has repo-wide reach - a bad
checkoutbreaks every workflow, and in a grouped PR of five you cannot tell which, nor merge the safe four. Dependabot still bumps every use of one action together, so versions stay consistent either way.No PR limit: that cap stops a swarm of Java dep PRs; actions are a trickle, and a limit would silently withhold an update.
astubbs/*ignored, with the reason inline - each is pinned to a SHA ahead of that fork's newest tag, so Dependabot would advance unreleased branch work. A workaround to remove once #212 lands.5. Where these live
repo-hygiene.yml(renamed fromshell-hygiene.ymlwhen the first non-shell check arrived within the hour), one job per concern:shell: sigpipe- now a required context in the master rulesetworkflows: action versions- new, advisoryThe job name
shell: sigpipeis untouched by the rename: the ruleset keys on the job, not the workflow.bin/AGENTS.md(new) records what earns a reviewer grant, what must never get one, and both guards' rules - because the honest answer to "which convention failed here?" was that none existed.Known-red check
claude-reviewfails here and cannot be fixed from inside this PR. Editingclaude-code-review.ymlmakes the action skip its own review, so none posts, and the gate correctly reports that. Needs an admin merge or a bypass.Follow-ups filed
next-patch-release)claude.yml) has no--allowedToolsat all, so@claude review this- the workaround the gate's own error recommends - produces a reviewer that structurally cannot execute anything. All four review passes here were static for that reason. Worth its own change.Checklist
check-copyright-headers.shclean (232 files, 0 violations)bin/AGENTS.mdnew,AGENTS.mdCI list, inline rationale in both workflows anddependabot.ymlAGENTS.md→ Changelog); CI tooling earns no entry under the entry testsrc/