From f725a341885427201a968e634ebb2480f29e5a7a Mon Sep 17 00:00:00 2001 From: dxnnystyles Date: Sat, 24 Oct 2026 17:56:03 -0700 Subject: [PATCH] fix: prevent subset benchmarks from overwriting other results --- faircode/benchmark.py | 82 +++++++++++++++++++++ faircode/cli.py | 3 + tests/test_benchmark.py | 3 +- tests/test_results_provenance.py | 123 +++++++++++++++++++++++++++++++ 4 files changed, 209 insertions(+), 2 deletions(-) diff --git a/faircode/benchmark.py b/faircode/benchmark.py index d03de17b..500dce10 100644 --- a/faircode/benchmark.py +++ b/faircode/benchmark.py @@ -368,11 +368,93 @@ def write_provenance(out_dir, runs) -> Path: return path +def _validate_existing_result_outputs(out_dir, requested_audits, provenance) -> None: + """Validate existing results before report writes to prevent partial replacement.""" + out_dir = Path(out_dir) + paths = {name: out_dir / name for name in RESULT_FILES} + provenance_path = out_dir / PROVENANCE_FILE + if not any(path.exists() for path in paths.values()) and not provenance_path.exists(): + return + + missing = [name for name in RESULT_FILES if not paths[name].exists()] + if missing: + raise ValueError( + "existing benchmark output is incomplete (missing " + ", ".join(missing) + + "); refusing to overwrite it. Restore the files or choose a new --out directory." + ) + + audit_sets = {} + for name, path in paths.items(): + try: + table = pd.read_csv(path) + except (OSError, UnicodeError, ValueError, pd.errors.ParserError, pd.errors.EmptyDataError) as exc: + raise ValueError( + f"existing benchmark output is malformed ({name}: {exc}); refusing to overwrite it. " + "Restore the file or choose a new --out directory." + ) from exc + if table.empty or "audit" not in table or table["audit"].isna().any(): + raise ValueError( + f"existing benchmark output is malformed ({name} has no usable audit rows); " + "refusing to overwrite it. Restore the file or choose a new --out directory." + ) + audit_sets[name] = set(table["audit"].astype(str)) + + existing_audits = audit_sets[RESULT_FILES[0]] + if any(audits != existing_audits for audits in audit_sets.values()): + raise ValueError( + "existing benchmark result files contain different audit sets; refusing to overwrite them. " + "Restore a consistent result set or choose a new --out directory." + ) + + if provenance_path.exists(): + try: + record = json.loads(provenance_path.read_text(encoding="utf-8")) + if not isinstance(record, dict): + raise ValueError("provenance is not a JSON object") + if record.get("schema") != 1 or not isinstance(record.get("files"), dict): + raise ValueError("unsupported or incomplete provenance record") + if not isinstance(record.get("runs"), list): + raise ValueError("missing run records") + for name, path in paths.items(): + if record["files"].get(name) != _sha256(path): + raise ValueError(f"{name} does not match its recorded hash") + recorded_audits = set() + for run in record["runs"]: + if not isinstance(run, dict) or not isinstance(run.get("audits"), list): + raise ValueError("malformed run audit list") + recorded_audits.update(run["audits"]) + if recorded_audits != existing_audits: + raise ValueError("recorded audits do not match the result files") + except (OSError, UnicodeError, json.JSONDecodeError, ValueError, TypeError) as exc: + raise ValueError( + f"existing benchmark provenance is malformed or inconsistent ({exc}); " + "refusing to overwrite it. Restore the files or choose a new --out directory." + ) from exc + if provenance is None: + raise ValueError( + "existing benchmark output has provenance but this report does not; " + "refusing to leave stale provenance. Supply provenance or choose a new --out directory." + ) + + other_audits = existing_audits - requested_audits + if other_audits: + names = ", ".join(sorted(other_audits)) + raise ValueError( + f"existing benchmark output contains results for other audits ({names}); " + "refusing to overwrite them. Run all audits or choose a new --out directory." + ) + + def write_report(fairness_df: pd.DataFrame, performance_df: pd.DataFrame, out_dir, make_plots: bool = True, plot_metric: str = "demographic_parity_diff", provenance=None): """Write results_fairness.csv, results_performance.csv, and a summary.csv to out_dir, plus (if make_plots) render figures/*.png via faircode.figures. With `provenance` (a dict from collect_provenance) also write PROVENANCE.json (#1129).""" + fairness_audits = set(fairness_df["audit"].astype(str)) if "audit" in fairness_df else set() + performance_audits = set(performance_df["audit"].astype(str)) if "audit" in performance_df else set() + if fairness_audits != performance_audits: + raise ValueError("benchmark result tables contain different audit sets; refusing to write output") + _validate_existing_result_outputs(out_dir, fairness_audits, provenance) out_dir = Path(out_dir) out_dir.mkdir(parents=True, exist_ok=True) fairness_df.to_csv(out_dir / "results_fairness.csv", index=False) diff --git a/faircode/cli.py b/faircode/cli.py index 660cac3c..cebb3560 100644 --- a/faircode/cli.py +++ b/faircode/cli.py @@ -927,6 +927,9 @@ def _held_out_uses_stdin(specs): write_report(fairness_df, performance_df, args.out, make_plots=not args.no_plots, provenance=collect_provenance(fairness_df["audit"].unique(), args.n_resamples, args.n_permutations, description="faircode benchmark")) + except ValueError as exc: + print(f"error: {exc}", file=sys.stderr) + return 2 except ImportError as exc: print(f"error: writing benchmark plots needs matplotlib " f"(pip install faircode[benchmark]): {exc}", file=sys.stderr) diff --git a/tests/test_benchmark.py b/tests/test_benchmark.py index b840cef6..0a173043 100644 --- a/tests/test_benchmark.py +++ b/tests/test_benchmark.py @@ -106,8 +106,7 @@ def test_baseline_demographic_parity_gap_is_a_plausible_fraction(result): # ── write_report() / --no-plots (#210) ────────────────────────────────────── -# write_report() itself is frozen (CLAUDE.md) - these only call it, they never -# modify faircode/benchmark.py. +# These exercise report writing without running the full seven-audit sweep. @pytest.fixture(scope="module") def result_dfs(result): diff --git a/tests/test_results_provenance.py b/tests/test_results_provenance.py index 1d53b402..017475bc 100644 --- a/tests/test_results_provenance.py +++ b/tests/test_results_provenance.py @@ -50,6 +50,129 @@ def test_write_report_without_provenance_writes_no_file(tmp_path): assert not (tmp_path / "PROVENANCE.json").exists() +def _result_frames(audits): + fairness = pd.DataFrame({ + "audit": audits, + "strategy": ["baseline"] * len(audits), + "protected_attribute": ["age"] * len(audits), + "metric": ["demographic_parity_diff"] * len(audits), + "value": [0.1] * len(audits), + "significant": [False] * len(audits), + "model": ["logistic_regression"] * len(audits), + }) + performance = pd.DataFrame({ + "audit": audits, + "strategy": ["baseline"] * len(audits), + "model": ["logistic_regression"] * len(audits), + "metric": ["accuracy"] * len(audits), + "value": [0.7] * len(audits), + }) + return fairness, performance + + +def _write_result_set(out_dir, audits, *, provenance=True): + fairness, performance = _result_frames(audits) + run = {"audits": list(audits)} if provenance else None + return benchmark.write_report(fairness, performance, out_dir, make_plots=False, provenance=run) + + +def _output_bytes(out_dir): + names = (*benchmark.RESULT_FILES, benchmark.PROVENANCE_FILE) + return {name: (out_dir / name).read_bytes() for name in names if (out_dir / name).exists()} + + +def test_subset_run_refuses_other_audits_without_changing_any_output(tmp_path): + out_dir = tmp_path / "results" + _write_result_set(out_dir, ["audit_a", "audit_b"]) + before = _output_bytes(out_dir) + fairness, performance = _result_frames(["audit_b"]) + + with pytest.raises(ValueError, match="contains results for other audits.*audit_a"): + benchmark.write_report( + fairness, performance, out_dir, make_plots=False, provenance={"audits": ["audit_b"]} + ) + + assert _output_bytes(out_dir) == before + + +def test_fresh_and_compatible_same_audit_outputs_can_be_written(tmp_path): + out_dir = tmp_path / "results" + _write_result_set(out_dir, ["audit_a"]) + before = _output_bytes(out_dir) + fairness, performance = _result_frames(["audit_a"]) + fairness.loc[0, "value"] = 0.4 + + benchmark.write_report( + fairness, performance, out_dir, make_plots=False, provenance={"audits": ["audit_a"]} + ) + + after = _output_bytes(out_dir) + assert after.keys() == before.keys() + assert after["results_fairness.csv"] != before["results_fairness.csv"] + assert json.loads(after["PROVENANCE.json"])["runs"] == [{"audits": ["audit_a"]}] + + +def test_complete_legacy_outputs_allow_a_compatible_rerun_without_provenance(tmp_path): + out_dir = tmp_path / "results" + _write_result_set(out_dir, ["audit_a"], provenance=False) + fairness, performance = _result_frames(["audit_a"]) + + benchmark.write_report(fairness, performance, out_dir, make_plots=False) + + assert set(_output_bytes(out_dir)) == set(benchmark.RESULT_FILES) + + +@pytest.mark.parametrize("broken", ["missing", "empty", "no-audit-column", "inconsistent-audits"]) +def test_incomplete_or_malformed_outputs_are_not_overwritten(tmp_path, broken): + out_dir = tmp_path / "results" + _write_result_set(out_dir, ["audit_a", "audit_b"], provenance=False) + if broken == "missing": + (out_dir / "summary.csv").unlink() + elif broken == "empty": + (out_dir / "results_performance.csv").write_text("", encoding="utf-8") + elif broken == "no-audit-column": + (out_dir / "results_performance.csv").write_text("metric,value\naccuracy,0.7\n", encoding="utf-8") + else: + (out_dir / "summary.csv").write_text("audit\naudit_c\n", encoding="utf-8") + before = _output_bytes(out_dir) + fairness, performance = _result_frames(["audit_b"]) + + with pytest.raises(ValueError): + benchmark.write_report(fairness, performance, out_dir, make_plots=False) + + assert _output_bytes(out_dir) == before + + +def test_malformed_or_stale_provenance_is_not_overwritten(tmp_path): + out_dir = tmp_path / "results" + _write_result_set(out_dir, ["audit_a", "audit_b"]) + (out_dir / benchmark.PROVENANCE_FILE).write_text("{broken", encoding="utf-8") + before = _output_bytes(out_dir) + fairness, performance = _result_frames(["audit_b"]) + + with pytest.raises(ValueError, match="provenance is malformed or inconsistent"): + benchmark.write_report( + fairness, performance, out_dir, make_plots=False, provenance={"audits": ["audit_b"]} + ) + + assert _output_bytes(out_dir) == before + + +def test_benchmark_cli_reports_a_refused_subset_run_cleanly(tmp_path, monkeypatch, capsys): + out_dir = tmp_path / "results" + _write_result_set(out_dir, ["audit_a", "audit_b"]) + before = _output_bytes(out_dir) + fairness, performance = _result_frames(["audit_b"]) + monkeypatch.setattr(benchmark, "run_benchmark", lambda **_: (fairness, performance)) + from faircode.cli import main + + status = main(["benchmark", "--no-plots", "--out", str(out_dir), "audit_b/audit.yaml"]) + + assert status == 2 + assert "refusing to overwrite" in capsys.readouterr().err + assert _output_bytes(out_dir) == before + + def test_the_committed_provenance_matches_the_committed_result_files(): record = json.loads((REPO_ROOT / "results" / "PROVENANCE.json").read_text(encoding="utf-8")) assert record["runs"], "results/PROVENANCE.json lists no run"