Skip to content

fix(state): avoid double-encoding reasoning columns on fork round-trip - #57536

Closed
nankingjing wants to merge 1 commit into
NousResearch:mainfrom
nankingjing:fix/57240-fork-reasoning-roundtrip
Closed

fix(state): avoid double-encoding reasoning columns on fork round-trip#57536
nankingjing wants to merge 1 commit into
NousResearch:mainfrom
nankingjing:fix/57240-fork-reasoning-roundtrip

Conversation

@nankingjing

Copy link
Copy Markdown
Contributor

Summary

Test plan

  • pytest tests/test_hermes_state.py::TestMessageStorage::test_get_messages_roundtrip_preserves_reasoning_columns
  • Existing test_reasoning_details_persisted_and_restored still passes

Fixes NousResearch#57240

get_messages() leaves reasoning_details/codex_* columns as stored TEXT.
replace_messages() must not json.dumps() those strings again or forked
sessions silently lose reasoning replay on resume.

Add _encode_json_column() and use it in append_message and
_insert_message_rows so pre-serialized TEXT passes through unchanged.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state duplicate This issue or pull request already exists labels Jul 3, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #57248 — same write-path-only mechanism for #57240: a helper that passes already-serialized TEXT through unchanged (else json.dumps), routed through append_message + _insert_message_rows at the same 6 dumps sites, read path untouched. #57248 (created earlier) is the canonical version; only the helper name differs. Related: #57240 (the issue) and #57454 (a sibling that additionally changes the read path — a genuine design divergence, not a dup).

@nankingjing

Copy link
Copy Markdown
Contributor Author

Closing in favor of the earlier canonical fix path that the maintainer review identified in the comment thread.

Per the triage comment on this PR:

Keeping the narrower cluster open does not help the maintainer pick a canonical fix; closing these reduces the supersede noise so the maintainer can land the right one. Thanks to the alt-glitch triage for the cross-references.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint duplicate This issue or pull request already exists P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

forking a session double-encodes the reasoning columns — forked sessions silently lose reasoning replay

2 participants