Repository navigation
ci(bin): make Node the default for new scripts, and make a rule a row rather than a script - #403
Merged
Merged
Conversation
… rather than a script THE ARGUMENT IS SILENT WRONG ANSWERS, NOT TASTE Shell in bin/ has produced answers that were confidently wrong. A gate written with gawk's ENDFILE parsed cleanly under mawk - the default awk on this platform - matched nothing, ran no check, and printed its success line over a file containing the exact defect it had been written to catch. `exp` turned out to be a reserved awk function name, which is a syntax error that only surfaces when the line is reached. Neither was caught by review. The structural evidence is stronger than either anecdote: TWO ENTIRE GATES EXIST ONLY TO POLICE SHELL'S TRAPS - check-shell-sigpipe.sh, for `grep -q` under pipefail inverting its own answer, and check-shell-hazards.sh - plus a shared helper written because `grep -c` prints 0 and exits 1. When a fifth of the tooling is tooling that guards the tooling, the language is the problem. So: new scripts in bin/ are Node. Existing scripts are GRANDFATHERED and this is not a migration backlog - churn is its own risk, and check-source-patterns.mjs only ever looks at what is new against the merge base. Shell keeps an escape hatch, and it is a sentence rather than a flag: `shell-justified: <reason>` in a comment, because a written reason is one somebody can disagree with later. "It is what the neighbouring scripts are" is explicitly not a reason - that is how a default outlives the thing that justified it. NODE RATHER THAN PYTHON, DECIDED BY THE REPO .github/scripts/ already holds eight JS gate implementations, each with a .test.js sibling, against exactly one Python file in the tree. Node is the established second language here AND it already carries the testing convention bin/ lacks. Python would be the outlier. A RULE IS A ROW, NOT A SCRIPT Most gates here are the same program - walk files, match a regex, complain - each re-implementing file walking, exclusions, an opt-out marker, an exit-code contract and a failure message, in a language where every one of those is a paragraph, and each a place to differ subtly from its neighbour. bin/lib/source-patterns.mjs is the table and check-source-patterns.mjs is the single runner they share. Three rules seed it: the new-shell-script rule, and two that encode the traps above so they cannot recur. A check that has to THINK - parse XML, call an API, compare numbers - is a real program and still gets its own file. Every row must carry a `why`, asserted by the self-test. A rule whose reason is not written down survives long after the thing it guarded stopped mattering, which is how a linter becomes noise. WHAT THE WORK ITSELF CAUGHT, WHICH IS THE ARGUMENT IN MINIATURE The self-test's must-NOT-match half caught a real false positive: the reserved-word rule flagged `if (exp == 3)`, because `==` starts with `=`. A pattern verified only against text that should match is a pattern nobody has shown to be selective, and this repo has already shipped one gate that matched nothing and reported success over the defect it was written for. Two further defects were found and belong to #401, which carries the bin/check-all.sh change and merges first. Recorded here because they are the same argument: that script ran every gate through `bash`, so the first .mjs gate reported as a FAILING GATE while being clean - a rule shown as broken - and both its discovery loops globbed *.sh only. The gate loop was the obvious half; the SELF-TEST loop was the worse one, because an undiscovered self-test costs nothing visible: the sweep still reports every test passing, having found fewer than exist. That is also why this commit adds no per-self-test step to a workflow. check-all.sh's header forbids reintroducing that list, and once its glob takes either suffix, repo-hygiene.yml's --with-tests picks up a Node self-test with no edit anywhere. DEPENDS ON #401 Both halves of the bin/check-all.sh change live there - the glob that finds a .mjs gate and the dispatch that runs it with node rather than bash - so on this branch the sweep does not fail on check-source-patterns.mjs. It does something quieter and worse: it never finds it. `bin/check-all.sh` reports 15 ran, 15 passed, having swept one gate fewer than the directory contains and said nothing, which is the precise failure that script was written to prevent. So the gate is verified directly here - `node bin/check-source-patterns.mjs` and `node bin/test-check-source-patterns.mjs`, both clean - and its integration with the sweep is exercised by #401 rather than by this PR. Re-adding the glob and dispatch here to make the local sweep green would duplicate the change across two branches, which is the duplication splitting them was meant to avoid. THE REVIEWER GRANT DOES NOT COVER A NEW LANGUAGE bin/AGENTS.md records that check-*.sh is granted to the review agent by pattern. A Node gate is invoked as `node bin/check-x.mjs` and matches none of those patterns, so the reviewer would silently have been unable to run it - running fewer checks than the directory contains, with nothing to say so. `Bash(node bin/check-*.mjs:*)` and `Bash(node bin/test-*.mjs:*)` are granted alongside the shell ones. CI COMPILE-CHECKS EVERY NODE SCRIPT, AND THAT IS A COMPILE RATHER THAN ANALYSIS pr-checklist.yml runs `node --check` over every tracked .mjs and .js, and fails if the glob matches nothing - a loop that found no files exits 0 and is indistinguishable from a clean pass, which is the failure this repo keeps meeting. It is not static analysis and is not described as such. JavaScript is the one language CodeQL's default setup here does not cover (actions, java-kotlin, python), and enabling it is a repository setting rather than a file, so it cannot be done in a commit.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
✅ 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 |
🧪🔒 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 |
|
This branch adds a .mjs gate, so bin/check-all.sh has to be able to see it and run it. Two separate holes, and the first is the dangerous one: DISCOVERY. The sweep globbed bin/check-*.sh. A Node gate does not fail under that - it is INVISIBLE. check-all prints "15 ran, 15 passed" having swept one fewer gate than the directory contains, and says nothing at all. Quieter than a failure and strictly worse, and precisely the shape that script exists to prevent. EXECUTION. Every gate was run through `bash -n` then `bash`, which gives a .mjs file a bash syntax error and reports a clean rule as a broken gate. Dispatch now picks node or bash by extension. WHY IT LIVES HERE RATHER THAN ON #401 It was briefly on that PR, on the reasoning that whichever branch merges first must carry it. That is true and it is not a good enough reason: it made the PR that ESTABLISHES Node in bin/ depend on a PR that already writes Node, which is backwards to read and backwards to review. The convention lands first; work written under it follows. #401 now depends on this.
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…ch, rather than carrying it This PR's gate is bin/check-throughput-regression.mjs, and bin/check-all.sh cannot see or run a .mjs gate without the glob and the extension dispatch. Those ~10 lines were briefly carried here, on the reasoning that whichever branch merges first must have them. That reasoning was right and the conclusion was wrong. It made #403 - the PR that ESTABLISHES Node as the default in bin/ - depend on this one, which already writes Node. Backwards to read and backwards to review: the convention should land first and work written under it should follow. So the dispatch goes back to #403 and this PR declares the dependency. The merge order is #403, then this, then the confluentinc#857 branch that depends on both. Until #403 lands, bin/check-all.sh on this branch does not sweep check-throughput-regression.mjs at all - it is not run and reported broken, it is simply not discovered. Verify it directly meanwhile: node bin/check-throughput-regression.mjs node bin/test-check-throughput-regression.mjs
5 tasks done
…two rules that were reinventing wheels MIGRATING A REAL RULE IS WHAT TESTS THE TABLE The table shipped with three rules invented for it, which proves nothing: an abstraction validated only against its author's examples is an abstraction nobody has shown fits anything. check-shell-sigpipe.sh was the right first migration because it asked to be moved - its own header says it is "a hazard category, not a gate of its own", and docs/inflight/ci-fold-sigpipe-into-shell-hazards.md tracked the fold. That note is now deleted rather than left as a request nobody will action. IT IMMEDIATELY DEMANDED A FIELD THE TABLE DID NOT HAVE, which is the point The rule only applies to files that set pipefail - piping into `grep -q` is only a wrong ANSWER when pipefail promotes the reader's SIGPIPE to the pipeline status. So rules gained `requires`. A table that could not express that would have been a table that only fits rules its author made up. AND THE FIRST PORT WAS WRONG IN A WAY THE OLD GATE WAS NOT A whole-file regex flagged thirteen files the shell gate passes, because most only MENTION the hazard in a comment - including the gate being replaced and its own self-test. The shell version got this by running line-oriented and piping through a second `grep -v` for comments; a single pattern has to carry both halves. Now line-anchored with a comment guard. Every flag spelling the deleted gate covered is pinned in the self-test - -q, -qE, -qF, -Eq, --quiet, --silent, `grep -v -q`, `grep -E -q` - because -qE, -qF and the space-separated form are exactly the ones a hand-written regex gets wrong, and reasoning about whether they match is how you convince yourself of the wrong answer. Verified against all twelve before deleting anything. TWO RULES DELETED BEFORE SHIPPING, FOR THE REASON THAT SHOULD GOVERN THE TABLE gawk's ENDFILE under mawk, and awk's reserved function names used as variables. Both real - each had bitten within a day - and both wrong to keep: each was generalised from a single incident into policing a language now frozen for new work. This repo already runs ShellCheck, SpotBugs with fb-contrib/findsecbugs/findbugs-slf4j, Infer, forbiddenapis, ArchUnit and CodeQL, and a rule one of those covers must never become a row here - a second implementation of somebody else's check is a wheel that will eventually disagree with theirs. The table header now says so. The surviving rules are the two that earn it: a repo policy no tool can know (Node for new scripts in bin/), and a hazard measured to be invisible to ShellCheck which once reported "no review posted" on four PRs whose reviews had posted. REFERENCES REPAIRED RATHER THAN LEFT DANGLING Seven of them, found by check-file-refs.sh. Live docs repoint at the rule; the dated solutions record carries a marker instead, because docs/citations.md forbids rewriting a dated record to match today's tree and the gate accepts a stated reason as the repair.
…idate, no queue Records the answer to 'which other check-*.sh are really just pattern matchers', so it is not re-derived and so the table is not assumed to want gates it does not fit. check-shell-hazards.sh is the one real candidate, and the interesting part is WHY: it is already a rule table written in shell - a HAZARDS heredoc with categories and a why per entry, a file-level opt-out, and a comment guard whose own comment says a comment about a hazard is not a use of it. It reached the same design independently, which is the best evidence available that the design is right. It is also why the fold is not trivial: matching its opt-out and comment semantics exactly is the work, and getting either subtly wrong is how a migration covers less than what it replaced - which already happened once, when the first sigpipe port flagged thirteen files the shell gate passes. The non-candidates are listed with the reason each fails, because 'why not' is the half that stops somebody trying: aggregation across files, cross-file consistency, required-vocabulary rather than forbidden-pattern, a ShellCheck wrapper, and everything that fetches from GitHub. And the rule that governs additions at all: check what we already run first. A rule ShellCheck, SpotBugs, Infer, forbiddenapis, ArchUnit or CodeQL covers must never become a row, because a second implementation of somebody else's check eventually disagrees with it.
…thing THE SECOND INSTANCE OF THE SAME HOLE, IN THE SAME PR The gates loop globbed check-*.sh and was widened to include .mjs. The SELF-TESTS loop globbed bin/test-*.sh and was not, so bin/test-check-source-patterns.mjs was executed by nothing: the named step had been dropped from pr-checklist.yml on the belief that check-all.sh --with-tests covered it, and two documents in this very PR asserted that it did. The compile step made it PARSE in CI, which reads like coverage and is not. So the must-NOT-match controls - the half that caught a real false positive in this PR, `exp == 3` matching `exp\s*=` - never ran once. MISSING A SELF-TEST IS WORSE THAN MISSING A GATE, and the comment in check-all.sh now says so. An unswept gate is absent from the count, and a count that moves is something somebody eventually notices. An unswept self-test leaves its gate present, running, and LOOKING tested. WHAT ELSE REVIEW FOUND `checker` and `runner` were the only variables in run_gate not declared local, so they leaked to global scope while every neighbour was contained. `forbid: /^/` for the new-shell-script rule was an abuse of the field - "the file existing is the violation" dressed up as a pattern. `forbid` is now optional, with the typedef saying to omit it when existence is the violation, so the next person adding a rule is not taught the wrong idiom by example. The runner now prints the merge base it resolved. `scope: 'added-files'` is only as good as origin/master being current, and a stale ref moves the base backwards and flags files master added - which the root AGENTS.md warns about in another context. Printing the base makes that visible rather than baffling. Files were read once per rule, and both rules match every shell script in bin/. Now cached. TWO CLAIMS THE MIGRATION FALSIFIED, corrected rather than left: bin/AGENTS.md still said "two entire gates exist only to police shell's traps" when one of them is now a row in the table, and still described the deleted gate's self-exclusion of two files - a .mjs rule cannot match /\.(sh|bash)$/, so it excludes nothing. WHAT REVIEW DID NOT FIND, which is worth as much No disagreement between this rule and the gate it replaced, over 1,470 generated cases - 7 prefixes x 6 pipe spellings x 7 grep forms x 3 suffixes, plus comment variants and 8 multi-line cases aimed at the comment guard. The migration is behaviour-preserving on everything anybody has thought to try. One correction to a comment rather than to code: the regex is described as line-anchored, and `[^|]` also matches a newline, so a match can straddle lines. It cannot bypass the comment guard - reaching a comment line's pipe would need `\|` immediately after the newline, and a comment starts with `#` - but the comment overstated the mechanism and now says what actually holds.
Owner
Author
|
@codex review Focus areas, in rough priority order:
Context worth having: this PR makes Node the default for new scripts in |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65977478cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Merge prep for #403 asked the checklist's 'other instances of the same defect' question - two loops had already needed widening to see .mjs, so a third that globs bin/test-*.sh looks like the same miss. It is not. That job exists to catch BSD userland differences - bash 3.2, BSD sed and awk - and a Node self-test has no such exposure. The .mjs self-tests are already swept on Linux by the bin/check-all.sh --with-tests step in the same workflow, whose globs cover both suffixes. Recorded because the NEXT person to ask that question will reach the same wrong conclusion I did, and 'fixing' the glob would look like closing a gap while actually just running the same test twice.
…nd eight claims the rename inverted THE ONE THAT MATTERED (P1): THE GATE STOPPED RUNNING LOCALLY .githooks/pre-commit carries a GATES array naming each gate by filename, and its missing-file branch is deliberately silent - "a branch older than a gate simply does not have it. Not an error - say nothing." Deleting check-shell-sigpipe.sh therefore removed it from every local commit without a word, and check-source-patterns.mjs was never registered in its place. So both the sigpipe rule AND the new-shell policy were dark on every commit until CI ran. That is the THIRD instance of this PR's own defect class - a coverage hole that reports nothing. The gates loop had it, the self-tests loop had it, and the pre-commit registry had it. The array now names the replacement, and a comment above the loop says that a renamed or deleted gate must be changed here in the same commit, because the silent branch is by design and will not tell the next person either. Verified the way the hook invokes it: git mode 100755, `#!/usr/bin/env node`, runs clean as `./bin/check-source-patterns.mjs`. TWO RULE FIXES, BOTH REAL HOLES `.bash` was not covered by the new-shell rule while the sigpipe rule in the same table already treated .bash as shell, so `bin/tool.bash` bypassed the Node-default policy silently. `shell-justified:` matched anywhere in the file, so `echo 'shell-justified: x'`, usage text or heredoc data exempted a script carrying no justification at all. Now anchored to a comment. Reason QUALITY stays a review judgment - no regex checks that - but the documented form is mechanically checkable, so it is checked. Both fixes carry self-test cases, including the must-NOT-match halves. SIX DOCUMENTS THE BULK RENAME MADE FALSE A global .sh -> .mjs substitution does not preserve meaning, and here it inverted several claims: - The copyright-gate note said the new module shares shell-corpus.sh's one-level corpus, so this PR "doubled the gap's reach". It uses `git ls-files` and matches every tracked .sh/.bash path - the PR CLOSED that gap for this rule. The note said the opposite of what happened. - docs/ci.md said check-shell-hazards.sh should eventually absorb the source-pattern rule. The migration runs the other way; following that sentence would undo this PR's architecture. - check-shell-hazards.sh's own header still said the sigpipe gate "BELONGS IN HERE, and has not moved yet". It has moved, outward. That file is now the candidate to fold INTO the table, not the destination. - static-shell-lint-severity-tiers.md ended up claiming ShellCheck aborted while processing a JavaScript module. ShellCheck never reads a .mjs. Restored to the deleted .sh and marked as the dated record docs/citations.md forbids rewriting. - ci-conflict-marker-gate.md cited a self-exclusion-by-name mechanism as prior art. No such mechanism exists: the rule avoids its own fixtures only because its files regex excludes .mjs. - bin/AGENTS.md said ".github/scripts/ holds eight JS gate implementations, each with a .test.js sibling" - implying sixteen files. It is four implementations and four tests. And it credited pr-checklist.yml with running the self-tests; that workflow only compile-checks, while repo-hygiene.yml runs them via check-all.sh --with-tests. TEN LIVE REFERENCES, NOT THE THREE REPORTED The review named three files still pointing readers at the deleted executable in the present tense. Sweeping the class found ten across check-shell-hazards.sh, chaos-test.sh, ci-mutation-test.sh, check-branch-self-reference.sh, quarantine-common.sh, chaos-experiment-common.sh, test-check-docs-data.sh, test-check-review-posted.sh and test-check-cve-exclusions.sh. Genuinely historical mentions are left alone. shell-corpus.sh also now states it has one consumer left, since its own rationale implied two.
…s findings
Ten findings, two P1. The pattern worth naming: FOUR of the ten were caused by one blanket
`sed 's/check-shell-sigpipe.sh/<new name>/'` across the docs when the gate was folded into the rule
table. A filename carries properties, and substituting one for another silently transfers claims that
were true of the first and false of the second.
* an open note said sigpipe shares shell-corpus.sh's one-level corpus - the rule does not use
shell-corpus at all and scans every tracked .sh/.bash, so the substitution claimed this PR DOUBLED
a scope gap it had actually closed (P1);
* another cited a self-exclusion-by-name mechanism the rule does not have - it avoids its own
fixtures only because its `files` regex excludes .mjs;
* a dated ShellCheck measurement became a claim that ShellCheck aborted while analysing a JavaScript
module;
* docs/ci.md ended up pointing the migration backwards, telling a reader to fold the rule INTO the
shell gate - undoing the architecture.
Fixing them by hand, one sentence at a time, then repeated the mistake in a smaller way: substituting a
DESCRIPTION for the name is safe in a present-tense sentence and still wrong in a historical one, so
"the sigpipe check (bin/lib/source-patterns.mjs) opened a PROSE comment with the word shellcheck"
briefly asserted a .mjs module had a shell directive problem. The distinction is not name-versus-
description, it is PRESENT-TENSE versus HISTORICAL, and a historical sentence must keep naming the
thing that was there at the time.
THE OTHER P1: THE PRE-COMMIT REGISTRY
.githooks/pre-commit's GATES array still named the deleted script, and its missing-file branch is
silent by design - so every local commit lost BOTH the sigpipe rule and the new-shell policy until CI,
with nobody told. Registered the replacement and wrote the failure mode into the array's own comment,
because the next rename will do this again otherwise.
CORRECTIONS OF FACT
`.github/scripts/` holds FOUR JS gate implementations with four .test.js siblings - eight files, not
eight implementations. The wording implied sixteen and overstated the quantitative case for Node,
which was quoted in this PR's description and in bin/AGENTS.md.
Compile-checking and self-testing are two different workflows: pr-checklist.yml compiles, and
repo-hygiene.yml runs bin/check-all.sh --with-tests, which is what discovers bin/test-*.mjs. Naming
only the first sends anyone repairing the wiring to a job that never runs the tests.
RULE COVERAGE
`.bash` now counts as shell for the new-script rule - the sigpipe rule already treated it as shell, so
bin/tool.bash bypassed the Node-default policy silently. And `shell-justified:` must now appear as a
COMMENT: it matched anywhere in the file, so echoed output, usage text or heredoc data exempted a
script carrying no justification at all. Reason quality stays a review judgement, because no regex
checks that; the documented form is mechanically checkable, so it is checked.
Owner
Author
|
@claude review this |
|
Claude finished @astubbs's task in 5m 11s —— View job Review:
|
THE CLAUDE REVIEW FOUND WHAT CODEX DID NOT, AND IT IS THE QUESTION CODEX WAS ASKED
`scope: 'added-files'` resolved a merge base with `git merge-base HEAD origin/master` and no guard. On
a SHALLOW CLONE that call does not error - it returns a commit that is not the merge base, exit 0 - so
the added-files list feeding the new-shell-script rule is silently wrong. Reproduced on a real shallow
checkout during review, not theorised; and reproduced again here before and after the fix.
That is the silent-wrong-answer class this whole PR argues against, occurring in the mechanism built to
prevent it. This repo already answers it elsewhere - check-copyright-headers.sh proves its fork point
with `git cat-file -e <ref>^{commit}` before trusting it - and this was the one new script that skipped
the convention.
Now it checks `--is-shallow-repository`, proves `origin/master` is actually present (a single-branch or
narrow fetch leaves no local ref, and merge-base's answer then means nothing), and FAILS CLOSED with
exit 2 when either check fails. A gate that cannot establish its baseline must say so; reporting "no
violations" because it could not look is the failure this file exists to prevent.
AND THE BUG THAT FIX WOULD HAVE CAUSED
.githooks/pre-commit registered this gate as "check-source-patterns.mjs:" - no soft exit codes - while
the script documents exit 2 as "cannot run". So the very case above would have HARD-BLOCKED every
commit on a shallow or narrowly-fetched clone, which is the anti-pattern that hook's own header warns
about: blocking a commit because the environment is incomplete teaches people to bypass the hook. Now
":2", with the comment above the array corrected to match the script's actual contract.
TWO KNOWN LIMITS OF THE SIGPIPE REGEX, STATED RATHER THAN LEFT IMPLIED
A pipeline split over a line continuation is not caught, and a heredoc body line containing `| grep -q`
as literal data is a false positive. Both are inherited from the line-oriented gate this replaced -
checked against it rather than assumed - so they are parity, not regressions. Recorded in the rule's
own `why` so nobody reads it as tighter than it is.
Owner
Author
|
Both findings fixed in Shallow / narrow-fetch merge base — fixed, and verified both ways. The script now checks Worth naming why it stung: the PR argues that shell produces silent wrong answers, and this was a silent wrong answer in the gate written to prevent them. Pre-commit soft code — fixed. Line continuations and heredocs — checked, and it is parity. I verified against the deleted gate rather than assuming: it was |
The new-shell-script rule read `git diff <merge-base> HEAD`, which cannot see a file staged for its first commit. The pre-commit hook is the one place this rule is meant to stop a new shell script BEFORE it lands, and there it saw nothing - so it allowed exactly the addition it exists to reject, leaving CI to catch it after the fact. Local enforcement of the PR's headline rule was silently ineffective. Found by the codex review on #401, filed P2. It is a functional hole in the rule rather than a polish item: the gate ran, reported clean, and was wrong. Now the union of committed and staged additions, deduplicated - the hook sees the staged file, CI sees the committed one. Verified by staging a new bin/*.sh and watching the rule reject it, then unstaging and watching it go clean again.
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…rong answer, not a polish item I had stopped at the P1s because the batch was large, which is not a reason. Working through them, none of the three is cosmetic: each makes the tooling report something confidently wrong. STAGED FILES WERE INVISIBLE TO THE NEW-SHELL RULE (arrives via the #403 merge) `git diff <merge-base> HEAD` cannot see a file staged for its first commit, so at the pre-commit hook - the one place the rule is meant to stop a new shell script BEFORE it lands - it saw nothing and allowed exactly the addition it exists to reject. Now the union of committed and staged additions. Verified by staging a new bin/*.sh, watching the rule reject it, and watching it go clean when unstaged. A RUN THAT GAINED A TEST CASE LOOKED FASTER Summing every <testcase> means a larger denominator for a reason that is not performance: a parameterised control picking up one more @EnumSource value inflates the control total and makes the ratio look healthier, which can mask a real subject regression. The check now compares the CASE SET identity between this run and each reference, and refuses a run whose workload differs rather than averaging the difference in and calling the result a verdict. When that leaves nothing comparable it says so - naming the runs and the reason - instead of falling back to a number. A TIMED-OUT LargeVolumeInMemoryTests REPORTED NOTHING Control flow left before the reporter, so the failing case produced no rate at all - and because earlier parameterised cases have already emitted this class's label, bin/performance-test.sh's "NOT MEASURED" check could not notice the missing failing case either. Silent twice over. It now reports on the failing exit and rethrows unchanged, matching what the other performance classes already do.
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…-multi-consumers-bug Brings in #403 ahead of its own merge, because #401 is stacked on it and is wanted here. WHAT IT CHANGES FOR THIS BRANCH Node becomes the default for NEW scripts in bin/, enforced by check-source-patterns.mjs against the merge base. Three scripts on this branch are new relative to master and were therefore caught - torture-overnight.sh, its self-test, and soak-deadlock-probe.sh. They predate the rule; they are "new" only because this branch has not merged yet. Each now carries a shell-justified reason written per script rather than boilerplate, and the torture harness's names its own weakness: the verdict logic is the part the rule is actually aimed at, it has been wrong three times, and if it grows further it should move to .mjs rather than be defended by a comment. check-hot-log-args.sh AND ITS SELF-TEST ARE DELETED Operator ruling. One bespoke script for one pattern is exactly what bin/lib/source-patterns.mjs replaces - the incoming PR's own argument is that a rule should be a row rather than a script - and the rule was NOT carried over as a row. That leaves a real gap: nothing mechanical now catches the eager log-argument form being written at a new call site. It is recorded as a known gap in three places rather than quietly dropped, and the cheap fix if it ever bites is a row, not another script. HotPathLogArgumentsAreDeferredTest survives and is the stronger of the two guards anyway: it asserts the SLF4J behaviour the fix RESTS on, which a source check cannot see. Six references to the deleted gate were updated - two docs, a test javadoc and a main-code comment. The javadoc's claim that the shortfall was "the leading candidate" for the regression was corrected in the same pass: it has since been measured, so that wording was stale. NOTED, NOT FIXED: check-all.sh reported one violation where the gate itself reported three. Running bin/check-source-patterns.mjs directly showed all of them. A summary that under-reports is worth knowing about in the script whose purpose is that the set of gates cannot drift silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…mption-multi-consumers-bug Brings in #401, which is stacked on #403 (merged here in the previous commit). WHAT ARRIVES A throughput regression check that normalises the subject against the neighbour classes in the SAME run, so a slow runner and a slow tree stop looking alike - which is exactly the control that had to be applied by hand to diagnose the control-loop defect on this branch. It ships as .mjs, conforming to the Node-first rule that arrived in the previous merge, with its verdict logic in a shared lib and a self-test that pins the bounds. Also wires throughput reporting into the three performance classes that previously emitted no figure, so the lane measures more than the two tests it started with. WHY IT IS HERE RATHER THAN AWAITED FROM MASTER Asked for directly. The earlier recommendation on this branch was to wait for it to land on master and pick it up with an ordinary master merge, on the grounds that this PR's own lane would gain nothing its parent PR's lane does not already provide. That reasoning is recorded rather than deleted, and the operator's call overrides it. ONE PLAN INSIDE IT IS NOW UNRUNNABLE AS WRITTEN, AND SAYS SO perf-validate-the-regression-check-against-a-known-defect.md planned a red/black experiment whose step 2 required this branch to pick up the CHECK without the FIX. This branch has held the fix since 5ed8856, so its lane can only produce the black arm now. Rewritten to say that, and to state what a red arm would now cost: a throwaway branch reverting the single log line, which is still the only variable. Nothing was lost by accident - the threshold had already been settled from recovered CI history, which demoted that experiment from source-of-the-threshold to confirmation before the ordering could matter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…s-bug ZERO CONTENT CHANGE, AND THAT IS THE EXPECTED RESULT RATHER THAN A SUSPICIOUS ONE Master gained exactly one commit since this branch last merged it: the squash of #403. This branch had already merged that PR's own branch (ci/node-first-tooling) directly, so both sides carry identical content by different history - the squash on master, the original commits here. `git diff HEAD` after the merge is empty. No conflicts, nothing staged. The merge is a pure history join that tells git the two lines agree, which is what stops the next master merge from trying to re-apply the same change. WHY THIS WAS WORTH CHECKING RATHER THAN ASSUMING A squash-merged PR's commits are not on master, so a branch that merged the pre-squash commits and a master that merged the squash look like divergent work on the same files - here, 34 overlapping ones. That normally produces conflicts or, worse, a clean merge that silently reverts one side. Verified it did neither: the working tree is byte-identical to the pre-merge tree, both sides' additions are still present (check-source-patterns.mjs and its lib from #403, check-throughput-regression.mjs and its lib from #401), and the deliberately deleted check-hot-log-args.sh has not come back. PRE-MERGE CHECKS AGENTS.MD REQUIRES Not a shallow clone. The io.confluent -> bz.stub rename is already applied on this branch, checked with the prescribed `grep -rnE 'io[./]*conflu'` rather than the habitual literal - the only surviving matches are the historical prefix mappings inside bin/check-copyright-headers.sh, which are meant to be there. All gates pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…s-bug Picks up the squash of #401, which landed on master minutes after the previous merge took #403's. MASTER'S VERSION WAS NEWER THAN THE BRANCH THIS HAD ALREADY MERGED, AND MASTER WINS This branch merged ci/perf-throughput-baseline directly at 161d768. The squash on master carries later refinements that never reached that branch ref: a shared ThroughputReport helper extracted into test utils, the four performance classes slimmed to call it instead of reporting inline, and a substantially reworked check-throughput-regression.mjs. Six files conflicted, all of them the same add/add shape - the pre-squash commits here against the squash there. HOW EACH WAS RESOLVED, AND WHY NOT WITH A BLANKET --theirs Five files turned out to be pure old-astubbs/parallel-consumer#401 content with no edits of this branch's own - checked by diffing each against 161d768 rather than assumed - so master's newer version was taken whole and each was verified byte-identical to master afterwards. There was nothing of ours in them to lose, which is the only condition under which taking one side wholesale is safe. The sixth, perf-validate-the-regression-check-against-a-known-defect.md, DID carry an edit of ours: the rewrite recording that the note's red/black sequence can no longer run here, because this branch now holds the fix as well as the check. Master's copy of that file is byte-identical to the old base - it never touched it - so ours is that base plus the rewrite, a strict superset. Kept ours, and confirmed the rewrite survived. VERIFIED AGAINST BOTH PARENTS, NOT ONE A resolution that drops one side reads as clean from the other, so: no conflict markers anywhere; ThroughputReport.java, check-throughput-regression.mjs and all four performance classes identical to master; the diff against master contains only this branch's own work. Gates pass and the module compiles, which matters here because those test classes were rewritten to call the extracted helper. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
…mode-battle-test #262's declared parent had moved a long way: #257 has since merged master up to #403, so taking it also takes nearly all of master, and 262's own CONFLICTING state against master goes with it. 846 files, nine conflicts. Package rename: both sides were already on `bz.stub.*`, so the merge was ordinary and `bin/rename-packages.sh` was not needed. Verified by `git ls-tree -d` on all three refs before starting, not assumed. Conflicts, and which side won: - ProducerManager - TAKE 257. Master has already absorbed 262's produce-callback fix and improved it: the callback is hoisted to a `sendCallback` field with the reasoning in javadoc rather than a comment, and it carries the fact 262 established last (Kafka's own ProducerBatch catches and logs whatever a callback throws, so the throw was only ever observable on the synchronous doSend path). 262's local copy is deleted as the duplicate it now is. `InternalRuntimeException` is `PCInternalRuntimeException` on master; the three places 262 still named the old class are renamed. - AbstractParallelEoSStreamProcessor#cleanUpContext - BOTH. Code takes 262's catch-and-log, which master does not have: this runs in runUserFunction's `finally`, and a throw from a finally REPLACES the exception the catch above is propagating, destroying the user function's real failure. Javadoc takes 262's correction (an ExternalEngine can never reach here holding a produce lock - its constructor rejects PERIODIC_TRANSACTIONAL_PRODUCER, so 257's claim that this is that path's only release is wrong) plus 257's new paragraph on the lock being taken rather than read. - KafkaClientUtils#createNewProducer - BOTH, and neither could just win. 262 added a typed (mode, transactionTimeout, stableTransactionalId) overload; master added (mode, Properties overrides). The auto-merged body already used all three parameters, so both overloads now delegate to one private four-arg builder. - TransactionalPartialResultSetIT - add/add, TAKE 257 plus one thing. 257's is the reviewed descendant of the same test: it pins MAX_REQUEST_SIZE_CONFIG rather than inheriting it, holds POISON_KEY to NONE rather than all-or-none (its oversized result can never be sent, so isAnyOf(0, n) would also accept a full set that cannot physically exist), and uses commons-lang3 `repeat` instead of a local Java-8 helper. Only 262's `@Timeout(600)` guard is carried over. - ProducerManagerTest - TAKE 262's extracted `acquireProduceLockInto` / `assertProduceLockStillOwnedByContext` helpers over 257's inline copies of the same code and comments; the fact 257's comment had and the helper javadoc did not (#257 made cleanUpContext the ONLY release point) is folded into the javadoc. - ProduceLockReleaseTest, WorkContainer - imports only. - docs/quarantined-tests.md is now EMPTY, by two independent routes meeting. #351 diagnosed and fixed OffsetEncodingBackPressureTest on master (it asserted an offset back pressure exists to stop advancing), and this branch performs its own rule-3 re-enable of ProducerManagerTest.producedRecordsCantBeInTransactionWithoutItsOffsetDirect. No @Quarantined annotation remains anywhere in the tree and the registry check confirms 0 entries. - docs/inflight/test-untracked-ci-flakes.md - master's rows (both entries 262 knew about are fixed and gone; simpleBatchTest and processInKeyOrder are new) plus 262's now-current account of the BlockedThreadAsserter collision. Fixed a blank line that was splitting the table in two. Worth flagging for the handoff: master's new `processInKeyOrder` section is about one of the three unexplained failures this branch pushed with at 48d210f, and says it is a solved flake still firing rather than a fresh one. - docs/inflight/pr-blockers-and-collisions.md - master's collision list, plus 262's transactional-stack section updated for reality: #261 merged on 2026-08-14, so the chain is now two PRs, not three. 261 is named rather than deleted because a reader who knows the stack as three links needs telling which one is already master. Verified locally: full-reactor `test-compile` green (main and both test source roots), check-quarantine-registry 0 entries, check-issue-refs clean. The unit suite has not run yet.
astubbs
added a commit
that referenced
this pull request
Sep 1, 2026
… only, no content Records that this branch's work has already landed on master by other routes, so that it can be deleted and so a later merge attempt is a no-op rather than the 74 conflicts a content merge raises. `-s ours` deliberately: every one of its twelve commits was audited against origin/master and nothing survives the audit. WHY IT LOOKED LIKE STRANDED WORK. It was branched 2026-08-19 from a46b7df, a mid-stack commit on fix/909-load-reproduction - which became #322 and merged 2026-08-26 as cf2741c. A child cut from a PR that merged a week later and was never rebased, so it carries its parent's confluentinc#909 ancestry as a fossil while the real version went in through #322. That is the whole explanation for both the 114-commit lag and the conflict count. WHERE EACH COMMIT ACTUALLY LIVES NOW: 2e22e83 ShardManager through the module identical patch-id on master 30c09a3 broker-level reproduction via #322 - RegistrationRaceStaleResidentIT and PausableInsertShardManager are on master, which is why they conflicted add/add 6982b9f the third precondition relocated to docs/solutions/logic-errors/ 909-needs-a-saturated-pipeline-...-2026-08-19.md 92c9a73 refuse a merge with work in flight .claude/hooks/check-merge-outstanding-work.sh b1b7a47 hooks simplify + self-test fix both halves on master: the `*merge*` pre-filter, and the `fails`->`failures` fold whose absence made the suite unable to fail e8db3c5 ShardKey javadoc contradiction master's KeyOrderedKey carries the correction 9013c47 drop the ShardKey refactoring row followed it a46b7df agent self-review as PR comments renamed next- -> ci-agent-self-review-... f7f558a index open work by cost superseded - inject-recorded-knowledge.sh does this from bin/lib/inflight-tags.sh 2db9a51 give every note a priority superseded by the impact axis ddf6465 classify by consequence superseded - see below THE ONE GENUINELY UNIQUE THING, AND THE DECISION IT SETTLES. The branch proposed a single consequence axis, `<!-- inflight-class: X -->`. Master went the other way and shipped three - inflight-type, inflight-impact, inflight-labels, plus inflight-state for disposition - with bin/lib/inflight-tags.sh as their single source, a gate in bin/check-inflight-tags.sh, and the session index consuming the same lib. The successor even carries the branch's own sentence forward verbatim, "Classify by CONSEQUENCE, not by what kind of file it is", and then splits it. Three axes win: a single axis cannot say that a feature addressing a crash belongs beside the crashes, which is the case bin/lib/inflight-tags.sh's header calls out by name. Sixty-five of the seventy-four conflicts were nothing but those two schemas meeting. Resolving them would have meant re-deciding the taxonomy sixty-five times. NOTHING IS LOST BY TAKING NO CONTENT. Thirty-six files exist on the branch and not on master; all are stale names from master's later prefix rework (next- and parked- into core-, ci-, release-, upstream-), two are the sigpipe scripts #403 deliberately folded into a rule row, one is a workflow 025d0b7 deleted on purpose, and three are Java classes present on master at other paths. The single note that is real open work and genuinely absent from master - bug-retry-queue-orphaned-by-inline-stale-removal.md, a distinct defect from master's bug-stale-sweep-iterator-evicts-fresh-replacement.md - is carried on fifty-six other refs, so it lands with whichever of those merges and cannot be stranded by this. Merged here rather than to master because #400 is where the harness-survey context lives, and the taxonomy question is the same question that PR is answering. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UTX8obQMsjs9kq2rkpU5cZ
astubbs
added a commit
that referenced
this pull request
Sep 2, 2026
Ten commits, no Java overlap - master's core work (#257's produce-lock handover, #393's thread-confined consumer) is nowhere near the offsets package this branch changes. All three conflicts were in docs, and all three were two ledgers being appended to from both sides on the same day. WHAT THE RESOLUTION KEPT Both flake ledgers gained entries from master AND from this branch on 2026-09-01, so every conflict resolution keeps both sides rather than choosing one: - test-load-tightness-flakes.md: master's inFlightMessagesCommittedIfProcessed- DuringShutdown[1] paragraph and this branch's committedOffsetRemoved[1] latest recurrence are about different tests and neither supersedes the other. - test-untracked-ci-flakes.md: both sides added a processInKeyOrder row for sightings on the same day from different branches. Merged into one entry rather than two, because two rows under one test name is exactly the conflation master's own section warns against. This branch's four sightings are folded in as a sub-section of master's [3] bullet, where they turn "seen once, not reproduced" into a control-armed contention finding - and master's isolated-run result stops reading as a contradiction, since isolation removing the failure is what the contention reading predicts. - refactoring.md auto-merged: master deleted the produce-lock double-release OPEN QUESTION entry because #257 answered it. TWO REPAIRS MADE WHILE IN THERE Master's table had a blank line between two rows, which markdown renders as two tables with the second missing its header. Closed up. The JStream entry said "delete this entry when #116 lands" - a marker docs/inflight/AGENTS.md explicitly forbids on master, because the merge is the moment nobody is looking here. Replaced with the migrate-rather-than-delete outcome that doc names, and the attestation block it opened is now closed rather than running into the next section's. INHERITED AND READ #403 makes Node the default for new bin/ scripts and deletes check-shell-sigpipe.sh; #382 rewrites the hook guards to derive identity from the command rather than the session; a new core AGENTS.md binds @GuardedBy to any field change in the engine. This branch adds no scripts and changes no shared field, so none of the three change what it does - recorded because a green build is not evidence the ground under a design held still. AND ONE FIX THE MERGE COULD NOT BE COMMITTED WITHOUT #403's new bin/check-source-patterns.mjs refused this merge commit, reporting seven of #381's experiment runners as new shell scripts and advising --no-verify. They are master's, already grandfathered there, and this branch never touched them. The cause is that its `added-files` scope is the union of committed and staged additions against the merge base - and while a merge is staged but not yet committed, HEAD is still the pre-merge tip, so every file master added since the branch was cut is an addition against that base. It corrects itself the moment the merge commit exists, which is why nothing on master has seen it: it fires only in the window where the fix has to be applied. Membership of origin/master settles it without needing to know about merges at all - a path already there is not new to this repository, whoever staged it - so the subtraction lives in bin/lib/added-files.mjs as a pure function of three lists. That placement is what lets it be tested by control pair like every rule in the table, instead of standing up a fixture repository, which is the reason the runner had no test to break. The four must-NOT-match cases are red-proven against the pre-fix implementation. Carried here rather than split out because the alternative was committing this merge with --no-verify, and a gate that fires on somebody else's merged work teaches exactly that bypass.
astubbs
added a commit
that referenced
this pull request
Sep 2, 2026
Master's Node-default ruling for new scripts in bin/ (bin/lib/source-patterns.mjs, landed 2026-09-01 on #403) fires on this branch's self-test, because the gate reads NEW as "added since the merge base" and merging master moved that base under a file written well before the rule existed. Taking the shell-justified escape hatch rather than porting, on the ruling's own reasoning: it grandfathers existing scripts, is explicitly not a migration backlog, and says churn is its own risk. bin/test-release-notes.sh is the release renderer's only safety net and the port is not this PR's subject. Left deliberately visible rather than silent - the justification names the rule and the date, so a later sweep can find it and decide the port on its merits.
astubbs
added a commit
that referenced
this pull request
Sep 2, 2026
… port bin/release-notes.py landed the same day the Node-default ruling did (bin/lib/source-patterns.mjs, #403), which says the repo already chose Node over Python. The gate matches only sh|bash, so nothing flagged the .py; the .sh beside it carries a shell-justified: line. Owner's ruling, 2026-09-02: let it ride, port later. This line is where that decision lives, so the next sweep finds a decision rather than an oversight. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMeraeBL2ycXVHEaTHM86W
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Node becomes the default language for new scripts in
bin/, and adding a rule becomes adding a row rather than adding a script. Operator ruling, 2026-09-01.The argument is silent wrong answers, not taste
Shell in
bin/has produced answers that were confidently wrong:ENDFILEparsed cleanly under mawk — the defaultawkon this platform — matched nothing, ran no check, and printed its success line over a file containing the exact defect it was written to catch.expturned out to be a reserved awk function name, a syntax error that only surfaces when the line is reached.Neither was caught by review. But the structural evidence is stronger than either anecdote: two entire gates existed only to police shell's traps —
check-shell-sigpipe.sh(forgrep -qunderpipefailinverting its own answer) andcheck-shell-hazards.sh— plus a shared helper written becausegrep -cprints0and exits1. When a fifth of the tooling is tooling that guards the tooling, the language is the problem. This PR deletes the first of those two, folding its single rule into the table below.What this is not
Not a migration backlog. Existing scripts are grandfathered; churn is its own risk, and
check-source-patterns.mjsonly ever looks at what is new against the merge base. Shell keeps an escape hatch, and it is a sentence rather than a flag —shell-justified: <reason>in a comment, because a written reason is one somebody can disagree with later. Reasons that qualify are listed inbin/AGENTS.md; "it is what the neighbouring scripts are" is explicitly not one, since that is how a default outlives what justified it.Node rather than Python, decided by the repo
.github/scripts/already holds eight JS gate implementations, each with a.test.jssibling, against exactly one Python file in the tree. Node is the established second language here and already carries the testing conventionbin/lacks.A rule is a row, not a script
Most gates here are the same program — walk files, match a regex, complain — each re-implementing file walking, exclusions, an opt-out marker, an exit-code contract and a failure message, in a language where every one of those is a paragraph and each is a place to differ subtly from its neighbour.
bin/lib/source-patterns.mjsis the table;bin/check-source-patterns.mjsis the single runner they share. Two rules seed it: the new-shell-script policy, and one MIGRATED from an existing gate (check-shell-sigpipe.sh, deleted here) rather than invented for the table. Two further rules were written and deleted before shipping - they encoded the awk traps above, and each generalised a single incident into policing a language now frozen for new work, which is the wheel-reinvention the table header now forbids.Every row must carry a
why, asserted by the self-test. A check that has to think — parse XML, call an API, compare numbers — is a real program and still gets its own file.The reviewer grant does not cover a new language
bin/AGENTS.mdrecords thatcheck-*.shis granted to the review agent by pattern. A Node gate is invoked asnode bin/check-x.mjsand matches none of those patterns, so the reviewer would silently have been unable to run it — running fewer checks than the directory contains, with nothing to say so.Bash(node bin/check-*.mjs:*)andBash(node bin/test-*.mjs:*)are granted alongside the shell ones.CI compile-checks every Node script — and that is a compile, not analysis
pr-checklist.ymlrunsnode --checkover every tracked.mjsand.js, and fails if the glob matches nothing — a loop that found no files exits 0 and is indistinguishable from a clean pass.It is not static analysis and is not described as such. JavaScript is the one language CodeQL's default setup here does not cover (
actions,java-kotlin,python), so the repo's eight existing JS gates and these new ones are unanalysed. Enablingjavascript-typescriptis a repository setting, not a file — it cannot be done in a commit, and is the operator's call.This PR is the base of the stack
bin/check-all.shnow globscheck-*.shandcheck-*.mjs, and dispatches by extension so a.mjsgate runs undernoderather thanbash. Both halves are needed and they close different holes:*.shdoes not make a Node gate fail — it makes it invisible. The sweep prints15 ran, 15 passedhaving swept one fewer gate than the directory contains, and says nothing. Quieter than a failure and strictly worse.bash -nthenbash, which gives a.mjsfile a bash syntax error and reports a clean rule as a broken gate.Those lines were briefly carried on #401 instead, on the reasoning that whichever branch merges first must have them. True, and the wrong conclusion: it made the PR that establishes Node in
bin/depend on one that already writes Node. The convention lands first; work written under it follows.Merge order: this PR, then #401, then the confluentinc#857 branch that depends on both.
Verification
node bin/check-source-patterns.mjs— clean, 3 rules over 190 files in scopenode bin/test-check-source-patterns.mjs— all pass, including the must-NOT-match halfnode --checkover all 11 tracked.mjs/.jsfiles — cleanbin/check-issue-refs.sh— cleanA defect this PR's own self-test caught: the reserved-word rule flagged
if (exp == 3), because==starts with=. A pattern verified only against text that should match is a pattern nobody has shown to be selective — which is exactly how this repo shipped a gate that matched nothing and reported success.The third instance of this PR's own defect class
The rule table needed
check-all.shto learn a second suffix in two places — the gates loop and the self-tests loop. Neither made a Node gate fail; both made it invisible, so the sweep printed "15 ran, 15 passed" having swept one fewer than the directory contains.Review found a third, and it was the worst:
.githooks/pre-commitnames gates by filename, and its missing-file branch is deliberately silent — "a branch older than a gate simply does not have it. Not an error - say nothing." Deletingcheck-shell-sigpipe.shtherefore removed it from every local commit with nobody told, and the replacement was never registered — so both the sigpipe rule and the new-shell policy were dark locally until CI. Registered, with the failure mode written into the array's own comment.What the codex review found, and the pattern behind it
Ten findings, two P1. Four of the ten came from one blanket
sedsubstituting the deleted filename across the docs — a filename carries properties, and swapping one for another silently transfers claims true of the first and false of the second. The worst said this PR doubled a scope gap it had actually closed; another had ShellCheck aborting while analysing a JavaScript module; another pointed the migration backwards, telling a reader to fold the rule into the shell gate.Fixing them by hand then repeated the mistake in miniature: substituting a description for the name is safe in a present-tense sentence and wrong in a historical one. The distinction is present-tense versus historical — a historical sentence must keep naming what was there at the time.
Also corrected:
.github/scripts/holds four JS implementations with four.test.jssiblings — eight files, not eight implementations. The original wording implied sixteen and overstated the quantitative case for Node.What the claude review then found, which codex had missed
It answered the focus question codex was asked and never addressed, and it reproduced the failure rather than reasoning about it — its own review checkout is shallow.
git merge-base HEAD origin/masterhad no shallow guard. On a shallow clone that call does not error; it returns a commit that is not the merge base, exit 0 — so the added-files list feeding the headline rule is silently wrong. That is the silent-wrong-answer class this PR argues against, occurring in the mechanism built to prevent it. Now checks--is-shallow-repository, provesorigin/masteris actually present, and fails closed with exit 2. Verified by cloning this repo--depth 1and running it there.And the fix would have caused a second bug without its partner.
.githooks/pre-commitregistered this gate with no soft exit codes, so the new "cannot run" would have hard-blocked every commit on a shallow or narrow clone — the exact anti-pattern that hook's own header warns about. Now":2".Staged files were invisible to the rule (codex, filed P2 — arguably misfiled).
git diff <base> HEADcannot see a file staged for its first commit, so at the pre-commit hook — the one place the rule is meant to stop a new shell script before it lands — it saw nothing and allowed exactly what it exists to reject. Now the union of committed and staged additions; verified by staging abin/*.shand watching it get rejected.Two known limits of the sigpipe regex are now stated rather than implied: a pipeline split over a line continuation is not caught, and a heredoc body containing
| grep -qas literal data is a false positive. Both checked against the deleted gate rather than assumed — they are parity, not regressions.Checklist
bin/AGENTS.mdgains the rule as its first section, ahead of naming, because language choice comes firstdocs/features/—N/A - repo tooling and conventions, no user-facing behaviourbin/test-check-source-patterns.mjs, control pairs (must-match and must-not-match) for every rule, plus assertions that each row carries awhyand afixce-simplifyandce-code-reviewlocally —N/A - asking for @claude review on the PR instead, which is the route that can open inline threads