fix(null_calibration): close SonarCloud follow-up from #267

.github/workflows/null-calibration-comparators.yml

@@ -243,7 +243,7 @@ jobs:
           shopt -s nullglob
           for csv in bench/null_calibration/results/raw/pvalues_*.csv; do
             cell="$(basename "$csv" .csv)"
-            python3 bench/null_calibration/report_failures.py "$csv" "$cell"
+            python3 bench/null_calibration/report_failures.py "$cell" < "$csv"
           done
 
       - name: Publish the summaries as annotations

bench/null_calibration/REPORT.md

@@ -176,7 +176,7 @@ The comparator lane completed successfully in [Actions run 40091522843](https://
 
 The run used the final two post-PR fixes from that tag lineage: the loud failure-reason reporting commit `17ce140` and `test_de` `sort_by` fix `bc435fe`. They were cherry-picked onto this work branch as `2599317` and `8784a11`. The measured file digests are recorded from the run itself: `calibration.R` md5 `12a90679f74d2e1a9ab28cec643b5e76`; extracted production `_FIT_R` md5 `cc9efdfdd6e4efe2f9a393ba4d178ce0`.
 
-After measurement, this PR refactored package-result normalization and cell-warning logic in response to Sonar findings, and hardened CSV serialization plus resume validation (schema and cell-key checks) so package error notes remain valid data. The comparator workflow now reads failure rows with a CSV-aware parser. These post-run changes do not alter estimator calls, simulation seeds, or formulas. At this PR revision the working branch's `calibration.R` md5 is `3ed22ba8b04f695d531a45925abbc13f`, not the measured run's `12a90679f74d2e1a9ab28cec643b5e76`; the results below remain tied to the measured tag/code digest above.
+After measurement, this PR refactored package-result normalization and cell-warning logic in response to Sonar findings, and hardened CSV serialization plus resume validation (schema and cell-key checks) so package error notes remain valid data. The comparator workflow now reads failure rows with a CSV-aware parser. These post-run changes do not alter estimator calls, simulation seeds, or formulas. At this PR revision the working branch's `calibration.R` md5 is `d47b0fcc96c19349d9223b0515e1a17f`, not the measured run's `12a90679f74d2e1a9ab28cec643b5e76`; the results below remain tied to the measured tag/code digest above.
 
 | n/group | test | package | summary rows (`n_attempted`) | p-values (failed fits) | α=0.05 (Wilson 95% CI) | verdict |
 |---:|---|---|---:|---:|---|---|

bench/null_calibration/calibration.R

@@ -428,7 +428,7 @@ report_cell_failures <- function(results_file, cell) {
   if (nrow(cell_rows) == 0) return(invisible(NULL))
   for (test_name in unique(cell_rows$test)) {
     test_rows <- cell_rows[cell_rows$test == test_name, , drop = FALSE]
-    if (any(!is.na(test_rows$pvalue))) next
+    if (!all(is.na(test_rows$pvalue))) next
     reasons <- if ("note" %in% names(test_rows)) {
       unique(stats::na.omit(test_rows$note))
     } else {
@@ -443,41 +443,44 @@ report_cell_failures <- function(results_file, cell) {
 
 validate_resume_cell <- function(results_file, provenance_file, cell, cell_key) {
   if (!file.exists(results_file)) return(integer(0))
+  delete_prefix <- "; delete "
+  rerun_suffix <- " before rerunning"
   prev <- tryCatch(read.csv(results_file, stringsAsFactors = FALSE,
                             check.names = FALSE, fill = FALSE),
                    error = function(e) e)
   if (inherits(prev, "error")) {
-    stop("cell ", cell, " has an unreadable CSV (", conditionMessage(prev),
-         "); delete ", results_file, " and ", provenance_file,
-         " before rerunning", call. = FALSE)
+    stop("cell ", cell, " has an unreadable CSV (", conditionMessage(prev), ")",
+         delete_prefix, results_file, " and ", provenance_file, rerun_suffix,
+         call. = FALSE)
   }
   if (!identical(names(prev), RESULT_COLUMNS)) {
     stop("cell ", cell, " has an incompatible CSV schema; expected ",
          paste(RESULT_COLUMNS, collapse = ", "), " but found ",
-         paste(names(prev), collapse = ", "), "; delete ", results_file,
-         " and ", provenance_file, " before rerunning", call. = FALSE)
+         paste(names(prev), collapse = ", "), delete_prefix, results_file,
+         " and ", provenance_file, rerun_suffix, call. = FALSE)
   }
   if (!file.exists(provenance_file)) {
-    stop("cell ", cell, " has rows but no provenance file; delete ",
-         results_file, " before rerunning", call. = FALSE)
+    stop("cell ", cell, " has rows but no provenance file", delete_prefix,
+         results_file, rerun_suffix, call. = FALSE)
   }
   old_key <- tryCatch(
     grep("^cell key:", readLines(provenance_file, warn = FALSE), value = TRUE),
     error = function(e) e)
   if (inherits(old_key, "error")) {
-    stop("cell ", cell, " has unreadable provenance (", conditionMessage(old_key),
-         "); delete ", results_file, " and ", provenance_file,
-         " before rerunning", call. = FALSE)
+    stop("cell ", cell, " has unreadable provenance (", conditionMessage(old_key), ")",
+         delete_prefix, results_file, " and ", provenance_file, rerun_suffix,
+         call. = FALSE)
   }
   if (length(old_key) != 1L) {
-    stop("cell ", cell, " provenance must contain exactly one cell key; delete ",
-         results_file, " and ", provenance_file, " before rerunning", call. = FALSE)
+    stop("cell ", cell, " provenance must contain exactly one cell key",
+         delete_prefix, results_file, " and ", provenance_file, rerun_suffix,
+         call. = FALSE)
   }
   old_key_value <- trimws(sub("^cell key:[ ]*", "", old_key[1]))
   if (!identical(old_key_value, as.character(cell_key))) {
     stop("cell ", cell, " has a mismatched cell key (found ", old_key_value,
-         ", expected ", cell_key, "); delete ", results_file, " and ",
-         provenance_file, " before rerunning", call. = FALSE)
+         ", expected ", cell_key, ")", delete_prefix, results_file, " and ",
+         provenance_file, rerun_suffix, call. = FALSE)
   }
   unique(as.integer(prev$sim[!is.na(prev$sim)]))
 }

bench/null_calibration/report_failures.py

@@ -6,8 +6,11 @@
 from __future__ import annotations
 
 import csv
-from pathlib import Path
+import io
+import re
 import sys
+from pathlib import Path
+from typing import TextIO
 
 RESULT_FIELDS = [
     "truth",
@@ -24,51 +27,65 @@
     "note",
 ]
 LEGACY_FIELDS = RESULT_FIELDS[:-1]
+CELL_LABEL = re.compile(r"[A-Za-z0-9][A-Za-z0-9_.-]*\Z")
 
 
 def _escape_annotation(text: str) -> str:
     """Escape data characters reserved by the GitHub Actions command protocol."""
     return text.replace("%", "%25").replace("\r", "%0D").replace("\n", "%0A")
 
 
-def failure_annotations(csv_path: str | Path, cell: str) -> list[str]:
-    """Return one warning for each distinct non-empty (test, failure-note) pair."""
+def _validate_cell_label(cell: str) -> str:
+    """Keep untrusted cell labels from injecting extra Actions commands."""
+    if not CELL_LABEL.fullmatch(cell):
+        raise ValueError("unsafe cell label for Actions annotation")
+    return cell
+
+
+def failure_annotations(stream: TextIO, cell: str) -> list[str]:
+    """Return warnings for each distinct non-empty (test, failure-note) pair."""
+    cell = _validate_cell_label(cell)
     annotations: list[str] = []
     seen: set[tuple[str, str]] = set()
-    with Path(csv_path).open(encoding="utf-8", newline="") as stream:
-        reader = csv.DictReader(stream)
-        if reader.fieldnames not in (RESULT_FIELDS, LEGACY_FIELDS):
-            raise ValueError(f"unexpected CSV header {reader.fieldnames!r}")
-        for record_number, row in enumerate(reader, start=2):
-            if None in row or any(value is None for value in row.values()):
-                raise ValueError(f"malformed CSV record {record_number}")
-            if row.get("status") != "failed":
-                continue
-            note = row.get("note")
-            if not note or note == "NA":
-                continue
-            key = (row.get("test", ""), note)
-            if key in seen:
-                continue
-            seen.add(key)
-            message = _escape_annotation(f"{key[0]} :: {note}")
-            annotations.append(
-                f"::warning title={cell}: a requested test failed::{message}"
-            )
+    reader = csv.DictReader(stream)
+    if reader.fieldnames not in (RESULT_FIELDS, LEGACY_FIELDS):
+        raise ValueError(f"unexpected CSV header {reader.fieldnames!r}")
+    for record_number, row in enumerate(reader, start=2):
+        if None in row or any(value is None for value in row.values()):
+            raise ValueError(f"malformed CSV record {record_number}")
+        if row.get("status") != "failed":
+            continue
+        note = row.get("note")
+        if not note or note == "NA":
+            continue
+        key = (row.get("test", ""), note)
+        if key in seen:
+            continue
+        seen.add(key)
+        message = _escape_annotation(f"{key[0]} :: {note}")
+        annotations.append(
+            f"::warning title={cell}: a requested test failed::{message}"
+        )
     return annotations
 
 
 def main(argv: list[str] | None = None) -> int:
     args = sys.argv[1:] if argv is None else argv
-    if len(args) != 2:
-        print(f"usage: {Path(sys.argv[0]).name} RESULTS.csv CELL", file=sys.stderr)
+    if len(args) != 1:
+        print(
+            f"usage: {Path(sys.argv[0]).name} CELL < RESULTS.csv",
+            file=sys.stderr,
+        )
         return 2
-    csv_path, cell = args
+    cell = args[0]
     try:
-        for annotation in failure_annotations(csv_path, cell):
+        # The workflow redirects a known result file to stdin. Read the raw
+        # stream with newline="" so csv preserves embedded CR/LF characters.
+        stream = io.TextIOWrapper(sys.stdin.buffer, encoding="utf-8", newline="")
+        for annotation in failure_annotations(stream, cell):
             print(annotation)
-    except (OSError, UnicodeError, csv.Error, ValueError) as error:
-        print(f"{csv_path}: {error}", file=sys.stderr)
+    except (OSError, csv.Error, ValueError) as error:
+        print(f"{cell}: {error}", file=sys.stderr)
         return 2
     return 0
 

scripts/tests/test_null_calibration_failures.py

@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: MPL-2.0
 """CSV parsing and annotation escaping for null-calibration package failures."""
 import csv
+import io
 from pathlib import Path
 import sys
 import tempfile
@@ -20,6 +21,10 @@ def write_rows(self, path, fields, rows):
                 {field: row.get(field) for field in fields} for row in rows
             )
 
+    def annotations_for(self, path, cell):
+        with path.open(encoding="utf-8", newline="") as stream:
+            return failure_annotations(stream, cell)
+
     def row(self, *, test, status, note=None):
         return {
             "truth": "null",
@@ -54,7 +59,7 @@ def test_distinct_failures_are_csv_aware_deduplicated_and_escaped(self):
             )
             cell = "null_original_tss_n4_t40_smoke"
             self.assertEqual(
-                failure_annotations(path, cell),
+                self.annotations_for(path, cell),
                 [
                     f"::warning title={cell}: a requested test failed::duplicate_test :: "
                     'comma, quote " and newline%0Acontinued',
@@ -63,6 +68,11 @@ def test_distinct_failures_are_csv_aware_deduplicated_and_escaped(self):
                 ],
             )
 
+    def test_unsafe_cell_label_is_rejected(self):
+        stream = io.StringIO(",".join(RESULT_FIELDS) + "\n")
+        with self.assertRaisesRegex(ValueError, "unsafe cell label"):
+            failure_annotations(stream, "bad\n::notice")
+
     def test_legacy_header_without_notes_and_missing_note_are_ignored(self):
         with tempfile.TemporaryDirectory() as directory:
             path = Path(directory) / "legacy.csv"
@@ -71,7 +81,7 @@ def test_legacy_header_without_notes_and_missing_note_are_ignored(self):
                 LEGACY_FIELDS,
                 [self.row(test="old_test", status="failed")],
             )
-            self.assertEqual(failure_annotations(path, "legacy_cell"), [])
+            self.assertEqual(self.annotations_for(path, "legacy_cell"), [])
 
     def test_extra_csv_field_is_rejected(self):
         with tempfile.TemporaryDirectory() as directory:
@@ -81,8 +91,9 @@ def test_extra_csv_field_is_rejected(self):
                 + "null,original,tss,4,40,1,1,0,edger_ql,failed,NA,reason,extra\n",
                 encoding="utf-8",
             )
