fix(diagnostics): unify diagnostic serialization

[bingran-you]

Purpose

Fixes the remaining open scope of #531 after #858: diagnostic to_dict() serialization had an inconsistent None policy. The base Diagnostic.to_dict() kept None fields while TransportClosedDiagnostic.to_dict() dropped selected optional fields, so result JSON diagnostic info shapes were not governed by one rule.

Changes

• Makes the base Diagnostic.to_dict() omit unpopulated None fields for every diagnostic subclass.
• Keeps the existing TransportClosedDiagnostic compacting behavior for empty raw_message, while delegating None filtering to the shared base method.
• Adds a registry-level schema-stability regression test covering every current diagnostic subclass and asserting serialized diagnostic payloads contain no None values.
• Fixes the unrelated CI failure exposed by the first PR run: the Daytona DinD long-exec heartbeat subprocess now kills its in-flight sleep child on termination, so fast commands return promptly. Also adds explicit type guards for Daytona DinD kwargs that ty flagged while validating the file.

Validation

• uv run --extra dev python -m pytest tests/test_acp.py::TestDiagnosticRegistry -q -> 6 passed
• uv run --extra dev python -m pytest tests/test_acp.py tests/test_api_error_capture.py tests/test_oracle_chokepoint.py tests/test_integration_check_results.py::test_check_results_flags_sandbox_startup_error_with_diagnostics tests/test_integration_check_results.py::test_check_results_flags_transport_error_with_diagnostics -q -> 153 passed, 1 skipped
• uv run --extra dev python -m pytest tests/test_daytona_dind_compose_up.py -q -> 12 passed
• uv run --extra dev python -m pytest tests/test_daytona_dind_compose_up.py::test_long_exec_heartbeat_returns_promptly_after_fast_command tests/test_daytona_dind_compose_up.py::test_long_exec_heartbeat_only_wraps_long_commands tests/test_acp.py::TestDiagnosticRegistry -q -> 8 passed
• uv run --extra dev ruff check src/benchflow/diagnostics.py src/benchflow/sandbox/daytona_dind.py tests/test_acp.py tests/test_daytona_dind_compose_up.py
• uv run --extra dev ruff format --check src/benchflow/diagnostics.py src/benchflow/sandbox/daytona_dind.py tests/test_acp.py tests/test_daytona_dind_compose_up.py
• uv run --extra dev ty check src/benchflow/diagnostics.py src/benchflow/sandbox/daytona_dind.py
• git diff --check

Notes

This intentionally leaves price_source enum modeling out of scope, matching the narrowed #531 follow-up. It does change optional diagnostic info dicts to omit unset optional fields consistently; top-level diagnostic fields in result.json remain present through RolloutDiagnostics.to_result_fields().

Closes #531

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[bingran-you]

Automation follow-up (2026-07-06): the first CI run failed in tests/test_daytona_dind_compose_up.py::test_long_exec_heartbeat_returns_promptly_after_fast_command, outside the initial diagnostics diff. I reproduced the issue from the log: a fast command could finish while the heartbeat subshell was still inside sleep, and killing only the subshell could leave the sleep child alive long enough for the wrapper to miss the 2s prompt-return budget.

Pushed 69cb21760 with two additions:

• heartbeat subprocess now traps termination and kills its in-flight sleep child, so fast commands return promptly;
• Daytona DinD dind_image / dind_snapshot kwargs now have explicit string type guards, clearing the focused ty validation for the file.

Updated validation:

• uv run --extra dev python -m pytest tests/test_daytona_dind_compose_up.py -q -> 12 passed
• uv run --extra dev python -m pytest tests/test_acp.py::TestDiagnosticRegistry -q -> 6 passed
• focused ruff check / format check / ty check passed for touched files.

Keeping status:blocked until the new CI run finishes green.

[bingran-you]

Automation status update (2026-07-06): refreshed CI is green on 69cb21760.

Current GitHub checks:

• test passed.
• pip-audit passed.
• manifest-parity / parity passed.
• integration-light / rollout-smoke passed.
• integration-scope matrix/review jobs skipped as expected for this light-scope diff.

Moved labels from status:blocked to status:ready; keeping review:pending for non-author human review before merge.

