Skip to content

feat(coverage-guard): deletions are not coverage regressions — ⛔ DECISION-1, awaiting Ruben - #480

Draft
rubenvdlinde wants to merge 1 commit into
mainfrom
S36/coverage-guard-deletion-neutral
Draft

feat(coverage-guard): deletions are not coverage regressions — ⛔ DECISION-1, awaiting Ruben#480
rubenvdlinde wants to merge 1 commit into
mainfrom
S36/coverage-guard-deletion-neutral

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

⛔ AWAITING A HUMAN DECISION — DO NOT MERGE

This is DECISION-1 on the fleet board and it belongs to Ruben. It changes
how the coverage ratchet judges every pull request in all 18 repositories.
Nothing here should land without an explicit call, and the coordinator has to
say so on the board first. I am not asking for a merge; I am asking for a
decision, with the trade-off measured rather than argued.

The finding

cgRatioDropped() is one integer cross-multiplication with no tolerance:

return (($covered * $baseStatements) < ($baseCovered * $statements));

Read plainly, it demands exactly one thing:

the code you touched must be at least as well covered as the code you did not.

For additions that is right, and it earns its keep. openconnector#1265 covered
27 of 54 new statements against a 61.86% floor, needed 34, and writing the missing
seven is what exposed a cascade that deleted nothing and reported success.

For deletions it inverts. Removing d statements of which c were covered
lowers the ratio whenever c/d exceeds the ratio of what remains — i.e. whenever
the deleted code was better tested than average. And 🔑 dead code is dead
because nothing CALLS it, not because nothing TESTED it.
gate-57
(orphaned-write-capability) exists to find precisely that code, so gate-57 and
the ratchet are in arithmetic opposition, not tension.

Such a pull request cannot satisfy the ratchet from inside its own subject.
The only moves are: delete less, add filler, or delete additional uncovered
statements until it balances. The gate can be satisfied by deleting more code
and cannot be satisfied by testing anything.

Two real pull requests are blocked by this today, both pure deletions of dead
code, neither able to comply:

PR whole-project file-scoped
opencatalogi#895 FAIL −0.08% FAIL −0.62% (112/115 vs 148/151)
launchpad#128 FAIL <0.01% FAIL −0.32% (183/254 vs 220/304)

The rule, and why it is ASYMMETRIC

Drop base-only methods (deletions) from the base side; KEEP head-only methods
(additions) on the head side.

The obvious rule is the symmetric one — "compare over statements present in both
reports"
— and it is broken. It also drops head-only statements, so a change
adding 40 new statements with 0 covered compares an empty set to an empty set
and PASSES. That is the openconnector#1265 shape. The symmetric rule would
have retired the half of this ratchet that works.

That is not a hypothetical: the mutation battery in this PR reintroduces the
symmetric rule and the suite goes red on exactly that case.

Measured — all four cases, with the old rule failing first

Every case is asserted twice: the file-scoped rule must still FAIL it (fail
first), then the new flag is exercised. A test that only ever sees the fixed
program cannot tell a fix from a no-op.

case file-scoped (today) --deletion-neutral
opencatalogi#895 — pure deletion FAIL 97.39% (112/115) vs 98.01% (148/151), −0.62% PASS — 112/115 vs 112/115
launchpad#128 — pure deletion FAIL 72.05% (183/254) vs 72.37% (220/304), −0.32% PASS — 183/254 vs 183/254
regression in surviving code 🔴 FAIL — 180/254 vs 183/254
new untested code (40 stmts, 0 covered) 🔴 FAIL — 183/294 (62.24%) vs 183/254 (72.05%)

A pure deletion becomes exactly neutral, not marginally passing. The
opencatalogi row reproduces the numbers CI printed on job 95170517430 to the
digit.

Also verified against a REAL clover artifact, not only fixtures

openregister's own lib/AppHost/Bootstrap.php, deleting the 100%-covered
register() method (38/38 statements):

file-scoped     : 99.47% (186/187) vs 99.56% (224/225)  -> FAIL, dropped by 0.09%
deletion-neutral: 99.47% (186/187) vs 99.47% (186/187)  -> OK

The intersection is by METHOD NAME, and that is NOT optional

Clover identifies a statement only by num, and a deletion shifts every line
after the cut.
On launchpad's real file, 117 of 254 surviving statements
(46%) sit at or after the deletion site
; on the fixture here it is 128 of 254
(50%). A line-number intersection therefore compares unrelated statements across
half the file and returns a confident, meaningless verdict — measured on the
fixture as 182/234 head vs 189/234 base, a regression that does not exist.

