ci(mutation): point PIT where survivors are real, and make the lane trustworthy - #111
Conversation
…the fix PR #110 restores the PIT lane by fixing the flake that was aborting it. Whether the lane is pointed anywhere useful is a separate question, and this records the answer rather than losing it. The short version: the full internal.* sweep has never completed, so it scores zero mutants while costing highcpu runner time on every PR - and internal.* is close to the worst possible target, because mutants to locks and loop conditions in the concurrency core hang by construction rather than dying fast, and the timing-based covering tests make a survivor unfalsifiable. Two traps written down explicitly because both are counter-intuitive: - Lowering timeoutConstant is NOT a safe speed-up. PIT counts TIMED_OUT as KILLED, so a shorter timeout reclassifies slow survivors as kills and inflates the score. - Mutating test code would be actively harmful, not merely wasteful. A mutated assertion fails its own test, so the mutant is recorded KILLED; the score climbs toward 100% while carrying no information about main code. Our targetClasses glob does match test classes by name - tests share the production package - and only pitest's mutableCodePaths default keeps them out. That is protection by unstated default, not by intent. Proposals are listed in payoff order and deliberately NOT scheduled: nightly full sweep, withHistory incremental analysis (verify free-vs-arcmutate first), retarget to offsets.* where a bug means lost records and mutants are decidable, explicit excludedGroups, explicit mutableCodePaths. Per-PR scoping to changed classes stays - that instinct was right. Docs only. No behaviour change, no workflow change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
… that never completes Corrects the payoff ordering in the entry added a moment ago. I had ranked incremental analysis second, as though cached results were being left on the table. They were not: no run has ever completed, so there was never anything to cache, and a run that dies in the coverage stage writes no useful history. It is an optimisation for 'completes, but slowly and repeatedly'. We are at 'does not complete', so retargeting to a finishable, decidable scope is the enabler and now leads the list. Second reason it would disappoint even after that: for scoped runs the dominant cost is the 332s instrumented full-suite coverage pass, paid regardless of how few classes are mutated. Caching mutant verdicts does not touch it - the lever there is narrowing what must run for coverage at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
The section is headed 'in payoff order' but 4.2 was demoted to last in the previous commit without renumbering. Renumbering would break the cross-reference from 3.3, so say it plainly instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
Dependency ReviewThe following issues were found:
License Issues.github/workflows/mutation-full-sweep.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 |
Reviewing PR #111
|
✅ SpotBugs ReportNo bugs found (new bugs only — baseline from base branch excluded). |
…part The pitest flag is trivial; carrying the file between CI runs is the actual work. Records three things worth not re-deriving: it must never be committed (regenerated every run, conflicts constantly, and its verdicts are valid only for one code state - a stale committed file is worse than none); our script does not clean, so locally it would persist and pay off immediately; and CI starts every job from an empty target/, so it needs deliberate caching. Plus the cache-key trap: per-SHA keys never hit, so you want a restore-key chain letting PR runs inherit master's history - with periodic invalidation so stale verdicts do not accumulate silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
|
Claude finished @astubbs's task in 2m 53s —— View job Reviewing PR #111
SummaryDocs-only, no behaviour change — verified against the diff (
One factual issue worth fixing before this is taken as settled analysis§4.2 / "Where the history file lives" in The new doc states:
and then spends a subsection discussing But
So on the version actually in use here, the basic history file is not free — it errors out without arcmutate too. That's not an open question needing verification, as the new doc frames it ("Flagged in the doc as needing verification first") — it was already tried and confirmed to fail in this exact repo, by an earlier PR (#69), which is why the history flags were removed from the build in the first place. This doesn't change the doc's bottom-line recommendation (§4.2 stays deprioritized either way, arguably more justified once this is corrected), but as written, a future reader skimming just the new plan doc would come away believing plain history files are usable today and only the git-aware variant costs money — which this repo already disproved. Worth reconciling the two sections (either correct §4.2's premise, or add a pointer to the PR #69 finding) before this doc is treated as the definitive record Everything else — the trap write-ups on |
🧪🔒 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 |
…ntirely This was the generalisable lesson from #108's inflight note, which had to be resolved away during the rebase onto master once #110 rewrote the entry it was updating. The specific flake is fixed and documented; the pattern behind it was about to be lost, so it moves here where it belongs. PIT refuses to run while ANY test is unstable without mutation - it needs a green baseline to attribute kills to mutants rather than noise. So one unrelated flake does not degrade the signal, it switches the whole lane off: zero mutants scored anywhere, regardless of which class flaked or whether it relates to the code being mutated. That has happened twice, both times somewhere unrelated - #101's shutdown-commit flake and #110's produce-lock flake. Three consequences worth holding onto, now written down: - The lane's green-ness has been tracking SUITE STABILITY, not mutation coverage. Weaker signal than the one we thought we had. - rerunFailingTestsCount cannot rescue it. Surefire reruns hide a flake from the unit gate, but PIT does its own coverage run and sees the raw result, so a papered-over flake still kills mutation testing. It has to be fixed. - A red mutation lane usually means "something somewhere is flaky" rather than anything about mutants - check for the "did not pass without mutation" line before investigating mutation config. Filed as section 3.0 rather than appended, because it is a property of the lane as a whole and precedes the specific complaints about scope and targets. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
|
Claude finished @astubbs's task in 2m 59s —— View job Reviewing PR #111
SummaryDocs-only, no behaviour change. I re-verified every checkable claim in the current HEAD ( The new §3.0 ("any single flaky test disables the lane entirely"), added since my last pass, is accurate and well-grounded: it matches PIT's actual behavior and cites #101 and #110 correctly as the two real occurrences. Previously flagged issue — still present, unaddressed by the four commits sinceMy prior review (at The doc still says (line 159–161):
But
So on the version actually in use here, the basic history file is not free — it errors out without arcmutate too. This isn't an open question needing verification (as framed by "Check before relying on it"/"Confirm ... rather than assuming") — it was already tried and confirmed to fail in this exact repo, by an earlier PR (#69), which is why the history flags were removed from the build in the first place. This also undercuts the entire "Where the history file lives, and why CI is the hard part" subsection (lines 163–181), which discusses Doesn't change the doc's bottom-line recommendation (§4.2 stays deprioritized either way — arguably more justified once corrected, since the "free OSS" fallback doesn't exist), but a reader who skims only the new plan doc would still come away believing plain history files are usable today and only the git-aware variant costs money. Worth reconciling before this is treated as the settled record Everything else — the new §3.0, the traps write-ups, the proposal ordering, the "keep" list, the numbers cited (42+/83+ min sweep times, 332s coverage pass, per-test timing) — reads as accurate for a parked/draft analysis doc. |
…ches PIT off The analysis in this PR argued that the mutation lane was measuring suite stability rather than mutation coverage. This acts on the parts of that which do not need a measurement first. Any single flaky test used to disable mutation testing repo-wide: PIT needs a green baseline to attribute a kill to a mutant rather than to noise, so one unrelated flake scored zero mutants everywhere (twice: #101, #110). skipFailingTests makes PIT drop that test's coverage instead of aborting. Verified both ways with a deliberate failing test: off = exit 1 and the green-suite abort, on = the run completes and scores. It has to live in the pom - the mojo parameter declares no `property`, so -DskipFailingTests is silently ignored - and parseSurefireConfig would let a <testFailureIgnore> override it from a distance. Its cost is silence: pitest logs nothing when it skips a test this way, so the job summary carries that caveat next to the no-coverage count. PIT was running up to four times per PR, three of them the same scoped computation, and the `local` copy checked out shallow - which makes the script fall back from scoped to the full sweep. One lane now: maven.yml. Unit and Integration leave the highcpu matrix for the same reason: measured as not faster than the hosted gate that already runs them, so they were a duplicate verdict and one more tick to triage. The measured fork tuning is kept in comments rather than lost with the entries. The full internal.* sweep has never completed - 42+ min on CI, 83+ min locally with minions dying on MEMORY_ERROR - so it scored zero mutants while holding a 16-core runner for its whole timeout on every push. It is now manual-only, and deliberately not nightly: scheduling a job that has never finished only moves the waste to a quieter hour. Its target is a dispatch input, so trying a decidable package is a form field rather than a commit. Every exit path now writes a job summary - scored, skipped, or died before scoring - because a green mutation tick means "nothing to mutate" at least as often as it means "all mutants killed". Two corrections to the analysis, both from reading pitest's source rather than its docs: skipFailingTests exists at all (the draft assumed the abort was unavoidable), and mutableCodePaths is NOT a pitest-maven parameter, so §4.5 was unimplementable - adding the element fails the build. Main-only is structural under Maven; crossModule=false is pinned in its place. Still outstanding and still unmeasured: the retarget off internal.* itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
changed the title
|
Claude finished @astubbs's task in 3m 28s —— View job Reviewing PR #111
SummaryThis adds the substantive implementation commit (
Previously flagged issue — still unresolved, now a third timeMy last two reviews (at
So the basic OSS history file is not free on this pitest version — it errors without arcmutate too, per PR #69's own finding. This isn't an open question needing "confirmation" as §4.2 frames it; it was already tried and failed in this exact repo. It also undercuts the "Where the history file lives, and why CI is the hard part" subsection (lines 267–285), which designs an Given this doc's own opening line now says "the cheap half is DONE... marked inline below" and Minor, non-blocking
Everything else — the CHANGELOG rewrite, |
…l finding The retarget the analysis argued for. internal.* is the concurrency core: mutants to locks, loop conditions and timeouts hang by construction rather than dying fast, which is why that sweep never completed - and its timing-based tests make a survivor unfalsifiable anyway, since "nothing asserts this" is indistinguishable from "the race didn't happen this run". The offset encoders/decoders are the opposite on both counts: deterministic tests, so a survivor is a real gap someone can act on, and the highest stakes in the codebase, since a silent bug there means lost or duplicated records. Applies to the sweep default only - in the script and as the workflow input default, so a bare `gh workflow run mutation-full-sweep` now does the right thing. Note where it does NOT apply: a PR run targets the classes that PR changed, so a PR touching internal.* still mutates internal.*. Whether the scoped lane should also refuse the hang-prone packages needs the measurement below before it is worth answering. target-tests is deliberately NOT narrowed to match. It is the one lever on the 332s instrumented coverage pass, but as a default it would trade accuracy for speed: a mutant killed only by a test outside offsets.* would be reported as no-coverage rather than killed, manufacturing survivors that aren't real. Left as a deliberate purchase rather than an inherited one. This is a config change made on the strength of an argument, not a result. Nothing has completed under the new target either - offsets.* is not obviously cheap, with RunLengthEncoderTest alone at ~140s re-run per mutant. The docs say so, and say not to quote a mutation score for this project until a sweep finishes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @astubbs's task in 1m 34s —— View job Reviewing PR #111 (latest push)
SummaryThe new commit (
Previously flagged issue — still present after this commitMy last two reviews flagged that §4.2 of New, small inconsistency introduced by this commit
But this commit's own table edit two lines below it (line 26) now says:
And §4.3's own heading was updated to Everything elseThe |
astubbs
changed the title
…adds it The docs told people to run `gh workflow run mutation-full-sweep`, which fails with "could not find any workflows named mutation-full-sweep" until this merges: workflow_dispatch requires the file on the DEFAULT BRANCH before it can be dispatched at all, even when dispatching a different ref. So a workflow introduced by a PR cannot be exercised by that PR. Also spells out what a bare invocation targets - master - since that is the question anyone reads the command and asks, and --ref is the answer for a branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @astubbs's task in 41s —— View job Reviewing PR #111 (latest push)
SummaryThe new commit (
Previously flagged issue — still unresolved (fifth pass)My prior four reviews flagged that §4.2 of Plan doc, lines 267–268:
So on the pitest version this repo pins ( Everything elseAccurate and consistent with the repo. No new issues introduced by this push. |
Review response: §4.2 contradiction — fixed, and you were right to keep raising itAddressed the finding flagged across five passes. It was not a documentation nitpick: the plan doc contradicted an already-verified finding in this repo's own ledger, which is precisely the failure the ledger exists to prevent. Rather than pick between the two documents, I re-ran it:
Thanks for the persistence on this one — the earlier passes were correct and I should have acted on the first. |
…dy said otherwise The automated reviewer flagged this contradiction on five consecutive passes and it kept being deferred. It was right, and it was not a nitpick: the plan doc asserted "the basic history file is free in OSS pitest, confirm whether the free tier covers what we want", while docs/inflight.md records the opposite as an already-verified PR #69 finding - 1.25.x dropped file-based history when #73 bumped 1.17.4 -> 1.25.8. Settled by running it rather than by choosing which document to believe: [ERROR] History has been enabled but no history plugin has been installed/activated. [ERROR] If you are using https://www.arcmutate.com remember to activate the history plugin So there is no free tier to check. History lives entirely in arcmutate, and the work item is obtaining and wiring a licence - free for OSS, but needing maintainer signup and a licence file at the repo root, which on a public repo means a committed key or a CI secret. The existing shelved plan in inflight.md covers that; §4.2 now points at it rather than duplicating it. The correction is written in place, with the reproduction, rather than the section being quietly rewritten - the same treatment as the other two corrections in this doc. inflight.md gains the re-verification date so the next reader finds agreement instead of a contradiction to arbitrate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @astubbs's task in 1m 4s —— View job Reviewing PR #111 (latest push)
SummaryThe new commit (
This resolves the contradiction cleanly: §4.2 now opens with the correction and a live reproduction, keeps the (unaffected) cache-key design discussion, and replaces "confirm which capability we want" with the actual work item (an arcmutate licence). The meta-lesson paragraph about deferred review findings is a good addition for future readers of this doc. I have no further findings on this PR. Everything across all reviewed commits ( |
|
Claude finished @astubbs's task in 1m 1s —— View job Reviewing PR #111
SummaryNo new commits have landed since my previous review — HEAD is still Recap of that resolution, re-confirmed against the current tree:
I have no further findings on this PR across all six reviewed commits ( |
Checking the PR's own claims against the diff caught a specifics error I had repeated into five files: "PIT was running up to four times per PR". The fourth copy, in pr-local-fast-feedback.yml, has had its `pull_request` trigger commented out since that runner went offline - so it only ever ran on manual dispatch. Three ran per PR: two highcpu entries plus the hosted lane, two of them the same scoped computation. The distinction matters in both directions. Rounding up overstates what was being wasted, and it obscures the actual reason that copy was worth deleting: not that it was burning runner time, but that it checks out SHALLOW, which makes the script fall back from scoped to the full sweep - so re-enabling the trigger would have silently shipped the hazard. Corrected in the plan doc, inflight, SELF_HOSTED_RUNNER, CHANGELOG, the highcpu workflow header and the PR body. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @astubbs's task in 1m 42s —— View job Reviewing PR #111 (latest push)
SummaryOne new commit since my last review, I verified the corrected count directly against the pre-removal workflow ( One leftover inconsistency this correction pass didn't reach
That sentence lists Suggested fix, consistent with the wording now used in Minor and comment-only — doesn't affect behavior — but it's the same class of cross-file numeric drift this PR has otherwise been careful to stamp out everywhere, and it sits right in the file that documents why the mutation job was deleted from this lane. Everything elseNo other changes since my last review ( |
…ecord Both files had grown a mutation entry many times the size of its neighbours, by the same mechanism: every correction this PR made was APPENDED as another clause instead of replacing the text it corrected. CHANGELOG.adoc: 412 words against a ~90-word median for the section, and the longest bullet in the file - it had become a copy of the PR description. Now 90 words: what changed, what it means, and a link to the plan doc that holds the reasoning. The neighbouring highcpu entry is trimmed too (154 -> 121). docs/inflight.md: 75 lines against a 5-line median, the largest of 75 entries in a 758-line file. That file's own header forbids exactly what I did - "This file is NOT a record... Do NOT keep a closed item by rewriting it into a FIXED/DONE narrative - making a stale entry accurate is the wrong move." Every closed item is deleted; the four genuinely-open follow-ups remain, in 13 lines, pointing at the plan doc. The duplication was not just noise. Two copies of the same claims have to be corrected together, and this PR twice showed that doesn't happen reliably - the §4.2 plan-doc/ledger contradiction, and the "four times per PR" fix that missed its sixth file. Shrinking the copy removes the sync obligation. Recorded the rule where the next writer will see it rather than relying on someone catching it again: a terseness note in AGENTS.md's Changelog section and in CHANGELOG.adoc's own header, both saying an entry highlights a change and points at the detail, with the ~90-word calibration and the instruction that corrections replace text rather than accrete onto it. README regenerated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @astubbs's task in 1m 34s —— View job Reviewing PR #111 (latest push)
SummaryOne new commit since my last review,
OverallAcross all nine reviewed commits ( |
The retarget only governed the manual sweep - the path that runs least often -
so the recurring case was left exactly as bad as before: a PR touching
internal.* still got hang-prone mutants and unfalsifiable survivors. Fixing the
rare path and calling it done was the wrong shape of fix.
The lane now intersects its changed-class list with the same decidable set that
the sweep targets (PIT_DECIDABLE_PACKAGES, default `offsets.`). Changed classes
outside it are not mutated, and are NAMED - in the log and in the job summary,
on both the skip path and the scoring path - so a run that mutated one of five
changed classes cannot read like a run that covered the PR.
The job timeout is not an argument against this. It does not bound the damage,
it converts "slow" into "cancelled with nothing scored", which is the same zero
signal the full sweep produced for months.
Measured on 12 cores before choosing the default:
one changed internal class (BrokerPollSystem)
coverage pass 271s, then mutation analysis of that SINGLE class had still
not reached the statistics stage 4 minutes later, when it was killed
one changed offsets class (OffsetSimpleSerialisation)
430s end to end, 18 mutations scored
The hosted PR lane has two cores, not twelve.
Verified all three paths against a scratch commit touching one internal and one
offsets class: mixed -> mutates offsets only and names the internal skip;
internal-only -> skips in ~1s with the reason; offsets-only -> scores. The
scratch commit is not part of this change.
Starting narrow on purpose: `offsets.` alone. `state.` is the obvious next
candidate but is bookkeeping around the same concurrency, so it earns inclusion
by measurement, not by argument.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @astubbs's task in 3m 14s —— View job Reviewing PR #111 (latest push)
SummaryOne new commit since my last review, What it does: I traced the new script logic (
Cross-checked for staleness (the failure mode this PR has been systematically hunting): grepped the repo for the now-false claim "a PR touching The measurement backing the default (12 cores: one changed Minor, non-blocking
OverallAcross all ten reviewed commits ( |
"Killed 9 (50%)" cannot be acted on - it does not say which half survived, so it reads as a grade rather than a work item. Each surviving mutant, by contrast, names a specific behaviour that can be broken without any test noticing: SURVIVED OffsetSimpleSerialisation.java:38 removed call to ObjectOutputStream::writeObject NO_COVERAGE OffsetSimpleSerialisation.java:55 removed call to SnappyOutputStream::write Adds -DoutputFormats=XML,HTML and parses mutations.xml into the job summary. No XML parser: pitest writes one <mutation> element per line, so sed does it without adding a dependency to the CI path. Anything not KILLED or TIMED_OUT is listed - PIT counts a timeout as a kill, which is also why the timeout cannot simply be lowered. Two details found by looking at the output rather than assuming it: - sort the line numbers NUMERICALLY. As text, 119 sorts above 38 and the table reads as though it were in no order at all. - strip fully-qualified paths from the descriptions. file:line already locates the mutant, so "for io/confluent/parallelconsumer/offsets/Foo::bar" is ~60 characters of noise on every row. Capped at 50 rows, and the cap is stated when it bites: a truncated list that looks complete is worse than no list. Verified end to end - 18 mutants generated, 9 killed, and the 9 that survived listed in file/line order with readable descriptions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @astubbs's task in 7m 11s —— View job Reviewing PR #111 (latest push)
SummaryOne new commit, This is the one commit in this PR's whole run where I couldn't verify the central claim by reading code alone — "pitest writes one
Everything checks out against real, freshly-generated output, not just the synthetic example in the commit message. One minor, non-blocking observation
OverallAcross all eleven reviewed commits ( |
Review response: error statuses were being listed as test gaps - fixedValid, and worth more than "non-blocking": the table's header said each is a behaviour nothing asserts, while the filter admitted anything that was not The two are now separated by meaning rather than by "not killed":
Verified by injecting a Also noted your verification method - regenerating |
The survivor table admitted anything that was not KILLED or TIMED_OUT, under a
heading reading "each is a behaviour nothing asserts". MEMORY_ERROR, RUN_ERROR
and NON_VIABLE mean the mutant could not be EVALUATED - the minion died, or the
bytecode would not load - which is an infrastructure problem, not a missing
test. Presenting one as the other is the lane misreporting itself, which is the
defect this PR exists to remove. Raised in review as non-blocking; fixed anyway,
because the decidable-package scope is meant to widen and MEMORY_ERROR is a
known risk here.
Split by meaning rather than by "not killed":
SURVIVED / NO_COVERAGE -> the survivor table; something is untested
anything else -> "Could not be evaluated - infrastructure, not test
gaps", with the count and the consequence stated:
the score is over a smaller set than it looks
An unevaluated mutant is neither killed nor surviving, and folding it into
either is how a broken run comes to look like a measured one.
Verified by injecting a MEMORY_ERROR into a real mutations.xml and re-running
the reporting path: survivors stayed at 9, the error reported separately.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @astubbs's task in 1m 12s —— View job Reviewing PR #111 (latest push)
SummaryOne new commit, I traced the new logic in
One new finding: the fix leaves a stale comment saying the opposite of what the code now does
That was true of the old filter ( This is squarely the defect class this PR spent itself hunting elsewhere (§ "a review finding that keeps being deferred is usually real," the quarantine-lane stale-comment purge, the Not a behavior bug — the code is correct — but worth closing given how deliberately this PR has been about exactly this failure mode elsewhere. OverallAcross all twelve reviewed commits ( |
Two things this PR left mis-stated. The inflight entry described the work as "retargeted but unmeasured", which buries the operational fact: automatic mutation testing is now deliberately NARROW. The only thing that runs by itself is one per-PR job over changed classes in offsets. alone; the sweep is dispatch-only. On most PRs mutation testing therefore does nothing, on purpose - and a reduction in coverage is exactly the kind of thing that must be tracked rather than left to be rediscovered. The entry now leads with that, and lists re-widening in order: measure the sweep, give it a trigger, widen the package list. The "why not nightly" reasoning was also stale. "Scheduling a job that has never finished only moves the waste to a quieter hour" is sound about internal.*, where mutants hang by construction - but it does not survive the retarget, and I had been repeating it as though it did. With offsets. the sweep is plausibly minutes. What is actually missing is one measurement, not a structural objection, and the workflow header and plan doc now say so. When it does get a trigger, prefer `push: branches: [master]` to a cron: a mutation score changes only when the code changes, so a nightly recomputes an identical answer whenever master did not move, and attributes a failure to a date instead of to a merge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @astubbs's task in 1m 39s —— View job Reviewing PR #111 (latest push)
SummaryOne new commit,
New finding: the correction didn't reach the doc's own summary table
That's the exact pre-correction reasoning §4.1 (line ~251) and Given this PR's own recurring theme (a claim fixed in one place and not swept everywhere — the §4.2 history-file contradiction, the Also still open: the doc's own Status line (flagged in an earlier review pass, still unaddressed)Line 3-4, unchanged since it was first written and not touched by any of the last ~10 commits:
But line 24 of the same table, three lines below, says the retarget itself is Everything elseNo other changes since my last review. All previously-verified claims (lane consolidation, |
Review response: both findings implemented, plus two you didn't catchBoth valid. Neither declined. 1. Summary table carried the pre-correction reasoning - FIXEDYou're right that this is the PR's own recurring failure mode landing inside the correction commit. Row rewritten close to your suggested wording:
2. The Status line contradicted its own table - FIXEDWorse than you flagged: you noted it in an earlier pass and it survived ~10 commits. It now reads:
While in there I found two more staleness bugs in the same block that no review caught: the intro still said "Two things ... the original analysis had wrong" when §4.2 made it three, and the table was missing two shipped items (the per-PR decidable allowlist, and the survivor list). Both fixed. 3. and 4. Two more instances, found by sweeping instead of point-fixingYour framing - a claim fixed in one place and not swept everywhere - is the actual defect, so this time I grepped every file for the claim rather than patching the line you pointed at. Two more:
The remaining "never completed" mentions are deliberate: they are historical statements about On the patternFour of your findings across this PR have been the same shape - §4.2 vs the ledger, the |
…as pointed out Review found the "not nightly because it never finishes" reasoning still in the plan doc's own summary table - the exact claim the previous commit corrected in §4.1 and the ledger. A reader who reads only the status table, which is what a status table is for, would take away the version that commit says it was fixing. It also re-flagged the doc's Status line, which claimed "the substantive retarget is still parked and unscheduled" while the table 20 lines below marked that same retarget Done. It had been flagged an earlier pass and survived ~10 commits. What is parked is the MEASUREMENT, not the retarget. Both fixed - and this time by grepping every file for the claim rather than patching the line that was pointed at, which found two more instances no review caught: - pr-highcpu-fast-feedback.yml still called the manual sweep "the full internal.* sweep"; it has been offsets.* since the retarget. - the PR description was the worst one. It did not merely repeat the stale reasoning, it EXTENDED it: "the same reasoning applies to the retargeted sweep below, which is why that one is not scheduled either" - exactly the inference the retarget disproves. Two more staleness bugs in the same header block, unflagged by anyone: the intro still said "Two things ... the original analysis had wrong" when §4.2 made it three, and the table was missing two shipped items (the per-PR decidable allowlist, and the survivor list). Remaining "never completed" mentions are deliberate: they are historical statements about internal.*, which are still true. The pattern is the point. Four review findings on this PR have had the same shape - a claim fixed in one place and not swept - and each time I fixed the instance shown. Sweeping first is cheaper than being told four times. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @astubbs's task in 1m 22s —— View job Reviewing PR #111 (latest push)
SummaryOne new commit,
I also independently swept the repo for every remaining "never completed" / "internal.* sweep" mention ( Two smaller items from the same commit also check out: the "Two things... the original analysis had wrong" intro is now "Three things... (§2, §3.4, §4.2)" — correctly counting the history-file correction alongside the two pre-existing ones — and the summary table's "Retargeted to OverallAcross all thirteen reviewed commits ( |
…s stop colliding (#112) The ledger had reached 677 lines and become a record of finished work. Entries told to "delete when #98 merges" outlived three merges; the rebalance-commit fix was still filed as awaiting PR after landing as #100; a static-state flake was still listed as deliberately-unfixed after #101 fixed it at source. Meanwhile #80, carrying the drain-zombie fix and the largest thing actually in flight, appeared only as a footnote about quarantine ownership. The file's own scope rule says entries vanish when work lands; it had stopped being applied. Every surviving claim is now checked against GitHub, git and the code. Obsolete entries are deleted rather than annotated: the jscpd cap is above baseline, no workflow has path filters so the docs-only inconsistency cannot happen, the ManagedPCInstance header carries its Modifications line, the CommitFailedException follow-up shipped in #108, and the stacked-PR gap is closed by a new all-branches ruleset. Upstream references now follow the convention the changelog already uses: fork numbering reaches #111, so every bare reference at or above #162 was silently upstream and read as ours. With the content correct, the file became a directory. It appeared in 26 of the last 30 master commits, so unrelated PRs conflicted on it constantly - not because they disagreed, but because their notes were adjacent, and the merge that resolves such a conflict silently drops one side (it happened between #108 and #110). Each item is now its own file, named <category>-<slug>.md; two PRs recording unrelated work touch disjoint files and cannot conflict, and finishing work is `git rm`, which never conflicts with an edit elsewhere. The prefix is the structure - `ls docs/inflight/` shows the shape of what is open without reading anything. There is deliberately no committed index: it would be edited by every PR, which is the problem the directory solves, and docs/TODO_INDEX.md is the cautionary case. This was parked on docs/inflight-as-directory with "migrating ~600 lines of existing entries" named as the reason not to do it. The audit is what made it cheap. docs/refactoring.md stays a single file - 2 of the last 30 commits touched it, so it has none of this problem. The manifest had drifted the same way, which matters more, because it is the declared source of truth that a future session trusts instead of re-deriving. Five entries disagreed with reality: bug-857 recorded no fork PR while #29 was open, #100 merged and #80 in review; fix-909 recorded no PR though #31 is open; bug-912 was in-progress when the schema's word for pushed-but-unPR'd is ready. Nothing catches this - upstream-map.py validate only checks the schema and upstream-sweep.sh only watches upstream, so "prs: []" beside an open PR passes every check we have. Hence the AGENTS.md rule to update it at every lifecycle transition, not only when starting. Four rules now live in docs/inflight/AGENTS.md so they are inherited rather than rediscovered: delete an entry in the PR that resolves it and never leave a "delete when #NN merges" marker on master; never record what gh or git can answer, which is why the open-PR table is gone; known code defects belong here even when an issue exists, because an agent scans this directory and will not read the tracker; and new guidance about how these notes are written goes into that file too. Four deferred code items moved to docs/refactoring.md, where deferred work lives: the SpotBugs thread-visibility findings, the produce-lock double release surfaced by #110, the commitAndWait message that misstates its own wait, and jacoco's single exec file under forked surefire. Also shares the agent-tooling gitignore rules that until now existed only in one checkout's .git/info/exclude, and deletes an empty duplicate-code-cross-check clone - a git init that never fetched - rather than ignoring it. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Conflict: #112 split docs/inflight.md into docs/inflight/, one file per item, because every PR edited the single file. This branch had rewritten that file, so it conflicted structurally rather than textually. Resolved master's way. The mutation note is now docs/inflight/ci-mutation-testing.md, following the new <category>-<slug> convention and the directory's rules - open items only, no FIXED/DONE narrative, no committed index. Also swept master's own notes for claims this PR invalidates, rather than only resolving my side of the conflict. ci-disabled-jobs-and-runner-load.md said "the highcpu lane runs six suites per branch on one box, including mutation sweeps ... consider moving mutation off-box. Mutation strategy is being reconsidered wholesale in #111" - #111 is this branch, and it did exactly that: the lane is down to two suites, both mutation entries are off-box, and Unit/Integration went with them. Left the runner-lost-communication symptom noted but marked for re-checking, since its stated cause is largely gone. Two references in the plan doc pointed at ledger content that the #112 audit dropped (the arcmutate shelved plan). Rather than leave a dangling pointer, the plan doc now says it is itself the record of that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @astubbs's task in 2m 7s —— View job Reviewing PR #111 (latest push)
SummaryNo new original commits from this branch since my last review — The merge itself required manual conflict resolution, though, since this branch had rewritten Most of it holds up:
New finding: the merge's own conflict resolution introduces a misleading cross-reference
That's a proposal to spin the quarantine-lane code off into its own open-source project someday — nothing about needing a paid licence, free-for-OSS tiers, or a licence key at the repo root. I grepped the whole The two "OSS" senses got conflated — arcmutate's free-for-open-source-projects licensing tier (a commercial-tool question) versus extracting quarantine-lane code as a new FOSS project (a totally different, "should this become its own thing" question). They share the acronym and nothing else. This is exactly the failure mode this PR spent itself hunting elsewhere (a claim/pointer that "reads as authoritative because of where it sits" and sends a future reader looking in the wrong place) — it just landed in the merge commit's conflict resolution instead of the PR's own commits, so none of the prior review passes could have caught it. Fix is a one-line removal or correction — either drop the "See ... for the same OSS-licence question in another form" sentence entirely (the arcmutate item is fully self-contained in the four lines above it), or point it at wherever the actual "is licence-gated OSS software worth adopting" precedent lives, if one exists. OverallNo other issues found in the merge's conflict resolution or the content it pulled in from master. All previously-fixed issues (the §4.2 history-file contradiction, the |
#113 abolished per-PR changelog entries: CHANGELOG.adoc no longer has an Unreleased section, and each release section is generated from the commit log at release time. So this PR's changelog entry is not merely conflicted, it is now forbidden - resolved by taking master's file, which drops it. README regenerated to match; both are now byte-identical to master. Two things the merge would otherwise have carried through silently. AGENTS.md: this branch had added "an entry is terse ... entries here run ~90 words ... a correction should replace text, not accrete onto it" to the Changelog section. #113 rewrote that section around a much stricter rule - one sentence, about 25 words, assembled at release time - so the surviving paragraph contradicted it (nearly 4x the word budget) and assumed a per-PR editing workflow that no longer exists. Removed; #113 expresses the same intent better. AGENTS.md now matches master exactly. docs/inflight/ci-mutation-testing.md: the previous merge's conflict resolution pointed the arcmutate item at parked-quarantine-lane-foss.md "for the same OSS-licence question in another form". Raised in review, and correct - I checked the file rather than take it on trust. That note is about extracting the quarantine lane as its own FOSS *project*; it says nothing about licence gating. Two unrelated senses of "OSS" conflated by the acronym. The pointer is removed and the arcmutate item now carries the constraint inline, since it was self-contained anyway. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review response: the bogus OSS cross-reference - fixed, plus two the merge would have carried silentlyCorrect, and I checked the file rather than take it on trust. Your framing is the part worth keeping: it landed in a merge commit's conflict resolution, which no prior pass could have seen. Conflict resolutions are new authored content and deserve the same scrutiny as a normal diff - I'd been treating them as mechanical. Two more from this merge, found by applying that lessonMaster moved again (#113) while this was open, and the auto-merge would have carried both through silently:
Neither was a conflict git could see - both were textual survivors that became false because the surrounding policy changed underneath them. |
|
Claude finished @astubbs's task in 1m 33s —— View job Reviewing PR #111 (latest push)
SummaryNo new original commits from this branch — I verified the merge's conflict resolution — the three fixes astubbs described in the review-response comment — directly against the tree rather than taking the commit message at its word:
OverallAcross all fourteen reviewed commits/merges ( |
The first sweep completed, but three places still told a future reader the runtime was unknown - including the mutation-full-sweep.yml header block, which is the first thing anyone reads before dispatching, and which was headed UNMEASURED. - docs/inflight/ci-mutation-testing.md: one bullet opened "now unblocked, at a known 22 minutes per run" and closed "Deliberately NOT wired until the runtime is known". It still is not wired, so that fact stays; the stale reason goes. - docs/plans/2026-08-03-002-mutation-testing-plan.md: "Do not quote a mutation score for this project until one has" - correct when written, now the opposite of the truth. Points at the baseline instead. - .github/workflows/mutation-full-sweep.yml: comments only - no trigger, runner, permission or step change. Also corrects the coverage pass from an estimated ~332s to the measured 311s, and drops the "until this merges, dispatch fails" note that #111 made obsolete. Also fixes the convention that made this PR's own title collide. AGENTS.md and .gitmessage both taught a trailing "(#NNN)" issue ref, but GitHub appends the PR number to exactly that slot on squash-merge, so a title ending "(#41)" merges as "(#41) (#123)" - two bare numbers with nothing to tell them apart. Both now put a fork issue at the front, as this commit does, matching Apache Kafka and our own pre-fork confluentincGH-725 history. Upstream refs stay inline and worded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The first sweep completed, but three places still told a future reader the runtime was unknown - including the mutation-full-sweep.yml header block, which is the first thing anyone reads before dispatching, and which was headed UNMEASURED. - docs/inflight/ci-mutation-testing.md: one bullet opened "now unblocked, at a known 22 minutes per run" and closed "Deliberately NOT wired until the runtime is known". It still is not wired, so that fact stays; the stale reason goes. - docs/plans/2026-08-03-002-mutation-testing-plan.md: "Do not quote a mutation score for this project until one has" - correct when written, now the opposite of the truth. Points at the baseline instead. - .github/workflows/mutation-full-sweep.yml: comments only - no trigger, runner, permission or step change. Also corrects the coverage pass from an estimated ~332s to the measured 311s, and drops the "until this merges, dispatch fails" note that #111 made obsolete. Also fixes the convention that made this PR's own title collide. AGENTS.md and .gitmessage both taught a trailing "(#NNN)" issue ref, but GitHub appends the PR number to exactly that slot on squash-merge, so a title ending "(#41)" merges as "(#41) (#123)" - two bare numbers with nothing to tell them apart. Issue refs now go at the FRONT, as this commit's subject does, matching Apache Kafka and our own pre-fork confluentincGH-725 history. That holds for upstream refs too: they just need the word, as in "fix(core) upstream confluentinc#909: subject", because a bare "confluentinc#909" autolinks to FORK issue 909 rather than the upstream one meant. Where a fork mirror exists, prefer its number (#119 mirrors upstream confluentinc#857) and leave the upstream number to the Upstream-Issue trailer, which is what tooling reads. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#123) The offsets.* sweep completes in 21m55s and scores 83% (185 mutants generated, 153 killed, 19 with no coverage, 92% test strength, 1392 tests at 7.52 per mutation). The internal.* sweep it replaced never finished, in 42+ minutes on CI or 83+ locally, so this is the first mutation number this project has ever had. Run #1 of mutation-full-sweep, against master at 5899150. Recorded in the repo because nothing else keeps it. The PIT report is a 14-day artifact and the statistics exist only in a job log - which ages out, and which a re-run silently replaces, as I proved earlier this week by re-running a job whose logs I had not saved. A baseline nobody can find is not a baseline. TWO DECISIONS THE NUMBER SETTLES - The sweep is schedulable. `push: branches: [master]` was left unwired pending a runtime, and 22 minutes on the highcpu box is affordable. It is still unwired, but that is now a cost decision rather than a blocked one. - Narrowing target-tests is NOT worth doing. Coverage is 311s of 1315s, about 24%, so the accuracy it costs - mutants killed outside offsets.* reported as no-coverage - buys little. That had been carried as a live option since #111. It also makes the marginal cost of widening PIT_DECIDABLE_PACKAGES explicit: the coverage pass is paid whatever the target, so adding state. costs only its own mutants. THREE PLACES STILL SAID THE RUNTIME WAS UNKNOWN - docs/inflight/ci-mutation-testing.md: one bullet opened "now unblocked, at a known 22 minutes per run" and closed "Deliberately NOT wired until the runtime is known". It still is not wired, so that fact stays; the stale reason goes. - docs/plans/2026-08-03-002-mutation-testing-plan.md: "Do not quote a mutation score for this project until one has" - correct when written, now the opposite of the truth. Points at the baseline instead. - .github/workflows/mutation-full-sweep.yml: the header block, which is the first thing anyone reads before dispatching, was headed UNMEASURED. Comments only - no trigger, runner, permission or step change. Also corrects the coverage pass from an estimated ~332s to the measured 311s, and drops a "until this merges, dispatch fails" note that #111 made obsolete. ISSUE REFS MOVE TO THE FRONT OF THE SUBJECT Found while citing #41 in this PR's own title. AGENTS.md and .gitmessage both taught a trailing "(#NNN)" issue ref, but GitHub appends the PR number to exactly that slot on squash-merge, so a title ending "(#41)" merges as "(#41) (#123)" - two bare numbers with nothing to tell them apart. Issue refs now go at the FRONT, as this subject does, matching Apache Kafka (KAFKA-14561: ... (#13114)) and our own pre-fork history (confluentincGH-725: ... (confluentinc#727)). That holds for upstream refs too; they just need the word, as in "fix(core) upstream confluentinc#909: subject", because a bare "confluentinc#909" autolinks to FORK issue 909 rather than the upstream one meant. Where a fork mirror exists, prefer its number (#119 mirrors upstream confluentinc#857) and leave the upstream number to the Upstream-Issue trailer, which is what tooling reads. .gitmessage's worked example was itself breaking the rule it documented; it now demonstrates it. Tracks #41, which is reopened rather than closed: the baseline answers that issue's question - PIT does complete on the self-hosted runner, in 22 minutes - but the re-widening work behind it continues there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tops flagging colours The gate is purely textual, so #111 and confluentinc#555 in a stylesheet read as unqualified issue references. Expanding every 3-digit shorthand to its 6-digit form is visually identical CSS and removes the whole class rather than today's instances. Same reason the HTML escaper now emits ' instead of the decimal ' - the hex form is what OWASP recommends anyway, so this is a small improvement rather than only an appeasement. Not using the issue-refs: N/A escape hatch: it exists for references that genuinely need no qualifier, and silencing a gate is worse than removing what trips it when removing it costs nothing. Refs #215
1 participant
Description
Started as analysis of whether the PIT lane is pointed anywhere useful. It now acts on that analysis: the plumbing that made the lane unreliable, and the retarget it was all clearing a path for.
What it does not do is claim a result. The retarget is a config change made on the strength of an argument, and no sweep has completed under the new target - so the PR ends with an experiment to run, not a mutation score.
Three things the original draft got wrong turned up while implementing it - two by reading pitest's source instead of its docs, one because the automated reviewer kept insisting. They are corrected in place in the doc rather than quietly patched, because a plan that silently self-corrects teaches nothing.
What changed
A single flaky test no longer switches mutation testing off repo-wide.
PIT needs a green baseline to attribute a kill to a mutant rather than to noise, so any test unstable without mutation aborted the whole run - zero mutants scored anywhere, regardless of which class flaked. It happened twice, both times unrelated (#101, #110), which meant the lane's green-ness had been tracking suite stability rather than mutation coverage.
skipFailingTestsmakes PIT drop the failing test's coverage instead of aborting. Verified both directions with a deliberate always-failing test (added, then removed):false1 tests did not pass without mutation ... requires a green suite, zero mutantstrueTwo traps worth knowing: it is pom-only (the
PitMojo@Parameterdeclares noproperty, so-DskipFailingTestsis silently ignored), andparseSurefireConfigwould let a<testFailureIgnore>override it from a distance. Its cost is silence - pitest logs nothing when it skips a test this way, so the job summary carries that caveat next to the no-coverage count. The flake itself is not hidden: it still reddens the Unit gate, which is where a flake belongs.One mutation lane, not three. PIT ran three times per PR - twice on highcpu, once on the hosted lane - two of them the same scoped computation. A fourth copy sat in the
locallane but was dormant: that workflow'spull_requesttrigger is commented out while its runner is offline, so it only ran on dispatch. (Being precise rather than rounding up to "four times per PR": a disabled trigger is a real difference, and the copy that isn't running isn't the one costing anything.)The dormant copy was still worth removing, because it carried a hazard the others don't: it checks out shallow, and a shallow checkout makes the script fall back from scoped to the full sweep - so re-enabling that trigger would have shipped the hazard with it. Now exactly one lane,
maven.yml, which checks out withfetch-depth: 0.Unit + Integration leave the highcpu matrix too, for the same reason rather than a mutation-specific one: measured as not actually faster than the GitHub-hosted gate that already runs them, so they were a second copy of an existing verdict and one more tick to triage per PR. The measured fork tuning (
1C, 8 broker-forks, and the-Dsurefire.forkCountvs bare-DforkCounttrap) is preserved in comments rather than lost with the entries. highcpu now carries only what needs the cores: Performance and the Chaos Pain Suite.The full sweep is manual-only. As
internal.*it had never once completed (42+ min on CI, 83+ min locally with minions dying onMEMORY_ERROR), so it scored zero mutants while holding a 16-core runner for its whole 60-minute timeout on every push.It is not left unscheduled because a sweep can never be scheduled - that reasoning was specific to
internal.*and does not survive the retarget tooffsets.*, where the sweep is plausibly minutes. It is unscheduled because nothing has measured its runtime yet: one dispatch settles it. When it is wired,push: branches: [master]beats a cron, since a mutation score changes only when the code does - a nightly recomputes an identical answer whenever master did not move, and blames a date instead of a merge.A green tick now says what kind of green it is. The old advice was "read the log, not the tick", which only helps people who already know to be suspicious. Every exit path writes a job summary instead: scored (with the run totals), skipped because nothing changed, or died before scoring - the last naming the failing tests if the green-suite abort is somehow reached. Only the final
- Statisticsblock is reported: PIT also prints a per-class tally in the identical>> Generated N Killed Mshape, and pasting those in would show several numbers that all look like the score.Three corrections to the analysis
mutableCodePathswas unimplementable, not merely unset. §4.5 proposed setting it explicitly. It is not a pitest-maven parameter at all - adding the element fails the build.MojoToReportOptionsConverterhard-codes the mutable path totarget/classes, widened only bycrossModule. So main-only is structural under Maven, not an unstated default anyone could flip.<crossModule>false</crossModule>is pinned in its place, with the reasoning attached.§4.2 contradicted this repo's own verified finding - flagged by the automated reviewer on five consecutive passes before it was acted on. The section claimed the basic history file is free in OSS pitest and asked someone to "confirm whether the free tier covers it", while the in-flight ledger already recorded the opposite as a verified PR #69 result. Settled by running it rather than choosing a document to believe:
withHistorydoes not work at all on pitest 1.25.8 - file-based history was dropped in the 1.17.4 → 1.25.8 bump (#73). So there is no free tier to check, and the real work item is obtaining and wiring an arcmutate licence (free for OSS, but needs maintainer signup and a licence file at the repo root - a committed key or CI secret on a public repo), not setting a flag. The ledger note gains the re-verification date so the next reader finds agreement rather than a contradiction to arbitrate.The meta-lesson is worth more than the fix, and is recorded in the doc: a review finding that keeps being deferred is usually a real one. This one cost a single local run to settle.
§3.4 may already be false. The script warns that pitest ignores
excluded.groups. ButparseSurefireConfigdefaults to true andSurefireConfigConverter.convertGroupsreads surefire's<excludedGroups>, which our pom sets. It hinges on whether${excluded.groups}interpolates in the rawXpp3Dom. Left unverified and marked as such rather than guessed at - a throwaway@Quarantinedunit test settles it, and §4.4 may turn out to be a comment to delete rather than work to do.Retargeted to
offsets.*The substantive change from the analysis (§4.3), and the reason the rest of it was worth doing.
internal.*is the concurrency core, which makes it close to the worst possible thing to mutate: mutants to locks, loop conditions and timeouts hang by construction rather than dying fast - that is why that sweep never completed - and the covering tests are timing-based, so a survivor is often unfalsifiable: "nothing asserts this" is indistinguishable from "the race didn't happen this run". A finding you cannot act on is not a finding.The offset encoders/decoders are the opposite on both counts. Deterministic tests, so a survivor is a real gap someone can go and close; and the highest stakes in the codebase, since a silent bug there means lost or duplicated records.
Applies in both places. It is the sweep default (script and workflow input), and the per-PR lane now intersects its changed-class list with the same decidable set (
PIT_DECIDABLE_PACKAGES, defaultoffsets.), naming in the log and summary any changed class it declined to mutate.The lane needed it as much as the sweep - a first cut of this change retargeted only the sweep, which is the path that runs least often, leaving the recurring case exactly as bad as before. The job timeout is no defence either: it converts "slow" into "cancelled with nothing scored", which is the same zero signal the sweep produced for months.
Measured before choosing the default (12 cores). One changed
internalclass (BrokerPollSystem): 271s coverage pass, then mutation analysis of that single class had still not reached the statistics stage four minutes later, when it was killed. One changedoffsetsclass: 430s end to end, 18 mutants scored. The hosted lane has two cores, not twelve.Starting narrow on purpose -
offsets.alone.state.is the obvious next candidate but is bookkeeping around the same concurrency, so it earns inclusion by measurement.The survivor list, because a score is not a finding
Killed 9 (50%)cannot be acted on - it does not say which half. Every run now ends with the mutants that lived:Each line is a behaviour nothing asserts - a work item. (That real example is worth reading: 8 of 9 are
NO_COVERAGE, so the snappy, base64 and Java-serialisation paths have no test touching them at all. The percentage alone would never have surfaced that.)Parsed from
mutations.xmlwithsed- pitest writes one element per line, so an XML parser would be a dependency for no gain. Mutants that could not be evaluated (MEMORY_ERROR,RUN_ERROR,NON_VIABLE) are counted separately rather than listed as test gaps, since they are infrastructure failures - listing them under "a behaviour nothing asserts" would be the lane misreporting itself, which is the defect this whole PR is about.target-testsis deliberately not narrowed to match, though the two pair naturally. It is the one lever on the 332s instrumented coverage pass, but as a default it trades accuracy for speed: a mutant killed only by a test outsideoffsets.*would be reported as no-coverage rather than killed, manufacturing survivors that aren't real. Left as a deliberate purchase (-f target-tests=...) rather than an inherited one.Still outstanding: the measurement
This is a config change made on the strength of an argument, not a result. Nothing has completed under the new target either -
offsets.*is not obviously cheap, withRunLengthEncoderTestalone at ~140s re-run per mutant. So:Only after this merges.
workflow_dispatchrequires the workflow file to be on the default branch before it can be dispatched at all - even when dispatching a different ref - so this PR cannot exercise the workflow it introduces (could not find any workflows named mutation-full-sweep). Worth knowing generally: any new dispatch-only workflow is unrunnable until it lands on master.If it completes, a
schedule:trigger and a history file both become real options and there is finally a mutation score worth quoting. If it doesn't, that is itself the answer on whether mutation testing can work on this codebase. The plan doc anddocs/inflight/ci-mutation-testing.mdboth say not to quote a score for this project until one has.Also in here: a stale comment that taught the wrong thing
Not planned, but the same defect class the rest of this PR is about - CI that misreports itself.
Asked whether the quarantine lane re-checks after a push, I found it does (
on: pull_request, verified against eight runs on this branch, one per pushed commit). But the comment sitting directly above that trigger still described a superseded design: "Runs NIGHTLY against master (running per-PR added ~10min of runner noise...)". It reads as authoritative because of where it sits, and it had convinced a reader that a re-push would not re-assess.The same claim had spread to
bin/check-quarantine-registry.sh,bin/check-quarantine-owners.sh, and - most instructively - to a test namedperPrWorkflowRunsTheAuditAndTheNightlyWorkflowRunsTheLane. Its assertions were correct, and its siblinglaneWorkflowDeclaresItsTriggerspinspull_request:explicitly. So the executable half of that contract had kept up while every human-readable half of it had not: a test can encode the right behaviour and still teach the wrong one, because people read the name. Renamed; no coverage change, all 11 tests in the class pass.While there, recorded the corollary that matters for reading the checks list: a green tick on that lane does not mean the quarantined tests passed - step-level
continue-on-errorputs the verdict in the step summary and sticky comment. Exactly the same trap as the mutation lane.Files
pom.xml-skipFailingTests,crossModule=false, both with their reasoningbin/ci-mutation-test.sh- job summary on every exit path,PIT_TARGET_CLASSES/PIT_TARGET_TESTS.github/workflows/mutation-full-sweep.yml- new,workflow_dispatchonly.github/workflows/pr-highcpu-fast-feedback.yml,pr-local-fast-feedback.yml- lane collapsedocs/plans/2026-08-03-002-mutation-testing-plan.md- the analysis, corrected in placedocs/inflight/ci-mutation-testing.md(new one-file-per-item ledger, per docs: audit the in-flight ledger against reality, then split it so PRs stop colliding #112) anddocs/inflight/ci-disabled-jobs-and-runner-load.md, whose "six suites including mutation sweeps" description this PR made falsedocs/SELF_HOSTED_RUNNER.md.github/workflows/quarantine-lane.yml,bin/check-quarantine-*.sh,QuarantinedAnnotationContractTest- the stale-comment purge aboveTesting
Local PIT runs against
offsets.*exercised all three summary paths (scored / skipped / died, both branches of died) and theskipFailingTestsbehaviour in both directions. One bug found in my own work and fixed: a no-matchgrepon the failure path would have aborted the script underset -euo pipefail, losing the summary and replacing PIT's exit status with grep's.-DwithHistory=truewas run against the current build to settle the §4.2 contradiction (it errors; see above).Full suite not run - no main code changed; the pom edit touches only the pitest plugin.
Checklist
QuarantinedAnnotationContractTest) because its name asserted a superseded design; its assertions are unchanged and all 11 pass. The CI behaviour claims are verified by the runs described above.CHANGELOG.adocentry: docs(changelog): compact it for readers, and generate future releases from the commit log #113 abolished per-PR entries, and release notes are now generated from the commit log.workflow_dispatchonly, so only someone with write access can start it and a fork PR can never reach the self-hosted runner through it. Net reduction in self-hosted exposure: four per-PR self-hosted jobs removed (Unit, Integration, and both Mutation entries), plus a dormant fifth in thelocallane.Note: the branch is still named
docs/mutation-testing-plan, which no longer describes it. Renaming a branch with an open PR auto-closes the PR, so it stays.🤖 Generated with Claude Code