fix(coverage): stop reporting a duplicate range and a line past EOF (#963)

[CaptainMittens]

Two faults in one range string, both in cbm_error_regions_push. scripts/setup-windows.ps1 has 326 lines and its parse-coverage report read:

113-113,113-113,245-327

The same line named twice, and an end line that does not exist. Found while building the coverage work in #1941 and filed rather than fixed there, at review request.

The past-EOF end line

The tree-sitter node is start=(244,2) end=(326,0). An end column of 0 means the node stopped right after the previous line's newline, so it holds no text on the row it points at. Adding 1 to that row to make it 1-based named a line past the end of the file whenever a region ran to EOF.

Fixed by clamping the end to the row above when the end column is 0 and the node spans more than one row.

The duplicate range

Line 113 carries two separate ERROR nodes, at columns 25-29 and 31-32, and each pushed its own range. A line range is advice — "read these lines" — and it says nothing new the second time. Both copies also count against CBM_MAX_ERROR_REGIONS, so a file with many multi-error lines could be clipped while holding fewer distinct lines than the cap allows.

Fixed by dropping a range that exactly repeats the one already open — same start line and same end line. Ranges that merely overlap are left alone, and that distinction is the whole point.

Each range is judged later by cbm_region_is_recovered, which asks whether definitions starting inside the range cover it. Two ranges with identical numbers always get the same verdict, so dropping one changes nothing. Two different ranges do not: merging 3-3 into 2-3 hands the wider range's covering definition to an error that definition does not explain, and a real parse failure then disappears from a report whose only job is to be honest about failures. perl_malformed_source_remains_partial_issue1838 pins that case.

The drop runs before the cap check, so a repeat that was never a distinct range is never counted as one the cap threw away.

The real file now reports 113-113,245-326.

Tests

Two, both proved RED first with the exact expected text:

Test	Red output
coverage_repeated_error_line_reports_one_range_issue963	"2-2,2-2" != "2-2"
coverage_range_never_ends_past_the_last_line_issue963	"1-5" != "1-4"

parse_coverage, index_resilience and mcp: 285 passed, 4 skipped. make -f Makefile.cbm lint-format clean.

Stack order

This is the middle of three. #1941 merged as aa44c28, and this branch is now rebased onto it, so the diff here is exactly one commit — e5c17de6, fix(coverage): stop reporting a duplicate range and a line past EOF, touching only internal/cbm/cbm.c and tests/test_parse_coverage.c. #1968 stacks on this one and should merge after it.

PR	Carries
#1941	the parse-coverage product work — merged as aa44c28
this one	the two range faults above
#1968	the CI gate

#1968 depends on this PR, not only on #1941: its allowlist entry quotes the post-fix figure of 25.2% for setup-windows.ps1, which is only true once the range above is corrected.

Closes #1965. Closes #1966. Part of #963.

[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]

Reviewed. Both fixes are right, and the reasoning in the code is better than I expected going in.

The past-EOF clamp. end.column == 0 && end.row > start.row is the correct guard — the second condition is what stops a single-line node collapsing to line 0, and it would be easy to omit. "An end column of 0 means the node stopped right after the previous line's newline, so it holds no text on the row it points at" is the sentence that makes this reviewable rather than a magic -1.

The duplicate drop, and a correction you made that is worth calling out. The PR description says this merges "a region that overlaps the one already open". The code does not do that, and the comment explains why it must not:

merging 3-3 into 2-3 hands the wider range's covering definition to an error the definition does not explain, and a real parse failure then disappears from the report

That is a genuine hazard — an overlap merge would have silently removed real findings from a report whose entire purpose is honesty — and you caught it, narrowed to an exact-repeat drop, and pinned the distinction with perl_malformed_source_remains_partial_issue1838. Good.

So the one thing I would ask: update the PR description to match. It is the first thing the next reader sees, and right now it describes an approach you deliberately rejected for a good reason. That reason deserves to be in the description, not only in the source.

Ordering the drop before the cap check is also correct, and the comment says so explicitly — a repeat that was never a distinct range must not be counted as one the cap threw away.

Sequencing

#1941 has to land first, and it is currently red on three legs — test-lsan-macos, test-unix (ubuntu-24.04-arm, gcc, 3/3) and test-unix (macos-15-intel).

I want to be careful here rather than hand you a wrong excuse: those failures completed at 14:35, and main broke at ~15:08, so they are not the broken-main incident. They are real and they are on that branch. (By contrast #1739's failures started at 15:22 and are the incident — I have told them so there.)

Your green on this PR is from the same 13:21 push, so it predates the breakage too; it is stale rather than wrong, and will need a re-run once #1993 fixes main.

#1968 carries the CI gate. That one is a maintainer decision rather than a review — a contribution that adds a required gate is something we look at separately on principle, never on the merit of the change. It is not being ignored; it just does not move on the same track as this.

One observation, meant kindly: you currently have five PRs open across two stacks, several sharing files. That is a lot of coupling to hold in your head, and #1976's description already carried one assumption that had gone stale. Landing #1941 and #1896 first would collapse most of it.

[CaptainMittens]

The description is fixed — I edited it at 18:25Z, about an hour after your review, so you were reading the old one. It now says:

Fixed by dropping a range that exactly repeats the one already open — same start line and same end line. Ranges that merely overlap are left alone, and that distinction is the whole point.

and the paragraph under it carries the 3-3 into 2-3 hazard and names perl_malformed_source_remains_partial_issue1838 as the test that pins it. You were right that it belonged there and not only in the source.

#1941's red legs — two of the three are now green

I checked before repeating your numbers back, and the current run disagrees with the 14:35 one. Run 33512870122 on #1941 is 32 pass, 2 fail:

Leg you named	Now
test-unix (ubuntu-24.04-arm, gcc, 3/3)	pass
test-unix (macos-15-intel)	pass
test-lsan-macos	still red

(ci-ok is the second failure, which is the aggregate gate reporting the first.)

The remaining one fails here:

FAIL tests/test_watcher.c:1479: cbm_watcher_poll_once(w) == 0, expected 1 == 1
SUMMARY: AddressSanitizer: 232937 byte(s) leaked in 375 allocation(s)

That assertion writes a file and expects the next poll to report one change. #1941 changes twelve files — Makefile.cbm, internal/cbm/cbm.{c,h}, src/mcp/mcp.c, three pipeline files, src/store/store.c and four test files — and none of them is the watcher or its test. The leak summary follows the failed assertion rather than preceding it, so I read it as fallout from the abort, not a second fault.

I am not going to call it environmental from here, because I have been wrong about a "flaky" leg on this repo before. I will run it down on #1941 and report there.

Sequencing

Agreed, and taken. I am not opening anything further on these two stacks until #1941 and #1896 are in.

One new PR does exist — #1998, for #1995 — but it is off main, touches only src/cypher/cypher.c and tests/test_cypher.c, and shares no file with any of the five. It adds no coupling to the pile you described.

On #1968: understood, and no argument. A contributor should not be the one deciding that a gate becomes required.

[DeusData]

Reviewed on a tree with #1941 trial-merged: both fixes bind (red "2-2,2-2" / "1-5" without your hunk, green with it) and I have no findings on the code. Order is #1941 first — its rebase and one perf fix are on that thread — then a rebase here: your cbm.c hunk applies at an offset and the test hunk re-places after main's #1746 tests, nothing else changes. The description refresh from earlier still applies (the code drops exact repeats, it does not merge overlaps — and your comment explaining why overlap-merging would hide a real parse failure is exactly the note that keeps this correct later). Merges on green after that; thanks for the careful reasoning.

[CaptainMittens]

CI here is red on three checks, and all three trace to one flake in the Windows harness self-test. Nothing in this PR's diff is involved.

Root cause — test / test-windows (windows-latest, CLANG64, x86_64, 2/2)

The shard died in tests/test_parallel_harness_contract.sh, not in a C suite:

Traceback (most recent call last):
  File "<stdin>", line 104, in <module>
ValueError: invalid literal for int() with base 10: ''
rm: cannot remove '.../logs/timeout_exit_race.log': Device or resource busy

The script reads a PID out of a file with int(path.read_text()). Windows still held a handle on the log (Device or resource busy), the read returned an empty string, and int('') raised. Every C test in that shard passed first — pass_1 through pass_32, hang_after_summary, no_summary, pass_after and stubborn_tree all report rc=0 pass=1 fail=0.

The other two are consequences

The crash happened before the manifest upload step, so that step logged No files were found with the provided path: build/c/test-logs/shard-manifest.txt. scripts/ci/verify-shard-union.sh then saw only one of the two shards:

OK:   macos-14-cc           — 1 shard(s), union of slices == full suite list
OK:   macos-15-intel-cc     — 1 shard(s), union of slices == full suite list
OK:   ubuntu-24.04-arm-gcc  — 3 shard(s), union of slices == full suite list
OK:   ubuntu-latest-gcc     — 3 shard(s), union of slices == full suite list
FAIL: windows-latest-CLANG64 shard indices [1] != 1..2

That fails test / shard-completeness, and ci-ok gates on both.

Attribution

• Neither this branch nor fix(coverage): narrow parse-error ranges, and stop the report hiding what it dropped (#963) #1941 changes tests/test_parallel_harness_contract.sh — zero lines.
• The same shard, same platform and compiler, passes on fix(coverage): narrow parse-error ranges, and stop the report hiding what it dropped (#963) #1941 with b04f5450 in the tree.
• The 31 other checks here are green, including ASan+UBSan, LeakSanitizer, MemorySanitizer, ThreadSanitizer on three platforms, all eight gcc test-unix shards, and Windows shard 1/2.

timeout_exit_race kills processes and reads their PID files, and Windows holds file handles longer than POSIX does, so that is where this class of flake surfaces.

No re-run needed on my account — I will rebase this branch onto main once #1941 lands, as agreed, and the force-push restarts every check. Flagging it so the red is not read as a fault in the diff.

[DeusData]

#1941 landed on main as aa44c28, so this branch now only needs your one fix on top of main. A merge of main into it conflicts (the stacked commits were re-merged on the way in), so a rebase onto main is the clean path: git rebase origin/main should leave exactly f89f01e's change. Keep the sign-off and it merges on green; the review is already done.

[CaptainMittens]

Rebased onto aa44c28. The branch is now exactly one commit — e5c17de6, the duplicate-range and past-EOF fix — with the sign-off intact. The five stacked commits dropped out as already-applied, as you expected.

Diff against main is internal/cbm/cbm.c (+39/-2) and tests/test_parse_coverage.c (+47).

Checked before pushing:

• make -f Makefile.cbm lint-format CLANG_FORMAT=clang-format-20 — clean.
• make -f Makefile.cbm test-focused TEST_SUITES="parse_coverage" — 37 passed, including coverage_repeated_error_line_reports_one_range_issue963 and coverage_range_never_ends_past_the_last_line_issue963.

One note on the earlier red, in case it comes back: test-windows-guards failed on section_cold_storm, which is the shared flake tracked in #2057, not this branch. It hit four unrelated branches the same day.