Repository navigation
fix(ci): a comment inside the PIT invocation's line continuation truncated it, so the lane mutated every module - #371
Conversation
…cated it, so the lane mutated every module `bin/ci-mutation-test.sh` explained its `-DexcludedTestClasses` flag with a comment placed INSIDE the backslash continuation of the `./mvnw` invocation. A backslash-newline splices the next line onto the command, so the `#` terminated it: Maven received everything up to `-DtargetTests` and nothing after, and each remaining line ran as a SEPARATE command. What the lane actually lost, every run: - `-pl parallel-consumer-core -am`, so it was not PR-scoped at all. It walked the whole reactor - ~24 minutes on core, then died in parallel-consumer-vertx with "No mutations found", a module no PR had touched. - `-DexcludedTestClasses`, the Lincheck harness exclusion the truncating comment was itself describing. Those harnesses assert a violation IS found, so using them as the suite PIT mutates against is meaningless and slow - #347 added the flag for that reason and it never arrived. - `-DjvmArgs`, `-DoutputFormats`, both timeouts, `-Dthreads` and `"$@"`. Then the orphaned `-DexcludedTestClasses=...` line executed as a command: "command not found", exit 127. Its own `2>&1 | tee "$PIT_LOG"` OVERWROTE the log the script parses for its verdict, and `STATUS=${PIPESTATUS[0]}` read that orphan pipeline rather than Maven's. So the lane's report was computed from a clobbered log with a status from the wrong command. Reproduced minimally rather than argued - a stub receiving three of its seven arguments, then `command not found`, exit 127. THE SELF-TEST COULD NOT HAVE CAUGHT IT, AND SAID SO IN ITS OWN HEADER. `bin/test-ci-mutation-test.sh` opened with "NO MAVEN RUNS HERE. Every case either exits before the build or feeds a captured PIT log through PIT_DRY_RUN_LOG" - which is a blind spot described as a feature. Every arm took the dry-run branch, so nothing exercised the branch that invokes Maven, and 24 arms stayed green through a broken invocation. Five argv arms now run the real branch against a stub `mvnw` that records what it was handed. Proven red against master's script: the 5 new arms flip, the 20 pre-existing ones pass either way - which is the evidence they were never capable of catching this. WHY THIS IS NOT A NEW SHELLCHECK RULE. ShellCheck does name it - SC2215, "This flag is used as a command name. Bad line break?" - but it is a `warning`, and `bin/check-shell-lint.sh` gates at `error`, so the gate passed it. Promoting the single code was rejected: the severity register's one-knob rule exists because a per-code list grows, and this repo has already removed that shape from SpotBugs once. The floor is the knob. What changed is the argument for raising it - `static-shell-lint-severity-tiers.md` recorded the warning tier as having no defect behind it, and now it has one, with a two-line reproduction of the gap. Defect-class sweep: `shellcheck --include=SC2215` across all 82 scripts in `bin/`, `.claude/hooks/` and `.github/` finds exactly one site - the one fixed here. No siblings. The repo's own `check-shell-sigpipe.sh` then caught a real bug in the new test code - a `grep -A1 ... | grep -q` whose EPIPE would fail the check when it FOUND the flag. Rewritten as a herestring, as that gate instructs. Verified: bin/test-ci-mutation-test.sh 25 passed 0 failed; bin/check-all.sh 15 passed, 0 failed, 0 could not run.
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 |
|
…ot parse
A simplify pass over this branch. Behaviour-preserving throughout: the self-test
still reports 25 passed / 0 failed, and the red control is unchanged - restoring
master's bin/ci-mutation-test.sh still flips exactly the five argv arms and
leaves the other twenty green.
bin/test-ci-mutation-test.sh
The new argv arms wrote their own mutations.xml with a double-quoted
status="KILLED". The subject only ever matches the single-quoted form -
grep -oE "status='(KILLED|SURVIVED|TIMED_OUT|NO_COVERAGE)'" and the PARSED sed
both require it - so that report parsed as zero evaluated mutants and every argv
arm was in fact ending on the "scored NOTHING" exit-2 verdict, not the clean run
its own comment claimed ("so the run still reaches a verdict"). It passed anyway,
because these arms judge the recorded argv and never the exit code. That is a
fixture quietly disagreeing with the thing it is a fixture for, which is the same
class of defect this branch exists to fix. They now call write_pit_report, the
helper the other twenty arms already use, at a count that agrees with the stub's
log; the run reaches exit 0. Verified by probe: exit 2 before, exit 0 after.
Extracted commit_edit for the "append a line, add, commit" fixture mutation. It
was written out four times, the fourth added by this branch. The file already
carries record_result for exactly this reason, and says so - "It was written out
three times, and the three copies had already drifted" - so the shape was already
established as one worth collapsing rather than a preference.
Dropped a dead rc=$? in assert_mvn_argv. It was captured and never read, and an
unread captured status cannot be told from a missing assertion by eye. The
comment now states positively that the exit code is deliberately not asserted
here, which is the fact the bare variable was obscuring.
docs/inflight/static-shell-lint-severity-tiers.md
The reproduce snippet disproved its own point. Its probe file had no shebang, so
shellcheck --severity=error reported SC2148 and exited 1 - not the "silent,
exit 0" the line beside it claims. A reader running it would have concluded the
error floor does catch SC2215, which is the opposite of the finding. Added the
shebang, and said why it is load-bearing so it does not get tidied away again.
Also corrected the quoted ShellCheck message, which was cut short mid-sentence
while presented as verbatim.
"A whole-tree class sweep found one other instance of this exact shape: none"
said both things at once. It means none; it now says so, and carries the sweep
command instead of a script count, per the counts rule in docs/inflight/AGENTS.md
("Name the shape and the command that yields the number"). The red-control
evidence lost its 5-and-20 tallies for the same reason and gained the two-step
recipe that reproduces them.
Considered and skipped: consolidating the five argv arms onto one shared fixture
and one subject run. Measured at ~0.3s across the five, on a suite that runs in
about 1.2s, so speed does not carry it - and every other assert helper in the
file builds a fixture per arm, so singling these out would cost more clarity
against the file's own convention than the shared setup would buy.
|
@claude review this Head is A simplify pass has since run over it. Where I would most like you to push back:
Class sweep already done, no need to repeat: Verified locally at this head: |
|
Claude finished @astubbs's task in 3m 22s —— View job Review complete
Tooling note: this session's Bash permission set could not run any script ( 1. Does the fix hold, and is the guard the right one?Yes. Confirmed by reading the post-fix invocation ( On the guard itself — the 2. Do the argv arms discriminate?Yes, and the fixture bug the simplify pass caught is real and correctly fixed. Traced it directly:
Counted the suite directly: 24 One minor, non-blocking observation: the herestring rewrite ( 3. Was rejecting the per-code lint promotion right?Agree with the call. Confirmed 4. Does the docs note say only true things now?The fix is sound: the original repro ( OverallThe fix is correct and minimal (the mutation-test script diff is exactly "hoist the comment, change nothing else"). The test suite genuinely discriminates the bug it's meant to catch, and the simplify-pass commit fixed a real second-order bug in the fixtures (parseable-status mismatch) rather than just polishing style. The severity-promotion call is well-reasoned and consistent with existing repo precedent. Nothing here blocks merge from my read; the array-based invocation is a "nice to have later" suggestion, not a requested change. |
…mpossible rather than discouraged Review of #371 accepted the fix and then made a better suggestion: the guard was a comment telling the next person not to reintroduce the bug, which is a rule somebody has to remember. Building the arguments as an array removes the mechanism instead. A `#` inside a backslash continuation is spliced onto the command, so the shell truncates there and every following line becomes a separate command - which is how this lane silently lost `-pl parallel-consumer-core -am`, mutated every module, and exited 127. Inside an ARRAY LITERAL there is no continuation to splice through, so a comment beside any element is inert. Verified rather than assumed: a probe array carrying a comment mid-literal yields all its elements, exit 0, shellcheck clean. That means the Lincheck exclusion comment can now sit exactly where it belongs - immediately above the flag it explains - which is where somebody originally put it and what broke the lane. The explanation and the flag are back together, and this time that is safe. This is the same standard applied on #344 an hour earlier: "the two states cannot coexist" beats "the gate happens to be shut". A warning comment is the second kind. It is kept, reworded to say why the array form is load-bearing, so that converting back to a continuation reads as the regression it would be. The array's first form tripped SC2054 - shellcheck reading the comma in `-DoutputFormats=XML,HTML` as an element separator rather than part of a value. Quoting the element says which it is, to the linter and the reader both; no suppression was added, since this PR's whole argument is that per-code exceptions are the thing to avoid. Also closes the one item the review left unverified: it could not execute shellcheck in its sandbox, so the note's quoted SC2215 message was flagged as unchecked. Run here, the tool emits "This flag is used as a command name. Bad line break or missing [ .. ]?" - verbatim what the note claims. Verified: bin/test-ci-mutation-test.sh 25 passed 0 failed; bin/check-all.sh 15 passed, 0 failed, 0 could not run; red control re-run against master's script - 20 pass, exactly the 5 argv arms fail - then restored to 25/25.
Adopted in Verified rather than assumed: a probe array carrying a comment mid-literal yields every element, exit 0, shellcheck clean. So the Lincheck exclusion comment now sits immediately above the flag it explains — which is where someone originally put it, and what broke the lane. The explanation and the flag are back together, and this time that is safe. The warning comment stays, reworded to say the array form is load-bearing, so converting back to a continuation reads as the regression it would be. One wrinkle worth recording: the first array form tripped
Now closed. Run locally, the tool emits verbatim: which is exactly what the note quotes. On the tooling note more generally — this is the second review in a row blocked from executing anything, so its numeric claims went unverified both times. Worth fixing at the action level. Re-run here at Thanks for the array suggestion — it converted this from a fix with a note attached into one where the failure mode no longer exists. |
…hot, and discharge the seam re-hook it inherited Takes #371, so this branch now carries the fixed PIT lane, and #337, which is what made the rest of this commit necessary. THE MERGE BROKE SEVEN TESTS, AND THAT IS THE GUARD WORKING. All three PartitionStateCommitEncodeShift894Test cases and all four PartitionStateCommitShiftCompounding894Test cases failed at their fired-assertions, each naming both the cause and the remedy: the encoder no longer calls getIncompleteOffsetsBelowHighestSucceeded(), which is the exact seam #337's RacingCommitCycleState overrode to inject its race, so every armed race landed on a dead seam. bug-torn-read-family.md predicted this and said whichever of the two landed second would owe the re-hook. #337 landed first, so this branch did it: the double now overrides the bounded getIncompleteOffsetsBelow(long), which catches BOTH entry points because the no-arg convenience method delegates through it in the base class - the same property this PR established for RemovedPartitionState, applied to the sibling double. Worth recording rather than quietly benefiting from: the dossier also said PartitionStateCommitShiftCompounding894Test armed races with no fired-assertion and could go silently green on a dead seam. That was true when written and #337 had fixed it before merging. Had it not, this signal would have been three failures instead of seven, and the other four would have passed while proving nothing. RacingCommitCycleState's javadoc also carried a claim this PR falsifies: that encodeOffsetsCompressed takes a second read of getOffsetHighestSucceeded() after the snapshot, so a completion above the high-water mark would widen the range against a stale set, and that its fixtures never reach that interleaving. The second read is what this PR removes. The caveat is kept rather than deleted - a reader of those fixtures still needs to know they establish the base/payload tear and not the widened-range one - but it now points at OffsetEncoderWidenedRangeRaceTest, which reproduces the interleaving it warned about. One content conflict, in docs/inflight/static-infer-findings.md, resolved toward master: this branch had wrapped one line in an inner `post-merge: checked-begin` to satisfy the self-reference gate, and master has since wrapped the whole surrounding paragraph in an outer block that already covers it, so the inner marker was a redundant nesting and was dropped. Verified: core unit suite 426 tests, 0 failures (417 before this merge - the 9 new tests are #337's, and all 7 regressions are cleared); bin/check-all.sh 15 passed, 0 failed, 0 could not run.
Two merges in one: #371's PIT fix, which is why this happened now, and #335's `Execution` state machine, which conflicted. THE PIT LANE. `Mutation Tests (PIT, PR-scoped)` was cancelled on this PR after half an hour. It was not PR-scoped: a comment inside the invocation's line continuation truncated it, so `-pl ... -am` never reached Maven and the lane walked the whole reactor. Master's fix builds the arguments as an array, and `bin/test-ci-mutation-test.sh` passes here, including the five argv arms that assert the scope flags survive. THE CONFLICT, and it is semantic rather than textual. #335 replaced the `maybeUserFunctionSucceeded` field with `recordVerdict(false)`, a transition on the `Execution` held in an `AtomicReference`, and calls it straight after `updateFailureHistory` - deliberately in that order, because the state write is what publishes the retry deadline. This branch had wrapped the same call site in `try`/`finally`, so a throw from `updateFailureHistory` could not leave the container half transitioned. Resolved by keeping master's structure and this branch's `finally`, because the two do not actually conflict: `finally` runs after the history write on every path that completes, so the ordering contract is untouched. It differs only on the throwing path, and there the choice is between publishing FAILED beside the previous attempt's deadline - one skipped backoff, record still makes progress - and not publishing at all, which strands the record and blocks its shard for the life of the process. The stall is strictly worse, and closing it is what this PR is for. The reasoning is written at the call site so the next reader does not take the `finally` for an oversight and remove it. Worth recording that both hazards the `finally`'s original comment named are now closed at source by this branch's own work: `computeRetryDueAt` catches the overflow and `getRetryDelayConfig` never throws. The `finally` is kept for the argument it always rested on - not today's known lines, but every line added above it later. `WorkClaimStateMachineTest` passes, including the ABA arms #335 added, and the vert.x comment that cited `maybeUserFunctionSucceeded` by name now cites the verdict instead. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ci-mutation-testing.md` measured forty consecutive runs of the PR-scoped PIT lane that scored no mutants, every skip individually correct. This records what happened the first time it had a decidable class to mutate, now that #371 has fixed the truncated invocation which was silently dropping the scope flags: the job hit `timeout-minutes: 30` and was cancelled. Scoping is not the problem, and the log says so in its own words - it named `OffsetSimultaneousEncoder` as the PR-scoped target, listed every other changed class as not decidable, and PIT reported one mutation test unit. One class, exactly as designed. The cost is the coverage pass. PIT sent 388 test classes to the minion and spent 367 seconds computing coverage before mutating anything, with a single `RunLengthEncoderTest` template invocation taking 141786 ms. Mutating that one unit then ran to the timeout without a result. So the lever is `-DtargetTests` on the test side, not the class side - and that is a different change from the ranked widening list in the same note, which is about which packages to admit. Widening scope while the coverage pass costs this much would make the timeout more likely, not less. Recorded rather than fixed, and it blocks nothing: the job is `continue-on-error: true`, so the cost is a red row and no mutation signal. It also qualifies a claim the note already carried - that a scoped run "scores mutants normally", true of the run that measured it, and not true of this one on CI. Both are observations, the difference between them is the coverage pass, and neither generalises to the other. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…r function (#205) A caller who has established diagnostic context - a trace_id, a request_id, a tenant - lost all of it the moment a record crossed into the worker pool, and again crossing into vert.x, Reactor or Mutiny. Confirmed before anything was written: PC set exactly two MDC keys of its own, `pcId` and `offset`, and `MDC.getCopyOfContextMap()`/`setContextMap()` appeared nowhere in the tree. So the logs a user's own function wrote could not be correlated back to the request that produced them. `MdcPropagation` carries it across: `capture()` on the thread that HAS the context, `enter(..)` returning an AutoCloseable scope on the thread that RUNS the work. Four boundaries - the core worker pool, the vert.x event loop, the Reactor scheduler and the Mutiny executor - which is the complete set: every `ExternalEngine` subclass plus core, checked rather than assumed. The caller's snapshot is taken in `supervisorLoop`, on the thread that calls `poll*()`. That is the only moment it is reachable: none of PC's threads exist yet and the SLF4J MDC is not inheritable. PC'S OWN KEYS WIN A COLLISION. `pcId` and `offset` are applied after the caller's map at every call site, so a caller key of the same name cannot shadow them - PC's own log lines are read by those keys, and a caller key shadowing `offset` would silently corrupt its diagnostics. A test collides on `pcId` deliberately. ON BY DEFAULT - confirmed by the maintainer at merge prep, not merely proposed - with `ParallelConsumerOptions.propagateMdc` as the kill switch. The asymmetry decides it: not propagating fails silently, for everyone, and only ever gets fixed by someone who reads a changelog; propagating fails visibly, as one extra key in a log line, and can be switched off. Setting it false restores the old behaviour exactly - leak included - so it is a true revert rather than half of one. The residual risk of a default-on capture is stated rather than buried: a REQUEST-SCOPED value in the MDC at `poll*()` time is pinned to the consumer for its life, which is the same actively-misleading-logs failure this change exists to prevent. Mitigated by logging the captured KEYS - never the values, which are the user's data - at INFO on startup, so the mistake is discoverable. That INFO line fires once per instance and structurally so: it is reachable only from `supervisorLoop`, which refuses a second `poll*()`. That trade-off - the pinning risk against the discoverability that mitigates it - is what was put to the maintainer and taken. So the risk on its own is not grounds to revisit the default; evidence of it actually biting would be. THE LEAK IS FIXED TOO, AND IT MAY MATTER MORE THAN THE PROPAGATION. Before this, whatever a user function put into the MDC stayed on the pooled thread for the next, unrelated, record - so a log line could carry another record's context entirely. The restore scope ends that. Cost is measured, not reasoned about: `ThreadMXBean.getThreadAllocatedBytes` over a million capture/enter/close cycles on logback 1.6.1 shows ZERO bytes for a caller who never touches the MDC, which is the default, and zero with propagation disabled. For a caller who does use it, a few hundred bytes per batch against the collector, two ArrayLists, a PollContextInternal and a FutureTask that `runUserFunction` already allocates per batch. One caveat recorded with it: the zero is a property of logback's MDCAdapter returning null for an empty context, and another SLF4J binding returning an empty map would allocate a small map per batch. Behaviour is correct either way. DELIBERATELY NOT COVERED: the operator chains of the Publisher or Uni the user returns. The context reaches the invocation of their function and PC's terminal signal handling; following it onto whatever schedulers their own chain hops to needs Reactor's context-propagation library and is the user's call. Every test was verified able to fail. The leak test, re-run with propagation disabled, reports four poison keys carried across records on one pooled thread - the real pre-existing defect. Each engine test, re-run with `propagateMdc(false)`, fails on its own "context must be visible" assertion, and was re-verified after the shared probe was extracted so the refactor could not quietly defuse them. They also assert WHICH thread observed - Reactor positively against `boundedElastic`, vert.x and Mutiny as "not a pc- thread" - so they cannot silently stop covering the second boundary. No existing assertion was weakened. Merging master in raised one real semantic conflict rather than a textual one. Master's `fix(reactor) #342` moved `ReactorProcessor` to the three-argument `Flux.subscribe`, because the two-argument overload has no completion callback and had `onComplete` - which retires the record - wired as the ON NEXT consumer: a Publisher emitting several elements retired its record once per element, and one completing empty retired it never. This change wraps those same callbacks in an MDC scope. They compose, and both survive: master's three-argument shape with each TERMINAL callback body inside the scope, the element consumer deliberately left outside it because it is neither the user's function nor terminal handling. Each side keeps its own pin and both pass. Static analysis arrived with that merge and fired on this change's own lines; all three findings are cleared. `observations()` returns `Queue` rather than `Collection`, which is what it always was. Two "possibly redundant method call" findings on `MDC.get` were real as observations and wrong as advice - the scope under test is what changes the context between the two reads, so caching would delete the assertion - and are resolved by routing every MDC read through one `assertMdc(key, expected)` helper rather than by suppression. That helper earns its place independently: it names the key in the failure message, which the bare assertion never did. TWO THINGS RIDE ALONG, and neither is about MDC propagation. Both are capture-only, both are docs, and both are here because they needed somewhere to land rather than a PR of their own. The first is one entry in the confluentinc#857 family ledger - a `ZOMBIE_MEMBER` chaos capture with its seed, which that file already explains as seed-dependent rather than branch-dependent, and which is recorded because the log expires and the seed is the asset. It is numbered twentieth because master took the intervening ordinals while this was open; that file's own advice is to cite the seed rather than the ordinal. A SECOND red on this branch was a different thing entirely - the BLOCKED-on-monitor discriminator, the poll thread parked acquiring the `commitCommand` monitor held by its own control thread, which is the live defect that file's capture series is hunting and whose fix is #29. It was written up here first and is deliberately NOT in this change: master recorded the same run independently, with the same seed and the same not-a-control caveat, inside a pairing analysis this branch's copy could not support. The local entry was deleted rather than kept beside it, because a fact stated twice drifts apart. What survives from that is a repair to master's paragraph, which cited this work by BRANCH name - a reference that stops resolving the moment the branch is deleted. The self-reference gate only inspects the branch it runs on, so it could not fire for whoever wrote the line and fired immediately here; it now names the PR, which is permanent. The second is `docs/mutation-lane-findings` - two notes recording what the PR-scoped mutation lane found once #371 fixed it, and a gap in the lane itself: its survivor table is written only to the job summary, which no API exposes, so recovering which mutants survived took a local re-run and a hand-written parser. The second note argues the survivor on #344's own new code is an EQUIVALENT mutant rather than a missing test - the bound samples `offsetHighestSucceeded`, and a succeeded offset is by construction absent from `incompleteOffsets`, so the two comparisons cannot differ while that invariant holds - and names the gaps that are real instead. Refs #197, #195, confluentinc#907. Closes neither: #197 is the release umbrella and this is one item in it, and #195 is the mirror of an upstream thread about whether the project is maintained, not about this finding. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Description
bin/ci-mutation-test.shexplained its-DexcludedTestClassesflag with a comment placed inside the backslash continuation of its./mvnwinvocation. A backslash-newline splices the next line onto the command, so the#terminated it: Maven received everything up to-DtargetTestsand nothing after, and each remaining line ran as a separate command.What the lane lost, on every run
-pl parallel-consumer-core -am— so it was not PR-scoped at all. It walked the whole reactor: ~24 minutes on core, then died inparallel-consumer-vertxwith "No mutations found", a module no PR had touched. That is how this was noticed.-DexcludedTestClasses— the Lincheck harness exclusion the truncating comment was itself describing. Those harnesses assert a violation is found, so mutating against them is meaningless and slow; test(core): a Lincheck lane, calibrated by refinding four real races unaided #347 added the flag for exactly that, and it never arrived.-DjvmArgs,-DoutputFormats, both timeouts,-Dthreads, and"$@".Then the orphaned
-DexcludedTestClasses=...line executed as a command — command not found, exit 127. Its own2>&1 | tee "$PIT_LOG"overwrote the log the script parses for its verdict, andSTATUS=${PIPESTATUS[0]}read that orphan pipeline rather than Maven's. So the lane's report was computed from a clobbered log, with a status from the wrong command.The fix is structural, not a warning
The arguments are now built as an array and passed as
./mvnw "${pit_args[@]}" "$@". There is no continuation to splice a#through, so a comment beside any element is inert — reintroducing this defect now requires converting back to a continuation first. Verified rather than assumed: a probe array carrying a comment mid-literal yields every element, exit 0, shellcheck clean.The first draft of this PR instead hoisted the comment and left a "keep comments out of the invocation below" warning. Review pointed out that is a rule someone has to remember, and that the argv arms only catch a truncation if the dropped flag happens to be one they assert. The array removes the mechanism instead. A pleasant consequence: the Lincheck comment can go back to sitting immediately above the flag it explains — where it originally was, and what broke the lane.
The array form initially tripped
SC2054(shellcheck reading the comma in-DoutputFormats=XML,HTMLas an element separator). The element is quoted rather than suppressed — this PR's argument is that per-code exceptions are what to avoid.The self-test could not have caught it, and said so in its own header
bin/test-ci-mutation-test.shopened with "NO MAVEN RUNS HERE" — a blind spot described as a feature. Every arm took thePIT_DRY_RUN_LOGbranch, so nothing exercised the branch that invokes Maven, and the suite stayed green through a broken invocation.Five argv arms now run the real branch against a stub
mvnwthat records what it was handed. Proven red against master's script: exactly those 5 fail, the pre-existing arms pass either way — which is the evidence they were never capable of catching this.A simplify pass then found a second-order bug in those new arms: the fixture wrote
status="KILLED"while the subject only grepsstatus='...', so the arms were silently landing on the exit-2 "scored NOTHING" verdict and passing anyway, because they assert argv and never the exit code. A fixture quietly disagreeing with the thing it tests — the same defect class this PR exists to fix. Now routed through the existingwrite_pit_report.Why this is not a new ShellCheck rule
ShellCheck names it —
SC2215, "This flag is used as a command name. Bad line break or missing [ .. ]?" (verified verbatim) — but it is awarning, andbin/check-shell-lint.shgates aterror. Promoting the single code was rejected: the severity register has a deliberate one-knob rule because per-code lists grow, and this repo already removed that shape from SpotBugs. The incident is instead recorded as the argument for raising the floor tier-wide, which stays its own work.docs/inflight/static-shell-lint-severity-tiers.mdcarries that, and two corrections the simplify pass caught in it: its reproduce snippet had no shebang, soshellcheck --severity=errorreturnedSC2148/exit 1 rather than the "silent, exit 0" printed beside it — the snippet disproved its own point — and raw counts were replaced by the commands that yield them, per the counts rule indocs/inflight/AGENTS.md.Defect-class sweep
shellcheck --include=SC2215acrossbin/,.claude/hooks/and.github/matches no sites now, and matched only the one fixed here before. No siblings.Verification
Note what this PR's own CI does not prove: it changes no Java, so the mutation lane correctly reports "nothing in scope" and exits before the invocation. The green tick is the scoping arm working, not evidence the fixed invocation runs. That evidence is behavioural and local:
bin/test-ci-mutation-test.sh— 25 passed, 0 failedbin/check-all.sh— 15 passed, 0 failed, 0 could not runChecklist
docs/inflight/static-shell-lint-severity-tiers.mdgains the SC2215 incident, the (corrected) reproduction of the severity gap, and why a per-code promotion was rejected.docs/features/— N/A — CI tooling, no user-facing behaviour.ce-simplifyandce-code-reviewlocally —ce-simplifywas run (it found the fixture/subjectstatus=mismatch and two errors in the note).ce-code-reviewwas not run locally;@claude review thiswas used on the PR instead, which is cheaper and is the only route that opens inline review threads. Its array suggestion is what produced the structural fix above.