feat(tool): preserve native tool call ids - #53
Conversation
2f13e38 to
d9bc967
Compare
Greptile SummaryThis PR threads native provider tool-call IDs through the
Confidence Score: 4/5Safe to merge; all changed paths are covered by focused tests and the normalization logic handles all targeted wire shapes correctly. The normalization logic in
Important Files Changed
Sequence DiagramsequenceDiagram
participant LM as LM (provider)
participant Adapter as base.Adapter
participant TC as ToolCalls
participant Hist as HistoryFrame
LM->>Adapter: LMOutput with tool_calls list
Note over Adapter: output.tool_calls → [LMToolCallPart(id="call_1", ...)]
Adapter->>TC: "from_dict_list([{name, args, id:"call_1"}])"
Note over TC: validate_input → _normalize_native_tool_call
Note over TC: OpenAI / responses-API / DSPy format → {name, args, id}
TC-->>Adapter: "ToolCalls(tool_calls=[ToolCall(id="call_1", ...)])"
Adapter-->>Adapter: "value[tool_call_field] = ToolCalls(...)"
Note over Hist: HistoryFrame constructed with outputs dict
Adapter->>Hist: "HistoryFrame(outputs={...ToolCalls(...)})"
Note over Hist: _normalize_tool_calls_outputs validator
Note over Hist: passes through ToolCalls instances unchanged
Note over Hist: converts raw {"tool_calls":[...]} dicts → ToolCalls
Note over TC: Downstream rendering
TC->>TC: with_call_ids("prefix") fills missing IDs
TC->>TC: "to_lm_parts() → [LMToolCallPart(id="call_1", ...)]"
Reviews (1): Last reviewed commit: "feat(tool): preserve native tool call id..." | Re-trigger Greptile |
| def to_lm_part(self, tool_call_id: str | None = None) -> LMToolCallPart: | ||
| return LMToolCallPart(id=tool_call_id or self.id, name=self.name, args=self.args) |
There was a problem hiding this comment.
The
or operator makes tool_call_id or self.id treat any falsy value — including an empty string "" — the same as None, silently falling back to self.id. While an empty-string ID is unlikely in practice today, an explicit None guard is the conventional pattern for "use argument if explicitly provided, else fall back to default", and avoids the subtle footgun if a provider ever emits a zero-length ID token.
| def to_lm_part(self, tool_call_id: str | None = None) -> LMToolCallPart: | |
| return LMToolCallPart(id=tool_call_id or self.id, name=self.name, args=self.args) | |
| def to_lm_part(self, tool_call_id: str | None = None) -> LMToolCallPart: | |
| return LMToolCallPart(id=self.id if tool_call_id is None else tool_call_id, name=self.name, args=self.args) |
| normalized_outputs = {} | ||
| changed = False | ||
| for key, value in self.outputs.items(): | ||
| if isinstance(value, dict) and set(value.keys()) == {"tool_calls"} and isinstance(value["tool_calls"], list): |
There was a problem hiding this comment.
The condition
set(value.keys()) == {"tool_calls"} requires the serialized dict to contain only the "tool_calls" key. If ToolCalls serialization ever gains additional envelope fields (e.g. a version tag, metadata), or if a caller manually adds context alongside "tool_calls", this check silently falls through and the value is left as a raw dict rather than a ToolCalls instance. Consider documenting the intentional strictness or relaxing to a membership check to avoid a silent deserialization miss.
| if isinstance(value, dict) and set(value.keys()) == {"tool_calls"} and isinstance(value["tool_calls"], list): | |
| if isinstance(value, dict) and "tool_calls" in value and isinstance(value["tool_calls"], list) and len(value) == 1: |
Summary
ToolCallsToolCallsboundaryToolCallsto normalized LM tool-call partsToolCallsStack
Validation
uv run --extra dev pytest -q tests/adapters/test_history.py tests/adapters/test_tool.py -k "field_frames_round_trip or toolcalls_vague_match or tool_calls_preserve_call_ids or native_tool_response_preserves_call_ids"