Skip to content

fix: prevent session poisoning from empty partial-stream-stub assistant turns - #68041

Closed
0xprincess wants to merge 2 commits into
NousResearch:mainfrom
0xprincess:fix/partial-stream-stub-empty-assistant-poisoning
Closed

fix: prevent session poisoning from empty partial-stream-stub assistant turns#68041
0xprincess wants to merge 2 commits into
NousResearch:mainfrom
0xprincess:fix/partial-stream-stub-empty-assistant-poisoning

Conversation

@0xprincess

@0xprincess 0xprincess commented Jul 20, 2026

Copy link
Copy Markdown

Problem

A mid-tool-call stream drop that delivers no text produces a partial-stream stub carrying content:'' and tool_calls=None. The conversation loop's finish_reason == 'length' truncation path took the no tool calls branch and appended it to messages as {"role":"assistant","content":""} before the continuation nudge, then persisted it.

Strict providers — Moonshot/Kimi via OpenRouter — reject empty assistant content on the next replay:

HTTP 400: Invalid request: the message at position N with role 'assistant' must not be empty

Because the message is persisted, every subsequent turn re-failed — the session was unrecoverable, and resuming it later hit the same wall.

Root cause chain

  1. SSE stream ends with no finish_reason mid-tool-call args → _build_partial_stream_stub (content '', tool_calls=None).
  2. conversation_loop length path appends the stub via build_assistant_message → empty assistant turn enters history + state.db.
  3. Next request replays history → Moonshot 400 → classified non-retryable → session poisoned.

Fix — three layers, smallest blast radius first

1. conversation_loop (length path): an empty partial-stream stub is no longer appended as an interim assistant message — only the continuation user-message is. Stubs that delivered partial text are still persisted, so continuation stitching (existing #32086 behavior) is unchanged.

2. chat_completion_helpers.build_assistant_message: never serialize a textless assistant turn with content:'' — pad to a single space, the same trick as the existing reasoning_content pad (#15250, #17400). Tool-call turns are exempt (content:'' alongside tool_calls is accepted everywhere).

3. conversation_loop send boundary (durable repair): pad a textless assistant turn's empty content to a single space after all content-mutating passes (surrogate sanitize, whitespace normalization, thinking-only drops), before token estimation. This repairs sessions already poisoned by older builds: the persisted stub rows are rebuilt to '' on every reload (_rows_to_conversation strips whitespace, so a DB-side pad can't survive) — only a send-time pad reaches them.

Verification

  • 485 tests pass, 0 fail across the four affected files (test_partial_stream_finish_reason.py, test_run_agent.py, test_chat_completion_helpers.py, test_prompt_caching.py, test_turn_finalizer_interrupt_alternation.py), including new regressions reproducing the exact poisoned history shape.
  • Live replay: a real poisoned session's resumed history (loaded via SessionDB, repaired, serialized through the patched loop) replayed against Moonshot via OpenRouter returns HTTP 200 (was HTTP 400 before the fix).
  • Two pre-existing tests in test_run_agent.py (test_empty_content, test_unterminated_think_block_stripped) asserted the old content == '' behavior — updated to expect ' ' with a comment explaining why.

Notes

  • No behavior change for tool-call turns, non-empty turns, or providers that already accept empty content.
  • The send-time pad mutates only the per-request API copy; the stored transcript keeps original bytes.

…nt turns

A mid-tool-call stream drop with no delivered text produces a
partial-stream stub carrying content:'' and tool_calls=None.  The
conversation loop's truncation path appended it to history as
{"role":"assistant","content":""} before the continuation nudge, and
strict providers (Moonshot/Kimi via OpenRouter) reject empty assistant
content with HTTP 400 ("the message at position N with role 'assistant'
must not be empty") on the next replay.  Because the message is
persisted, every subsequent turn re-failed — the session was
unrecoverable.

Three layers, smallest blast radius first:

1. conversation_loop (length path): an EMPTY partial-stream stub is no
   longer appended as an interim assistant message; only the
   continuation user-message is.  Stubs that delivered partial text are
   still persisted so continuation stitching is unchanged.

2. chat_completion_helpers.build_assistant_message: never serialize a
   textless assistant turn with content:'' — pad to a single space, the
   same trick as the reasoning_content pad (NousResearch#15250, NousResearch#17400).  Tool-call
   turns are exempt (content:'' alongside tool_calls is accepted
   everywhere).

3. conversation_loop send boundary: pad a textless assistant turn's
   empty content to a single space AFTER all content-mutating passes
   (surrogate sanitize, whitespace normalization, thinking-only drops),
   before token estimation.  This is the durable repair for sessions
   ALREADY poisoned by older builds: the persisted stub rows are rebuilt
   to '' on every reload (_rows_to_conversation strips whitespace, so a
   DB-side pad can't survive) and only a send-time pad repairs them.

Verified: 485 tests pass across the four affected files; live replay of
a real poisoned session's resumed history against Moonshot via
OpenRouter returns HTTP 200 (was HTTP 400).
@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint area/streaming Streaming responses: gateway delivery, provider wire P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Jul 20, 2026
The empty-content pad loop called .strip() on am.get("content") without
checking the type.  Multimodal assistant turns carry content as a list
of parts (text/image), so a session with any image-bearing turn crashed
during request assembly:

    AttributeError: 'list' object has no attribute 'strip'

Guard with isinstance(content, str) — a multimodal turn is never the
empty textless shape the pad repairs.  Adds a regression test driving a
multimodal history through the loop.
@teknium1

Copy link
Copy Markdown
Contributor

Merged via PR #73028 (rebase-merge) with your two commits cherry-picked and authorship preserved — thank you @0xprincess, this was a well-diagnosed fix with excellent test coverage. On top of your work we added two follow-ups needed for current main: the multimodal test adapted to main's assistant-content flattening, and a codex_responses exemption (commentary-phase turns persist content:'' by design). All three layers landed as you designed them. Commits 309f06b / 2a26be6 on main.

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

Labels

area/streaming Streaming responses: gateway delivery, provider wire comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades 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