Diagnostic dataclasses use bare-str fields where Literal/enum belongs; reason constant duplicated across modules

[bingran-you]

Summary

Multiple diagnostic and result fields are declared as bare str even though they only ever take a small closed set of values, and one of those constants is duplicated in two source files with no shared source of truth. Static type checkers, IDEs, and consumers cannot rely on the closed-set property today.

Concrete instances

A. Diagnostic.reason is bare str

src/benchflow/diagnostics.py:89, 115, 142 — IdleTimeoutDiagnostic.reason: str = "idle_timeout", SandboxStartupDiagnostic.reason: str = "sandbox_startup_failed", TransportClosedDiagnostic.reason: str = "transport_closed".

These are dataclass-class invariants — there is exactly one valid value per class. Should be Literal["idle_timeout"] etc., or a shared DiagnosticReason = Literal["idle_timeout", "sandbox_startup_failed", "transport_closed", …] re-exported from the diagnostic module.

Today, IdleTimeoutDiagnostic(reason="not_actually_a_timeout") type-checks. That's a regression vector.

B. IDLE_TIMEOUT constant duplicated

src/benchflow/_utils/scoring.py:10 declares IDLE_TIMEOUT = "idle_timeout". src/benchflow/diagnostics.py:89 hardcodes the same string as the diagnostic's default. Renaming one side does not error.

C. usage_source / price_source are bare str

src/benchflow/metrics.py:47, models.py:138, rollout.py:452: usage_source: str = "unavailable". The only two values ever assigned in v0.5 are "provider_response" and "unavailable". Compare to TrajectorySource = Literal["acp", ...] in models.py:15 — same project, different rigor.

price_source is null today but typed the same way; tighten when its enum is decided.

D. Inconsistent to_dict() Nones-policy

TransportClosedDiagnostic.to_dict() (diagnostics.py:170-194) drops keys with None values. IdleTimeoutDiagnostic.to_dict() does NOT drop Nones — uses bare asdict(). Same registry, different serialization shape. Hides "field is absent" vs "field is null" inconsistently.

Severity

P2.

No current incorrect-output bug — these are type-safety / API-stability hygiene issues. But:

• (A) and (B) together mean a one-line rename of "idle_timeout" in scoring.py will silently misclassify every future idle-timeout incident.
• (C) means consumers cannot exhaustively pattern-match on these fields.
• (D) means transport_error_info and idle_timeout_info look different to JSON-schema-driven consumers for no documented reason.

Proposed fix

1. Introduce DiagnosticReason (or per-class Literal) and replace the bare-str defaults. Move IDLE_TIMEOUT / TRANSPORT_CLOSED / SANDBOX_STARTUP_FAILED constants into diagnostics.py and re-export from _utils/scoring.py.

2. Typedef UsageSource = Literal["provider_response", "unavailable"] in metrics.py; apply to all three sites.

3. Decide the to_dict() Nones policy (drop-or-keep) once and apply uniformly. Lean toward "drop None" to match TransportClosedDiagnostic since it's already in production.

4. Add a unit test that asserts to_dict() outputs are JSON-schema-stable across all Diagnostic subclasses (no surprise key presence/absence).

Found via

v0.5 e2e validation §2, §3, §4+§5 at 5a7a326. Independently noted by three subagents inspecting result.json and diagnostics.py.

[bingran-you]

Automation fix update (2026-06-30): opened PR #858 with the narrow diagnostics/usage-source typing fix.\n\nWhat changed:\n- diagnostic reason strings now have shared constants + literal types in benchflow.diagnostics; _utils.scoring.IDLE_TIMEOUT reuses the diagnostic source of truth.\n- usage telemetry now has a closed UsageSource literal contract covering current provider_response, agent_native_acp, and unavailable values.\n- rollout result construction validates usage_source via normalize_usage_source() so a future unregistered source fails explicitly.\n- regression tests guard GitHub issue #531 for diagnostic reason typing and usage-source typing.\n\nValidation on PR #858:\n- uv run ruff format --check src/ tests/\n- uv run ruff check .\n- uv run ty check src/\n- uv run python -m pytest tests/test_acp.py tests/test_metrics.py -q (87 passed, 1 skipped)\n- uv run python -m pytest tests/ (4702 passed, 14 skipped, 7 deselected)\n\nI intentionally left the diagnostic to_dict() None-policy unchanged because changing result JSON shape should be a separate compatibility decision.