feat(tool): normalize OpenAI tool-call wire formats and sanitize tool names - #43
feat(tool): normalize OpenAI tool-call wire formats and sanitize tool names#43isaacbmiller wants to merge 1 commit into
Conversation
… names
Adds robust normalization in `ToolCalls.validate_input` so the adapter
postprocessor can call `ToolCalls.model_validate(tool_calls)` directly
on whatever LiteLLM hands us, instead of hand-rolling `json_repair` and
shape-sniffing in every call site.
- `_normalize_openai_tool_call` handles OpenAI Chat Completions
(`{type:'function', function:{name, arguments}}`), OpenAI Responses
API (`{type:'function_call', name, arguments, call_id}`), and DSPy-
native `{name, args}` shapes.
- Cached LiteLLM pydantic objects whose `model_dump()` raises
`TypeError` (the MockValSer/SchemaSerializer bug) fall back to
attribute access via `_build_call`; objects with no recoverable
attributes re-raise the original `TypeError` instead of silently
returning malformed data.
- `_validate_call_list` reports the failing index, type, and value when
an item can't be coerced, replacing the previous generic
`Received invalid value for \`dspy.ToolCalls\`` error.
- `ToolCall` gains an optional `id` field for OpenAI `tool_call_id`
round-tripping.
- `_sanitize_tool_name` enforces OpenAI's `^[a-zA-Z0-9_-]+$` pattern at
`Tool` construction time so emitted tool definitions don't 400 from
the API.
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Greptile SummaryThis PR centralises OpenAI tool-call wire-format normalization into
Confidence Score: 4/5Safe to merge; normalization logic is sound across all three wire formats and the TypeError-recovery path is well-tested. The core normalization branches work correctly for every identified real-world input. The
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[ToolCalls.model_validate / validate_input] --> B{data type?}
B -->|list| C[_validate_call_list]
B -->|dict with tool_calls list| C
B -->|dict with name+args| D[ToolCall directly]
B -->|other| E[raise ValueError]
C --> F[for each item: _normalize_openai_tool_call]
F --> G{type == function and function?}
G -->|yes| H[_build_call fn+fn+item]
G -->|no| I{type == function_call and name?}
I -->|yes| J[_build_call all=item]
I -->|no| K{not dict and model_dump?}
K -->|yes| L[model_dump and recurse]
L -->|TypeError + name=None| M[re-raise TypeError]
L -->|TypeError + name OK| N[_build_call via getattr]
K -->|no| O[return item as-is]
H --> P[guard: name != None and args != None]
J --> P
N --> P
O --> P
P -->|pass| Q[ToolCall created]
P -->|fail| R[raise ValueError with index+type+value]
Reviews (1): Last reviewed commit: "feat(tool): normalize OpenAI tool-call w..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| call_id = ( | ||
| ToolCalls._get_tool_call_value(id_source, "call_id") | ||
| or ToolCalls._get_tool_call_value(id_source, "id") | ||
| ) | ||
| if call_id: |
There was a problem hiding this comment.
or silently drops a falsy call_id
The expression _get_tool_call_value(id_source, "call_id") or _get_tool_call_value(id_source, "id") will skip call_id whenever the field is present but falsy (e.g., an empty string ""). In that case the code falls through to the id field, potentially returning the wrong ID or None, and the if call_id: guard below would then omit the field entirely. Using an explicit is None check avoids silently losing an explicit-but-empty value.
| call_id = ( | |
| ToolCalls._get_tool_call_value(id_source, "call_id") | |
| or ToolCalls._get_tool_call_value(id_source, "id") | |
| ) | |
| if call_id: | |
| call_id = ToolCalls._get_tool_call_value(id_source, "call_id") | |
| if call_id is None: | |
| call_id = ToolCalls._get_tool_call_value(id_source, "id") | |
| if call_id is not None: | |
| normalized["id"] = call_id |
| if isinstance(data, dict): | ||
| inner = data.get("tool_calls") | ||
| if isinstance(inner, list): | ||
| return cls._validate_call_list(inner) | ||
| if "name" in data and "args" in data: | ||
| return {"tool_calls": [cls.ToolCall(**data)]} |
There was a problem hiding this comment.
The "name" in data and "args" in data fast-path at line 462 constructs a ToolCall directly from data without going through _normalize_openai_tool_call. This means a single DSPy-native dict whose args value is a JSON string (rather than an already-parsed dict) will hit a Pydantic ValidationError instead of being repaired by json_repair, inconsistently with the list path which calls _parse_tool_call_args. Additionally, a single OpenAI Chat Completions dict {type:"function", function:{...}} passed here falls through to the final raise ValueError rather than being normalized.
| def test_toolcalls_normalizes_cached_litellm_tool_call_object(): | ||
| class Function: | ||
| name = "search" | ||
| arguments = '{"query": "hello", "k": 5}' | ||
|
|
||
| class CachedToolCall: | ||
| id = "call_123" | ||
| type = "function" | ||
| function = Function() | ||
|
|
||
| def model_dump(self): | ||
| raise TypeError("'MockValSer' object cannot be converted to 'SchemaSerializer'") | ||
|
|
||
| tc = ToolCalls.model_validate([CachedToolCall()]) | ||
|
|
||
| assert len(tc.tool_calls) == 1 | ||
| assert tc.tool_calls[0].name == "search" | ||
| assert tc.tool_calls[0].args == {"query": "hello", "k": 5} | ||
| assert tc.tool_calls[0].id == "call_123" | ||
|
|
||
|
|
||
| def test_toolcalls_normalizes_cached_dspy_native_object_with_broken_model_dump(): | ||
| """A cached pydantic object that doesn't match the OpenAI wire shape but has | ||
| DSPy-native (name, args) attributes should still normalize via getattr when | ||
| model_dump() raises TypeError (MockValSer error).""" | ||
| class CachedDSPyToolCall: | ||
| id = "call_456" | ||
| name = "lookup" | ||
| args = {"key": "value"} | ||
|
|
||
| def model_dump(self): | ||
| raise TypeError("'MockValSer' object cannot be converted to 'SchemaSerializer'") | ||
|
|
||
| tc = ToolCalls.model_validate([CachedDSPyToolCall()]) | ||
|
|
||
| assert len(tc.tool_calls) == 1 | ||
| assert tc.tool_calls[0].name == "lookup" | ||
| assert tc.tool_calls[0].args == {"key": "value"} | ||
| assert tc.tool_calls[0].id == "call_456" | ||
|
|
||
|
|
||
| def test_toolcalls_propagates_typeerror_when_no_recoverable_attributes(): | ||
| """When model_dump raises and the object has no name attribute, the | ||
| TypeError should propagate rather than being silently swallowed.""" | ||
| class BrokenObject: | ||
| def model_dump(self): | ||
| raise TypeError("'MockValSer' object cannot be converted to 'SchemaSerializer'") | ||
|
|
||
| with pytest.raises(TypeError, match="MockValSer"): | ||
| ToolCalls.model_validate([BrokenObject()]) | ||
|
|
||
|
|
||
| def test_toolcalls_rejects_explicit_none_name(): | ||
| """A normalized item with name=None should fail with a clear targeted error, | ||
| not slip through the key-existence guard into a downstream pydantic error.""" | ||
| with pytest.raises(ValueError, match="Could not normalize tool call at index 0"): | ||
| ToolCalls.model_validate([{"name": None, "args": {"q": "x"}}]) | ||
|
|
||
|
|
||
| def test_toolcalls_rejects_explicit_none_args(): | ||
| """A normalized item with args=None should also be rejected with a clear error.""" | ||
| with pytest.raises(ValueError, match="Could not normalize tool call at index 0"): | ||
| ToolCalls.model_validate([{"name": "search", "args": None}]) | ||
|
|
There was a problem hiding this comment.
The PR description says "OpenAI Chat Completions wire format" and "OpenAI Responses API wire format" are among the tested cases, but the added tests only exercise these formats via Python object attribute access. There are no tests that pass actual dicts such as {"type": "function", "function": {"name": "search", "arguments": "{}"}} through ToolCalls.model_validate, which risks silent regressions if someone refactors the branch conditions.
|
Superseded by #47, which moves all tool-call wire-shape coercion to a single boundary function ( |
1 participant
What
Adds robust normalization in
ToolCalls.validate_inputso the adapter postprocessor can callToolCalls.model_validate(tool_calls)directly on whatever LiteLLM hands us, instead of hand-rollingjson_repairand shape-sniffing in every call site.Changes
_normalize_openai_tool_callhandles three wire formats:{type:'function', function:{name, arguments}}{type:'function_call', name, arguments, call_id}{name, args}model_dump()raisesTypeError(MockValSer/SchemaSerializer bug) fall back to attribute access via_build_call; objects with no recoverable attributes re-raise the originalTypeErrorinstead of silently returning malformed data._validate_call_listreports the failing index, type, and value when an item can't be coerced, replacing the previous genericReceived invalid value for \dspy.ToolCalls`` error.ToolCall.idoptional field added for OpenAItool_call_idround-tripping._sanitize_tool_nameenforces OpenAI's^[a-zA-Z0-9_-]+$pattern atToolconstruction time so emitted tool definitions don't 400 from the API.Why this is a standalone PR
This is a prerequisite for the native-FC adapter pipeline work (formerly #40), but it's a pure data-normalization concern with no adapter-layer coupling. Splitting it out lets reviewers focus on the wire-format edge cases (tested across 7 cases including TypeError recovery, garbage
model_dump, None-name guards, and explicit-None-args) without wading through the adapter refactor.Tests
tests/adapters/test_tool.py— 36 tests pass, including:MockValSerrecovery (with and without recoverable attrs)model_dumpreturns (None, list, dict-with-no-name)Nonefornameandargstests/adapters/tests pass against this branch in isolation.