-            with self.assertRaisesRegex(ValueError, "malformed CSV record"):
-                failure_annotations(path, "bad_cell")
+            with path.open(encoding="utf-8", newline="") as stream:
+                with self.assertRaisesRegex(ValueError, "malformed CSV record"):
+                    failure_annotations(stream, "bad_cell")
 
 
 if __name__ == "__main__":

test/unit/test_null_calibration_csv.R

@@ -17,12 +17,14 @@ cell <- "null_original_tss_n4_t40_smoke"
 double_quote <- intToUtf8(34)
 note_multiline <- paste0("comma, quote ", double_quote, " and newline", intToUtf8(10), "continued")
 note_control <- paste0("second reason 50% and newline", intToUtf8(10), "continued")
+duplicate_test <- "duplicate_test"
+failed_status <- "failed"
 rows <- data.frame(
   truth = rep("null", 5), shape = rep("original", 5), offset = rep("tss", 5),
   n_per = rep(4L, 5), taxa = rep(40L, 5), sim = rep(1L, 5),
   taxon = seq_len(5), true_log2fc = rep(0, 5),
-  test = c("duplicate_test", "duplicate_test", "duplicate_test", "missing_note", "ok_test"),
-  status = c("failed", "failed", "failed", "failed", "ok"),
+  test = c(duplicate_test, duplicate_test, duplicate_test, "missing_note", "ok_test"),
+  status = c(failed_status, failed_status, failed_status, failed_status, "ok"),
   pvalue = c(NA_real_, NA_real_, NA_real_, NA_real_, 0.01),
   note = c(note_multiline, note_multiline, note_control, NA_character_, NA_character_),
   stringsAsFactors = FALSE
@@ -45,7 +47,7 @@ stopifnot(nrow(round_trip) == nrow(rows) + 1L,
 # repeated reasons, and still warn when a failed row has no note.
 r_messages <- capture.output(report_cell_failures(csv_path, cell))
 r_log <- paste(r_messages, collapse = "\n")
-stopifnot(sum(grepl(paste0("WARNING ", cell, ": test duplicate_test"),
+stopifnot(sum(grepl(paste0("WARNING ", cell, ": test ", duplicate_test),
                     r_messages, fixed = TRUE)) == 1L,
           grepl(note_multiline, r_log, fixed = TRUE),
           grepl(paste0("WARNING ", cell, ": test missing_note produced no p-value in this cell"),
@@ -65,19 +67,20 @@ stopifnot(nzchar(python))
 reader_script <- "bench/null_calibration/report_failures.py"
 annotations <- suppressWarnings(system2(
   python,
-  args = c(shQuote(reader_script), shQuote(csv_path), shQuote(cell)),
-  stdout = TRUE, stderr = TRUE
+  args = c(shQuote(reader_script), shQuote(cell)),
+  stdout = TRUE, stderr = TRUE, stdin = csv_path
 ))
 reader_status <- attr(annotations, "status")
-stopifnot(is.null(reader_status) || identical(reader_status, 0L))
 expected_annotations <- c(
   paste0("::warning title=", cell,
-         ": a requested test failed::duplicate_test :: comma, quote ", double_quote,
+         ": a requested test failed::", duplicate_test, " :: comma, quote ", double_quote,
          " and newline%0Acontinued"),
   paste0("::warning title=", cell,
-         ": a requested test failed::duplicate_test :: second reason 50%25 and newline%0Acontinued")
+         ": a requested test failed::", duplicate_test,
+         " :: second reason 50%25 and newline%0Acontinued")
 )
-stopifnot(identical(unname(annotations), expected_annotations))
+stopifnot(is.null(reader_status) || identical(reader_status, 0L),
+          identical(unname(annotations), expected_annotations))
 
 # Refuse legacy 11-column CSVs and mismatched provenance before any append.
 legacy <- rows[, RESULT_COLUMNS[-length(RESULT_COLUMNS)], drop = FALSE]
@@ -88,9 +91,9 @@ legacy_error <- tryCatch({
   validate_resume_cell(legacy_path, provenance_path, cell, 123)
   ""
 }, error = function(e) conditionMessage(e))
-stopifnot(grepl("incompatible CSV schema", legacy_error, fixed = TRUE))
+stopifnot(grepl("incompatible CSV schema", legacy_error, fixed = TRUE),
+          identical(validate_resume_cell(csv_path, provenance_path, cell, 123), 1L))
 
-stopifnot(identical(validate_resume_cell(csv_path, provenance_path, cell, 123), 1L))
 writeLines("cell key: 999", provenance_path)
 key_error <- tryCatch({
   validate_resume_cell(csv_path, provenance_path, cell, 123)
@@ -102,8 +105,8 @@ provenance_error <- tryCatch({
   validate_resume_cell(csv_path, provenance_path, cell, 123)
   ""
 }, error = function(e) conditionMessage(e))
-stopifnot(grepl("no provenance file", provenance_error, fixed = TRUE))
-stopifnot(identical(validate_resume_cell(tempfile(), provenance_path, cell, 123), integer(0)))
+stopifnot(grepl("no provenance file", provenance_error, fixed = TRUE),
+          identical(validate_resume_cell(tempfile(), provenance_path, cell, 123), integer(0)))
 unlink(c(csv_path, provenance_path, legacy_path, bad_csv_path))
 
 cat("Null-calibration CSV round-trip, warning reader and resume checks passed\n")