Conversation
dosubot
Bot
added
size:S
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
_deduplicate_self_repeatinghelper only detects exactly-two-part repetitions (abcabc) and skips shorter values, so if other repetition patterns (e.g.abcabcabcorabab) might occur in streaming, consider either documenting this limitation or generalizing the detection logic to handle arbitrary repeated substrings. - When deduplicating
tool_call.id,Noneis converted to an empty string; if downstream logic distinguishes betweenNoneand a real (even if malformed) id, it may be safer to short-circuit and skip deduplication whenidis falsy instead of normalizing to"".
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_deduplicate_self_repeating` helper only detects exactly-two-part repetitions (`abcabc`) and skips shorter values, so if other repetition patterns (e.g. `abcabcabc` or `abab`) might occur in streaming, consider either documenting this limitation or generalizing the detection logic to handle arbitrary repeated substrings.
- When deduplicating `tool_call.id`, `None` is converted to an empty string; if downstream logic distinguishes between `None` and a real (even if malformed) id, it may be safer to short-circuit and skip deduplication when `id` is falsy instead of normalizing to `""`.
## Individual Comments
### Comment 1
<location path="astrbot/core/provider/sources/openai_source.py" line_range="883-884" />
<code_context>
+ func_name_ls.append(
+ cls._deduplicate_self_repeating(tool_call.function.name)
+ )
+ tool_call_ids.append(
+ cls._deduplicate_self_repeating(tool_call.id or "")
+ )
</code_context>
<issue_to_address>
**issue (bug_risk):** Converting `None` tool_call IDs to an empty string may change behavior and cause key collisions.
Previously, `tool_call_ids.append(tool_call.id)` preserved `None`, but `tool_call.id or ""` now turns `None` into `""` and uses that as the key in `tool_call_extra_content_dict`. Multiple `None` IDs will all map to `""`, potentially merging distinct entries and changing behavior. Consider either keeping `None` as-is (e.g., branch on `tool_call.id is None`) or updating `_deduplicate_self_repeating` to accept `Optional[str]` and leave `None` unchanged.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a _deduplicate_self_repeating helper method to address streaming duplication issues in tool call names and IDs. Reviewers identified a critical bug where cls is used instead of self in an instance method, which will lead to a runtime error. Furthermore, the deduplication logic is noted to be overly simplistic, potentially affecting valid strings or failing for triple repetitions, and requires unit tests for validation.
dosubot
Bot
added
size:M
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
_deduplicate_self_repeatingheuristic is quite aggressive (e.g., it will collapseaaaatoaorabcabcabctoabc), which risks corrupting legitimately repetitive IDs/names; consider constraining it to simpleABABdouble-repeat patterns or adding additional guards (e.g., minimum unit size, character set pattern) to better match the known MiniMax/NVIDIA failure mode. - You currently recompute
_deduplicate_self_repeating(tool_call.id)multiple times; consider deduplicating once into a localdeduped_idand reusing it fortool_call_idsandtool_call_extra_content_dictto avoid divergence and keep the logic clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_deduplicate_self_repeating` heuristic is quite aggressive (e.g., it will collapse `aaaa` to `a` or `abcabcabc` to `abc`), which risks corrupting legitimately repetitive IDs/names; consider constraining it to simple `ABAB` double-repeat patterns or adding additional guards (e.g., minimum unit size, character set pattern) to better match the known MiniMax/NVIDIA failure mode.
- You currently recompute `_deduplicate_self_repeating(tool_call.id)` multiple times; consider deduplicating once into a local `deduped_id` and reusing it for `tool_call_ids` and `tool_call_extra_content_dict` to avoid divergence and keep the logic clearer.
## Individual Comments
### Comment 1
<location path="astrbot/core/provider/sources/openai_source.py" line_range="891-902" />
<code_context>
+ func_name_ls.append(
+ self._deduplicate_self_repeating(tool_call.function.name)
+ )
+ tool_call_ids.append(self._deduplicate_self_repeating(tool_call.id))
# gemini-2.5 / gemini-3 series extra_content handling
extra_content = getattr(tool_call, "extra_content", None)
if extra_content is not None:
- tool_call_extra_content_dict[tool_call.id] = extra_content
+ deduped_id = self._deduplicate_self_repeating(tool_call.id)
+ if deduped_id is not None:
+ tool_call_extra_content_dict[deduped_id] = extra_content
</code_context>
<issue_to_address>
**suggestion:** Reuse the same deduplicated ID instance instead of recomputing it to keep behavior consistent.
The ID is currently deduplicated twice: once when appending to `tool_call_ids` and again when setting `deduped_id` for `tool_call_extra_content_dict`. This risks subtle divergence if `_deduplicate_self_repeating` changes and adds unnecessary work. Compute `deduped_id` once, append it to `tool_call_ids`, and reuse it as the key in `tool_call_extra_content_dict`.
```suggestion
args_ls.append(args)
func_name_ls.append(
self._deduplicate_self_repeating(tool_call.function.name)
)
deduped_id = self._deduplicate_self_repeating(tool_call.id)
tool_call_ids.append(deduped_id)
# gemini-2.5 / gemini-3 series extra_content handling
extra_content = getattr(tool_call, "extra_content", None)
if extra_content is not None and deduped_id is not None:
tool_call_extra_content_dict[deduped_id] = extra_content
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
_deduplicate_self_repeatinghelper collapses any fully repeating pattern (e.g.abab→ab,hahaha→ha), which might unintentionally alter legitimate tool names/IDs/arguments; consider constraining it to the specific duplication pattern you’ve observed (e.g., exact double repetition with a minimum length) or scoping it to known-affected providers. - When deduplicating
function.argumentsbeforejson.loads, this can change the original payload beyond just fixing chunk duplication; if that’s not desired, consider limiting deduplication to identifiers (id,name) and only applying it to arguments when you can positively detect the streaming concatenation pattern.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_deduplicate_self_repeating` helper collapses any fully repeating pattern (e.g. `abab` → `ab`, `hahaha` → `ha`), which might unintentionally alter legitimate tool names/IDs/arguments; consider constraining it to the specific duplication pattern you’ve observed (e.g., exact double repetition with a minimum length) or scoping it to known-affected providers.
- When deduplicating `function.arguments` before `json.loads`, this can change the original payload beyond just fixing chunk duplication; if that’s not desired, consider limiting deduplication to identifiers (`id`, `name`) and only applying it to arguments when you can positively detect the streaming concatenation pattern.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
zouyonghe
left a comment
zouyonghe
left a comment
There was a problem hiding this comment.
我这边补充一个回归风险：
当前 _deduplicate_self_repeating() 会对所有 OpenAI 兼容 provider 的 tool call 名和 id 无条件生效，只要字符串满足“前半段 == 后半段”就会被折叠，例如合法的 pingping 会被改成 ping，abab 会被改成 ab。
但 _parse_openai_completion() 是 openai_source.py 的共享解析路径，普通非流式和流式最终都会走这里，所以这个改动不只影响 MiniMax/NVIDIA 的异常场景，也会影响正常 provider 的合法工具名 / id。后续工具查找是按折叠后的名字继续执行的，这会导致原本存在的工具变成 tool not found。
建议至少把这个修复收窄到已知受影响的 provider / model，或者增加更强的判定信号，而不是对所有 OpenAI 兼容返回统一改写。
…rary repetitions, dedupe arguments
f23d255 to
e329971
Compare
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
_deduplicate_self_repeatinghelper only detects exactly two repetitions (e.g.abcabc) with a relatively highmin_lengthdefault of 20; if streaming ever yields 3+ repeats or shorter tool names/IDs, the bug will still surface—consider making the repetition detection more general and/or lowering or parameterizing the length threshold. - By applying
_deduplicate_self_repeatingto bothtool_call.idand theextra_contentkey, different original IDs that collapse to the same base string will now collide intool_call_extra_content_dict; it may be worth explicitly deciding how to handle collisions or at least guarding/logging when a key is being overwritten.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_deduplicate_self_repeating` helper only detects exactly two repetitions (e.g. `abcabc`) with a relatively high `min_length` default of 20; if streaming ever yields 3+ repeats or shorter tool names/IDs, the bug will still surface—consider making the repetition detection more general and/or lowering or parameterizing the length threshold.
- By applying `_deduplicate_self_repeating` to both `tool_call.id` and the `extra_content` key, different original IDs that collapse to the same base string will now collide in `tool_call_extra_content_dict`; it may be worth explicitly deciding how to handle collisions or at least guarding/logging when a key is being overwritten.
## Individual Comments
### Comment 1
<location path="astrbot/core/provider/sources/openai_source.py" line_range="878-882" />
<code_context>
if tool_call.type == "function":
# workaround for #1454
if isinstance(tool_call.function.arguments, str):
+ deduped_args = self._deduplicate_self_repeating(
+ tool_call.function.arguments
+ )
try:
- args = json.loads(tool_call.function.arguments)
+ args = json.loads(deduped_args)
except json.JSONDecodeError as e:
logger.error(f"解析参数失败: {e}")
</code_context>
<issue_to_address>
**issue:** Consider guarding json.loads() against unexpected None from the deduplication helper.
`_deduplicate_self_repeating` is typed to allow returning `None`, but `json.loads(None)` would raise a `TypeError` that isn’t caught by the `JSONDecodeError` handler. It’s currently safe only because the caller always passes a `str`. To avoid a future unhandled exception, either make `_deduplicate_self_repeating` return `str` when given `str`, or guard before parsing, e.g. `if deduped_args is None: args = {};` (or another explicit fallback) before calling `json.loads`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Blueteemo
left a comment
Blueteemo
left a comment
There was a problem hiding this comment.
已解决。感谢反馈，min_length=20 参数可以有效保护短名称不被误伤，且只处理正好 2 次重复的情况，平衡了修复效果和回归风险。
|
补充：关于 Sourcery 提出的 json.loads(None) 会抛 TypeError 的问题，经分析这是误报。代码中有 isinstance guard：
只有 arguments 是 str 时才调用 json.loads，不会收到 None。所有问题均已处理。 |
a4c4a7d to
9bd38ca
Compare
[现场报告] 流式 tool_call 的
|
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/core/provider/sources/openai_source.py" line_range="66-79" />
<code_context>
return text[: cls._ERROR_TEXT_CANDIDATE_MAX_CHARS]
+ @staticmethod
+ def _deduplicate_self_repeating(
+ value: str | None, min_length: int = 20
+ ) -> str | None:
+ """If string is a self-repeating pattern like 'astr_kb_searchastr_kb_search'
+ (exactly 2 repetitions, min 20 chars), return the base unit.
+ This handles streaming chunk duplication issues for tool names/IDs.
+ Returns None unchanged."""
+ if value is None:
+ return None
+ if not value or len(value) < min_length:
+ return value
+ half = len(value) // 2
+ if value[:half] == value[half:]:
+ return value[:half]
+ return value
+
</code_context>
<issue_to_address>
**issue (bug_risk):** _deduplicate_self_repeating unconditionally halves every string of at least 20 characters whose two halves match, so it changes legitimate values as well as duplicated chunks. For example, a valid JSON numeric argument such as `123456789012345123456789012345` is parsed as a different, shorter number, and a legitimate repeated tool name or ID is changed; if one call has an ID equal to the repeated base of another call, the normalized IDs collide and `tool_call_extra_content_dict` overwrites one call's metadata.
**Triggers:** When a valid tool argument, function name, or opaque tool-call ID happens to consist of two identical halves.
**Suggested fix:** Only deduplicate arguments after parsing the original JSON fails, validate a deduplicated function name against the registered tool set, and avoid rewriting opaque IDs unless the provider-specific duplication can be identified without creating collisions.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and the heuristic changes tool names, IDs, and JSON arguments before they are used, so a legitimate value that happens to consist of two identical halves could be truncated and cause a tool to run with the wrong input or identifier. Reverting stops future incorrect calls, but any external side effect from a call made with the altered value would need separate repair or rerunning.
Blocking findings: astrbot/core/provider/sources/openai_source.py:79
| def _deduplicate_self_repeating( | ||
| value: str | None, min_length: int = 20 | ||
| ) -> str | None: | ||
| """If string is a self-repeating pattern like 'astr_kb_searchastr_kb_search' | ||
| (exactly 2 repetitions, min 20 chars), return the base unit. | ||
| This handles streaming chunk duplication issues for tool names/IDs. | ||
| Returns None unchanged.""" | ||
| if value is None: | ||
| return None | ||
| if not value or len(value) < min_length: | ||
| return value | ||
| half = len(value) // 2 | ||
| if value[:half] == value[half:]: | ||
| return value[:half] |
There was a problem hiding this comment.
issue (bug_risk): _deduplicate_self_repeating unconditionally halves every string of at least 20 characters whose two halves match, so it changes legitimate values as well as duplicated chunks. For example, a valid JSON numeric argument such as 123456789012345123456789012345 is parsed as a different, shorter number, and a legitimate repeated tool name or ID is changed; if one call has an ID equal to the repeated base of another call, the normalized IDs collide and tool_call_extra_content_dict overwrites one call's metadata.
Triggers: When a valid tool argument, function name, or opaque tool-call ID happens to consist of two identical halves.
Suggested fix: Only deduplicate arguments after parsing the original JSON fails, validate a deduplicated function name against the registered tool set, and avoid rewriting opaque IDs unless the provider-specific duplication can be identified without creating collisions.
|
感谢大家继续跟进，也感谢 @x1051445024 补充现场日志和流式侧方案。 我重新检查了当前实现，最新 review 指出的风险仍然存在： 针对当前提交的去重函数做了独立验证，确认三次重复不会被处理、合法的长重复字符串会被截半，两个不同 ID 也可能归一成同一个值。这还不是完整的流式端到端测试。 @x1051445024 看到你还提交了 #9593，目前也有 #9681 在处理流式 index 和缺失 ID 问题。想确认一下，你在现场报告中提到的重复 id/name 防护，是否已有单独分支或 PR？如果方便，也请提供脱敏的最小复现脚本或原始 chunk 样本，尤其是重复携带完整字段和同 index 换 ID 的情况。请去除密钥、用户内容等敏感信息，保留字段结构、分片顺序及必要的 SDK/网关版本即可。 我倾向于围绕这些样本补充流式回放测试，区分正常分片与异常重复，再确定修复方式，避免继续扩大最终字符串去重的范围，也避免与现有 PR 重复修改。同 index 出现不同 ID 是否应拆成新调用，也需要结合原始分片确认。 维护者如果已有倾向的处理方案或相关修复，也欢迎指出，后续可以据此协调 #7735 的范围。 |
|
@Blueteemo 我重新核对了 #9593、#9681，并整理了独立 PR：#10146。 先澄清范围：#9593 是我的提交，主要处理 index 归一化和序列化时的缺失 ID 兜底；#9681 是 @LIKIQ 的提交，对缺失 ID、名称及工具结果消息配对的处理更完整。目前两者都未合并，也都没有处理重复完整 进一步回放后，我也需要收窄之前现场报告中的建议：
#10146 采用默认关闭的显式兼容选项 下面是最小合成回放输入，每行表示按顺序到达的 {"index":0,"id":"call_abc","type":"function","function":{"name":"reverse_image_search","arguments":"{\"query\":"}}
{"index":0,"id":"call_abc","type":"function","function":{"name":"reverse_image_search","arguments":"\"sample\""}}
{"index":0,"id":"call_abc","type":"function","function":{"name":"reverse_image_search","arguments":"}"}}可直接运行新 PR 的 当前验证环境：Windows、Python 3.12.12、OpenAI SDK 2.46.0、Pydantic 2.13.4。之前事故记录中的 SDK 是 2.33.0；手头没有当时的原始 SSE 或准确网关构建版本，所以不会把构造样本当作原始证据，也不声称已经解决所有网关变体。高级配置方法、测试边界和与现有 PR 的关系都写在 #10146 描述中。 |
|
继续跟进后，当前结论已经明确：本 PR 在最终字符串上做自重复截半，仍可能误改合法的工具名、调用 ID 和 JSON 参数，也可能造成 ID 碰撞；min_length 不能消除这个风险。 #10146 已基于本 PR 的讨论提供更安全的替代方案：默认关闭、只在明确启用时于流式边界处理重复的完整 metadata pair，不改写 arguments 或 extra_content，并加入 35 个流式回放测试；最新 Sourcery review 已批准。 继续修改本 PR 会与 #10146 重复，而且当前实现不适合合并。因此将本 PR 关闭为 superseded by #10146，保留讨论和事故证据供后续参考。感谢 @zouyonghe 和 @x1051445024 的审查与复现补充。 |
3 participants
问题描述
使用 MiniMax 模型通过 NVIDIA 代理调用时，工具调用名出现重复拼接：
astr_kb_search→astr_kb_searchastr_kb_searchcall_xxx→call_xxxcall_xxx这是因为流式响应处理过程中
chunk被重复累积导致的。修复方案
在
openai_source.py的_parse_openai_completion方法中，对 tool call 的id和name增加去重校验。当检测到字符串是由前半段和后半段相同的“自重复”模式构成时（例如abcabc→abc），取前半段作为正确值。修改内容
ProviderOpenAIOfficial类中添加_deduplicate_self_repeating静态方法，用于检测并修复自重复字符串。tool_calls时，对function.name、id和extra_content的 key 都应用去重处理。关联 Issue
Fixes #7694
Summary by Sourcery
Deduplicate repeated values in OpenAI-compatible streaming tool calls to restore correct tool invocation behavior.
Bug Fixes:
Enhancements: