Skip to content

Commit 5ef8131

Browse files
fix(coverage): name the whole-file number for what it is, a range end (#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>
1 parent 8a97328 commit 5ef8131

2 files changed

Lines changed: 100 additions & 6 deletions

File tree

src/mcp/mcp.c

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4579,8 +4579,11 @@ static void add_coverage_report(yyjson_mut_doc *doc, yyjson_mut_val *root, cbm_s
45794579
yyjson_mut_val *fe = yyjson_mut_obj(doc);
45804580
yyjson_mut_obj_add_strcpy(doc, fe, "path", rows[i].rel_path);
45814581
yyjson_mut_obj_add_bool(doc, fe, "whole_file", true);
4582+
/* The end of the range, not the length of the file. A grammar
4583+
* can end an error node past the last line, so this number can
4584+
* be larger than the file. See range_end_is_not_file_length. */
45824585
const char *dash = rows[i].detail ? strchr(rows[i].detail, '-') : NULL;
4583-
yyjson_mut_obj_add_int(doc, fe, "lines", dash ? atoi(dash + 1) : 0);
4586+
yyjson_mut_obj_add_int(doc, fe, "range_end", dash ? atoi(dash + 1) : 0);
45844587
yyjson_mut_arr_add_val(pu_files, fe);
45854588
}
45864589
pu_n++;
@@ -7950,13 +7953,13 @@ static void add_parse_partial_summary(yyjson_mut_doc *doc, yyjson_mut_val *root,
79507953
/* Attach the whole-file half of the coverage summary. Always emits a top-level
79517954
* "parse_unusable_count" (0 on clean runs) so the CI coverage gate can read it
79527955
* without parsing anything else. When files were flagged:
7953-
* "parse_unusable": {"files":[{path,lines,whole_file}..(<=50)], "count":N,
7956+
* "parse_unusable": {"files":[{path,range_end,whole_file}..(<=50)], "count":N,
79547957
* "truncated":bool, "note":"..."}
79557958
*
79567959
* These files WERE indexed, exactly like parse_partial ones. The difference is
79577960
* that their range covers 80% or more of the file, so the range is not worth
7958-
* printing — "lines" gives the size and "whole_file" says plainly that reading
7959-
* the ranges is the same as reading the file. */
7961+
* printing — "range_end" gives the last line the range names and "whole_file"
7962+
* says plainly that reading the ranges is the same as reading the file. */
79607963
static void add_parse_unusable_summary(yyjson_mut_doc *doc, yyjson_mut_val *root,
79617964
const cbm_file_error_t *errs, int count) {
79627965
int unusable = 0;
@@ -7979,9 +7982,11 @@ static void add_parse_unusable_summary(yyjson_mut_doc *doc, yyjson_mut_val *root
79797982
yyjson_mut_val *fe = yyjson_mut_obj(doc);
79807983
yyjson_mut_obj_add_strcpy(doc, fe, "path", errs[i].path ? errs[i].path : "");
79817984
yyjson_mut_obj_add_bool(doc, fe, "whole_file", true);
7982-
/* The range string is "start-end"; its end line is the file length. */
7985+
/* The end of the range, not the length of the file. A grammar can end
7986+
* an error node past the last line, so this number can be larger than
7987+
* the file. See range_end_is_not_file_length. */
79837988
const char *dash = errs[i].reason ? strchr(errs[i].reason, '-') : NULL;
7984-
yyjson_mut_obj_add_int(doc, fe, "lines", dash ? atoi(dash + 1) : 0);
7989+
yyjson_mut_obj_add_int(doc, fe, "range_end", dash ? atoi(dash + 1) : 0);
79857990
yyjson_mut_arr_add_val(files, fe);
79867991
shown++;
79877992
}

tests/test_index_resilience.c

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -488,6 +488,94 @@ TEST(index_parse_partial_reported) {
488488
PASS();
489489
}
490490

491+
/* The whole-file class as index_status prints it, and what its number means.
492+
*
493+
* Each parse_unusable entry reports the END of the file's one range. The field
494+
* was called "lines", which reads as the length of the file, and the two are
495+
* not the same number — a grammar can end an error node past the last line,
496+
* which this repo has already seen (a 326-line PowerShell file whose range
497+
* ended at 327). "lines" also already means a definition's line span in the
498+
* rest of this response, so the old name collided as well.
499+
*
500+
* The end line is checked against the persisted coverage row rather than a
501+
* constant, so the test states the property and not a measurement. */
502+
TEST(index_parse_unusable_names_the_range_end) {
503+
RProj lp;
504+
memset(&lp, 0, sizeof(lp));
505+
snprintf(lp.tmpdir, sizeof(lp.tmpdir), "/tmp/cbm_resil_XXXXXX");
506+
if (!cbm_mkdtemp(lp.tmpdir)) {
507+
FAIL("mkdtemp failed");
508+
}
509+
rh_to_fwd_slashes(lp.tmpdir);
510+
511+
/* Python gets no C preprocessor refinement, so a root-level ERROR still
512+
* reports one range over the whole file — the parse_unusable class. */
513+
ri_write_text(lp.tmpdir, "unparseable.py", ")))\n((( \n]]] [[[\ndef x(:\n");
514+
ri_write_text(lp.tmpdir, "good.py", "def alpha():\n return 1\n");
515+
516+
char *resp = NULL;
517+
cbm_store_t *store = ri_index_capture(&lp, &resp);
518+
if (!resp) {
519+
FAIL("no MCP response");
520+
}
521+
if (!store) {
522+
free(resp);
523+
FAIL("store did not open");
524+
}
525+
526+
/* The end line the report should be naming, read from the persisted row. */
527+
cbm_coverage_row_t *rows = NULL;
528+
int cov_count = 0;
529+
ASSERT_EQ(cbm_store_coverage_get(store, lp.project, &rows, &cov_count), CBM_STORE_OK);
530+
int want_end = 0;
531+
for (int i = 0; i < cov_count; i++) {
532+
if (rows[i].rel_path && strstr(rows[i].rel_path, "unparseable.py") && rows[i].detail) {
533+
const char *dash = strchr(rows[i].detail, '-');
534+
if (dash) {
535+
want_end = atoi(dash + 1);
536+
}
537+
}
538+
}
539+
cbm_store_free_coverage(rows, cov_count);
540+
ASSERT_GT(want_end, 0);
541+
542+
yyjson_doc *d = yyjson_read(resp, strlen(resp), 0);
543+
ASSERT_NOT_NULL(d);
544+
yyjson_val *sc = yyjson_obj_get(yyjson_doc_get_root(d), "structuredContent");
545+
ASSERT_NOT_NULL(sc);
546+
ASSERT_EQ(yyjson_get_int(yyjson_obj_get(sc, "parse_unusable_count")), 1);
547+
548+
yyjson_val *pu = yyjson_obj_get(sc, "parse_unusable");
549+
ASSERT_NOT_NULL(pu);
550+
yyjson_val *files = yyjson_obj_get(pu, "files");
551+
ASSERT_NOT_NULL(files);
552+
int found = 0;
553+
size_t idx = 0;
554+
size_t fmax = 0;
555+
yyjson_val *fe = NULL;
556+
yyjson_arr_foreach(files, idx, fmax, fe) {
557+
const char *fp = yyjson_get_str(yyjson_obj_get(fe, "path"));
558+
/* The clean neighbour must not be listed at all. */
559+
ASSERT_NULL(fp ? strstr(fp, "good.py") : NULL);
560+
if (!fp || !strstr(fp, "unparseable.py")) {
561+
continue;
562+
}
563+
found = 1;
564+
yyjson_val *range_end = yyjson_obj_get(fe, "range_end");
565+
ASSERT_NOT_NULL(range_end);
566+
ASSERT_EQ(yyjson_get_int(range_end), want_end);
567+
ASSERT_TRUE(yyjson_get_bool(yyjson_obj_get(fe, "whole_file")));
568+
/* The old name is gone, not kept beside the new one. */
569+
ASSERT_NULL(yyjson_obj_get(fe, "lines"));
570+
}
571+
ASSERT_TRUE(found);
572+
573+
yyjson_doc_free(d);
574+
free(resp);
575+
rh_cleanup(&lp, store);
576+
PASS();
577+
}
578+
491579
/* INV(parse-partial-clears-on-fix, #963): the persisted coverage signal must
492580
* stay FRESH — after the broken file is fixed and the project re-indexed
493581
* (incremental route: the DB already exists), its parse_partial row is gone
@@ -812,6 +900,7 @@ SUITE(index_resilience) {
812900
RUN_TEST(index_clean_run_no_logfile);
813901
RUN_TEST(index_parse_partial_reported);
814902
RUN_TEST(index_parse_partial_clears_on_fix);
903+
RUN_TEST(index_parse_unusable_names_the_range_end);
815904
RUN_TEST(index_not_indexed_by_design_reported);
816905
RUN_TEST(index_relative_repo_path_canonicalized);
817906
}

0 commit comments

Comments
 (0)