From 59e3c9640553163284e6ea65f64f37731eb67420 Mon Sep 17 00:00:00 2001 From: TMHSDigital <154358121+TMHSDigital@users.noreply.github.com> Date: Wed, 23 Sep 2026 16:41:13 -0400 Subject: [PATCH] fix(report): refuse mixed datasets unless asked, and keep secrets and paths out of what is shared #43: plumbline report took the dataset identity from the first artifact and rendered every arm under it, so runs over different datasets printed as if comparable ("against dataset 02496ef4, 1 rows, 6 rows"), and two runs of one adapter got identical "### mock" headings. Different datasets are now refused with the hashes named, unless --allow-mixed is passed; then the header says how many datasets there are and that they are not comparable, and every arm names its dataset and row count. Arms sharing an adapter name are told apart by model, then by run time. #46: - redact walked dicts only, so {"endpoints": [{"api_key": ...}]} reached the artifact. It now walks lists and tuples. - The report and the artifact named the dataset by the path as given, so an absolute path shared a username and directory layout. A dataset inside the working directory is named relative to it, one outside by its file name; the dataset hash still identifies the rows. - A backtick in --model ended its code span early. Values in code spans use a longer fence when they contain one. Fixes #43. Fixes #46. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 12 +++++ src/plumbline/cli.py | 43 +++++++++++++---- src/plumbline/report/markdown.py | 81 +++++++++++++++++++++++++++++--- src/plumbline/runner/execute.py | 16 +++++-- tests/test_cli.py | 40 ++++++++++++++++ tests/test_report.py | 41 ++++++++++++++++ tests/test_runner.py | 15 ++++++ 7 files changed, 230 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c4fd08..8e496e5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -83,6 +83,18 @@ different event from one that moved because it was wrong. ### Fixed +- `plumbline report` combined artifacts from different datasets under the first + one's hash, as if their figures were comparable, and gave two runs of one + adapter identical headings (#43). Different datasets are now refused unless + `--allow-mixed` is passed, when every arm names its dataset and row count; + arms that share an adapter name are told apart by model, then by time. +- A credential inside a list in the run config reached the artifact, because + redaction walked only dictionaries; it now walks lists and tuples too (#46). + The report and the artifact named the dataset by the path as typed, so an + absolute path shared a username and a directory layout; a dataset inside the + working directory is named relative to it and one outside by its file name, + and the hash still says which rows they were. A backtick in a model name no + longer breaks out of its code span. - The JevBench loader skipped the duplicate-id check the JSONL loader makes, so repeated ids loaded silently (#44). Both loaders now share it. The dataset loader read a byte order mark as part of the first row and refused it, turned diff --git a/src/plumbline/cli.py b/src/plumbline/cli.py index e89de55..dca08e7 100644 --- a/src/plumbline/cli.py +++ b/src/plumbline/cli.py @@ -13,6 +13,7 @@ from __future__ import annotations +import dataclasses import sys from collections.abc import Callable from functools import partial @@ -107,7 +108,7 @@ def run( # so a mistake costs nothing. if report is not None and report.is_dir(): _fail(f"--report {report} is a directory; give it a file path, such as {report}/report.md") - load = _load(dataset, data_format) + load = dataclasses.replace(_load(dataset, data_format), source=_shown(dataset)) _status(load.statement()) for refusal in load.refusals: _status(f" refused {refusal}") @@ -135,7 +136,7 @@ def run( guard=execute.CostGuard(max_cost_usd=max_cost_usd, max_cases=max_cases), pricing_table=_pricing(pricing), workers=workers, - extra_config={"dataset": str(dataset), "format": data_format}, + extra_config={"dataset": _shown(dataset), "format": data_format}, ) ) @@ -178,16 +179,26 @@ def report( float | None, typer.Option("--error-cost", help="What one wrong answer costs, in USD.") ] = None, n_boot: Annotated[int, typer.Option("--boot", min=1, help="Bootstrap draws per null.")] = 2000, + allow_mixed: Annotated[ + bool, + typer.Option( + "--allow-mixed", + help="Allow runs over different datasets in one document; each arm names its own.", + ), + ] = False, ) -> None: """Render one document from runs that already happened.""" results = [_guard(partial(execute.RunResult.read, path)) for path in artifacts] - document = markdown.render( - results, - options=markdown.ReportOptions( - n_boot=n_boot, - cost_escalation_usd=escalation_cost, - cost_error_usd=error_cost, - ), + document = _guard( + lambda: markdown.render( + results, + options=markdown.ReportOptions( + n_boot=n_boot, + cost_escalation_usd=escalation_cost, + cost_error_usd=error_cost, + allow_mixed_datasets=allow_mixed, + ), + ) ) if out is not None: out.parent.mkdir(parents=True, exist_ok=True) @@ -215,6 +226,20 @@ def version() -> None: typer.echo(__version__) +def _shown(dataset: Path) -> str: + """The dataset as a report or an artifact names it. + + Relative to the working directory when it is inside it, as the caller most + likely typed it; otherwise the file name alone. An absolute path in a report + that gets shared names the user and their directory layout, and the dataset + hash already says which rows these were. + """ + try: + return str(dataset.resolve().relative_to(Path.cwd().resolve())) + except ValueError: + return dataset.name + + def _load(dataset: Path, data_format: str) -> loader.LoadReport: if data_format not in FORMATS: _fail(f"--format must be one of {list(FORMATS)!r}, got {data_format!r}") diff --git a/src/plumbline/report/markdown.py b/src/plumbline/report/markdown.py index 67327e2..6cc8ccf 100644 --- a/src/plumbline/report/markdown.py +++ b/src/plumbline/report/markdown.py @@ -91,6 +91,13 @@ class ReportOptions: min_threshold_rows: int = 200 """Below this, a threshold is fitted to noise and none is printed.""" + allow_mixed_datasets: bool = False + """Put runs over different datasets in one document, each arm naming its own. + + Off by default: figures from different datasets are not comparable, and a + document that lays them side by side invites exactly that comparison. + """ + @property def has_costs(self) -> bool: return self.cost_escalation_usd is not None and self.cost_error_usd is not None @@ -111,6 +118,16 @@ def render( if not results: raise ValueError("no runs to report on") options = options or ReportOptions() + datasets = sorted({result.dataset_hash for result in results}) + if len(datasets) > 1 and not options.allow_mixed_datasets: + named = ", ".join(f"`{digest[:8]}`" for digest in datasets) + raise ValueError( + f"these runs are over {len(datasets)} different datasets ({named}), and figures " + "from different datasets are not comparable. Report them separately, or allow " + "mixed datasets (--allow-mixed) to put them in one document with each arm " + "naming its dataset." + ) + headings = _headings(results, mixed=len(datasets) > 1) lines = _header(results, load, options) lines.extend(_how_to_read()) @@ -123,7 +140,7 @@ def render( continue lines.extend(["", "---", "", f"## {GROUP_TITLES[semantics]}", "", GROUP_NOTES[semantics]]) for result in group: - lines.extend(_arm(result, options)) + lines.extend(_arm(result, options, headings[id(result)])) return "\n".join(lines).rstrip() + "\n" @@ -132,6 +149,17 @@ def _header( results: Sequence[RunResult], load: LoadReport | None, options: ReportOptions ) -> list[str]: first = results[0] + datasets = sorted({result.dataset_hash for result in results}) + if len(datasets) > 1: + named = ", ".join(f"`{digest[:8]}`" for digest in datasets) + return [ + "# plumbline report", + "", + f"Generated {options.clock.isoformat()} against {len(datasets)} datasets " + f"({named}). {len(results)} arm(s), each naming its own dataset and row " + "count. Figures from different datasets are not comparable.", + "", + ] rows = ", ".join(sorted({f"{result.dataset_rows} rows" for result in results})) return [ "# plumbline report", @@ -142,6 +170,33 @@ def _header( ] +def _headings(results: Sequence[RunResult], *, mixed: bool) -> dict[int, str]: + """A heading per arm that tells it apart from every other. + + The adapter name alone, when it is unique; then the model; then the run's + time. With mixed datasets every arm also names its dataset and row count. + """ + + def label(result: RunResult, depth: int) -> str: + parts = [result.adapter_name] + if depth >= 1: + parts.append(result.model_requested) + if depth >= 2: + parts.append(result.timestamp) + return ", ".join(parts) + + chosen: dict[int, str] = {} + for result in results: + for depth in range(3): + candidate = label(result, depth) + if sum(1 for other in results if label(other, depth) == candidate) == 1 or depth == 2: + break + if mixed: + candidate += f", dataset `{result.dataset_hash[:8]}` ({result.dataset_rows} rows)" + chosen[id(result)] = candidate + return chosen + + def _how_to_read() -> list[str]: return [ "## How to read this", @@ -171,7 +226,7 @@ def _dataset_section(load: LoadReport) -> list[str]: return lines -def _arm(result: RunResult, options: ReportOptions) -> list[str]: +def _arm(result: RunResult, options: ReportOptions, heading: str) -> list[str]: scoreable = [ record for record in result.records if record.question_type in SUPPORTED_QUESTION_TYPES ] @@ -179,7 +234,7 @@ def _arm(result: RunResult, options: ReportOptions) -> list[str]: successes = [record for record in scoreable if record.prediction is not None] failures = [record for record in scoreable if record.prediction is None] - lines = ["", f"### {result.adapter_name}", "", *_provenance(result, options)] + lines = ["", f"### {heading}", "", *_provenance(result, options)] if excluded: lines.append( f"- **Excluded**: {excluded} rows of an unsupported question type were not scored." @@ -492,10 +547,11 @@ def _scaled(prediction: Prediction, temperature: float) -> float: def _provenance(result: RunResult, options: ReportOptions) -> list[str]: reported = result.model_reported or "not reported" + endpoint = result.config.get("endpoint") line = ( - f"- **Model**: requested `{result.model_requested}`, reported `{reported}`" - + (f", revision `{result.revision}`" if result.revision else "") - + (f", endpoint `{endpoint}`" if (endpoint := result.config.get("endpoint")) else "") + f"- **Model**: requested {_code(result.model_requested)}, reported {_code(reported)}" + + (f", revision {_code(result.revision)}" if result.revision else "") + + (f", endpoint {_code(str(endpoint))}" if endpoint else "") + "." ) hits = result.cache_stats.get("hits", 0) @@ -504,6 +560,19 @@ def _provenance(result: RunResult, options: ReportOptions) -> list[str]: return [line] +def _code(value: str) -> str: + """A markdown code span that holds ``value`` whatever it contains. + + A model string is whatever the caller passed, and a backtick inside a single + backtick span ends the span early. Markdown allows a longer fence: two + backticks with a space inside each, which a single backtick cannot close. + """ + if "`" not in value: + return f"`{value}`" + fence = "``" if "``" not in value else "```" + return f"{fence} {value} {fence}" + + def _asked_as(records: Sequence[CaseRecord]) -> list[str]: """State how each kind of row was actually asked, and flag any mismatch.""" pairs: dict[tuple[str, str], int] = {} diff --git a/src/plumbline/runner/execute.py b/src/plumbline/runner/execute.py index 4b78336..2cc63a2 100644 --- a/src/plumbline/runner/execute.py +++ b/src/plumbline/runner/execute.py @@ -450,13 +450,23 @@ def redact(config: Mapping[str, Any]) -> dict[str, Any]: for key, value in config.items(): if any(marker in key.lower() for marker in _SECRET_MARKERS): cleaned[key] = "[redacted]" - elif isinstance(value, Mapping): - cleaned[key] = redact(value) else: - cleaned[key] = value + cleaned[key] = _redacted(value) return cleaned +def _redacted(value: Any) -> Any: + """A value with every mapping inside it redacted, through lists and tuples too. + + A key inside a list of endpoints is as much a secret as one at the top level. + """ + if isinstance(value, Mapping): + return redact(value) + if isinstance(value, list | tuple): + return [_redacted(item) for item in value] + return value + + def estimate_run_cost( cases: Sequence[Case], pricing: Pricing | None, diff --git a/tests/test_cli.py b/tests/test_cli.py index 5118979..c23f889 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -348,3 +348,43 @@ def test_a_run_where_every_case_fails_exits_non_zero_and_says_why(tmp_path: Path assert "every case failed" in result.stderr.lower() assert "accuracy" in result.stderr assert "for the same reason" in report.read_text(encoding="utf-8") + + +def test_a_dataset_outside_the_working_directory_is_named_not_located(tmp_path: Path) -> None: + """An absolute path in a shared report names the user and their layout (#46).""" + dataset = a_dataset(tmp_path / "d.jsonl", n_rows=12) + report = tmp_path / "report.md" + result = invoke( + "run", + str(dataset), + "--results", + str(tmp_path / "r"), + "--report", + str(report), + "--boot", + "100", + ) + + assert result.exit_code == 0 + assert str(tmp_path) not in report.read_text(encoding="utf-8") + artifact = json.loads(next((tmp_path / "r").glob("*.json")).read_text(encoding="utf-8")) + assert artifact["config"]["dataset"] == "d.jsonl" + + +def test_report_refuses_mixed_datasets_unless_allowed(tmp_path: Path) -> None: + for name, rows in (("a", 12), ("b", 13)): + invoke( + "run", + str(a_dataset(tmp_path / f"{name}.jsonl", n_rows=rows)), + "--results", + str(tmp_path / "r"), + "--boot", + "100", + ) + artifacts = [str(path) for path in sorted((tmp_path / "r").glob("*.json"))] + + refused = invoke("report", *artifacts, "--boot", "100") + allowed = invoke("report", *artifacts, "--boot", "100", "--allow-mixed") + + assert refused.exit_code == 1 and "different datasets" in refused.output + assert allowed.exit_code == 0 and "2 datasets" in allowed.stdout diff --git a/tests/test_report.py b/tests/test_report.py index 88bc3f6..3ffe107 100644 --- a/tests/test_report.py +++ b/tests/test_report.py @@ -471,3 +471,44 @@ def test_a_run_with_no_figures_names_a_shared_failure_reason(same: bool) -> None else: assert "8 different reasons, are in the artifact" in text assert "broke on" not in text + + +# Which runs belong in one document (#43), and what a heading or a code span can hold (#46) + + +def test_artifacts_from_different_datasets_are_refused_in_one_report() -> None: + first, second = a_run(n_cases=40), a_run(n_cases=41) + assert first.dataset_hash != second.dataset_hash + + with pytest.raises(ValueError, match="different datasets"): + render(first, second) + + +def test_mixed_datasets_can_be_allowed_and_then_every_arm_names_its_own() -> None: + first, second = a_run(n_cases=40), a_run(n_cases=41) + options = markdown.ReportOptions(n_boot=200, today=date(2026, 9, 20), allow_mixed_datasets=True) + + text = markdown.render([first, second], options=options) + + assert "2 datasets" in text and "not comparable" in text + assert f"dataset `{first.dataset_hash[:8]}`" in text + assert f"dataset `{second.dataset_hash[:8]}`" in text + + +def test_two_arms_with_one_adapter_name_get_headings_that_tell_them_apart() -> None: + cases = make_cases(40, labels=LABELS) + fast = execute.run(MockAdapter(gold_by_text(cases), seed=5, model_requested="mock-fast"), cases) + slow = execute.run(MockAdapter(gold_by_text(cases), seed=5, model_requested="mock-slow"), cases) + + headings = [line for line in render(fast, slow).splitlines() if line.startswith("### ")] + + assert len(headings) == len(set(headings)) == 2 + + +def test_a_backtick_in_a_model_name_cannot_break_out_of_its_code_span() -> None: + cases = make_cases(40, labels=LABELS) + odd = execute.run(MockAdapter(gold_by_text(cases), seed=5, model_requested="we`ird"), cases) + + line = next(line for line in render(odd).splitlines() if line.startswith("- **Model**")) + + assert "`` we`ird ``" in line diff --git a/tests/test_runner.py b/tests/test_runner.py index ae92f7e..9529e96 100644 --- a/tests/test_runner.py +++ b/tests/test_runner.py @@ -796,3 +796,18 @@ def test_a_second_run_never_overwrites_the_first_artifact(tmp_path: Path) -> Non assert len(list(directory.glob("*.json"))) == 2 assert execute.RunResult.read(first).probability_semantics == "calibrated_claim" assert execute.RunResult.read(second).probability_semantics == "restricted_softmax" + + +def test_a_credential_inside_a_list_is_redacted_too() -> None: + """redact walked dicts only, so a key inside a list of endpoints reached disk (#46).""" + config = { + "endpoints": [{"api_key": "sk-live-123", "url": "https://a"}], + "pair": ({"token": "t"},), + } + + cleaned = execute.redact(config) + + assert cleaned["endpoints"][0]["api_key"] == "[redacted]" + assert cleaned["endpoints"][0]["url"] == "https://a" + assert cleaned["pair"][0]["token"] == "[redacted]" + assert "sk-live-123" not in json.dumps(cleaned)