[Birfy]

Closes #259

What

TrajectoryStore.record() only caught OSError, but serialised the payload with json.dump(..., default=str, allow_nan=False) inside the write. A record that cannot be encoded (NaN/inf inside a structured output, a dict output with non-string keys, a circular value, an object whose __str__ raises, or a non-string TrajectoryEvent.detail under record_text=True) therefore escaped record(), breaking the store's own "a failed write must never fail a rollout" guarantee.

The worst consequence was in _Runtime.eval_one: _record_eval runs inside its retry loop, so the escaped error was read as a backend transient and the already-paid rollout was re-run ATTEMPTS times before the gate failed.

Change

• trajectory_key, _prepare and the JSON encoding now run before the disk is touched. Any failure there is counted in trajectory_write_failures and record() returns None, exactly like a full disk. Nothing is written and no .tmp is left behind.
• The encoded text is then written with the same tmp-then-os.replace path. json.dumps produces the same bytes json.dump did, so records that encoded before are byte-identical (the cross-process dedupe property is unchanged).
• Docstring and CHANGELOG updated.

Tests

New regressions in tests/test_trajectory.py (all 6 fail on main, pass here):

• test_unencodable_record_is_counted_not_raised[nan-inside-output|tuple-key|str-raises]: returns None, counts one failure, leaves the directory empty, and a later encodable record of the same identity still lands.
• test_circular_output_is_counted_not_raised
• test_non_string_event_detail_is_counted_not_raised
• test_eval_one_does_not_rerun_a_rollout_whose_record_cannot_be_encoded: the gate returns the reward after one run call, not ATTEMPTS.

Full suite: pytest -q passes locally (Python 3.12).

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-14T12:21:38.116478Z	ae81893	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[Birfy]

Review

Verdict: LGTM (Closes #259)

Faithful fix for the contract gap: record() promised “a failed write must never fail a rollout,” but only OSError was caught while json.dump(..., allow_nan=False, default=str) and _prepare ran inside the write. Unencodable payloads (NaN/inf, non-string dict keys, circular refs, raising __str__, non-string event detail under record_text=True) escaped and were treated as backend transients inside eval_one’s retry loop — re-running an already-paid rollout ATTEMPTS times.

Correctness

• Encoding (trajectory_key / _prepare / json.dumps) now happens before any disk touch; failure → trajectory_write_failures++, return None, no file / no orphan .tmp. Matches the full-disk degradation path.
• Successful path uses json.dumps then fh.write(text) with the same kwargs as before (sort_keys=True, default=str, allow_nan=False), so encodable records stay byte-identical for cross-process dedupe.
• KeyboardInterrupt / SystemExit still propagate (except Exception, not BaseException). Good.
• Confirmed on main: record(... output={"logprob": nan}) raises ValueError; all 6 new regressions fail on main and pass on this branch.

Tests

• Parametrized unencodable cases + circular + non-string detail + the critical eval_one “paid once, not ATTEMPTS” gate.
• Full suite: pytest -q exit 0 locally (Python 3.13 / venv).

Docs

• Docstring + CHANGELOG entry are clear and match the issue acceptance criteria.

Nits (non-blocking)

• Wrapping trajectory_key in the same broad except Exception is slightly wider than the issue’s encoding story (part.encode only fails if a field isn’t a str), but it’s consistent with “recording must never fail a rollout” and harmless.
• CI matrix still pending at review time; local green is enough to ship once CI catches up.

Approve when CI is green / with admin bypass as usual. Not merging from this review.

[Birfy]

Independently verified ae81893cd344de800062df831c3c52a586be9c76 against old base a9e8f087807cd2f3e45e2aca7a3a8378b7c4ad42: 45 focused trajectory/cache tests and 37 additional probes pass on Python 3.12.14. All six added regressions and all 12 selected independent runtime/worker controls fail against the old code for the intended recording failures.

Direct runtime controls across six unencodable output shapes show old code calling the synthetic provider three times then raising; this head returns the successful reward after one call, counts one recording failure, and leaves the recording root absent. Real sync/async worker drivers also finish successfully for NaN, tuple-key and raising-string outputs: each fixture makes 14 synthetic calls with 14 counted recording failures and no duplicate (rendered, task) invocation or worker retirement.

Additional controls verify no disk/temp mutation on encoding failure, later healthy writes, ten byte-identical old/new serialization comparisons, existing-record deduplication, and unchanged KeyboardInterrupt / SystemExit propagation with lock release.

Exact-head tests and docs are terminal green, including Python 3.9–3.13 wheel/install smoke. Local checks used in-process synthetic callables, not paid models, remote workers or live datasets; the local run was focused, not the full suite. No new concrete defect found in this scoped verification; no formal approval is submitted.

[Birfy]

Re-check at latest head

Verdict: LGTM (still; Closes #259)

Re-verified after the merge-main commit on head 0f204730:

• Focused regressions (unencodable / circular / non_string_event_detail / eval_one_does_not_rerun…) → 6 passed
• pytest tests/test_trajectory.py → green
• CI on this head: tests 3.9–3.13 + docs build SUCCESS (deploy skipped)

Prior LGTM at ae81893 still holds; no new review feedback to address. Ready to merge.

(Posted as COMMENT — author account cannot APPROVE.)

[Birfy]

Re-check after merging main (post-#264/#253)

CHANGELOG conflict resolved by keeping this PR's Fixed entry above the newly merged #263 FileCache note. Exact-head CI (build + 3.9–3.13) is green again; still MERGEABLE. Prior LGTM stands (self-approve blocked → COMMENT).