diff --git a/.github/workflows/null-calibration-comparators.yml b/.github/workflows/null-calibration-comparators.yml index 4e57f3e..c4e43f3 100644 --- a/.github/workflows/null-calibration-comparators.yml +++ b/.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 diff --git a/bench/null_calibration/REPORT.md b/bench/null_calibration/REPORT.md index 2627b56..d4da813 100644 --- a/bench/null_calibration/REPORT.md +++ b/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 | |---:|---|---|---:|---:|---|---| diff --git a/bench/null_calibration/calibration.R b/bench/null_calibration/calibration.R index 767c1d6..857cdf9 100644 --- a/bench/null_calibration/calibration.R +++ b/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)])) } diff --git a/bench/null_calibration/report_failures.py b/bench/null_calibration/report_failures.py index c62d4c3..bc3e62f 100644 --- a/bench/null_calibration/report_failures.py +++ b/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,6 +27,7 @@ "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: @@ -31,44 +35,57 @@ def _escape_annotation(text: str) -> str: 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 diff --git a/scripts/tests/test_null_calibration_failures.py b/scripts/tests/test_null_calibration_failures.py index 69b9c92..00209c2 100644 --- a/scripts/tests/test_null_calibration_failures.py +++ b/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__": diff --git a/test/unit/test_null_calibration_csv.R b/test/unit/test_null_calibration_csv.R index 6f9f707..6c6882b 100644 --- a/test/unit/test_null_calibration_csv.R +++ b/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")