feat(results): preserve structured failure diagnostics

[yangziao56]

Summary

• preserve typed rollout failure categories and diagnostic events in the canonical trainer-facing results.jsonl
• keep healthy rows and callers without structured diagnostics byte-shape compatible by omitting the optional block
• expose only a strict per-diagnostic safe subset; omit raw messages, stderr, sandbox IDs, PIDs, tool-call IDs, probe output, tracebacks, task names, timestamps, and API fingerprints
• keep existing error, stop-condition, training-readiness, reward, and Prime-SFT semantics unchanged

Closes #1037.

Schema

Failed rows may now contain:

{
  "info": {
    "diagnostics": {
      "schema_version": 1,
      "error_category": "pipe_closed",
      "events": {
        "transport_error_info": {
          "channel": "error",
          "category": "pipe_closed",
          "details": {
            "process_exit_code": 255,
            "transport_diagnosis": "process_exited",
            "sandbox_reachable": false
          }
        }
      }
    }
  }
}

The outer category is retained even when no typed event exists. New diagnostic classes fail closed to event metadata without details until they opt safe numeric/boolean fields into the trainer export policy.

Validation

• uv run python -m pytest tests/test_train_mode_artifact_emission.py tests/test_acp.py -q
• uv run python -m pytest tests/ -q
• uv run ty check src/
• uv run ruff check .

The issue also asks whether this bounded infrastructure contribution is eligible for FrontierPhysics credit. This PR does not assume or claim any point value.

AI-assistance disclosure: Codex assisted with implementation, tests, QA, and PR drafting under Ziao Yang's direction. The submitted behavior and test results were verified in the repository environment.

[tulerfeng]

Thanks for putting this together — losing the structured failure detail on the way into results.jsonl has been a real gap. I ran an independent end-to-end validation of this PR using the Gemini 3.5 Flash API. The PR does what it claims — I recommend merging it.

End-to-end verification

Everything below comes from real bench eval runs — real Docker containers, a real Gemini provider, real failures and a real passing rollout — plus targeted adversarial and mutation probes against the redaction layer.

base 4fa4958 (previous version) vs head fd4c6b9 (PR version, 01fa9cd + fd4c6b9), running the same command against both trees:

bench eval run --trials 1 --tasks-dir tests/examples/hello-world-task \
  --agent gemini --model gemini-3.5-flash-lite --sandbox docker

what was verified	result
real healthy rollout, base vs head	structurally identical; no diagnostics block on either
real failed rollouts, 3 modes	acp_error, sandbox_setup, install_failure all land in error_category on head; all lost on base
legacy-caller byte shape	byte-for-byte identical: 1026 B healthy row, 893 B failed row
adversarial redaction	16 poisoned fields across all 7 registered diagnostics, zero leaked
mutation testing	5 injected defects, 4 caught; the 5th cannot produce a leak by construction
#1037 acceptance criteria	5/5 pass

Diagnostics appear on failures, and only on failures. Three matched base/head pairs, each a real rollout:

failure mode	base (previous version)	head (PR version)
ACP 400 from the provider	info.diagnostics absent	{"schema_version": 1, "error_category": "acp_error"}
sandbox startup failure	absent	{"schema_version": 1, "error_category": "sandbox_setup"}
agent install failure (rc=127)	absent	{"schema_version": 1, "error_category": "install_failure"}
healthy rollout, reward=1.0	absent	absent

The healthy pair is the one that carries the regression risk, so I ran it on both trees end to end: 1/1 passed, reward 1.00, 38.6k tokens, telemetry 100% on each. Comparing the two results.jsonl rows — top-level key order identical, info key order identical, zero keys added by head, and no diagnostics block on either side. The only deltas are model non-determinism: different assistant wording, one extra run_shell_command turn on base, and the token/timing numbers that follow from it (38,643 vs 38,647 total tokens). The success path is untouched.

