fix(coverage): narrow parse-error ranges, and stop the report hiding what it dropped (#963)

[CaptainMittens]

What this fixes

index_status reported src/cli/cli.c as one error range of 1-13047 — the
whole file. The file was indexed fine; the report was wrong, and "grep 13,000
lines" is not useful advice.

The cause is conditional compilation splitting a brace. Tree-sitter parses raw
text with no preprocessor, so this shape gives it two if headers and one
closing brace:

#ifndef _WIN32
if (...) {
#else
if (...) {
#endif

At file scope it cannot recover, the root node becomes an error node, and the
old code blamed the whole file. cli.c has three such blocks. This is not
specific to us — a platform guard around lstat/stat is a common shape in C.

The fix reads the SECOND parse. The pipeline already preprocesses any file
containing #ifndef, and that expanded parse is clean. The report simply never
consulted it.

Measured result

file	before	after	share of file
src/cli/cli.c	1-13047, one range	85 ranges, 1819 lines	100% → 13.9%
tests/test_cli.c	6821 lines to EOF	70 ranges, 441 lines	48.6% → 3.1%
src/cli/activation_transaction.c	907 lines to EOF	11 ranges, 175 lines	38% → 7.5%
whole repo	78 flagged files	58 flagged files	—

What survives is honest. The three biggest ranges left in cli.c are real
discarded platform blocks that are genuinely absent from the graph on this
platform.

Two silent caps, both fixed

Narrower ranges mean more of them, which pushed real files against limits that
used to be unreachable. There were two, in series, and both dropped ranges with
no signal at all:

cap	where	was	now
CBM_MAX_ERROR_REGIONS	internal/cbm/cbm.c	64	256
COVERAGE_RANGE_MAX	src/mcp/mcp.c	128	256

Both were measured binding at 64 — cli.c and tests/test_cli.c sat on
exactly 64 ranges each. That means every coverage number this project has ever
reported for those files was a floor, not a measurement.

A raised cap is still a cap, so a range string can now end with ,+<N> naming
how many ranges were dropped, and check_index_coverage reports
"truncated": true — both when it sees that marker and when its own limit
stops the list.

A third coverage kind: parse_unusable

A file whose single range still covers 80 percent or more of it now reports
parse_unusable instead of listing the range. The file WAS indexed; pointing a
reader at nearly every line just tells them nothing, so the report says "read
the source directly".

Its main customer is not C. The narrowing step only runs for C, C++ and CUDA,
so a Python, Java or Ruby file whose root node fails still reports one
whole-file range. Verified against real broken files in those languages.

It is deliberately not called parse_failed. "Failed" reads as a skip phase,
and a reader who thinks a file was skipped believes it is absent from the graph
entirely. Two places in the report already fell through to "skipped" for want
of an explicit branch; both now name the kind.

This is the first of three PRs

The scripts/ci/self-index-coverage-gate.sh commit that used to sit here is now
#1968, at review request. It adds a required check to pr-smoke, which is a
maintainer decision rather than part of a parse-coverage fix.

A third PR sits between them, because two of the findings below are fixed rather
than only filed, and #1968's allowlist text quotes a figure that is only correct
once they are:

PR	Carries	Merge
this one	the parse-coverage product work	first
#1971	the duplicate range and the past-EOF line (#1965, #1966)	second
#1968	the CI gate	third

Cross-fork PRs cannot base on a fork branch, so #1971 and #1968 each show this
PR's commits in their diff until this one merges. Both descriptions name which
commits are their own.

Nothing else moved. The four commits here touch no CI or scripts/ci/ file at
all.

One field renamed after review

Each parse_unusable entry carries one number, and it was called lines. That
reads as the length of the file, and the two are not the same number — a
grammar can end an error node past the last line, which this repo has already
met (scripts/setup-windows.ps1 has 326 lines and its range ends at 327).
lines also already means a definition's line span everywhere else in the same
response.

The field is now range_end, at both places that emit it, with the caveat
written in a comment beside each. One test, proved RED first against the old
name. Deriving the real file length instead is not available here: neither
cbm_file_error_t nor cbm_coverage_row_t carries it.

Tests

Every test was checked RED before it was kept, either by breaking the code
under it or by measuring first.

• The range narrows to the dropped branch; explained lines are excluded; a
  range never starts or ends on a directive; real garbage beside a split brace
  is still flagged; a clean file stays unflagged.
• The cap reports what it dropped, and an under-cap file carries no marker.
• The three coverage classes are told apart, including a non-C whole-file case.
• The Studio Export range join puts ONE marker at the end with the summed
  count — a marker left mid-string makes every reader stop there and silently
  lose the ranges after it.
• check_index_coverage emits every range in front of a marker, never turns
  the marker's digits into a range, and reports truncated from both caps.
• tests/test_index_resilience.c gained a ceiling beside its floor: exactly
  one of two fixture files is flagged and the clean neighbour is absent.
• The three _Thread_local grammar forms are pinned as measured, so a
  tree-sitter bump that changes them turns a test red instead of leaving a
  stale note behind.

Full suite: 7735 passed, 28 failed, 7 skipped. The 28 are pre-existing
agent-client install/uninstall failures in the cli suite — a clean worktree
at main reports the identical 263/28, so they belong to a separate ticket.

Found on the way, filed not fixed

All now have issues, per review:

Finding	Issue
scripts/setup-windows.ps1 names line 113 twice — two ERROR nodes on one line each push their own range	#1965, fixed in #1971
The same file's range ends at line 327 of a 326-line file — a node ending at column 0 still gets +1	#1966, fixed in #1971
cbm_count_lines and the inline orig_lines counter disagree by one on a trailing newline	#1967
scripts/smoke-invariants.sh asserts 14 tools when the server registers 16, and never checks the other direction	#1969

Related to #963.

Fixes #1984

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

dco is the only red check here — everything else is green — and since you will be amending for it anyway, there is one small extra thing to fold into the same pass.

1. Sign-off. None of the four commits carry a Signed-off-by: line, so all four need one. git rebase --signoff main (or an interactive reword) will do it in one go, then force-push with lease.

2. While you are in there: please drop the Claude-Session: lines. They point at a private session URL that nobody reading the history can open, so it is noise in the permanent record. Co-Authored-By: is fine and can stay — that is honest attribution and we are not asking you to remove it. This is a convention that was not written down anywhere, which is on us, not on you.

On the change itself: the commit messages here are better than most of what lands in this repo, and specifically for reasons worth naming.

You did not just state that the range was wrong — you found the mechanism (three #ifndef _WIN32 blocks splitting a brace, so the raw parse cannot resync at file scope and the root becomes ERROR), and then noticed the pipeline already had the answer sitting unused in the preprocessed parse.

The three rules are all documented as found by running it, which is the part that usually goes missing:

• the blank-line rule, where treating a dropped branch's blank line as proof suppressed every C range in the suite;
• the directive-line rule, with its cost stated explicitly — a #define the raw parse really dropped no longer surfaces on its own;
• the top-level macro rule, tied to the recovery walker's deliberate refusal in fix(cpp): remap recovered definitions to original lines #949.

Recording a known cost rather than quietly absorbing it is the right instinct, and it is what makes the rule reviewable instead of something a future reader has to re-derive.

Also noted: the ordering rationale for the three coverage steps is argued from where each one's evidence lives, not from what happened to work — and you fixed 13 pre-existing clang-format violations from earlier commits on the branch while confirming the reflowed tool descriptions concatenate byte-identically, so no output moved.

Get the sign-off on and this goes into review properly. Thanks for a strong first contribution.

[DeusData]

One more ask, which I missed in my last comment and should have flagged first time — apologies for the two-part review.

Please split fccf8215 (the CI gate) into its own PR.

That commit adds a new blocking step to pr-smoke in .github/workflows/pr.yml, a new gate script (scripts/ci/self-index-coverage-gate.sh), an exemption allowlist, and a hardcoded baseline ceiling. Those are maintainer-owned policy here: they add a required check every future PR must clear, and that decision belongs to the project owner rather than to a code review. It is not a judgement on the gate — building it with four checks each verified to fail independently is the right way to build one — but it should not ride in on a parse-coverage fix.

The good news is your commit boundary is already exactly on that line: fccf8215 touches only pr.yml and the three scripts/ci/ files, and the other three commits touch zero gate surfaces. So it should lift out with a clean rebase -i drop and no rework.

That leaves this PR as the ~1200 lines of product work, which is what I actually want to review.

So the full list, all mechanical:

1. Signed-off-by on all four commits (blocking — the only red check).
2. Drop the Claude-Session: lines. Co-Authored-By: is fine and stays.
3. Split fccf8215 out into its own PR.

Two non-blocking notes while you are in there, take or leave:

• add_parse_unusable_summary labels atoi(dash+1) as lines, i.e. the file length — but that is the range end, and your own found-on-the-way note shows a range end can exceed the file length (245–327 on a 326-line file). Worth renaming or deriving properly in a change whose whole thesis is honest reporting.
• orig_lines in cbm_extract_file_ex counts every \n plus one, while cbm_count_lines skips a trailing newline. They feed different consumers so nothing breaks today, but one helper would be easier to keep honest.

And please do file your two found-not-fixed items as issues — the duplicated 113-113 range especially.

[CaptainMittens]

All three done, plus both notes. No apology needed for the two-part review — the split was the right call and it lifted out cleanly.

1. Signed-off-by. All four commits carry it. dco is green.

2. Claude-Session: lines. Gone from every commit message, and from the PR description too. Co-Authored-By: stays.

3. The gate is now #1968. You were right that the boundary was already on the line — git cherry-pick of the other three onto main applied with no conflict, and the only tree difference against the old head is the four gate files. #1968 stacks on this PR and should merge after it, because checks 1 and 2 read the parse_unusable kind and the +<N> marker that this PR adds. Its description says so.

One thing you should know that was not visible before the split: a follow-up already exists that fixes three ways the gate could report PASS without having looked at everything — reading the ceiling could abort the whole gate under pipefail, the report's own truncated flag was never read, and check 1 named files it had already accepted. It is not in #1968 because its allowlist text depends on a range fix in a product PR I have not opened yet. It follows as its own PR. Given the gate is a required check, you may prefer to hold #1968 until that one is beside it — your call.

---

Note 1, taken. You are right, and it was worse than a mislabel: lines already means a definition's line span everywhere else in the same response, so one word carried two meanings in one document. The field is now range_end at both emit sites, with the caveat written in a comment beside each. Deriving the real file length was the other option and is not open here — neither cbm_file_error_t nor cbm_coverage_row_t carries it, only the path and the range string.

One test, proved RED first (range_end is NULL against the old name). It reads the expected end line from the persisted coverage row rather than a constant, so it states the property and not a measurement, and it asserts the old name is gone rather than kept beside the new one. index_resilience, parse_coverage and mcp: 283 passed, 4 skipped.

Note 2, filed rather than changed: #1967. You have it exactly — cbm_count_lines ignores a trailing newline, the inline orig_lines counter does not, and they disagree by one on nearly every file. Nothing breaks today because one sizes an array (generous is safe) and the other decides the 80% threshold (generous is harmless). I left it out of this PR because picking one convention is a real change with two defensible answers, not a rename, and this PR is already large. The issue says which answer needs writing down.

---

Issues filed, and I found two more while writing them up:

Finding	Issue
The duplicated 113-113 range	#1965
The range ending at 327 on a 326-line file	#1966
The two line counters that disagree (your note 2)	#1967
smoke-invariants.sh asserts 14 tools when the server registers 16 — and only checks one direction, so it prints all 14 tools present while covering 14 of 16	#1969

That last one is the same shape as everything else in this PR: a check reporting success without having looked at everything.

This PR is now the ~1200 lines of product work plus the range_end rename. Ready for review.

[CaptainMittens]

Thanks — all four points are addressed. Taking them in order.

Sign-off (blocking)

Done. All four commits now carry Signed-off-by, and dco is green.

The cause was simple: this repo ships scripts/install-git-hooks.sh, and I had never run it. I have added a prepare-commit-msg hook locally instead, so the trailer goes on automatically from here. A commit template would not have worked — git commit -m and git commit -F ignore the template, and that is how most of these commits get written.

The Claude-Session: lines

Gone from every commit. Co-Authored-By: stayed, as you said it could.

Splitting the CI gate

Done — the gate is now #1968. You were right that it does not belong here: it adds a required check to pr-smoke, and that is your call to make, not a detail of a parse-coverage fix.

One thing changed while splitting it. A third PR now sits between the two, so the order is:

PR	Carries	Merge
#1941 (this one)	the parse-coverage product work	first
#1971	the duplicate range and the past-EOF line	second
#1968	the CI gate	third

#1971 exists because #1968's allowlist comment quotes 25.2% for scripts/setup-windows.ps1, and that figure is only correct once the range fix lands. Without a middle PR the gate would either carry a wrong number or carry a product fix — which is the thing you asked me not to do.

Cross-fork PRs cannot base on a fork branch, so #1971 and #1968 each show this PR's commits until this one merges. Both descriptions name which commits are their own.

Note 1 — lines is really the range end

Taken. You are right, and it is worse than a bad name: lines already means a definition's line span in six other places in the same response. One word, two meanings, one document.

The field is now range_end at both places that emit it, with the caveat written beside each:

/* The end of the range, not the length of the file. A grammar can end
 * an error node past the last line, so this number can be larger than
 * the file. See range_end_is_not_file_length. */

I could not give you the real file length instead, which would have been the better fix. Neither cbm_file_error_t nor cbm_coverage_row_t carries it, so there is nothing to read. Renaming was the honest option available.

One test covers it, proved red first against the old name. It reads the expected end line from the persisted coverage row rather than hard-coding a number, and asserts the old lines key is absent.

Note 2 — orig_lines versus cbm_count_lines

Confirmed and filed as #1967, not fixed here. Two counters over the same buffer disagree by one on a trailing newline. It is a real inconsistency but it is not what this PR is about, and the numbers it would move are pinned by tests in the commits above.

The two found-not-fixed items

Both filed, and both fixed:

Finding	Issue	Fix
line 113 named twice — two ERROR nodes on one line each pushed a range	#1965	#1971
the range ends at line 327 of a 326-line file	#1966	#1971

The duplicate you singled out was the more interesting of the two. Both copies 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. The merge in #1971 runs before the cap check, so a merged region is never miscounted as a dropped one.

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

Two more turned up on the way and are filed rather than fixed: #1969 (smoke-invariants.sh asserts 14 tools when the server registers 16, and never checks the other direction) and #1972 (the gate's ceiling drifts against a moving main, so it reddens PRs that did not cause it — worth your opinion before #1968 merges).

CI

Green on this PR: dco, lint, and all three pr-smoke legs. #1971 and #1968 are still running.

[CaptainMittens]

Rebased onto a824d82a. The conflict came from 17b5a432 ("fix(perl): update grammar for format declarations"), which added two fixtures and two tests to tests/test_parse_coverage.c in the same place this branch adds its own. Both sides are purely additive, so both sets of tests are kept — nothing was dropped.

Rebasing surfaced a real regression in the branch stacked above this one (#1971), which I have fixed there rather than here. The short version: that commit merged overlapping error ranges into one, and the merge handed a wider range's covering definition to an error the definition does not explain. perl_malformed_source_remains_partial_issue1838 — one of the tests 17b5a432 added — caught it. #1971 now drops only an exact repeat of the range already open, which still fixes the 113-113,113-113 duplicate it was written for.

Full suite at the top of the stack: parse_coverage, extraction, pipeline, mcp and index_resilience all pass.

[CaptainMittens]

test / test-lsan-macos is red on this branch. I believe it is an unrelated flaky test, and I cannot re-run the job myself — I have no write access here. A re-run from a maintainer would confirm it.

The evidence, in case it saves you the dig:

What failed — one test out of 7789:

FAIL tests/test_watcher.c:1479: cbm_watcher_poll_once(w) == 0, expected 1
watcher   rc=1  pass=71  fail=1
7789 passed, 1 failed, 7 skipped

watcher_identical_watch_preserves_dirty_baseline appends a file, touches the watcher, re-registers the same path, then expects cbm_watcher_poll_once to report one change. It reported none.

The leak is downstream of that assertion, not a separate problem. The test opens a store at line 1459 and fails at line 1479, so the close never runs:

SUMMARY: AddressSanitizer: 232937 byte(s) leaked in 375 allocation(s)
  #12 store_open_internal                                   store.c:823
  #13 test_watcher_identical_watch_preserves_dirty_baseline  test_watcher.c:1459

Those 375 allocations are one abandoned sqlite connection.

Why I do not think this branch causes it:

1. This branch touches neither tests/test_watcher.c nor src/watcher/watcher.c.
2. Its only change to shared code is a comment. src/store/store.c shows 24 changed lines; every one is inside a doc block describing parse_partial, parse_unusable and the skip phases. No executable change.
3. test-lsan-macos passes on fix(coverage): stop reporting a duplicate range and a line past EOF (#963) #1971, which contains every commit on this branch plus one more.

Why the test can lose a race. The assertion depends on the file write and the poll landing in different filesystem timestamp ticks. On a loaded macOS runner they can land in the same tick, and the watcher then correctly reports no change. If that reading is right, the fix belongs in the test rather than in any product code, and I am happy to open a separate issue for it — I have not, because one red run is not yet evidence of a pattern.

[DeusData]

Thank you for the careful failure attribution. #1993 has now repaired main, so the inherited compile failure is gone. We will handle the requested rerun in the PR review queue rather than asking you to change this branch or file a watcher issue from a single run. Nothing further is needed from you at this stage, and thank you for tracing the leak to the failed assertion rather than treating it as a separate defect.