Merged
Conversation
1. 常见的openai和anthropic协议，如 智谱的codingpan https://open.bigmodel.cn/api/coding/paas/v4 2. 新出的一些没有模型列表的自定义模型提供商，如科大讯飞 https://maas-coding-api.cn-huabei-1.xf-yun.com/v2
dosubot
bot
added
size:S
![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 1 issue, and left some high level feedback:
- Both
anthropic_source._create_http_clientandcreate_proxy_clientnow always return anAsyncClientinstead ofNone; please double-check all call sites that previously handled aNonereturn to avoid behavioral changes or leaked unused clients. - You are creating a new
ssl.create_default_context()in each helper; consider centralizing or reusing a shared SSL context (or providing a small utility) so the behavior is consistent across providers and avoids repeated context construction.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Both `anthropic_source._create_http_client` and `create_proxy_client` now always return an `AsyncClient` instead of `None`; please double-check all call sites that previously handled a `None` return to avoid behavioral changes or leaked unused clients.
- You are creating a new `ssl.create_default_context()` in each helper; consider centralizing or reusing a shared SSL context (or providing a small utility) so the behavior is consistent across providers and avoids repeated context construction.
## Individual Comments
### Comment 1
<location path="astrbot/core/utils/network_utils.py" line_range="103" />
<code_context>
Returns:
- An httpx.AsyncClient configured with the proxy, or None if no proxy
+ An httpx.AsyncClient configured with the proxy and system SSL context
"""
+ ctx = ssl.create_default_context()
</code_context>
<issue_to_address>
**nitpick:** Return description is slightly misleading when no proxy is provided.
The function now always returns a client, even when `proxy` is `None`, but the phrase "configured with the proxy" suggests a proxy is always present. Please reword to reflect that the client is always created with the system SSL context and only applies the proxy when one is provided.
</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}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the HTTP client configuration to use the system SSL certificate store via ssl.create_default_context(), ensuring better compatibility with system-trusted CA chains. These changes are implemented in both the Anthropic provider and the shared network utilities. Feedback suggests refactoring the Anthropic provider to utilize the create_proxy_client utility from network_utils.py to eliminate redundant logic and maintain consistency across providers.
Comment on lines
1
to
5
| import base64 | ||
| import json | ||
| import ssl | ||
| from collections.abc import AsyncGenerator | ||
| from typing import Literal |
Contributor
There was a problem hiding this comment.
既然逻辑已经迁移到 network_utils.py，这里可以移除不再需要的 ssl 导入，并引入 create_proxy_client。
Suggested change
| import base64 | |
| import json | |
| import ssl | |
| from collections.abc import AsyncGenerator | |
| from typing import Literal | |
| import base64 | |
| import json | |
| from astrbot.core.utils.network_utils import create_proxy_client | |
| from collections.abc import AsyncGenerator | |
| from typing import Literal |
Comment on lines
+110
to
+119
| def _create_http_client(self, provider_config: dict) -> httpx.AsyncClient: | ||
| """创建带代理的 HTTP 客户端，使用系统 SSL 证书""" | ||
| ctx = ssl.create_default_context() | ||
| proxy = provider_config.get("proxy", "") | ||
| if proxy: | ||
| logger.info(f"[Anthropic] 使用代理: {proxy}") | ||
| return httpx.AsyncClient(proxy=proxy, headers=self.custom_headers) | ||
| if self.custom_headers: | ||
| return httpx.AsyncClient(headers=self.custom_headers) | ||
| return None | ||
| return httpx.AsyncClient( | ||
| proxy=proxy, headers=self.custom_headers, verify=ctx | ||
| ) | ||
| return httpx.AsyncClient(headers=self.custom_headers, verify=ctx) |
Contributor
There was a problem hiding this comment.
建议使用 network_utils.py 中的 create_proxy_client 辅助函数来替代重复的逻辑。这不仅能减少代码重复，还能确保所有提供商在使用代理和 SSL 配置时保持一致。
def _create_http_client(self, provider_config: dict) -> httpx.AsyncClient:
"""创建带代理的 HTTP 客户端，使用系统 SSL 证书"""
return create_proxy_client(
"Anthropic",
provider_config.get("proxy", ""),
headers=self.custom_headers,
)References
- 当为不同情况实现类似功能时，应将逻辑重构为共享的辅助函数，以避免代码重复。
Contributor
Author
dosubot
bot
added
size:M
Contributor
Author
|
@Soulter 求大佬合并下代码，很重要，我没模型用啦 |
Member
|
@sourcery-ai review |
Contributor
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/utils/network_utils.py" line_range="107" />
<code_context>
- An httpx.AsyncClient configured with the proxy, or None if no proxy
+ An httpx.AsyncClient created with the system SSL context; the proxy is applied only if one is provided.
"""
+ ctx = ssl.create_default_context()
if proxy:
logger.info(f"[{provider_label}] 使用代理: {proxy}")
</code_context>
<issue_to_address>
**suggestion (performance):** Consider reusing a shared SSLContext instead of creating a new one per client
Creating a new `ssl.SSLContext` on each call adds unnecessary overhead if this helper is used often. Consider defining a module-level context (e.g., `_SYSTEM_SSL_CTX = ssl.create_default_context()`) and reusing it for each client, while still relying on the system certificate store.
Suggested implementation:
```python
Returns:
An httpx.AsyncClient created with the shared system SSL context; the proxy is applied only if one is provided.
"""
if proxy:
logger.info(f"[{provider_label}] 使用代理: {proxy}")
return httpx.AsyncClient(proxy=proxy, verify=_SYSTEM_SSL_CTX, headers=headers)
return httpx.AsyncClient(verify=_SYSTEM_SSL_CTX, headers=headers)
```
` since the import section is not visible).
Here are the changes:
<file_operations>
<file_operation operation="edit" file_path="astrbot/core/utils/network_utils.py">
<<<<<<< SEARCH
Returns:
An httpx.AsyncClient created with the system SSL context; the proxy is applied only if one is provided.
"""
ctx = ssl.create_default_context()
if proxy:
logger.info(f"[{provider_label}] 使用代理: {proxy}")
return httpx.AsyncClient(proxy=proxy, verify=ctx, headers=headers)
return httpx.AsyncClient(verify=ctx, headers=headers)
=======
Returns:
An httpx.AsyncClient created with the shared system SSL context; the proxy is applied only if one is provided.
"""
if proxy:
logger.info(f"[{provider_label}] 使用代理: {proxy}")
return httpx.AsyncClient(proxy=proxy, verify=_SYSTEM_SSL_CTX, headers=headers)
return httpx.AsyncClient(verify=_SYSTEM_SSL_CTX, headers=headers)
>>>>>>> REPLACE
</file_operation>
</file_operations>
<additional_changes>
1. At the top of `astrbot/core/utils/network_utils.py`, after the existing `import ssl` (or adding it if missing), define a shared SSL context:
```python
import ssl
_SYSTEM_SSL_CTX = ssl.create_default_context()
```
2. Ensure `_SYSTEM_SSL_CTX` is defined at module scope (not inside any function) so it is created once when the module is imported and then reused by this helper.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
|
@sourcery-ai review |
Contributor
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="tests/unit/test_network_utils.py" line_range="8-17" />
<code_context>
+from astrbot.core.utils import network_utils
+
+
+def test_create_proxy_client_reuses_shared_ssl_context(
+ monkeypatch: pytest.MonkeyPatch,
+):
+ captured_calls: list[dict] = []
+
+ class _FakeAsyncClient:
+ def __init__(self, **kwargs):
+ captured_calls.append(kwargs)
+
+ monkeypatch.setattr(network_utils.httpx, "AsyncClient", _FakeAsyncClient)
+
+ network_utils.create_proxy_client("OpenAI")
+ network_utils.create_proxy_client("OpenAI", proxy="http://127.0.0.1:7890")
+
+ assert len(captured_calls) == 2
+ assert isinstance(captured_calls[0]["verify"], ssl.SSLContext)
+ assert captured_calls[0]["verify"] is captured_calls[1]["verify"]
</code_context>
<issue_to_address>
**suggestion (testing):** Add assertions to verify that `proxy` and `headers` arguments are correctly forwarded to `httpx.AsyncClient`.
This test covers SSL context reuse but doesn’t yet validate that `headers` and `proxy` are forwarded correctly. Please extend it (or add a new test) to assert that:
- When `proxy` is omitted, `AsyncClient` is called without a `proxy` kwarg.
- When `proxy` is provided, `AsyncClient` receives the expected `proxy` value.
- When `headers` is provided to `create_proxy_client`, it is forwarded as the `headers` kwarg.
You can keep using `_FakeAsyncClient` and check `captured_calls[i]["proxy"]` and `captured_calls[i]["headers"]`.
</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
+8
to
+17
| def test_create_proxy_client_reuses_shared_ssl_context( | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| ): | ||
| captured_calls: list[dict] = [] | ||
|
|
||
| class _FakeAsyncClient: | ||
| def __init__(self, **kwargs): | ||
| captured_calls.append(kwargs) | ||
|
|
||
| monkeypatch.setattr(network_utils.httpx, "AsyncClient", _FakeAsyncClient) |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add assertions to verify that proxy and headers arguments are correctly forwarded to httpx.AsyncClient.
This test covers SSL context reuse but doesn’t yet validate that headers and proxy are forwarded correctly. Please extend it (or add a new test) to assert that:
- When
proxyis omitted,AsyncClientis called without aproxykwarg. - When
proxyis provided,AsyncClientreceives the expectedproxyvalue. - When
headersis provided tocreate_proxy_client, it is forwarded as theheaderskwarg.
You can keep using _FakeAsyncClient and check captured_calls[i]["proxy"] and captured_calls[i]["headers"].
Member
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Since
create_proxy_clientnow always returns anhttpx.AsyncClientinstead ofNonewhen no proxy is provided, double-check all call sites that previously handled aNonereturn to ensure they no longer rely on the optional behavior. - Consider allowing
create_proxy_clientto optionally accept a customssl.SSLContextorverifyparameter so callers that need a non-system CA bundle (e.g., on-prem/self-signed environments) can override the shared system context when necessary.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since `create_proxy_client` now always returns an `httpx.AsyncClient` instead of `None` when no proxy is provided, double-check all call sites that previously handled a `None` return to ensure they no longer rely on the optional behavior.
- Consider allowing `create_proxy_client` to optionally accept a custom `ssl.SSLContext` or `verify` parameter so callers that need a non-system CA bundle (e.g., on-prem/self-signed environments) can override the shared system context when necessary.
## Individual Comments
### Comment 1
<location path="tests/unit/test_network_utils.py" line_range="20-22" />
<code_context>
+
+ monkeypatch.setattr(network_utils.httpx, "AsyncClient", _FakeAsyncClient)
+
+ network_utils.create_proxy_client("OpenAI")
+ network_utils.create_proxy_client("OpenAI", proxy="http://127.0.0.1:7890")
+ network_utils.create_proxy_client("OpenAI", headers=headers)
+
+ assert len(captured_calls) == 3
</code_context>
<issue_to_address>
**suggestion (testing):** Add an explicit test for an empty proxy string to match how `provider_config.get("proxy", "")` is used.
Since `create_proxy_client` is frequently called with `provider_config.get("proxy", "")`, it can receive an empty string, which the helper treats as "no proxy". Please add a case like `create_proxy_client("OpenAI", proxy="")` and assert it matches the no-proxy behavior (no `proxy` in kwargs, same `verify` handling) so tests cover this usage explicitly.
</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
+20
to
+22
| network_utils.create_proxy_client("OpenAI") | ||
| network_utils.create_proxy_client("OpenAI", proxy="http://127.0.0.1:7890") | ||
| network_utils.create_proxy_client("OpenAI", headers=headers) |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add an explicit test for an empty proxy string to match how provider_config.get("proxy", "") is used.
Since create_proxy_client is frequently called with provider_config.get("proxy", ""), it can receive an empty string, which the helper treats as "no proxy". Please add a case like create_proxy_client("OpenAI", proxy="") and assert it matches the no-proxy behavior (no proxy in kwargs, same verify handling) so tests cover this usage explicitly.
Contributor
Author
|
@zouyonghe 大佬为啥不直接合并我的代码呀，我目前只能这样改了后本地编译使用了，官方版这个坑不修复一堆模型都不能用 |
Member
|
@sourcery-ai review |
|
Hi @zouyonghe! 👋 Only authors and team members can run @sourcery-ai commands on public repos. If you are a team member, install the @sourcery-ai bot to get access ✨ |
Member
|
先review一下，问题应该不大 |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Now that
create_proxy_clientcentralizes client construction, consider allowing additionalhttpx.AsyncClientoptions (e.g., timeouts, HTTP/2, limits) to be passed through so other providers don’t need to bypass this helper for more advanced configurations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Now that `create_proxy_client` centralizes client construction, consider allowing additional `httpx.AsyncClient` options (e.g., timeouts, HTTP/2, limits) to be passed through so other providers don’t need to bypass this helper for more advanced configurations.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
感谢感谢 |
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
3 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.
Modifications / 改动点
Screenshots or Test Results / 运行截图或测试结果
Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能，已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试，并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库，或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Use a shared system SSL context for HTTP clients to avoid TLS verification issues with certain model providers while standardizing proxy-aware client creation.
Bug Fixes:
Enhancements:
Tests: