ci(coverage): fail a PR when this repo's own parse-coverage report goes bad (#963)

[CaptainMittens]

A CI gate that fails a pull request when this repository's own parse-coverage report stops being useful advice (#963). It indexes the merge base and the branch with the binary just built, and fails only on a finding the branch added.

The precondition from #1985 is now met

You wrote:

the gate is accepted in principle as a required check once merge-base comparison is included, so unrelated pull requests do not fail merely because main moved the baseline

and:

with a constant ceiling, a legitimate language-coverage improvement reads as a gate failure on an unrelated PR, and the person who hits it has no way to tell that from a real regression

All four checks are now differential. The gate resolves the base commit, checks it out into a temporary worktree, indexes both trees with the same binary, and compares:

#	Check	Fails when
1	whole-file parse failure (parse_unusable)	the path is flagged on this branch and not at the merge base
2	a +N clipping marker on a range list	the marker is new at head
3	one range over 25% of a file of 200+ lines	the file was within the share at the merge base
4	parse_partial_count	it is above the merge base's count

scripts/ci/parse-partial-baseline.txt stops being a gate. The script still prints the recorded number so a reader can see the drift, and says in the output that nothing fails on it. Nobody has to raise that number again — which is what #1972 was, and what #1824 would have been. Blazor .razor files map to C#, their markup lands in ERROR regions by design, and parse_partial_count rises by roughly the repository's .razor count. Under the old gate that coverage improvement read as a failure on an unrelated branch.

Every run prints both sides, so a failure reads as "the base reports X, this branch reports Y" rather than as a bare number:

==> base 5fbab7bb7332bd06aaa880653ddfb2696e648f90: parse_partial=59 unusable=0
==> head:            parse_partial=59 unusable=0
==> allowlisted=1 recorded=59 (record only, not enforced)
PASS: this branch did not make the parse-coverage report worse than the merge base

What this gate cannot see, stated plainly

Both trees are indexed with the same binary, so the comparison isolates what the tree changed. A pull request that changes the extractor itself moves the base side and the head side together, and this gate will not fail on it.

Catching that needs the base commit's own binary, which means a second full build. In job 99872656120 the build step took 11m49s and this whole gate step took 26s (13:34:32 to 13:34:58), so a second binary costs about 27x what the comparison costs. That is not a trade I wanted to make silently, so I did not make it. Two things still cover the case: the FLOOR asserted in tests/test_index_resilience.c stops the coverage signal being switched off, and the absolute counts for both sides now print on every run, so a jump is visible in the log even when it does not fail. The script header and the scripts/ci/README.md row both say this.

If you would rather have the second build, say so and I will add it — it is a workflow change, and those are yours to call.

One check stays absolute, on purpose

index_status lists at most 500 files per class and sets truncated when it drops the rest. Checks 1 to 3 read that list file by file, so a clipped list means they judged only part of the tree and would still print PASS. That is the same silent clipping this gate exists to catch, so a clipped list stops the run on either side rather than being compared.

Tests

tests/test_coverage_gate_contract.sh drives the production script with a fake $BIN — a small shell script that prints canned JSON — so no seam is added to the script itself and no indexing happens. Same stubbing shape as tests/repro/repro_script_summary.sh. It builds a two-commit fixture repository so the base worktree is real.

Fifteen cases: each of the four findings present at both sides (must pass) and new at head (must fail); the count equal to, below, and above the base; the recorded number not gating; both sides printing; a clipped file list still stopping the run; the allowlist skipping a path on both sides; an unresolvable base commit stopping the run; and --help describing the comparison.

Red-green: I reverted each of the four comparisons one at a time and confirmed the matching "present at both sides" case goes red, then restored it. A gate test that only ever passes proves nothing.

ok: unusable at both sides is not this branch's doing
ok: unusable new at head fails
ok: unusable gone at head passes
ok: +N marker at both sides passes
ok: +N marker new at head fails
ok: a range already over the share at base passes
ok: a range newly over the share fails
ok: count equal to base passes
ok: count below base passes
ok: count above base fails
ok: the recorded number does not gate
ok: a clipped file list at head fails
ok: an allowlisted path newly over the share passes
ok: an unreachable base commit stops the run
ok: --help describes the merge-base comparison
PASS: coverage-gate contract

Runs from scripts/test.sh as Step 0y. A real end-to-end run against a built binary passes and takes 45s for two indexes, against 26s for the one index the gate used to do.

Where the base commit comes from

In order: $COVERAGE_GATE_BASE_SHA, then the first parent of HEAD when HEAD is a merge commit, then git merge-base origin/main HEAD. pr.yml now passes COVERAGE_GATE_BASE_SHA: ${{ github.event.pull_request.base.sha }}, so CI uses the commit GitHub itself used to build the merge rather than relying on the fallback. The checkout is shallow, so the script fetches the base commit with --depth=1 when the object is not present.

A note on the commit list

ci(coverage): raise the parse-partial ceiling to 59 is still in this branch, and the commit after it removes the ceiling. I kept both rather than rewriting history: the first commit is the measurement that shows main moved the number on its own, and the second is the fix for that being able to fail a branch at all. The recorded 59 in the data file is that same measurement, which is why the file keeps it.

This branch stacks on #1941

GitHub cannot base a cross-fork pull request on a branch in the fork, so this pull request shows #1941's four fix(coverage) and test(coverage) commits underneath its own four. Please review those on #1941. When #1941 merges, the list here shrinks on its own.

Refs #963, #1972, #1985

[github-actions]

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

• It will not be closed for inactivity. No stale bot touches pull requests here.
• It may still sit a while before a human reads it. That is on us, not on you.
• Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

• Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
• Get CI green, or say which failures you believe are pre-existing.
• Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
• Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

[DeusData]

Maintainer decision on the gate, since that is what this PR was split out to get.

The gate is accepted in principle. One condition before it becomes a required check: #1972's merge-base comparison has to land here, not as a follow-up.

The reasoning is your own evidence. A required check that compares parse_partial_count against a constant goes red on unrelated PRs the moment main adds a grammar gap — and you demonstrated exactly that during the split, when main took the count 58 → 59 through _Atomic(type) in src/daemon/runtime.c, with nothing on your branch responsible. You measured it three ways to prove where it came from, which is the only reason that is knowable at all. But the next person to hit it will not have your context; they will have a red required check on a PR that did nothing wrong, and no obvious way to tell a real regression from a ceiling that needs bumping.

A gate that cannot distinguish "this PR added a gap" from "main added a gap" is not yet a gate — and a required check whose verdict depends on when you branched is the shape we do not ship. You said as much yourself in #1972 and left it alone because reshaping a gate is the maintainer's call. It is, and this is the call: bring the merge-base comparison into this PR and the gate goes in as required.

Nothing else is being asked. Specifically:

• The allowlist entry stays. scripts/setup-windows.ps1 at 25.2% is a real grammar gap, documented as one, and your point that the 25% threshold should not be raised to hide it is right. We are taking ownership of that entry as part of accepting the gate rather than treating it as something the PR grants itself.
• The 25%/200-line shape, proportions-not-line-numbers, and Ubuntu-only are all fine as they stand, and the reasons you gave for each are the right ones.

Three things worth saying plainly

You verified the gate fails, not only that it passes. Three negative controls — ceiling lowered, allowlist entry removed, and the earlier MAX_SINGLE_RANGE_PCT=3 run. A gate proven only to pass is indistinguishable from a gate that never checks, and most contributions of this kind stop at the passing run.

You then found three ways your own gate could have passed without checking. That is a rarer instinct than writing the gate.

And one of those three is a house-wide hazard, not a local slip. sed | grep -oE | head -1 aborting under pipefail because head closes the pipe and the writer takes SIGPIPE is a shape this repo has been bitten by before, and there are still dozens of call sites carrying it elsewhere. Your writeup of it is the clearest statement of the failure mode we have. Thank you for that.

Sequencing

#1941 → #1971 → this one, as you laid out. #1941 is currently red on three legs from its own 13:21 push — those completed at 14:35, before main broke at ~15:08, so they are real rather than the incident.

