fix(metrics): guard reward reads against a non-dict rewards field

[Galius5136]

#1054 fixed _classify_completed_outcomes, so bench eval run now survives a
result.json whose rewards is not a mapping. The same payload still kills
bench eval metrics, and it aborts bench skills eval too. This closes both.

Reproduced on 3b9dd067 (main with #1054 merged), with #1054's own repro file:

$ bench eval metrics <jobs-dir>          # result.json has "rewards": 1.0
AttributeError: 'float' object has no attribute 'get'
  File "src/benchflow/metrics.py", line 364, in collect_metrics
    reward = r.get("rewards", {}).get("reward") if r.get("rewards") else None
exit 1

Root cause

result.json is read back as a raw json.loads payload with no shape
validation, so rewards can be any JSON value. r.get("rewards", {}) supplies
the {} default only when the key is absent; when the key is present with a
non-mapping value, the .get chain is applied to that value. The trailing
if r.get("rewards") guards falsiness, not shape.

The repo already has the total accessor for this -- _utils/scoring.py:111
extract_reward, which returns None for any non-mapping rewards. Every
reader of a persisted rewards in src/ is isinstance-guarded already
(review/runner.py:169, eval_lift.py:303, export_prime_sft.py:212,
viewer/payload.py:424, eval_artifacts.py:170, traj_capture.py:789,
loop_strategies.py:251, and since #1054 evaluation.py:435). These two were
the last that were not.

What changes

metrics.py:364 -- the crash. reward = extract_reward(r). A non-mapping
rewards now reads as no reward, so TaskMetrics.score_outcome routes the task
to errored through classify_score_outcome -- the same classifier, and the
same bucket, that #1054 gave the identical payload in bench eval run. The
failing line sits outside the enclosing try, which is why one malformed
artifact took down the aggregation for the whole results directory rather than
just that file.

metrics.py _safe_reward -- the same unguarded access, one level up in the
best-result selection, where it is swallowed rather than raised: it throws
inside the enclosing except Exception, which then logs "Skipping corrupt
result file" for the file being compared -- i.e. drops the well-formed retry
and lets the malformed artifact win the pick. Guarding it restores
tests/test_scoring.py::test_malformed_rewards_shape_tolerated's invariant
("a malformed entry must not shadow a well-formed one") for collect_metrics.

skill_eval/_core.py:741 -- if rewards: -> if isinstance(rewards, dict):.
Same failure mode, reached from bench skills eval; the enclosing handler
catches only (json.JSONDecodeError, KeyError), so the AttributeError
propagates out of _run_job. extract_reward is deliberately not used here
because it would drop the existing next(iter(rewards.values()), None) fallback
and change {"score": 0.7} from 0.7 to None. The isinstance guard keeps
that behaviour exactly.

What does not change

Measured A/B over every well-formed shape, both modules:

rewards	collect_metrics	_run_job
{"reward": 1.0}	1.0, passed	1.0
{"reward": 0.0}	0.0, failed	0.0
{"score": 0.7}	--	0.7 (fallback preserved)
null	None, errored	None
{}	None, errored	None

Identical before and after. The only inputs whose behaviour changes are the ones
that used to raise.

Tests

11 new cases, all red on 3b9dd067 with the source change reverted and green
with it:

• test_metrics.py::test_collect_metrics_non_dict_rewards_counts_as_errored
  -- 5 payloads (1.0, 1, true, [1.0], "1.0")
• test_metrics.py::test_collect_metrics_non_dict_rewards_does_not_shadow_a_scored_retry
  -- malformed + well-formed for one task; asserts the well-formed artifact is
  the one kept, not just its reward
• test_skill_eval.py::TestSkillEvaluatorResultCollection::test_non_dict_rewards_does_not_crash_collection
  -- same 5 payloads, using the class's existing
  monkeypatch.setattr("benchflow.evaluation.Evaluation.run", ...) idiom, so no
  docker and no provider key

Per-hunk mutation: each of the three source hunks reverted on its own is caught
by its own test, disjointly (3/3), so no hunk is unpinned and no test is
redundant.

Verification

• uv run ruff check src tests tools: All checks passed!
• uv run ruff format --check src tests tools: 617 files already formatted
• uv run ty check: All checks passed!
• full suite: 1 failed, 5975 passed with the change vs 1 failed, 5964 passed
  on the untouched baseline -- +11 is exactly the new cases, and there are no
  head-only failures. The single failure,
  test_integration_check_results.py::test_check_results_accepts_symlinked_current_repo_inferred_source,
  is pre-existing and reproduces identically on the untouched baseline tree.
• CLI A/B: bench eval metrics on a jobs dir whose result.json carries
  "rewards": 1.0 -- traceback and exit 1 before, normal table with Errored 1
  and exit 0 after.

Known follow-up, deliberately not in this PR

With the guard in place a malformed artifact scores 0.0 in the selection, so
it can still tie out a well-formed artifact whose reward is 0.0 -- and
collect_metrics keeps the first-seen artifact on a tie, which is the module's
own determinism rule (test_collect_metrics_best_result_picking: "Both errored
(no rewards): first seen is kept"). Fixing that means making clause 2 of the
selection predicate (r.get("rewards") is not None) shape-aware, which is a
semantic change rather than a guard: is not None and isinstance(..., dict)
differ on rewards: {}, and truthiness differs again in clause 3, so a naive
swap would change which artifact wins for well-formed inputs. Happy to send it
separately if you want it.

This finishes the read path #1054 started: same payload, same classification,
the two sites it did not touch.

---

[devin-ai-integration]

Devin Review found 1 potential issue.

1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)

[devin-ai-integration]

🟡 Zero-score retries remain hidden

When malformed data precedes a valid zero-score retry, _safe_reward ties them. Metrics retain the malformed attempt and report an error.

Prompt for agents

Update collect_metrics best-result selection in src/benchflow/metrics.py so malformed non-mapping rewards rank as unscored, while a valid numeric reward, including 0.0, replaces them. Preserve deterministic tie handling for equivalent valid results and decide explicitly how empty or nonnumeric reward mappings rank. Add regression coverage where the malformed artifact sorts first and the valid retry has reward 0.0.

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[Galius5136]

This is a known follow-up and is intentionally out of scope for this crash fix.

Handling the malformed-first / valid-0.0 retry case requires changing the best-result selection semantics, including an explicit policy for empty and non-numeric reward mappings. This PR deliberately preserves the existing deterministic tie behavior for well-formed inputs and only makes persisted non-mapping rewards safe to read.

I've documented this case in the Known follow-up section of the PR description; I'd prefer to address the ranking semantics separately rather than bundle that policy change into the shape-guard fix.

[Galius5136]

Human validation completed on the PR head (12c1d578).

Validated manually:

• H1 — baseline CLI repro: bench eval metrics crashes on rewards: 1.0 with AttributeError at metrics.py:364, exit 1.

• H2 — fixed CLI: same artifact is classified as Errored 1, exit 0.

• H3 — regression A/B: 11 new tests fail on the baseline source and all 58 targeted tests pass with the fix.

• H4 — mutation: all 3 source hunks are independently caught by their regression tests.

• H5 — ruff check, ruff format --check, and ty check all pass; full suite is 1 failed, 5975 passed, with the single failure reproducing identically on the untouched baseline.

• H6 — real BenchFlow artifact: ran an actual oracle + Docker rollout, verified the intact result.json, then changed only rewards from {"reward": 1.0} to 1.0. On that same artifact:

  • baseline source: AttributeError at metrics.py:364, exit 1;
  • PR source: normal metrics output with Errored 1, exit 0.

The artifact was restored afterward.

So the bench eval metrics failure is now reproduced and the fix manually validated both at the CLI level and against a real artifact produced by BenchFlow.

skill_eval remains covered at the unit / _run_job level only; I have not run the full bench skills eval command end to end because that path requires a provider-backed agent in this environment.