Repository navigation
ci(static): turn on the analysis this repo was not running, and register what stays off - #356
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 |
🧪🔒 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 |
|
astubbs
changed the title
|
@codex review this |
|
@claude review this A steer, since this branch is mostly about analysers whose failure mode is looking clean. The thing worth being hardest on is any place a lane can report success having measured nothing - that is the defect class the whole PR exists to remove, and it has already been found twice inside the fixes for it. Highest value first:
Also worth a look, lower stakes: the SpotBugs lane runs What I would rather you did not spend time on: the four rule registries under Note this PR is gated on #347 and #348, so a red |
|
Claude finished @astubbs's task in 6m 53s —— View job Review complete
Went after the parse rather than reasoning about it — built two fixture 1.
|
|
Answering the two bot reports on this PR that carry something real, so neither sits unaddressed.
This one is mine, and it is deliberate. The That produced two structurally similar eight-line helpers, and I did not merge them. Sharing needs the message and the logger to travel with the call, because the two log different text under different logger names, and that text is what an operator greps for when a container executor was not picked up. The options were:
I took the third and recorded why in the code. Happy to be argued out of it. The other rows are pre-existing and untouched by this PR -
Expected, and the number is consistent with what the registry records. The measurement in Two things worth stating plainly, because a
|
![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
parallel-consumer/bin/ci-mutation-test.sh
Lines 352 to 353 in 19b504c
Avoid SIGPIPE while truncating the survivor table
When the survivor text following the first 50 rows exceeds the pipe buffer, head -50 exits early, printf receives SIGPIPE, and set -o pipefail terminates the script with status 141 before it reaches the mutation verdict. A sufficiently large but otherwise valid PIT report therefore makes the advisory lane look broken and leaves its summary incomplete; truncate without an early-closing pipeline, such as with a reader that consumes the full input.
AGENTS.md reference: bin/AGENTS.md:L30-L34
ℹ️ 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".
Confirmed and fixed in df53f9d. (Replying here rather than in a thread - this one arrived in the review body, so it has no thread to resolve.) The consequence is worse than a truncated table, which is why it was worth fixing rather than noting: the pipeline dies before the verdict block, so the lane whose entire purpose is to distinguish "scored mutants" from "measured nothing" would itself exit 141 and report neither. A large but perfectly valid PIT report makes the guard look broken. The cap is now awk's rather than head's: awk consumes its whole input and simply stops printing after NR 50, so nothing closes early. One process fewer, and the "showing the first 50 of N" line below it is unchanged. On the defect class, since a fix that only removes today's instance invites tomorrow's.
So the gate stays as it is - widening it to Good catch. Along with the RacerD self-test arm, that is two findings on this branch where a check reported success without doing its work, which is the thing the branch is about. |
astubbs
left a comment
astubbs
left a comment
There was a problem hiding this comment.
for the check that dont cause an error, do they at least get reported? and in the mvn build output if that's where they attach?
needs work
… seeing nothing #356 flagged that this work's framing - "no static analysis this repo runs can see the torn-read class" - is measurably too strong. It is, and it is over-strong in exactly the way this PR already caught itself being once. WHAT WAS ACTUALLY MEASURED, AND WHAT WAS CLAIMED. The measurement was stock SpotBugs at `effort=Max`, which was the only static analysis the repo ran that day, reporting nothing on any of the four calibration targets - including the nine-line textbook probe. The claim generalised that one configuration to the whole tool class. #356 turns fb-contrib on and measured its `MUI_CONTAINSKEY_BEFORE_GET` naming `ShardManager.removeWorkFromShardFor` statically, in seconds, with no harness: #345's seam, and one of the four targets. Verified here independently rather than taken on report - that method is a literal `processingShards.containsKey(shardKey)` followed by `processingShards.get(shardKey)` with the result dereferenced, nothing held across the pair, so the detector has a real shape to match and the finding is credible on inspection. So the honest width is three of four, not four of four. #346's seam is a stale-check rather than `containsKey`-before-`get`, and the two value-divergence torn reads are not what a check-then-act detector looks for. What survives unqualified is the claim that actually carries the lane: the seam fb-contrib names is one somebody had already found by hand, whereas the defect Lincheck turned up was on nobody's list. WHY THE SAME SHAPE TWICE. This PR's own headline result is that the WorkManager hit rate is machine-dependent - 2.33% per iteration here against 0.69% on Antony's machine, a likelihood-ratio test rejecting equality at p = 0.011 - so no single-machine calibration transfers. A rate measured on one machine is not the rate; a family one analyser cannot see is not a family no analyser can see. Both are a sample of one generalised to a population, and the second slipped through while the first was being corrected. WHERE EACH EDIT LANDS. `docs/testing.md` owns the Lincheck lane and is durable reference, so the corrected width is stated there in place - it would otherwise be flatly false on master the moment #356 lands, and #356 `depends on` this PR, so this one lands first. `docs/plans/` is a dated record whose claims may not be rewritten (`docs/citations.md`), so the plan doc keeps its claim and gains a flag beside it naming `docs/testing.md` as the owner of the corrected version. ALSO FIXED: THE PLAN DOC CONTRADICTED ITSELF ON THE MACHINE QUESTION. Its interim correction paragraph read the 2-misses-in-8 / 0-in-8 split as "the signature of a bound sitting on the edge rather than of a difference between machines", and then instructed the reader to read two tables through that conclusion - which the 48-run fit later in the same section refutes with the likelihood-ratio test. A reader who stopped at the first complete-looking statement got the finding backwards. Restated as what the split actually shows: ambiguous alone, and settled by the fit as both a marginal bound and a real difference between machines. No measurement changed. No test or assertion changed, and the bound stays at 1,000 - the slower machine's refinement remains open and Antony's call (`docs/inflight/test-lincheck-lane-open-items.md`). Lincheck lane re-run on this tree to confirm the master merge left it intact: all five classes selected, 8 tests, fresh surefire reports, green - which for these harnesses means Lincheck still finds every seam. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t roster
The did-anything-run guard in `bin/lincheck-test.sh` asserted `selected -gt 0`. Zero is the loud
failure; a SHORT count is the quiet one. If three of the five harnesses stopped being selected - a
rename, a dropped `@Tag("lincheck")`, a changed surefire include pattern - the guard still passed and
the summary still printed a healthy-looking "Lincheck report files: 2". That is the same false green
the guard exists to catch, one step in.
So it now asserts the exact roster: `EXPECTED_LINCHECK_CLASSES=5`, skipped when `LINCHECK_TEST`
selects a single class, where a short count is the whole point. Adding a harness deliberately fails
until the number is updated. That friction IS the check - a harness the lane silently stopped running
is indistinguishable from one that never existed, so the alternative is a number nothing defends.
Suggested by the reviewer on #356 ("if it is cheap, an assertion that the lane selected
exactly five Lincheck classes would close the same hole from the other side"). It was cheap.
RED-PROVEN RATHER THAN OBSERVED PASSING, because a guard nobody has seen fail is decoration
(`docs/investigating.md`). Three arms, one variable each:
expected=4, whole lane -> exit 1, "Lincheck report files: 5, EXPECTED 4" + the roster message
expected=5, whole lane -> exit 0, 5 reports, 8 tests, all five classes named in the log
LINCHECK_TEST=one class -> exit 0, 1 report, exact check correctly skipped
Also records the class in its owner,
`docs/solutions/workflow-issues/a-check-that-reports-success-without-having-run.md`, rather than
starting a second home for it: a table row, plus a section 7 generalising what the original bug and
that doc's existing OSS Index cache trap have in common. **A guard reading a location it does not own
needs to establish provenance, not just presence** - point a structural did-it-run check at a shared,
persistent directory and it can no longer separate "this run produced nothing" from "a previous run
produced something", which are the exact two states it exists to separate. Nobody thinks of `target/`
as a cache, which is why this instance got past the same authors who had already written the cache
trap down.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…osts static analysis too Three review findings on #348, none of them touching the probe bodies, the control arms or the module's deliberate unwiring. THE ONE FACTUAL DEFECT. The note twice said `docs/refactoring.md` records the same "volatile for the flags" fix on **two** sibling fields. Its `AT_STALE_THREAD_WRITE_OF_PRIMITIVE` entry lists **three** - `AbstractParallelEoSStreamProcessor.lastWorkRequestWasFulfilled`, `ConsumerManager.commitRequested` and `RetryQueue.closed` - and all three are still plain in the current tree, checked rather than assumed. The count is externally verifiable and this note's whole job is to be a precise accounting of what is open, so it is now three and they are named. Naming them also makes the entry usable: the next person growing the module does not have to go and find which fields were meant. THE UNWIRING COSTS STATIC ANALYSIS, NOT JUST CVE SCANNING. SpotBugs runs `-pl parallel-consumer-core -am` today, so `jcstress-poc` is one of several unanalysed modules and nobody would remark on it. #356 widens that to the whole reactor with `includeTests`; a module reached through neither `<parent>` nor the root `<modules>` is outside the widened sweep as well, which leaves the probe classes the only Java in the tree nothing analyses - and they are concurrency code. Recorded beside the dependency-lane bullet it mirrors, so a later reader sees the exclusion was priced rather than overlooked. The containment argument still wins; the module stays unwired. THE POSITIVE-CONTROL ITEM NOW NAMES ITS MECHANICAL CLOSURE. "No check makes a vacuous run distinguishable from a clean one" stated the gap without saying what would close it. The fix is to assert a non-zero observation count rather than instruct a human to read the calibration - the same instruction #356 reached from three unrelated instances of the class. An open item that names its own remedy can be picked up; one that only names the hazard gets re-diagnosed.
…thread, and defer @GuardedBy until a checker reads it **`@GuardedBy` is not adopted here, deliberately.** It appears nowhere in this codebase and `spotbugs-annotations` is not a declared dependency - only the SpotBugs plugin is. Adding the annotation now means adding a dependency and writing assertions nothing verifies, on a branch that is not the static-analysis change. #356 turns on RacerD, which is the tool that actually CONSUMES `@GuardedBy`, and NullAway, which has an opinion on the nullable `ThreadFacts` fields. Annotate after that lands. An unchecked annotation is a claim with no enforcement - the shape this repo keeps finding and removing. **What is worth stating now, because it is already load-bearing.** `ProgressProbe`'s fields split three ways: safely published (volatile peaks and toggles, concurrent maps, the two synchronized finding lists), sampler-thread-confined plain fields (`lastCount`, `lastAdvance`, `rebalanceDwellStart`), and `samplerThread` itself. The two seams this branch extracted - `recordRebalanceDwell` and `recordLagStagnation` - are package-private and driven by broker-free tests from the MAIN thread. They are safe because they touch only the first group and never the confined clock fields; the caller owns the re-arm. **That is why those tests are legitimate rather than racy, and nothing said so.** Now documented at both methods, with the constraint stated as a constraint: reaching for a plain field inside a seam would silently turn its tests into a data race, and no current check would notice. That gap - a real invariant guarded only by prose - is the concrete reason to annotate later, rather than a tidiness argument, and it is what the new note records so the follow-up is mechanical instead of re-derived. Three gate catches worth naming, since all three were real: `blind-spot` is a bug impact and this is a task (`test-debt`); the note cites a rule registry that only exists once #356 lands, so it carries a scoped `file-refs: N/A`; and its reference to this PR needed the post-merge marker. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ef027e0 to
c6282ba
Compare
|
@claude review this Focus on It exists because the previous push failed
Secondary, much lower priority: Context for the rest of the PR: it is now 10 commits re-cut from 55, both dependencies (#347 and #348) have merged, and all seven earlier review threads are resolved. No need to re-review that ground. |
|
Claude finished @astubbs's task in 8m 19s —— View job Review complete
One limitation up front: this session's sandbox refused every 1. Exit-status handling - one real gap, everything else is sound
The real gap:
Suggested fix: 2. The exception list - "the names resolve" catches rot in one direction onlyThe assertion ( The mirror-image gap is the opposite direction: a new A stronger invariant than "the name resolves": have each excepted script assert its own excuse where practical - 3. The membership test - you're right, no collision
4. Self-test honesty - the named arm is genuine; one blind spotTraced each arm against
5.
|
…ds themselves No linter ran over `bin/` or `.claude/hooks/` at all, in a repo whose gates are shell and whose recurring defect is a script reporting success without having run. `shell: lint` gates at `error` only; the severities below it are tiered in docs/inflight/static-shell-lint-severity-tiers.md with what turns each on. Two real defects in the guards, both found by the lane on its first run: `bin/check-shell-sigpipe.sh` opened a prose comment with the word `shellcheck`, so it parsed as a DIRECTIVE, errored, and aborted analysis of the rest of that file - a sentence about the linter silently switched the linter off, inside the script whose whole job is guarding against silent failure. The warning count rose from 10 to 14 once that file was actually analysed. The same guard then rejected a line that already used the herestring it prescribes: `[ -z "$w" ] || grep -qF "$w" <<<"$out"` is a logical OR, but the regex read the second `|` of `||` as a pipe. A guard that fires on its own recommended fix is worse than no guard, since the only way to satisfy it is to rewrite correct code. Anchored with `(^|[^|])`, and pinned in both directions - the OR-plus-herestring must pass, and a real pipe with an `||` earlier on the line must still fail. `bin/check-quarantine-registry.sh` gets the brace quoting the lane asked for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyqWU2v4hy7NGoxdrfqb74
…ed the wrong thing Across the last 40 `maven.yml` pull_request runs the lane reported success 40 times and scored ZERO mutants 40 times. Each skip was individually correct - 23 had no core main-source change, 14 were outside the decidable packages - but "correct skip", "stale scope that can never match again" and "185 mutants killed" were all the same green tick. The exit code is now the verdict, four states, none sharing a code: 0 scored, 2 CANNOT RUN, 3 nothing in scope, and PIT's own status passed through. **The verdict also counted the wrong number.** `>> Generated 42 mutations` says PIT created 42 mutants, not that any were scored - so a run whose minions all die returns MEMORY_ERROR for every one, generates plenty, measures nothing, and passed. Not hypothetical: the plan that scoped this lane recorded `state.*` hanging with minions dying. The verdict now reads mutations.xml and gates on mutants EVALUATED, treating a missing report as a broken lane rather than a clean one. Control arm: with the verdict back on the generated count, exactly the three new red arms flip, and they flip to 0. A `head -50` truncating the survivor table could also kill the run before the verdict: under `pipefail`, once the unread remainder passes the 64 KiB pipe buffer, printf takes SIGPIPE and 141 becomes the script's status. awk now caps at NR 50 and consumes its whole input. Swept every `| head` under pipefail in the tree - this was the only one where it was not harmless. 20 self-test arms, each red control paired with a green near-miss. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyqWU2v4hy7NGoxdrfqb74
…an a count The tool was recorded as blocked and never run: Infer's Maven integration builds under a JDK of its own choosing and this project needs 17. Capturing javac directly sidesteps the wrapper, established with a two-arm control - the exact command Infer runs succeeds standalone at JDK 17, and Infer's captured Maven output carries JDK 24+ warnings the JDK 17 run does not emit. It finds 13 thread-safety findings, including `lastCommitTime` which was on nobody's list. The verdict on the calibration is negative and recorded as such: RacerD does NOT refind the four named torn-read races, because those are check-then-act and two-read divergence while RacerD models unguarded access. **The ratchet compares identities, not a count.** A ceiling of 13 cannot tell a fixed race from a race swapped for a different one - the total is 13 either way - which an independent review caught. It fails in both directions: a new identity means a race was introduced, and one that stopped firing means somebody fixed a race without ratcheting, which is how a set quietly stops meaning anything. Both `comm` inputs are `LC_ALL=C sort`ed, because Python's tuple order and the shell's lexical order disagree and the first version reported known findings as new. Four ratchet arms, driven through a new `RACERD_DRY_RUN_REPORT` seam so they need no toolchain. The decisive one is the same-count swap: regress the ratchet to a ceiling and exactly that arm flips, to 0. The JDK near-miss arm resolves `RACERD_JDK`, then `JAVA_HOME`, then the SDKMAN path, and fails closed under `CI` - it previously looked only for a developer's own SDKMAN install, printed `skip` on a hosted runner, and incremented neither counter, so the one arm proving the JDK guard does not misfire had never run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyqWU2v4hy7NGoxdrfqb74
Both exist for the same reason and were both written after this branch proved the need on itself: knowing a check exists is not the same as running it, and knowing a tool reported something is not the same as reading it. **`bin/check-pr-analysis-surfaces.sh [PR]` - read the analysis on your own PR.** The tools here report to five places that are not each other: the Maven console, check annotations, sticky PR comments, job summaries, and files under `target/`. Checking all five by hand is a scavenger hunt, so it does not happen, and a finding nobody read is indistinguishable from one that does not exist. This work is the proof - it switched on `-Xlint:all` and SpotBugs-over-test-code, both fired on files it was editing, including on the exact line it had just rewritten to satisfy a DIFFERENT detector, and nobody looked. A human found them scrolling Files Changed. Two things make it usable rather than another wall of output. It splits by DIFF LINE, not by file: a PR adding one helper to an 1800-line class inherits every finding in it, so only findings on lines the diff wrote set the exit code and the rest print as inherited context. And it de-duplicates across jobs, because `setup-java` registers javac's problem matcher so every compiling job annotates the same warning - one deprecation reported by eight jobs is one finding, with all eight named. **`bin/check-all.sh` - run every gate, discovered rather than remembered.** A push on this branch failed `check-branch-self-reference.sh` in CI after a local sweep of seven gates chosen by hand. The gate was not new, not subtle, and not broken; it was not on somebody's list. There are eighteen `check-*.sh` and twenty-two `test-*.sh` in `bin/`, so this globs both sets and a gate added tomorrow is swept with no edit anywhere. That is the whole design, which is why the discovery loop must stay a glob rather than becoming the array it keeps wanting to be. Its exception list is the one hand-maintained thing left, and it is guarded: five scripts are not tree gates - four report PULL REQUEST state, one needs a maven-log argument - so they are skipped by default and run under `--pr`, and every name in the list is asserted to exist. Rename one and the runner exits 2 rather than quietly sweeping one gate fewer than it claims. **Neither counts a skip as a pass**, which is the rule this repo keeps relearning. Both exit 2 for "cannot run", and `check-all` gives exit 2 and exit 3 their own columns outside the pass count - a runner that laundered "could not measure" into "measured and found nothing" would be the defect it exists to catch, one level up. Both are `gh`-reads-only, which is what keeps the `check-` prefix honest under bin/AGENTS.md: the prefix grants them to the review agent by pattern, and the reviewer is exactly who should be running them. Twenty-three self-test arms between them, fixtures in throwaway directories so the real repository cannot make an arm pass by accident. The pairs are the point - the same finding inside and outside the diff must land in different sections, and deleting an excepted script must make the sweep refuse. One arm in this family first passed for the wrong reason, an empty `PATH` exiting 127 on "bash: command not found", and now keeps a shell on PATH so it tests what it claims. **What an independent review then found in them, and what it got wrong.** A gate bash cannot PARSE exits 2, the same code as "cannot run", so an unparseable gate landed in the skip bucket. The review called that "never fails the sweep", which is too strong: measured, `check-shell-lint.sh` runs ShellCheck over all of `bin/` at severity=error and an unterminated `if` is SC1072/SC1073, so the sweep normally goes red via the linter. The residual hole is narrower and real - with ShellCheck absent the linter itself exits 2, both skips agree, and the sweep reports zero failures. `bash -n` now runs first, and a parse failure is a FAIL that names the broken gate rather than a skip that excuses it. The exception list's assertion caught rot in one direction only. Existence proves a rename did not happen and says nothing about a script whose recorded REASON expired - one that stopped needing an argument would sit skipped forever. Each reason is now corroborated by an independent signal in the script itself: the PR-scoped four must actually call `gh`, the needs-args one must actually print a usage line. `check-pr-analysis-surfaces.sh` classified findings on `start_line` alone, but annotations carry an `end_line` and a SpotBugs finding spanning a method can start on an untouched line while ending inside one the PR added - misfiled as inherited, a false negative in the direction that matters. Any overlap now counts as yours. It also hard-stopped after five pages of annotations in silence; past 500 it says so, because "no findings on your diff" quietly meaning "none in the first 500" is the false-clean it exists to prevent. Two findings were checked and deliberately not acted on. The `[[ " $LIST " == *" $n "* ]]` membership test is sound - the space padding makes a partial-name collision impossible, and `$n` is quoted so a filename carrying glob metacharacters matches literally. And `--pr` keeping the needs-args gate skipped is coherent rather than arbitrary: `--pr` widens scope, while that script is excluded because the runner has no value to supply for its argument, and inventing one would be the measure-nothing-report-success failure this branch is about. The review also found the gap in the tests themselves - every arm passed `--gates-only`, so the self-tests loop and the documented "self-tests run before gates" ordering had no coverage at all. 10 arms to 16. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyqWU2v4hy7NGoxdrfqb74
…d register what stays off SpotBugs analysed one module of five, main code only, with stock detectors. There was no `<compilerArgs>` block in this build at all, so javac's own analysis was entirely off. Neither Error Prone nor forbidden-apis existed. Now: SpotBugs over the whole reactor including test code, with fb-contrib, find-sec-bugs and findbugs-slf4j; Error Prone 2.42.0 with NullAway 0.12.7; `-Xlint:all` without `-Werror`; forbidden-apis on `jdk-unsafe`; and the enforcer's `requireUpperBoundDeps`. **Error Prone is pinned below the JDK 17 line and cannot simply be bumped.** 2.43.0 is the first release compiled at class file 65, JDK 17 reads to 61, and Jabel's Byte Buddy refuses Java 21 - the two constraints are mutually exclusive, so the pin is a floor and a ceiling at once. **Suppression is a checked-in rule filter, not a regenerated baseline.** The baseline job rewrote an exclude filter on every push to master keyed on class, method and bug type, recording nothing about what it swallowed, so anything not triaged inside one PR window was absorbed permanently. Every entry in config/spotbugs-exclude.xml has a row in the registry with a reason and a re-enable trigger. Coarser on purpose: an off rule is visible in a diff and the off-count only goes down. **Two thirds of the -Xlint channel was the Truth generator's own output** - 559 of 843 warnings, 535 of them `rawtypes` on generated `List` parameters. javac cannot scope a warning by path, so `rawtypes` is off for TEST compilation only: main keeps every category, which is where the 58 real warnings are. 843 to 266, predicted before running and confirmed after, and verified through `help:effective-pom` rather than a green build - the check that caught the first `-Xlint` attempt landing in a profile CI never activates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyqWU2v4hy7NGoxdrfqb74
Adds `static: spotbugs` over the whole reactor, `static: racerd` on ubuntu-latest, and wires the new self-tests into Repo Hygiene. Deletes the `spotbugs-baseline` job the rule filter replaces. **SpotBugs runs `spotbugs:check -Dspotbugs.failOnError=false` as well as `spotbugs:spotbugs`, and the second goal is the reporting half.** `spotbugs:spotbugs` writes the XML and prints not one finding - the log says "Skipping ... report goal" and stops - so every bit of reporting was GitHub-side and a local build was silent over hundreds of findings. `check` prints each finding WITH its rule name, which is the token needed to tier it, and `failOnError=false` keeps the lane advisory. **The Infer archive is verified against a pinned SHA-256 on every run, cache hits included.** The cache is the mutable half: a fresh download arrives over TLS from a pinned release URL, a restored one arrives from a writable store keyed by a string. Verifying only the fresh copy would check the safer half and unpack the other unexamined. `setup-java` is pinned on the RacerD preflight self-test step, because that self-test now fails closed without a JDK rather than skipping. test-compile, not compile: the pom sets `includeTests`, and `target/test-classes` has to exist for that to mean anything. Whole reactor rather than `-pl parallel-consumer-core -am`, since `-am` adds only the parent pom and left vertx, reactor, mutiny and the examples unanalysed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyqWU2v4hy7NGoxdrfqb74
…sis found
**`@SuppressWarnings("BanJNDI")` sat on three method bodies, one of them fifty
lines.** A suppression is a claim about one call; at method scope it also excuses
every JNDI lookup a later edit drops into that body. Both classes now carry a
private helper whose entire body is the call being excused. The two are
deliberately NOT shared: they log different messages under different logger
names, and that text is what an operator greps for when a container executor was
not picked up. Verified by mutation - removing one annotation fails the compile
with [BanJNDI] - because otherwise "it compiles" would equally describe Error
Prone not running.
`PCMetricsDef` used default-locale `toUpperCase`/`format`, so generated
DOCUMENTATION differed by the author's locale. Fixed with `Locale.ROOT` - and
then fb-contrib flagged `s.replace("_", " ")` on that same line, both arguments
length-1 Strings where `replace(char, char)` exists. Satisfying one detector on a
line while leaving another's finding on it is worse than not touching the line,
because the diff then reads as reviewed. Swept the class: one such site in main
code tree-wide.
The metrics example's Prometheus endpoint called `response.getBytes()` twice -
once for Content-Length, once for the body - in the platform default charset. Two
encodings of one string that disagree the moment a tag is non-ASCII.
`ReactorTest` called `new Thread(...).run()`, which runs on the calling thread.
fb-contrib and Error Prone found it independently, disagreeing with a human audit
that had ruled it a defensible scratchpad.
An ArchUnit rule pins that the shard map is never replaced. The nested AGENTS.md
makes `@GuardedBy` a policy that arrives with the file rather than a backlog
entry; its CLAUDE.md bridge needs the `.gitignore` negation, without which it is
silently never committed - which happened once already here.
`LincheckToolchainProbeTest`'s javadoc said SpotBugs sees nothing on its shape.
True of the STOCK detectors; with fb-contrib it is named in seconds, on that very
probe. Narrowed rather than deleted, since the probe is still the right red
control for Lincheck.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GyqWU2v4hy7NGoxdrfqb74
…alsified Every rule switched off is listed with a reason and a re-enable trigger, per engine, each with a ranked "turn these on next" so the backlog is a work order rather than a list. Plus the two-profile design - full rules on new code, a small enforced set on all of it - which is classified here and deliberately wired in a later PR. **Notes are injected as current state, so a stale one misroutes the next agent rather than merely being wrong.** Corrected here, all found during this work: - the `-Xlint` inventory said "172 warnings, all in test code or against deprecated third-party API". Both halves wrong: 58 are main code and our own API. A headline count also means nothing without saying whether generated sources are in it - the same tree is 843 or 284 on that one decision - so the old figure was unfalsifiable rather than merely stale. - the SpotBugs baseline described in the present tense after its deletion. - `RFI_SET_ACCESSIBLE`'s "two sites", which is 12 calls across 5 files. - a coverage note whose every claim this work falsified - retired per docs/inflight/AGENTS.md rather than rewritten into a DONE narrative, with the one genuinely open question kept as its own note. - the build-hardening register, extracted here off an encoder-race PR, ends with all thirteen entries struck and is cut from 252 lines to 64. Three inbound citations naming a specific entry are repointed at the notes that now own the subject: a citation whose file resolves but whose anchor does not is exactly what the cite-by-anchor rule exists to prevent, and no gate checks it. - the jcstress containment note, which recorded in the FUTURE tense that this PR would make `jcstress-poc` the only Java in the tree nothing analyses. Written at the request made on #348 while this was still open; merging master brought both halves together, so it now describes as pending a state that exists. Resolved, and given the follow-on it lacked: if those probes ever want analysis, scope a SpotBugs invocation at that module rather than pulling it into the reactor, since joining the reactor is what the containment exists to prevent. Also: `@SneakyThrows` blinding SpotBugs dataflow now has a home rather than being a row in a rule registry, with 21 main-code sites counted and the cheapest experiment written down; a polyglot survey for the ten non-Java clients, whose headline is that CodeQL already supports nine of them and the Go client's gating test command has no `-race`; and three solution write-ups for the defect class this work kept hitting. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyqWU2v4hy7NGoxdrfqb74
f097df9 to
147067e
Compare
Brings in the static-analysis lanes (#356 work): ShellCheck gate, -Xlint:all, SpotBugs-over-tests, the RacerD/PIT lane fixes, and the two pre-review commands bin/check-all.sh and bin/check-pr-analysis-surfaces.sh. docs/agent-harness.md merged cleanly on both sides' edits. Gates re-run after the merge per the inherited instruction: bin/check-all.sh. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019HwDcZdfv1Ne4u3g6ThrG9
…on is test-scope only The engine's new AGENTS.md rule, inherited from #356, says to write `@GuardedBy` in the same change that fixes a race, and that "no separate project is needed". Followed literally in `parallel-consumer-core/src/main/java`, that does not compile. Measured rather than inferred: a throwaway annotated field in `internal/` fails with error: package com.google.errorprone.annotations.concurrent does not exist `error_prone_annotations` resolves at TEST scope only - it arrives transitively and nothing declares it - so Error Prone's GuardedBy CHECK runs on main code while the ANNOTATION is unresolvable there. The failure message names the import, not the scope, so it reads as "Error Prone is not set up" when the analysis is in fact wired correctly. `jcip-annotations` is present and also test-scope; `jsr305` is absent entirely, so all three spellings Error Prone accepts are equally unavailable. Two changes, both small: - The engine rule now states the prerequisite where an agent meets it, and routes to the note for the options. Left as a pom change to make deliberately rather than folded in here: a published library's compile-scope dependencies are not a chaos-suite PR's call, and `provided` scope is probably the right answer given @GuardedBy's CLASS retention. - The note is rewritten against the real blocker and renamed. Its old premise - wait for #356, because RacerD must land before the annotations mean anything - died when #356 merged, and its claim that `spotbugs-annotations` was the missing dependency was wrong. The old name also carried state ("once-racerd-lands"), which `docs/inflight/AGENTS.md` forbids. Nothing cited it, so the rename is free. The note also now records what the rule's own carve-out settles for this branch: `ConsumerOffsetCommitter.owningThread` is `volatile`, which is publication rather than locking, so there is no lock to name and no annotation to write for it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uu5CHkQG4aFeyMHQu1jQ7M
…and record where it is inert The engine's AGENTS.md tells you to write `@GuardedBy` with the fix. Until now doing that in `src/main/java` failed to compile: `error_prone_annotations` resolved only in test scope, so the CHECK ran on main code while the ANNOTATION was unresolvable there, and the failure named the import rather than the scope - which reads as "Error Prone is not set up" when the analysis was wired correctly all along. Declared at `provided` scope: @GuardedBy is CLASS-retention, so javac needs it, nothing reads it at runtime, and consumers of this library do not inherit it. Verified with a control probe - an unguarded read of a monitor-guarded field now fails the build at ERROR, naming both the lock and the access, while the correctly-guarded read compiles silently. Testing it turned up something worth more than the dependency. On a ReadWriteLock the annotation is not merely awkward, it is WRONG BOTH WAYS, and the spelling a reasonable person reaches for first is the one that reads as a passing check: @GuardedBy("rwLock") accepted, then NOT ANALYSED - reports nothing @GuardedBy("rwLock.readLock()") fires, but also on every legitimate write-lock-held access @GuardedBy carries one lock expression and a ReadWriteLock is two, so it cannot express "read under the read lock, write under the write lock" at all. Annotating `RetryQueue` with the first spelling compiled clean while `size()` read the map under no lock whatsoever - a claim with no enforcement, which is the shape this repo keeps finding and removing. The engine rule now says not to annotate ReadWriteLock-guarded state and names the two classes it applies to here. Run once with the enforcing spelling, `RetryQueue` produced four distinct verdicts, separated in the note rather than reported as a count: two real-but-dead (`size()`/`isEmpty()` have no callers anywhere), one real and live (`removeAll` reads `unique.isEmpty()` before taking the write lock), and two false positives from the ReadWriteLock modelling. The live one is independently corroborated - static-racerd-findings.md already records `this.unique` read via `Map.size()`/`isEmpty()` racing with writes, from a different engine entirely. Assessed P2 and deliberately not fixed here; the reasoning both ways, and why a chaos-suite branch is the wrong place to review a ShardManager concurrency change, is in the note. The note this replaces claimed the missing dependency was `spotbugs-annotations` and said to wait for #356 before annotating anything. Both were wrong: #356 had already merged, and the missing artifact was Error Prone's own. Renamed off the old state-carrying name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uu5CHkQG4aFeyMHQu1jQ7M
… mid-merge Found by running `bin/check-all.sh` between merging master and committing that merge - which is a normal thing to do, and is exactly when the arm is wrong. The merge base is still the OLD one until the merge commit exists, so every file master added since it reads as a line this branch added, and the arm demands an attestation on notes this branch has never seen. It fired on two of #356's registries that way. Committing the merge makes `git merge-base HEAD origin/master` the merged tip and the accusations vanish - verified on both sides of the commit. Recorded rather than fixed, because the behaviour is correct for the definition the arm uses, and the alternative - diffing against the merge base as it will be AFTER the merge - cannot be computed before that merge exists. The header now says what to do instead: finish the merge, never mark somebody else's note.
…sk the wrong repository (#364) A hook that could not look up a PR said "No PR was found for this branch". Three unrelated situations produced that one sentence and the operator could not tell them apart: the branch really has no PR; the lookup FAILED; or the lookup asked the WRONG REPOSITORY. `2>/dev/null || true` discarded both the error and the exit status, so absence and failure were the same value. The third cause is specific to this fork and the most damaging. None of these calls carried `-R`, and `gh` prefers `upstream` unless `remote.origin.gh-resolved` is set - a local, uncommitted config, so CI runners and fresh agent sandboxes all start without it. AGENTS.md has a section on exactly this, and calls the silent wrong-repo success the case that hurts. Not hypothetical: hit twice in one session on #356, from two different causes, both times asserting "nothing could be measured" without saying why. Each lookup now derives the repository slug from `origin`, runs bounded, and reports which of four states it is in: found; measured absence; failed, quoting what `gh` actually said; or did not run because the slug could not be derived - in which case `gh` is deliberately NOT called rather than called unqualified. Every guard stays fail-closed. This changes the message, never the verdict. The same defect is fixed in `remind-inflight-on-push.sh`, where it was live-dangerous - an upstream PR number would have matched this repo's `docs/inflight/pr-<n>-*.md` and quoted an unrelated note - and in `check-merge-outstanding-work.sh`, which keeps its deliberate fail-open and merely stops being fail-SILENT. A class sweep found a fourth instance in `bin/check-pr-ready.sh`, which could report a stranger's PR as this branch's. Checked and ruled out: `check-branch-self-reference.sh` and `check-pr-analysis-surfaces.sh` already qualify every call; `check-squash-subject.sh`, `pre-commit-gate.sh`, `inject-merge-checklist.sh` and `inject-recorded-knowledge.sh` make no `gh` calls at all. A simplification pass then found the branch's own defect one level down. The lookup contracts to answer found/failed/none, but two hooks dispatched on only the first two, so an EMPTY answer - interpreter killed, or a `BaseException` their `except Exception` cannot catch - matched neither test and fell through to silence, exactly where "none" falls through. `check-history-rewrite.sh` already carried the backstop the other two lacked. Both now route an unrecognised answer to their existing advisory. It also overlapped two independent `gh` calls that had been waiting on each other: measured 1.85s concurrent against 2.34s sequential, non-overlapping across three runs, the gap equal to one round trip. The lookup is now written four times, and the reasoning is narrower than "avoid shared helpers". #341 draws the line at guards versus gates: "Guards are inlined; gates share through `bin/lib/`", because sourcing a helper adds a failure mode where the helper cannot be found, and a guard that cannot load its dependency fails OPEN - four repeated lines beat a new way to pass silently. #357 landed `.claude/hooks/lib/hook-common.sh` while this branch was open, and drew that line in code rather than in prose, which moves it. Its two non-blocking reminders source the library behind `[ -r "$hook_lib" ] || exit 0` - fail-open, and harmless for something that only advises - while every hook that actually refuses a tool call still inlines. The split is therefore refusing-guard against advisory-reminder, not hook against gate, and all of these are `PreToolUse`, so the event type does not tell you which is which. Two of the four copies are free to move on that basis: `remind-inflight-on-push.sh`, which is advisory and already sources that library, and `bin/check-pr-ready.sh`, a gate where a load failure is visible rather than silently permissive. The two refusing guards stay inlined unless someone deliberately changes their failure mode with a fail-closed loader. `docs/inflight/ci-pr-lookup-is-copied-into-three-hooks.md` records that, along with the reason `bin/check-quarantine-owners.sh`'s older slug derivation must not be copied: it predates the `file://` fix and would ask GitHub about a repository that does not exist, reintroducing the very class this closes. The pattern was adopted from #350 rather than invented - origin-derived slug, bounded in python3 rather than GNU-only `timeout(1)`, no swallowing. One departure: these use `gh pr list --head`, which exits 0 printing nothing when there is no PR, so absence is read off the exit status and #350's stderr regex is unnecessary. An independent review raised three defects, all fixed, and overstated a fourth. It said an unparseable gate "never fails the sweep"; measured, `check-shell-lint.sh` runs ShellCheck over all of `bin/` at severity=error and an unterminated `if` is SC1072/SC1073, so the sweep normally does go red. The residual hole is narrower and real - with ShellCheck absent the linter itself exits 2, both skips agree, and the sweep reports zero failures - so `bash -n` now runs first and a parse failure is a FAIL naming the broken gate. The code carries the measured claim, not the review's. A second review then found the branch's remaining gap, and it was this same subject one level further out: `bin/check-pr-ready.sh` received the fix above and shipped with no behavioural coverage at all. Its self-test had eight cases and every one grepped the script's own source text, so they passed identically before and after a behavioural change - nothing exercised the new failure messaging, nothing checked that `-R` reached the `gh` argv, nothing proved `PR_READY_REPO` overrode anything. `bin/AGENTS.md` states the bar that missed: a regression test that has never failed proves nothing. Twelve cases now run the script against a stubbed `gh` that records its own argv, each in a throwaway repository on a named branch. Restoring master's pre-fix script fails 10 of them; the 2 that survive are invariants, so they are controls rather than coverage. The new self-test arms come in pairs, and the pairs are the point: only the arms that CAN flip are red controls, the rest are fail-open invariants that must pass either way by construction. Proven red against the pre-fix hooks rather than merely passing - restore the three hooks from master with the new arms in place and 12 fail, the 10 that pass being the invariants. Those figures are re-measured against this branch's final state after merging master; an earlier draft of this message carried numbers that had drifted from an earlier shape of the branch, which is this change's own subject turning up in its own commit message. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
… at it Found by bin/check-pr-analysis-surfaces.sh, not by the review: SpotBugs (fb-contrib HES_EXECUTOR_OVERWRITTEN_WITHOUT_SHUTDOWN) fires on the foreignThread assignment in this fixture's @beforeeach, and it fires on a line this branch wrote - the category AGENTS.md says you fix or explain, and which nobody had read here. The static lane is non-gating, so it went green and said nothing, which is the #356 shape that script exists to catch. The named harm is not present: the pool's threads are daemons, so a leaked one could not hold a JVM open, and JUnit runs the teardown between tests so the field is never actually overwritten while live. But overwriting a live ExecutorService field is a leak shape regardless of who currently guarantees it cannot happen, and the guarantee here is JUnit's lifecycle rather than anything this class states. Shutting down what is about to be dropped is simply correct. Chose this over the registry route deliberately. docs/inflight/static-spotbugs-rule-registry.md sanctions suppression by RULE, in a checked-in filter, which would switch HES off for the whole repository and lose every future instance in new code - and it prices that at "the count of off-rules only goes down". Three lines at one site is the cheaper answer. Verified rather than assumed: SpotBugs over the module at the CI settings reported the finding before and does not report it after, with test sources demonstrably in scope (test classes appear among the other 303 findings). Same-class sweep: HES_EXECUTOR_OVERWRITTEN_WITHOUT_SHUTDOWN has exactly one other instance in the reactor, ChaosConductor$ChaosConductorBuilder.pcExecutor, which also carries HES_EXECUTOR_NEVER_SHUTDOWN. It is pre-existing on master in a file this branch does not touch, so it is inherited and stays for the registries rather than being bulk-fixed here.
… the pre-push sweep could not see it `PR Checklist` went red on `bin/test-todo-index.sh` - `--check on a current index passes (expected '0', got '1')` - while `bin/check-all.sh` reported clean. Two separate defects, and the second is the one that matters. THE IMMEDIATE CAUSE. `list_files()` scans `*.yaml`, and `src/docs/development/upstream-map.yaml` is DOCUMENTATION that happens to be YAML. Its branch-accounting notes quote markers as evidence - "still carries `// todo refactored to constant in the remove statics branch`" is the point being made about a branch - so the scan indexed the quotation. That is precisely the pathology this script's own header already documents for `.adoc`/`.md`: the index lists the same marker from its .java source, so the entry duplicates and the count inflates. The extension list simply did not cover a doc written in YAML. `src/docs/` is now out of scope, and it contributed nothing before: `git show HEAD:docs/todo-index.md | grep src/docs/` was empty. THE GAP THAT LET IT REACH CI, recorded in `docs/inflight/ci-hygiene-gaps.md` rather than patched here. `check-all.sh` discovers gates by globbing `bin/check-*` and runs each with NO ARGUMENTS. `todo-index.sh` fails both halves: the name does not match, and run bare it REGENERATES the index - a pre-push sweep that mutates the working tree is the one thing it must not do. `--check` is the read-only mode and the uniform contract has no way to ask for it. So this is a design constraint rather than an oversight, which is why it gets a note with three costed options instead of a special case wedged into the glob. It matters because `check-all.sh`'s promise is load-bearing: AGENTS.md sends every agent to it precisely so the gate set cannot drift from what someone remembered. A sweep that says "no gate failed" while a tree-health gate is red is the same failure #356 motivated the script with, arriving through a door it does not watch. Not fixed here because changing the gate every agent runs before every push is not a ride-along for a JStream fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RVXBFaG4YXFP7Gjrj6pbVM
1 participant
Serves no single issue. Turns on the static analysis this repo was not running, records every rule
it switches off so they can be turned back on deliberately, and - by the end - fixes the analysis
lanes' own habit of reporting success without having measured anything.
depends on #347
depends on #348
Those land first by request, and there is a substantive reason too: this PR analyses test code
for the first time, and #347 adds the Lincheck harnesses. The rule registry
should be built against a tree that contains them.
What lands
error_prone_core2.42.0 + NullAway 0.12.7, pinned below the JDK 17 line, with a rule registry-Xlint<compilerArgs>block existed at all-Xlint:all, no-Werror,rawtypesoff for test compilation onlystatic: racerdonubuntu-latest, digest-pinned toolchain, gated on an identity ratchetforbidden-apisjdk-unsafeover main coderequireUpperBoundDepsshell: lint, errors only, with a self-test carrying two red armsThe finding that justifies the detectors
fb-contrib's
MUI_CONTAINSKEY_BEFORE_GETnamesShardManager.removeWorkFromShardFor-containsKey, thenget, then dereference, which is #345's check-then-getseam verbatim - statically, in seconds, with no harness and no annotation.
That narrows a standing claim both concurrency PoCs rest on. The register said "nothing static
does". Honestly bounded, it is one of four known instances: #346's seam
is a stale-check before a lookup, and the two torn-read value-divergence cases are outside what any
check-then-act detector looks at.
Suppression is a registry, not a baseline
The
spotbugs-baselinejob regenerated an exclude filter on every push to master, keyed on classplus method plus bug type, recording nothing about what it swallowed. Deleted. In its place
config/spotbugs-exclude.xml, where every entry has a row indocs/inflight/static-spotbugs-rule-registry.mdwith a reason and a re-enable trigger. Coarser onpurpose: visible in a diff, and the off-count only goes down. Same argument
static-archunit-main-code-rules.mdalready makes for refusingFreezingArchRule. ShellCheckseverities and Error Prone checks are tiered the same way, and each registry carries a ranked
"turn these on next" so the backlog is a work order rather than a list.
643 findings unfiltered, 278 after. The lane cannot fail a build - it runs
spotbugs:check -Dspotbugs.failOnError=false- and the registry now says so outright, with thetrigger that flips it.
The recurring defect, including in this PR's own work
A check that dies, skips, or measures nothing reports the same green as a check that passed.
That is the thread through everything here, and the uncomfortable part is how much of it was found
inside this branch rather than in the code it was pointed at.
<Source name="~.*src/test.*"/>, but SpotBugs recordssourcepathpackage-relative. Build green,XML valid, zero effect. Caught by asserting the excluded rule had reached zero.
-Xlint:alldid nothing for two runs, having landed in theintellij-idea-onlyprofile CInever activates. Caught by
help:effective-pom | grep -c, not by the build.mutants generated rather than evaluated - so a run whose minions all die would still pass.
another passed green at 13 either way. Then its self-test's ratchet arms turned out to assert
nothing at all while printing
ok:.SDKMAN path and printing
skipon a hosted runner.head -50in the mutation lane could kill the verdict by SIGPIPE underpipefail, so alarge but valid report would have made the lane look broken.
|of||as a pipe.forbidden-apismutation arm was contaminated - both arms exited 1 on the enforcer, withzero forbidden-method errors.
Each is fixed with a control arm that flips, and each fix carries the reasoning in its commit.
Reading the analysis, which is the half that was missing
Turning a channel on is not the same as reading it, and this PR proved that on itself:
-XlintandSpotBugs-over-tests both fired on files this PR was editing - including on the exact
PCMetricsDef.toCamelCaseline it had just rewritten to satisfy a different detector - and theauthor never looked. A human found them scrolling Files Changed.
The cause is structural: the tools report to five places that are not each other - the Maven
console, check annotations, sticky PR comments, job summaries, and files under
target/. Sobin/check-pr-analysis-surfaces.shcollapses that into one command, splitting findings on a lineyour diff wrote (yours) from those merely in a file you touched (inherited), and
de-duplicating the same javac warning across every compiling job. On this PR: 28 findings in 3
files, exactly 1 of them ours. It is
gh-reads-only, so thecheck-prefix grants it to the reviewagent automatically.
bin/check-all.shis the same problem one level up. There are eighteencheck-*.shgates andtwenty-two self-tests in
bin/, and an agent preparing a push had to know which applied - so it ranwhichever subset it remembered. That is not hypothetical either: a push on this branch failed
check-branch-self-reference.shin CI after a local sweep of seven gates chosen by hand. The gatewas not new, not subtle, and not broken; it was not on somebody's list.
check-all.shglobs bothsets instead, so a gate added tomorrow is swept with no edit anywhere and nobody has to register it.
Neither tool counts a skip as a pass - both exit 2 for "cannot run", and
check-allgives exit 2and exit 3 their own columns outside the pass count. A runner that laundered "could not measure"
into "measured and found nothing" would be this PR's own defect class, one layer up.
Both went through an independent review round of their own. It found three real defects - a gate
that bash cannot parse also exits 2 and so was filed as a skip; the exception list's assertion
caught renames but not a recorded reason that had expired; and findings were classified on
start_linealone, so one spanning a method could start on an untouched line, end on an added one,and be misfiled as inherited. All fixed. It also overstated its headline: an unparseable gate does
normally fail the sweep, via ShellCheck at
severity=error- the real hole is narrower, needingShellCheck to be absent so that two skips agree. The measured version is in the code.
Two related repairs: SpotBugs printed nothing to the build log until now, and two-thirds of the
-Xlintchannel was the Truth generator's own output (843 warnings to 266, main code unchangedat 58).
Records corrected
Notes are injected as current state, so a stale one actively misroutes the next agent. Corrected
here: the
-Xlintinventory (said "all in test code"; 58 are main), the SpotBugs baseline describedin the present tense after its deletion,
RFI_SET_ACCESSIBLE's "two sites" (twelve, in five files),bin/racerd-test.sh's header denying the CI lane the same commit added, and a coverage note whoseevery claim this branch had falsified - retired, with the one genuinely open question kept.
The build-hardening register, extracted here from #344, ends with all
thirteen entries struck and is cut from 252 lines to 64.
Deferred, with the reasoning recorded
classification is done; the wiring is deliberately a separate PR.
@SneakyThrowsblinds SpotBugs dataflow - 21 main-code sites, now tracked with the cheapestexperiment written down.
dependencyConvergencemeasured and deferred with its tail recorded, whilerequireUpperBoundDepsis on and passing.client's gating test command has no
-race.Checklist
docs/inflight/notes named above.docs/features/- N/A - build and CIonly. The code changes are a bug fix in an example, a locale fix in a doc generator, and a
suppression narrowing.
bin/test-check-shell-lint.sh,bin/test-check-racerd.sh(7 arms including the same-count swap a ceiling passes),bin/test-ci-mutation-test.sh(20 arms),bin/test-check-pr-analysis-surfaces.sh(7 arms),and two new arms on
bin/test-check-shell-sigpipe.sh.