增强 OpenAI-compatible Provider 与 FAISS 兼容性#8689
Conversation
- tolerate raw string and dict completions from compatible providers - sanitize malformed tool calls before agent execution - support low-level FAISS SWIG add/search signatures - skip unreadable distribution metadata in dependency scans
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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 3 issues, and left some high level feedback:
- A lot of docstrings, comments, and log messages in
openai_source.py,entities.py,embedding_storage.py, andrequirements_utils.pynow contain???instead of readable Chinese text; this looks like an accidental encoding or search/replace corruption and should be reverted or corrected before merging. - In the FAISS
searchpath, you handle the SWIG signature fallback but rely onfaiss.normalize_L2(vector)in-place without explicitly ensuringvectoris contiguous andfloat32like in_add_with_ids; consider normalizing a contiguousfloat32copy to avoid subtle issues when passing viaswig_ptr. - The new error-handling branches that detect SWIG signature mismatches and unreadable distribution metadata are keyed off substring matches in exception messages (e.g.,
'missing 3 required positional arguments','missing' inTypeError`); if possible, tighten these checks (e.g., exact messages or additional guards) to reduce the risk of catching unrelated errors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- A lot of docstrings, comments, and log messages in `openai_source.py`, `entities.py`, `embedding_storage.py`, and `requirements_utils.py` now contain `???` instead of readable Chinese text; this looks like an accidental encoding or search/replace corruption and should be reverted or corrected before merging.
- In the FAISS `search` path, you handle the SWIG signature fallback but rely on `faiss.normalize_L2(vector)` in-place without explicitly ensuring `vector` is contiguous and `float32` like in `_add_with_ids`; consider normalizing a contiguous `float32` copy to avoid subtle issues when passing via `swig_ptr`.
- The new error-handling branches that detect SWIG signature mismatches and unreadable distribution metadata are keyed off substring matches in exception messages (e.g., `'missing 3 required positional arguments'`, `'missing' in `TypeError`); if possible, tighten these checks (e.g., exact messages or additional guards) to reduce the risk of catching unrelated errors.
## Individual Comments
### Comment 1
<location path="astrbot/core/provider/sources/openai_source.py" line_range="733-737" />
<code_context>
state.handle_chunk(chunk)
except Exception as e:
- logger.error("Saving chunk state error: " + str(e))
+ logger.warning("Saving chunk state skipped: " + str(e))
# logger.debug(f"chunk delta: {delta}")
# handle the content delta
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Downgrading state.handle_chunk errors from error to warning may hide stream reconstruction issues.
Because `state.handle_chunk` reconstructs the final completion (including usage), treating its failures as warnings while continuing can hide subtle output or accounting corruption. If you need to keep the stream flowing, please at least log the full traceback and chunk metadata, or make this downgrade configurable so operational debugging remains feasible.
```suggestion
if delta is not None or chunk.usage:
try:
state.handle_chunk(chunk)
except Exception as e:
# Keep stream flowing but log full traceback and chunk metadata for debugging
logger.warning(
"Saving chunk state skipped for chunk %r: %s",
chunk,
e,
exc_info=True,
)
```
</issue_to_address>
### Comment 2
<location path="astrbot/core/db/vec_db/faiss_impl/embedding_storage.py" line_range="27-36" />
<code_context>
assert self.index is not None, "FAISS index is not initialized."
faiss.normalize_L2(vector)
- distances, indices = self.index.search(vector, k)
+ try:
+ distances, indices = self.index.search(vector, k)
+ except TypeError as exc:
+ if "missing 3 required positional arguments" not in str(exc):
+ raise
+ distances = np.empty((vector.shape[0], k), dtype=np.float32)
+ indices = np.empty((vector.shape[0], k), dtype=np.int64)
+ self.index.search(
+ vector.shape[0],
+ faiss.swig_ptr(vector),
</code_context>
<issue_to_address>
**suggestion (bug_risk):** FAISS search fallback also depends on a specific error message and assumes vector dtype/shape.
The `search` fallback currently relies on the exact substring `"missing 3 required positional arguments"`, which is tied to a specific FAISS version and may break if the error message changes. It also calls the pointer-based overload without first ensuring `vector` is contiguous float32, so non-contiguous or non-float32 inputs could cause `faiss.swig_ptr` misuse. Consider mirroring `_add_with_ids` by normalizing via `np.ascontiguousarray(vector, dtype=np.float32)` and using a more robust way to detect the legacy signature than matching a hard-coded error string.
</issue_to_address>
### Comment 3
<location path="astrbot/core/provider/entities.py" line_range="455-456" />
<code_context>
"""Convert to OpenAI tool calls format. Deprecated, use to_openai_to_calls_model instead."""
ret = []
for idx, tool_call_arg in enumerate(self.tools_call_args):
+ if idx >= len(self.tools_call_name):
+ break
+ tool_name = self.tools_call_name[idx]
+ if not isinstance(tool_name, str) or not tool_name.strip():
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Silently breaking when tools_call_args is longer than tools_call_name risks dropping tool calls.
By breaking when `idx >= len(self.tools_call_name)`, any extra `tools_call_args` entries are silently discarded, which can mask upstream data inconsistencies. Consider emitting a warning (and similarly on `len(self.tools_call_ids)` mismatches) so inconsistent tool-call lists are detectable rather than silently losing tool invocations.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if delta is not None or chunk.usage: | ||
| try: | ||
| state.handle_chunk(chunk) | ||
| except Exception as e: | ||
| logger.error("Saving chunk state error: " + str(e)) | ||
| logger.warning("Saving chunk state skipped: " + str(e)) |
There was a problem hiding this comment.
suggestion (bug_risk): Downgrading state.handle_chunk errors from error to warning may hide stream reconstruction issues.
Because state.handle_chunk reconstructs the final completion (including usage), treating its failures as warnings while continuing can hide subtle output or accounting corruption. If you need to keep the stream flowing, please at least log the full traceback and chunk metadata, or make this downgrade configurable so operational debugging remains feasible.
| if delta is not None or chunk.usage: | |
| try: | |
| state.handle_chunk(chunk) | |
| except Exception as e: | |
| logger.error("Saving chunk state error: " + str(e)) | |
| logger.warning("Saving chunk state skipped: " + str(e)) | |
| if delta is not None or chunk.usage: | |
| try: | |
| state.handle_chunk(chunk) | |
| except Exception as e: | |
| # Keep stream flowing but log full traceback and chunk metadata for debugging | |
| logger.warning( | |
| "Saving chunk state skipped for chunk %r: %s", | |
| chunk, | |
| e, | |
| exc_info=True, | |
| ) |
| try: | ||
| self.index.add_with_ids(vectors, ids) | ||
| except TypeError as exc: | ||
| if "missing" not in str(exc): | ||
| raise | ||
| self.index.add_with_ids( | ||
| vectors.shape[0], | ||
| faiss.swig_ptr(vectors), | ||
| faiss.swig_ptr(ids), | ||
| ) |
There was a problem hiding this comment.
suggestion (bug_risk): FAISS search fallback also depends on a specific error message and assumes vector dtype/shape.
The search fallback currently relies on the exact substring "missing 3 required positional arguments", which is tied to a specific FAISS version and may break if the error message changes. It also calls the pointer-based overload without first ensuring vector is contiguous float32, so non-contiguous or non-float32 inputs could cause faiss.swig_ptr misuse. Consider mirroring _add_with_ids by normalizing via np.ascontiguousarray(vector, dtype=np.float32) and using a more robust way to detect the legacy signature than matching a hard-coded error string.
| if idx >= len(self.tools_call_name): | ||
| break |
There was a problem hiding this comment.
suggestion (bug_risk): Silently breaking when tools_call_args is longer than tools_call_name risks dropping tool calls.
By breaking when idx >= len(self.tools_call_name), any extra tools_call_args entries are silently discarded, which can mask upstream data inconsistencies. Consider emitting a warning (and similarly on len(self.tools_call_ids) mismatches) so inconsistent tool-call lists are detectable rather than silently losing tool invocations.
![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 several robustness improvements, including fallback mechanisms using SWIG pointers for FAISS vector database operations, safer parsing and validation of tool calls in OpenAI-compatible providers, and better error handling when collecting installed package distributions. However, a critical encoding issue has corrupted all Chinese characters in comments and docstrings across the modified files, replacing them with question marks. Additionally, feedback highlights a potential segmentation fault in the FAISS search fallback when handling 1D vectors, and a potential AttributeError in _parse_openai_completion when a tool call is parsed as a dictionary but accessed via attributes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| except ModuleNotFoundError: | ||
| raise ImportError( | ||
| "faiss 未安装。请使用 'pip install faiss-cpu' 或 'pip install faiss-gpu' 安装。", | ||
| "faiss ??????? 'pip install faiss-cpu' ? 'pip install faiss-gpu' ???", |
There was a problem hiding this comment.
Critical Encoding Issue
It appears that all Chinese characters in comments, docstrings, and log messages across all modified files in this pull request have been corrupted and replaced with question marks (?). This usually happens when a file containing UTF-8 characters is opened or saved using an editor configured with an incompatible encoding (like ASCII or ISO-8859-1).
Please restore the original Chinese text and ensure your editor/IDE is configured to use UTF-8 encoding before committing.
| faiss.normalize_L2(vector) | ||
| distances, indices = self.index.search(vector, k) | ||
| try: | ||
| distances, indices = self.index.search(vector, k) | ||
| except TypeError as exc: | ||
| if "missing 3 required positional arguments" not in str(exc): | ||
| raise | ||
| distances = np.empty((vector.shape[0], k), dtype=np.float32) | ||
| indices = np.empty((vector.shape[0], k), dtype=np.int64) | ||
| self.index.search( | ||
| vector.shape[0], | ||
| faiss.swig_ptr(vector), | ||
| k, | ||
| faiss.swig_ptr(distances), | ||
| faiss.swig_ptr(indices), | ||
| ) |
There was a problem hiding this comment.
Potential Segmentation Fault / Crash in SWIG Fallback
In the search method, if vector is a 1D array (e.g., shape (dimension,)), vector.shape[0] will represent the dimension of the vector (e.g., 1536) rather than the number of query vectors n (which should be 1).
When the SWIG fallback is triggered, passing vector.shape[0] as n to self.index.search will cause FAISS to read out of bounds and crash with a segmentation fault.
To prevent this, we should ensure vector is contiguous, float32, and reshaped to 2D (shape (1, dimension)) if it is 1D.
| faiss.normalize_L2(vector) | |
| distances, indices = self.index.search(vector, k) | |
| try: | |
| distances, indices = self.index.search(vector, k) | |
| except TypeError as exc: | |
| if "missing 3 required positional arguments" not in str(exc): | |
| raise | |
| distances = np.empty((vector.shape[0], k), dtype=np.float32) | |
| indices = np.empty((vector.shape[0], k), dtype=np.int64) | |
| self.index.search( | |
| vector.shape[0], | |
| faiss.swig_ptr(vector), | |
| k, | |
| faiss.swig_ptr(distances), | |
| faiss.swig_ptr(indices), | |
| ) | |
| vector = np.ascontiguousarray(vector, dtype=np.float32) | |
| if vector.ndim == 1: | |
| vector = vector.reshape(1, -1) | |
| faiss.normalize_L2(vector) | |
| try: | |
| distances, indices = self.index.search(vector, k) | |
| except TypeError as exc: | |
| if "missing 3 required positional arguments" not in str(exc): | |
| raise | |
| distances = np.empty((vector.shape[0], k), dtype=np.float32) | |
| indices = np.empty((vector.shape[0], k), dtype=np.int64) | |
| self.index.search( | |
| vector.shape[0], | |
| faiss.swig_ptr(vector), | |
| k, | |
| faiss.swig_ptr(distances), | |
| faiss.swig_ptr(indices), | |
| ) |
| if tool_call.type == "function": | ||
| func_name = getattr(tool_call.function, "name", None) | ||
| if not isinstance(func_name, str) or not func_name.strip(): | ||
| logger.warning( | ||
| "Skipping malformed tool call with empty function name: %s", | ||
| tool_call, | ||
| ) | ||
| continue | ||
| func_name = func_name.strip() | ||
| tool_call_id = getattr(tool_call, "id", None) | ||
| if not isinstance(tool_call_id, str) or not tool_call_id.strip(): | ||
| tool_call_id = f"call_{uuid.uuid4().hex}" | ||
| logger.warning( | ||
| "Generated missing tool_call id for %s: %s", | ||
| func_name, | ||
| tool_call_id, | ||
| ) | ||
| # workaround for #1454 | ||
| if isinstance(tool_call.function.arguments, str): | ||
| try: | ||
| args = json.loads(tool_call.function.arguments) | ||
| except json.JSONDecodeError as e: | ||
| logger.error(f"解析参数失败: {e}") | ||
| logger.error(f"??????: {e}") | ||
| args = {} | ||
| else: | ||
| args = tool_call.function.arguments | ||
| # Some API may return None for tools with no parameters | ||
| if args is None: | ||
| args = {} | ||
| if not isinstance(args, dict): | ||
| logger.warning( | ||
| "Tool call arguments for %s are not an object: %s", | ||
| func_name, | ||
| type(args).__name__, | ||
| ) | ||
| args = {} | ||
| args_ls.append(args) | ||
| func_name_ls.append(tool_call.function.name) | ||
| tool_call_ids.append(tool_call.id) | ||
| func_name_ls.append(func_name) | ||
| tool_call_ids.append(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 | ||
| tool_call_extra_content_dict[tool_call_id] = extra_content |
There was a problem hiding this comment.
AttributeError when tool_call is a dict
At line 954, there is a workaround for #1359 where tool_call is parsed from a JSON string using json.loads(tool_call), which turns it into a dict.
However, the subsequent code at line 960 directly accesses attributes of tool_call (e.g., tool_call.type, tool_call.function, tool_call.id), which will raise an AttributeError: 'dict' object has no attribute 'type' when tool_call is a dictionary.
We should handle both object and dictionary access patterns safely to prevent runtime crashes when this workaround is triggered.
is_dict = isinstance(tool_call, dict)
tc_type = tool_call.get("type") if is_dict else getattr(tool_call, "type", None)
if tc_type == "function":
tc_func = tool_call.get("function") if is_dict else getattr(tool_call, "function", None)
func_name = tc_func.get("name") if isinstance(tc_func, dict) else getattr(tc_func, "name", None)
if not isinstance(func_name, str) or not func_name.strip():
logger.warning(
"Skipping malformed tool call with empty function name: %s",
tool_call,
)
continue
func_name = func_name.strip()
tool_call_id = tool_call.get("id") if is_dict else getattr(tool_call, "id", None)
if not isinstance(tool_call_id, str) or not tool_call_id.strip():
tool_call_id = f"call_{uuid.uuid4().hex}"
logger.warning(
"Generated missing tool_call id for %s: %s",
func_name,
tool_call_id,
)
# workaround for #1454
tc_args = tc_func.get("arguments") if isinstance(tc_func, dict) else getattr(tc_func, "arguments", None)
if isinstance(tc_args, str):
try:
args = json.loads(tc_args)
except json.JSONDecodeError as e:
logger.error(f"解析参数失败: {e}")
args = {}
else:
args = tc_args
# Some API may return None for tools with no parameters
if args is None:
args = {}
if not isinstance(args, dict):
logger.warning(
"Tool call arguments for %s are not an object: %s",
func_name,
type(args).__name__,
)
args = {}
args_ls.append(args)
func_name_ls.append(func_name)
tool_call_ids.append(tool_call_id)
# gemini-2.5 / gemini-3 series extra_content handling
extra_content = tool_call.get("extra_content") if is_dict else getattr(tool_call, "extra_content", None)
if extra_content is not None:
tool_call_extra_content_dict[tool_call_id] = extra_content
x1051445024
changed the title
x1051445024
changed the title
x1051445024
changed the title
x1051445024
changed the title
Mirror the existing add_with_ids/search SWIG-compat fallbacks for delete(). Some faiss builds compile IndexIDMap.remove_ids to accept only a faiss::IDSelector, so passing a raw int64 ndarray raises TypeError and breaks batch delete / memory cleanup. Fall back to IDSelectorBatch.
dosubot
Bot
added
the
size:L
2 participants
摘要
add_with_ids/search/remove_ids调用签名，并避免一维查询向量触发 fallback 时越界背景
在一个 Windows 打包安装环境中观察到以下问题，其中插件
site-packages里的 FAISS 版本会优先被加载：IndexIDMap.search() missing 3 required positional arguments: 'k', 'distances', and 'labels'IndexFlat.search() missing 3 required positional arguments: 'k', 'distances', and 'labels'IndexIDMap_remove_ids, argument 2 of type 'faiss::IDSelector const &'（批量删除 / 清理旧记忆时触发，删除整体失败）<class 'str'>，导致API 返回的 completion 类型错误ToolCall.id、Pydantic 校验失败或工具参数解析失败评审反馈处理
embedding_storage.py中被错误转码的中文注释和报错文本search()fallback 前会把查询向量转换成 contiguousfloat32，并在一维输入时 reshape 成(1, dimension)，避免低层 SWIG 调用读取越界delete()与add_with_ids/search保持一致：先按常规方式调用remove_ids，捕获到IDSelector相关TypeError时改用IDSelectorBatch选择器重试，避免某些 faiss 构建下批量删除 / 记忆清理直接报错state.handle_chunk()失败时仍保持流式输出继续，但补充 chunk 信息和 traceback，方便排查最终 completion/usage 重建问题验证
ruff format --check .通过ruff check .通过py_compile通过修改文件IDSelectorBatch构造与remove_idsfallback 正常工作