fix(provider): fall back when upstream omits tool_call id - #9681
Open
LIKIQ wants to merge 1 commit into
Open
Conversation
Some OpenAI-compatible upstreams (Gemini's compatible endpoint, several proxy gateways) return tool_calls without an id. The openai SDK deserializes responses leniently, so the required field silently becomes None instead of raising. Streaming responses can additionally grow a ghost tool_call carrying a null id when the upstream numbers tool_call.index from 1, because the SDK uses that index as a list subscript and inserts a second, argument-only entry. Either way the whole agent turn died with `ValidationError: 1 validation error for ToolCall` while assembling the assistant message, i.e. after the tool had already been executed, so users saw neither the tool result nor an answer. - normalize streaming tool_call indexes to a contiguous 0-based sequence, which also stops the real arguments from being accumulated onto a ghost entry - fall back to a deterministic placeholder id when the upstream omits one - normalize ids in the agent runner as well, so the executed tool result and the assistant tool call always reference the same id as the OpenAI spec requires (only patching the serializer would orphan the tool message) - make LLMResponse.to_openai_tool_calls* index-safe and null-safe, since FunctionBody.name raises the very same error when it is None refs AstrBotDevs#9590
dosubot
Bot
added
size:L
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/test_openai_source.py" line_range="2319" />
<code_context>
+@pytest.mark.asyncio
+async def test_query_stream_falls_back_when_tool_call_id_missing(monkeypatch):
</code_context>
<issue_to_address>
**suggestion (testing):** Cover the case where the upstream explicitly returns `"id": null` in streaming tool calls.
This test only exercises the case where `id` is omitted. Since the OpenAI SDK deserializes `"id": null` to `None` in streaming, please also add a variant where `tool_call_delta` includes `"id": None` for the same index, and assert that `tools_call_ids` and `ToolCall.id` still use the deterministic `call_0` placeholder.
</issue_to_address>
### Comment 2
<location path="tests/test_tool_loop_agent_runner.py" line_range="2095-2104" />
<code_context>
+ yield response
+
+
+def test_sanitize_malformed_tool_calls_fills_missing_ids(runner):
+ """缺失/空白的 tool_call id 必须被回退成确定性占位 id。"""
+ resp = LLMResponse(
+ role="tool",
+ completion_text="",
+ tools_call_name=["tool_a", "tool_b"],
+ tools_call_args=[{}, {}],
+ tools_call_ids=[None, " "], # type: ignore[list-item]
+ )
+
+ runner._sanitize_malformed_tool_calls(resp)
+
+ assert resp.tools_call_ids == ["call_0", "call_1"]
+ # 关键：不再抛 ValidationError
+ assert [tc.id for tc in resp.to_openai_tool_calls_model()] == ["call_0", "call_1"]
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for malformed tool names and `extra_content` preservation when ids are normalized.
The current tests around `_sanitize_malformed_tool_calls` cover ids well but don’t exercise name normalization or `extra_content` remapping. Please add: (1) a test where `tools_call_name` includes `None` or whitespace-only entries, asserting `ToolCall.FunctionBody.name` is set to `MALFORMED_TOOL_NAME_PLACEHOLDER` and no validation error is raised; and (2) a test where `tools_call_extra_content` is keyed by a malformed id, verifying that after normalization the content is still accessible under the new id and that `to_openai_tool_calls_model()` exposes it correctly.
Suggested implementation:
```python
def get_current_key(self) -> str:
return "test_key"
def test_sanitize_malformed_tool_calls_fills_missing_ids(runner):
"""缺失/空白的 tool_call id 必须被回退成确定性占位 id。"""
resp = LLMResponse(
role="tool",
completion_text="",
tools_call_name=["tool_a", "tool_b"],
tools_call_args=[{}, {}],
tools_call_ids=[None, " "], # type: ignore[list-item]
)
runner._sanitize_malformed_tool_calls(resp)
assert resp.tools_call_ids == ["call_0", "call_1"]
# 关键：不再抛 ValidationError
assert [tc.id for tc in resp.to_openai_tool_calls_model()] == ["call_0", "call_1"]
def test_sanitize_malformed_tool_calls_normalizes_tool_names(runner):
"""非法的 tool_call name（None/空白）应被回退成占位名称，并且不抛 ValidationError。"""
resp = LLMResponse(
role="tool",
completion_text="",
tools_call_name=[None, " "], # type: ignore[list-item]
tools_call_args=[{}, {}],
tools_call_ids=["call_0", "call_1"],
)
runner._sanitize_malformed_tool_calls(resp)
tool_calls = resp.to_openai_tool_calls_model()
# 名称被统一回退成 MALFORMED_TOOL_NAME_PLACEHOLDER
assert [tc.function.name for tc in tool_calls] == [
MALFORMED_TOOL_NAME_PLACEHOLDER,
MALFORMED_TOOL_NAME_PLACEHOLDER,
]
def test_sanitize_malformed_tool_calls_preserves_extra_content_on_id_normalization(runner):
"""当 tool_call.id 被归一化时，extra_content 需要正确地从旧 id 重映射到新 id。"""
# extra_content 以非法 id 作为 key
extra_content = {
" ": {"debug": "payload-for-whitespace-id"},
}
resp = LLMResponse(
role="tool",
completion_text="",
tools_call_name=["tool_a"],
tools_call_args=[{}],
tools_call_ids=[" "], # type: ignore[list-item]
tools_call_extra_content=extra_content,
)
runner._sanitize_malformed_tool_calls(resp)
# id 应该被归一化，同时 extra_content 重映射到新的 id
assert resp.tools_call_ids == ["call_0"]
assert " " not in resp.tools_call_extra_content
assert resp.tools_call_extra_content["call_0"] == {"debug": "payload-for-whitespace-id"}
tool_calls = resp.to_openai_tool_calls_model()
assert [tc.id for tc in tool_calls] == ["call_0"]
# OpenAI 模型里也应暴露同样的 extra_content
assert tool_calls[0].extra_content == {"debug": "payload-for-whitespace-id"}
```
1. 确保 `tests/test_tool_loop_agent_runner.py` 顶部已经从正确的模块导入 `LLMResponse` 和 `MALFORMED_TOOL_NAME_PLACEHOLDER`；如果尚未导入，需要增加类似：
`from <your_module> import LLMResponse, MALFORMED_TOOL_NAME_PLACEHOLDER`。
2. 如果 `to_openai_tool_calls_model()` 返回的对象字段名与示例不同（例如不是 `function.name` 或不是 `extra_content`），请根据实际模型结构调整断言访问路径。
3. 如果已有对 `_sanitize_malformed_tool_calls` 的其他测试分组或命名约定（比如使用类封装或不同前缀），可以将新测试函数移动到对应分组以保持一致性。
</issue_to_address>
### Comment 3
<location path="astrbot/core/provider/sources/openai_source.py" line_range="645" />
<code_context>
llm_response = LLMResponse("assistant", is_chunk=True)
state = ChatCompletionStreamState()
+ # 上游返回的 tool_call.index 可能从 1 开始、乱序或缺失，而 openai SDK 直接把它
+ # 当成 tool_calls 列表的下标使用（_build_events / accumulate_delta），一旦错位就会
+ # insert 出一个只有 arguments、没有 id / name 的幽灵 tool_call（refs: AstrBot#9590）。
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting tool_call index normalization and ID fallback into dedicated helper methods to keep the hot-path streaming and parsing logic focused and easier to read.
The new logic is functionally solid, but both index normalization and ID fallback are tightly inlined into hot paths. You can reduce cognitive load by pushing these concerns into small helpers that encapsulate state, while keeping behavior identical.
### 1. Extract tool_call index normalization into a helper
Instead of mutating `tool_call_index_map` inline inside the streaming loop, encapsulate the mapping in a tiny helper or state object:
```python
# near ChatCompletionStreamState
class ToolCallIndexNormalizer:
def __init__(self) -> None:
# raw_index -> normalized_index
self._map: dict[int, int] = {}
def normalize(self, raw_index: int) -> int:
if raw_index not in self._map:
self._map[raw_index] = len(self._map)
normalized_index = self._map[raw_index]
if normalized_index != raw_index:
logger.debug(
f"normalize tool_call index {raw_index} -> {normalized_index}"
)
return normalized_index
```
Then `_query_stream` becomes:
```python
llm_response = LLMResponse("assistant", is_chunk=True)
state = ChatCompletionStreamState()
index_normalizer = ToolCallIndexNormalizer()
async for chunk in stream:
choice = chunk.choices[0] if chunk.choices else None
delta = choice.delta if choice else None
if delta and (dtcs := delta.tool_calls):
for idx, tc in enumerate(dtcs):
if tc.function and tc.function.arguments:
tc.type = "function"
raw_index = getattr(tc, "index", None)
if raw_index is None:
raw_index = idx
tc.index = index_normalizer.normalize(raw_index)
...
```
This keeps the streaming loop focused on “what” is happening, and hides the “how” of normalization.
### 2. Centralize tool_call id normalization
You’ve added `fallback_tool_call_id`, and the reviewer notes `LLMResponse._safe_tool_call_id` already exists. Right now, `_parse_openai_completion` has its own ID normalization; you can delegate ID normalization to `LLMResponse` and avoid duplicating responsibility.
For example, keep `_parse_openai_completion` collecting raw IDs (including `None`), and let `LLMResponse` normalize when constructing the final response:
```python
# in _parse_openai_completion
raw_call_id = getattr(tool_call, "id", None)
tool_call_ids.append(raw_call_id)
extra_content = getattr(tool_call, "extra_content", None)
if extra_content is not None:
# temporarily key by raw id; will be remapped by LLMResponse
tool_call_extra_content_dict[raw_call_id] = extra_content
```
Then in `LLMResponse`:
```python
class LLMResponse:
...
def _safe_tool_call_id(self, raw_id: str | None, index: int) -> str:
if isinstance(raw_id, str) and raw_id.strip():
return raw_id
return fallback_tool_call_id(index)
def finalize_tool_calls(self) -> None:
if not self.tools_call_ids:
return
normalized_ids: list[str] = []
normalized_extra: dict[str, Any] = {}
for idx, raw_id in enumerate(self.tools_call_ids):
call_id = self._safe_tool_call_id(raw_id, idx)
normalized_ids.append(call_id)
extra = self.tools_call_extra_content.get(raw_id)
if extra is not None:
normalized_extra[call_id] = extra
self.tools_call_ids = normalized_ids
self.tools_call_extra_content = normalized_extra
```
Call `llm_response.finalize_tool_calls()` at the end of `_parse_openai_completion`. This keeps all ID fallback logic (including alignment with list index and extra_content mapping) in one place, and removes the need for inline `getattr` + warning + `fallback_tool_call_id` in `_parse_openai_completion`, while preserving the current behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| final_response = responses[-1] | ||
| assert final_response.tools_call_ids == ["call_0"] | ||
| assert final_response.tools_call_args == [{"city": "上海"}] | ||
| assert final_response.to_openai_tool_calls_model()[0].id == "call_0" |
Contributor
There was a problem hiding this comment.
suggestion (testing): Cover the case where the upstream explicitly returns "id": null in streaming tool calls.
This test only exercises the case where id is omitted. Since the OpenAI SDK deserializes "id": null to None in streaming, please also add a variant where tool_call_delta includes "id": None for the same index, and assert that tools_call_ids and ToolCall.id still use the deterministic call_0 placeholder.
Comment on lines
+2095
to
+2104
| def test_sanitize_malformed_tool_calls_fills_missing_ids(runner): | ||
| """缺失/空白的 tool_call id 必须被回退成确定性占位 id。""" | ||
| resp = LLMResponse( | ||
| role="tool", | ||
| completion_text="", | ||
| tools_call_name=["tool_a", "tool_b"], | ||
| tools_call_args=[{}, {}], | ||
| tools_call_ids=[None, " "], # type: ignore[list-item] | ||
| ) | ||
|
|
Contributor
There was a problem hiding this comment.
suggestion (testing): Add tests for malformed tool names and extra_content preservation when ids are normalized.
The current tests around _sanitize_malformed_tool_calls cover ids well but don’t exercise name normalization or extra_content remapping. Please add: (1) a test where tools_call_name includes None or whitespace-only entries, asserting ToolCall.FunctionBody.name is set to MALFORMED_TOOL_NAME_PLACEHOLDER and no validation error is raised; and (2) a test where tools_call_extra_content is keyed by a malformed id, verifying that after normalization the content is still accessible under the new id and that to_openai_tool_calls_model() exposes it correctly.
Suggested implementation:
def get_current_key(self) -> str:
return "test_key"
def test_sanitize_malformed_tool_calls_fills_missing_ids(runner):
"""缺失/空白的 tool_call id 必须被回退成确定性占位 id。"""
resp = LLMResponse(
role="tool",
completion_text="",
tools_call_name=["tool_a", "tool_b"],
tools_call_args=[{}, {}],
tools_call_ids=[None, " "], # type: ignore[list-item]
)
runner._sanitize_malformed_tool_calls(resp)
assert resp.tools_call_ids == ["call_0", "call_1"]
# 关键：不再抛 ValidationError
assert [tc.id for tc in resp.to_openai_tool_calls_model()] == ["call_0", "call_1"]
def test_sanitize_malformed_tool_calls_normalizes_tool_names(runner):
"""非法的 tool_call name（None/空白）应被回退成占位名称，并且不抛 ValidationError。"""
resp = LLMResponse(
role="tool",
completion_text="",
tools_call_name=[None, " "], # type: ignore[list-item]
tools_call_args=[{}, {}],
tools_call_ids=["call_0", "call_1"],
)
runner._sanitize_malformed_tool_calls(resp)
tool_calls = resp.to_openai_tool_calls_model()
# 名称被统一回退成 MALFORMED_TOOL_NAME_PLACEHOLDER
assert [tc.function.name for tc in tool_calls] == [
MALFORMED_TOOL_NAME_PLACEHOLDER,
MALFORMED_TOOL_NAME_PLACEHOLDER,
]
def test_sanitize_malformed_tool_calls_preserves_extra_content_on_id_normalization(runner):
"""当 tool_call.id 被归一化时，extra_content 需要正确地从旧 id 重映射到新 id。"""
# extra_content 以非法 id 作为 key
extra_content = {
" ": {"debug": "payload-for-whitespace-id"},
}
resp = LLMResponse(
role="tool",
completion_text="",
tools_call_name=["tool_a"],
tools_call_args=[{}],
tools_call_ids=[" "], # type: ignore[list-item]
tools_call_extra_content=extra_content,
)
runner._sanitize_malformed_tool_calls(resp)
# id 应该被归一化，同时 extra_content 重映射到新的 id
assert resp.tools_call_ids == ["call_0"]
assert " " not in resp.tools_call_extra_content
assert resp.tools_call_extra_content["call_0"] == {"debug": "payload-for-whitespace-id"}
tool_calls = resp.to_openai_tool_calls_model()
assert [tc.id for tc in tool_calls] == ["call_0"]
# OpenAI 模型里也应暴露同样的 extra_content
assert tool_calls[0].extra_content == {"debug": "payload-for-whitespace-id"}- 确保
tests/test_tool_loop_agent_runner.py顶部已经从正确的模块导入LLMResponse和MALFORMED_TOOL_NAME_PLACEHOLDER；如果尚未导入，需要增加类似：
from <your_module> import LLMResponse, MALFORMED_TOOL_NAME_PLACEHOLDER。 - 如果
to_openai_tool_calls_model()返回的对象字段名与示例不同（例如不是function.name或不是extra_content），请根据实际模型结构调整断言访问路径。 - 如果已有对
_sanitize_malformed_tool_calls的其他测试分组或命名约定（比如使用类封装或不同前缀），可以将新测试函数移动到对应分组以保持一致性。
| llm_response = LLMResponse("assistant", is_chunk=True) | ||
|
|
||
| state = ChatCompletionStreamState() | ||
| # 上游返回的 tool_call.index 可能从 1 开始、乱序或缺失，而 openai SDK 直接把它 |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider extracting tool_call index normalization and ID fallback into dedicated helper methods to keep the hot-path streaming and parsing logic focused and easier to read.
The new logic is functionally solid, but both index normalization and ID fallback are tightly inlined into hot paths. You can reduce cognitive load by pushing these concerns into small helpers that encapsulate state, while keeping behavior identical.
1. Extract tool_call index normalization into a helper
Instead of mutating tool_call_index_map inline inside the streaming loop, encapsulate the mapping in a tiny helper or state object:
# near ChatCompletionStreamState
class ToolCallIndexNormalizer:
def __init__(self) -> None:
# raw_index -> normalized_index
self._map: dict[int, int] = {}
def normalize(self, raw_index: int) -> int:
if raw_index not in self._map:
self._map[raw_index] = len(self._map)
normalized_index = self._map[raw_index]
if normalized_index != raw_index:
logger.debug(
f"normalize tool_call index {raw_index} -> {normalized_index}"
)
return normalized_indexThen _query_stream becomes:
llm_response = LLMResponse("assistant", is_chunk=True)
state = ChatCompletionStreamState()
index_normalizer = ToolCallIndexNormalizer()
async for chunk in stream:
choice = chunk.choices[0] if chunk.choices else None
delta = choice.delta if choice else None
if delta and (dtcs := delta.tool_calls):
for idx, tc in enumerate(dtcs):
if tc.function and tc.function.arguments:
tc.type = "function"
raw_index = getattr(tc, "index", None)
if raw_index is None:
raw_index = idx
tc.index = index_normalizer.normalize(raw_index)
...This keeps the streaming loop focused on “what” is happening, and hides the “how” of normalization.
2. Centralize tool_call id normalization
You’ve added fallback_tool_call_id, and the reviewer notes LLMResponse._safe_tool_call_id already exists. Right now, _parse_openai_completion has its own ID normalization; you can delegate ID normalization to LLMResponse and avoid duplicating responsibility.
For example, keep _parse_openai_completion collecting raw IDs (including None), and let LLMResponse normalize when constructing the final response:
# in _parse_openai_completion
raw_call_id = getattr(tool_call, "id", None)
tool_call_ids.append(raw_call_id)
extra_content = getattr(tool_call, "extra_content", None)
if extra_content is not None:
# temporarily key by raw id; will be remapped by LLMResponse
tool_call_extra_content_dict[raw_call_id] = extra_contentThen in LLMResponse:
class LLMResponse:
...
def _safe_tool_call_id(self, raw_id: str | None, index: int) -> str:
if isinstance(raw_id, str) and raw_id.strip():
return raw_id
return fallback_tool_call_id(index)
def finalize_tool_calls(self) -> None:
if not self.tools_call_ids:
return
normalized_ids: list[str] = []
normalized_extra: dict[str, Any] = {}
for idx, raw_id in enumerate(self.tools_call_ids):
call_id = self._safe_tool_call_id(raw_id, idx)
normalized_ids.append(call_id)
extra = self.tools_call_extra_content.get(raw_id)
if extra is not None:
normalized_extra[call_id] = extra
self.tools_call_ids = normalized_ids
self.tools_call_extra_content = normalized_extraCall llm_response.finalize_tool_calls() at the end of _parse_openai_completion. This keeps all ID fallback logic (including alignment with list index and extra_content mapping) in one place, and removes the need for inline getattr + warning + fallback_tool_call_id in _parse_openai_completion, while preserving the current behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the crash reported in #9590. Whenever the upstream returns a
tool_callwithout anid, the whole agent turn dies withValidationError: 1 validation error for ToolCall— and it dies after the tool has already been executed, so the user gets neither the tool result nor an answer. This is easy to hit with MCP tools.修复 #9590 报告的崩溃。只要上游返回的
tool_call缺少id，整轮 agent 就会以ValidationError: 1 validation error for ToolCall失败；而且失败发生在工具已经执行之后，用户既拿不到工具结果也拿不到回答。调用 MCP 工具时很容易触发。Two independent triggers / 两条独立的触发路径:
BaseModel.construct→field_get_default,PydanticUndefined → None), so the requiredidfield silently becomesNoneinstead of raising. Verified with openai 2.14.0 / pydantic 2.13.3: non-streaming missing id →None, non-streaming"id": null→None, final streaming snapshot →None.tool_call.indexoffset — when an upstream numberstool_call.indexfrom 1, the SDK uses that index as a list subscript:_build_eventsraisesIndexError(swallowed by us asSaving chunk state error), thenaccumulate_deltadoesacc_value.insert(index, delta_entry)and fabricates a second, argument-only entry with no id and no name. The existingtc.type = "function"workaround then promotes that ghost fragment to a function call, so it passes the downstreamtype == "function"check. The real arguments also end up on the ghost entry, which is why解析参数失败shows up in the same logs. The previous fix (Streaming tool_call arguments lost when OpenAI-compatible proxy omitsindexfield (e.g. Gemini) #6661) only handled a missing index, not an offset one.Modifications / 改动点
astrbot/core/provider/sources/openai_source.py_query_stream: keep a per-stream index map and remap upstreamtool_call.indexto a contiguous 0-based sequence before handing chunks to the SDK. Normal upstreams are an identity mapping. This removes the ghost entry and restores the lost arguments._parse_openai_completion: fall back to a deterministic placeholder id when the upstream omits one (with a warning), and keyextra_contentby the effective id.astrbot/core/agent/runners/tool_loop_agent_runner.py_sanitize_malformed_tool_callsnow normalizes ids as well as names: pads the id list sozip()can't silently drop a tool, falls back for empty/Noneids, and de-duplicates if a placeholder would collide with a real id.astrbot/core/provider/entities.pyfallback_tool_call_id()/MALFORMED_TOOL_NAME_PLACEHOLDERshared by the provider and the runner, so both derive the same placeholder.to_openai_tool_calls_model()/to_openai_tool_calls()are now index-safe and null-safe (FunctionBody.nameis a requiredstrtoo, so aNonename raises the exact same error).tests/test_openai_source.py,tests/test_tool_loop_agent_runner.py: 8 regression cases.Why the runner also needs the fallback (and not just the serializer, as #9593 does): patching only
entities.pymakes the assistant message carrytool_calls[].id = "call_0"while therole="tool"result message still hastool_call_id = None(Message.serialize()popsNonefields). The two no longer pair up, so the tool message is either dropped as an orphan by_sanitize_assistant_messages()— after which strict upstreams return 400 because an assistant message withtool_callsis not followed by matching tool messages — or rejected outright. Normalizing at theLLMResponse.tools_call_idssource keeps tool execution and message assembly on the same id, which the OpenAI spec requires.Screenshots or Test Results / 运行截图或测试结果
The 8 new regression cases (3 in
test_openai_source.py, 5 intest_tool_loop_agent_runner.py):They do fail without this patch —
git stash-ing the three source files and re-running gives 7 failures, one of which reproduces the reported crash verbatim atastrbot/core/provider/entities.py:431:Affected modules, full run:
The 3 failures are
test_file_uri_to_path_preserves_*(Windows path normalization); they fail identically on unpatchedmasterin the same environment, i.e. they are pre-existing and unrelated. Whole suite: 2131 passed / 31 failed, all 31 pre-existing Windows-environment failures (verified against unpatched sources).Checklist / 检查清单
Summary by Sourcery
Prevent agent crashes and orphaned tool results when upstream OpenAI-compatible providers omit or misindex tool call identifiers.
Bug Fixes:
Enhancements:
Tests: