fix(tasks): stop indexing a rollout key that is absent on disk

[ethan-scitix]

Type

• fix — bug fix or alignment correction

Summary

• rollout["prediction"] raises KeyError on resume. build_prediction_record spells "could not extract" as prediction=None; obj_to_dict drops None-valued keys; so on disk the key is absent, not null. The loader hydrates postprocess_result from disk and hands it to feedback, which then KeyErrors for exactly the samples whose extraction failed. The identical line is fine on a fresh run — which is why every in-memory test passes.
• 39 sites across 39 task modules were indexing it. All become .get("prediction"): exactly equivalent when the key is present, and otherwise yielding the same None the fresh path produced. No behaviour change on a fresh run — the resumed path simply stops diverging from it.
• The real fix is the enforcer. The contract was already correct in three places and still nothing stopped the drift (below), so this adds check_preflight.py --check check_record_key_access.
• Scope is deliberately prediction only; _UNGATED_ROLLOUT_KEYS records why extra / score / metrics are different in kind.

Related Issues

Surfaced while reviewing #65 (PlatinumBench), whose task uses .get("prediction") with a three-line comment explaining why. It is not the first: sieval/tasks/ruler_0shot_gen.py:261-264 is already on main with the same .get() and the same three-line explanation, on its resume-report path. That sharpens the argument rather than softening it — the hazard was documented in prose inside a merged module, and 39 other modules drifted off it anyway. Documentation of the mechanism is not enforcement of it. Refs #65.

Suggested merge order: this PR before #65. #65 is unaffected either way (it is already compliant), but landing the guard first means it merges into a tree where the rule is enforced rather than one where it is a comment.

Why nothing caught this

This is the part worth reviewing, more than the 39 one-line edits.

The contract was already stated three times over:

1. RolloutPrediction.prediction is declared NotRequired[JSONValue | None] — the type does not lie.
2. Its docstring: "absent on disk in that case, so read extracted instead", and extracted is labelled "The durable signal."
3. test_records.py::TestSerializationRoundTrip::test_none_prediction_is_absent_but_extracted_survives already pins the exact behaviour, and passes.

records.py is also defensive about the same hazard elsewhere: _checked_metrics hard-rejects a None metric because "a None metric would be absent on disk, turning 'not measured' into 'never existed'".

And yet 38 modules drifted off it with CI green the whole time, because neither type checker reports a [] subscript of a NotRequired key:

class Rollout(TypedDict):
    index: int
    prediction: NotRequired[str | None]

def unsafe(r: Rollout) -> object:
    return r["prediction"]      # may KeyError at runtime

• ty → All checks passed!
• mypy --strict → Success: no issues found in 1 source file

Both are behaving per spec — the typing spec leaves that diagnostic optional (pyright has reportTypedDictNotRequiredAccess, not used here). So the root cause is not the serializer dropping None, which is a deliberate, documented, tested design. It is that a correctly declared, correctly documented, unit-tested contract had no enforcement at the call site. Documentation and a test of the mechanism did not stop 38 violations of it.

The check

check_record_key_access is structural, not name-based: a key is flagged only when its base provably comes from a record's rollouts — either indexed (post["rollouts"][0][...]) or iterated (for r in post["rollouts"], comprehensions, and the .get("rollouts", []) report-stage idiom). That precision matters: t_eval_before_calling_0shot_gen.py has a task-local datum["prediction"] whose sibling key is ground_truth. It is not a record, absence there is a genuine bug, and a name-based rule would have forced a .get() that silently swallows it. It is correctly left untouched.

Gated set is prediction alone. _UNGATED_ROLLOUT_KEYS names the rest with reasons rather than ignoring them:

• extra — absence is an authoring choice, not a runtime outcome: the task reading r["extra"]["grade"] is the same task that wrote that extra, unconditionally. And all 11 reads are nested, so a mechanical .get() would turn KeyError into NoneType is not subscriptable — strictly worse. Needs a per-site pass, not a sweep.
• score / metrics — same authoring argument, plus RolloutJudgement states "absent is not zero", so a .get() returning None becomes a wrong metric instead of a loud failure.

A NotRequired rollout key in neither set makes the check FAIL rather than be silently skipped, so adding one to records.py forces the classification. That is what keeps the gate from quietly narrowing over time.

Test Plan

Automated

• Lint/format clean (ruff check && ruff format --check, 370 files)
• Type check clean (ty check)
• Unit + integration + acceptance: 2999 passed (13 new)
• Full preflight clean (scripts/check_preflight.py, exit 0), including the new check_record_key_access

New tests cover both rollout shapes (indexed / iterated / comprehension / .get("rollouts")), the .get() form passing, Required keys not being flagged, the ungated keys not being flagged, the non-record datum false-positive case, an unparsable module, a missing records.py, and the unclassified-key failure.

Manual

• Reproduced the bug against production code, not a mock: feeding GSM8KZeroShotGenTask.feedback a prediction record that has been through the real serializer raises KeyError('prediction'), while the in-memory record scores normally.

  fresh (in-memory) record : {'rollouts': [{'index': 0, 'prediction': None, 'extracted': False}]}
  resumed (from disk)      : {'rollouts': [{'index': 0, 'extracted': False}]}
  fresh run   -> feedback ok, finalize=True
  resumed run -> KeyError('prediction') raised out of feedback
  

• The guard discriminates: it reports exactly 39 violations on the parent commit and PASS after the edits. Not a check that trivially passes.

