Skip to content

fix(sessions): guard _save_session_log against list-type assistant content - #29451

Open
EloquentBrush0x wants to merge 1 commit into
NousResearch:mainfrom
EloquentBrush0x:fix/session-snapshot-list-content
Open

fix(sessions): guard _save_session_log against list-type assistant content#29451
EloquentBrush0x wants to merge 1 commit into
NousResearch:mainfrom
EloquentBrush0x:fix/session-snapshot-list-content

Conversation

@EloquentBrush0x

@EloquentBrush0x EloquentBrush0x commented May 20, 2026

Copy link
Copy Markdown
Contributor

Problem

_save_session_log (introduced in #29490) silently writes no file when any assistant message in the session has list-type content — the kind produced by multimodal turns or tool-use responses where the API returns content as a list of parts rather than a bare string.

The loop unconditionally calls _clean_session_content(msg["content"]), which is typed and implemented for str. When content is a list, re.sub raises TypeError. The outer try/except Exception swallows it silently (only logged under verbose_logging), so users who set sessions.write_json_snapshots = true see no JSON files and no error message.

_flush_messages_to_session_db, which handles the same messages list, already has an explicit isinstance(content, list) branch — confirming this format occurs in real sessions.

Fix

Add an isinstance(content, str) guard before the _clean_session_content call. List-type content passes through unchanged; REASONING_SCRATCHPAD tags cannot appear in structured content blocks, so skipping normalisation for them is correct.

# before
if msg.get("role") == "assistant" and msg.get("content"):

# after
if msg.get("role") == "assistant" and isinstance(msg.get("content"), str) and msg.get("content"):

Tests

Added test_save_session_log_writes_with_list_content to TestSessionJsonSnapshotOptIn. It passes an assistant message whose content is [{"type": "text", …}, {"type": "tool_use", …}], asserts the snapshot file is written, and asserts the list is preserved unchanged.

All 5 tests in TestSessionJsonSnapshotOptIn pass; 338/339 in test_run_agent.py pass (the 1 pre-existing failure is an unrelated missing openai module in the local env).

Checklist

  • Reproduces with write_json_snapshots = true and a session containing tool calls before the fix
  • Regression test added
  • No behavior change when content is a string
  • No behavior change when write_json_snapshots is false (fast no-op path is unchanged)

…ntent

When sessions.write_json_snapshots is enabled, _save_session_log iterates
messages and passes each assistant content value to _clean_session_content,
which expects a str.  Assistant messages whose content is a list of
OpenAI-style content parts (multimodal turns, tool-use responses) cause
re.sub to raise TypeError.  The outer try/except swallows the error
silently, so no session_{sid}.json is ever written for those sessions
despite the opt-in flag being set.

Add an isinstance(content, str) guard so list-type content passes through
unchanged (REASONING_SCRATCHPAD tags cannot appear in structured content
blocks, so skipping the normalisation is correct).

Add a regression test that exercises _save_session_log with an assistant
message whose content is a list containing text and tool_use parts,
asserting both that the file is written and that the list is preserved.
@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 labels May 20, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused regression fix. Current main still invokes the string-only _clean_session_content() for any truthy assistant content at run_agent.py:2597; that helper calls re.sub at run_agent.py:2523, so list-valued content aborts the opt-in writer through the broad catch at run_agent.py:2646.

The proposed string guard is correct: list content proceeds to _redact_message_content(), which already supports list-of-parts input at run_agent.py:2544-2555, and the added test covers both snapshot creation and preservation of the structured list.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users area/sessions Session lifecycle, resume, persistence, history labels Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users 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.

3 participants