From a4be043aeca93e795d85262658a9dfda9c4cd0fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=93=88=E5=9F=BA=E7=B1=B3?= <140241684+BlueX888@users.noreply.github.com> Date: Wed, 14 Oct 2026 08:44:59 +0800 Subject: [PATCH] fix(models): tolerate null template keys in the legacy thinking disable path (#6838) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(models): tolerate null template keys in the legacy thinking disable path `ModelConfig.when_thinking_enabled` is a plain `dict | None` with no per-key validation, so a template key left blank in config.yaml (`extra_body:`, `thinking:`) loads as an explicit None. The legacy disable path read those keys with `effective_wte.get(key, {})`, whose default only applies to an absent key — so it called `.get` on None and every create_chat_model for the profile crashed with AttributeError: 'NoneType' object has no attribute 'get', on the default thinking_enabled=False path (title generation, summarization, subagent and lead-agent builds). Guard the three reads with the same `or {}` idiom the sibling accesses in this module already use (`_infer_dialect`, the contract path), so a null value falls through the dialect heuristics like an absent one and the legacy path matches the contract path, which already tolerates the same profile shape. * fix(models): tolerate null nested thinking templates --------- Co-authored-by: Willem Jiang --- README.md | 2 + .../harness/deerflow/models/AGENTS.md | 1 + .../harness/deerflow/models/factory.py | 6 +-- backend/tests/test_model_factory.py | 40 +++++++++++++++++++ 4 files changed, 46 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 0b2d7d5f91..d8328f1fed 100644 --- a/README.md +++ b/README.md @@ -343,6 +343,8 @@ streams simulate chunks from a non-streaming response, while no-tool streams sta To route OpenAI models through `/v1/responses`, keep using `langchain_openai:ChatOpenAI` and set `use_responses_api: true` with `output_version: responses/v1`. + When thinking is disabled on a legacy profile, blank `extra_body`, `thinking`, and nested `extra_body.thinking` keys in a `when_thinking_enabled` template are treated as absent during dialect inference. When thinking is enabled, template values are forwarded as configured. + Models whose provider contract differs from DeerFlow's generic thinking/effort assumptions can declare a per-model mapping-valued `reasoning:` block (thinking `unsupported`/`optional`/`required`, the accepted effort values with aliases and a default, the payload dialect, and the reasoning-history requirement). The setup wizard's Z.AI GLM-5.3-Flash profile uses it: thinking stays on for every foreground and background call, and the effort selector offers the model's own `low`/`high`/`max` levels. Ollama's existing boolean `reasoning: true` remains a native provider setting and is forwarded to ChatOllama. When migrating a profile to a custom effort `path`, remove any old `reasoning_effort` setting from the profile and thinking templates; configuration validation rejects the leftover key. The chat UI drops a remembered provider-specific effort when switching to a legacy model that does not advertise it. Profiles without the block keep their existing provider behavior. See `config.example.yaml` for the shape and the equivalent manual configuration. For vLLM 0.19.0, use `deerflow.models.vllm_provider:VllmChatModel`. For Qwen-style reasoning models, DeerFlow toggles reasoning with `extra_body.chat_template_kwargs.enable_thinking` and preserves vLLM's non-standard `reasoning` field across multi-turn tool-call conversations. Legacy `thinking` configs are normalized automatically for backward compatibility, without modifying model defaults or caller-owned `extra_body` dictionaries; a reused dictionary can change the legacy switch between requests. If the endpoint reports a cumulative usage snapshot on every streaming chunk, set `cumulative_stream_usage: true` so DeerFlow converts those snapshots into per-chunk deltas; the option is disabled by default and leaves usage unchanged when a stable completion id is unavailable. Reasoning models may also require the server to be started with `--reasoning-parser ...`. If your local vLLM deployment accepts any non-empty API key, you can still set `VLLM_API_KEY` to a placeholder value. diff --git a/backend/packages/harness/deerflow/models/AGENTS.md b/backend/packages/harness/deerflow/models/AGENTS.md index 56fcf757a5..40edf70c94 100644 --- a/backend/packages/harness/deerflow/models/AGENTS.md +++ b/backend/packages/harness/deerflow/models/AGENTS.md @@ -33,6 +33,7 @@ fast-path permit check from queue insertion, or an older caller can be overtaken while handing off to the wait queue. - `create_chat_model(name, thinking_enabled)` instantiates LLM from config via reflection +- Legacy disable-path heuristics treat null `extra_body`, `thinking`, and nested `extra_body.thinking` template values as absent, allowing native/vLLM fallbacks. The enable path still forwards configured values. Regression cases live in `tests/test_model_factory.py`. - Supports `thinking_enabled` flag with per-model `when_thinking_enabled` / `when_thinking_disabled` overrides; on both the legacy and the contract path every template and every synthesized `extra_body` payload goes through `_merge_thinking_payload` (the native-Anthropic disable assigns the top-level `thinking` mapping outright — a tagged union keyed by `type`): keys are never removed (a template can only add or override; nested mappings inherit the profile's other keys), template wins on conflicts, so a template's `extra_body` never clobbers sibling keys such as `extra_body.tool_stream`. Corollary: a disable template cannot clear a key the profile sets, so enable-only keys such as `extra_body.thinking.budget_tokens` belong in `when_thinking_enabled`, never in the base `extra_body`. Templates are deep-copied as they are merged, so constructor kwargs never alias the cached `ModelConfig`. The vLLM switch has two spellings (`_VLLM_THINKING_SWITCHES`: legacy `thinking` alias, `enable_thinking`); when a profile spells it differently from the payload, the payload's value is mirrored onto the profile's spelling rather than deleting or canonicalizing it — `VllmChatModel` collapses the pair to `enable_thinking`, other OpenAI-compatible classes send both, and either way the server reads the template's intent. Do not canonicalize `thinking` → `enable_thinking` in the factory: chat templates differ on which key they honor, and only `VllmChatModel` opts into that remap - Supports vLLM-style thinking toggles via `when_thinking_enabled.extra_body.chat_template_kwargs.enable_thinking` for Qwen reasoning models, while normalizing legacy `thinking` configs for backward compatibility - A per-request `reasoning_effort` kwarg (the regular, non-bootstrap lead-agent build passes it even when `None`) is popped from `kwargs` and layered like `model_overrides`: a non-`None` value replaces the profile's, and the thinking transforms applied afterwards (`when_thinking_enabled`, `when_thinking_disabled`, the `extra_body.thinking` disable path) still decide the final value. Never let a key reach the constructor through both `kwargs` and the profile settings — Python raises `got multiple values for keyword argument` and the lead agent cannot be built for that model. Codex checks the requested level itself. Pinned by `tests/test_model_factory.py` and `tests/test_lead_agent_model_resolution.py` diff --git a/backend/packages/harness/deerflow/models/factory.py b/backend/packages/harness/deerflow/models/factory.py index 4f4ae514c8..abf89071da 100644 --- a/backend/packages/harness/deerflow/models/factory.py +++ b/backend/packages/harness/deerflow/models/factory.py @@ -229,14 +229,14 @@ def _apply_legacy_thinking_settings( if model_config.when_thinking_disabled is not None: # User-provided disable settings win on conflicts; profile keys survive. _merge_thinking_payload(settings, model_config.when_thinking_disabled) - elif has_thinking_settings and effective_wte.get("extra_body", {}).get("thinking", {}).get("type"): + elif has_thinking_settings and ((effective_wte.get("extra_body") or {}).get("thinking") or {}).get("type"): # OpenAI-compatible gateway: thinking is nested under extra_body _merge_thinking_payload(settings, {"extra_body": {"thinking": {"type": "disabled"}}}) settings["reasoning_effort"] = "minimal" - elif has_thinking_settings and (disable_chat_template_kwargs := _vllm_disable_chat_template_kwargs(effective_wte.get("extra_body", {}).get("chat_template_kwargs") or {})): + elif has_thinking_settings and (disable_chat_template_kwargs := _vllm_disable_chat_template_kwargs((effective_wte.get("extra_body") or {}).get("chat_template_kwargs") or {})): # vLLM uses chat template kwargs to switch thinking on/off. _merge_thinking_payload(settings, {"extra_body": {"chat_template_kwargs": disable_chat_template_kwargs}}) - elif has_thinking_settings and effective_wte.get("thinking", {}).get("type"): + elif has_thinking_settings and (effective_wte.get("thinking") or {}).get("type"): # Native langchain_anthropic: thinking is a direct constructor parameter settings["thinking"] = {"type": "disabled"} diff --git a/backend/tests/test_model_factory.py b/backend/tests/test_model_factory.py index 2db148ee1f..0a073699ea 100644 --- a/backend/tests/test_model_factory.py +++ b/backend/tests/test_model_factory.py @@ -435,6 +435,46 @@ def test_legacy_non_mapping_template_values_are_forwarded_not_rejected(monkeypat assert captured["extra_body"] == expected_extra_body +@pytest.mark.parametrize( + ("thinking_enabled", "when_thinking_enabled", "expected"), + [ + (False, {"extra_body": None, "thinking": {"type": "enabled"}}, {"thinking": {"type": "disabled"}}), + (True, {"extra_body": None, "thinking": {"type": "enabled"}}, {"extra_body": None, "thinking": {"type": "enabled"}}), + (False, {"extra_body": None}, {}), + (False, {"thinking": None}, {}), + (False, {"extra_body": {"thinking": None}}, {}), + (True, {"extra_body": {"thinking": None}}, {"extra_body": {"thinking": None}}), + (False, {"extra_body": {"thinking": None}, "thinking": {"type": "enabled"}}, {"thinking": {"type": "disabled"}}), + ( + False, + {"extra_body": {"thinking": None, "chat_template_kwargs": {"enable_thinking": True}}}, + {"extra_body": {"chat_template_kwargs": {"enable_thinking": False}}}, + ), + ], + ids=["thinking-off", "thinking-on", "extra-body-null", "thinking-null", "nested-null-off", "nested-null-on", "nested-null-native-fallback", "nested-null-vllm-fallback"], +) +def test_legacy_null_template_values_build_on_both_thinking_paths(monkeypatch, thinking_enabled, when_thinking_enabled, expected): + """A template key left blank in ``config.yaml`` (``extra_body:``, ``thinking:``) loads as an + explicit ``None`` — and ``dict.get(key, {})`` only defaults an *absent* key. The legacy + disable path therefore called ``.get`` on ``None`` and every ``create_chat_model`` for the + profile crashed with ``AttributeError`` — on the default ``thinking_enabled=False`` path, + i.e. title generation, summarization, subagent and lead-agent builds. A null template + value must fall through the disable-path dialect heuristics like an absent one, as the + guarded sibling accesses (``_infer_dialect``, the contract path) already do, while the + enable path keeps forwarding the template unchanged.""" + model = _make_model("legacy-null", supports_thinking=True, when_thinking_enabled=when_thinking_enabled) + captured: dict = {} + _patch_factory(monkeypatch, _make_app_config([model]), model_class=_capturing_class(FakeChatModel, captured)) + + factory_module.create_chat_model(name="legacy-null", thinking_enabled=thinking_enabled) + + for key in ("extra_body", "thinking"): + if key in expected: + assert captured[key] == expected[key] + else: + assert key not in captured + + @pytest.mark.parametrize("thinking_enabled", [True, False], ids=["thinking-on", "thinking-off"]) def test_legacy_when_thinking_enabled_deep_merges_into_the_base_extra_body(monkeypatch, thinking_enabled): """The legacy enable path used ``dict.update``, so ``when_thinking_enabled.extra_body``