fix(coverage): stop reporting a duplicate range and a line past EOF (#963) - #1971
fix(coverage): stop reporting a duplicate range and a line past EOF (#963)#1971CaptainMittens wants to merge 5 commits into
Conversation
|
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. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
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. |
…eusData#963) src/cli/cli.c reported an error range of 1-13047 — the whole file. The file indexed fine; the report was wrong. Three #ifndef _WIN32 blocks split a brace (two `if` headers, one closing brace), so the raw tree-sitter parse cannot resync at file scope, the root node becomes ERROR, and cbm.c takes its whole-file branch. The pipeline already parses these files a second time after preprocessing, and that parse is clean. The report just never consulted it. Build one byte per original line from the preprocessed pass, then cut each raw error range down to the runs of lines the second parse could not vouch for. Three rules, all found by running it and all load-bearing: - An expanded line only vouches for its original line when it HAS TEXT. The preprocessor emits a blank line where it dropped a branch; treating that blank as proof suppressed every C range in the suite. - Preprocessor directive lines (with backslash continuations) never count as missing code — the preprocessor consumes them, so the second parse can never vouch for one. Without this every #include block reported as a miss. Known cost: a #define the raw parse really dropped no longer shows up on its own. - A TOP-LEVEL macro invocation line never counts as vouched-for even when the expanded line parses clean. The macro can expand to a whole definition that the recovery walker deliberately refuses to adopt (DeusData#949), so a clean second parse there proves nothing. An in-body invocation is the benign DeusData#1071 case and is left to the existing macro subtraction. The order of the three coverage steps is now settled by where each one's evidence lives: recovery subtraction -> before the refinement; its evidence is a whole definition that STARTS inside the range, so it must be asked while the range still matches the construct the refinement -> middle DeusData#1071 macro rule -> after the refinement; its evidence is per-line, so a narrow range points at the call itself Measured on this repo: src/cli/cli.c goes from one whole-file range to 64 ranges over ~9.8% of the file, tests/test_cli.c from 48.6% to ~2.9%, src/cli/activation_transaction.c from 38% to 7.5%. What survives is honest — the biggest remaining ranges in cli.c are genuinely discarded #ifdef _WIN32 and #ifdef CBM_CLI_ENABLE_TEST_API blocks, absent from the graph on this platform. Both percentages above are floors, not measurements: cli.c and test_cli.c now land on exactly 64 ranges, which is CBM_MAX_ERROR_REGIONS. That cap drops regions with no signal, and a follow-up raises it and adds a truncation marker. Five tests, all red before the change: the range narrows to the dropped branch; lines the preprocessor explained are excluded; a range never starts or ends on a directive; real garbage beside a split brace stays flagged; a clean file stays unflagged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
…-file failures (DeusData#963) Two silent failures in the parse-coverage report, both made visible by the Phase 2 range refinement that came before this. ## The caps dropped ranges with no signal Two caps sat in series and both returned early without saying anything: CBM_MAX_ERROR_REGIONS = 64 internal/cbm/cbm.c COVERAGE_RANGE_MAX = 128 src/mcp/mcp.c Raising only the first would have moved the clip from 64 to 128, so both move to 256. This was live behaviour, not a theoretical limit: after Phase 2 split one whole-file range into many small ones, src/cli/cli.c and tests/test_cli.c both reported exactly 64 ranges — the cap binding, dead-on, twice. Every coverage figure measured before this change was a floor. With the cap at 256 the true numbers are cli.c 13.9% (not 9.8%) and test_cli.c 3.1%, and the longest list in the repo is 85 ranges. A raised cap is still a cap, so the report now says when it clipped: - cbm_error_regions_t gained a `dropped` counter, and cbm_collect_error_regions walks to the end instead of stopping at the cap, so the count is exact rather than a lower bound. That costs little — the walk never descends into an ERROR subtree. - cbm_error_ranges_str appends ",+<N>" when N ranges were thrown away. - coverage_add_ranges reads that marker and sets "truncated": true, and also sets it when its own limit stops the loop. Before this the marker was invisible: the parser stopped at the '+' with no error and no leftover, so a clipped list arrived looking complete. - objectscript_export_append_error_ranges strips markers off both operands before joining two Studio Export parts and adds one back at the end. A marker left mid-string would make every reader stop there and silently lose every range after it. ## A whole-file range is not advice "Look at lines 1 to 13047" of a 13046-line file tells a reader nothing. Those files now carry their own kind rather than being described as partially covered. New `parse_unusable` field in CBMFileResult, set when one range covers 80% or more of the file. Its customers are non-C languages: the Phase 2 refinement that narrows a whole-file range using the preprocessed parse only runs for C, C++ and CUDA, so a Python, Java, Ruby or TypeScript file whose root node is ERROR still reports 1-N. Verified against real files in all four. The kind is `parse_unusable`, not `parse_failed`. index_coverage.kind already means one of two things — indexed-but-partial, or a skip phase saying the file was never indexed at all — and `parse_failed` reads as the second when it is the first. The store.c schema comment, which is the only written record of this vocabulary, now describes all three classes and says why. Two places would have mislabelled the new kind as "skipped", which is exactly that confusion: coverage_status fell through to its catch-all pass, and add_coverage_report fell into its else branch. A reader who finds a file under "skipped" believes it is absent from the graph, when it was indexed. Both now have explicit branches. index_status gained parse_unusable_count so a CI gate can read it without parsing anything else, get_code_snippet says "read the source directly" instead of naming useless ranges, and the three tool descriptions that listed two coverage kinds now list three. ## Tests Seven added. The cap test moved from 64 to 256; a new test asserts the marker carries a real drop count and that nothing follows it; an inverse test asserts an under-cap file carries no marker at all. For the new kind: a Python file whose root is ERROR is unusable, a file with a local parse failure stays partial, a clean file is neither, and — the one that matters most — the #ifdef-split C file that started this work is partial and never unusable. If that last one ever flips, the Phase 2 refinement has stopped working. Full suite: 7732 passed, 28 failed, 7 skipped. The 28 are pre-existing agent-client install/uninstall failures in the cli suite, identical in count and identity at clean HEAD. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
…e grammar limit (DeusData#963) Phase 5. Four test groups, each checked RED before it was kept. - The Studio Export range join puts ONE ",+<N>" marker at the end with the summed drop count. A marker left mid-string makes every reader stop there and silently lose the ranges after it. Reaching the join through the pipeline needs an export file with 256+ error regions across two <Class> elements, so it goes through a test seam, following the pattern already in this repo (CBM_COVERAGE_MARKER_TEST_API). - 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 — the producer's and its own 256 limit. - test_index_resilience now has a ceiling beside its floor: exactly one of the two fixture files is flagged, the clean neighbour is absent, and the range does not cover the whole file. - The three _Thread_local forms are pinned as measured. Only the array form fails today; the plan's Phase 0 also listed the pointer form, and that is wrong on the grammar shipped now. Also fixes 13 clang-format violations the earlier commits on this branch left in cbm.c, mcp.c and pass_definitions.c. `make -f Makefile.cbm lint-format` would have failed CI. The changes are whitespace only — the two reflowed tool descriptions concatenate byte-identically, so no output moved. Full suite: 7735 passed, 28 failed, 7 skipped. The 28 are the pre-existing cli install/uninstall failures, identical at clean HEAD. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
…DeusData#963) Review follow-up on this branch. Each parse_unusable entry carries one number, and the field 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. A report whose whole thesis is honest reporting should not name that number after the wrong thing. "lines" also already means something else in this same response. Every search result carries a "lines" field holding a definition's line span. One word, two meanings, one document. The field is now "range_end", at both places that emit it — add_coverage_report reading the persisted rows, and add_parse_unusable_summary reading the per-run errors. The comment beside each one says the number can exceed the file, so the next reader does not have to rediscover it. Deriving the real file length instead was the other option and is not available 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 field name. It reads the 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. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
…eusData#963) 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. Two separate faults, both in cbm_error_regions_push. Past-EOF end line. A tree-sitter node that ends at column 0 stopped right after the previous line's newline, so it holds no text on the row it points at. Adding 1 to that row named a line past the end of the file whenever the region ran to EOF. The node here is start=(244,2) end=(326,0). Clamp the end to the row above when the end column is 0 and the node spans more than one row. 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 says nothing new the second time. Drop a range that exactly repeats the one already open. The drop runs BEFORE the cap check, so a repeat is never miscounted as a range the cap threw away. Only an EXACT repeat is dropped, never a range that merely overlaps. Each range is judged separately afterwards by cbm_region_is_recovered, which asks whether definitions starting inside that range cover it. Two ranges holding the same numbers always get the same verdict, so dropping the repeat 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 vanishes from the report. That is not hypothetical. An earlier version of this commit merged on overlap and broke perl_malformed_source_remains_partial_issue1838, the test added with the Perl grammar refresh in 17b5a43. The malformed fixture produces two ERROR nodes, at lines 2-3 and 3-3. Merged, the 2-3 range looks fully covered by before_error and is removed, so parse_incomplete comes back false on a file that plainly does not parse. That test now pins this boundary. The real file reports "113-113,245-326". Two tests, both proved RED first with the exact expected text: 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" Suites run on this change: parse_coverage 34, extraction 325, pipeline 264, mcp 246, index_resilience 7 — all passing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
2b9ed3c to
a5af586
Compare
|
Reviewed. Both fixes are right, and the reasoning in the code is better than I expected going in. The past-EOF clamp. 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:
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 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 — I want to be careful here rather than hand you a wrong excuse: those failures completed at 14:35, and 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 #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. |
DeusData
added
bug
|
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:
and the paragraph under it carries the #1941's red legs — two of the three are now greenI checked before repeating your numbers back, and the current run disagrees with the 14:35 one. Run
( The remaining one fails here: That assertion writes a file and expects the next poll to report one change. #1941 changes twelve files — 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. SequencingAgreed, 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 On #1968: understood, and no argument. A contributor should not be the one deciding that a gate becomes required. |
2 participants
Two faults in one range string, both in
cbm_error_regions_push.scripts/setup-windows.ps1has 326 lines and its parse-coverage report read: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: merging3-3into2-3hands 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_issue1838pins 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:
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_resilienceandmcp: 285 passed, 4 skipped.make -f Makefile.cbm lint-formatclean.Stack order
This is the middle of three. It stacks on #1941 and should merge after it. Until #1941 merges, the diff here shows its commits too — the commit that belongs to this PR is the last one,
fix(coverage): stop reporting a duplicate range and a line past EOF, touching onlyinternal/cbm/cbm.candtests/test_parse_coverage.c.#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.