From 69cb217603813495db194b78e3215d08993b373e Mon Sep 17 00:00:00 2001 From: Bingran You Date: Mon, 6 Jul 2026 01:37:58 -0700 Subject: [PATCH 1/2] Fix diagnostic serialization and heartbeat cleanup --- src/benchflow/diagnostics.py | 23 ++--------- src/benchflow/sandbox/daytona_dind.py | 15 +++++-- tests/test_acp.py | 58 +++++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 23 deletions(-) diff --git a/src/benchflow/diagnostics.py b/src/benchflow/diagnostics.py index f00c41cc..17022a19 100644 --- a/src/benchflow/diagnostics.py +++ b/src/benchflow/diagnostics.py @@ -93,8 +93,8 @@ class Diagnostic: summary_description: ClassVar[str] = "" def to_dict(self) -> dict[str, Any]: - """Serialize for result.json — drops Nones is the caller's choice.""" - return asdict(self) + """Serialize for result.json, omitting unpopulated optional fields.""" + return {k: v for k, v in asdict(self).items() if v is not None} def format_issue(self, task_name: str) -> str: """Render the per-task line check_results emits for this diagnostic.""" @@ -237,24 +237,7 @@ def to_dict(self) -> dict[str, Any]: the same way so result.json stays compact and tests that compare explicit dicts continue to work. """ - raw = asdict(self) - out: dict[str, Any] = {} - for k, v in raw.items(): - if v is None and k in { - "stderr_snippet", - "process_pid", - "sandbox_reachable", - "sandbox_probe_rc", - "sandbox_probe_stdout", - "sandbox_probe_error", - "sandbox_probe_error_type", - "sandbox_probe_traceback", - }: - continue - if k == "raw_message" and not v: - continue - out[k] = v - return out + return {k: v for k, v in super().to_dict().items() if k != "raw_message" or v} @dataclass diff --git a/src/benchflow/sandbox/daytona_dind.py b/src/benchflow/sandbox/daytona_dind.py index a5397efd..048942da 100644 --- a/src/benchflow/sandbox/daytona_dind.py +++ b/src/benchflow/sandbox/daytona_dind.py @@ -113,8 +113,13 @@ def _with_long_exec_heartbeat(command: str, timeout_sec: int | None) -> str: f'({command}\n) >"$bf_out" 2>"$bf_err" &', "bf_pid=$!", "(", + ' bf_sleep_pid=""', + ' trap \'[ -n "$bf_sleep_pid" ] && kill "$bf_sleep_pid" 2>/dev/null || true; exit 0\' TERM INT EXIT', ' while kill -0 "$bf_pid" 2>/dev/null; do', - f" sleep {interval}", + f" sleep {interval} &", + " bf_sleep_pid=$!", + ' wait "$bf_sleep_pid" 2>/dev/null || exit 0', + ' bf_sleep_pid=""', ' if kill -0 "$bf_pid" 2>/dev/null; then', " echo '[benchflow] Daytona DinD command still running' >&2", " fi", @@ -339,8 +344,12 @@ async def start(self, force_build: bool) -> None: env._client_manager = await _sdk.DaytonaClientManager.get_instance() - dind_image: str = env._kwargs.get("dind_image", "docker:28.3.3-dind") - dind_snapshot: str | None = env._kwargs.get("dind_snapshot") + dind_image = env._kwargs.get("dind_image", "docker:28.3.3-dind") + if not isinstance(dind_image, str): + raise TypeError("dind_image must be a string") + dind_snapshot = env._kwargs.get("dind_snapshot") + if dind_snapshot is not None and not isinstance(dind_snapshot, str): + raise TypeError("dind_snapshot must be a string") params: _SandboxParams if dind_snapshot: diff --git a/tests/test_acp.py b/tests/test_acp.py index 663dc1d2..5af510ac 100644 --- a/tests/test_acp.py +++ b/tests/test_acp.py @@ -1728,6 +1728,64 @@ def test_diagnostic_reason_constants_share_scoring_source(self) -> None: DIAGNOSTIC_REASON_TRANSPORT_CLOSED, ) + def test_diagnostic_serialization_drops_none_fields_uniformly(self) -> None: + """Guards the fix from PR #895 for issue #531: diagnostic info dicts use one None-policy.""" + from benchflow.diagnostics import ( + AgentPromptTimeoutDiagnostic, + IdleTimeoutDiagnostic, + ProviderApiErrorDiagnostic, + SandboxStartupDiagnostic, + SuspectedApiErrorDiagnostic, + TransportClosedDiagnostic, + VerifierTimeoutDiagnostic, + ) + + expected_keys_by_cls = { + IdleTimeoutDiagnostic: { + "reason", + "idle_timeout_sec", + "idle_duration_sec", + "wall_clock_elapsed_sec", + "n_tool_calls", + "n_message_chunks", + "n_thought_chunks", + "last_activity_at", + }, + AgentPromptTimeoutDiagnostic: { + "reason", + "timeout_sec", + "n_tool_calls", + "pending_tool_call_ids", + "terminal_event_recorded", + "terminal_trajectory_complete", + }, + SandboxStartupDiagnostic: {"reason", "attempts", "raw_message"}, + TransportClosedDiagnostic: {"reason", "transport_diagnosis"}, + VerifierTimeoutDiagnostic: { + "timeout_budget_sec", + "elapsed_sec", + "task_name", + }, + ProviderApiErrorDiagnostic: { + "subcategory", + "transient", + "total_requests", + "failed_requests", + "fingerprint", + }, + SuspectedApiErrorDiagnostic: { + "total_tokens", + "n_tool_calls", + "total_requests", + "failed_requests", + }, + } + + for diag_cls, expected_keys in expected_keys_by_cls.items(): + payload = diag_cls().to_dict() + assert set(payload) == expected_keys + assert all(value is not None for value in payload.values()) + def test_summary_warning_uses_registry_metadata(self) -> None: """Summary warning text comes from the registry's ``summary_description``, not a per-call f-string in evaluation.py.""" From b572f77187a28724ae778647ba324fc494219e90 Mon Sep 17 00:00:00 2001 From: Bingran You Date: Fri, 10 Jul 2026 09:55:01 -0700 Subject: [PATCH 2/2] fix(daytona): structure DinD config errors --- src/benchflow/sandbox/daytona_dind.py | 13 +++++++++++-- tests/test_daytona_dind_compose_up.py | 14 ++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/src/benchflow/sandbox/daytona_dind.py b/src/benchflow/sandbox/daytona_dind.py index 2dd8be44..d12a291c 100644 --- a/src/benchflow/sandbox/daytona_dind.py +++ b/src/benchflow/sandbox/daytona_dind.py @@ -96,6 +96,15 @@ def _raise_if_startup_provider_error(env: DaytonaSandbox, exc: BaseException) -> ) from exc +def _startup_config_error(env: DaytonaSandbox, message: str) -> SandboxStartupError: + return SandboxStartupError( + message, + sandbox_state="config_error", + attempts=0, + build_timeout_sec=env.task_env_config.build_timeout_sec, + ) + + def _with_long_exec_heartbeat(command: str, timeout_sec: int | None) -> str: if ( timeout_sec is None @@ -351,10 +360,10 @@ async def start(self, force_build: bool) -> None: dind_image = env._kwargs.get("dind_image", "docker:28.3.3-dind") if not isinstance(dind_image, str): - raise TypeError("dind_image must be a string") + raise _startup_config_error(env, "dind_image must be a string") dind_snapshot = env._kwargs.get("dind_snapshot") if dind_snapshot is not None and not isinstance(dind_snapshot, str): - raise TypeError("dind_snapshot must be a string") + raise _startup_config_error(env, "dind_snapshot must be a string") params: _SandboxParams if dind_snapshot: diff --git a/tests/test_daytona_dind_compose_up.py b/tests/test_daytona_dind_compose_up.py index a1103515..6781ac70 100644 --- a/tests/test_daytona_dind_compose_up.py +++ b/tests/test_daytona_dind_compose_up.py @@ -149,6 +149,20 @@ def test_daytona_sdk_error_during_dind_start_is_structured_startup_error(): assert "Daytona DinD startup failed" in str(raised.value) +def test_daytona_dind_type_guard_uses_structured_startup_error(): + """Guards the follow-up fix from PR #895 for invalid DinD config values.""" + from benchflow.sandbox.daytona_dind import _startup_config_error + + env = SimpleNamespace( + task_env_config=SimpleNamespace(build_timeout_sec=1800), + ) + err = _startup_config_error(env, "dind_image must be a string") + assert isinstance(err, SandboxStartupError) + assert err.diagnostic.sandbox_state == "config_error" + assert err.diagnostic.build_timeout_sec == 1800 + assert "dind_image must be a string" in err.diagnostic.raw_message + + def test_long_exec_heartbeat_only_wraps_long_commands(): command = "python preprocess.py" assert _with_long_exec_heartbeat(command, 599) == command