diff --git a/src/benchflow/cli/_failure_evidence.py b/src/benchflow/cli/_failure_evidence.py index 72b28bfb..198231d0 100644 --- a/src/benchflow/cli/_failure_evidence.py +++ b/src/benchflow/cli/_failure_evidence.py @@ -21,7 +21,7 @@ import json import re -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, NamedTuple if TYPE_CHECKING: from collections.abc import Callable @@ -34,46 +34,91 @@ _PYTEST_SUMMARY_RE = re.compile(r"\d+ failed\b") -def _ctrf_failure_line(ctrf_path: Path) -> str | None: +class FailureLine(NamedTuple): + """A failure one-liner in two parts with distinct truncation contracts. + + ``body`` is the free-text evidence (test name plus assertion) and competes + for the display line's char budget; ``suffix`` is the compact multi-failure + roll-up count (empty when the evidence covers everything) and must survive + display truncation. Renderers give ``body`` the full line budget and append + ``suffix`` whole past it — never truncating the concatenation, or a long + assertion would silently eat the "there is more broken than this" signal. + """ + + body: str + suffix: str = "" + + +def _display_test_name(raw_name: str) -> str: + """Test name for display: node-id path segments dropped, param id kept. + + Full pytest node ids ("tests/test_x.py::test_build[param]") waste the + 100-char line budget; keep the function name plus any ``[param]`` id. + The split must not touch the bracket part — a param id may itself contain + ``::`` — so only the text before the first ``[`` is segment-split. (When + the report was written by pytest-json-ctrf — as of 0.5.x — the param id + is already gone — the plugin stores ``nodeid.split('[')[0]`` as ``name`` + — but other CTRF producers keep the full name, and we must not re-trim + it.) + """ + head, bracket, param = raw_name.partition("[") + return head.rsplit("::", 1)[-1] + bracket + param + + +def _ctrf_failure_line(ctrf_path: Path) -> FailureLine | None: """`` failed[: ]`` from the first failed CTRF test. The verifier's pytest run writes a CTRF report (``pytest --ctrf``, see the standard path in ``task_authoring/structural_checks.py``). Per test the useful failure evidence is the ``E …`` assertion line inside ``trace``; ``message`` is only a generic phase description, so it is the fallback. + When the report holds more than one failed test, the first failure's line + carries a count suffix (``(+N more failures; P/T checks passed)``) so the + console never under-reports how much is broken. """ if ctrf_path.stat().st_size > _ARTIFACT_READ_BYTES: # JSON only parses whole — an oversized report is skipped (not # truncated) and the next evidence source gets its turn. return None data = json.loads(ctrf_path.read_text(encoding="utf-8", errors="replace")) - tests = (data.get("results") or {}).get("tests") or [] - for test in tests: - if not isinstance(test, dict) or test.get("status") != "failed": - continue - # Full pytest node ids ("tests/test_x.py::test_build") waste the - # 100-char line budget; keep the test function name. - name = str(test.get("name") or "test").rsplit("::", 1)[-1] - trace = test.get("trace") - if isinstance(trace, str): - for line in trace.splitlines(): - stripped = line.strip() - if stripped.startswith("E "): - assertion = stripped[2:].strip() - # " failed:" already says it's an assertion — drop - # the prefix to reclaim line budget for the message. - assertion = assertion.removeprefix("AssertionError: ") - return f"{name} failed: {assertion}" - message = test.get("message") - if isinstance(message, str) and message.strip(): - return f"{name} failed: {' '.join(message.split())}" - return f"{name} failed" - return None - - -def _stdout_tail_failure_line(stdout_path: Path) -> str | None: + raw_tests = (data.get("results") or {}).get("tests") or [] + tests = [test for test in raw_tests if isinstance(test, dict)] + failed = [test for test in tests if test.get("status") == "failed"] + if not failed: + return None + suffix = "" + if len(failed) > 1: + # Counts come from the same rolled-up test list the first-failure pick + # uses, so the suffix can never disagree with the shown evidence. + # T is all entries — skips count against the denominator, not as passes. + extra = len(failed) - 1 + passed = sum(1 for test in tests if test.get("status") == "passed") + plural = "" if extra == 1 else "s" + suffix = ( + f" (+{extra} more failure{plural}; {passed}/{len(tests)} checks passed)" + ) + test = failed[0] + name = _display_test_name(str(test.get("name") or "test")) + trace = test.get("trace") + if isinstance(trace, str): + for line in trace.splitlines(): + stripped = line.strip() + if stripped.startswith("E "): + assertion = stripped[2:].strip() + # " failed:" already says it's an assertion — drop + # the prefix to reclaim line budget for the message. + assertion = assertion.removeprefix("AssertionError: ") + return FailureLine(f"{name} failed: {assertion}", suffix) + message = test.get("message") + if isinstance(message, str) and message.strip(): + return FailureLine(f"{name} failed: {' '.join(message.split())}", suffix) + return FailureLine(f"{name} failed", suffix) + + +def _stdout_tail_failure_line(stdout_path: Path) -> FailureLine | None: """Last pytest summary ("N failed, M passed …") — else last ``FAILED …`` - line — from a bounded tail of the verifier's test-stdout capture.""" + line — from a bounded tail of the verifier's test-stdout capture. No count + suffix: the summary line already carries the full counts itself.""" with stdout_path.open("rb") as fh: fh.seek(0, 2) # SEEK_END fh.seek(max(0, fh.tell() - _ARTIFACT_READ_BYTES)) @@ -82,19 +127,19 @@ def _stdout_tail_failure_line(stdout_path: Path) -> str | None: for line in reversed(tail.splitlines()): bare = line.strip().strip("=").strip() if _PYTEST_SUMMARY_RE.match(bare): - return bare + return FailureLine(bare) if last_failed is None and line.strip().startswith("FAILED "): last_failed = line.strip() - return last_failed + return FailureLine(last_failed) if last_failed is not None else None def artifact_failure_evidence( job_dir: Path, rollout_name: str -) -> tuple[str, Path] | None: - """One-liner mined from the rollout's verifier artifacts, or ``None``. +) -> tuple[FailureLine, Path] | None: + """``FailureLine`` mined from the rollout's verifier artifacts, or ``None``. - Returns the one-liner plus the verifier dir it came from (for the report - block's single ``(details: …)`` pointer line). Never raises. + Returns the structured one-liner plus the verifier dir it came from (for + the report block's single ``(details: …)`` pointer line). Never raises. """ # Local import: RolloutPaths pulls in the task package, which the CLI # shouldn't pay for until a failure actually needs artifact evidence. @@ -102,7 +147,7 @@ def artifact_failure_evidence( rollout_paths = RolloutPaths(rollout_dir=job_dir / rollout_name) verifier_dir = rollout_paths.verifier_dir - attempts: list[tuple[Path, Callable[[Path], str | None]]] = [ + attempts: list[tuple[Path, Callable[[Path], FailureLine | None]]] = [ # ctrf.json has no RolloutPaths property — the verifier recovers it by # literal name (verifier_core.py, `_recover_main_verifier_outputs`). (verifier_dir / "ctrf.json", _ctrf_failure_line), diff --git a/src/benchflow/cli/_shared.py b/src/benchflow/cli/_shared.py index 232fec60..0fbf3790 100644 --- a/src/benchflow/cli/_shared.py +++ b/src/benchflow/cli/_shared.py @@ -21,7 +21,7 @@ from rich.markup import escape from benchflow._utils.text import truncate_end -from benchflow.cli._failure_evidence import artifact_failure_evidence +from benchflow.cli._failure_evidence import FailureLine, artifact_failure_evidence if TYPE_CHECKING: from pathlib import Path @@ -106,7 +106,8 @@ def _exit_if_evaluation_had_errors(result: object) -> None: # Final-block failure lines: keep the block skimmable on big jobs and each -# line inside a typical terminal width. +# line inside a typical terminal width — except lines carrying a multi-failure +# count suffix, which deliberately run ~40 chars past the budget. _MAX_FAILURE_LINES = 5 _FAILURE_LINE_LIMIT = 100 _FAILURE_REASON_METRICS = 3 @@ -114,7 +115,7 @@ def _exit_if_evaluation_had_errors(result: object) -> None: def _failure_reason( failure: TaskFailure, job_dir: Path | None = None -) -> tuple[str, Path | None]: +) -> tuple[FailureLine, Path | None]: """One cheap line explaining why a FAILED (scored, reward != 1) task failed, plus the verifier dir when artifacts supplied the evidence. @@ -123,13 +124,15 @@ def _failure_reason( first — they explain the miss); else the reward plus a one-liner mined from the rollout's verifier artifacts (bounded CLI-side reads resolved via the ``rollout_name`` the engine records — the engine stays file-free); - else just the reward. The returned path is non-``None`` only for the - artifact tier, so the caller can print one ``(details: …)`` pointer per - report block. + else just the reward. The reason is a ``FailureLine`` so the artifact + tier's multi-failure count suffix stays separable from the truncatable + body (only the artifact tier ever sets it). The returned path is + non-``None`` only for the artifact tier, so the caller can print one + ``(details: …)`` pointer per report block. """ if failure.verifier_error: # Collapse whitespace: verifier errors are routinely multi-line. - return " ".join(failure.verifier_error.split()), None + return FailureLine(" ".join(failure.verifier_error.split())), None rewards = failure.rewards or {} reward = rewards.get("reward") metrics = [ @@ -144,15 +147,18 @@ def _failure_reason( shown = ", ".join( f"{name} {value}" for name, value in metrics[:_FAILURE_REASON_METRICS] ) - return f"reward {reward} — {shown}", None + return FailureLine(f"reward {reward} — {shown}"), None # No rollout_name (pre-#957-follow-up result.json, sharded aggregation): # nothing to resolve — the bare reward is as honest as it gets. if job_dir is not None and failure.rollout_name: evidence = artifact_failure_evidence(job_dir, failure.rollout_name) if evidence is not None: detail, verifier_dir = evidence - return f"reward {reward} — {detail}", verifier_dir - return f"reward {reward}", None + return ( + FailureLine(f"reward {reward} — {detail.body}", detail.suffix), + verifier_dir, + ) + return FailureLine(f"reward {reward}"), None def _report_eval_result(result: EvaluationResult, job_dir: Path | None = None) -> None: @@ -207,11 +213,16 @@ def _report_eval_result(result: EvaluationResult, job_dir: Path | None = None) - reason, verifier_dir = _failure_reason(failure, job_dir) if artifact_pointer is None and verifier_dir is not None: artifact_pointer = verifier_dir - line = truncate_end( - f" ✗ {failure.task_name}: {reason}", + # The char budget governs only the free-text body; the multi-failure + # count suffix is appended AFTER truncation so a long assertion can + # never swallow the "more than this is broken" signal. Worst case the + # line runs ~40 chars past the budget — completeness beats strict + # width for this one signal. + body = truncate_end( + f" ✗ {failure.task_name}: {reason.body}", _FAILURE_LINE_LIMIT, ) - console.print(f"[dim]{escape(line)}[/dim]") + console.print(f"[dim]{escape(body + reason.suffix)}[/dim]") extra = len(failures) - _MAX_FAILURE_LINES if extra > 0: console.print(f"[dim] … and {extra} more[/dim]") diff --git a/tests/test_cli_live_progress.py b/tests/test_cli_live_progress.py index 5725568b..5e5ec3bb 100644 --- a/tests/test_cli_live_progress.py +++ b/tests/test_cli_live_progress.py @@ -476,20 +476,11 @@ def _bare_failure(task_name: str, rollout_name: str | None = None): ) -def _write_ctrf(verifier_dir, *, name: str | None, trace: str | None = None) -> None: - """Write a minimal CTRF report (pytest-json-ctrf shape). - - ``name`` is the failed test's node id; ``None`` writes an all-passed report. - """ +def _write_ctrf_tests(verifier_dir, tests: list[dict]) -> None: + """Write a CTRF report (pytest-json-ctrf shape) with the given tests.""" import json verifier_dir.mkdir(parents=True, exist_ok=True) - tests = [{"name": "tests/test_x.py::test_ok", "status": "passed"}] - if name is not None: - test: dict = {"name": name, "status": "failed"} - if trace is not None: - test["trace"] = trace - tests.append(test) (verifier_dir / "ctrf.json").write_text( json.dumps( { @@ -500,6 +491,20 @@ def _write_ctrf(verifier_dir, *, name: str | None, trace: str | None = None) -> ) +def _write_ctrf(verifier_dir, *, name: str | None, trace: str | None = None) -> None: + """Write a minimal CTRF report. + + ``name`` is the failed test's node id; ``None`` writes an all-passed report. + """ + tests = [{"name": "tests/test_x.py::test_ok", "status": "passed"}] + if name is not None: + test: dict = {"name": name, "status": "failed"} + if trace is not None: + test["trace"] = trace + tests.append(test) + _write_ctrf_tests(verifier_dir, tests) + + def _write_stdout(verifier_dir, text: str) -> None: verifier_dir.mkdir(parents=True, exist_ok=True) (verifier_dir / "test-stdout.txt").write_text(text) @@ -527,6 +532,121 @@ def test_failure_reason_artifact_tier_reads_ctrf(tmp_path): "Build failed for py312" in out ) assert f"(details: {tmp_path / 'fix-build__ab12cd34' / 'verifier'})" in out + # A single failed test needs no roll-up count. + assert "(+" not in out + + +def test_ctrf_multi_failure_appends_count_suffix(tmp_path): + # Dogfood follow-up (skillsbench dialogue-parser, reward 0.667): the CTRF + # held 2 failed / 4 passed but the console named only the first failure — + # a reader stopping at the console under-counted what was broken. With >1 + # failed test the line gains a count suffix, and a param id carried by the + # report's test name is preserved end-to-end. (pytest-json-ctrf, as of + # 0.5.x, strips the param id at generation time — nodeid.split('[')[0] — + # so this fixture uses the spec-conformant full name other producers emit.) + _write_ctrf_tests( + tmp_path / "dialogue-parser__ab12cd34" / "verifier", + [ + {"name": "test_outputs.py::test_system_basics", "status": "passed"}, + {"name": "test_outputs.py::test_narrative_content", "status": "passed"}, + { + "name": "test_outputs.py::test_graph_logic[reachability]", + "status": "failed", + "trace": ( + "> assert not unreachable\n" + "E AssertionError: Unreachable nodes found: " + "['End']...\n" + ), + }, + { + "name": "test_outputs.py::test_visualization_validity", + "status": "failed", + }, + {"name": "test_outputs.py::test_content_integrity", "status": "passed"}, + {"name": "test_outputs.py::test_structural_integrity", "status": "passed"}, + ], + ) + from benchflow.evaluation import TaskFailure + + out = _reported( + _failed_result( + [ + TaskFailure( + task_name="dialogue-parser", + rewards={"reward": 0.667}, + verifier_error=None, + rollout_name="dialogue-parser__ab12cd34", + ) + ] + ), + tmp_path, + ) + # The body hits the 100-char budget and truncates; the suffix rides after + # it whole. Counts are report-wide: 1 extra failure, 4 of 6 checks passed. + assert ( + "✗ dialogue-parser: reward 0.667 — test_graph_logic[reachability] " + "failed: Unreachable nodes found:… (+1 more failure; 4/6 checks passed)" in out + ) + + +def test_ctrf_count_suffix_plural_and_param_id_with_colons(tmp_path): + # 3 failed tests pluralize the suffix, and a param id containing "::" must + # not be mangled by the node-id segment split (`test_foo[a::b]`, not `b]`). + _write_ctrf_tests( + tmp_path / "multi__ab12cd34" / "verifier", + [ + {"name": "tests/test_x.py::test_foo[a::b]", "status": "failed"}, + {"name": "tests/test_x.py::test_bar", "status": "failed"}, + {"name": "tests/test_x.py::test_baz", "status": "failed"}, + {"name": "tests/test_x.py::test_ok", "status": "passed"}, + ], + ) + out = _reported( + _failed_result([_bare_failure("multi", "multi__ab12cd34")]), tmp_path + ) + assert ( + "✗ multi: reward 0.0 — test_foo[a::b] failed " + "(+2 more failures; 1/4 checks passed)" in out + ) + + +def test_ctrf_count_suffix_survives_truncation(tmp_path): + # A long assertion must not push the count suffix off the line: the body + # alone is truncated to the 100-char budget, then the suffix is appended + # whole — so the "more than this is broken" signal always survives. + _write_ctrf_tests( + tmp_path / "long__ab12cd34" / "verifier", + [ + { + "name": "tests/test_x.py::test_long", + "status": "failed", + "trace": "E AssertionError: " + "verbose diagnosis " * 20 + "\n", + }, + {"name": "tests/test_x.py::test_other", "status": "failed"}, + {"name": "tests/test_x.py::test_ok", "status": "passed"}, + ], + ) + out = _reported(_failed_result([_bare_failure("long", "long__ab12cd34")]), tmp_path) + suffix = " (+1 more failure; 1/3 checks passed)" + (line,) = [ln for ln in out.splitlines() if "✗ long" in ln] + assert line.rstrip().endswith(f"…{suffix}") + assert len(line.rstrip()) <= 100 + len(suffix) + + +def test_stdout_tail_tier_never_gets_count_suffix(tmp_path): + # The suffix is a CTRF-tier concept: the stdout tail's pytest summary line + # already carries the full counts, so nothing is appended there. + _write_stdout( + tmp_path / "fix-build__ab12cd34" / "verifier", + "FAILED tests/test_build.py::test_a - AssertionError: boom\n" + "FAILED tests/test_build.py::test_b - AssertionError: boom\n" + "=========== 2 failed, 1 passed in 4.20s ===========\n", + ) + out = _reported( + _failed_result([_bare_failure("fix-build", "fix-build__ab12cd34")]), tmp_path + ) + assert "✗ fix-build: reward 0.0 — 2 failed, 1 passed in 4.20s" in out + assert "(+" not in out def test_failure_reason_artifact_tier_requires_rollout_name(tmp_path):