Don't ship 'strict': false on tool definitions when the tool didn't opt in#7782
Don't ship 'strict': false on tool definitions when the tool didn't opt in#7782alvinttang wants to merge 1 commit into
Conversation
…ible servers
OpenAI-compatible chat-completion servers (vLLM, Qwen-Tongyi/DashScope,
Mistral via LiteLLM, ...) reject any unknown field on the function tool
definition with:
extra_forbidden — 'loc': ('body', 'tools', 0, 'function', 'strict')
``convert_tools`` was emitting ``strict: false`` for every tool that did
not explicitly opt in, breaking these endpoints. OpenAI's own API treats
``strict`` as optional with an implicit default of ``False`` when the key
is absent — so dropping the key for the non-strict case preserves OpenAI
semantics while restoring compatibility with everyone else.
Behaviour:
* Tool with ``strict=False`` (default) -> ``strict`` key absent.
* Tool with ``strict=True`` -> ``strict: true`` preserved.
Refs microsoft#5814
t0ugh-sys
left a comment
There was a problem hiding this comment.
Code Review
Thank you for this well-scoped fix! The change is clean and addresses a real compatibility problem that many users have hit (vLLM, Qwen/DashScope, Mistral via LiteLLM). A few observations:
✅ What looks good
-
Clear rationale and comment. The inline comment explains why
strictis omitted rather than just what is done, and links the relevant issue (#5814). This is excellent for future maintainers. -
Minimal, focused diff. The production change is ~8 lines — only
convert_tools()is touched. No unnecessary refactoring, no scope creep. -
Good test coverage. Two new dedicated tests cover both the omission (
strictnot present) and the opt-in (strict=Truepropagated) paths. The existingtest_tool_callingassertions are updated cleanly with a helper to keep them DRY. -
Correct use of
tool_schema.get("strict"). Using.get()instead of the previousif "strict" in tool_schemaguard is both safer and correctly treats any falsy value (including explicitFalse) as "don't emit."
⚠️ Potential concerns
-
Edge case:
strict=0or other falsy-but-intentional values.tool_schema.get("strict")will silently dropstrict: 0orstrict: "". This is almost certainly fine in practice (nobody setsstrict=0), but worth noting. If you wanted to be maximally explicit,if tool_schema.get("strict") is Truewould make the intent even clearer — though the current approach is pragmatic and correct for all realistic inputs. -
FunctionDefinitionis aTypedDict(total=False)— mutation after construction. The code buildsfunction_defwithoutstrict, then conditionally setsfunction_def["strict"] = True. This works becauseTypedDict(total=False)allows missing keys, and runtimeTypedDictinstances are just plain dicts in Python. However, static type checkers may not fully track the conditional addition. Since you mention pyright delta is 0, this seems fine in practice — just flagging it. -
No changelog entry. This is a user-facing behavioral change (even if the previous behavior was arguably a bug). Consider adding a changelog/note so users of downstream tools know
strict: falsewill no longer appear in API payloads. This may matter for users who were (incorrectly) relying on the field's presence. -
The helper in
test_tool_callingcould be slightly more defensive._expected_function_payloadfilters byk != "strict", which assumesstrictis the only key that might be conditionally present. If more keys are added this way in the future, the helper would need updating. This is a minor nit — it's fine for now.
💡 Suggestion
Consider adding a brief comment near the .get("strict") check noting that BaseTool.schema always includes strict (even when False), so the .get() returning None only happens for raw dict schemas that don't include the field. This would help future readers understand why the condition works for both FunctionTool and raw schema paths.
Summary
This is a solid, well-tested fix for a real interoperability issue. The change is minimal, the tests are thorough, and the reasoning is sound. LGTM with minor suggestions above. 👍
Was poking at AgentChat 0.4.7+ with vLLM and got hit by #5814 —
extra_forbidden — loc=('body','tools',0,'function','strict'). Same story other folks have been reporting with Qwen/DashScope and Mistral via LiteLLM.convert_tools()inautogen-ext's OpenAI client always shippedstrictin the function payload. The dead-looking guardif "strict" in tool_schemaat line 264 was, well, dead —BaseTool.schemaalways setsstrict=<bool>, so every request went out with the key. OpenAI accepts it. Almost nobody else does.@ekzhu had agreed in the thread that
strictshould be optional. This patch just makes the serialization match: build theFunctionDefinitionwithoutstrict, then addfunction_def["strict"] = Trueonly when the tool actually opted in.FunctionDefinitionisTypedDict(total=False)so absence is valid, and OpenAI's documented default for the missing key isFalse— same semantics, just doesn't trip the other servers' schema validators.Reproduction in test
test_convert_tools_omits_strict_when_not_enabledbuilds aFunctionToolwith no strict flag and asserts"strict" not in function_def. Companiontest_convert_tools_includes_strict_when_enabledcovers the opt-in case.Without the fix:
After the fix: 2/2 pass. Updated
test_tool_calling's_expected_function_payloadhelper to stripstrictfrom the expected dict so the existing assertions still hold. Fullpytest tests/models/: 161 passed, 64 skipped.ruff check+ruff formatclean.pyrightdelta: 0 (24 pre-existing errors unchanged).Notes
test_create_args_with_strict_tools) keep emittingstrict=Trueand stay green.count_tokens_openaidoesn't readfunction["strict"], so the token-counting path is unaffected.extra_forbidden.Refs #5814