Separately: main does not currently compile right now (duplicate struct members merged into type_registry.h; #1993 is the repair). Your green here predates that and is stale rather than wrong. Wait for #1993 before rebasing anything in this stack.

[DeusData]

One interaction to record while the gate design is being reworked, so it is not discovered as a mystery later.

#1824 makes Blazor .razor files discoverable by mapping them to C#. By design those files land in parse_partial: the C# grammar recovers the @code block while the surrounding markup lands in ERROR regions. That is the intended and honest outcome — it is strictly more than the Module-and-imports other markup-hosted languages yield today — but it means parse_partial_count rises by roughly the repo's .razor count when that PR lands.

That rise is correct, not a regression, and it is exactly the case your gate cannot currently distinguish — the same shape you already identified when main took the count 58 → 59 on its own and filed #1972.

It is a second, concrete argument for the merge-base comparison being a precondition rather than a follow-up: with a constant ceiling, a legitimate language-coverage improvement reads as a gate failure on an unrelated PR, and the person who hits it has no way to tell that from a real regression.

Nothing to do here beyond what was already asked. Recording it so the ordering is deliberate.

[CaptainMittens]

The two red checks are not caused by this branch.

ci-ok is the aggregate and fails only because of test / test-windows-guards
(job 100085897520).

That job stopped in tests\windows\test_daemon_stability.py, after the five sections before it passed:

PASS: parameter surface rejects bare/unknown/out-of-range daemon invocations
PASS: hook fails open without a daemon; notice is visible once, then rate-limited
PASS: start survives an occupied --port, status reports the pid, second start is a no-op
PASS: stop refuses while a session is attached and succeeds once the session closes
PASS: kill -9 recovery — stale state cleared, cold client worked, fresh start got a new pid
SETUP FAIL: permanent daemon did not start for the churn check
RED (tests/windows/test_daemon_stability.py): section_churn_stability failed

The failure is in the setup for the sixth section, not in an assertion.

Three checks against this branch:

Check	Result
Files changed here that match daemon or windows	none of the 20
Same job on sibling PRs	passes on #1986 and #1976
Where the failure sits	daemon start for section_churn_stability, not a test the branch touches

I cannot re-run the job — gh run rerun --failed answers Must have admin rights to Repository, which a fork account does not have. Please re-run it when convenient. Nothing on this branch needs to change.

[DeusData]

Status check, since this has been sitting: the condition on this PR is still unmet, and it is not something you can clear.

Our maintainer accepted this gate conditional on #1972 landing first — "The coverage gate's parse-partial ceiling drifts, reddening PRs that did not cause it". I checked: #1972 is still open, unclosed. Merging a coverage gate while its known drift defect is unfixed would produce exactly the outcome that issue describes: PRs going red for something they did not cause. So this stays blocked on our side, not yours.

Your two reds are also not yours. test-windows-guards fails on test_daemon_stability.py — section_churn_stability in your run — and ci-ok merely aggregates it.

While attributing it I noticed something worth passing on, because it reframes the whole family. That guard has now gone red on three different sections across unrelated PRs — section_start_status_port, section_crash_recovery, and yours — and every single instance carries the same two lines:

SETUP FAIL: index did not run:
SETUP FAIL: ASCII control did not index via CLI:

The setup failures are common to all three. That suggests the per-section "REGRESSION: a fixed Windows bug is broken again" lines may be downstream of one shared setup fault rather than three independent daemon regressions — which matters, because we have an open PR fixing the section_crash_recovery symptom specifically, and if the setup is the real cause that fix would not clear the others. I have recorded it on our side so it gets chased as one problem rather than three.

Nothing for you to do here. When #1972 lands I will come back to this and re-run.

[CaptainMittens]

Understood on the block, and I confirmed #1972 is still open on my side too.

On the shared-cause theory, one line from my run that is not in your quote, plus an observation.

My run carries a third setup failure alongside the two you named:

SETUP FAIL: permanent daemon did not start for the churn check
SETUP FAIL: index did not run:
SETUP FAIL: ASCII control did not index via CLI:

I checked #1918's run as well, which is your section_start_status_port instance. It carries your two lines but not the churn one — which fits the theory rather than complicating it. The churn line is that section's own setup step, and the two shared lines sit underneath it.

The observation: SETUP FAIL: index did not run: prints nothing after the colon, in both runs. The guard reports that indexing failed but captures no reason, so the log cannot say why it failed. Further down the same run, test_hook_augment.py is skipped with PRECONDITION ... exit=2, which looks like the same failure carrying onward.

That supports reading these as one setup fault rather than three daemon regressions. A guard whose index never ran cannot show anything about the behaviour it guards, in either direction — so the three "a fixed Windows bug is broken again" verdicts are currently unsupported rather than wrong. Capturing the indexer's own stderr into that empty message would likely name the cause outright.

I have not looked at the section_crash_recovery run, so that third instance is still your reading rather than mine.