Unchanged callers produce unchanged bytes. Calling build_rollout_results_record exactly the way every pre-#1038 caller calls it — passing none of the new diagnostic arguments — the output is byte-for-byte identical between base and head: 1026 bytes for a healthy row, 893 bytes for a failed row that carries no structured diagnostics. Nothing shifts for a caller that has not been updated.

Redaction holds under adversarial input. I instantiated all 7 registered diagnostics with poisoned values — API keys, absolute paths, container IDs, raw exception text — in every field the classes carry, then scanned the emitted block for surviving poison. 16 poisoned fields probed, zero leaked. Every value that does reach the block is a bounded scalar: an int, a float, a bool, or a string clamped to a closed enum; no free-form string is copied through. Non-finite floats do not survive either, so json.dumps(..., allow_nan=False) stays valid.

I also probed the one path the classifier does not control — a caller passing an arbitrary error_category string — to check the redaction layer is a real backstop rather than a well-behaved happy path:

error_category = "pipe_closed sk-ant-api03-DEADBEEF..."
->  {"schema_version": 1, "error_category": "pipe_closed ***REDACTED***"}
    secret present anywhere in the record: False

Job-level aggregation via write_job_results_jsonl preserves the block intact, verified both synthetically and on the real failed rollouts above.

The new tests genuinely constrain the policy. To check they do more than execute the code, I broke the redaction layer five ways and re-ran the suite (136 tests):

mutation	outcome
M1 drop field-kind validation, copy every allowlisted field raw	caught, 2 tests failed
M2 drop the exact-type guard, let subclasses through	caught
M3 stop clamping transport_diagnosis	caught — leaks without the guard
M4 stop clamping the provider subcategory	caught — leaks without the guard
M5 allowlist a raw string field under an integer kind	survived, but cannot leak

4 of 5 caught. M5 does not affect the correctness of this PR: it is a hypothetical future edit rather than anything the current code does, and even if someone made it, the field-kind validator rejects the mismatched value at emit time, so no data can escape. The mutation is inert, not uncovered. The two tests doing the real work are test_results_jsonl_export_policy_omits_untrusted_fields and test_results_jsonl_omits_untrusted_structured_diagnostic_values.

One behavioural note, not a defect: the registry lookup is an exact-type match, so a subclass of a registered diagnostic is dropped from the block entirely — not even its channel/category survives. M2 shows this is deliberate, since a subclass could carry fields the parent's allowlist never vetted, and loosening it is exactly what the suite catches. Fail-closed is the right call; it may just be worth a sentence in the docstring so a future author does not read the silence as a bug.

Potential conflict with PR #1025

Nothing to do with this PR's correctness, but it will bite whoever lands second — and it is the kind that is easy to miss, because git will not flag it.

The two PRs touch different parts of diagnostics.py: #1025 appends a new class and adds it to the registry tuple, this PR adds results_jsonl_fields declarations to the seven existing classes. Those merge cleanly with no textual conflict, and both branches are green on their own. The conflict is semantic and only shows up afterwards:

test_results_jsonl_export_policy_covers_the_registry asserts that every class in DIAGNOSTIC_REGISTRY declares results_jsonl_fields. NoToolCallCompletionDiagnostic from #1025 does not, so it inherits the base class default of None. I reproduced this by dropping that class into this branch — registry goes 7 → 8 and the test fails immediately:

assert diagnostic_cls.results_jsonl_fields is not None
E   AssertionError: assert None is not None
E    +  where None = <class 'benchflow.diagnostics.NoToolCallCompletionDiagnostic'>.results_jsonl_fields

That is the test doing exactly its job — it is the guard that stops a new diagnostic from silently bypassing the allowlist, which is the whole point of the mechanism. But it does mean whichever of these two PRs lands second needs a one-line results_jsonl_fields declaration, otherwise main goes red straight after a merge that looked clean. Cheapest fix is to add the declaration in the same commit.