[bingran-you]

Users Simulation automation review (2026-07-06): ready by simulation on head 69cb2176.

Evidence:

• Isolated worktree checks passed: focused pytest (tests/test_acp.py, tests/test_daytona_dind_compose_up.py: 91 passed, 1 skipped), targeted diagnostics integration checks, ruff, ruff format --check, ty, and CLI smoke (bench --help, bench agent list, bench tasks --help, known-good bench tasks check).
• CI integration-light rollout artifact was downloaded and revalidated locally with .agents/skills/benchflow-experiment-review/scripts/validate_run_artifacts.py: 1/1 rollout healthy, ACP + LLM trajectories present, 24 LLM exchanges with provider usage, results.jsonl training-ready, reward 1, 420,929 tokens, 22 tool calls, timing total 358.2s.

Caveat: the CI GLM user-endpoint artifact still has agent_result.cost_usd: null / price_source: null; token usage and timing are healthy, but this does not satisfy the stricter Users Simulation request for non-null USD cost metadata. No code blocker found in this PR.

[bingran-you]

Daily scan follow-up (2026-07-07): resolved the merge conflict on current head fe6f8879c3168ffb048e56a86a5deaec61895ea1 with a non-force push.

Conflict resolution was limited to src/benchflow/sandbox/daytona_dind.py, preserving origin/main's bf_stop_heartbeat helper and this PR's EXIT cleanup coverage. Local validation in an isolated worktree passed:

• uv run python -m pytest tests/test_acp.py::TestDiagnosticRegistry tests/test_daytona_dind_compose_up.py::test_long_exec_heartbeat_only_wraps_long_commands tests/test_daytona_dind_compose_up.py::test_long_exec_heartbeat_returns_promptly_after_fast_command (8 passed)
• uv run ruff check .
• uv run ty check src/
• uv run ruff format --check .

GitHub now reports the PR mergeable; rollout-smoke is still pending at this comment time.

[bingran-you]

Users Simulation automation review (2026-07-07): ready by simulation on current head fe6f8879c3168ffb048e56a86a5deaec61895ea1.

Evidence:

• Isolated PR checks passed: uv sync --extra dev --extra sandbox-daytona --locked, tests/test_acp.py, tests/test_daytona_dind_compose_up.py, focused ruff, focused ty, and bench tasks check on the repo demo task plus a longer-timeout temp copy.
• A live Claude Haiku subscription-auth canary completed functionally on Docker: rollout root /tmp/benchflow-pr895-cli9.wbPJNL/2026-07-07__05-29-24/benchflow-demo-long__eb305920, reward 1.0, 1 tool call, 64,785 tokens, ACP trajectory present. That local canary is not publishable training data because native subscription auth omitted trajectory/llm_trajectory.jsonl and results.jsonl was not training-ready.
• The current GitHub integration-light-jobs artifact for this same head was downloaded and passed the hard trajectory gate locally: 1/1 healthy rollout, ACP + LLM trajectories, provider usage, results.jsonl training-ready, reward 1.0, 228,564 tokens, 13 tool calls, timing total 305.7s.

Caveat: the GLM user-endpoint artifact has healthy tokens/timing but agent_result.cost_usd / price_source are null, so USD cost metadata is not fully covered for that provider path. No code regression found in this PR's diagnostics/Daytona DinD diff.

[bingran-you]

Users Simulation automation review (2026-07-08): ready by simulation on head fe6f8879c3168ffb048e56a86a5deaec61895ea1.

Evidence:

• Diagnostic serialization behavior validated: TransportClosedDiagnostic serializes compactly with None fields omitted, and format_issue_for_field("transport_error_info", ...) renders the expected diagnostic message.
• CLI/help surfaces passed in simulation: benchflow --help, benchflow eval --help, benchflow doctor --help, benchflow train --help, benchflow sandbox --help, benchflow skills --help, benchflow continue --help.
• Focused tests passed: tests/test_acp.py -k DiagnosticRegistry, tests/test_cli_docs_drift.py, tests/test_init_cli.py -k doctor.
• I re-ran the focused clean-check after a subagent reported local static noise: uv run ty check src/benchflow/diagnostics.py src/benchflow/sandbox/daytona_dind.py passed in a fresh PR checkout.
• Shared base sweep passed: ruff check ., ty check src, and 149 CLI/SDK/task/usage/skill tests.
• Current-head integration-light-jobs artifact passed benchflow-experiment-review: ACP + LLM trajectories and training-ready results.jsonl; reward 1.0, 228,564 tokens, 13 tool calls, total timing 305.7s.

