fix(agent): strip reasoning replay fields for strict chat-completions providers - #64296
fix(agent): strip reasoning replay fields for strict chat-completions providers#64296Soju06 wants to merge 1 commit into
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
This PR strips reasoning/reasoning_details from assistant messages when sending to strict chat-completions providers.
Looks Good
- Gated strip preserves reasoning continuity for OpenRouter/NousResearch.
- Copy-on-write scheme ensures persisted history unchanged.
- Thorough PR description with correctness notes.
- Comprehensive test coverage added.
No Issues Found
Reviewed by Hermes Agent
f07a6b7 to
25034a3
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing both the normal transport and the iteration-limit bypass; the replay leak is present on current main.
Problems
agent/transports/chat_completions.py:280putsreasoningandreasoning_detailsbehind the same OpenRouter/Nous allow-list. Current main treats them differently:agent/conversation_loop.py:832-835always removes trajectory-onlyreasoning, while retainingreasoning_detailsfor replay. The direct_build_api_kwargs()path would therefore still sendreasoningto the two allowed endpoints.PR_BODY.md:1is a duplicate PR description, not a repository artifact.
Suggested changes
- Strip
reasoningunconditionally; retain onlyreasoning_detailsfor the capability-gated endpoints, and update the OpenRouter/Nous regression test accordingly. - Add a regression test for
agent/chat_completion_helpers.py:1738, since the summary path bypasses the transport and is changed separately. - Exclude
PR_BODY.mdfrom the salvage.
Automated hermes-sweeper review.
| ): | ||
| out_msg = mutable_msg() | ||
| out_msg.pop("codex_reasoning_items", None) | ||
| out_msg.pop("codex_message_items", None) | ||
| out_msg.pop("tool_name", None) | ||
| out_msg.pop("effect_disposition", None) | ||
| out_msg.pop("timestamp", None) # #47868 — leak into strict providers | ||
| if strip_reasoning_fields: |
There was a problem hiding this comment.
reasoning should not share the reasoning_details allow-list. Current main always removes trajectory-only reasoning before dispatch (agent/conversation_loop.py:832-835); retain only reasoning_details for OpenRouter/Nous and pop reasoning unconditionally here.
25034a3 to
5b85eb8
Compare
5b85eb8 to
420a894
Compare
|
Addressed all three points:
Rebased on current main; full transport, strict-validation, and run_agent test suites pass (530 tests). |
420a894 to
3bdeec1
Compare
|
@teknium1 Gentle ping — all points from the review here have been addressed (summary in the comment above), the branch is rebased on current main, and CI is green. Ready for another look whenever convenient. |
… providers Strict OpenAI-compatible Chat Completions providers reject assistant replay messages carrying non-standard fields (reasoning, reasoning_details, and Codex Responses bookkeeping) with HTTP 400. Strip them at the transport boundary for providers that opt into strict validation, keeping the fields in the persisted transcript for providers that consume them.
3bdeec1 to
b1823f5
Compare
Problem
Assistant messages in agent history can carry two non-standard reasoning replay fields:
reasoning(trajectory reasoning text) andreasoning_details(OpenRouter's unified structured-reasoning format, e.g. signed Anthropic thinking blocks).reasoning_detailsis deliberately kept on API-bound messages so OpenRouter can maintain multi-turn reasoning continuity — but it is not part of the OpenAI Chat Completions message schema, and it currently reaches every chat-completions endpoint.Strict OpenAI-compatible providers (Fireworks, Mistral, and many self-hosted/proxy gateways) reject any request whose replayed assistant messages contain these fields with HTTP 400
Extra inputs are not permitted, field: 'messages[N].reasoning_details'. Once a session contains such a turn, every subsequent request fails. The common trigger is a mixed-provider session: turns produced via OpenRouter (or an Anthropic reasoning model) persistreasoning_details, and a model switch or fallback to a strict direct provider makes the whole history unreplayable. The existing reactive recovery only fires on Anthropic-flavoredthinking_signatureerror text, so these generic 400s classify as non-retryable format errors — and even when recovery applies, it costs a failed round trip per session.Change
ChatCompletionsTransport.convert_messages()now stripsreasoning/reasoning_detailsfrom the wire copy of messages, gated on the target endpoint via a new_base_url_consumes_reasoning_details()helper: the fields are kept for endpoints that consume the replay (openrouter.ai,nousresearch.com) and stripped for everything else, including when nobase_urlis supplied (safe default for strict providers). This mirrors the existing keep/strip pattern used for the Geminiextra_contentthought_signature.build_kwargs()passesbase_urlthrough toconvert_messages()(it is already provided by both the profile and legacy call paths).agent/chat_completion_helpers.pyhand-builds messages and bypasses the transport; it already mirrored the other schema-foreign strips (tool_name,codex_*,timestamp) but leakedreasoning_details. The same gated strip is mirrored there.Correctness notes
reasoning_detailsis still replayed for reasoning continuity (signed Anthropic thinking blocks on tool-call turns require it). A new test pins this.convert_messages()uses the existing copy-on-write scheme, so persisted history keeps the fields and future turns on routes that consume them can still replay reasoning state. A test asserts the source list is untouched.reasoningwas already popped on the main-loopapi_messagesbuild for all providers ("trajectory storage only"); stripping it in the transport extends the same guarantee to direct_build_api_kwargs()entry points without changing main-loop behavior.reasoning_contentis intentionally not touched: it is a provider-schema echo-back field (DeepSeek/Kimi thinking mode) reconciled per-provider byreapply_reasoning_echo_for_provider().Tests
New:
tests/agent/transports/test_chat_completions.py: stripsreasoning/reasoning_detailsfor a strict base_url and when base_url is unknown; keeps them foropenrouter.ai/inference-api.nousresearch.com; source messages never mutated.tests/run_agent/test_strict_api_validation.py: end-to-end through_build_api_kwargs()— stripped for a Fireworks agent (history retained,reasoning_contentuntouched), kept for an OpenRouter agent.Runs (all green):
tests/agent/transports/,tests/run_agent/test_strict_api_validation.pytests/run_agent/test_provider_parity.py,test_deepseek_reasoning_content_echo.py,test_thinking_only_sanitizer.py,test_thinking_sig_recovery_persistence.py,test_deepseek_v4_thinking_live.py,tests/agent/test_compressed_summary_metadata.py,tests/plugins/model_providers/tests/agent/+tests/run_agent/(one pre-existing environment-dependent failure intests/agent/lsp/test_broken_set.pyreproduces identically on a clean checkout ofmain).🤖 Generated with Claude Code