Skip to content

fix(quality): stop the coverage-baseline gate reporting success while doing nothing (#61) - #138

Merged
rubenvdlinde merged 1 commit into
mainfrom
fix/61-honest-baseline-gate
Aug 3, 2026
Merged

fix(quality): stop the coverage-baseline gate reporting success while doing nothing (#61)#138
rubenvdlinde merged 1 commit into
mainfrom
fix/61-honest-baseline-gate

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Closes the still-live half of #61.

The bug — a gate that reports success while doing nothing

update-baseline ended in:

git push || echo "::warning::Could not push the updated coverage baseline ..."

On any repo whose development/main carries a ruleset ("Changes must be made through a pull request") that push is rejected, and the || swallows it. The job then reports success.

Reproduced on a live run — openregister run 30851583987, job 91814324437, conclusion success:

remote: error: GH013: Repository rule violations found for refs/heads/development.
- Changes must be made through a pull request.
! [remote rejected] development -> development (push declined due to repository rule violations)
##[warning]Could not push the updated coverage baseline ...

The baseline never moved, and nothing anywhere went red. A swallowed failure is worse than no job at all: it is indistinguishable from the work having been done.

Note: the features-extract half of #61 was already fixed (read-only + hard-fail + artifact). This PR finishes the job for the two sites that still swallowed a push.

Which fix, and why

Chosen: make the gate honest and read-only — mirroring the design already shipped for features-extract — rather than granting the bot a ruleset bypass.

I have admin on both repos, so adding github-actions[bot] as a bypass actor was available. I deliberately did not:

  • it re-introduces exactly the hazard the features-extract redesign removed — a quality pipeline mutating the branch it is judging, via a [skip ci] commit that runs no checks;
  • a bypass actor widens what every workflow in that repo may push, not just this job;
  • it does not generalise: each repo that later enables enable-coverage-guard would need the same manual ruleset surgery, and forgetting it silently restores the swallowed-push behaviour.

Changes

job before after
update-baseline"Coverage Baseline Check" contents: write, commit + git push || echo ::warning:: contents: read, no commit/push. Recomputes, hard-fails on drift, attaches the recomputed value as the coverage-baseline artifact
baseline-protection banned every manual .coverage-baseline change enforces the property that matters: the ratchet may be raised, never lowered; fails closed on an unparseable value
journeydoc-capture refused push → ::warning:: + changed=0 refused push fails the job; screenshots still uploaded as an artifact

Why baseline-protection had to change too

Making the push job fail loudly on its own would have produced an unclosable gate: baseline-protection forbade humans editing .coverage-baseline, and the bot could not push it — so the baseline would be unchangeable by anyone, and every push to development would be permanently red with no way to close it.

The blanket ban was only ever coherent while the bot auto-committed. The anti-gaming property actually worth enforcing is the baseline never goes downcoverage-guard.php only writes it when current > baseline. So a PR may now raise it (that is how the push-side gate is closed) but never lower it.

Verification

The new baseline-protection script was extracted verbatim from the YAML and run against real git repos. Every failing case was shown to actually fail:

case expected got
no .coverage-baseline change pass pass
raised 55.54 → 60.00 pass pass
lowered 55.54 → 50.00 fail fail
equal 55.54 → 55.54 pass pass
malformed value abc fail fail
new baseline (absent on base) pass pass
lowered by 0.01 fail fail
raised int 9 → 10 pass pass
lowered int 10 → 9 fail fail

Differential against the logic shipping today, confirming this is not a no-op: old logic rejects raised 55.54 → 60.00 (the deadlock); new logic accepts it while still rejecting every lowering.

An earlier version of that harness was itself broken (git init -b is unsupported on git 2.25.1), so every case fell through to "no baseline change" and falsely passed. Caught by comparing expected against actual rather than reading the passes.

What this change cannot break

Scanned 22 caller repos.

  • enable-coverage-guard defaults false; openregister is its only caller. No other repo can reach either baseline job at all.
  • enable-journeydoc-capture defaults false and no caller in the fleet sets it — 0 executions. That edit cannot execute today.
  • Nothing needs: update-baseline (verified by parsing the job graph), so it cannot cascade into Quality Report — the only quality context any ruleset requires (openregister Beta/Main).
  • baseline-protection does feed Quality Report, but the new script exit 0s early when the PR does not touch .coverage-baseline — every PR today. On the only path it can reach it is strictly more permissive than before.
  • Renaming the job display name is safe: no ruleset references Update Coverage Baseline, and the job runs on push only, so it can never be a required PR status check. The job key is unchanged.
  • Untouched, and not dependent on any changed job: PHP quality, frontend build, eslint/stylelint, security, license, SBOM, phpunit, newman, playwright, hydra-gates, features-check/features-extract, branch-protection, releases, docs deploy.

YAML parses; job count unchanged at 18.

… doing nothing

`update-baseline` ended in `git push || echo "::warning::..."`. On any repo whose
development/main carries a ruleset ("Changes must be made through a pull request")
that push is rejected, and the `||` swallowed it — so the job reported SUCCESS
while the baseline never moved. Reproduced on openregister run 30851583987, job
91814324437 (conclusion: success):

    remote: error: GH013: Repository rule violations found for refs/heads/development.
    - Changes must be made through a pull request.
    ! [remote rejected] development -> development (push declined ...)

A swallowed failure is worse than no job at all: it is indistinguishable from the
work having been done.

Chosen fix: make the gate honest and READ-ONLY, mirroring the design already
shipped for features-extract, rather than granting the bot a ruleset bypass.
Letting CI push to the branch it is judging is the hazard that design deliberately
removed, and a bypass actor would widen what every workflow in the repo can push.

- update-baseline -> read-only (`contents: read`), no commit, no push. Recomputes
  the baseline, hard-fails on drift, and attaches the recomputed value as the
  `coverage-baseline` artifact. Renamed "Coverage Baseline Check" so the name
  stops promising a write.
- baseline-protection -> enforce the property that actually matters (the ratchet
  never goes DOWN) instead of banning every manual change. The blanket ban was
  coherent only while the bot auto-committed; with a read-only push job it left
  the baseline unchangeable by bot or human alike — an unclosable gate. A PR may
  now raise the baseline (this is how the push-side gate is closed) but never
  lower it. Fails closed on an unparseable value.
- journeydoc-capture -> a refused push now fails the job instead of setting
  changed=0, which had made a rejected push look identical to "nothing changed".
  Screenshots are still uploaded as an artifact, so a refusal stays recoverable.

Blast radius: `enable-coverage-guard` defaults false and openregister is its only
caller across 22 repos scanned; `enable-journeydoc-capture` has no caller at all.
Nothing `needs:` update-baseline, so it cannot cascade into Quality Report — the
only quality context any ruleset requires.

Refs #61
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