Thermo-nuclear review: no structural blocker found. The diagnostic change is narrow and does not add broad special-case branching.

Metadata caveat: GLM artifact token/timing coverage is healthy, but result.json.agent_result.cost_usd / price_source are null. This is a shared GLM user-endpoint cost-accounting gap, not a PR-specific diagnostics regression.

Labels: status:ready / review:pending.

[bingran-you]

Users Simulation automation review (2026-07-10): blocked on head fe6f8879c3168ffb048e56a86a5deaec61895ea1.

What passed:

• benchflow --help, benchflow eval --help, benchflow eval run --help, benchflow tasks --help
• benchflow tasks check src/benchflow/demo_task
• diagnostics serialization smoke: all diagnostic classes serialize without top-level None fields
• focused diagnostics/result-json tests and tests/test_daytona_dind_compose_up.py with a temporary ACP import shim in the isolated review worktree
• ruff check src/benchflow/diagnostics.py src/benchflow/sandbox/daytona_dind.py tests/test_acp.py

Blocking finding:

• src/benchflow/sandbox/daytona_dind.py:352-357 now raises bare TypeError for invalid dind_image / dind_snapshot. That bypasses the existing SandboxStartupError diagnostic path, so a startup config mistake no longer lands in structured sandbox_startup_info and instead becomes a generic runtime failure. Please validate these values earlier in the config boundary or rethrow as SandboxStartupError so Daytona DinD startup failures stay structured.

No model-backed rollout artifacts were accepted in this pass; the blocker is in the startup-diagnostic path before the rollout artifact gate.

[bingran-you]

Daily scan follow-up (2026-07-10): pushed b572f7718 to address the current-head structured-diagnostic blocker.

Fix:

• Invalid Daytona DinD config values for dind_image / dind_snapshot now raise SandboxStartupError with sandbox_state="config_error" instead of bare TypeError.
• Added a PR fix(diagnostics): unify diagnostic serialization #895 regression test so the invalid-config path keeps producing structured sandbox_startup_info.

Local validation in /tmp/benchflow-daily-pr895:

• uv run --extra dev python -m pytest tests/test_daytona_dind_compose_up.py tests/test_acp.py::TestDiagnosticRegistry::test_sandbox_startup_error_diagnostic_view_is_consistent -q -> 14 passed
• uv run --extra dev ruff check src/benchflow/sandbox/daytona_dind.py tests/test_daytona_dind_compose_up.py -> passed
• uv run --extra dev ruff format --check src/benchflow/sandbox/daytona_dind.py tests/test_daytona_dind_compose_up.py -> passed
• uv run --extra dev ty check src/benchflow/sandbox/daytona_dind.py -> passed

Keeping status:blocked / review:changes-requested until refreshed GitHub CI is green.

[bingran-you]

Users Simulation automation follow-up (2026-07-10): ready by simulation on current head b572f77187a28724ae778647ba324fc494219e90.

The structured-diagnostic blocker from #895 (comment) is addressed by the latest head: invalid Daytona DinD dind_image / dind_snapshot config now raises SandboxStartupError with sandbox_state="config_error" instead of a bare TypeError, so the failure remains in the structured sandbox-startup diagnostic path.

Validation now available:

• GitHub CI is green on b572f7718: test, pip-audit, manifest-parity, detect-scope, and integration-light / rollout-smoke.
• The latest integration-light-jobs artifact validates healthy with benchflow-experiment-review: ACP trajectory, LLM trajectory, results.jsonl, reward present, 292502 tokens, 16 tool calls, and training_ready=1.

Moving the PR back to status:ready; keeping review:changes-requested because human review is still required before merge.

[bingran-you]

Users Simulation automation follow-up (2026-07-10): ready by simulation on current head b572f77187a28724ae778647ba324fc494219e90.

