fix(collect): reject corrupt resume files (from #1154)

[cursor]

Summary

Same-repository replacement for #1154 so required CI can run without the fork Approve-and-run gate.

Effective scope is unchanged: reject corrupt resume JSONL records in collect, with the quadratic-safe trailing-record scan and focused regressions. burtenshaw already APPROVED the reviewed fork head abf4999c; this branch is that content merged onto current main a3ee7644.

Please close #1154 as superseded after this lands.

  

---

Note

Medium Risk
Changes rollout dataset persistence and resume semantics; corrupt files now error instead of partial resume, but behavior is localized to collect JSONL I/O with strong regression tests.

Overview
Hardens RolloutSerializer JSONL resume and append so corrupt results.jsonl files fail fast instead of being silently ignored.

collected_episode_ids() now validates every non-empty line (UTF-8, JSON object, string episode_id) and raises ValueError with file/line context; CollectRunner.run(resume=True) therefore aborts before opening sessions when the on-disk file is bad. Appends use binary a+b writes with _append_separator, which only scans the tail of the file (chunked, not full-file) to insert a newline when the last complete record lacks one, and refuses to append if the trailing bytes are a partial JSON record—leaving the file unchanged.

Tests cover newline repair, long final lines, malformed interior/trailing records, and resume failing without mutating the file or creating sessions.

^{Reviewed by Cursor Bugbot for commit 9a1c1de. Bugbot is set up for automated code reviews on this repo. Configure here.}

[cursor]

Release-manager note

On-repo replacement for #1154 so required workflows can run without the fork Approve-and-run gate.

Effective diff is the same two-file collect-resume integrity fix. burtenshaw APPROVED fork head abf4999c; this head is that content on current main a3ee7644.

Merge gates: terminal exact-head CI + explicit acceptance of this replacement head (prior fork approval does not auto-transfer). Outside the Thursday release decision unless Ben pulls it in.

View PR

  

_{Sent by Cursor Automation: Release}

[cursor]

Alignment Review Report

Reviewed the diff a3ee764...9a1c1de (2 files: src/openenv/core/harness/collect.py, tests/core/test_harness_collect.py). This is a targeted bug fix for corrupt/partial resume files in the collect harness.

Automated Checks

• Lint: PASS — usort check, ruff format --check, and ruff check are all clean on both changed files.
• Debug code: CLEAN — no print/breakpoint/pdb/TODO introduced. (The lone print( in the file is the pre-existing progress.console.print(...) Rich call, outside this diff.)
• Tests: 50 passed — ran the PR's tests/core/test_harness_collect.py against the PR's collect.py.

Open RFCs Context

All RFCs are In Review (000–008) or Draft (010, 011). The only one covering core/harness/ is RFC 005 – Agentic Harness Integration (In Review). Per rfcs/README.md, bug fixes don't require an RFC, and this change introduces no new public API/abstraction, so no RFC action is needed. (RFC 002's "resume" refers to environment lifecycle suspend/resume, not disk-based collect resume — unrelated.)

Tier 1: Fixes Required

None. Types are correct (BinaryIO, int | str), the added BinaryIO import is used, no syntax/import errors, and no credential exposure (error messages carry only file path + line + JSONDecodeError.msg, never record contents).

Tier 2: Alignment Discussion

Principle Conflicts: None identified. The change strengthens data integrity / production-readiness (a stated optimization in PRINCIPLES.md). No client↔server import-boundary crossing (this is core-library code), no change to reward computation, and no change to the Gymnasium reset/step/state signatures.

RFC Conflicts: None identified.

Notes (non-blocking)

1. Deliberate behavior change — worth a changelog line. collected_episode_ids() now raises ValueError on malformed / partial / non-object / missing-episode_id lines instead of silently continue-ing. This is the correct fix: silent-skip previously caused already-collected episodes to be re-run, and combined with a missing record separator could concatenate two records into one corrupt line. Two heads-ups:

   • The CLI openenv collect (src/openenv/cli/commands/collect.py) calls runner.run(resume=...) → collected_episode_ids() with no try/except, so a corrupt resume file now surfaces as an uncaught ValueError traceback rather than a clean typer error. The message itself is informative (.../results.jsonl:<n>: invalid JSON: ...); wrapping it in a typer.BadParameter would be a nice optional follow-up.
   • Pre-1.0 breaking-behavior changes are permitted per INVARIANTS.md "Breaking Change Policy" — flagging only for release-notes awareness.
   • Suggested reviewers: @burtenshaw (harness code owner), @Darktex (author of the Breaking Change Policy / RFC 005).

2. Minor test nit (see inline). test_append_does_not_read_the_entire_results_file doesn't quite verify its namesake — details inline.

Summary

• 0 mechanical issues to fix
• 2 low-severity, non-blocking notes for human awareness (intentional behavior change + a weak test)
• 0 RFC conflicts

The fix is correct and well-scoped: the write path (_append_separator) guarantees a newline separator so a prior record lacking a trailing newline isn't merged with the next one, and the read path fails loudly on corruption instead of silently dropping data. Test coverage is solid (chunk-boundary scan, partial-record rejection without mutating the file, interior/non-object/missing-id rejection, and a runner-level guard that no sessions start on a corrupt file).

  

_{Sent by Cursor Automation: Pre-review}

[cursor]

Minor / non-blocking: this test doesn't quite verify its namesake. The fixture is written with a trailing newline ('...ep1"} '), so _append_separator takes the 1-byte fast path (handle.read(1) == b" " → early return) and never enters the chunked backward scan. Separately, the implementation reads through the open file handle (handle.read(...)), not Path.read_bytes, so patching read_bytes doesn't actually guard against a full read either. test_append_scans_a_long_final_record_across_chunks (no trailing newline) is the test that truly exercises the scan. If you want to lock in the "bounded read" guarantee here, drop the trailing newline and assert on bytes read through the handle.