feat(provider): support reset/default subcommands to clear session override#8752
feat(provider): support reset/default subcommands to clear session override#8752Rat0323 wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The logic for parsing and validating provider indices (including the new
default/reset/clearhandling) is now duplicated across chat, TTS, and STT branches; consider extracting a small helper to centralize the digit conversion, bounds checking, and error messaging so future changes only need to be made in one place. - The three subcommand aliases (
"default","reset","clear") are hardcoded in several separate conditionals; defining a shared constant or helper for the allowed reset commands would reduce the chance of them diverging between provider types over time.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The logic for parsing and validating provider indices (including the new `default/reset/clear` handling) is now duplicated across chat, TTS, and STT branches; consider extracting a small helper to centralize the digit conversion, bounds checking, and error messaging so future changes only need to be made in one place.
- The three subcommand aliases (`"default"`, `"reset"`, `"clear"`) are hardcoded in several separate conditionals; defining a shared constant or helper for the allowed reset commands would reduce the chance of them diverging between provider types over time.
## Individual Comments
### Comment 1
<location path="astrbot/builtin_stars/builtin_commands/commands/provider.py" line_range="195-194" />
<code_context>
)
return
- if idx2 > len(self.context.get_all_tts_providers()) or idx2 < 1:
+ if idx2 in ("default", "reset", "clear"):
+ await sp.session_remove(
+ umo,
+ f"provider_perf_{ProviderType.TEXT_TO_SPEECH.value}",
+ )
+ event.set_result(
+ MessageEventResult().message("✅ Successfully reset TTS provider to global default.")
+ )
+ return
+ try:
+ idx2_int = int(idx2)
</code_context>
<issue_to_address>
**suggestion:** Consider extracting the TTS/STT reset path into a shared helper to avoid divergence.
The TTS and STT reset branches both perform the same sequence: check for "default"/"reset"/"clear", call `session_remove` with a provider-specific key, then return a success message. A shared helper (e.g., `reset_provider(ProviderType, umo, success_message)`) would reduce duplication and keep their behavior consistent when you change the semantics or key format later.
Suggested implementation:
```python
if await reset_provider(
sp=sp,
umo=umo,
provider_type=ProviderType.TEXT_TO_SPEECH,
idx_value=idx2,
event=event,
success_message="✅ Successfully reset TTS provider to global default.",
):
return
try:
```
1. Define the shared helper (either as a module-level coroutine or a private method on the same class) that encapsulates the reset logic used by both TTS and STT branches, for example:
```python
async def reset_provider(
sp,
umo,
provider_type: ProviderType,
idx_value: str | int | None,
event,
success_message: str,
) -> bool:
if idx_value not in ("default", "reset", "clear"):
return False
await sp.session_remove(
umo,
f"provider_perf_{provider_type.value}",
)
event.set_result(
MessageEventResult().message(success_message)
)
return True
```
2. Update the STT reset branch to use this helper in the same way as the TTS branch, for example:
```python
if await reset_provider(
sp=sp,
umo=umo,
provider_type=ProviderType.SPEECH_TO_TEXT,
idx_value=idx2, # or the appropriate variable for STT’s index argument
event=event,
success_message="✅ Successfully reset STT provider to global default.",
):
return
```
3. Ensure any type hints (e.g., `ProviderType`, `MessageEventResult`) and imports remain consistent with the rest of the file.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request adds support for resetting Chat, TTS, and STT providers to their global defaults using "default", "reset", or "clear" subcommands, updates parameter type annotations, and introduces unit tests to verify this behavior. The feedback recommends refactoring the duplicated reset logic into a shared helper method and caching the retrieved provider lists locally to avoid redundant, inefficient method calls.
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.
| if idx2_int > len(self.context.get_all_tts_providers()) or idx2_int < 1: | ||
| event.set_result( | ||
| MessageEventResult().message("❌ Invalid provider index.") | ||
| ) | ||
| return | ||
| provider = self.context.get_all_tts_providers()[idx2 - 1] | ||
| provider = self.context.get_all_tts_providers()[idx2_int - 1] |
There was a problem hiding this comment.
Calling self.context.get_all_tts_providers() multiple times is inefficient and potentially unsafe if it returns a generator or a newly constructed sequence. Converting it to a list once and reusing it is more efficient and robust.
| if idx2_int > len(self.context.get_all_tts_providers()) or idx2_int < 1: | |
| event.set_result( | |
| MessageEventResult().message("❌ Invalid provider index.") | |
| ) | |
| return | |
| provider = self.context.get_all_tts_providers()[idx2 - 1] | |
| provider = self.context.get_all_tts_providers()[idx2_int - 1] | |
| providers = list(self.context.get_all_tts_providers()) | |
| if idx2_int > len(providers) or idx2_int < 1: | |
| event.set_result( | |
| MessageEventResult().message("❌ Invalid provider index.") | |
| ) | |
| return | |
| provider = providers[idx2_int - 1] |
| if idx2_int > len(self.context.get_all_stt_providers()) or idx2_int < 1: | ||
| event.set_result( | ||
| MessageEventResult().message("❌ Invalid provider index.") | ||
| ) | ||
| return | ||
| provider = self.context.get_all_providers()[idx - 1] | ||
| provider = self.context.get_all_stt_providers()[idx2_int - 1] |
There was a problem hiding this comment.
Calling self.context.get_all_stt_providers() multiple times is inefficient and potentially unsafe if it returns a generator or a newly constructed sequence. Converting it to a list once and reusing it is more efficient and robust.
| if idx2_int > len(self.context.get_all_stt_providers()) or idx2_int < 1: | |
| event.set_result( | |
| MessageEventResult().message("❌ Invalid provider index.") | |
| ) | |
| return | |
| provider = self.context.get_all_providers()[idx - 1] | |
| provider = self.context.get_all_stt_providers()[idx2_int - 1] | |
| providers = list(self.context.get_all_stt_providers()) | |
| if idx2_int > len(providers) or idx2_int < 1: | |
| event.set_result( | |
| MessageEventResult().message("❌ Invalid provider index.") | |
| ) | |
| return | |
| provider = providers[idx2_int - 1] |
| if idx_int > len(self.context.get_all_providers()) or idx_int < 1: | ||
| event.set_result( | ||
| MessageEventResult().message("❌ Invalid provider index.") | ||
| ) | ||
| return | ||
| provider = self.context.get_all_providers()[idx_int - 1] |
There was a problem hiding this comment.
Calling self.context.get_all_providers() multiple times is inefficient and potentially unsafe if it returns a generator or a newly constructed sequence. Converting it to a list once and reusing it is more efficient and robust.
| if idx_int > len(self.context.get_all_providers()) or idx_int < 1: | |
| event.set_result( | |
| MessageEventResult().message("❌ Invalid provider index.") | |
| ) | |
| return | |
| provider = self.context.get_all_providers()[idx_int - 1] | |
| providers = list(self.context.get_all_providers()) | |
| if idx_int > len(providers) or idx_int < 1: | |
| event.set_result( | |
| MessageEventResult().message("❌ Invalid provider index.") | |
| ) | |
| return | |
| provider = providers[idx_int - 1] |
Description
Currently, when an administrator runs
/provider <idx>in a session, the session-level provider override is persisted in the database underprovider_perf_<type>.However, there was no command or method to clear/delete this session-level preference once it was set. As a result, even if the administrator changes the default model or provider in the global WebUI configuration, the session remains locked/stuck on the old overridden provider.
This PR implements
default,reset, andclearsubcommands for:/provider(e.g./provider resetor/provider default)/provider tts(e.g./provider tts resetor/provider tts default)/provider stt(e.g./provider stt resetor/provider stt default)When executed, it removes the session override preference using
sp.session_remove(...)so that the session correctly falls back to the default configuration.Proposed Changes
idx2inprovidercommand tostr | int | Noneto support string commands.("default", "reset", "clear")for Chat, TTS, and STT providers.sp.session_removeto delete the preference override keys.tests/unit/test_provider_commands.py.Validation
All 3 new tests in
tests/unit/test_provider_commands.pypass:test_provider_reset_chat_completiontest_provider_reset_ttstest_provider_reset_sttSummary by Sourcery
Add support for resetting session-level provider overrides back to the global defaults for chat, TTS, and STT providers.
New Features:
Enhancements:
Tests: