Skip to content

fix(state,cli,tui-gateway): keep reasoning fields intact across forks and branches - #57248

Closed
cryptoyasenka wants to merge 1 commit into
NousResearch:mainfrom
cryptoyasenka:fix/fork-reasoning-double-encode
Closed

fix(state,cli,tui-gateway): keep reasoning fields intact across forks and branches#57248
cryptoyasenka wants to merge 1 commit into
NousResearch:mainfrom
cryptoyasenka:fix/fork-reasoning-double-encode

Conversation

@cryptoyasenka

@cryptoyasenka cryptoyasenka commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Keeps a session's reasoning fields intact when the session is copied. Both
copy paths lost them, two different ways.

Fork, double-encoding. get_messages() returns reasoning_details /
codex_reasoning_items / codex_message_items as the raw TEXT stored in
those columns (only content and tool_calls are hydrated), and both
writers re-ran an unguarded json.dumps() on whatever they received. The
fork endpoint pipes get_messages() straight into replace_messages()
(_handle_fork_session in gateway/platforms/api_server.py, lines
3498-3499), so every forked session stored its reasoning fields wrapped in
one extra JSON-string layer. On resume,
get_messages_as_conversation() decoded the column back to the inner
string, and every consumer's isinstance(..., list) gate silently dropped
it: preserved Anthropic thinking blocks, Codex encrypted-reasoning and
message-item replay, and OpenRouter multi-turn reasoning context were all
lost after a fork. One more encoding layer accumulates per fork, and the
double-encoded string still went out to providers in a shape none of them
ever produced.

Branch, dropped columns. The branch writers never agreed on what to
copy. gateway/slash_commands.py forwarded the full field set, but the
hermes_cli /branch loop forwarded reasoning and none of the structured
columns, and both TUI branch writers persisted role/content/timestamp alone,
so the same /branch, typed in three places, produced three different rows.
A TUI branch is a draft until its first submit, so the seed write is the only
write that ever persists the copied transcript: whatever it drops is gone.
Same end state as the fork bug, reached by omission instead of encoding.

The hermes_state.py writers now route the three structured fields through
a shared guard that keeps already-serialized TEXT as-is and dumps live
structures exactly as before. The CLI and TUI branch writers now forward the
reasoning fields the gateway path already forwarded. The read path is
untouched, so get_messages, the GET messages endpoint, session export and
session search all keep their current shapes.

Related Issue

Fixes #57240

