Skip to content

fix(agent): preserve Qwen3.6 / vLLM preserve_thinking history (#56004) - #56019

Open
Tranquil-Flow wants to merge 1 commit into
NousResearch:mainfrom
Tranquil-Flow:fix/56004-qwen-preserve-thinking
Open

Tranquil-Flow wants to merge 1 commit into
NousResearch:mainfrom
Tranquil-Flow:fix/56004-qwen-preserve-thinking

Conversation

@Tranquil-Flow

Copy link
Copy Markdown
Contributor

What was fixed

Qwen3.6 (and similar self-hosted models served via vLLM with the preserve_thinking chat-template flag) loses its prior-turn chain-of-thought on every replay. The agent loop strips both reasoning_content (strict-provider gate in copy_reasoning_content_for_api) and reasoning (unconditional pop in conversation_loop.py), so vLLM ≥0.20 has nothing to render under the preserve_thinking template.

Root cause

Two client-side strip paths in agent/agent_runtime_helpers.py and agent/conversation_loop.py unconditionally remove the reasoning fields that vLLM/Qwen3.6 need to re-render prior thinking. vLLM ≥0.20 also drops reasoning_content on incoming assistant messages (vllm-project/vllm#38488), so the existing DeepSeek/Kimi/MiMo reasoning_content echo-back path is incompatible regardless.

Fix

Added a _preserves_thinking_history() predicate on AIAgent that detects Qwen3.6-style endpoints by model name (qwen3.6 / qwen-3.6, conservative word-boundary regex — does NOT match plain qwen3 per the model card). Result cached on the AIAgent instance keyed by (provider, model, base_url) — same invalidation contract as the adjacent _thinking_pad_cache, so all switch_model / _try_activate_fallback paths automatically invalidate it.

Three touch points now gated on the predicate:

  1. copy_reasoning_content_for_api — skip the strict-provider reasoning_content strip on preserves-thinking-history providers.
  2. conversation_loop.py — skip the unconditional reasoning pop (keep the field on the outgoing message for vLLM to render).
  3. reapply_reasoning_echo_for_provider — strip reasoning on the way out ONLY for strict providers (handles the strict-fallback case where Qwen3.6 → Mistral would 422 on the unknown field).

Verification

  • 11 new regression tests, all green on this commit, 9/11 RED on upstream/main with AttributeError: 'AIAgent' object has no attribute '_preserves_thinking_history' (real production-path coverage).
  • 5 detection tests (qwen3.6 positive, qwen-3.6 hyphen positive, qwen3 negative — no false positives on older Qwen, deepseek/mistral negative — distinct path through _needs_thinking_reasoning_pad).
  • 3 strip-path integration tests covering both copy_reasoning_content_for_api and reapply_reasoning_echo_for_provider in both directions (preserve-history provider AND strict-fallback).
  • 1 cache-invalidation test (keyed on model/provider/base_url).
  • 1 integration sanity test asserting the predicate is exposed on AIAgent.
  • No-regression sweep over existing reasoning/conversation/fallback tests (279 + 152 passing).
python3 -m pytest tests/run_agent/test_qwen_preserve_thinking_56004.py -v -o "addopts=" --tb=short
============================= 11 passed in 0.52s ==============================

Files changed (1 focused commit, 4 files, +289/-19)

  • agent/agent_runtime_helpers.py — gated reasoning_content strip + new reasoning strip branch in reapply_reasoning_echo_for_provider (handles strict-fallback).
  • agent/conversation_loop.py — gated the unconditional reasoning pop.
  • run_agent.py — new _preserves_thinking_history() predicate on AIAgent.
  • tests/run_agent/test_qwen_preserve_thinking_56004.py — 11 new regression tests.

Notes

  • Detection is intentionally model-name based per the issue author's "reasonable default if a config flag is undesirable" note. A future config-driven flag (custom_providers[*].preserve_thinking: true) is documented in the docstring as a more robust follow-up — today this covers the reported Qwen3.6 / vLLM case correctly.
  • extra_body: chat_template_kwargs: {preserve_thinking: true} already works server-side today; this PR completes the client-side loop.

Auto-published by Moonsong via Path B automated pipeline.

@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 provider/qwen Qwen / Alibaba Cloud (OAuth) P3 Low — cosmetic, nice to have labels Jul 1, 2026
@alt-glitch

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Related: fixes #56004; same reasoning-replay family as #20577 and its open fix PR #20607. This PR and #20607 are competing fixes with different mechanisms: #20607 re-embeds <think> blocks inline into content (run_agent.py only), while this PR adds a _preserves_thinking_history() predicate that skips the reasoning-strip paths and keeps the reasoning field for vLLM>=0.20 to render. A maintainer should pick one approach.

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for isolating the normal replay stripping path; current main still removes reasoning in agent/conversation_loop.py:818-821, so the underlying report remains actionable.

Problems

  • agent/chat_completion_helpers.py:1692-1694 independently builds the maximum-iterations summary request and unconditionally removes reasoning. This PR does not touch that path, so the stated replay guarantee is incomplete.
  • The new qwen[-_]?3\.6 predicate is model-name-only, although the behavior depends on the served endpoint and its preserve_thinking chat-template configuration. It can therefore send the non-standard field to an endpoint that does not accept it. Related open PR feat(custom-provider) Support opt-in preserve thinking #58380 uses an explicit custom-provider opt-in.
  • The added conversation-loop test at tests/run_agent/test_qwen_preserve_thinking_56004.py:292-305 tests only the predicate, not the outgoing payload.

Suggested changes

  • Make the capability endpoint-configured, cover the summary builder, and add payload-level tests for normal replay, summary replay, and strict fallback.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 15, 2026
@alt-glitch alt-glitch added the area/local-models Local model inference/runtimes: llama.cpp, Ollama, LM Studio, MLX/vLLM, GGUF, VRAM and offload label Sep 18, 2026

This branch has not been deployed

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

Labels

area/local-models Local model inference/runtimes: llama.cpp, Ollama, LM Studio, MLX/vLLM, GGUF, VRAM and offload comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P3 Low — cosmetic, nice to have provider/qwen Qwen / Alibaba Cloud (OAuth) sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants