Skip to content

refactor(tool-call): normalize wire shapes at LiteLLM boundary - #47

Open
isaacbmiller wants to merge 14 commits into
mainfrom
isaac/canonical-tool-call
Open

refactor(tool-call): normalize wire shapes at LiteLLM boundary#47
isaacbmiller wants to merge 14 commits into
mainfrom
isaac/canonical-tool-call

Conversation

@isaacbmiller

Copy link
Copy Markdown

Why

Tool-call wire-shape coercion was scattered across tool.py, base.py, two_step_adapter.py, and inspect_history.py. Every layer sniffed shapes (v['function']['name'] vs v['name'] vs output_item.model_dump() vs pydantic attribute access), which led to cascading fallbacks that drifted apart, parts of the pipeline that crashed when one provider's shape leaked into another's branch, and a recurring crash from the pydantic v2 MockValSer / SchemaSerializer bug (pydantic/pydantic#7713, BerriAI/litellm#9345) that needs to be worked around in exactly one spot, not five.

What

  • New to_tool_call(item) in dspy/adapters/types/tool.py is the only function that touches wire shapes. It accepts:
    • OpenAI Chat Completions pydantic / dict ({type:'function', function:{name, arguments}, id})
    • OpenAI Responses API pydantic / dict ({type:'function_call', name, arguments, call_id})
    • falls back to attribute access if model_dump() raises TypeError from the MockValSer bug
  • ToolCalls.ToolCall gains an optional id: str | None field so we can round-trip provider call IDs.
  • BaseLM normalizes once at the two LiteLLM exit points (_process_lm_response_choices line 270 and _process_response line 322). After this, output['tool_calls'] is always list[ToolCall].
  • Consumers collapse to one-liners:
    • Adapter._call_postprocess and TwoStepAdapter postprocess: 8-line dict comprehensions → ToolCalls(tool_calls=list(tool_calls))
    • inspect_history: tc['function']['name']tc.name
    • ToolCalls.validate_input: 27 lines → 14 lines, now only handles serialization round-trips (its actual job), not provider wire shapes
  • json_repair no longer imported in base.py or two_step_adapter.py; it lives where shape coercion lives (tool.py).

Why now

The recurring MockValSer crash isn't fixable upstream: pydantic#7713 has been open since October 2023 (missed the v2.12 milestone, October 2025) and litellm#9345 was closed as not_planned. The getattr fallback is the standard workaround, but it had to live in tool.py AND in adapter postprocess AND in inspect_history. Putting it in one place lets us delete the others.

Verification

  • tests/adapters/test_tool_call_normalization.py (new): 12 boundary tests pinning each wire shape, MockValSer fallback, error reporting, and ToolCall.id round-trip.
  • tests/adapters/: 265 passed, 9 skipped.
  • tests/clients/ + tests/predict/ + tests/signatures/ + tests/callback/ + tests/reliability/test_pydantic_models.py: 604 passed, 96 skipped, 2 xfailed.
  • 3 pre-existing assertions in test_chat_adapter.py / test_json_adapter.py updated to expect ToolCall.id (now captured from the mocked LiteLLM response).
  • test_tool_call_with_null_content_does_not_raise updated to feed the canonical shape; raw OpenAI dicts are no longer a valid input to _call_postprocess because BaseLM normalizes upstream.

Supersedes #43.

Adds `to_tool_call(item)` in `dspy/adapters/types/tool.py` as the single
boundary that converts OpenAI Chat Completions and Responses API shapes
(pydantic or dict) into a canonical `ToolCalls.ToolCall` with optional
`id` field. `BaseLM` normalizes once at both extraction points so no
downstream adapter, postprocess step, or history renderer ever sees a
raw wire shape; consumers in `Adapter`, `TwoStepAdapter`, and
`inspect_history` collapse to one-liners.

Falls back to attribute access when `model_dump()` raises `TypeError`
from the long-standing pydantic v2 MockValSer / SchemaSerializer bug
(see pydantic/pydantic#7713 and BerriAI/litellm#9345). The fallback
is contained to `to_tool_call` only.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
@greptile-apps

greptile-apps Bot commented May 12, 2026

Copy link
Copy Markdown

Greptile Summary

This PR consolidates all LiteLLM tool-call wire-shape coercion into a single inbound boundary (to_tool_call in tool.py) and two symmetric outbound methods (format_as_litellm_tool_definition / format_as_litellm_tool_call), eliminating the scattered sniffing logic that caused cascading failures and the recurring MockValSer pydantic bug to surface in multiple places.

  • Boundary normalization: BaseLM now calls to_tool_call at both exit points before any downstream code sees tool calls, so output['tool_calls'] is always list[ToolCall] after the LiteLLM boundary; _process_response also now always emits a text key (possibly None), removing the KeyError that previously fired for tool-call-only Responses API responses.
  • Outbound serialization: ToolCall.format_as_litellm_tool_call correctly emits arguments as a JSON-encoded string (not a dict) and conditionally includes id/call_id, fixing the silent data-loss in the old format() method.
  • Test coverage: 12 new boundary tests pin each wire shape, MockValSer fallback, wrong-dialect rejection, and id round-trip fidelity; existing adapter tests updated to reflect the canonical ToolCall objects instead of raw OpenAI dicts.

Confidence Score: 5/5

The change is safe to merge: the single inbound normalizer and its outbound counterparts are well-tested, the MockValSer fallback is isolated, and the previously crashing KeyError in inspect_history is now structurally prevented.

The refactor eliminates a documented class of recurring crashes and replaces fragmented wire-shape sniffing with a single, tested boundary. The one minor inconsistency — format_as_litellm_tool_definition raising for non-standard model_type while the response-processing path tolerates them — only affects custom BaseLM subclasses that explicitly set a non-standard model_type AND override supports_function_calling to True, an extremely narrow combination.

No files require special attention. The normalizer logic in dspy/adapters/types/tool.py is the most complex addition but is thoroughly covered by the new boundary tests.

Important Files Changed

Filename Overview
dspy/adapters/types/tool.py Major refactor: adds to_tool_call inbound normalizer, format_as_litellm_tool_definition/format_as_litellm_tool_call outbound serializers with two-dialect contract, ToolCall.id field, and removes from_dict_list. Core logic is solid; minor inconsistency where format_as_litellm_tool_definition raises for non-standard model_type but the response-processing path tolerates it silently.
dspy/clients/base_lm.py Normalizes tool calls at both LiteLLM exit points via to_tool_call; _process_response now always emits text (even as None), fixing the downstream KeyError for tool-call-only Responses API outputs.
dspy/utils/inspect_history.py Updated to access tool_call.name/tool_call.args directly on ToolCall objects; text-is-always-present contract from base_lm.py removes the KeyError risk on line 81.
dspy/adapters/base.py Removes 8-line dict comprehension that sniffed wire shapes in favour of ToolCalls(tool_calls=list(tool_calls)); forwards model_type to format_as_litellm_tool_definition.
dspy/adapters/two_step_adapter.py Identical simplification to base.py: removes 8-line dict comprehension and json_repair import.
tests/adapters/test_tool.py Adds 12 new boundary tests covering both wire shapes, MockValSer fallback, wrong-dialect rejection, id round-trips, and argument-as-JSON-string enforcement.
dspy/adapters/types/base_type.py Moves BaseLM import under TYPE_CHECKING to break a circular import; no logic changes.

Sequence Diagram

sequenceDiagram
    participant Adapter as Adapter._call_preprocess
    participant BaseLM as BaseLM
    participant LiteLLM as LiteLLM
    participant Normalizer as to_tool_call (tool.py)
    participant Post as Adapter._call_postprocess

    Adapter->>BaseLM: "lm(messages, tools=[format_as_litellm_tool_definition(model_type)])"
    BaseLM->>LiteLLM: litellm.completion / litellm.responses
    LiteLLM-->>BaseLM: raw wire shape (Chat or Responses API)
    BaseLM->>Normalizer: to_tool_call(tc, model_type)
    Note over Normalizer: handles dict, pydantic, MockValSer fallback
    Normalizer-->>BaseLM: ToolCalls.ToolCall(name, args, id)
    BaseLM-->>Adapter: "output[tool_calls] = list[ToolCall]"
    Adapter->>Post: "ToolCalls(tool_calls=list(tool_calls))"
    Post-->>Adapter: "value[field] = ToolCalls instance"
Loading

Reviews (13): Last reviewed commit: "docs(tool-call): document the wire-shape..." | Re-trigger Greptile

Comment thread dspy/adapters/types/tool.py
isaacbmiller and others added 5 commits May 12, 2026 15:26
`base_type.py` only used `BaseLM` as a parameter annotation on
`adapt_to_native_lm_feature`, so it didn't need a runtime import.
Moving it under `TYPE_CHECKING` breaks the
`base_lm → tool → base_type → base_lm` cycle, letting `base_lm.py`
import `to_tool_call` at module level instead of lazily inside two
hot paths.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Moves the canonical `ToolCall` data type and the inbound `to_tool_call`
boundary into a new `dspy/clients/tool_call.py`. The dependency now
flows `adapters → clients`, which is the correct direction; `base_lm.py`
imports from a same-layer sibling instead of reaching up into the
adapter layer (and no lazy imports / TYPE_CHECKING hacks needed).

`ToolCalls` keeps the same public API: `dspy.ToolCalls` is still the
adapter-layer `Type` used in signatures, and `ToolCalls.ToolCall`
remains a usable handle via a `ClassVar` alias for the clients-layer
type.

Adds outbound symmetry: `Tool.format_as_litellm_function_call(model_type)`
now emits the flattened Responses-API shape
(`{type:'function', name, description, parameters}`) when
`model_type='responses'` and the wrapped Chat-Completions shape
otherwise. `adapters/base.py` threads `lm.model_type` in.

Postprocess fix: both `Adapter._call_postprocess` and `TwoStepAdapter`
now use `output.get('text')` instead of `output['text']`. The Responses
API path omits the `text` key entirely when an output is tool-calls-only,
so the previous direct subscript crashed with KeyError.

Verified end-to-end against live gpt-5-nano on both `model_type='chat'`
and `model_type='responses'` paths through `ChatAdapter(use_native_function_calling=True)`.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
- Re-export `ToolCall` at the standard public locations: `dspy.ToolCall`,
  `dspy.adapters.ToolCall`, `dspy.adapters.types.ToolCall`. Backward-compat
  access via `dspy.ToolCalls.ToolCall` keeps working — the ClassVar alias
  resolves to the same class object, so `isinstance` checks across either
  path are equivalent.
- Restore the top-level `from dspy.clients.base_lm import BaseLM` in
  `adapters/types/base_type.py`. The cycle dissolved when `base_lm.py`
  stopped importing from `adapters/types/tool.py` (it now imports from
  the leaf module `clients/tool_call.py`), so the `TYPE_CHECKING`
  workaround is no longer needed.
- Delete `ToolCalls.from_dict_list` and its test. The method was a thin
  wrapper around the canonical `ToolCalls(tool_calls=[ToolCall(**d) ...])`
  constructor and its only remaining caller was a test of itself; its
  docstring also referenced a parameter name (`dict_list`) that didn't
  match the signature. Removed the matching API doc entry.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
`ToolCall.format()` dropped `self.id`, which meant
`to_tool_call(tc.format())` lost the provider call-id we just added the
field for. Include `id` in the payload when set; omit the key when
absent to avoid fabricating one on the wire.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…es/tool.py

Moves ToolCall and to_tool_call back into dspy/adapters/types/tool.py
so the three tool-call types (Tool, ToolCall, ToolCalls) live in one
file. base_lm.py imports to_tool_call lazily inside its two hot paths
to avoid the base_lm → adapters/tool → base_type → base_lm import
cycle; base_type.py keeps BaseLM under TYPE_CHECKING for the same
reason.

Adds a TODO(MaximeRivest) marker on ToolCall and at the lazy-import
sites flagging that this interface should move to ToolPart, which
will let us collapse the lazy imports.

Public surface is unchanged: dspy.ToolCall, dspy.ToolCalls.ToolCall,
and dspy.adapters.{ToolCall,types.ToolCall} all resolve to the same
class object.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Comment thread dspy/adapters/types/tool.py Outdated
isaacbmiller and others added 8 commits May 12, 2026 16:19
…l alias

ToolCall is now defined inline at the top of the ToolCalls class body —
one class definition, adjacent to the container it belongs to. No
separate module-level class, no module-level `ToolCall = ToolCalls.ToolCall`
alias, no public re-exports at `dspy` / `dspy.adapters` /
`dspy.adapters.types`. The canonical access path is now
`dspy.ToolCalls.ToolCall`.

`to_tool_call` stays a module-level function (it's a boundary, not a
constructor) and binds `ToolCall = ToolCalls.ToolCall` locally for
readability.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…re execute docstring

- Dropped the local `ToolCall = ToolCalls.ToolCall` rebind in
  `to_tool_call` — the rebind was just noise. Refer to
  `ToolCalls.ToolCall` directly.
- Restored the original `execute` docstring (Args/Returns/Raises) and
  the precise `dict[str, Any] | list[Tool] | None` type annotation that
  predated this PR.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Tool, ToolCalls, and to_tool_call all live in dspy/adapters/types/tool.py
so their tests live in a single tests/adapters/test_tool.py file.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
The boundary in dspy/adapters/types/tool.py::to_tool_call now normalizes
both Chat Completions and Responses API tool-call wire shapes into a
canonical ToolCalls.ToolCall before they leave the LM layer. Update the
test's expected value to assert the canonical shape instead of the raw
Responses-API dict, which was the very thing the boundary was added to
eliminate.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…n inspect_history

Two issues flagged in prior review passes:

1. `ToolCalls.ToolCall.format()` serialized `function.arguments` as a
   Python dict. OpenAI Chat Completions requires it to be a JSON-encoded
   string when the payload is replayed as an assistant tool-call message
   (e.g. `{"role":"assistant","tool_calls":[...]}`). Switched to
   `json.dumps(self.args)`. `to_tool_call` already accepts both shapes
   on the way in, so the round-trip stays total — verified by the
   existing `test_toolcall_format_preserves_id_for_round_trip` test.

2. `pretty_print_history` did `outputs[0]["text"]`, which raises
   `KeyError` for Responses API outputs that contain only tool calls
   (no `text` key). Switched to `.get("text")`, matching the same
   fix applied in `adapters/base.py` earlier in this PR.

Added regression tests for both:

* `test_toolcall_format_arguments_is_json_string_for_openai_assistant_message`
* `test_toolcall_format_empty_args_is_json_object_string`
* `test_pretty_print_history_handles_tool_calls_only_output`

Updated the existing `TOOL_CALL_TEST_CASES` parametrization to expect
JSON-string arguments.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…s present

The previous fix in inspect_history.py and adapters/base.py used `.get("text")`
to paper over a real inconsistency: `_process_completion` always set
`output["text"]` (possibly to None) while `_process_response` only added
the key when there was message content. Downstream code shouldn't see that
variance — the whole point of the LM-layer boundary is to hand off one
canonical shape.

Fix the source: `_process_response` now always sets `result["text"]`,
to None when the Responses API returned no message blocks (e.g. tool-calls-only
outputs). With that contract in place, the defensive `.get("text")` calls
in adapters/base.py, two_step_adapter.py, and inspect_history.py are reverted
to direct key access.

Restored `ToolCall` to extend `Type` (it was downgraded to `pydantic.BaseModel`
when this PR moved it through the clients layer; that move is no longer in
effect and the downgrade has no justification).

Also propagates the new shape into test expectations:
- test_lm.py::test_responses_api_tool_calls
- test_inspect_global_history.py::test_pretty_print_history_handles_tool_calls_only_output

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
… sides

Tightens three conceptual gaps the previous shape left open:

1. Outbound symmetry. Added `ToolCalls.ToolCall.format_as_litellm_tool_call(model_type)`
   so a canonical tool call can be re-serialized to either the Chat
   Completions assistant wire shape (`type: function, function: {...}, id`)
   or the Responses API shape (`type: function_call, name, arguments, call_id`).
   Previously `format()` was hard-coded to chat — replaying a tool call into
   the Responses API in a multi-turn loop was impossible through the boundary.
   `format()` is kept as a thin alias that delegates to the chat dialect so
   this type still plugs into the existing `Type.serialize_model` machinery
   for prompt rendering.

2. Inbound declaration replaces positional sniffing. `to_tool_call` now
   requires `model_type`. Each caller passes what they know
   (`_process_completion` is always "chat"; `_process_response` is always
   "responses"). Wrong-dialect payloads raise a precise error instead of
   silently falling through. `_to_tool_call_chat` and `_to_tool_call_responses`
   split the two paths; only the MockValSer-pydantic-bug fallback is preserved
   (with its own scoped attribute-access branch per dialect).

3. Naming. Renamed `Tool.format_as_litellm_function_call` →
   `format_as_litellm_tool_definition`. The method serializes a tool
   *definition* for the `tools=` array, not a function *call*; the new name
   pairs cleanly with `ToolCall.format_as_litellm_tool_call` so the
   inbound/outbound boundary now reads as a matched set.

Six new symmetry tests in test_tool.py; existing callers in
`adapters/base.py` and `clients/base_lm.py` updated; doc reference updated.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…ry methods

The wire side stays `dict[str, Any]` / `str` — no type narrowing in the
signatures — but the contract is now spelled out on each boundary
method:

  Tool.format_as_litellm_tool_definition  (outbound, definitions)
  ToolCalls.ToolCall.format_as_litellm_tool_call  (outbound, calls)
  to_tool_call  (inbound, calls)

Each docstring lists the exact valid model_type values, the precise
key sets for each dialect, the JSON-string requirement on `arguments`,
which fields are conditionally emitted, and what raises. The three
methods cross-reference each other so the dialect set stays
discoverable in one place if it ever grows.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant