Repository navigation
ci(mutation): give the PIT lane its own job again - a required check must not wait on it - #463
Conversation
…must not wait on it #457 batched several small per-PR jobs to get under the free plan's 20-concurrent-job cap. One of those folds was wrong: it put the PR-scoped PIT mutation lane in as the last two steps of `scan: repo`, which is a required status check. A GitHub job emits ONE check run, and that check does not report until the whole job finishes. PIT's runtime is bimodal - about 11 seconds when no in-scope class changed, and up to ~20 minutes when it actually mutates, with nothing in between. So on any PR touching mutable code the fold did two bad things at once: it withheld the fast scanners' verdict for a quarter of an hour, and it let a lane that is deliberately non-gating block the merge with its RUNTIME. `continue-on-error` protects the outcome; it does nothing about the wait. Measured on job 101622402881: every other step of `scan: repo` - both duplication scanners, dependency review and the whole-tree CVE audit - completed 3m29s into the job, and PIT alone held the required context for the remaining ~17 minutes. WHY THE HARNESS SCORED THE FOLD AS FREE. It ranked candidate folds on MEDIAN step duration. PIT's median IS about 11 seconds, because most PRs change nothing in scope - so a median made a 20-minute worst case look like a rounding error. For a step whose duration is bimodal or long-tailed, score the fold on the tail it can actually produce, not the middle. The comment above the folded block claimed "Every check above has already reported by the time this starts." That was false and is the mistake in one line: the steps had finished and the duplication tools had posted their PR comments, so the lane looked reported - but the check had not reported and structurally could not. A step completing and a check reporting are different events. What changes: - `Mutation Tests (PIT, PR-scoped)` is a job again, carrying the two steps verbatim (the exit-code case block is byte-identical). `continue-on-error: true` stays on the STEPS, never the job - a job-level flag makes the check-run conclusion success even when the step failed, which is exactly why requiring this context would be vacuous. `timeout-minutes: 20` stays on the run step: a step timeout ends the step and lets the job report, where the old lane's 30-minute JOB cap cancelled the row outright. Job cap 25 - the step's 20 plus headroom for a fetch-depth-0 checkout, a JDK download and a cold cache restore. `permissions: contents: read`, narrowing what the old standalone lane inherited: PIT reports to the log and job summary, never to a PR comment. - `scan: repo`'s job cap drops 45 -> 25. The 45 existed only to contain PIT. What remains measured 3m29s, and the one step that can legitimately run long is the Maven audit, whose standalone job held 20. - The JDK and cache-restore steps in `scan: repo` go back behind the CVE credentials guard. They were unguarded only because the folded mutation lane needed them on fork and Dependabot PRs; it carries its own copies now, so a fork PR no longer sets up a toolchain nothing uses. - The new job's `if:` is `!cancelled() && pull_request`. The `!cancelled()` on `scan:`/`static:` guards a skipped-REQUIRED-check hazard that does not apply here - this context is not required and never will be, so a skip is a grey row nobody waits on. It is used for the other reason written on `scan:`: the implicit `success()` on the `needs: prepare-deps` edge would silently delete the only mutation signal a PR gets on a transient cache failure, and the restore is prefix-fallback, so PIT degrades to a colder cache rather than a false verdict. No ruleset edit is owed. `Mutation Tests (PIT, PR-scoped)` was never a required context and must not become one; `scan: repo` stays required and still produces its context. Docs: docs/ci.md gains "A required check must not wait on a non-gating lane" and its required-checks table records the second, independent reason this lane stays out; .github/workflows/README.md no longer names the fold; the #457 write-up in docs/solutions/ gains the lesson, with the median-hides-a-bimodal-tail point as its methodological half. The other folds in that run were fine and remain fine. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RRMqsJQrGcF6ZH8xyX8AU6
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 |
[superseded - a quarantined test changed outcome] 🧪🔒 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 Updated for Superseded by a newer quarantine lane report. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #463 +/- ##
============================================
+ Coverage 82.35% 82.79% +0.44%
- Complexity 1493 1502 +9
============================================
Files 95 95
Lines 5207 5207
Branches 513 513
============================================
+ Hits 4288 4311 +23
+ Misses 724 705 -19
+ Partials 195 191 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
🟢 Throughput — OKThis branch measured about 5% faster than master, on the one test this measures. That is INSIDE this test's own run-to-run spread of about 17%, so read it as a reading and not as a result - re-running the same commit moves it by about as much.
Allowable range 🟢 ≥ 0.70 · 🟡 0.50–0.70 (about a 30% loss) · 🔴 < 0.50 (about a 50% loss) What the numbers mean, and what they cannot tell youThe one that gets misread. Why a shape and not a rate. A rate depends on which runner you drew. A shape does not: every test here processes a fixed number of records, so a runner twice as slow doubles the subject and the controls together and leaves their ratio alone. That is the whole trick, and it is why the reported rate is shown last and labelled as this machine only. Reading the comparison. By conservation, not by correction. Every test in this lane processes a fixed number of records, so within one run the ratio of one test's time to another's is invariant under machine speed — a runner twice as slow doubles both terms and leaves the ratio alone. There is no machine-index correction to be wrong, because nothing needed correcting. Per-method times, not class times. A class time is Reference is the median of 10 recent What this still cannot do. It removes machine-to-machine variance. It does not remove this test's own run-to-run variance, measured at about 30% on a single unchanged commit while its controls stayed within 5%. That is a property of the test, not of the comparison, and no arithmetic here can touch it — which is why the reference is a median and the bounds are deliberately coarse. 🟡 means look at this; only 🔴 is outside the measured spread. Runs used: abd1d39, 60138dc, a532837, e8f7beb, 29f6a0f, a75400f, 12bf414, ce6f39a, 70a88bf, 27211b9 Since the previous push: ratio 1.038 -> 1.05, share 1.655 -> 1.533, rate 65902 -> 72113 (+9.4%). One push of difference sits inside this test's measured spread - read it as movement, not as a result. Updated for |
|
@claude review this |
|
Claude finished @astubbs's task in 2m 50s —— View job Review complete
Overall: the workflow change and its stated reasoning (bimodal PIT runtime, step- vs job-level 🔴 One real regression this PR doesn't touch:
|
…w's job names, and a test says so Un-folding the PIT lane into its own job broke `bin/check-pr-analysis-surfaces.sh`, whose "SURFACES THIS SCRIPT CANNOT READ" listing filters check runs by name. #457 had dropped the `Mutation` alternative because the fold made `scan: repo` the row carrying the PIT job summary; with the job back, nothing matched `Mutation Tests (PIT, PR-scoped)` and its summary silently stopped being listed - no failure, just a row that stops appearing. That is the third instance of one class, and the second self-inflicted: the comment two lines above the regex already warned about it, citing `racerd` outliving the job that became `static: infer`. A warning that has now been walked into twice is not a control. So the class is closed with a test rather than a fourth comment. `bin/test-check-pr-analysis-surfaces.sh` reads the `scan`, `mutation` and `static` job names out of `.github/workflows/maven.yml`, extracts the pattern out of the subject, and asserts each name still matches. Neither side is restated in the test, so a rename or an un-fold flips it red with nobody editing it, and a job that disappears fails loudly instead of passing over an absent name. Control: against the pre-fix regex the new arm reports `the mutation job is named "Mutation Tests (PIT, PR-scoped)" ... which the listing pattern "static: analysis|scan: repo|CVE|Quarantine" does NOT match`, with `scan` and `static` still ok - 9 passed, 1 failed. With the alternative restored, 10 passed, 0 failed. The arm is static where every other arm is end-to-end through a faked `gh`, and deliberately: the filtering happens inside `gh --jq`, so a fake would have to reimplement jq, and a fake that knows the pattern would be testing itself. The invariant is testable without the plumbing. Only maven.yml jobs are asserted, because only their `name:` is the check-run name verbatim. The pattern's other two alternatives match no check run on a PR head today - the quarantine lane's job is named `tests` (`Quarantine Lane` is the workflow, which the check-runs API does not return) and `deps: whole-tree CVE scan` is `schedule`-only since the CVE fold - but deciding their fate is not this change's, so they are left alone. Notes that described the fold as current state are corrected rather than annotated, since they are live notes: `ci-fewer-jobs-ruleset-edits.md` (the fold is undone, the job cap is 25 on `mutation`, and the "second repair" paragraph now records the revert and names the guard), `test-untracked-ci-flakes.md` (the step-level `timeout-minutes: 20` is unchanged and still in force - only its location moved), `ci-orphaned-pit-comment-on-open-prs.md`, and `docs/refactoring.md`, whose setup-block duplication entry gained a seventh copy with the restored job. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RRMqsJQrGcF6ZH8xyX8AU6
|
Both findings fixed in The regex.
That exact ordering is deliberate - it is the string Verified live: Closing the class, not the instance. The control, which is the part worth checking. With the pre-fix regex still in the file, the new arm fails on exactly the one job and no other: With I chose the derived-from-the-workflow form over a hardcoded A finding while proving the above, reported not fixed. Two of the pattern's alternatives match no check run on a PR head today. Notes. Corrected as current state, not annotated with history:
Sweep.
|
One conflicted note, three rows, resolved row by row rather than by side. The mutation-lane row is this branch's: master still describes the PIT bound as arriving with the fold into scan: repo, and this branch is what un-folds it, so ours is the current shape. The two sighting rows are master's: it carries a third sighting of the lifecycle test that this branch never saw, and it has rewritten the registration-race row to stop keeping a running total, deriving it from the codecov query instead - which is the rule this repo applies to counts, so taking ours there would have reinstated one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RRMqsJQrGcF6ZH8xyX8AU6
🧪🔒 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 Since the previous push: Updated for |
1 participant
Fixes a CI design defect introduced by #457 (merged as
867c407d1).Description
#457 batched several small per-PR jobs to get under the free plan's
20-concurrent-job cap. One of those folds was wrong: it put the PR-scoped PIT mutation lane in as
the last two steps of
scan: repo, which is a required status check.A GitHub job emits ONE check run, and that check does not report until the whole job finishes. PIT's
runtime is bimodal - about 11 seconds when no in-scope class changed, and up to ~20 minutes
when it actually mutates, with nothing in between. So on any PR touching mutable code the fold did
two bad things at once: it withheld the fast scanners' verdict for a quarter of an hour, and it let
a lane that is deliberately non-gating block the merge with its runtime.
continue-on-errorprotects the outcome; it does nothing about the wait.
Measured on job
101622402881(verified against the API for this PR, not taken on trust): everyother step of
scan: repocompleted 3m29s into the job, and PIT alone held the required contextfor the remaining ~17 minutes.
Why the optimisation harness scored the fold as free. It ranked candidate folds on median
step duration. PIT's median really is about 11 seconds, because most PRs change nothing in scope -
so a median made a 20-minute worst case look like a rounding error. For a step whose duration is
bimodal or long-tailed, score the fold on the tail it can produce, not the middle.
The comment written above the folded block claimed "Every check above has already reported by the
time this starts." That was false, and is the mistake in one line: the steps had finished and the
duplication tools had posted their PR comments, so the lane looked reported - but the check had
not reported and structurally could not. A step completing and a check reporting are different
events.
What changes
Mutation Tests (PIT, PR-scoped)is its own job again, carrying the two steps acrossverbatim - the exit-code
caseblock is byte-identical (diffagainstHEAD~1confirms). Itgets
needs: prepare-deps, afetch-depth: 0checkout,actions/setup-java@v5on Temurin 17 andthe restore-only Maven cache step, copied from
scan: repoas they were. Nocache: maven.continue-on-error: truestays on the STEPS, never the job. A job-level flag makes thecheck-run conclusion
successeven when the step failed - which is precisely why requiring thiscontext would be vacuous. Step-level keeps the advisory semantics while still rendering red where
a reader sees it.
timeout-minutes: 20stays on the run step. Above the 19m18s scored run on record; a steptimeout ends the step and lets the job report, where the old lane's 30-minute JOB cap cancelled
the row outright.
and a cold cache restore. The job now contains nothing but that setup and PIT.
scan: repo's cap drops 45 to 25. The 45 existed only to contain PIT. What remains measured3m29s, and the only step that can legitimately run long is the Maven audit, whose standalone job
held 20; 25 is that plus ~5 minutes for checkout, JDK, cache and the three no-build scanners.
if:on the new job is!cancelled() && pull_request, and the reasoning is written intothe job comment. The
!cancelled()onscan:/static:guards a skipped-required-check hazardthat does not apply here - this context is not required and never will be, so a skip is a grey
row nobody waits on. It is used for the other reason written on
scan:: the implicitsuccess()on the
needs: prepare-depsedge would silently delete the only mutation signal a PR gets on atransient cache failure, and the restore is prefix-fallback, so PIT degrades to a colder cache
rather than to a false verdict.
permissions: contents: readon the new job. The old standalone lane declared none andinherited the workflow-wide
pull-requests: write/checks: write; PIT reports to the log andthe job summary and never to a PR comment. Narrowing, nothing widens.
scan: repogo back behind the CVE credentials guard. Theywere unguarded only because the folded mutation lane needed them on fork and Dependabot PRs. It
carries its own copies now, so a fork PR no longer sets up a toolchain nothing in that job uses.
No ruleset edit is owed
Worth stating explicitly, because a reader who watched #457's ruleset dance
will assume one is.
Mutation Tests (PIT, PR-scoped)was never a required context and must notbecome one - confirmed against
rulesets/15055005, which does not list it.scan: repostaysrequired and still produces its context. Nothing to add, nothing to remove.
Docs
docs/ci.md- new section "A required check must not wait on a non-gating lane"; therequired-checks table's PIT row keeps the vacuous-to-require reasoning and gains the second,
independent reason (its runtime must not gate either); the maven.yml description no longer
describes the lane as steps of
scan: repo..github/workflows/README.md- itsmaven.ymlline named the fold; corrected.docs/solutions/workflow-issues/batching-per-pr-jobs-under-a-concurrency-cap-2026-09-07.md-short new section with the lesson. The other folds in that run were fine and remain fine; the
write-up says so.
Verification
actionlint: 3 findings, exactly the 3 the base carries (SC2086 inexperiments.ymlandquarantine-lane.yml,if: falseinmaven.yml). None added.bash bin/check-all.sh --with-tests --strict:53 ran - 50 passed, 0 failed, 0 could not run, 3 nothing-in-scope, 5 skipped/no gate failed. The full sweep, not the gates-only shortcutthat let two failures through on ci(fewer-jobs): batch the small per-PR jobs so the sharded suites stop queueing #457.
python3+yamlon bothHEADandHEAD~1: 6 jobs to 7, added set isexactly
{mutation}, removed set empty.Mutation Tests (PIT, PR-scoped)is among the jobs whoseif:namespull_request.Checklist
docs/ci.md,.github/workflows/README.md, the ci(fewer-jobs): batch the small per-PR jobs so the sharded suites stop queueing #457 solutions write-updocs/features/- N/A - CI-internal change, no user-facing featureactionlint, a YAML job-count parse and the fullbin/check-all.sh --with-testssweep, andbin/test-ci-mutation-test.sh(unchanged) still guards the exit-code contract these steps depend ondocs/inflight/working note (pr-/branch-) started at the PR's first commit - N/A - the defect is diagnosed, fixed and recorded in its durable owners (docs/ci.mdand the existing solutions write-up) in this one commit; there is no open cross-branch state for a note to trackce-simplifyandce-code-reviewlocally - N/A - single-workflow extraction plus prose;ce-simplifyhas no code diff to work on. Please@claude review thison the PR instead, which is the route that can open the inline threads that gate the merge🤖 Generated with Claude Code
https://claude.ai/code/session_01RRMqsJQrGcF6ZH8xyX8AU6