The earlier blocker on fe6f8879... is resolved: invalid dind_image / dind_snapshot now raise SandboxStartupError through _startup_config_error(...), preserving structured sandbox_startup_info instead of escaping as a bare TypeError.

Current-head checks are green:

• test
• pip-audit
• parity
• rollout-smoke
• detect-scope jobs

Restoring labels to status:ready / review:pending for human review.

[bingran-you]

Daily scan refresh (2026-07-11): merged current origin/main into #895 and resolved the only content conflict in src/benchflow/sandbox/daytona_dind.py.

Conflict resolution kept current main's stop-file Daytona DinD heartbeat loop, which avoids orphaned sleep without the older trap-only branch, and preserved this PR's structured _startup_config_error(...) path for invalid dind_image / dind_snapshot values.

Validation before push from isolated worktree /tmp/benchflow-daily-pr895-refresh:

• uv run --extra dev python -m pytest tests/test_daytona_dind_compose_up.py tests/test_acp.py::TestDiagnosticRegistry -q -> 19 passed
• uv run --extra dev ruff check src/benchflow/sandbox/daytona_dind.py src/benchflow/diagnostics.py tests/test_acp.py tests/test_daytona_dind_compose_up.py -> passed
• uv run --extra dev ruff format --check src/benchflow/sandbox/daytona_dind.py src/benchflow/diagnostics.py tests/test_acp.py tests/test_daytona_dind_compose_up.py -> passed
• uv run --extra dev ty check src/benchflow/sandbox/daytona_dind.py src/benchflow/diagnostics.py -> passed
• git diff --check -> clean

Pushed head fbbf8e1f4. Moving to status:in-progress while refreshed GitHub CI runs; keep review:pending for the non-author human review gate before merge.

[bingran-you]

Daily scan CI follow-up (2026-07-11): refreshed GitHub CI is green on head fbbf8e1f4 after the origin/main merge.

Current checks:

• test passed
• pip-audit passed
• manifest-parity / parity passed
• integration-light / rollout-smoke passed
• detect-scope jobs passed; integration-scope matrix/review-pack skipped as expected for this light-scope diff

Moving back to status:ready; keeping review:pending for non-author human review before merge.

[bingran-you]

Users Simulation automation review (2026-07-11): ready by simulation on head fbbf8e1f4dae92a5f4ccbb831563ba2d57662e43.

Scope checked: diagnostic None serialization UX, Daytona DinD diagnostic surfacing, and CLI/sandbox regression risk.

Evidence:

• Local focused gate passed: uv sync --extra dev --locked; pytest tests/test_acp.py tests/test_daytona_dind_compose_up.py -q -> 94 passed; focused ruff; ty check src; bench --help; SDK Scene.single(agent="oracle").
• Subagent also ran uv sync --extra dev --extra sandbox-daytona --locked, targeted diagnostic cases, broader ruff check ., and CLI/SDK smokes for task check on docker/daytona, sandbox/eval help, Scene.single, and RolloutConfig.
• Fake Daytona DinD start() boundary smoke confirms invalid dind_image=None raises SandboxStartupError with sandbox_state="config_error" and compact diagnostic serialization.
• GitHub current-head checks are green: test, pip-audit, manifest-parity / parity, and integration-light / rollout-smoke passed.

Artifact health:

• Current-head integration-light run 29146314187 validated healthy with benchflow-experiment-review: 1/1 healthy, ACP + LLM trajectories present, training-ready results.jsonl, reward present, 344,641 tokens, 18 tool calls, 279.6s timing.
• Caveat: this GLM user-endpoint artifact still has null agent_result.cost_usd / price_source; token, timing, and trajectory metadata are complete.

Thermo review: no blocker. diagnostics.py shrinks, daytona_dind.py remains below 1k lines, _startup_config_error is an earned helper, and no wrong-layer or spaghetti branch growth was found. Caveat: test-file-only ty still reports the existing SimpleNamespace fake-env pattern, but production ty check src passes.

[bingran-you]

Daily scan hygiene (2026-07-12): added the missing Closes #531 reference to the PR body.

Reason: #895 is already the active fix for the narrowed remaining diagnostic serialization scope, but without the close reference GitHub may not close #531 after merge. No source changes were needed.