From 8d19f1534105b4d4a43e434dadc5796c916057f6 Mon Sep 17 00:00:00 2001 From: jiarui Date: Sat, 15 Aug 2026 14:22:01 -0700 Subject: [PATCH 1/3] Flag chat-only completions (zero tool calls with model output) as a diagnostic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A rollout where the agent produced real output (tokens, agent messages) but ended without a single tool call was recorded as a clean scored fail, indistinguishable in aggregates from a genuine attempt (#988). The existing zero-signal net (suspected_api_error) only covers the zero-token half of the no-op space. Add NoToolCallCompletionDiagnostic: visibility only — reward and error stay untouched (chat-only completion is agent behavior, not infrastructure failure). Detection reuses the established guards (_executed_prompts, oracle exclusion, the native-subscription tool-telemetry exemption from PR #886) and additionally requires at least one agent_message trajectory event so trajectory-capture loss (#982) is not misflagged. Surfaces in result.json (no_tool_call_completion_info), the per-rollout CLI line (", no-op"), summary.json (no_tool_call_completions), and the registry-driven job summary warning, which now counts category-less diagnostics by field presence. Fixes #988 --- src/benchflow/diagnostics.py | 31 ++++++ src/benchflow/evaluation.py | 30 +++-- src/benchflow/models.py | 6 + src/benchflow/rollout/__init__.py | 66 +++++++++++ src/benchflow/rollout/_results.py | 3 + tests/test_no_tool_completion.py | 175 ++++++++++++++++++++++++++++++ 6 files changed, 303 insertions(+), 8 deletions(-) create mode 100644 tests/test_no_tool_completion.py diff --git a/src/benchflow/diagnostics.py b/src/benchflow/diagnostics.py index 3b73cd1d8..d2f651e2a 100644 --- a/src/benchflow/diagnostics.py +++ b/src/benchflow/diagnostics.py @@ -334,6 +334,36 @@ def format_issue(self, task_name: str) -> str: ) +@dataclass +class NoToolCallCompletionDiagnostic(Diagnostic): + """Chat-only completion: the agent produced model output (tokens and at + least one agent message) but ended its turn without issuing a single tool + call (#988). Unlike the api-error verdicts this is agent/model BEHAVIOR, + not an infrastructure failure — the reward and error channels are left + untouched (``category`` stays None), so the slot remains a scored fail. + The diagnostic only makes the no-op visible in result.json, the CLI line, + and the job summary so baseline sweeps don't read these rollouts as + genuine attempts.""" + + total_tokens: int = 0 + n_output_tokens: int = 0 + n_agent_messages: int = 0 + n_message_chars: int = 0 + + field: ClassVar[str] = "no_tool_call_completion_info" + category: ClassVar[str | None] = None + summary_description: ClassVar[str] = ( + "completed without a single tool call (chat-only)" + ) + + def format_issue(self, task_name: str) -> str: + return ( + f"{task_name}: chat-only completion — agent produced " + f"{self.n_output_tokens or self.total_tokens} tokens across " + f"{self.n_agent_messages} message(s) but made 0 tool calls" + ) + + # Public registry — every diagnostic kind goes here exactly once. DIAGNOSTIC_REGISTRY: tuple[type[Diagnostic], ...] = ( IdleTimeoutDiagnostic, @@ -343,6 +373,7 @@ def format_issue(self, task_name: str) -> str: VerifierTimeoutDiagnostic, ProviderApiErrorDiagnostic, SuspectedApiErrorDiagnostic, + NoToolCallCompletionDiagnostic, ) # field_name → Diagnostic class, for check_results lookup. diff --git a/src/benchflow/evaluation.py b/src/benchflow/evaluation.py index 277a5b987..56dc50f98 100644 --- a/src/benchflow/evaluation.py +++ b/src/benchflow/evaluation.py @@ -1403,8 +1403,11 @@ def _log_and_report(self, td: Path, result: RunResult) -> None: if isinstance(reward, (int, float)) and not isinstance(reward, bool) else "" ) + # Chat-only completions (#988): the agent answered but never called a + # tool — mark the line so a sweep log shows the no-op at a glance. + no_op = ", no-op" if getattr(result, "no_tool_completion", False) else "" logger.info( - f"[{status}] {td.name} ({reward_part}tools={result.n_tool_calls}){err}" + f"[{status}] {td.name} ({reward_part}tools={result.n_tool_calls}{no_op}){err}" ) self._fire_progress(self._on_result, td.name, result) @@ -1890,6 +1893,11 @@ async def run(self) -> EvaluationResult: "error": audit_counts["errored"], "verifier_errored": audit_counts["verifier_errored"], "idle_timeout": error_category_counts.get(IDLE_TIMEOUT, 0), + "no_tool_call_completions": sum( + 1 + for r in all_results.values() + if r.get("no_tool_call_completion_info") + ), "error_categories": error_category_counts or None, "verifier_error_categories": verifier_error_category_counts or None, "score": f"{pass_rate(passed=audit_counts['passed'], total=job_result.total):.1%}", @@ -1968,13 +1976,19 @@ async def run(self) -> EvaluationResult: # new diagnostic class adds its warning automatically (issue #503). for diag_cls in DIAGNOSTIC_REGISTRY: if diag_cls.category is None: - continue - counts = ( - error_category_counts - if diag_cls.channel == "error" - else verifier_error_category_counts - ) - count = counts.get(diag_cls.category, 0) + # Category-less diagnostics are behavior flags (e.g. chat-only + # completions, #988) that surface no error channel — count + # them by field presence in result.json instead. + count = sum( + 1 for r in all_results.values() if r.get(diag_cls.field) + ) + else: + counts = ( + error_category_counts + if diag_cls.channel == "error" + else verifier_error_category_counts + ) + count = counts.get(diag_cls.category, 0) if count > 0: logger.warning(summary_warning(diag_cls, count, job_result.total)) diff --git a/src/benchflow/models.py b/src/benchflow/models.py index b3558e0c5..c212086d0 100644 --- a/src/benchflow/models.py +++ b/src/benchflow/models.py @@ -104,6 +104,10 @@ class RolloutResult: and ``verifier_error``. See #389 follow-up. partial_trajectory: True when the trajectory was salvaged from a timed-out or crashed session and may be incomplete. + no_tool_completion: True when the agent produced model output but ended + without a single tool call (chat-only completion, #988). + Behavior visibility only — rewards and error stay as the + verifier/agent left them. trajectory_source: Provenance label for ``trajectory`` — one of ``"acp"`` (trusted), ``"scraped"`` (UNTRUSTED, agent-writable, forgeable), ``"partial_acp"`` (partial ACP capture). Verifier @@ -149,6 +153,7 @@ def __init__( verifier_error_category: str | None = None, export_error: str | None = None, partial_trajectory: bool = False, + no_tool_completion: bool = False, trajectory_source: TrajectorySource | None = None, reward_events: list[RewardEvent] | None = None, evolved_skills: dict[str, str] | None = None, @@ -181,6 +186,7 @@ def __init__( self.verifier_error_category = verifier_error_category self.export_error = export_error self.partial_trajectory = partial_trajectory + self.no_tool_completion = no_tool_completion self.trajectory_source = trajectory_source self.reward_events = reward_events self.evolved_skills = evolved_skills diff --git a/src/benchflow/rollout/__init__.py b/src/benchflow/rollout/__init__.py index 8e73b0f96..1588cdae9 100644 --- a/src/benchflow/rollout/__init__.py +++ b/src/benchflow/rollout/__init__.py @@ -89,6 +89,7 @@ from benchflow.contracts import RoundResult as RoundResult from benchflow.diagnostics import ( AgentPromptTimeoutError, + NoToolCallCompletionDiagnostic, ProviderApiErrorDiagnostic, RolloutDiagnostics, SuspectedApiErrorDiagnostic, @@ -2625,6 +2626,70 @@ def _maybe_classify_api_error(self) -> None: # excluded from score denominators (rerun-able, never counted). self._rewards = None + def _maybe_flag_no_tool_completion(self) -> None: + """Flag a chat-only completion: model output but zero tool calls (#988). + + The complement of the zero-signal heuristic above: the agent DID + respond (tokens, agent messages) yet ended its turn without a single + tool call — typically narrating intent ("Proceeding to add ...") and + stopping. That is agent/model behavior, not an infrastructure + failure, so unlike the api-error verdicts this leaves reward and + error untouched (the slot stays a scored fail); it only records a + diagnostic so result.json, the CLI line, and the job summary make + the no-op visible instead of letting baseline sweeps read it as a + genuine attempt. + """ + if self._error is not None: + return + # Same rationale as _maybe_classify_api_error: only judge rollouts + # where the agent actually ran (#389). + if not getattr(self, "_executed_prompts", None): + return + if getattr(self, "_n_tool_calls", 0): + return + config = getattr(self, "_config", None) + # Oracle rollouts run the reference solution directly — zero tool + # calls is their normal shape (they also record no executed prompts, + # so this is belt-and-braces). + if config is not None and config.primary_agent == "oracle": + return + # Same guard as _maybe_classify_api_error: agents whose trajectories + # carry no tool telemetry (e.g. omnigent's flat session events) look + # tool-free on every healthy run, so the zero-tool signal is + # meaningless there. TODO(#988): swap for a per-adapter + # tool-telemetry capability flag if maintainers prefer that gate. + from benchflow.agents.env import uses_native_subscription_auth + + if config is not None and uses_native_subscription_auth( + config.agent, + config.model, + getattr(self, "_agent_env", None) or {}, + ): + return + trajectory = getattr(self, "_trajectory", None) or [] + agent_messages = [ + e + for e in trajectory + if isinstance(e, dict) and e.get("type") == "agent_message" + ] + # No agent messages either: that is a zero-signal/capture question + # owned by the verdicts above (#982), not a chat-only completion. + if not agent_messages: + return + usage_metrics = getattr(self, "_usage_metrics", None) or {} + self._diagnostics.set( + NoToolCallCompletionDiagnostic( + total_tokens=_as_nonnegative_int(usage_metrics.get("total_tokens")), + n_output_tokens=_as_nonnegative_int( + usage_metrics.get("n_output_tokens") + ), + n_agent_messages=len(agent_messages), + n_message_chars=sum( + len(e.get("text") or "") for e in agent_messages + ), + ) + ) + def _loop_strategy_metadata(self) -> dict[str, Any] | None: """Loop-strategy run summary for the result.json ``loop`` block. @@ -2649,6 +2714,7 @@ def _loop_strategy_metadata(self) -> dict[str, Any] | None: def _build_result(self) -> RolloutResult: rollout_dir = self._require_rollout_dir() self._maybe_classify_api_error() + self._maybe_flag_no_tool_completion() # For Scene/multi-turn rollouts, each execute() call records the # prompt(s) it sent into self._executed_prompts. Use that as the # authoritative prompt list so n_prompts and prompts.json reflect diff --git a/src/benchflow/rollout/_results.py b/src/benchflow/rollout/_results.py index 588e5cbd6..88512dbb9 100644 --- a/src/benchflow/rollout/_results.py +++ b/src/benchflow/rollout/_results.py @@ -346,6 +346,9 @@ def _build_rollout_result( verifier_error_category=verifier_error_category, export_error=export_error, partial_trajectory=partial_trajectory, + no_tool_completion=( + diagnostics.get("no_tool_call_completion_info") is not None + ), trajectory_source=trajectory_source, evolved_skills=evolved_skills, source_provenance=source_provenance, diff --git a/tests/test_no_tool_completion.py b/tests/test_no_tool_completion.py new file mode 100644 index 000000000..cdb902813 --- /dev/null +++ b/tests/test_no_tool_completion.py @@ -0,0 +1,175 @@ +"""Chat-only completion capture (#988). + +The complement of the zero-signal net in test_api_error_capture.py: an agent +that produced real model output (tokens, agent messages) but ended its turn +without a single tool call. Motivating fixture: a baseline rollout that +narrated "Proceeding to add PLAN.md with the required sections." (2,727 +output tokens) and returned — previously recorded as a clean scored fail, +indistinguishable in aggregates from a genuine attempt. + +Semantics under test: the diagnostic is VISIBILITY ONLY — reward and error +stay exactly as the verifier/agent left them (chat-only completion is agent +behavior, not infrastructure failure), unlike suspected_api_error which +nulls the reward. +""" + +from types import SimpleNamespace + +from benchflow.diagnostics import ( + DIAGNOSTIC_BY_FIELD, + DIAGNOSTIC_REGISTRY, + NoToolCallCompletionDiagnostic, +) +from benchflow.models import RunResult + + +def _chat_only_trajectory() -> list[dict]: + return [ + {"type": "user_message", "text": "Add PLAN.md with the required sections."}, + { + "type": "agent_message", + "text": "Proceeding to add PLAN.md with the required sections.", + }, + ] + + +class _DiagBag: + def __init__(self): + self.recorded = [] + + def set(self, diag): + self.recorded.append(diag) + + +def _rollout_double( + *, + error=None, + executed_prompts=("p",), + n_tool_calls=0, + agent="claude-agent-acp", + agent_env=None, + trajectory=None, + usage_metrics=None, +): + from benchflow.rollout import Rollout + + r = Rollout.__new__(Rollout) + r._error = error + r._executed_prompts = list(executed_prompts) + r._agent_env = agent_env or {"BENCHFLOW_PROVIDER_NAME": "litellm"} + r._config = SimpleNamespace( + agent=agent, model="claude-haiku-4-5-20251001", primary_agent=agent + ) + r._usage_metrics = usage_metrics or { + "total_tokens": 30000, + "n_output_tokens": 2727, + } + r._n_tool_calls = n_tool_calls + r._trajectory = _chat_only_trajectory() if trajectory is None else trajectory + r._rewards = {"reward": 0.0} + r._diagnostics = _DiagBag() + return r + + +class TestDetection: + def test_chat_only_completion_flagged_reward_and_error_untouched(self): + r = _rollout_double() + r._maybe_flag_no_tool_completion() + assert len(r._diagnostics.recorded) == 1 + diag = r._diagnostics.recorded[0] + assert isinstance(diag, NoToolCallCompletionDiagnostic) + assert diag.n_output_tokens == 2727 + assert diag.n_agent_messages == 1 + assert diag.n_message_chars > 0 + # The whole point of the design: visibility only. + assert r._rewards == {"reward": 0.0} + assert r._error is None + + def test_run_with_tool_calls_never_flagged(self): + r = _rollout_double(n_tool_calls=9) + r._maybe_flag_no_tool_completion() + assert r._diagnostics.recorded == [] + + def test_errored_rollout_not_flagged(self): + # api_error / suspected_api_error / timeout paths own their channels. + r = _rollout_double(error="suspected provider api error: ...") + r._maybe_flag_no_tool_completion() + assert r._diagnostics.recorded == [] + + def test_setup_failure_path_not_flagged(self): + # No executed prompts -> the agent never ran (#389). + r = _rollout_double(executed_prompts=()) + r._maybe_flag_no_tool_completion() + assert r._diagnostics.recorded == [] + + def test_oracle_never_flagged(self): + r = _rollout_double(agent="oracle") + r._maybe_flag_no_tool_completion() + assert r._diagnostics.recorded == [] + + def test_no_agent_messages_not_flagged(self): + # Zero output AND zero messages is zero-signal/capture territory + # (suspected_api_error, #982) — not a chat-only completion. + r = _rollout_double(trajectory=[{"type": "user_message", "text": "p"}]) + r._maybe_flag_no_tool_completion() + assert r._diagnostics.recorded == [] + + def test_native_subscription_telemetry_gap_not_flagged(self): + # Same exemption as the zero-signal heuristic (PR #886): flat-telemetry + # agents look tool-free on every healthy run. + r = _rollout_double(agent_env={"CLAUDE_CODE_OAUTH_TOKEN": "oauth-token"}) + r._maybe_flag_no_tool_completion() + assert r._diagnostics.recorded == [] + + +class TestPrecedence: + def test_zero_token_case_still_routes_to_suspected_api_error(self): + # The zero-signal net runs first and claims the error channel; the + # chat-only flag must then decline (error is set). + r = _rollout_double( + usage_metrics={"total_tokens": 0, "n_output_tokens": 0}, + trajectory=[], + ) + r._api_failure_summary_cached = None + r._maybe_classify_api_error() + assert "suspected provider api error" in (r._error or "") + assert r._rewards is None + r._maybe_flag_no_tool_completion() + chat_only = [ + d + for d in r._diagnostics.recorded + if isinstance(d, NoToolCallCompletionDiagnostic) + ] + assert chat_only == [] + + +class TestDiagnosticsRegistry: + def test_registered(self): + assert NoToolCallCompletionDiagnostic in DIAGNOSTIC_REGISTRY + assert ( + DIAGNOSTIC_BY_FIELD["no_tool_call_completion_info"] + is NoToolCallCompletionDiagnostic + ) + + def test_no_error_category(self): + # Deliberate: chat-only completion is not an error, so it must never + # surface an error_category or leave the scored-fail bucket. + assert NoToolCallCompletionDiagnostic.category is None + + def test_format_issue(self): + diag = NoToolCallCompletionDiagnostic( + total_tokens=30000, + n_output_tokens=2727, + n_agent_messages=1, + n_message_chars=55, + ) + line = diag.format_issue("some-task") + assert "chat-only" in line and "2727" in line and "0 tool calls" in line + + +class TestResultSurface: + def test_run_result_defaults_false(self): + assert RunResult(task_name="t").no_tool_completion is False + + def test_run_result_carries_flag(self): + assert RunResult(task_name="t", no_tool_completion=True).no_tool_completion From 01bb6bcb5aecc822e4ab3b41b497596d0a94653c Mon Sep 17 00:00:00 2001 From: jiarui Date: Sun, 16 Aug 2026 20:28:44 -0700 Subject: [PATCH 2/3] Address review: fresh-run summary aggregation, scraped-trajectory guard, formatting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings from PR #1025 (thanks @Galius5136): 1. Fresh runs build summary rows from the in-memory RolloutResult via rollout_result_payload(), which carries no diagnostic payloads — so no_tool_call_completions read 0 on a fresh job and only became correct on resume, when rows are read back from result.json. Generalize the #501 persisted-timing enrichment into _enrich_payload_with_persisted_fields(), which now also copies every DIAGNOSTIC_REGISTRY field from the persisted result.json, making fresh and resumed aggregation identical. 2. Salvage paths (the gemini scraped-trajectory fallback) rebuild tool_call events the ACP session never counted, so _n_tool_calls can be 0 while the trajectory shows real tool activity. The detection now treats the trajectory as authoritative: any tool_call event in it disqualifies the chat-only flag. 3. ruff format on evaluation.py and rollout/__init__.py; the full 'ruff format --check src tests tools' sweep now passes. New tests: scraped-trajectory negative case; enrichment copies diagnostic fields, skips nulls, never overwrites in-memory values, and stays silent when result.json is absent. --- src/benchflow/evaluation.py | 48 +++++++++-------- src/benchflow/rollout/__init__.py | 13 +++-- tests/test_no_tool_completion.py | 88 +++++++++++++++++++++++++++++++ 3 files changed, 123 insertions(+), 26 deletions(-) diff --git a/src/benchflow/evaluation.py b/src/benchflow/evaluation.py index 56dc50f98..06d3e7b2d 100644 --- a/src/benchflow/evaluation.py +++ b/src/benchflow/evaluation.py @@ -1164,20 +1164,21 @@ def _prune_docker(self): finally: _PRUNE_LOCK.release() - def _enrich_payload_with_persisted_timing( + def _enrich_payload_with_persisted_fields( self, payload: dict, result: RolloutResult ) -> None: - """Copy ``timing`` from the rollout's on-disk result.json into payload. - - ``RolloutResult`` does not carry phase timing, but the rollout writer - (``rollout.py``) persists it under ``rollout_dir/result.json``. Reading - it back lets ``phase_timing_summary`` aggregate phase totals for fresh - runs (issue #501). Best-effort: legacy SDK paths that mock the writer - — or any case where no rollout_name is set — silently leave timing - absent rather than crash summary generation. + """Copy persisted-only fields from the rollout's on-disk result.json. + + ``RolloutResult`` does not carry phase timing or the structured + diagnostic payloads, but the rollout writer (``rollout.py``) persists + both under ``rollout_dir/result.json``. Reading them back lets + ``phase_timing_summary`` (issue #501) and the diagnostic counters + (#988) aggregate fresh runs the same way they cover resumed tasks, + which are read straight from result.json. Best-effort: legacy SDK + paths that mock the writer — or any case where no rollout_name is + set — silently leave the fields absent rather than crash summary + generation. """ - if "timing" in payload: - return rollout_name = getattr(result, "rollout_name", "") or "" if not rollout_name: return @@ -1187,11 +1188,15 @@ def _enrich_payload_with_persisted_timing( try: persisted = json.loads(rfile.read_text()) except (json.JSONDecodeError, OSError) as e: - logger.debug("Could not read persisted timing from %s: %s", rfile, e) + logger.debug("Could not read persisted fields from %s: %s", rfile, e) return timing = persisted.get("timing") - if isinstance(timing, dict): + if "timing" not in payload and isinstance(timing, dict): payload["timing"] = timing + for diag_cls in DIAGNOSTIC_REGISTRY: + info = persisted.get(diag_cls.field) + if diag_cls.field not in payload and isinstance(info, dict): + payload[diag_cls.field] = info async def _run_single_task( self, task_dir: Path, cfg: EvaluationConfig @@ -1802,10 +1807,11 @@ async def run(self) -> EvaluationResult: task_name=name, ) # ``rollout_result_payload`` is RolloutResult-driven and so cannot - # see ``timing`` (it lives only in the persisted result.json). - # Pull it from disk so phase-timing aggregates cover fresh pairs - # the same way they cover resumed tasks (issue #501). - self._enrich_payload_with_persisted_timing(payload, result) + # see ``timing`` or the diagnostic payloads (they live only in + # the persisted result.json). Pull them from disk so phase-timing + # (#501) and diagnostic aggregates (#988) cover fresh pairs the + # same way they cover resumed tasks. + self._enrich_payload_with_persisted_fields(payload, result) all_results[name] = payload # EvaluationResult is the score/invariant view. summary.json is the @@ -1894,9 +1900,7 @@ async def run(self) -> EvaluationResult: "verifier_errored": audit_counts["verifier_errored"], "idle_timeout": error_category_counts.get(IDLE_TIMEOUT, 0), "no_tool_call_completions": sum( - 1 - for r in all_results.values() - if r.get("no_tool_call_completion_info") + 1 for r in all_results.values() if r.get("no_tool_call_completion_info") ), "error_categories": error_category_counts or None, "verifier_error_categories": verifier_error_category_counts or None, @@ -1979,9 +1983,7 @@ async def run(self) -> EvaluationResult: # Category-less diagnostics are behavior flags (e.g. chat-only # completions, #988) that surface no error channel — count # them by field presence in result.json instead. - count = sum( - 1 for r in all_results.values() if r.get(diag_cls.field) - ) + count = sum(1 for r in all_results.values() if r.get(diag_cls.field)) else: counts = ( error_category_counts diff --git a/src/benchflow/rollout/__init__.py b/src/benchflow/rollout/__init__.py index 1588cdae9..ba272cefd 100644 --- a/src/benchflow/rollout/__init__.py +++ b/src/benchflow/rollout/__init__.py @@ -2667,6 +2667,15 @@ def _maybe_flag_no_tool_completion(self) -> None: ): return trajectory = getattr(self, "_trajectory", None) or [] + # The tool-call counter and the trajectory can disagree on salvage + # paths: the scraped-trajectory fallback (e.g. gemini's native + # trajectory file) rebuilds tool_call events the ACP session never + # counted, so _n_tool_calls stays 0 while the trajectory shows real + # tool activity. Any tool_call event means the agent DID act. + if any( + isinstance(e, dict) and e.get("type") == "tool_call" for e in trajectory + ): + return agent_messages = [ e for e in trajectory @@ -2684,9 +2693,7 @@ def _maybe_flag_no_tool_completion(self) -> None: usage_metrics.get("n_output_tokens") ), n_agent_messages=len(agent_messages), - n_message_chars=sum( - len(e.get("text") or "") for e in agent_messages - ), + n_message_chars=sum(len(e.get("text") or "") for e in agent_messages), ) ) diff --git a/tests/test_no_tool_completion.py b/tests/test_no_tool_completion.py index cdb902813..e61a9c09f 100644 --- a/tests/test_no_tool_completion.py +++ b/tests/test_no_tool_completion.py @@ -121,6 +121,27 @@ def test_native_subscription_telemetry_gap_not_flagged(self): r._maybe_flag_no_tool_completion() assert r._diagnostics.recorded == [] + def test_scraped_trajectory_with_tool_calls_not_flagged(self): + # Salvage paths (e.g. the gemini scraped-trajectory fallback) rebuild + # tool_call events the ACP session never counted: _n_tool_calls stays + # 0 while the trajectory shows real tool activity. The trajectory is + # authoritative — never flag those. + trajectory = [ + *_chat_only_trajectory(), + { + "type": "tool_call", + "tool_call_id": "call-1", + "kind": "execute", + "title": "python probe.py", + "status": "completed", + "content": [], + }, + ] + r = _rollout_double(trajectory=trajectory) + assert r._n_tool_calls == 0 + r._maybe_flag_no_tool_completion() + assert r._diagnostics.recorded == [] + class TestPrecedence: def test_zero_token_case_still_routes_to_suspected_api_error(self): @@ -173,3 +194,70 @@ def test_run_result_defaults_false(self): def test_run_result_carries_flag(self): assert RunResult(task_name="t", no_tool_completion=True).no_tool_completion + + +class TestFreshRunSummaryEnrichment: + """Fresh runs build summary rows from in-memory RolloutResult via + rollout_result_payload(), which carries no diagnostic payloads — only the + persisted result.json has them. The enrichment step must pull them back + so fresh and resumed runs aggregate identically (PR #1025 review).""" + + def _evaluation_double(self, jobs_dir): + from benchflow.evaluation import Evaluation + + ev = Evaluation.__new__(Evaluation) + ev._jobs_dir = jobs_dir + ev._job_name = "job" + return ev + + def test_payload_gains_persisted_diagnostic_fields(self, tmp_path): + import json + + rollout_dir = tmp_path / "job" / "r1" + rollout_dir.mkdir(parents=True) + info = { + "total_tokens": 30000, + "n_output_tokens": 2727, + "n_agent_messages": 1, + "n_message_chars": 55, + } + (rollout_dir / "result.json").write_text( + json.dumps( + { + "timing": {"total": 12.5}, + "no_tool_call_completion_info": info, + "suspected_api_error_info": None, + } + ) + ) + ev = self._evaluation_double(tmp_path) + payload = {} + result = RunResult(task_name="t", rollout_name="r1") + ev._enrich_payload_with_persisted_fields(payload, result) + assert payload["no_tool_call_completion_info"] == info + assert payload["timing"] == {"total": 12.5} + # Null diagnostics must not materialize as keys. + assert "suspected_api_error_info" not in payload + + def test_missing_result_json_is_silent(self, tmp_path): + ev = self._evaluation_double(tmp_path) + payload = {} + ev._enrich_payload_with_persisted_fields( + payload, RunResult(task_name="t", rollout_name="absent") + ) + assert payload == {} + + def test_existing_payload_fields_not_overwritten(self, tmp_path): + import json + + rollout_dir = tmp_path / "job" / "r1" + rollout_dir.mkdir(parents=True) + (rollout_dir / "result.json").write_text( + json.dumps({"timing": {"total": 99.0}}) + ) + ev = self._evaluation_double(tmp_path) + payload = {"timing": {"total": 1.0}} + ev._enrich_payload_with_persisted_fields( + payload, RunResult(task_name="t", rollout_name="r1") + ) + assert payload["timing"] == {"total": 1.0} From c378761ed6438b4e214ec7c367faa55d39881686 Mon Sep 17 00:00:00 2001 From: jiarui Date: Sun, 16 Aug 2026 23:10:33 -0700 Subject: [PATCH 3/3] Add Evaluation-level wiring regression test for fresh-run enrichment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on PR #1025: removing the _enrich_payload_with_persisted_fields() call site left the suite green, because the existing tests exercise the helper in isolation. This test runs a real Evaluation.run() over a mocked rollout that persists no_tool_call_completion_info only to its on-disk result.json — exactly the fresh-run shape — and asserts summary.json counts it on the first pass. Verified red with the call site removed, green with it present. --- tests/test_no_tool_completion.py | 60 ++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/tests/test_no_tool_completion.py b/tests/test_no_tool_completion.py index e61a9c09f..1e64fd891 100644 --- a/tests/test_no_tool_completion.py +++ b/tests/test_no_tool_completion.py @@ -15,6 +15,8 @@ from types import SimpleNamespace +import pytest + from benchflow.diagnostics import ( DIAGNOSTIC_BY_FIELD, DIAGNOSTIC_REGISTRY, @@ -261,3 +263,61 @@ def test_existing_payload_fields_not_overwritten(self, tmp_path): payload, RunResult(task_name="t", rollout_name="r1") ) assert payload["timing"] == {"total": 1.0} + + +@pytest.mark.asyncio +async def test_fresh_run_summary_counts_chat_only_completion(tmp_path): + """End-to-end wiring regression (PR #1025 review): a fresh Evaluation.run() + whose rollout persisted ``no_tool_call_completion_info`` must count it in + summary.json on the FIRST pass — not only after a resume re-reads + result.json. This test fails if the enrichment call site in the fresh-pair + loop is removed.""" + import json + from unittest.mock import AsyncMock + + from benchflow.evaluation import Evaluation, EvaluationConfig, RetryConfig + + task_dir = tmp_path / "chat-only-task" + task_dir.mkdir(parents=True) + (task_dir / "task.toml").write_text( + 'version = "1.0"\n[verifier]\ntimeout_sec = 60\n' + "[agent]\ntimeout_sec = 60\n[environment]\n" + ) + jobs_dir = tmp_path / "jobs" + + cfg = EvaluationConfig(retry=RetryConfig(max_retries=0)) + job = Evaluation( + tasks_dir=task_dir, jobs_dir=jobs_dir, config=cfg, job_name="fresh-run" + ) + + async def run_and_persist(*args, **kwargs): + # Simulate the rollout writer: the diagnostic payload exists ONLY in + # the on-disk result.json, exactly like a real fresh run. + rollout_dir = jobs_dir / "fresh-run" / "r1" + rollout_dir.mkdir(parents=True, exist_ok=True) + (rollout_dir / "result.json").write_text( + json.dumps( + { + "no_tool_call_completion_info": { + "total_tokens": 30000, + "n_output_tokens": 2727, + "n_agent_messages": 1, + "n_message_chars": 55, + } + } + ) + ) + return RunResult( + task_name="chat-only-task", + rollout_name="r1", + rewards={"reward": 0.0}, + no_tool_completion=True, + ) + + job._sdk = AsyncMock() + job._sdk.run = AsyncMock(side_effect=run_and_persist) + + await job.run() + + summary = json.loads((jobs_dir / "fresh-run" / "summary.json").read_text()) + assert summary["no_tool_call_completions"] == 1