Open
Conversation
dosubot
bot
added
the
size:L
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取请求旨在通过引入一个通用的 Rerank 适配器来增强系统的灵活性和可扩展性。该适配器能够与各种兼容 NewAPI 的 Rerank 服务无缝集成，简化了用户配置和认证流程。同时，对配置文案的优化提升了用户体验，确保了清晰准确的指引。 Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
dosubot
bot
added
the
area:provider
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - 我发现了 4 个问题，并给出了一些整体性的反馈：
- 在
default.py中，新增加的「通用 Rerank」模板使用的是"provider": "openai"，而 OpenAI 的聊天补全模板已经切换为"generic"；建议这里也统一 provider 标识，以避免把这个通用适配器和特定于 OpenAI 的适配器混淆。 OpenAIRerankProvider.rerank方法目前的类型标注是documents: list[str]，但_normalize_documents和测试都同时支持字符串和映射；建议将类型注解以及相关接口更新为list[str | Mapping]之类的形式，使实际接受的输入更加清晰，并避免被误用。
给 AI Agent 的提示词
Please address the comments from this code review:
## Overall Comments
- 在 `default.py` 中，新增加的「通用 Rerank」模板使用的是 `"provider": "openai"`，而 OpenAI 的聊天补全模板已经切换为 `"generic"`；建议这里也统一 provider 标识，以避免把这个通用适配器和特定于 OpenAI 的适配器混淆。
- `OpenAIRerankProvider.rerank` 方法目前的类型标注是 `documents: list[str]`，但 `_normalize_documents` 和测试都同时支持字符串和映射；建议将类型注解以及相关接口更新为 `list[str | Mapping]` 之类的形式，使实际接受的输入更加清晰，并避免被误用。
## Individual Comments
### Comment 1
<location path="astrbot/core/provider/sources/openai_rerank_source.py" line_range="138-147" />
<code_context>
+ async def rerank(
+ self,
+ query: str,
+ documents: list[str],
+ top_n: int | None = None,
+ ) -> list[RerankResult]:
+ if not documents:
+ return []
+ if not query.strip():
+ logger.warning("通用 Rerank 查询文本为空，返回空结果。")
+ return []
+ if not self.client:
+ logger.error("通用 Rerank 客户端会话已关闭，返回空结果。")
+ return []
+
+ payload = self._build_payload(
+ query=query,
+ documents=self._normalize_documents(documents),
+ top_n=top_n,
+ )
</code_context>
<issue_to_address>
**suggestion:** `rerank` 中 `documents` 的类型注解比 `_normalize_documents` 所支持的范围更窄。
由于 `_normalize_documents` 同时接受字符串和映射类型，并且会对不支持的类型抛出异常，目前的 `list[str]` 注解并没有反映真实可接受的输入范围，也可能误导类型检查器和 IDE。建议放宽类型（例如 `list[str | Mapping[str, Any]]` 或 `list[Any]`），以与真实契约保持一致。
Suggested implementation:
```python
async def rerank(
self,
query: str,
documents: list[str | Mapping[str, Any]],
top_n: int | None = None,
) -> list[RerankResult]:
```
1. 确保在 `openai_rerank_source.py` 文件开头导入了 `Mapping` 和 `Any`，通常为：
`from typing import Any, Mapping`
2. 如果代码库中使用了 `typing` 类型别名（例如 `DocumentInput = str | Mapping[str, Any]`），可以考虑定义并使用这样的别名，而不是内联联合类型，以便在不同 provider 之间保持类型一致。
</issue_to_address>
### Comment 2
<location path="astrbot/core/provider/sources/openai_rerank_source.py" line_range="42-47" />
<code_context>
+ if not self.api_key:
+ raise ValueError("通用 Rerank API Key 不能为空。")
+
+ self.client = aiohttp.ClientSession(
+ headers={
+ "Authorization": f"Bearer {self.api_key}",
+ "Content-Type": "application/json",
+ },
+ timeout=aiohttp.ClientTimeout(total=self.timeout),
+ )
+
</code_context>
<issue_to_address>
**issue (bug_risk):** 在 `__init__` 中创建 `aiohttp.ClientSession` 可能存在问题，这取决于 provider 的构造方式。
这种做法假设 `__init__` 总是在一个已激活的事件循环中执行；但如果 provider 在导入时或事件循环启动之前就被构造，就可能导致 `DeprecationWarning`/`RuntimeWarning` 以及生命周期管理问题。建议在首次使用时再延迟创建 `ClientSession`（例如在 `rerank` 中或单独的异步初始化方法里），这样可以保证它一定是在运行中的事件循环内创建；或者需要在文档中明确说明并验证：外围框架只会在事件循环内构造此类实例。
</issue_to_address>
### Comment 3
<location path="astrbot/core/provider/sources/openai_rerank_source.py" line_range="159-161" />
<code_context>
+ try:
+ async with self.client.post(self.api_url, json=payload) as response:
+ if response.status >= 400:
+ response_text = await response.text()
+ raise RuntimeError(
+ f"通用 Rerank API 请求失败: HTTP {response.status}, {response_text}"
+ )
+
</code_context>
<issue_to_address>
**🚨 suggestion (security):** 在错误信息中包含完整响应体可能过大或包含敏感数据。
当前在 HTTP 出错时将完整响应体插入异常信息。对于通用的 rerank 接口，响应体可能非常大，并且可能包含用户内容。为减少日志量并避免泄露敏感数据，建议将 `response_text` 截断至固定长度，或用经过脱敏的摘要代替，同时保留状态码以及一小段片段即可。
Suggested implementation:
```python
try:
async with self.client.post(self.api_url, json=payload) as response:
if response.status >= 400:
response_text = await response.text()
snippet = response_text[:200]
if len(response_text) > 200:
snippet += "...[truncated]"
logger.warning(
"通用 Rerank API 请求失败: HTTP %s, body snippet: %s",
response.status,
snippet,
)
raise RuntimeError(
f"通用 Rerank API 请求失败: HTTP {response.status}"
)
```
如果代码的其它部分依赖异常信息中携带完整响应体，可以考虑：
1. 调整相关异常处理逻辑，不再依赖错误文本中的完整响应内容。
2. 根据你的日志策略，酌情修改片段长度（此处为 200 字符）或日志级别（例如改用 `logger.debug` 而不是 `warning`）。
</issue_to_address>
### Comment 4
<location path="tests/test_openai_rerank_source.py" line_range="25" />
<code_context>
+ return self._text
+
+
+class _MockClient:
+ def __init__(self, response: _MockResponse):
+ self.response = response
</code_context>
<issue_to_address>
**suggestion (testing):** 建议增加一个小测试，用来断言 `terminate` 确实会关闭 client 并将其置空。
`terminate()` 已经在异步测试中被调用，但目前并未断言它的副作用。请增加一个聚焦的异步测试：给 provider 设置一个 mock client，调用 `terminate()`，然后断言 client 的 `close()` 被调用（例如通过一个 `closed` 标志位），并且 `provider.client` 被设置为 `None`，这样一旦清理逻辑发生变更，就能通过测试及时发现。
Suggested implementation:
```python
class _MockClient:
def __init__(self, response: _MockResponse):
self.response = response
self.calls: list[tuple[str, dict]] = []
self.closed = False
def post(self, url: str, json: dict):
self.calls.append((url, json))
return self.response
async def close(self):
self.closed = True
@pytest.mark.asyncio
async def test_terminate_closes_and_nulls_client(provider):
"""Ensure terminate() closes the client and clears the reference."""
mock_response = _MockResponse(payload={}, text="")
mock_client = _MockClient(mock_response)
provider.client = mock_client
await provider.terminate()
assert mock_client.closed is True
assert provider.client is None
```
为使上述测试能够编译并通过，你可能需要：
1. 确保在 `tests/test_openai_rerank_source.py` 顶部导入了 `pytest`：
- `import pytest`
2. 将 `test_terminate_closes_and_nulls_client` 的 `provider` 参数调整为与你现有的 fixture 或工厂保持一致：
- 如果你当前使用了不同名称的 fixture（例如 `openai_rerank_source`），请相应地重命名该参数。
3. 确认 provider 对象具有 `client` 属性，并提供一个异步的 `terminate()` 方法，该方法会清理 `client` 并调用其 `close()`。
</issue_to_address>请帮我变得更有用！你可以在每条评论上点 👍 或 👎，我会根据这些反馈改进后续的评审。
Original comment in English
Hey - I've found 4 issues, and left some high level feedback:
- In
default.py, the new "通用 Rerank" template uses"provider": "openai"while the chat completion template for OpenAI was switched to"generic"; consider aligning the provider identifier here as well to avoid confusing this generic adapter with the OpenAI-specific one. - The
OpenAIRerankProvider.rerankmethod is typed asdocuments: list[str]but_normalize_documentsand the tests expect both strings and mappings; updating the type hints and any related interfaces to reflectlist[str | Mapping]would make the accepted input clearer and prevent misuse.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `default.py`, the new "通用 Rerank" template uses `"provider": "openai"` while the chat completion template for OpenAI was switched to `"generic"`; consider aligning the provider identifier here as well to avoid confusing this generic adapter with the OpenAI-specific one.
- The `OpenAIRerankProvider.rerank` method is typed as `documents: list[str]` but `_normalize_documents` and the tests expect both strings and mappings; updating the type hints and any related interfaces to reflect `list[str | Mapping]` would make the accepted input clearer and prevent misuse.
## Individual Comments
### Comment 1
<location path="astrbot/core/provider/sources/openai_rerank_source.py" line_range="138-147" />
<code_context>
+ async def rerank(
+ self,
+ query: str,
+ documents: list[str],
+ top_n: int | None = None,
+ ) -> list[RerankResult]:
+ if not documents:
+ return []
+ if not query.strip():
+ logger.warning("通用 Rerank 查询文本为空，返回空结果。")
+ return []
+ if not self.client:
+ logger.error("通用 Rerank 客户端会话已关闭，返回空结果。")
+ return []
+
+ payload = self._build_payload(
+ query=query,
+ documents=self._normalize_documents(documents),
+ top_n=top_n,
+ )
</code_context>
<issue_to_address>
**suggestion:** The `documents` type annotation in `rerank` is narrower than what `_normalize_documents` supports.
Since `_normalize_documents` accepts both strings and mappings and will raise on unsupported types, the current `list[str]` annotation does not reflect the actual accepted inputs and can mislead type checkers and IDEs. Please widen the type (e.g., `list[str | Mapping[str, Any]]` or `list[Any]`) to match the real contract.
Suggested implementation:
```python
async def rerank(
self,
query: str,
documents: list[str | Mapping[str, Any]],
top_n: int | None = None,
) -> list[RerankResult]:
```
1. Ensure `Mapping` and `Any` are imported at the top of `openai_rerank_source.py`, typically:
`from typing import Any, Mapping`
2. If the codebase uses `typing` aliases (e.g. `DocumentInput = str | Mapping[str, Any]`), consider defining and using such an alias instead of an inline union, to keep types consistent across providers.
</issue_to_address>
### Comment 2
<location path="astrbot/core/provider/sources/openai_rerank_source.py" line_range="42-47" />
<code_context>
+ if not self.api_key:
+ raise ValueError("通用 Rerank API Key 不能为空。")
+
+ self.client = aiohttp.ClientSession(
+ headers={
+ "Authorization": f"Bearer {self.api_key}",
+ "Content-Type": "application/json",
+ },
+ timeout=aiohttp.ClientTimeout(total=self.timeout),
+ )
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Creating an `aiohttp.ClientSession` in `__init__` may be problematic depending on how providers are constructed.
This assumes `__init__` always runs inside an active event loop, which may not hold if providers are constructed at import time or before the loop starts, leading to `DeprecationWarning`/`RuntimeWarning` and lifecycle issues. Consider lazily creating the `ClientSession` on first use (e.g., in `rerank` or an async init method) so it’s guaranteed to be created within a running loop, or explicitly document and verify that the surrounding framework only constructs this class inside an event loop.
</issue_to_address>
### Comment 3
<location path="astrbot/core/provider/sources/openai_rerank_source.py" line_range="159-161" />
<code_context>
+ try:
+ async with self.client.post(self.api_url, json=payload) as response:
+ if response.status >= 400:
+ response_text = await response.text()
+ raise RuntimeError(
+ f"通用 Rerank API 请求失败: HTTP {response.status}, {response_text}"
+ )
+
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Including the full response body in error messages could be large or contain sensitive data.
You’re interpolating the full response body into the exception message on HTTP errors. For generic rerank endpoints this can be very large and may include user content. To reduce log volume and avoid leaking sensitive data, consider truncating `response_text` to a fixed length or replacing it with a sanitized summary while still including the status code and possibly a short snippet.
Suggested implementation:
```python
try:
async with self.client.post(self.api_url, json=payload) as response:
if response.status >= 400:
response_text = await response.text()
snippet = response_text[:200]
if len(response_text) > 200:
snippet += "...[truncated]"
logger.warning(
"通用 Rerank API 请求失败: HTTP %s, body snippet: %s",
response.status,
snippet,
)
raise RuntimeError(
f"通用 Rerank API 请求失败: HTTP {response.status}"
)
```
If other parts of this method rely on the exception message containing the full response body, you may want to:
1. Adjust any exception handling to no longer expect the full body in the error text.
2. Optionally tune the snippet length (200 chars here) or the logging level (e.g., `logger.debug` instead of `warning`) according to your logging policy.
</issue_to_address>
### Comment 4
<location path="tests/test_openai_rerank_source.py" line_range="25" />
<code_context>
+ return self._text
+
+
+class _MockClient:
+ def __init__(self, response: _MockResponse):
+ self.response = response
</code_context>
<issue_to_address>
**suggestion (testing):** Add a small test to assert `terminate` actually closes and nulls the client
`terminate()` is already used in async tests, but its side effects aren’t asserted. Please add a focused async test that sets a mock client on the provider, calls `terminate()`, and then asserts the client’s `close()` was called (e.g., via a `closed` flag) and `provider.client` is set to `None`, so changes to the cleanup behavior are caught by tests.
Suggested implementation:
```python
class _MockClient:
def __init__(self, response: _MockResponse):
self.response = response
self.calls: list[tuple[str, dict]] = []
self.closed = False
def post(self, url: str, json: dict):
self.calls.append((url, json))
return self.response
async def close(self):
self.closed = True
@pytest.mark.asyncio
async def test_terminate_closes_and_nulls_client(provider):
"""Ensure terminate() closes the client and clears the reference."""
mock_response = _MockResponse(payload={}, text="")
mock_client = _MockClient(mock_response)
provider.client = mock_client
await provider.terminate()
assert mock_client.closed is True
assert provider.client is None
```
To make this compile and pass, you may need to:
1. Ensure `pytest` is imported at the top of `tests/test_openai_rerank_source.py`:
- `import pytest`
2. Adjust the `provider` argument in `test_terminate_closes_and_nulls_client` to match your existing fixture or factory:
- If you currently use a differently named fixture (e.g. `openai_rerank_source`), rename the parameter accordingly.
3. Confirm that the provider object has a `client` attribute and an async `terminate()` method that clears `client` and calls `close()` on it.
</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}
Member
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并给出了一些总体性的反馈：
_build_payload中硬编码的top_k上限 100 可能与所有后端不匹配；建议把这个限制做成可配置项（或者从 provider_settings 中推导），而不是内嵌一个 magic number。- 在
OpenAIRerankProvider.__init__中，timeout被直接强制转换为int；更安全的做法是先校验它是否为正数，如果传入的值无效，则回退到一个默认值或抛出更清晰的错误。
给 AI Agent 的提示词
Please address the comments from this code review:
## Overall Comments
- The hardcoded `top_k` cap of 100 in `_build_payload` may not match all backends; consider making this limit configurable (or derived from provider_settings) instead of embedding a magic number.
- In `OpenAIRerankProvider.__init__`, `timeout` is blindly cast to `int`; it might be safer to validate that it is a positive number and fall back to a default or raise a clearer error if the provided value is invalid.
## Individual Comments
### Comment 1
<location path="astrbot/core/provider/sources/openai_rerank_source.py" line_range="94-103" />
<code_context>
+ async with client.post(self.api_url, json=payload) as response:
+ if response.status >= 400:
+ response_text = await response.text()
+ logger.warning(
+ "通用 Rerank API 请求失败: HTTP %s, body snippet: %s",
+ response.status,
+ self._truncate_error_response(response_text),
+ )
+ raise RuntimeError(
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Consider whether logging raw response snippets may leak sensitive data from the rerank provider.
The warning log currently includes up to 200 chars of the raw response body, which may contain query text, document content, or other sensitive data depending on the provider’s error format. Consider either redacting the body, restricting logs to safe structured fields (e.g., JSON error code/message), or moving the raw snippet to a debug-level log instead of warning.
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续的代码评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The hardcoded
top_kcap of 100 in_build_payloadmay not match all backends; consider making this limit configurable (or derived from provider_settings) instead of embedding a magic number. - In
OpenAIRerankProvider.__init__,timeoutis blindly cast toint; it might be safer to validate that it is a positive number and fall back to a default or raise a clearer error if the provided value is invalid.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The hardcoded `top_k` cap of 100 in `_build_payload` may not match all backends; consider making this limit configurable (or derived from provider_settings) instead of embedding a magic number.
- In `OpenAIRerankProvider.__init__`, `timeout` is blindly cast to `int`; it might be safer to validate that it is a positive number and fall back to a default or raise a clearer error if the provided value is invalid.
## Individual Comments
### Comment 1
<location path="astrbot/core/provider/sources/openai_rerank_source.py" line_range="94-103" />
<code_context>
+ async with client.post(self.api_url, json=payload) as response:
+ if response.status >= 400:
+ response_text = await response.text()
+ logger.warning(
+ "通用 Rerank API 请求失败: HTTP %s, body snippet: %s",
+ response.status,
+ self._truncate_error_response(response_text),
+ )
+ raise RuntimeError(
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Consider whether logging raw response snippets may leak sensitive data from the rerank provider.
The warning log currently includes up to 200 chars of the raw response body, which may contain query text, document content, or other sensitive data depending on the provider’s error format. Consider either redacting the body, restricting logs to safe structured fields (e.g., JSON error code/message), or moving the raw snippet to a debug-level log instead of warning.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+94
to
+103
| logger.warning( | ||
| f"通用 Rerank top_n={top_n} 超出接口限制，已截断为 {top_k}。" | ||
| ) | ||
| payload["top_k"] = top_k | ||
|
|
||
| return payload | ||
|
|
||
| @staticmethod | ||
| def _parse_results(response_data: Any) -> list[RerankResult]: | ||
| if not isinstance(response_data, dict): |
Contributor
There was a problem hiding this comment.
🚨 suggestion (security): 请考虑记录原始响应片段时，是否可能从 rerank 提供方泄露敏感数据。
当前的 warning 日志会包含最长 200 个字符的原始响应 body，根据提供方的错误格式，这里面可能包含查询文本、文档内容或其他敏感数据。建议要么对 body 做脱敏处理，要么只记录安全的结构化字段（例如 JSON 错误码 / 错误信息），或者将原始片段日志下沉到 debug 级别，而不是 warning。
Original comment in English
🚨 suggestion (security): Consider whether logging raw response snippets may leak sensitive data from the rerank provider.
The warning log currently includes up to 200 chars of the raw response body, which may contain query text, document content, or other sensitive data depending on the provider’s error format. Consider either redacting the body, restricting logs to safe structured fields (e.g., JSON error code/message), or moving the raw snippet to a debug-level log instead of warning.
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
2 participants
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.
新增一个面向 NewAPI 兼容 Rerank 接口的通用重排序供应商适配器，用于解决当前缺少可复用通用适配器的问题。
主要解决以下场景：
同时修正了配置文案，明确说明这里需要填写完整请求 URL，AstrBot 不会自动在末尾补全
/v1/rerank。Modifications / 改动点
在
astrbot/core/provider/sources/openai_rerank_source.py中新增通用 Rerank 适配器实现在
astrbot/core/provider/manager.py中注册新的 Rerank 供应商在
astrbot/core/config/default.py中补充默认配置模板，并更新重排序相关配置提示文案更新前端i18n文案：
dashboard/src/i18n/locales/zh-CN/features/config-metadata.jsondashboard/src/i18n/locales/en-US/features/config-metadata.json在
tests/test_openai_rerank_source.py中补充定向测试This is NOT a breaking change. / 这不是一个破坏性变更。
Screenshots or Test Results / 运行截图或测试结果
验证步骤：
启动填写过后点击测试
测试结果：
Checklist / 检查清单
requirements.txt和pyproject.toml文件相应位置。/ I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations inrequirements.txtandpyproject.toml.Summary by Sourcery
添加一个通用的、兼容 NewAPI 的重排（rerank）提供方适配器，并将其接入到 provider 配置与管理器中。
New Features:
Enhancements:
Documentation:
Tests:
Original summary in English
Summary by Sourcery
Add a generic NewAPI-compatible rerank provider adapter and wire it into the provider configuration and manager.
New Features:
Enhancements:
Documentation:
Tests:
Original summary in English
Summary by Sourcery
添加一个通用的、兼容 NewAPI 的重排（rerank）提供方适配器，并将其接入到 provider 配置与管理器中。
New Features:
Enhancements:
Documentation:
Tests:
Original summary in English
Summary by Sourcery
Add a generic NewAPI-compatible rerank provider adapter and wire it into the provider configuration and manager.
New Features:
Enhancements:
Documentation:
Tests: