Studio: forward client reasoning_content to llama-server for opt-in preserve_thinking - #7758
Studio: forward client reasoning_content to llama-server for opt-in preserve_thinking#7758mahille wants to merge 3 commits into
Conversation
…reserve_thinking ChatMessage lacked a reasoning_content field, so Pydantic's default extra="ignore" silently dropped client-sent prior-turn reasoning before the proxy forwarded messages to llama-server; templates then rendered empty <think> blocks and multi-turn reasoning collapsed (unslothai#5846). This restores the inbound half of unslothai#7289 that was narrowed out pre-merge: typed field (non-string values -> None, not 422), reasoning-only turns kept with content="" (llama.cpp reads reasoning_content only after its content/tool_calls key check), and overflow retries clip preserved traces before sizing. Opt-in only: preserve_thinking launch default stays False; rendering stays template-gated without it.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e48656e7c5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| has_reasoning = bool(m.get("reasoning_content")) | ||
| if not has_content and not has_tool_calls and not has_reasoning: | ||
| continue | ||
| if not has_content and not has_tool_calls: |
There was a problem hiding this comment.
When a prior assistant turn has reasoning_content plus only provider-synthetic builtin tool_calls (the _server_tool / Gemini native_part cases handled by _strip_provider_synthetic_tool_history), this padding path does not run because has_tool_calls is true; the scrubber then removes all of those tool calls and its existing empty-turn check at routes/inference.py:16987 drops the whole message because it still ignores reasoning_content. That loses the preserved reasoning exactly when switching such a history to local GGUF, so the scrubber also needs to treat reasoning_content as content and add the empty content key after removing synthetic calls.
Useful? React with 👍 / 👎.
| total_est = sum(_estimate_message_tokens(m) for m in messages) | ||
| # Before sizing: see _clip_reasoning_contents for why this cannot wait for a target. | ||
| clipped = _clip_reasoning_contents(messages) | ||
| total_est = _estimate_messages_tokens(messages) |
There was a problem hiding this comment.
When the upstream overflow was caused mostly by a huge preserved reasoning_content, this new pre-sizing clip can already shrink the retry body below the usable context window, but total_est is then multiplied by a keep_ratio that is still based on the original pre-clip n_prompt_tokens. In that case a modest conversation with a 40k-character trace gets clipped and still has most middle turns dropped even though the clipped prompt estimate would fit, defeating the goal of clipping traces before evicting history; recompute/short-circuit the drop target after the clip instead of applying the stale original overflow ratio.
Useful? React with 👍 / 👎.
| "from assistant messages to replay text-part signatures." | ||
| ), | ||
| ) | ||
| reasoning_content: Optional[str] = Field( |
There was a problem hiding this comment.
Adding this typed field makes _openai_messages_for_passthrough() dump reasoning_content, but that helper is also reused for the safetensors/MLX client-tool templating path at routes/inference.py:11890-11893, not just for llama-server. In a non-GGUF request with client tools or tool history, prior assistant traces now reach the local template despite this change intending to defer that path, so either that path needs the same intentional preserve-thinking support or it should remove reasoning_content before templating.
Useful? React with 👍 / 👎.
1 participant
Follow-up to #7289. Fixes #5846.
Background
#7289 shipped only the size-gate half of the Qwen3.x thinking fix:
Qwen3.5-35B-A3Bwas misread as a 3B model, so thinking was disabled by default for 35B. The inbound half was narrowed out before merge and is restored here.On current main,
ChatMessage(studio/backend/models/inference.py) has noreasoning_contentfield, and Pydantic's defaultextra="ignore"silently drops it at the request boundary. Both llama-server-facing builders (_openai_messages_for_passthrough,_openai_messages_for_gguf_chat) re-serialize viamodel_dump(exclude_none=True), so the field can't reappear downstream. Result: when an OpenAI-compatible client (opencode, etc.) sends prior-turn reasoning back, the template renders an empty<think>block and the model loses its chain of thought across turns — exactly the "model can't remember what it decided last turn" symptom in #5846, reproducible even with Think + Preserve Think enabled. (LM Studio works because it doesn't drop the field.)Fix
This is opt-in only — no defaults change:
ChatMessage.reasoning_content: Optional[str]— forwarded verbatim to llama-server, where templates that branch onmessage.reasoning_contentrender it back into<think>…</think>when the client setspreserve_thinking=true. The launch defaultpreserve_thinking=false(llama_cpp.py) and the per-request opt-in (ChatCompletionRequest.preserve_thinking) are untouched. When the flag is off, templates gate the rendering, so forwarding is inert.Noneinstead of 422: gateways emit structured reasoning (list/dict), and typing the field must not turn previously-ignored payloads into request failures._drop_empty_assistant_sentinelsand padded withcontent=""— llama.cpp requires acontentortool_callskey and only readsreasoning_contentafter that check, so the turn was previously rejected outright.context_overflow=truncate_middle): preserved traces are clipped before sizing (_clip_reasoning_contents) and counted conservatively (_estimate_messages_tokens), so a huge trace in a protected turn no longer force-evicts the whole conversation middle. Clipping mutates only the retry body, never the client's history.Why a typed field and not
model_config(extra="allow")Per-message dicts are forwarded to llama-server without any key allow-list (unlike the passthrough body builder, which explicitly forwards only known OpenAI/llama-server fields).
extra="allow"onChatMessagewould therefore forward every arbitrary key any client sends straight into template rendering — an unbounded widening of the client→llama-server wire contract. This codebase already has that scar: adding the single typedextra_contentfield leaked Gemini-only keys toward llama-server and required the explicit scrubber_strip_provider_synthetic_tool_history. The typed field forwards exactly one documented key; structured/None values are normalized away at parse orexclude_nonedump.Repo-wide interaction check (mirrors the review rounds on #7289)
Consumers of inbound message dicts were traced end to end:
reasoning_contentreaches the template verbatim. Tested._build_external_messagesand the per-provider translators rebuild dicts field-by-field, remote APIs 400 on unknown keys, and each provider has its own reasoning-replay contract (Anthropic signedthinkingblocks, Geminithought_signatureviaextra_content). Excluded by design, not deferred./v1/messagesand OpenAI/v1/responsesendpoints —AnthropicMessagestill drops the field at parse and both paths rebuild messages field-by-field; restoring there needs their native reasoning models. Deferred follow-ups._extract_content_partsrebuilds role+content only; forwarding there without plumbingpreserve_thinkingthrough that path's template kwargs would put an uncontrolled key on the wire. Deferred follow-up.ChatMessage.contentalready acceptsreasoning-type content parts, which forward toward llama-server unparsed. Out of scope.Review-history points from #7289 explicitly addressed: structured reasoning →
None, not 422 (Codex); reasoning-only turns getcontent=""(Codex); overflow retries clip reasoning before content (Codex);preserve_thinkingdefault left off (Codex / maintainer). Deviation from the narrowed-out reference implementation: the deadreasoning=/contents=parameterization on_clip_long_contentswas not restored — its only call site passed a token target as a per-side character budget (unit mismatch) withreasoning=False; the standalone pre-sizing clip covers the behavior.Tests
11 new tests across three files: parse-level contract (round-trip, structured → None, absent-not-null), sentinel padding, byte-identical forwarding on both llama-server paths, the
_strip_provider_synthetic_tool_historyasymmetry lock (scrubsextra_content, keepsreasoning_content), and the overflow clip/sizing behavior.