Related PRs, not duplicates. Three open PRs overlap this area. #57454
("preserve reasoning fields when forking sessions", 2026-07-03) is an
independent fix for the same issue, scoped to the fork path; if it lands
first, only the branch half of this PR is still needed. #24769 ("preserve
tool and reasoning metadata during branch", 2026-05-13) and #42273 ("persist
full message history when branching", 2026-06-08) both predate this PR, sit
on older bases, and address the branch half alone. This PR is the only one
that covers the fork and branch paths together, at the writer level.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • hermes_state.py
    • new SessionDB._reasoning_json_text() helper: falsy → NULL, str
      (the column's own TEXT coming back through get_messages) → stored
      as-is, anything else → json.dumps as before
    • append_message and _insert_message_rows use it at all six dumps
      sites (3 fields × 2 writers)
  • hermes_cli/cli_commands_mixin.py
    • the /branch copy loop forwards reasoning_details,
      codex_reasoning_items and codex_message_items next to the
      reasoning it already carried
  • tui_gateway/server.py, tui_gateway/methods_session.py
    • _persist_branch_seed() and the session.branch RPC forward the
      reasoning fields instead of role/content/timestamp alone
  • tests/hermes_state/test_reasoning_roundtrip.py (new)
    • fork round-trip per field (get_messagesreplace_messages, the
      fork handler's exact copy step), fork-of-fork stability, an
      append_message round-trip with a stored row's TEXT, and a
      direct-write control pinning that live-runtime serialization is
      unchanged
  • tests/cli/test_branch_command.py, tests/test_tui_gateway_server.py
    • one branch regression per writer: branch a session whose assistant
      turn carries every reasoning field, reload the branched session, and
      assert each field came back

The branch writers and their regressions were added in response to
review on this PR; the round-trip guard alone left the branch
paths dropping the same fields before they ever reached it.

How to Test

  1. Cherry-picked onto current main, the three suites this PR touches:
    python -m pytest tests/cli/test_branch_command.py tests/hermes_state/test_reasoning_roundtrip.py tests/test_tui_gateway_server.py -q
    gives 535 passed.
  2. The same command on main, with the new tests in place and the
    production changes reverted: 8 failed, 527 passed. The 8 are exactly the
    8 new tests (the 5 round-trip variants, the CLI branch regression, and
    the two TUI branch regressions). The 6th test in the new file,
    TestDirectWrite::test_reasoning_fields_hydrate_as_structures, passes in
    both runs, pinning that live writes behave identically before and after.
  3. Reverting only the branch-writer forwarding leaves the three branch
    regressions failing on the reloaded row with a KeyError on the first
    dropped field ('reasoning_details' in the CLI test, whose writer
    already carried reasoning; 'reasoning' in both TUI tests), so they
    cannot pass against the unfixed writers.
  4. Quick manual check (the repro from forking a session double-encodes the reasoning columns — forked sessions silently lose reasoning replay #57240):
import tempfile
from pathlib import Path

from hermes_state import SessionDB

db = SessionDB(Path(tempfile.mkdtemp()) / "state.db")
db.create_session("src", source="cli")
db.append_message(
    "src", role="assistant", content="done",
    reasoning_details=[{"type": "reasoning.text", "text": "step one"}],
)
db.create_session("fork", source="cli")
db.replace_messages("fork", db.get_messages("src"))
print(type(db.get_messages_as_conversation("fork")[0]["reasoning_details"]))

Before: <class 'str'>. After: <class 'list'>.

Verified on Windows 11, Python 3.13.

Green ubuntu run for this head: https://github.com/cryptoyasenka/hermes-agent/actions/runs/31055688426

Checklist

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate. The three neighbours are disclosed above.
  • My PR contains only changes related to this fix/feature (no unrelated commits). One commit, one defect: reasoning fields lost when a session is copied; the branch writers were added at the reviewer's request
  • I've run the suites this PR touches (tests/cli/test_branch_command.py, tests/hermes_state/test_reasoning_roundtrip.py, tests/test_tui_gateway_server.py): 535 passed. Full pytest tests/ -q in my environment has pre-existing collection errors from optional deps that aren't installed, unrelated to this change; the full matrix is covered by the ubuntu run linked above
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Windows 11

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings): the helper's docstring documents the round-trip contract
  • I've updated cli-config.yaml.example if I added/changed config keys: N/A, no config keys
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows: N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide: stdlib-only, tests use tmp_path
  • I've updated tool descriptions/schemas if I changed tool behavior: N/A, no tool change

Screenshots / Logs

$ python -m pytest tests/cli/test_branch_command.py tests/hermes_state/test_reasoning_roundtrip.py tests/test_tui_gateway_server.py -q   # with this PR
535 passed in 56.49s

On main, with the new tests in place and the production changes reverted:

$ python -m pytest tests/cli/test_branch_command.py tests/hermes_state/test_reasoning_roundtrip.py tests/test_tui_gateway_server.py -q
FAILED tests/cli/test_branch_command.py::TestBranchPreservesReasoningFields::test_reasoning_fields_survive_branch
FAILED tests/hermes_state/test_reasoning_roundtrip.py::TestAppendMessageRoundTrip::test_string_value_not_double_encoded
FAILED tests/hermes_state/test_reasoning_roundtrip.py::TestForkRoundTrip::test_codex_message_items_survive_fork
FAILED tests/hermes_state/test_reasoning_roundtrip.py::TestForkRoundTrip::test_codex_reasoning_items_survive_fork
FAILED tests/hermes_state/test_reasoning_roundtrip.py::TestForkRoundTrip::test_fork_of_fork_stays_stable
FAILED tests/hermes_state/test_reasoning_roundtrip.py::TestForkRoundTrip::test_reasoning_details_survive_fork
FAILED tests/test_tui_gateway_server.py::test_persist_branch_seed_keeps_reasoning_fields
FAILED tests/test_tui_gateway_server.py::test_session_branch_keeps_reasoning_fields
8 failed, 527 passed in 70.47s

With the branch-writer forwarding reverted, the new branch regressions:

E       KeyError: 'reasoning_details'
FAILED tests/cli/test_branch_command.py::TestBranchPreservesReasoningFields::test_reasoning_fields_survive_branch
E           KeyError: 'reasoning'
FAILED tests/test_tui_gateway_server.py::test_persist_branch_seed_keeps_reasoning_fields
FAILED tests/test_tui_gateway_server.py::test_session_branch_keeps_reasoning_fields
3 failed

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for isolating the raw-row API fork failure. The write-path guard matches the current API flow: gateway/platforms/api_server.py:2027-2028 sends get_messages() rows into replace_messages(), while hermes_state.py:3819-3829 and 3929-3936 currently re-encode those raw TEXT fields.

Problems

  • The existing CLI /branch path remains incomplete: hermes_cli/cli_commands_mixin.py:939-947 copies reasoning but does not forward reasoning_details, codex_reasoning_items, or codex_message_items to append_message.
  • The TUI branch paths also persist only role/content: tui_gateway/server.py:1798-1800 and 8119-8124. These paths therefore discard the structured fields before the new helper can preserve them.

Suggested changes

  • Forward the three structured reasoning fields in those CLI/TUI branch writers and add branch regressions that reload and verify all three fields.

Automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 15, 2026
@cryptoyasenka cryptoyasenka changed the title fix(state): don't double-encode reasoning fields on message round-trips fix(state,cli,tui-gateway): keep reasoning fields intact across forks and branches Jul 15, 2026
@cryptoyasenka
cryptoyasenka force-pushed the fix/fork-reasoning-double-encode branch from 0d4d653 to 0382edf Compare July 17, 2026 11:57
@cryptoyasenka

Copy link
Copy Markdown
Contributor Author

Rebased onto current main and re-verified against it; the rebase carried both commits over with zero content drift (range-diff clean).

The forwarding suggested above is in the PR as the second commit (fix(cli,tui-gateway): keep reasoning fields across a branch): the hermes_cli /branch copy loop forwards reasoning_details / codex_reasoning_items / codex_message_items next to the reasoning it already carried, and both TUI branch writers (_persist_branch_seed and the session.branch RPC) forward the full reasoning field set instead of role/content alone. One branch regression per writer branches a session whose assistant turn carries every reasoning field, reloads the branched session, and asserts each field came back.

The regressions cannot pass against the unfixed writers: with the branch-writer forwarding reverted to main's, exactly those three tests fail on the reloaded row (KeyError: 'reasoning_details' in the CLI test, KeyError: 'reasoning' in both TUI tests). With hermes_state.py reverted instead, 5 of the 6 round-trip tests fail while the direct-write control stays green on both.

Against today's main the touched suites are green: 369 passed across the two branch-test files, 6/6 on the round-trip file, 814 passed on the scoped checklist set, ruff clean. PR description updated to match.

Ready for another look.

… and branches

get_messages() only deserializes content and tool_calls; the structured
reasoning columns (reasoning_details, codex_reasoning_items,
codex_message_items) come back as the raw TEXT they were stored as.
Feeding those rows straight back into a write, which is exactly what
the POST /api/sessions/{id}/fork handler does by piping get_messages()
into replace_messages(), hit an unguarded json.dumps() and stored the
already-serialized string encoded a second time. On replay of the fork,
json.loads() then yields the inner string instead of a list, and every
consumer's isinstance(..., list) gate silently drops it: preserved
Anthropic thinking blocks, Codex encrypted-reasoning/message-item
replay, and OpenRouter multi-turn reasoning context are all lost after
a fork, with one more encoding layer added per fork.

The /branch copy loop had the same defect from the other side: it
forwarded reasoning but none of the structured columns, and both TUI
branch writers persisted role/content alone, dropping reasoning and
reasoning_content along with them.

Route the six dumps sites in append_message and _insert_message_rows
through a shared guard that keeps already-serialized strings as-is;
structured values from the live runtime are dumped exactly as before.
Forward the reasoning fields in all three branch writers, matching the
set gateway/slash_commands.py already forwards on its own /branch path.
@teknium1

teknium1 commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Merged via PR #82109 with your commit cherry-picked intact (authorship preserved) — rebased onto current main with one additive test-file conflict resolved. Thank you @cryptoyasenka for the thorough fix: the branch-writer coverage across all three surfaces is what made this the salvage base over the competing fix. Closing this original since the salvage landed.

@teknium1 teknium1 closed this Aug 9, 2026
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 P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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

3 participants