Repository navigation
fix(models): tolerate null template keys in the legacy thinking disable path - #6838
Conversation
…le 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.
github-actions
Bot
added
area:backend
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Reviewed at head 0fb4b3b. The fix is correct and minimal: all three legacy disable-path template reads now treat an explicit null (blank YAML key) like an absent one, matching the guarded sibling accesses in _infer_dialect and the contract path, and no previously-working configuration changes behavior. I verified the new test's four parametrized expectations against the head expressions by simulation and they all check out. One residual gap worth a follow-up, posted inline: the same crash class survives one level deeper on line 232, where the inner .get("thinking", {}) still uses the default-arg pattern and raises on extra_body.thinking: null.
| # 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", {}).get("type"): |
There was a problem hiding this comment.
The fix guards the top-level template keys, but the same crash class survives one level deeper: a blank nested key — when_thinking_enabled: {extra_body: {thinking: }}, which YAML loads as {"extra_body": {"thinking": None}} — still hits .get("thinking", {}) here and raises AttributeError: 'NoneType' object has no attribute 'get', because .get(key, default) only defaults an absent key. The neighboring reads already tolerate this shape: the vLLM elif on line 236 uses .get("chat_template_kwargs") or {}, and _infer_dialect (line 162-164) guards with isinstance. Minimal consistent fix: ((effective_wte.get("extra_body") or {}).get("thinking") or {}).get("type") — and consider adding a {"extra_body": {"thinking": None}} case to the parametrize list so the nested-null shape stays covered.
Why
A
config.yamlprofile like this crashedcreate_chat_modelwith an opaqueAttributeError:ModelConfig.when_thinking_enabledis a plaindict | Nonewith no per-key validation (backend/packages/harness/deerflow/config/model_config.py:166-169), so the loader accepts the profile silently. On the legacy (noreasoning:contract) disable path,_apply_legacy_thinking_settingsread the template witheffective_wte.get("extra_body", {})(backend/packages/harness/deerflow/models/factory.py:232) —dict.get(key, {})only defaults an absent key, so the explicitNonecame back and the chained.getraisedAttributeError: 'NoneType' object has no attribute 'get'.thinking_enabled=Falseis the default at everycreate_chat_modelcall site (title generation, summarization, subagent and lead-agent builds), so the crash hit the first model build, not an exotic flag combination. The sibling accesses in the same module already tolerate the identical shape —_infer_dialect(factory.py:162) and_apply_contract_thinking_settings(factory.py:274) both useeffective_wte.get("extra_body") or {}— so only the legacy path diverged: the same profile under areasoning:contract built fine.What changed
factory.py:232,:236,:239) now use(effective_wte.get(key) or {}), matching the guarded sibling accesses: a nullextra_body/thinkingtemplate key falls through the dialect heuristics exactly like an absent one..gettoo.test_legacy_null_template_values_build_on_both_thinking_pathscovering both thinking directions and all three null template shapes.Surface area
(Change is confined to the model factory under
backend/packages/harness/deerflow/models/; no previously-working configuration changes behavior — only previously-crashing profile shapes build.)Bug fix verification
backend/tests/test_model_factory.py::test_legacy_null_template_values_build_on_both_thinking_pathsmainand green on this branch? Yes. Onmain:3 failed, 1 passed—AttributeError: 'NoneType' object has no attribute 'get'atfactory.py:232(nullextra_body) andfactory.py:239(nullthinking); thethinking-oncase was never broken, which matches the contract path already tolerating the same shape. On this branch:4 passed.Validation
Full offline suite
make test(4 parallel shards):7510 passed, 6 failed, 131 skipped. All six failures are pre-existing on unpatchedmainand unrelated to this change: five intests/test_web_fetch_relative_links.py(byte-identical failure sets when the file is run on patched vs unpatched trees) and one load-flake intests/test_scheduled_goal_slow_rollback.pythat passes21 passed, 14 skippedin isolation.AI assistance
Tool(s) used: Claude Code (ZCode agent)
How you used it: AI located the crash by code inspection, wrote the regression test and the three-line guard fix, and ran the red/green validation quoted above; the final diff and test results were reviewed before opening this PR.