fix: resolve /model command misleading behavior when switching to model from different provider#5578
Open
pandyzhou wants to merge 3 commits intoAstrBotDevs:masterfrom
Open
Conversation
…where Made-with: Cursor
dosubot
bot
added
the
size:M
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! 此拉取请求旨在解决 Highlights
Changelog
Activity
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 - 我发现了两个问题，并留下了一些整体性的反馈：
- 在
_find_provider_for_model中，对p.get_models()抛出的异常被静默忽略；建议至少记录提供商 ID 和错误详情，这样在跨提供商解析失败时，更容易诊断配置错误或 API 故障。 - 新的跨提供商查找逻辑会在每次处理
/model <name>字符串输入时，对所有提供商调用一次get_models()；如果get_models()调用成本较高或提供商数量较多，建议增加简单的缓存机制或对查找频率进行限制，避免产生不必要的重复查询。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_find_provider_for_model`, exceptions from `p.get_models()` are silently ignored; consider at least logging provider ID and error details so that misconfigured providers or API failures are easier to diagnose when cross-provider resolution fails.
- The new cross-provider lookup calls `get_models()` on all providers for every `/model <name>` string input; if `get_models()` is expensive or there are many providers, consider adding simple caching or a rate limit on lookups to avoid unnecessary repeated queries.
## Individual Comments
### Comment 1
<location path="astrbot/builtin_stars/builtin_commands/commands/provider.py" line_range="51-68" />
<code_context>
+ *[p.get_models() for p in all_providers],
+ return_exceptions=True,
+ )
+ for provider, result in zip(all_providers, results):
+ if isinstance(result, BaseException):
+ continue
+ provider_id = provider.meta().id
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Swallowing `BaseException` from `get_models()` may hide provider-specific issues.
Since every `BaseException` from `get_models()` is ignored, systemic issues (auth, networking, misconfig) can be silently skipped, making failures hard to diagnose. Please log these exceptions (ideally with the provider id), or at least log when all providers fail to return models so that configuration/runtime problems are visible.
```suggestion
async def _find_provider_for_model(
self, model_name: str, exclude_provider_id: str | None = None
) -> tuple["Provider" | None, str | None]:
"""在所有 LLM 提供商中查找包含指定模型的提供商。返回 (provider, provider_id) 或 (None, None)。"""
all_providers = self.context.get_all_providers()
results = await asyncio.gather(
*[p.get_models() for p in all_providers],
return_exceptions=True,
)
loop = asyncio.get_running_loop()
failed_providers: list[str] = []
for provider, result in zip(all_providers, results):
if isinstance(result, BaseException):
provider_id = provider.meta().id
failed_providers.append(provider_id)
loop.call_exception_handler(
{
"message": "Failed to get models from provider",
"exception": result,
"provider_id": provider_id,
}
)
continue
provider_id = provider.meta().id
if exclude_provider_id and provider_id == exclude_provider_id:
continue
if model_name in result:
return provider, provider_id
# 如果所有提供商都未能返回模型列表，则记录一条系统级错误，便于排查配置/运行时问题
if failed_providers and len(failed_providers) == len(all_providers):
loop.call_exception_handler(
{
"message": "Failed to get models from all providers",
"providers": failed_providers,
}
)
return None, None
```
</issue_to_address>
### Comment 2
<location path="astrbot/builtin_stars/builtin_commands/commands/provider.py" line_range="56-57" />
<code_context>
+ ) -> tuple["Provider" | None, str | None]:
+ """在所有 LLM 提供商中查找包含指定模型的提供商。返回 (provider, provider_id) 或 (None, None)。"""
+ all_providers = self.context.get_all_providers()
+ results = await asyncio.gather(
+ *[p.get_models() for p in all_providers],
+ return_exceptions=True,
+ )
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using `asyncio.gather(..., return_exceptions=True)` without any aggregate check may mask global failures.
With `return_exceptions=True`, if every `get_models()` call fails, `_find_provider_for_model` will return `(None, None)` just like the “model not found” case. Consider detecting when all entries in `results` are exceptions and either raising/logging a clear error so callers can distinguish configuration/connectivity failures from a genuinely missing model.
Suggested implementation:
```python
all_providers = self.context.get_all_providers()
results = await asyncio.gather(
*[p.get_models() for p in all_providers],
return_exceptions=True,
)
# 如果所有调用都失败，则这是配置/连接性问题，而不是单纯的“模型不存在”
if results and all(isinstance(r, Exception) for r in results):
logger = getattr(self.context, "logger", None)
msg = f"Failed to fetch models from all providers when searching for model '{model_name}'"
if logger is not None:
logger.error(msg, extra={"model_name": model_name, "provider_count": len(all_providers), "errors": results})
raise RuntimeError(msg)
```
This change assumes that the rest of `_find_provider_for_model` continues to work with `results` as a list aligned with `all_providers`. If you later refactor to skip individual failing providers, you may want to:
1. Filter out `Exception` entries in `results` when iterating to find the provider for `model_name`.
2. Optionally log per-provider failures (e.g., inside a `for provider, result in zip(all_providers, results)` loop).
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续的评审。
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- In
_find_provider_for_model, exceptions fromp.get_models()are silently ignored; consider at least logging provider ID and error details so that misconfigured providers or API failures are easier to diagnose when cross-provider resolution fails. - The new cross-provider lookup calls
get_models()on all providers for every/model <name>string input; ifget_models()is expensive or there are many providers, consider adding simple caching or a rate limit on lookups to avoid unnecessary repeated queries.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_find_provider_for_model`, exceptions from `p.get_models()` are silently ignored; consider at least logging provider ID and error details so that misconfigured providers or API failures are easier to diagnose when cross-provider resolution fails.
- The new cross-provider lookup calls `get_models()` on all providers for every `/model <name>` string input; if `get_models()` is expensive or there are many providers, consider adding simple caching or a rate limit on lookups to avoid unnecessary repeated queries.
## Individual Comments
### Comment 1
<location path="astrbot/builtin_stars/builtin_commands/commands/provider.py" line_range="51-68" />
<code_context>
+ *[p.get_models() for p in all_providers],
+ return_exceptions=True,
+ )
+ for provider, result in zip(all_providers, results):
+ if isinstance(result, BaseException):
+ continue
+ provider_id = provider.meta().id
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Swallowing `BaseException` from `get_models()` may hide provider-specific issues.
Since every `BaseException` from `get_models()` is ignored, systemic issues (auth, networking, misconfig) can be silently skipped, making failures hard to diagnose. Please log these exceptions (ideally with the provider id), or at least log when all providers fail to return models so that configuration/runtime problems are visible.
```suggestion
async def _find_provider_for_model(
self, model_name: str, exclude_provider_id: str | None = None
) -> tuple["Provider" | None, str | None]:
"""在所有 LLM 提供商中查找包含指定模型的提供商。返回 (provider, provider_id) 或 (None, None)。"""
all_providers = self.context.get_all_providers()
results = await asyncio.gather(
*[p.get_models() for p in all_providers],
return_exceptions=True,
)
loop = asyncio.get_running_loop()
failed_providers: list[str] = []
for provider, result in zip(all_providers, results):
if isinstance(result, BaseException):
provider_id = provider.meta().id
failed_providers.append(provider_id)
loop.call_exception_handler(
{
"message": "Failed to get models from provider",
"exception": result,
"provider_id": provider_id,
}
)
continue
provider_id = provider.meta().id
if exclude_provider_id and provider_id == exclude_provider_id:
continue
if model_name in result:
return provider, provider_id
# 如果所有提供商都未能返回模型列表，则记录一条系统级错误，便于排查配置/运行时问题
if failed_providers and len(failed_providers) == len(all_providers):
loop.call_exception_handler(
{
"message": "Failed to get models from all providers",
"providers": failed_providers,
}
)
return None, None
```
</issue_to_address>
### Comment 2
<location path="astrbot/builtin_stars/builtin_commands/commands/provider.py" line_range="56-57" />
<code_context>
+ ) -> tuple["Provider" | None, str | None]:
+ """在所有 LLM 提供商中查找包含指定模型的提供商。返回 (provider, provider_id) 或 (None, None)。"""
+ all_providers = self.context.get_all_providers()
+ results = await asyncio.gather(
+ *[p.get_models() for p in all_providers],
+ return_exceptions=True,
+ )
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using `asyncio.gather(..., return_exceptions=True)` without any aggregate check may mask global failures.
With `return_exceptions=True`, if every `get_models()` call fails, `_find_provider_for_model` will return `(None, None)` just like the “model not found” case. Consider detecting when all entries in `results` are exceptions and either raising/logging a clear error so callers can distinguish configuration/connectivity failures from a genuinely missing model.
Suggested implementation:
```python
all_providers = self.context.get_all_providers()
results = await asyncio.gather(
*[p.get_models() for p in all_providers],
return_exceptions=True,
)
# 如果所有调用都失败，则这是配置/连接性问题，而不是单纯的“模型不存在”
if results and all(isinstance(r, Exception) for r in results):
logger = getattr(self.context, "logger", None)
msg = f"Failed to fetch models from all providers when searching for model '{model_name}'"
if logger is not None:
logger.error(msg, extra={"model_name": model_name, "provider_count": len(all_providers), "errors": results})
raise RuntimeError(msg)
```
This change assumes that the rest of `_find_provider_for_model` continues to work with `results` as a list aligned with `all_providers`. If you later refactor to skip individual failing providers, you may want to:
1. Filter out `Exception` entries in `results` when iterating to find the provider for `model_name`.
2. Optionally log per-provider failures (e.g., inside a `for provider, result in zip(all_providers, results)` loop).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Related Documentation Checked 1 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
![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 PR addresses the misleading behavior of the /model command when switching models across different providers and introduces automatic cross-provider model lookup and switching, which is a significant improvement. However, it introduces a critical security vulnerability: model selection is not properly isolated between user sessions. Modifying the shared provider instance's state allows one user to change the model for all other users sharing that provider, potentially leading to service disruption or unexpected costs. Additionally, I've provided some refactoring suggestions to further simplify the code logic, enhance efficiency, and improve readability, particularly by integrating model lookup into the _find_provider_for_model method to avoid redundant get_models() calls.
…vider lookup Made-with: Cursor
…ommand Made-with: Cursor
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.
Motivation / 动机
问题：使用
/model <模型名>切换到属于其他提供商的模型时（例如当前为 deepseek，输入gpt-4属于另一提供商），命令会输出「切换模型到 gpt-4。」，看似成功。但会话的provider_perf仍指向原提供商，后续 LLM 请求仍发往 deepseek，导致 "Model Not Exist" 错误，用户难以定位问题。解决：实现智能模型解析——输入模型名时自动在所有提供商中查找，若在其他提供商中找到则自动切换提供商并设置模型，避免误导性成功提示和请求失败。
Modifications / 改动点
修改文件：
astrbot/builtin_stars/builtin_commands/commands/provider.py新增：
_find_provider_for_model()辅助方法，在所有 LLM 提供商中查找包含指定模型的提供商修改：
model_ls中字符串分支逻辑：/provider更新：Tips 文案，说明可自动跨提供商查找并切换
修复：整数分支异常处理中缺少
return，导致错误消息被成功消息覆盖This is NOT a breaking change. / 这不是一个破坏性变更。
Screenshots or Test Results / 运行截图或测试结果
验证步骤：
gpt-4模型的提供商）/model gpt-4Checklist / 检查清单
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
改进
/model命令，以便在不同提供方之间正确解析模型，并在切换模型时避免产生具有误导性的成功消息。Bug 修复：
/model行为：当指定其他提供方的模型时，不再在提供方保持不变的情况下仍声称切换成功。增强功能：
/model的提示文案，说明自动跨提供方查找和自动切换提供方的行为。Original summary in English
Summary by Sourcery
Improve /model command to correctly resolve models across providers and avoid misleading success messages when switching models.
Bug Fixes:
Enhancements: