Repository navigation
fix(runstore): rounds/status survive NaN rewards and torn JSONL (#271) - #272
Conversation
append_round encodes with allow_nan=False and nulls non-finite floats; rounds() skips malformed lines; on_round tracks only finite rewards so best_reward is order-independent; the loopback panel dumps the same way.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Birfy
left a comment
Birfy
left a comment
There was a problem hiding this comment.
LGTM (COMMENT; self-approve blocked)
Closes #271. Verified locally:
- Diff vs main:
append_roundencode-first +allow_nan=False+ non-finite →null;rounds()skips torn/corrupt lines;on_round/ final status track only finite rewards; panel_jsonsanitizes the same way. - Regressions:
test_append_round_writes_null_not_nan…andtest_rounds_skips_a_torn_line…fail on main / pass on branch; helpers + status/panel strict-JSON tests pass; fulltests/test_runstore_cli.py26 passed. - Distinct from #268 (
_write_jsonorphan*.tmp).
Nits (non-blocking): _write_json still allows NaN if some other caller passes a non-finite float into update_status directly; this PR closes the on_round / append_round / panel paths. A follow-up could put allow_nan=False + sanitize inside _write_json once #268 lands.
CI pending on this push.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce5bb63879
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| break the status panel's ``JSON.parse``. Encode before opening the | ||
| file so a serializer refusal cannot leave a torn partial line. | ||
| """ | ||
| payload = _sanitize_jsonable(asdict(info)) |
There was a problem hiding this comment.
When a round has a non-finite reward, this conversion stores held_out_reward as null, but both cmd_status and cmd_watch still format that field with :.3f (agentdescent/cli.py lines 535 and 572). Consequently, the ordinary non-JSON status <run_id> and watch <run_id> commands raise TypeError for exactly the NaN/Infinity runs this change is intended to make observable; those consumers need a null-safe display path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in f06cdaa. cmd_status (text status <run_id>) and cmd_watch now format held_out_reward through _fmt_reward, which prints a null reward (stored for a non-finite one) as n/a instead of hitting TypeError on :.3f. Regression: test_text_status_and_watch_print_null_rewards drives both commands in-process over a NaN round followed by a finite round. It fails on the previous head with cli.py:535: TypeError and passes now.
| line = json.dumps(payload, default=str, allow_nan=False) + "\n" | ||
| with open(self.rounds_path, "a", encoding="utf-8") as fh: | ||
| fh.write(json.dumps(asdict(info), default=str) + "\n") | ||
| fh.write(line) |
There was a problem hiding this comment.
If the process is killed midway through its final write, the torn JSONL fragment normally has no trailing newline. On resume, append mode writes the next valid object directly after that fragment, producing one concatenated invalid line; rounds() then skips both the torn record and the first valid resumed round. Before appending, ensure an existing unterminated tail is separated or truncated so the newly encoded round remains independently readable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in f06cdaa. append_round now opens rounds.jsonl in ab+, and when the last byte is not \n it writes a newline before the encoded round, so a torn fragment left by a kill stays its own skipped line instead of swallowing the next round on resume. This is the same repair AuditStore._write_line uses in #277. rounds() also reads bytes and decodes each line, so a tear inside a multi-byte character costs that one line instead of raising UnicodeDecodeError for the whole file. Regressions: test_append_after_a_torn_tail_without_newline_keeps_the_next_round and test_rounds_survives_a_tear_inside_a_multibyte_character. Both fail on the previous head and pass now; tests/test_runstore_cli.py 32 passed.
Birfy
left a comment
Birfy
left a comment
There was a problem hiding this comment.
LGTM (COMMENT; self-approve blocked)
Closes #271. Independently verified on ce5bb63:
Correctness
append_round: sanitize →json.dumps(..., allow_nan=False)before opening the file — non-finite floats becomenull, serializer refusal cannot leave a torn partial line.rounds(): skips blank /ValueError(coversJSONDecodeError) / non-dict rows; one bad line no longer hides the rest from status / MCP / panel.on_round+ final status: only finite rewards enterbest;last_reward/best_rewarduse_finite_or_none; emptybest→None— kills themax([1.0, nan])vsmax([nan, 1.0])order dependence.- Loopback
_json: same sanitize +allow_nan=Falseso browserJSON.parsealways gets strict JSON.
Design fit
Matches the package pattern from #264/#268/#270 (encode-first, fail-closed for non-portable tokens). Distinct from #268 (_write_json orphan *.tmp).
Tests
- 4 regressions fail on main / pass on branch:
test_append_round_writes_null_not_nan_for_nonfinite_rewardtest_rounds_skips_a_torn_line_and_keeps_the_resttest_finite_or_none_and_sanitize_strip_nonfinite_floatstest_status_and_panel_payload_stay_strict_json_after_nan_rounds
- Full
tests/test_runstore_cli.py: 26 passed on branch. - Smoke:
test_runstore_cli+test_evalcachegreen.
Docs
CHANGELOG Fixed entry is accurate and scoped to #271.
Nits (non-blocking)
_write_jsonstill defaults toallow_nan=True. Callers that pass a non-finite float intoupdate_statusdirectly (bypassingon_round/append_round) can still writeNaNintostatus.json. This PR correctly closes theon_round/append_round/ panel paths; a follow-up could put sanitize +allow_nan=Falseinside_write_jsononce #268 lands.- Extra blank line before
_finite_or_none(cosmetic). rounds()silently drops non-dict JSON values (arrays/scalars) — intentional and fine; no log/count of skipped lines (acceptable for this scope).
CI matrix still pending on this push at review time.
…e-rounds-nan-corrupt # Conflicts: # CHANGELOG.md # tests/test_runstore_cli.py
…e-rounds-nan-corrupt # Conflicts: # CHANGELOG.md
Birfy
left a comment
Birfy
left a comment
There was a problem hiding this comment.
Re-review at a9e23e7: still LGTM. This push only merges current main (1049ab5, after #250/#260/#279 landed); the sole conflict was CHANGELOG.md, resolved by keeping both entries, and the branch diff against main is unchanged apart from that. Full local suite passed and CI is green. Mergeable, no open blocking feedback.
… status/watch print null rewards (#271)
…e-rounds-nan-corrupt
Birfy
left a comment
Birfy
left a comment
There was a problem hiding this comment.
Re-review at 8a5ddd6: LGTM (COMMENT, since self-approve is blocked)
f06cdaa addresses the two Codex P2s that were still open on ce5bb63. Each has a thread reply. 8a5ddd6 then merges current main (3947aee); the only conflict was CHANGELOG.md, resolved by keeping both entries.
Text status / watch with null rewards (cli.py)
cmd_status(per-run text view) andcmd_watchnow formatheld_out_rewardthrough_fmt_reward:None→n/a, numbers →:.3f, anything else →str. Before this, a NaN round (stored asnullby this PR) crashed both commands withTypeErroratcli.py:535/:572._status_linealready handled aNonebest_reward.
Torn tail without a newline (runstore.py)
append_roundopens inab+. If the last byte is not\n, it writes a newline first, so a fragment left by a kill stays its own skipped line instead of swallowing the next round on resume. This mirrorsAuditStore._write_linein #277, and encoding still happens before the file is touched.rounds()reads bytes and decodes per line, so a tear inside a multi-byte character costs one line instead of aUnicodeDecodeErrorfor the whole file. This is the same pattern as #274 and #255.
Tests
- New:
test_append_after_a_torn_tail_without_newline_keeps_the_next_round,test_rounds_survives_a_tear_inside_a_multibyte_character,test_text_status_and_watch_print_null_rewards. All 3 fail on the previous head and pass now. tests/test_runstore_cli.py: 32 passed. Every test file touching runstore/cli/rounds()passes, andtest_runstore_cli+test_evalcacheare green after the merge.
Nit (non-blocking, unchanged from before)
_write_jsoncallers other thanon_roundcan still pass a non-finite float intoupdate_status. A follow-up could sanitize there.
Exact-head CI (build + 3.9–3.13) is green and the PR is MERGEABLE. No open blocking feedback.
Summary
Closes #271.
append_roundencodes first withallow_nan=Falseand writes non-finite floats asnull(no portable-breakingNaN/Infinitytokens).rounds()skips torn/corrupt JSONL lines instead of raising, so one bad line no longer hides the rest from status / MCP / the panel.on_round(and the final status write) track only finite rewards forbest_reward/last_reward, somax(best)cannot become order-dependent onNaN._jsonsanitizes +allow_nan=Falseso the browser'sJSON.parsealways gets strict JSON.Test plan
test_append_round_writes_null_not_nan_for_nonfinite_rewardfails on main / passes on branchtest_rounds_skips_a_torn_line_and_keeps_the_restfails on main / passes on branchtests/test_runstore_cli.py(26) green locally