fix(ci): a security failure DELETED the test tier (#194); the coverage threshold never gated (#189) - #253
Merged
rubenvdlinde merged 4 commits intoAug 8, 2026
Conversation
…e threshold never gated (#189) #194 — gating the test tier on the security tier silently deletes all test evidence. phpunit, newman, playwright and journeydoc-capture each carried `&& needs.security.result != 'failure'`. Because composer audit queries the LIVE Packagist feed, CVE-2026-67434 against squizlabs/php_codesniffer — a code formatter that never runs in production — turned the whole fleet's test tier into 'skipped' on 2026-08-06 with no commit anywhere. A skipped job is a grey tick, not a red X, so nothing counted it and a review filed a false 'fully green' report. Option E of the issue: decouple AND render loudly. * the four test jobs no longer read needs.security.result; 'needs:' is kept for ordering only, suppressed by the existing !cancelled(). * security still blocks the merge unchanged, via the required 'quality / Quality Report' check. Nothing is weakened at the merge gate. * Quality Report gains a third gate: an ENABLED test job in state 'skipped' is the ABSENCE OF A VERDICT — it hard-fails and says so in words, and distinguishes 'tests passed, security failed' from 'tests never ran'. * scripts/assert-no-producer-deletes-a-verdict.py makes it an invariant, and closes the direction #229 left open: #229 asserted every job can REACH the required check, this asserts no job can be DELETED before it gets there. Proved live, not argued: run 31259774225 on fixture/issue-194-evidence-deletion. Identical failing test job under the two conditions — gated on security -> skipped (evidence gone, old tally GREEN) decoupled -> failure (verdict exists) new invariant -> failure ('TEST TIER NOT EXECUTED — NO VERDICT EXISTS') #189 — playwright-coverage-threshold has never gated. Three defects, all fixed: 1. below-threshold emitted ::warning:: and exited 0, so the knob was decorative. It now ::error::s and exits non-zero. 2. the metric was count(test() calls) / count(scenario headings) — two independent totals never compared to each other, which ten unrelated tests raised as much as covering ten scenarios did, and which could exceed 100% while covering nothing. Replaced with real per-scenario matching against @e2e references in gate-19's dialect. The old ratio is kept and reported as testsPerScenarioPercent, never gated on. 3. zero scenarios scored 100%. Zero enforceable scenarios is now NOT MEASURABLE and fails — a measurement that could not be taken is not a pass. Default threshold 75 -> 0 to bound the blast radius: measured across all 31 fleet callers, exactly one repo (pipelinq) enables this, and it sets its own value. Gating is now opt-in by setting a number, which is what a threshold input should mean. scripts/test-spec-coverage-gate.py extracts the shipped program out of quality.yml and runs it against fixtures — including the one #189 says cannot currently exist: coverage below threshold turning the job red. 24 assertions; the positive control neuters the gate to warning-only and 5 of them flip to FAIL, so its clean pass is a verdict. Both new scripts are wired into quality-resolve-probe.yml. A checker with no callers is not a checker.
…-19 liveness gap Peer review asked for a mutation standard rather than a single positive control. Seven mutants, each reintroducing one specific defect, plus an anti-widening control that reworks a log string nothing asserts on and which the suite must NOT notice — without it a suite that failed on any edit would score a perfect kill rate while being worthless. It earned its keep on the first run: 'exclude-directive-read-as-reference' SURVIVED. The fixture used '@e2e exclude <slug>' space-separated, and under that form the guarded and unguarded regexes are indistinguishable — both capture 'exclude', which contains no '#' or '::' and so resolves to no slug. The assertion had been passing while proving nothing. The guard is load-bearing only for '@e2e exclude::<slug>' and '@e2e exclude#<slug>', where the unguarded regex marks the named scenario COVERED; the fixture now uses those forms and the mutant dies. 7 of 7 killed, control survives. An unapplied mutant is reported as SKIPPED (wiring) and FAILS the run rather than counting as a kill — an anchor that has drifted means the battery measures less than it claims. Also documents a real limitation rather than leaving it to be discovered: #249 rewrote gate-19 to parse test files with a real JS parser, so it will not count an @e2e reference inside a describe.skip or an empty test body. This step reads the annotation as text and will, so its number is an UPPER BOUND on real coverage. A passing threshold here is not evidence that gate-19 would pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #194. Closes #189.
Both defects live in
.github/workflows/quality.yml, so they land together.#194 — gating the test tier on the security tier deletes all test evidence
Cause
Four jobs carried the same clause:
phpunit,newman,playwright,journeydoc-capture. (The issue also citessbom;mainhad already dropped it there. Verified — 4 sites, not 5.)composer auditqueries the live Packagist advisory feed on every run. So on 2026-08-06, whenCVE-2026-67434began being served againstsquizlabs/php_codesniffer, the entire fleet's test tier turnedskippedsimultaneously, with no commit in any repo. The dependency is a code formatter that never executes in production.The damage was not the skip. It was that a skipped job renders as a grey tick, not a red X —
skippedis notfailure, socontains(needs.*.result, 'failure')reads straight past it. The absence of a verdict was indistinguishable from a good one, and a review filed a "fully green" report over 16 repos with no test coverage.Fix — option E (A + D)
(A) Decouple. The four jobs no longer read
needs.security.result.needs:is kept for ordering only; the existing!cancelled()already suppresses the implicitsuccess(), so they reach their own verdict regardless.Security still blocks the merge, unchanged.
securityis inQuality Report'sneeds:, andquality / Quality Reportis a required context onmainandbetain all 25 fleet repos. Nothing is weakened at the merge gate. The cost is a few CI minutes on a PR that could not have merged anyway.(D) Loud rendering. Two additions to the Quality Report:
🚨 **NO VERDICT — enabled but never ran**, not⏭️— pairingneeds.<job>.resultwith the input that switches the job on, so the 20-odd repos that run no E2E stay quiet;TEST TIER NOT EXECUTED — NO VERDICT EXISTS, names the jobs, and states in words that this is not a test failure but the absence of a test result. When the tests did run and only security failed, it emits a::notice::saying exactly that — the distinction the 2026-08-06 reports could not make.It reuses the superseded-run discriminator from the cancelled gate, so
cancel-in-progressdoes not produce a wall of red.Evidence — a LIVE Actions run, not a simulation
https://github.com/ConductionNL/.github/actions/runs/31259774225 (
fixture/issue-194-evidence-deletion, workflow kept in this PR astest-tier-gating-fixture.yml).A failing
securityjob and two byte-identical failing test jobs differing only in theirif::if:security!cancelled() && needs.security.result != 'failure'skipped← evidence deleted!cancelled()failure← verdict existscontains(needs.*.result,'failure')success← green over a deleted test tierfailureVerbatim from the run:
The #229 audit, both directions
#229fixed direction 1 (frontend-testsmissing fromQuality Report'sneeds:). Full enumeration of all 17 jobs:needs:if:needs:always()needs:;#229holds.success()trap.scripts/assert-no-producer-deletes-a-verdict.pymakes it permanent, checking both shapes (explicitneeds.*.resultgate;needs:with no status function). Run againstorigin/mainit correctly reports all four. Positive control included.#189 —
playwright-coverage-thresholdhas never gatedCause, and the decision
Decision: make it gate, and stop lying about what it measures. Removing it was the alternative; the metric is worth having once it means something, and gate-19 already supplies the annotation dialect to make it real.
Three defects, all fixed:
::warning::+ no non-zero exit. Reproduced against the real pipelinq checkout with the verbatim old program:838 scenarios / 232 tests → 28%, exit 0. Now::error::+process.exit(1).tests.length / scenarios.length— two independent totals never compared to each other. Replaced with real per-scenario matching: a scenario is covered when a Playwright file carries an@e2ereference resolving to its slug, using gate-19's dialect (check_e2e_coverage.py) so a repo annotates once and both numbers move together.@e2e exclude <reason>leaves the denominator. The old ratio survives astestsPerScenarioPercent— reported, never gated on.Incidental finding: the old regex
/^###?\s+(S\d+|Scenario[:\s]|REQ-)/cannot match#### Scenario:(four hashes), which is 2659 of pipelinq's headings — the dominant scenario form in the fleet was never counted at all.measurable: false,coverage: null,::error::NOT MEASURABLE, exit 1 — a failure to measure, not a pass — and it fails even at threshold 0.gate-19 is not invoked from this job on purpose: it has no hydra-gates checkout, and adding one would make E2E depend on a second repository to report a number. gate-19 stays the diff-scoped PR enforcer; this is the whole-repo floor.
Proof the gate CAN fail
scripts/test-spec-coverage-gate.pyextracts the Node program out of this workflow's heredoc and executes it — the shipped text, not a transcription. 24 assertions, all passing, including:Positive control: neuter the gate back to warning-only and 5 assertions flip to FAIL. A suite that stayed green there would not be measuring enforcement.
Both new scripts are wired into
quality-resolve-probe.yml, so they run on every PR intomain. A checker with no callers is not a checker.Blast radius — stated, not discovered
Measured 2026-08-08 across all 31 callers of
quality.ymlin the org (everycode-quality.yml, not code search):enable-playwright-coveragetruefalsepipelinq is the entire blast radius. Its honest number is 22% (2659 scenarios, 2140
@e2e-excluded with reasons, 519 enforceable, 116 covered). At 75 it would go red on merge of this PR with no code change.So ConductionNL/pipelinq#732 lands first, ratcheting 75 → 20 — the floor it can actually hold, with slack. Nothing turns red on this merge.
The default is also changed 75 → 0, so a repo that switches measurement on gets the number without being turned red by a floor it never chose. Gating is opt-in by setting a value.
betabranch also carries75and is human-gated, so it is untouched. It needs the same ratchet on the nextdevelopment → betasync or its next beta PR goes red.Files
.github/workflows/quality.yml— both fixes.github/workflows/quality-resolve-probe.yml— wires the two new checkers in.github/workflows/test-tier-gating-fixture.yml— new; the quality.yml: gating the test tier on the security tier silently deletes all test evidence (CVE-2026-67434 disabled PHPUnit/Newman/E2E fleet-wide today) #194 live proof, runs only onfixture/**scripts/assert-no-producer-deletes-a-verdict.py— newscripts/test-spec-coverage-gate.py— new🤖 Generated with Claude Code
Round 2 — mutation testing, and a limitation stated up front
The mutation battery found a real hole in my own test
A single positive control was not enough.
--mutation-batteryreintroduces seven specific defects one at a time and requires the suite to notice each, plus an anti-widening control that reworks a log string nothing asserts on and which the suite must not notice — without it, a suite that failed on any edit at all would score a perfect kill rate while being worthless.It earned its keep immediately. On the first run,
exclude-directive-read-as-referenceSURVIVED:The fixture used
@e2e exclude <slug>, space-separated. Under that form the guarded and unguarded regexes are indistinguishable — both captureexclude, which contains no#or::and so resolves to no slug. Measured:@e2e exclude widget-renders — server side[][]— equivalent@e2e exclude::widget-renders-on-the-dashboard[]["widget-renders-on-the-dashboard"]— differs@e2e exclude#widget-shows-an-empty-state[]["widget-shows-an-empty-state"]— differsSo that assertion had been passing while proving nothing. The guard is load-bearing only for the separator forms, where the unguarded regex silently marks the named scenario covered — the exact opposite of what an exclusion says. Fixture fixed; mutant now dies.
Final: 7 of 7 killed, 0 survived, control correctly survives, 25 assertions at baseline.
An unapplied mutant reports
SKIPPED (wiring)and fails the run rather than counting as a kill — a drifted anchor means the battery measures less than it claims.Known gap, stated rather than discovered later
#249rewrote gate-19 to read test files with a real JS parser, so it will not count an@e2ereference that sits inside adescribe.skipor an empty test body. This step reads the annotation as text and will.So this number is an UPPER BOUND on real coverage, and the two figures can legitimately disagree in that one direction. Closing the gap means shipping a JS parser into the heredoc; gate-19 already catches the case on every PR that touches a spec. A passing threshold here is not evidence that gate-19 would pass. This is written into the workflow comment, not just here.
Signalling convention
Matches gate-19 post-
#249: the exit code is a status only (0/1), never a count — counts go to stdout. A failure to measure is reported distinctly from a measured pass, and an unexpected crash exits non-zero rather than falling through to green.Housekeeping
main(#217,#246,#249,#248,#250,#255— allhydra-gates/-only, zero overlap with this branch's five files; verified each file's blob hash is unchanged by the merges).run:block size for the new heredoc step: 9,026 bytes against the 16,384 limit (assert-run-steps-resolvable.pygreen) — nowhere near the#161resolution-breaking class.