Method-name attribution reconciles exactly (183/254 over 15 methods). A control
asserting the line-number version does not reconcile is in the suite, so nobody
can quietly "simplify" it later.

📌 Known residual — stated, not left to be discovered

A RENAME reads as a delete plus an add. The old name leaves the base side,
the new name arrives on the head side and must be covered. That is arguably the
right incentive — a renamed method is new code as far as the suite is concerned —
but a pure rename of a well-covered method is not free, and an author who did
not expect it will read the failure as noise. It is documented in the script
header, in quality.yml, and printed in the failure message itself.

The hole writing the tests found — in my own first implementation

If one side cannot be attributed to methods it falls back to a single
<whole file> bucket. That bucket's key then exists on one side only, so the
asymmetric rule classified the entire base-side file as deleted, dropped it,
found nothing to compare, and printed:

OK: none of the changed PHP existed at the merge base

A file the guard could not read would have passed every possible drop.
cgCollapseFiles() closes it: when either side falls back, both sides
collapse to that file's single bucket, so the file is compared exactly as today —
deletion penalty included. There is a test for it and a mutant that removes it.

What runs this

quality-config/coverage-guard.php is copied into all 18 app repos and its
verdict blocks pull requests, and nothing in this repository has ever executed
it.
This PR adds The coverage guard's deletion rule to
quality-resolve-probe.yml and wires it into the Shared-workflow guard
required check, with:

  • fail-first assertions (both blocked PRs must still fail the old rule);
  • a mutation battery — symmetric rule, deletion penalty restored, attribution
    by ordinal, fallback removed — all four must be killed;
  • an anti-widening mutant that reworks a log string and must NOT be caught,
    so a suite that failed on any edit could not score a perfect kill rate;
  • a positive control on the harness itself: pointed at a program that just
    exit(0)s, the suite must FAIL (measured: 18 assertions red, 5 mutants
    survive). A harness that read "could not run" as "nothing to report" would
    print a clean pass forever.

Local result on this branch: subject 0 failed / controls 0 failed / 0 surviving mutants.

Blast radius, and why merging is inert until each repo opts in

quality.yml is consumed @main by all 18 repos, but the flag is probed
exactly like --changed-files was: a repo whose scripts/coverage-guard.php
predates it keeps today's behaviour and gets a ::notice:: naming the canonical
file to copy. So merging this changes nothing until a repo adopts the new guard —
which is a separate, per-repo decision.

What this does NOT do

  • It does not touch .coverage-baseline, the push-side floor, or the
    whole-project mode.
  • It does not add a tolerance band. The comparison stays exact.
  • It does not weaken the ratchet on additions — CASE 4 is the proof.
  • It does not reproduce the whole-project column of the board's table: those
    numbers depend on the rest of each repo's tree, which I did not have. The
    file-scoped and asymmetric columns are reproduced exactly.

AWAITING A HUMAN DECISION (DECISION-1). Do not merge without Ruben's call.

`cgRatioDropped()` is one integer cross-product with no tolerance, so the
ratchet demands that the code you touched be at least as well covered as the
code you did not. Right for additions; inverted for deletions, because removing
`d` statements of which `c` were covered lowers the ratio whenever c/d exceeds
the ratio of what remains — and dead code is dead because nothing CALLS it, not
because nothing TESTED it. gate-57 exists to find exactly that code, so gate-57
and the ratchet are in arithmetic opposition.

`--deletion-neutral` compares method buckets ASYMMETRICALLY: base-only methods
(deletions) leave the base side, head-only methods (additions) stay on the head
side. The symmetric version is broken — it also drops head-only statements, so
40 new statements with 0 covered pass. A mutant reintroducing it is in the
suite.

The intersection is by METHOD NAME, never line number: a deletion shifts every
line after the cut, and a line-number intersection on the launchpad fixture
reports 182/234 vs 189/234 — a confident, fabricated regression.

Reproduced, with the file-scoped rule failing first in every case:
  opencatalogi#895  PASS  112/115 vs 112/115   (was FAIL -0.62%)
  launchpad#128     PASS  183/254 vs 183/254   (was FAIL -0.32%)
  regression        FAIL  180/254 vs 183/254
  new untested      FAIL  183/294 (62.24%) vs 183/254 (72.05%)

Also verified against a real openregister clover artifact: deleting the
100%-covered Bootstrap::register() (38/38) fails the old rule at -0.09% and is
exactly neutral (186/187 vs 186/187) under the new one.

Residual, stated rather than discovered: a rename reads as delete+add, so the
new name must be covered. Named in the failure message.

The suite is wired into the Shared-workflow guard with its own positive
control, because nothing in this repository executed coverage-guard.php before.
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