ci(coverage): fail a PR when this repo's own parse-coverage report goes bad (#963) - #1968
Open
CaptainMittens wants to merge 8 commits into
Open
ci(coverage): fail a PR when this repo's own parse-coverage report goes bad (#963)#1968CaptainMittens wants to merge 8 commits into
CaptainMittens wants to merge 8 commits into
Conversation
…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>
|
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) 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. Merge a region that overlaps the one already open instead of repeating it. The walk visits children in source order, so a region starting at or before the open range's end really does overlap it. The merge runs BEFORE the cap check, so it is never miscounted as a dropped range. The real file now 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" Full suite: 7798 passed, 2 failed — both pre-existing agent-client install assertions in tests/test_cli.c, unrelated to this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
…es bad (DeusData#963) A coverage range is advice — "these lines are missing from the graph, read them". It stops being advice when it names most of the file, and it stops being honest when the list was clipped without saying so. Both happened here: src/cli/cli.c reported its whole 13,046 lines as one range, and two caps in series dropped ranges with no signal. Nothing would have caught either. scripts/ci/self-index-coverage-gate.sh indexes this repo with the binary just built and fails on any of four things: 1. A file reports a whole-file parse failure (parse_unusable). Zero today. 2. Any range string carries the "+<N>" truncation marker. With the cap at 256, a file that still overflows is worth stopping for. 3. Any single range covers more than 25% of its file, for files of 200 lines or more. The floor matters: a 5-line PL/SQL limitation fixture with a 3-line range is 60% of itself and says nothing about report quality. 4. parse_partial_count rises above the ceiling in parse-partial-baseline.txt (58 today). This complements the FLOOR in tests/test_index_resilience.c, which stops the signal being switched off by accident. Every check was verified to FAIL, not just to pass: empty allowlist -> setup-windows.ps1 flagged at 25.5% MAX_SINGLE_RANGE_PCT=3 -> cli.c flagged at 3.9% ceiling 57 -> parse_partial_count 58 flagged a repo of broken files -> 4 whole-file failures flagged a 1200-line garbage file -> its clipped range list flagged scripts/setup-windows.ps1 is the one allowlist entry, and it is a real gap rather than noise: one range covers lines 245-327 of a 326-line file because the tree-sitter PowerShell grammar cannot parse the `} else {` branch running to EOF, so those 83 lines genuinely are absent from the graph. Every other file of 200+ lines sits at 3.9% or below, so the 25% threshold has room and should not be raised to hide this. Wired into the existing pr-smoke job, Ubuntu leg only. That job is already in ci-ok's needs, so the gate is a required check with no workflow-graph surgery. Ubuntu only because the flagged ranges depend on which conditional-compilation branches the preprocessor keeps — on a machine where _WIN32 is defined a different set of lines is flagged, which is why the gate asserts proportions and never exact line numbers. The changes filter now notices edits to the gate, the allowlist and the baseline. Runs in 21 seconds. Not extended into scripts/smoke-invariants.sh on purpose: that runs from smoke.yml, whose triggers are workflow_dispatch and push to qa/smoke-**, and which is documented non-gating — it would never run on a PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
…cking (DeusData#963) Verifying the gate against a locally built binary turned up three defects in the gate itself, all of the same shape it exists to catch: it could report PASS without having looked at everything. Reading the ceiling aborted the whole gate on a long file. The ceiling came from `sed | grep -oE | head -1`. Under `set -o pipefail` head closes the pipe, grep dies of SIGPIPE, and the gate exits with no message at all. It does not bite on today's 7-line baseline file and it does bite on a long one, which is proved. Replaced with one awk that stops after the first number. The report's own truncation flag was never read. index_status lists at most 500 files per class (COVERAGE_FILE_CAP) and sets "truncated" when it dropped the rest. Checks 2 and 3 walk that list file by file, so a clipped list means they judge part of the repo and still print PASS. New check 0 stops instead. Today the list holds all 58 files and truncated is false, so this is a guard, not a fix for live behaviour. Check 1 named files it had already accepted. It subtracted allowlisted paths from the count but still printed them in the failure text, so the message disagreed with the number beside it. It now counts and names the same set. The gate also did not answer --help, which scripts/ci/README.md says every script there does. It fed --help to basename, printed a usage error from the wrong program and exited 0. It now prints a Usage: block and exits 0, rejects an unknown flag with exit 2 and the house line "Please consult --help.", and is enrolled in HELP_ENTRIES and STRICT_ENTRIES in the venue parity contract so the rule is enforced rather than only written down. Breaking --help fails that contract with exit 1, which is checked. Added the missing row to the scripts/ci/README.md table. The allowlist reason for scripts/setup-windows.ps1 carried the old numbers. After the range fix in the previous commit the file reports 113-113,245-326, so the widest range is 82 lines rather than 83 — 25.2% of 326. Still over the 25% limit, so the entry stays, and the reason now says why narrowing it by one line did not clear the gate. Verified against a locally built binary: healthy PASS, parse_partial=58 (ceiling 58) parse_unusable=0, exit 0 allowlist emptied FAIL naming setup-windows.ps1 at 25.2%, exit 1 ceiling at 57 FAIL naming the count, exit 1 both restored PASS, exit 0 Venue parity contract: 19 --help entries, 9 strict-flag entries, green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
…dded (DeusData#963) The gate's check 4 failed on its own pull request: parse_partial_count is 59 against a ceiling of 58. The rise did not come from this branch. Measured with the binary built from this branch, three trees indexed: this branch, no merge 58 matches the baseline as written origin/main alone 59 main merged into it 59 identical file list to main alone CI tests the merge of a pull request into main, so the gate sees 59. The one file main added to the flagged list is src/daemon/runtime.c, at one line: src/daemon/runtime.c 47-47 1 line of 3291 0.03% of the file Line 47 is a function-style _Atomic declaration: static _Atomic(cbm_daemon_runtime_containment_hook_t) runtime_containment_hook_seam; The tree-sitter C grammar does not parse that form. The keyword form four lines above it, `static _Atomic uint32_t ...`, parses fine. This is the same class of grammar limitation this branch already pins for _Thread_local in tests/test_parse_coverage.c. It arrived with fc1b1ee on main. So the ceiling moves to 59 rather than the file being allowlisted. An allowlist entry is for a file whose single range is over the 25% limit and has a written reason to stay; one line out of 3291 is nowhere near it, and hiding the file would remove a real gap from the count the ceiling exists to watch. Known cost, worth stating plainly: a ceiling checked against a moving main drifts. Any merge to main that adds a partially-parsing file reddens every open pull request until someone edits this file by hand, and the pull request that goes red is never the one that caused it. This commit does not fix that - the fix is to compare against the merge base instead of a checked-in number, which changes what the gate is and belongs to whoever owns CI policy here. Filed separately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
CaptainMittens
force-pushed
the
ci/self-index-coverage-gate
branch
from
August 31, 2026 20:02
0c20dcb to
439167e
Compare
adfjadfj16-a11y
approved these changes
Aug 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split out of #1941 at review request: the CI gate on its own, because it adds a required check to
pr-smokeand that is a maintainer decision rather than part of a parse-coverage fix.Stack order
Last of three. Merge in this order:
Cross-fork PRs cannot base on a fork branch, so until the two below merge, the diff here shows their commits too. The three commits that belong to this PR are the last three:
ci(coverage): fail a PR when this repo's own parse-coverage report goes badci(coverage): fix three ways the coverage gate could pass without checkingci(coverage): raise the parse-partial ceiling to 59, for a gap main addedThe dependency on #1971 is real, not just ordering: the allowlist entry quotes 25.2% for
setup-windows.ps1, which is only true once #1971 corrects that range.What it adds
scripts/ci/self-index-coverage-gate.shindexes this repo with the binary just built and fails on any of four things:parse_unusable). Zero today.+<N>truncation marker. With the cap at 256, a file that still overflows is worth stopping for.parse_partial_countrises above the ceiling inparse-partial-baseline.txt. This complements the FLOOR intests/test_index_resilience.c, which stops the signal being switched off by accident.Wired into the existing
pr-smokejob, Ubuntu leg only. That job is already inci-ok'sneeds, so the gate is a required check with no workflow-graph surgery. Ubuntu only because the flagged ranges depend on which conditional-compilation branches the preprocessor keeps — on a machine where_WIN32is defined a different set of lines is flagged, which is why the gate asserts proportions and never exact line numbers. Runs in 21 seconds.scripts/setup-windows.ps1is the one allowlist entry, and it is a real gap rather than noise: the tree-sitter PowerShell grammar cannot parse the} else {branch running to EOF, so those lines genuinely are absent from the graph. Every other file of 200+ lines sits at 3.9% or below, so the 25% threshold has room and should not be raised to hide this.Three ways the gate could pass without checking
Verifying the gate against a locally built binary turned up three holes. All are fixed in the second commit, which was previously described here as a follow-up — it is now in this PR, since #1971 exists to carry the range fix its allowlist text depends on.
sed | grep -oE | head -1headclosing the pipe early raised SIGPIPE, and underpipefailthat aborted the whole gate — a crash, not a checktruncatedflag was never readindex_statuslists at most 500 files per class; a clipped list meant checks 2 and 3 judged part of the repo and still printed PASSThe same commit adds
--helpand enrols the script in the venue parity contract (tests/test_venue_parity_contract.sh).The ceiling is 59, not 58 — and that is main's doing
The third commit raises the ceiling by one.
maingainedsrc/daemon/runtime.c, whose line 47 the C grammar cannot parse:The function-style
_Atomic(type)fails; the keyword form_Atomic uint32_tparses. One line out of 3291, 0.03% of the file — a real grammar gap, correctly flagged, nothing to fix in the source.Measured three ways with one binary, so the rise is not being blamed on the wrong tree:
origin/mainaloneThe merged file list and main's own file list are identical. The rise came from
main, not from this branch.This will happen again, and the gate as written cannot tell the difference — filed as #1972 with a suggested fix (compare against the merge base rather than a constant). Left as a filed issue rather than changed here, because reshaping a CI gate is the maintainer's call.
Verified to fail, not only to pass
Re-run against a binary built from the actual merge of this branch into current
main:PASS—parse_partial=59 (ceiling 59) parse_unusable=0 allowlisted=1FAIL: parse_partial_count is 59, above the ceiling 58FAIL: scripts/setup-windows.ps1 has one range of 82 lines — 25.2% of 326, over 25%The 82 lines / 25.2% in that last row is the post-#1971 figure and confirms the range fix is in the binary under test; before it, the same case read 83 lines / 25.5%.
Earlier verification of the other checks, from the original split:
MAX_SINGLE_RANGE_PCT=3flaggedcli.cat 3.9%, a repo of broken files flagged 4 whole-file failures, and a 1200-line garbage file flagged its clipped range list.Suites on this branch:
parse_coverage,index_resilience,mcp— 285 passed, 4 skipped.tests/test_venue_parity_contract.shOK.make -f Makefile.cbm lint-formatclean.Closes nothing on its own. Part of #963.
Fixes #1985