• Confirmed on a real run's shard data that the key is genuinely absent on disk — a stored postprocessed record for a truncated sample reads {"index": 0, "extracted": false}.

• Trigger window characterised (narrower than "any resume"): needs a sample that both failed extraction and was interrupted between its postprocess and feedback records landing. Reachable in normal operation — at interruption, samples are spread across stages — and likeliest on tasks that truncate often, which are also the long-running ones most likely to be interrupted.

Checklist

Required (all PRs)

• PR title follows conventional format (type(scope): description)
• No internal paths, credentials, or personal info in committed files
• AI-generated code has AI-Generated Code - <model> (<provider>) in module docstring — touched files already carry theirs; no new modules added
• No new upper-layer dependencies added to core/ (core/ is untouched)
• Deleted code verified — nothing deleted

If: Breaking Change

Not a breaking change. .get() returns the same value whenever [] would have succeeded; it only replaces a crash with the value the fresh path already produced.

If: New Dependency

Not applicable — no new dependencies.

🤖 Generated with Claude Code

---

Review follow-ups — 289756d3

Second commit, all on the guard itself; the 39 call-site fixes are unchanged.

• enumerate / zip were blind spots. _rollout_bound_names only bound a name when the for target was a bare ast.Name and the iterable was directly a rollout container, so for i, r in enumerate(post["rollouts"]) — the natural way to write feedback for a task needing the rollout index — passed the check while reintroducing the identical KeyError. Tuple targets now bind every Name element, and _rollout_container unwraps the pass-through builtins (enumerate/zip/reversed/list/sorted) and a walrus. Probe of twelve shapes: 5 flagged before, 8 now. The remaining four all need the rollout list to pass through a name first (rs = post["rollouts"], a helper parameter) — dataflow, not a syntactic walk — and are documented as deliberate limits, with one pinned by a test.
• Classification widened to all five record TypedDicts. JudgementRecord.reference shares prediction's exact shape (build_judgement_record writes it unconditionally, so None is present in memory and dropped on disk) yet sat in neither set — nothing forced a decision on it, and nothing would have forced one for a key added to PromptRecord either. Now classified ungated, with the reason recorded: the single [] read (ruler's report) consumes it as an iterable, so a mechanical .get() would swap KeyError for a list(None) TypeError — the same trap as extra's nested reads. Gating a key whose reads are not rollout subscripts would be inert and would read as coverage the check does not have.
• An unreadable records.py now FAILs instead of SKIPping. It is the check's subject, not an optional input: renaming it would otherwise narrow the gate to nothing with preflight still green — the same silent narrowing the unclassified-key branch exists to prevent.
• Dropped a duplicate integration test whose body was identical to its sibling.

What reference actually lacks is a durable signal for which of its two absence causes applies (task truth is a procedure vs. this sample's gold is missing) — prediction has extracted plus detect_extraction_failure for exactly this, reference has nothing. Out of scope here; tracked in #71.

Verification

• Guard still reports exactly 39 violations on the parent commit, PASS here — the widening did not cost detection.
• New tests fail when the tuple-target handling is reverted (assert 'PASS' == 'FAIL' on both enumerate/zip cases).
• ruff check + ruff format clean; ty check clean.
• 3016 passed (19 new here) after rebasing onto ee30abe6 (feat(livecodebench): replace the whole-suite timeout with upstream's per-case rule #66), full scripts/check_preflight.py exit 0, tests/performance 66 passed.
• Rebased onto ee30abe6 (feat(livecodebench): replace the whole-suite timeout with upstream's per-case rule #66). feat(livecodebench): replace the whole-suite timeout with upstream's per-case rule #66 rewrote both livecodebench task modules; verified after the rebase that their .get("prediction") survived at the new line numbers, that feat(livecodebench): replace the whole-suite timeout with upstream's per-case rule #66's per-case timeout logic is intact, and that commit 1's stat is byte-identical to its pre-rebase form (43 files, 416/42). 39 task modules, 39 .get("prediction") sites, and the only remaining ["prediction"] in the tree is t_eval's intentional task-local datum.

[ethan-scitix]

Field evidence for this, in case it helps with priority: it fires in production, not
only in review.

Hit it today verifying #66 on a full 1,000-question run. Re-grading the LiveCodeBench lane
(sieval run --resume after clearing its terminal records — no model calls) failed on
4 of 90 rollouts with exception::KeyError 'prediction'. All four were truncations —
finish_reason: length, 0 chars of texts — so prediction was None, dropped by
obj_to_dict, absent on reload. The lane could not be re-graded at all until the line was
changed to .get.

Two details that support the framing in the description:

• Resume is not a rare path. It is exactly what a harness bump makes people take: feat(livecodebench): replace the whole-suite timeout with upstream's per-case rule #66
  changed how LiveCodeBench grades, so the natural way to see the effect is to re-grade
  existing generations rather than re-run inference. The first thing anyone does after a
  grader change lands on this bug.
• The blast radius is per-lane truncations. Any lane whose model sometimes runs out of
  tokens has some rollouts with prediction=None, so the 39 sites are not equally likely
  to fire but are all live. On this run the same 1,000 questions produced blank
  generations in mmlu_pro, aime, hmmt and livecodebench.

I had opened #72 for the single lane before finding this PR — closed as a duplicate. Its
regression test is redundant too: test_none_prediction_is_absent_but_extracted_survives
already pins the behaviour, which is your point exactly — the record-level test passes
while the consumers drift, so the preflight check is the part that actually holds.