diff --git a/docs/reference/cli.md b/docs/reference/cli.md index eae7f0a5..85abfdca 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -301,6 +301,11 @@ alike (so it also wins over an exported `on`). Note that on a TTY, mutes INFO logging while it owns the screen; pair it with `BENCHFLOW_NO_PROGRESS=1` to get plain heartbeat lines on a TTY. +The final `Score: P/T (…%)` line is pass-threshold aggregation — a task counts +as passed only at reward 1.0 — while `mean reward` beside it is the average raw +verifier reward, so `0/1 (0.0%)` next to `mean reward 0.80` means partial +credit below the pass threshold, not a flat zero. + Set `BENCHFLOW_ACP_HANDSHAKE_TIMEOUT` to a number of seconds (default 60) to give slow-starting agents more time to answer the pre-prompt ACP handshake (`initialize`/`session_new`) — heavyweight task images can push agent startup diff --git a/src/benchflow/cli/_failure_evidence.py b/src/benchflow/cli/_failure_evidence.py index 198231d0..cb170037 100644 --- a/src/benchflow/cli/_failure_evidence.py +++ b/src/benchflow/cli/_failure_evidence.py @@ -4,27 +4,32 @@ task's reason would otherwise be the bare ``reward X`` fallback, mine the rollout's verifier artifacts for a one-line explanation. Lives in its own module so ``cli/_shared.py`` stays the side-effect-free display helpers it -advertises. +advertises. Also home of :func:`metric_breakdown`, the one canonical +rewards-dict flattening — shared by ``_shared.py``'s in-memory metric tier and +the ``reward.json`` probe here, so the two render identically. Contract (the engine stays file-free — these reads are CLI-side, report-time only): - The rollout dir is resolved exactly, from the ``rollout_name`` the engine records on every result — never guessed from globs. -- Evidence sources are tried in order (CTRF report, then test-stdout tail); - the first that yields a line wins. +- Evidence sources are tried in order (CTRF report, then reward.json metric + breakdown, then test-stdout tail); the first that yields a line wins. - Every read is bounded (``_ARTIFACT_READ_BYTES`` per file) and nothing here raises — any surprise degrades to ``None``, i.e. the bare reward reason. +- The report block's ``(details: …)`` pointer is decided separately, by + :func:`verifier_dir_for` from dir existence alone — every failure block + with artifacts on disk gets one pointer, evidence or not. """ from __future__ import annotations import json import re -from typing import TYPE_CHECKING, NamedTuple +from typing import TYPE_CHECKING, Any, NamedTuple if TYPE_CHECKING: - from collections.abc import Callable + from collections.abc import Callable, Mapping from pathlib import Path # Never read more than this many bytes per file, and evidence is only mined @@ -32,6 +37,8 @@ _ARTIFACT_READ_BYTES = 64 * 1024 # Pytest final summary, decoration stripped: "1 failed, 2 passed in 40.86s". _PYTEST_SUMMARY_RE = re.compile(r"\d+ failed\b") +# Metric-breakdown cap: one metric-happy verifier can't flood the line. +_BREAKDOWN_METRICS = 3 class FailureLine(NamedTuple): @@ -49,6 +56,88 @@ class FailureLine(NamedTuple): suffix: str = "" +def _numeric_items(mapping: Mapping[str, Any]) -> list[tuple[str, Any]]: + """The named numeric metrics of one mapping level, insertion-ordered. + + ``reward`` is excluded at every level: at the top it is the aggregate the + line already leads with, and a nested ``reward`` would render a confusing + ``reward 0.8 — reward 0.8``. + """ + return [ + (name, value) + for name, value in mapping.items() + if name != "reward" and isinstance(value, (bool, int, float)) + ] + + +def metric_breakdown(rewards: Mapping[str, Any]) -> str | None: + """Compact ``name value`` metric breakdown of a rewards mapping, capped at + ``_BREAKDOWN_METRICS`` — or ``None`` when it holds no named metrics. + + The numeric evidence may sit flat in the mapping, or nested one level under + a ``metrics`` sub-dict (env0-style verifiers: ``{"reward": …, "metrics": + {…}, "details": {…}}``), or under ``details`` when ``metrics`` yields + nothing. Flat keys win outright — a flat rewards dict renders exactly as + before. + + Ordering leads with the lowest-signal metrics — they explain the miss: + + - A ``_found`` metric with a matching positive numeric + ``_total`` anywhere in the mapping renders as `` found/total`` + and sorts by that fraction, so ``deadlines 1/5`` leads even though 1 is + not 0. A total consumed by a pair is dropped from the shown list (the + fraction already carries it). + - Unpaired metrics keep the zero-first rule: zero/failed first, stable + within each group. + """ + shown = _numeric_items(rewards) + if not shown: + for key in ("metrics", "details"): + sub = rewards.get(key) + if isinstance(sub, dict): + shown = _numeric_items(sub) + if shown: + break + if not shown: + return None + # found/total pairing looks across ALL levels: env0 keeps the found counts + # under "metrics" and the totals under "details". First-seen wins. + totals: dict[str, Any] = {} + for source in (rewards, rewards.get("metrics"), rewards.get("details")): + if isinstance(source, dict): + for name, value in _numeric_items(source): + totals.setdefault(name, value) + + def _pair_total(name: str, value: Any) -> Any | None: + """The positive numeric ``_total`` for a ``_found`` metric, + else ``None``. Bools (either side) and non-positive totals never pair.""" + base = name.removesuffix("_found") + if base == name or isinstance(value, bool): + return None + total = totals.get(base + "_total") + if isinstance(total, bool) or not isinstance(total, (int, float)): + return None + return total if total > 0 else None + + consumed = { + name.removesuffix("_found") + "_total" + for name, value in shown + if _pair_total(name, value) is not None + } + entries: list[tuple[float, str]] = [] + for name, value in shown: + if name in consumed: + continue + total = _pair_total(name, value) + if total is not None: + base = name.removesuffix("_found") + entries.append((value / total, f"{base} {value}/{total}")) + else: + entries.append((0.0 if value == 0 else 1.0, f"{name} {value}")) + entries.sort(key=lambda entry: entry[0]) # stable: ties keep dict order + return ", ".join(fragment for _, fragment in entries[:_BREAKDOWN_METRICS]) + + def _display_test_name(raw_name: str) -> str: """Test name for display: node-id path segments dropped, param id kept. @@ -65,6 +154,19 @@ def _display_test_name(raw_name: str) -> str: return head.rsplit("::", 1)[-1] + bracket + param +def _bounded_json(path: Path) -> dict[str, Any] | None: + """The file parsed as a JSON object, or ``None`` when it can't serve. + + JSON only parses whole — an oversized (> ``_ARTIFACT_READ_BYTES``) file is + skipped, not truncated, and a non-object payload is skipped the same way; + either way the next evidence source gets its turn. + """ + if path.stat().st_size > _ARTIFACT_READ_BYTES: + return None + data = json.loads(path.read_text(encoding="utf-8", errors="replace")) + return data if isinstance(data, dict) else None + + def _ctrf_failure_line(ctrf_path: Path) -> FailureLine | None: """`` failed[: ]`` from the first failed CTRF test. @@ -76,11 +178,9 @@ def _ctrf_failure_line(ctrf_path: Path) -> FailureLine | None: 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. + data = _bounded_json(ctrf_path) + if data is None: return None - data = json.loads(ctrf_path.read_text(encoding="utf-8", errors="replace")) 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"] @@ -115,6 +215,25 @@ def _ctrf_failure_line(ctrf_path: Path) -> FailureLine | None: return FailureLine(f"{name} failed", suffix) +def _reward_json_failure_line(reward_path: Path) -> FailureLine | None: + """Metric breakdown mined from the verifier's ``reward.json``. + + env0-style verifiers write ``{"reward": …, "metrics": {…}, "details": + {…}}`` (and non-pytest stdout, no CTRF report) — when the result's rewards + dict was stripped to the bare aggregate (older persisted results, sharded + aggregation), the numeric evidence still sits on disk. Reuses the same + one-level flattening as the in-memory reward-dict tier so the two render + identically. A file with no named metrics repeats nothing the line does + not already say — it yields to the stdout tail. No count suffix: the + breakdown is a metric summary, not a first-of-N failure pick. + """ + data = _bounded_json(reward_path) + if data is None: + return None + shown = metric_breakdown(data) + return FailureLine(shown) if shown is not None else None + + 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. No count @@ -133,13 +252,28 @@ def _stdout_tail_failure_line(stdout_path: Path) -> FailureLine | None: return FailureLine(last_failed) if last_failed is not None else None -def artifact_failure_evidence( - job_dir: Path, rollout_name: str -) -> tuple[FailureLine, Path] | None: - """``FailureLine`` mined from the rollout's verifier artifacts, or ``None``. +def verifier_dir_for(job_dir: Path, rollout_name: str) -> Path | None: + """The rollout's verifier dir when it exists on disk, else ``None``. + + Powers the report block's ``(details: …)`` pointer rule: every failure + block with artifacts on disk gets one pointer — from dir existence alone, + independent of which tier supplied the reason line (the pointer is most + valuable exactly when evidence extraction fails). Never raises. + """ + # Local import: RolloutPaths pulls in the task package (see + # artifact_failure_evidence). + from benchflow.task.paths import RolloutPaths + + try: + verifier_dir = RolloutPaths(rollout_dir=job_dir / rollout_name).verifier_dir + return verifier_dir if verifier_dir.is_dir() else None + except Exception: + return None + - Returns the structured one-liner plus the verifier dir it came from (for - the report block's single ``(details: …)`` pointer line). Never raises. +def artifact_failure_evidence(job_dir: Path, rollout_name: str) -> FailureLine | None: + """``FailureLine`` mined from the rollout's verifier artifacts, or ``None`` + when every probe misses. Never raises. """ # Local import: RolloutPaths pulls in the task package, which the CLI # shouldn't pay for until a failure actually needs artifact evidence. @@ -151,6 +285,7 @@ def artifact_failure_evidence( # 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), + (rollout_paths.reward_json_path, _reward_json_failure_line), (rollout_paths.test_stdout_path, _stdout_tail_failure_line), ] for path, extract in attempts: @@ -163,5 +298,5 @@ def artifact_failure_evidence( # the report must never crash over evidence mining. continue if detail is not None: - return detail, verifier_dir + return detail return None diff --git a/src/benchflow/cli/_shared.py b/src/benchflow/cli/_shared.py index 0fbf3790..feb1afa4 100644 --- a/src/benchflow/cli/_shared.py +++ b/src/benchflow/cli/_shared.py @@ -21,7 +21,12 @@ from rich.markup import escape from benchflow._utils.text import truncate_end -from benchflow.cli._failure_evidence import FailureLine, artifact_failure_evidence +from benchflow.cli._failure_evidence import ( + FailureLine, + artifact_failure_evidence, + metric_breakdown, + verifier_dir_for, +) if TYPE_CHECKING: from pathlib import Path @@ -110,55 +115,38 @@ def _exit_if_evaluation_had_errors(result: object) -> None: # count suffix, which deliberately run ~40 chars past the budget. _MAX_FAILURE_LINES = 5 _FAILURE_LINE_LIMIT = 100 -_FAILURE_REASON_METRICS = 3 -def _failure_reason( - failure: TaskFailure, job_dir: Path | None = 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. +def _failure_reason(failure: TaskFailure, job_dir: Path | None = None) -> FailureLine: + """One cheap line explaining why a FAILED (scored, reward != 1) task failed. Priority: the verifier's own error if set; else the reward plus a compact - breakdown of the named metrics in the reward dict (zero/failed metrics - 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 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. + breakdown of the named metrics in the reward dict — flat, or flattened one + level from an env0-style ``metrics``/``details`` sub-dict, lowest-signal + metrics first (see :func:`metric_breakdown`); 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 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 block's + ``(details: …)`` pointer is NOT decided here — the caller keys it off + on-disk artifacts alone, independent of which tier supplied the reason. """ if failure.verifier_error: # Collapse whitespace: verifier errors are routinely multi-line. - return FailureLine(" ".join(failure.verifier_error.split())), None + return FailureLine(" ".join(failure.verifier_error.split())) rewards = failure.rewards or {} reward = rewards.get("reward") - metrics = [ - (name, value) - for name, value in rewards.items() - if name != "reward" and isinstance(value, (bool, int, float)) - ] - if metrics: - # Zero/failed metrics first (stable within each group), capped so one - # metric-happy verifier can't flood the line. - metrics.sort(key=lambda kv: kv[1] != 0) - shown = ", ".join( - f"{name} {value}" for name, value in metrics[:_FAILURE_REASON_METRICS] - ) - return FailureLine(f"reward {reward} — {shown}"), None + shown = metric_breakdown(rewards) + if shown is not None: + return FailureLine(f"reward {reward} — {shown}") # 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 ( - FailureLine(f"reward {reward} — {detail.body}", detail.suffix), - verifier_dir, - ) - return FailureLine(f"reward {reward}"), None + detail = artifact_failure_evidence(job_dir, failure.rollout_name) + if detail is not None: + return FailureLine(f"reward {reward} — {detail.body}", detail.suffix) + return FailureLine(f"reward {reward}") def _report_eval_result(result: EvaluationResult, job_dir: Path | None = None) -> None: @@ -170,8 +158,9 @@ def _report_eval_result(result: EvaluationResult, job_dir: Path | None = None) - dim ``✗ task: reason`` line (capped at ``_MAX_FAILURE_LINES``) so the "why" doesn't require opening summary.json; when ``job_dir`` is given, a reason that would otherwise be a bare ``reward X`` is upgraded from the rollout's - verifier artifacts (bounded reads, displayed failures only), with a single - ``(details: …/verifier)`` pointer line for the block. When ``job_dir`` is + verifier artifacts (bounded reads, displayed failures only), and every + failure block with artifacts on disk gets one ``(details: …/verifier)`` + pointer line — evidence mined or not. When ``job_dir`` is given, the result/summary paths are printed so testers know where to look (the guide repeatedly says "read summary.json" but the CLI never said where). @@ -210,9 +199,14 @@ def _report_eval_result(result: EvaluationResult, job_dir: Path | None = None) - failures = getattr(result, "task_failures", None) or [] artifact_pointer: Path | None = None for failure in failures[:_MAX_FAILURE_LINES]: - reason, verifier_dir = _failure_reason(failure, job_dir) - if artifact_pointer is None and verifier_dir is not None: - artifact_pointer = verifier_dir + reason = _failure_reason(failure, job_dir) + # Pointer rule: every failure block with artifacts on disk gets one + # (details:) pointer — the first displayed failure whose verifier dir + # exists, independent of which tier supplied its reason line (byte- + # identical reasons must not differ on provenance the console can't + # show, and the pointer matters most when every probe missed). + if artifact_pointer is None and job_dir is not None and failure.rollout_name: + artifact_pointer = verifier_dir_for(job_dir, failure.rollout_name) # 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 @@ -226,8 +220,9 @@ def _report_eval_result(result: EvaluationResult, job_dir: Path | None = None) - extra = len(failures) - _MAX_FAILURE_LINES if extra > 0: console.print(f"[dim] … and {extra} more[/dim]") - # One pointer per block (not per line): the first artifact-backed reason's - # verifier dir, so "where do I look next" needs no summary.json dig either. + # One pointer per block (not per line): the first displayed failure whose + # verifier dir exists — artifact-backed or not — so "where do I look next" + # needs no summary.json dig even when evidence mining came up empty. if artifact_pointer is not None: console.print(f"[dim] (details: {escape(str(artifact_pointer))})[/dim]") if job_dir is not None: diff --git a/tests/test_cli_live_progress.py b/tests/test_cli_live_progress.py index c7c0d9f0..f9b54b10 100644 --- a/tests/test_cli_live_progress.py +++ b/tests/test_cli_live_progress.py @@ -518,6 +518,129 @@ def test_report_eval_result_prints_failure_reason_lines(): assert "✗ sum-csv: reward 0.0" in out +def _env0_rewards(reward=0.8): + """env0-shaped rewards: numeric evidence nested one level under "metrics", + with the matching totals under "details" (observed on gdoc-extract-content).""" + return { + "reward": reward, + "metrics": { + "summary_doc_exists": 1, + "decisions_found": 5, + "deadlines_found": 1, + "originals_preserved": 1, + "agent_acted": 1, + }, + "details": {"decisions_total": 5, "deadlines_total": 5}, + } + + +def _failure( + task_name: str, rollout_name: str | None = None, *, rewards: dict | None = None +): + """A FAILED (scored) task; ``rewards`` defaults to the bare `reward 0.0` + fallback shape.""" + from benchflow.evaluation import TaskFailure + + return TaskFailure( + task_name=task_name, + rewards={"reward": 0.0} if rewards is None else rewards, + verifier_error=None, + rollout_name=rollout_name, + ) + + +def test_failure_reason_flattens_nested_metrics(): + # env0 dogfood (gdoc-extract-content, reward 0.8): the decisive numbers sat + # one level under "metrics" and the console printed the bare `reward 0.8` + # fallback. The breakdown tier must flatten one level, pair _found + # with a positive _total (env0 keeps totals under "details"), and + # lead with the furthest-from-max metric — deadlines 1/5, not the + # insertion-ordered all-ones. + out = _reported( + _failed_result([_failure("gdoc-extract-content", rewards=_env0_rewards())]) + ) + assert ( + "✗ gdoc-extract-content: reward 0.8 — deadlines 1/5, " + "summary_doc_exists 1, decisions 5/5" in out + ) + # The 3-metric cap still applies after flattening. + assert "originals_preserved" not in out + assert "agent_acted" not in out + + +def test_failure_reason_nested_details_when_no_metrics(): + # No "metrics" sub-dict: "details" numerics carry the breakdown, and + # unpaired values keep the zero-first rule. + out = _reported( + _failed_result( + [ + _failure( + "report-task", + rewards={"reward": 0.5, "details": {"sections": 1, "tables": 0}}, + ) + ] + ) + ) + assert "✗ report-task: reward 0.5 — tables 0, sections 1" in out + + +def test_failure_reason_flat_metrics_win_over_nested(): + # Flat numeric keys keep working exactly as before — a nested sub-dict + # never overrides them. + out = _reported( + _failed_result( + [ + _failure( + "flat-task", + rewards={"reward": 0.2, "checks": 0, "metrics": {"x": 1}}, + ) + ] + ) + ) + assert "✗ flat-task: reward 0.2 — checks 0" in out + assert "x 1" not in out + + +def test_failure_reason_pairing_consumes_total_key(): + # found/total living in the SAME dict: the pair renders as a fraction and + # the consumed *_total key never renders as its own metric. + out = _reported( + _failed_result( + [ + _failure( + "pair-task", + rewards={ + "reward": 0.4, + "metrics": { + "deadlines_found": 1, + "deadlines_total": 5, + "notes": 1, + }, + }, + ) + ] + ) + ) + assert "✗ pair-task: reward 0.4 — deadlines 1/5, notes 1" in out + assert "deadlines_total" not in out + + +def test_failure_reason_zero_total_never_pairs(): + # A non-positive total can't pair (no division): both keys render as + # plain zero-first values, and nothing raises. + out = _reported( + _failed_result( + [ + _failure( + "zero-task", + rewards={"reward": 0.0, "metrics": {"a_found": 0, "a_total": 0}}, + ) + ] + ) + ) + assert "✗ zero-task: reward 0.0 — a_found 0, a_total 0" in out + + def test_report_eval_result_caps_failure_lines_at_five(): from benchflow.evaluation import TaskFailure @@ -549,18 +672,6 @@ def test_report_eval_result_truncates_failure_lines(): assert line.rstrip().endswith("…") -def _bare_failure(task_name: str, rollout_name: str | None = None): - """A FAILED task whose reason would be the bare `reward 0.0` fallback.""" - from benchflow.evaluation import TaskFailure - - return TaskFailure( - task_name=task_name, - rewards={"reward": 0.0}, - verifier_error=None, - rollout_name=rollout_name, - ) - - def _write_ctrf_tests(verifier_dir, tests: list[dict]) -> None: """Write a CTRF report (pytest-json-ctrf shape) with the given tests.""" import json @@ -595,6 +706,14 @@ def _write_stdout(verifier_dir, text: str) -> None: (verifier_dir / "test-stdout.txt").write_text(text) +def _write_reward_json(verifier_dir, rewards: dict) -> None: + """Write the verifier's reward.json (env0 shape: reward + metrics/details).""" + import json + + verifier_dir.mkdir(parents=True, exist_ok=True) + (verifier_dir / "reward.json").write_text(json.dumps(rewards)) + + def test_failure_reason_artifact_tier_reads_ctrf(tmp_path): # Dogfood follow-up: `✗ fix-build: reward 0.0` while verifier/ctrf.json held # the real answer. A bare-reward reason is upgraded from the rollout dir the @@ -610,7 +729,7 @@ def test_failure_reason_artifact_tier_reads_ctrf(tmp_path): ), ) out = _reported( - _failed_result([_bare_failure("fix-build", "fix-build__ab12cd34")]), tmp_path + _failed_result([_failure("fix-build", "fix-build__ab12cd34")]), tmp_path ) assert ( "✗ fix-build: reward 0.0 — test_build_success failed: " @@ -686,9 +805,7 @@ def test_ctrf_count_suffix_plural_and_param_id_with_colons(tmp_path): {"name": "tests/test_x.py::test_ok", "status": "passed"}, ], ) - out = _reported( - _failed_result([_bare_failure("multi", "multi__ab12cd34")]), tmp_path - ) + out = _reported(_failed_result([_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 @@ -711,7 +828,7 @@ def test_ctrf_count_suffix_survives_truncation(tmp_path): {"name": "tests/test_x.py::test_ok", "status": "passed"}, ], ) - out = _reported(_failed_result([_bare_failure("long", "long__ab12cd34")]), tmp_path) + out = _reported(_failed_result([_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}") @@ -728,7 +845,7 @@ def test_stdout_tail_tier_never_gets_count_suffix(tmp_path): "=========== 2 failed, 1 passed in 4.20s ===========\n", ) out = _reported( - _failed_result([_bare_failure("fix-build", "fix-build__ab12cd34")]), tmp_path + _failed_result([_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 @@ -742,8 +859,8 @@ def test_failure_reason_artifact_tier_requires_rollout_name(tmp_path): out = _reported( _failed_result( [ - _bare_failure("fix-build", None), - _bare_failure("other-task", "other-task__99999999"), + _failure("fix-build", None), + _failure("other-task", "other-task__99999999"), ] ), tmp_path, @@ -764,7 +881,7 @@ def test_failure_reason_artifact_tier_stdout_tail_summary(tmp_path): "=========== 1 failed, 2 passed in 40.86s ===========\n", ) out = _reported( - _failed_result([_bare_failure("fix-build", "fix-build__ab12cd34")]), tmp_path + _failed_result([_failure("fix-build", "fix-build__ab12cd34")]), tmp_path ) assert "✗ fix-build: reward 0.0 — 1 failed, 2 passed in 40.86s" in out @@ -777,7 +894,7 @@ def test_failure_reason_artifact_tier_stdout_failed_line(tmp_path): "FAILED tests/test_build.py::test_build - AssertionError: boom\n", ) out = _reported( - _failed_result([_bare_failure("fix-build", "fix-build__ab12cd34")]), tmp_path + _failed_result([_failure("fix-build", "fix-build__ab12cd34")]), tmp_path ) assert ( "✗ fix-build: reward 0.0 — FAILED tests/test_build.py::test_build - " @@ -799,8 +916,8 @@ def test_failure_reason_artifact_tier_tries_sources_in_order(tmp_path): out = _reported( _failed_result( [ - _bare_failure("task-a", "task-a__11111111"), - _bare_failure("task-b", "task-b__22222222"), + _failure("task-a", "task-a__11111111"), + _failure("task-b", "task-b__22222222"), ] ), tmp_path, @@ -812,7 +929,8 @@ def test_failure_reason_artifact_tier_tries_sources_in_order(tmp_path): def test_failure_reason_artifact_tier_never_raises(tmp_path): # The tier's safety contract: a corrupt CTRF report falls through to the # stdout tail; with no other evidence it degrades to the bare `reward 0.0` - # — no pointer, no exception. + # — no exception. The block's single pointer survives (first displayed + # failure with an existing verifier dir — evidence or not). corrupt = tmp_path / "corrupt-task__11111111" / "verifier" corrupt.mkdir(parents=True) (corrupt / "ctrf.json").write_text("{not json") @@ -824,9 +942,9 @@ def test_failure_reason_artifact_tier_never_raises(tmp_path): out = _reported( _failed_result( [ - _bare_failure("corrupt-task", "corrupt-task__11111111"), - _bare_failure("empty-task", "empty-task__22222222"), - _bare_failure("salvaged-task", "salvaged-task__33333333"), + _failure("corrupt-task", "corrupt-task__11111111"), + _failure("empty-task", "empty-task__22222222"), + _failure("salvaged-task", "salvaged-task__33333333"), ] ), tmp_path, @@ -837,27 +955,105 @@ def test_failure_reason_artifact_tier_never_raises(tmp_path): assert out.count("(details:") == 1 +def test_failure_reason_artifact_tier_reads_reward_json(tmp_path): + # env0-style verifiers write reward.json + non-pytest stdout (no CTRF): + # when the result's rewards dict was stripped to the bare aggregate, the + # probe mines the same flattened low-metrics breakdown from disk — and the + # (details:) pointer prints. + _write_reward_json(tmp_path / "gdoc__ab12cd34" / "verifier", _env0_rewards()) + out = _reported( + _failed_result([_failure("gdoc", "gdoc__ab12cd34", rewards={"reward": 0.8})]), + tmp_path, + ) + assert ( + "✗ gdoc: reward 0.8 — deadlines 1/5, summary_doc_exists 1, decisions 5/5" in out + ) + assert f"(details: {tmp_path / 'gdoc__ab12cd34' / 'verifier'})" in out + + +def test_reward_json_yields_to_ctrf(tmp_path): + # Probe order: the CTRF report (named failing checks) stays first; + # reward.json only fills in when CTRF yields nothing. + verifier = tmp_path / "both__ab12cd34" / "verifier" + _write_ctrf(verifier, name="t::test_x") + _write_reward_json(verifier, _env0_rewards()) + out = _reported(_failed_result([_failure("both", "both__ab12cd34")]), tmp_path) + assert "✗ both: reward 0.0 — test_x failed" in out + assert "deadlines" not in out + + +def test_reward_json_wins_over_stdout_tail(tmp_path): + # And the other side of the probe order: with BOTH reward.json and a + # pytest-shaped stdout tail present, the named metric breakdown beats the + # anonymous counts — reordering those two probes fails this. + verifier = tmp_path / "order__ab12cd34" / "verifier" + _write_reward_json(verifier, _env0_rewards()) + _write_stdout(verifier, "=== 1 failed, 4 passed in 2.00s ===\n") + out = _reported( + _failed_result([_failure("order", "order__ab12cd34", rewards={"reward": 0.8})]), + tmp_path, + ) + assert "✗ order: reward 0.8 — deadlines 1/5" in out + assert "1 failed, 4 passed" not in out + + +def test_reward_json_corrupt_falls_through(tmp_path): + # The probe keeps the tier's never-raise contract: corrupt JSON falls + # through to the stdout tail. + verifier = tmp_path / "task__ab12cd34" / "verifier" + verifier.mkdir(parents=True) + (verifier / "reward.json").write_text("{not json") + _write_stdout(verifier, "=== 1 failed in 2.00s ===\n") + out = _reported(_failed_result([_failure("task", "task__ab12cd34")]), tmp_path) + assert "✗ task: reward 0.0 — 1 failed in 2.00s" in out + + +def test_reward_json_without_named_metrics_yields(tmp_path): + # A bare {"reward": …} reward.json repeats nothing the line doesn't + # already say — the probe yields to the stdout tail. + verifier = tmp_path / "bare__ab12cd34" / "verifier" + _write_reward_json(verifier, {"reward": 0.0}) + _write_stdout(verifier, "=== 2 failed, 1 passed in 1.50s ===\n") + out = _reported(_failed_result([_failure("bare", "bare__ab12cd34")]), tmp_path) + assert "✗ bare: reward 0.0 — 2 failed, 1 passed in 1.50s" in out + + +def test_pointer_prints_when_every_probe_misses(tmp_path): + # The third stacked miss from the env0 dogfood: exactly when evidence + # extraction fails, the user also lost the pointer to find it themselves. + # A failed task whose verifier dir exists gets the (details:) pointer even + # beside a bare reward reason (here: non-pytest stdout, no CTRF, no + # reward.json — every probe misses). + verifier = tmp_path / "gdoc__ab12cd34" / "verifier" + _write_stdout(verifier, "verifier ran custom checks; see reward artifacts\n") + out = _reported(_failed_result([_failure("gdoc", "gdoc__ab12cd34")]), tmp_path) + assert "✗ gdoc: reward 0.0" in out + assert f"(details: {verifier})" in out + + def test_failure_reason_artifact_tier_yields_to_metric_breakdown(tmp_path): # Named metrics already explain the miss — artifacts are only for reasons - # that would otherwise be a bare `reward X`. - from benchflow.evaluation import TaskFailure - + # that would otherwise be a bare `reward X`. The (details:) pointer is + # decided separately, by on-disk artifacts alone: it prints here even + # though the reason came from the in-memory metrics (an identical line + # mined from reward.json must not differ on provenance the console can't + # show). _write_ctrf(tmp_path / "plan-meeting__ab12cd34" / "verifier", name="t::test_x") out = _reported( _failed_result( [ - TaskFailure( - task_name="plan-meeting", + _failure( + "plan-meeting", + "plan-meeting__ab12cd34", rewards={"reward": 0.3, "sections": 0.0}, - verifier_error=None, - rollout_name="plan-meeting__ab12cd34", ) ] ), tmp_path, ) assert "✗ plan-meeting: reward 0.3 — sections 0.0" in out - assert "details:" not in out + assert "test_x" not in out + assert f"(details: {tmp_path / 'plan-meeting__ab12cd34' / 'verifier'})" in out def test_report_eval_result_artifact_reads_stay_capped(tmp_path): @@ -866,7 +1062,7 @@ def test_report_eval_result_artifact_reads_stay_capped(tmp_path): # once per block, not per line. for i in range(7): _write_ctrf(tmp_path / f"task-{i}__ab12cd34" / "verifier", name=f"t::test_{i}") - failures = [_bare_failure(f"task-{i}", f"task-{i}__ab12cd34") for i in range(7)] + failures = [_failure(f"task-{i}", f"task-{i}__ab12cd34") for i in range(7)] out = _reported(_failed_result(failures), tmp_path) assert out.count("✗ task-") == 5 assert "… and 2 more" in out