Skip to content

Conversation

@Soulter
Copy link
Member

@Soulter Soulter commented Jan 26, 2026

Modifications / 改动点

  • This is NOT a breaking change. / 这不是一个破坏性变更。

Screenshots or Test Results / 运行截图或测试结果


Checklist / 检查清单

  • 😊 如果 PR 中有新加入的功能,已经通过 Issue / 邮件等方式和作者讨论过。/ If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
  • 👀 我的更改经过了良好的测试,并已在上方提供了“验证步骤”和“运行截图”。/ My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
  • 🤓 我确保没有引入新依赖库,或者引入了新依赖库的同时将其添加到了 requirements.txtpyproject.toml 文件相应位置。/ 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.txt and pyproject.toml.
  • 😮 我的更改没有引入恶意代码。/ My changes do not introduce malicious code.

Summary by Sourcery

新功能:

  • 在提供方配置中,支持从环境变量中解析聊天补全提供方密钥列表。
Original summary in English

Summary by Sourcery

New Features:

  • Support resolving chat completion provider key lists from environment variables in provider configuration.

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Jan 26, 2026
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - 我发现了 1 个问题,并留下了一些总体反馈:

  • key 中引用的环境变量缺失时,你目前会用空字符串替代,这可能会导致下游出现难以调试的错误;可以考虑保留原始的 $VAR 占位符、跳过该 key,或者直接快速失败,并提供更清晰的错误路径。
  • _resolve_env_key_list 辅助函数会就地修改 provider_config,同时又将其返回;为了提高清晰性并避免意外复用引发的 bug,可以考虑改为返回一个新的字典,或者通过文档说明/重命名该方法,使其就地修改的行为更加明确。
给 AI Agent 的提示词
Please address the comments from this code review:

## Overall Comments
-`key` 中引用的环境变量缺失时,你目前会用空字符串替代,这可能会导致下游出现难以调试的错误;可以考虑保留原始的 `$VAR` 占位符、跳过该 key,或者直接快速失败,并提供更清晰的错误路径。
- `_resolve_env_key_list` 辅助函数会就地修改 `provider_config`,同时又将其返回;为了提高清晰性并避免意外复用引发的 bug,可以考虑改为返回一个新的字典,或者通过文档说明/重命名该方法,使其就地修改的行为更加明确。

## Individual Comments

### Comment 1
<location> `astrbot/core/provider/manager.py:422-427` </location>
<code_context>
+                    env_key = env_key[1:-1]
+                if env_key:
+                    env_val = os.getenv(env_key)
+                    if env_val is None:
+                        provider_id = provider_config.get("id")
+                        logger.warning(
+                            f"Provider {provider_id} 配置项 key[{idx}] 使用环境变量 {env_key} 但未设置。",
+                        )
+                        resolved_keys.append("")
+                    else:
+                        resolved_keys.append(env_val)
</code_context>

<issue_to_address>
**suggestion (bug_risk):** 使用空字符串来表示缺失的环境变量可能会隐藏配置错误,并且难以调试。

在这里使用空字符串会把一个清晰的配置错误变成一个隐蔽的运行时 bug(例如,使用空白 API key 发送请求)。建议改为快速失败:要么保留占位符、要么抛出错误、要么跳过这个 key,从而让缺失的环境变量能被清晰地暴露出来。

Suggested implementation:

```python
                    env_val = os.getenv(env_key)
                    if env_val is None:
                        provider_id = provider_config.get("id")
                        logger.error(
                            f"Provider {provider_id} 配置项 key[{idx}] 使用环境变量 {env_key} 但未设置,无法继续加载该 provider。",
                        )
                        raise RuntimeError(
                            f"Missing required environment variable '{env_key}' for provider '{provider_id}' key[{idx}]"
                        )
                    else:
                        resolved_keys.append(env_val)

```

如果在更高层级对 provider 加载有统一的错误处理(例如捕获异常并将 provider 标记为无效),你可能需要确保这一 `RuntimeError` 要么被向上抛出(从而导致启动失败),要么以一种能清楚报告是哪个 provider 加载失败以及失败原因的方式进行处理。
</issue_to_address>

Sourcery 对开源项目免费使用——如果你觉得我们的评审有帮助,欢迎分享 ✨
帮我变得更有用!请在每条评论上点 👍 或 👎,我会根据你的反馈改进后续评审。
Original comment in English

Hey - I've found 1 issue, and left some high level feedback:

  • When an environment variable referenced in key is missing you currently substitute an empty string, which can lead to hard-to-debug downstream failures; consider either leaving the original $VAR token, skipping that key, or failing fast with a clearer error path.
  • The _resolve_env_key_list helper mutates provider_config in place and also returns it; for clarity and to avoid accidental reuse bugs, consider either returning a new dict or documenting/renaming the method to make its in-place behavior explicit.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- When an environment variable referenced in `key` is missing you currently substitute an empty string, which can lead to hard-to-debug downstream failures; consider either leaving the original `$VAR` token, skipping that key, or failing fast with a clearer error path.
- The `_resolve_env_key_list` helper mutates `provider_config` in place and also returns it; for clarity and to avoid accidental reuse bugs, consider either returning a new dict or documenting/renaming the method to make its in-place behavior explicit.

## Individual Comments

### Comment 1
<location> `astrbot/core/provider/manager.py:422-427` </location>
<code_context>
+                    env_key = env_key[1:-1]
+                if env_key:
+                    env_val = os.getenv(env_key)
+                    if env_val is None:
+                        provider_id = provider_config.get("id")
+                        logger.warning(
+                            f"Provider {provider_id} 配置项 key[{idx}] 使用环境变量 {env_key} 但未设置。",
+                        )
+                        resolved_keys.append("")
+                    else:
+                        resolved_keys.append(env_val)
</code_context>

<issue_to_address>
**suggestion (bug_risk):** Using an empty string for missing env vars may hide configuration errors and be hard to debug.

Using an empty string here can turn a clear configuration error into a subtle runtime bug (e.g., requests sent with a blank API key). Consider failing fast instead: either keep the placeholder, raise an error, or skip this key so missing env vars are clearly surfaced.

Suggested implementation:

```python
                    env_val = os.getenv(env_key)
                    if env_val is None:
                        provider_id = provider_config.get("id")
                        logger.error(
                            f"Provider {provider_id} 配置项 key[{idx}] 使用环境变量 {env_key} 但未设置,无法继续加载该 provider。",
                        )
                        raise RuntimeError(
                            f"Missing required environment variable '{env_key}' for provider '{provider_id}' key[{idx}]"
                        )
                    else:
                        resolved_keys.append(env_val)

```

If there is any higher-level error handling for provider loading (e.g., catching exceptions to mark a provider as invalid), you may want to ensure this `RuntimeError` is either propagated up (to fail startup) or handled in a way that clearly reports which provider failed to load and why.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@dosubot dosubot bot added the area:provider The bug / feature is about AI Provider, Models, LLM Agent, LLM Agent Runner. label Jan 26, 2026
@Soulter Soulter merged commit a41391f into master Jan 26, 2026
6 checks passed
@Soulter Soulter deleted the provider-env-key branch January 26, 2026 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:provider The bug / feature is about AI Provider, Models, LLM Agent, LLM Agent Runner. size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants