Skip to content

fix(quality): measure the coverage ratchet floor at the merge base, and delete the staleness job - #181

Merged
rubenvdlinde merged 1 commit into
mainfrom
fix/coverage-ratchet-merge-base
Aug 6, 2026
Merged

fix(quality): measure the coverage ratchet floor at the merge base, and delete the staleness job#181
rubenvdlinde merged 1 commit into
mainfrom
fix/coverage-ratchet-merge-base

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Two jobs assert the same constant in opposite directions

.coverage-baseline is read two ways at once:

  • the phpunit guard fails when coverage is below it;
  • Coverage Baseline Check (push, main/development) fails when the file is stale — that is, when coverage is above it.

Together they demand exact equality with a checked-in constant. The only way to restore that equality is to commit, in a pull request, the number the tree will measure after that pull request lands — while the base branch keeps moving and the author's own diff changes the answer.

Measured on openregister: an author committed 58.93; development advanced 16030 → 16038 tests while the PR sat; the merge result measured 58.88; the guard reported "dropped by 0.05%". While the follow-up pinned 58.88, development moved two further commits. There is no value a PR author can commit that is guaranteed correct when it lands.

It is not one repo. On 2026-08-06 six of sixteen apps were red on this job, every one of them for coverage being higher than recorded:

repo committed CI measured delta
openregister 58.87 58.88 +0.01
docudesk 61.39 61.41 +0.02
procest 29.60 29.69 +0.09
openbuild 57.39 58.83 +1.44
larpingapp 67.68 71.73 +4.05
doriath 55.78 55.79 +0.01

Not one failure in current history is a coverage drop. Three of the six are one or two hundredths of a percent — a single extra covered statement reddening a mainline.

The staleness rule outlived its enforcement mechanism

While this job auto-committed the recomputed value, "the file equals the measurement" was an invariant a machine maintained. That git push was removed when branch rulesets rejected it (#61) — but the invariant it maintained was left in place, and became a standing human obligation against a moving target. That is the whole bug.

The fix

The floor is measured, not typed. On a pull request the phpunit job now measures the merge base — same PHP, same xdebug driver, same suite, same job — and compares against that. The merge base is immutable for a given head, so it cannot go stale; a rebase re-measures both sides together; and because both numbers come from one driver, the xdebug/pcov statement-counting difference that can invalidate a locally measured constant cancels instead of being baked in.

--against takes precedence over the committed value rather than combining with it. max(committed, measured) would preserve the openregister failure exactly, so a hand-typed number can never fail a PR when a real measurement exists.

Coverage Baseline Check is deleted. A push to development is the result of an already-gated merge: there is nothing left to reject, and failing it only produces a mainline no pull request can turn green. Its anti-decay purpose is served better by the merge-base comparison, which tracks the base branch exactly and cannot go stale at all. It is deleted rather than downgraded to a warning, because a job that can no longer fail is the "renders like a pass" shape this repo has been removing all week.

.coverage-baseline survives as a conservative fail-safe floor, still enforced on pushes, still protected from being lowered by baseline-protection. It will drift below actual coverage. That is intended — a floor too low is merely conservative and is never the binding constraint on a PR.

The measurement is deterministic — checked, because an exact comparison depends on it

A concurrent report suggested coverage was non-deterministic in openbuild (58.83 / 57.43 / 57.43) and larpingapp (71.73 / 70.04 / 70.04). It is not. Those three samples come from three different commits.

Positive control — two independent runs of the same commit:

repo commit run statements
decidesk f4ed5611 31050137982 8687/15065
decidesk f4ed5611 31050142139 8687/15065
decidesk 3731e11f 31046395985 8687/15065
decidesk 3731e11f 31046397308 8687/15065

And each value is stable across many commits until the code actually changes — openbuild reads 8018/13971 across eight consecutive commits, then 8034/13988, then 8229/13987. The statement total moves at each step, which is the signature of the code changing, not of a flaky measurement; a flaky one would move coveredstatements while statements held still. The 8034 → 8229 step is a commit that added a 349-line SeedHelloWorldFixtureTest.php.

So there is one defect here, not two, and the merge-base comparison is its complete fix.

It still catches a real drop — proof

Verified against real CI clover artifacts from this fleet (openbuild runs 31016006546 = 8018/13971 = 57.39% and 31050116167 = 8229/13987 = 58.83%):

case result
real 1.44% drop exit 1 — FAIL
improvement exit 0
unchanged exit 0
PR adds 20 untested statements exit 1 — FAIL
PR adds 20 tested statements exit 0
one covered statement lost (8229 → 8228) exit 1 — FAIL
empty merge-base report exit 2 — refuses, does not read as 0%
missing merge-base report exit 2 — refuses

The load-bearing comparison: the old guard passes that same real 1.44% drop (exit 0, "Coverage unchanged"), because openbuild's floor had decayed to exactly 57.39. This change makes the ratchet stricter, not laxer.

Two holes found in the first draft and closed before shipping: a one-statement regression rounded away at two decimals (comparison is now an exact integer cross-product of ratios), and an empty clover report read as 0% (now a hard error — as the merge-base side it would set the floor to zero and pass every drop).

Rollout order and the capability probe

scripts/coverage-guard.php is byte-identical in all 16 apps and was updated in every one of them before this PR merges. The workflow probes --capabilities and fails loudly if a repo's script predates --against: an older copy accepts the flag and silently ignores it, which would demote the ratchet to the constant check while still reporting success.

fetch-depth: 0 is set unconditionally on the app checkout. Keying it off the input would need inputs.x && 0 || 1, and 0 is falsy in GitHub expressions — the || 1 branch wins and the depth silently stays 1.

Largest run: step in the file is unchanged at 8.5 KB, well under the ~19 KB that made this workflow unresolvable in #161/#168.

No waiver, no continue-on-error, no widened threshold.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant