fix: 插件依赖自动安装逻辑与 Dashboard 安装体验优化#5954
Open
zouyonghe wants to merge 31 commits intoAstrBotDevs:masterfrom
Open
fix: 插件依赖自动安装逻辑与 Dashboard 安装体验优化#5954zouyonghe wants to merge 31 commits intoAstrBotDevs:masterfrom
zouyonghe wants to merge 31 commits intoAstrBotDevs:masterfrom
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! This pull request significantly enhances the stability and usability of plugin management by ensuring that all plugin dependencies are installed at the appropriate lifecycle stages. It also improves the flexibility and robustness of the pip installer by allowing more natural and varied input formats for package specifications, making the system more resilient to user input and ensuring a smoother plugin experience. 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
feature:plugin
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - 我发现了 3 个问题
给 AI Agent 的提示
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/core/utils/pip_installer.py" line_range="137-146" />
<code_context>
return names
+def _split_package_install_input(raw_input: str) -> list[str]:
+ normalized = raw_input.strip()
+ if not normalized:
+ return []
+
+ if "\n" in normalized or "\r" in normalized:
+ return _split_multiline_package_input(normalized)
+
+ if _is_valid_install_requirement(normalized):
+ return [normalized]
+
+ split_tokens = shlex.split(normalized)
+ if split_tokens and all(_is_valid_install_token(token) for token in split_tokens):
+ return split_tokens
</code_context>
<issue_to_address>
**issue (bug_risk):** 带有选项和参数的单行输入（例如 `-e .` 或 `--index-url URL`）可能被错误分类，并作为单个 pip 参数传入。
由于 `_is_valid_install_token` 只会自动接受以 `-` 开头的 token，其他情况则要求能被 `Requirement` 解析，因此任何选项+值的组合（例如 `-e .`、`--index-url https://…`）都会在 `all(_is_valid_install_token(...))` 检查中失败。然后代码会回退为 `[normalized]`，这样 pip 会看到一个带空格的单一参数（`"-e ."`），这是无效的。为避免这种情况，可以放宽对选项参数的校验（允许选项+值对），或者只要输入是以 `-` 开头，就跳过逐 token 的 requirement 校验，直接使用 `shlex.split` 的结果。
</issue_to_address>
### Comment 2
<location path="astrbot/core/utils/pip_installer.py" line_range="163-167" />
<code_context>
+ return True
+
+
+def _is_valid_install_token(token: str) -> bool:
+ if token.startswith("-"):
+ return True
+
+ return _is_valid_install_requirement(token)
+
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** 类路径或基于文件的规格（例如 `.` 或 `../pkg`）即使和合法选项一起使用，也会被拒绝为无效 token。
由于非选项 token 会传给 `_is_valid_install_requirement`，像 `.` 或 `../pkg` 这样的路径值会被判定为无效，从而导致整个拆分结果被丢弃。这会破坏可编辑安装（`-e .`）以及类似“选项 + 路径”的用法。可以考虑对一些常见模式做特殊处理（例如 `-e` 后面的 `.`），或者对明显是路径/URL 的 token 放宽验证。另一种选择是：仅在单包规格时做严格校验，而对任意包含 `-` 的片段，将其视为完整的 pip CLI 调用并原样透传。
</issue_to_address>
### Comment 3
<location path="tests/test_pip_installer.py" line_range="127-136" />
<code_context>
+
+
+@pytest.mark.asyncio
+async def test_install_keeps_single_requirement_with_version_range_intact(monkeypatch):
+ run_pip = AsyncMock(return_value=0)
+
+ monkeypatch.setattr(PipInstaller, "_run_pip_in_process", run_pip)
+
+ installer = PipInstaller("")
+ await installer.install(package_name="demo-package >= 1.0, < 2.0")
+
+ run_pip.assert_awaited_once()
+ recorded_args = run_pip.await_args_list[0].args[0]
+
+ assert recorded_args[0:2] == ["install", "demo-package >= 1.0, < 2.0"]
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** 可以考虑为“部分无效”的、以空格分隔的输入添加一个回归测试，以验证回退行为。
当前测试覆盖了合法的空格分隔输入、多行输入以及单条复杂 requirement 字符串，但从未触发 `_split_package_install_input` 遇到无效 token、并回退为将整个字符串视作一个规格的分支。请添加一个带混合/部分无效字符串的回归测试（例如 `"demo-package another package with spaces"`），以确认我们会回退为 `[raw_input]`，而不是丢弃或错误拆分 token。
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进评审质量。
Original comment in English
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/core/utils/pip_installer.py" line_range="137-146" />
<code_context>
return names
+def _split_package_install_input(raw_input: str) -> list[str]:
+ normalized = raw_input.strip()
+ if not normalized:
+ return []
+
+ if "\n" in normalized or "\r" in normalized:
+ return _split_multiline_package_input(normalized)
+
+ if _is_valid_install_requirement(normalized):
+ return [normalized]
+
+ split_tokens = shlex.split(normalized)
+ if split_tokens and all(_is_valid_install_token(token) for token in split_tokens):
+ return split_tokens
</code_context>
<issue_to_address>
**issue (bug_risk):** Single-line input with options and arguments (e.g. `-e .` or `--index-url URL`) can be misclassified and passed as a single pip arg.
Because `_is_valid_install_token` only auto-accepts tokens starting with `-` and otherwise requires them to be `Requirement`-parsable, any option+value pair (e.g. `-e .`, `--index-url https://…`) fails the `all(_is_valid_install_token(...))` check. The code then falls back to `[normalized]`, so pip sees a single argument with spaces (`"-e ."`), which is invalid. To avoid this, either relax validation for option arguments (so option+value pairs are allowed) or bypass per-token requirement validation whenever the input starts with `-` and just use `shlex.split` as-is.
</issue_to_address>
### Comment 2
<location path="astrbot/core/utils/pip_installer.py" line_range="163-167" />
<code_context>
+ return True
+
+
+def _is_valid_install_token(token: str) -> bool:
+ if token.startswith("-"):
+ return True
+
+ return _is_valid_install_requirement(token)
+
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Path-like or file-based specs (e.g. `.` or `../pkg`) are rejected as invalid tokens even when used with valid options.
Since non-option tokens are passed to `_is_valid_install_requirement`, path-like values such as `.` or `../pkg` will be rejected and the whole split discarded. This breaks editable installs (`-e .`) and similar option+path usages. Consider either special-casing common patterns (e.g. `.` after `-e`) or relaxing validation for tokens that clearly look like paths/URLs. Another option is to apply strict validation only for single-package specs and otherwise treat any fragment containing `-` as a full pip CLI invocation to pass through unchanged.
</issue_to_address>
### Comment 3
<location path="tests/test_pip_installer.py" line_range="127-136" />
<code_context>
+
+
+@pytest.mark.asyncio
+async def test_install_keeps_single_requirement_with_version_range_intact(monkeypatch):
+ run_pip = AsyncMock(return_value=0)
+
+ monkeypatch.setattr(PipInstaller, "_run_pip_in_process", run_pip)
+
+ installer = PipInstaller("")
+ await installer.install(package_name="demo-package >= 1.0, < 2.0")
+
+ run_pip.assert_awaited_once()
+ recorded_args = run_pip.await_args_list[0].args[0]
+
+ assert recorded_args[0:2] == ["install", "demo-package >= 1.0, < 2.0"]
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider a regression test for partially-invalid space-separated input to verify fallback behavior.
Current tests cover valid space-separated input, multiline input, and single complex requirement strings, but they never exercise the branch where `_split_package_install_input` encounters an invalid token and falls back to treating the whole string as one spec. Please add a regression test with a mixed/partially invalid string (e.g. `"demo-package another package with spaces"`) to confirm we fall back to `[raw_input]` instead of dropping or mis-splitting tokens.
</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 effectively addresses the issue of installing plugin dependencies before loading them, covering various scenarios such as new installations, installations from zip files, reloading after failures, and updates. While the code refactoring is clear, the pip command input parsing is more robust, and new regression tests improve quality, two critical security issues were identified in the pip_installer.py file. These include the logging of sensitive information (credentials in URLs) when executing pip commands and potential pip option injection if the dashboard input is not properly restricted. Addressing these vulnerabilities is crucial to prevent credential leakage and unauthorized code execution. A minor issue regarding exception handling was also noted in the specific comments.
astrbot/core/utils/pip_installer.py
Outdated
Comment on lines
+596
to
+597
| package_specs = _split_package_install_input(package_name) | ||
| args.extend(package_specs) |
Contributor
There was a problem hiding this comment.
The PipInstaller.install method now allows for arbitrary pip options to be injected if the package_name input is user-controlled (e.g., from a dashboard). An attacker could use options like --index-url or --extra-index-url to redirect package installation to a malicious server, or use other options to execute arbitrary code during the installation process. This is a significant increase in attack surface compared to the previous implementation which only allowed a single package name. It is recommended to validate that each token in the input is a valid package name and not an option, unless the option is explicitly allowed and sanitized.
astrbot/core/utils/pip_installer.py
Outdated
| args.extend(self.pip_install_arg.split()) | ||
|
|
||
| logger.info(f"Pip 包管理器: pip {' '.join(args)}") | ||
| logger.info("Pip 包管理器 argv: %s", ["pip", *args]) |
Contributor
There was a problem hiding this comment.
The code logs the full list of arguments passed to the pip command. These arguments can include URLs with embedded credentials (e.g., https://user:password@repo.com) or sensitive API tokens if they are provided as part of the package name or pip options. This can lead to the exposure of secrets in the application logs. It is recommended to sanitize the arguments before logging, for example by masking credentials in URLs using a regular expression.
astrbot/core/star/star_manager.py
Outdated
Comment on lines
+507
to
+508
| plugin_path = os.path.join(self.plugin_store_path, dir_name) | ||
| await self._install_plugin_requirements(plugin_path, dir_name) |
Contributor
There was a problem hiding this comment.
reload_failed_plugin 方法的返回类型协定是 (success, error_message) 元组，但对 _install_plugin_requirements 的调用缺少异常处理。如果依赖安装失败导致 _install_plugin_requirements 抛出异常，整个方法会直接崩溃，而不是按预期返回 (False, error_message)。这破坏了该方法的健壮性。建议添加 try...except 块来捕获此异常，并以约定的方式返回错误信息。
Suggested change
| plugin_path = os.path.join(self.plugin_store_path, dir_name) | |
| await self._install_plugin_requirements(plugin_path, dir_name) | |
| try: | |
| plugin_path = os.path.join(self.plugin_store_path, dir_name) | |
| await self._install_plugin_requirements(plugin_path, dir_name) | |
| except Exception as e: | |
| error_msg = f"安装插件 {dir_name} 的依赖失败: {e!s}" | |
| logger.error(error_msg) | |
| return False, error_msg |
dosubot
bot
added
size:XL
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - 我发现了 2 个问题，并给出了一些整体性的反馈：
- 在
reload_failed_plugin中，你现在调用_install_plugin_requirements时没有使用try/except，这与_check_plugin_dept_update不一致，并且意味着一旦依赖安装出错，会直接中断整个 reload 流程；建议将该调用也包在同样的错误日志模式中，这样有问题的 requirements 安装就不会意外破坏恢复逻辑。 _package_specs_override_index中用于检测 index 覆盖的逻辑并不能识别像-ihttps://example.com/simple这样的短格式选项，因此用户有可能以为自己已经指定了自定义 index，但实际上仍然使用的是你的默认 index；建议同时处理这种-i<url>形式的合并写法。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `reload_failed_plugin` you now call `_install_plugin_requirements` without a `try/except`, which is inconsistent with `_check_plugin_dept_update` and means a dependency install error will abort the reload path entirely; consider wrapping this call in the same error-logging pattern so a bad requirements install doesn’t unexpectedly break recovery logic.
- The index override detection in `_package_specs_override_index` doesn’t catch short-form options like `-ihttps://example.com/simple`, so users can still inadvertently get your default index even when they think they’ve specified their own; consider handling the concatenated `-i<url>` form as well.
## Individual Comments
### Comment 1
<location path="astrbot/core/utils/pip_installer.py" line_range="137-146" />
<code_context>
return names
+def _split_package_install_input(raw_input: str) -> list[str]:
+ normalized = raw_input.strip()
+ if not normalized:
+ return []
+
+ if "\n" in normalized or "\r" in normalized:
+ return _split_multiline_package_input(normalized)
+
+ if _is_valid_install_requirement(normalized):
+ return [normalized]
+
+ split_tokens = shlex.split(normalized)
</code_context>
<issue_to_address>
**issue (bug_risk):** Single-line inputs with inline comments are treated as a single invalid requirement rather than being parsed token-wise.
For a single-line input like `"requests==2.31.0 # latest"`, we first fail `_is_valid_install_requirement`, then `shlex.split` returns tokens where the `#`/comment part is not a valid requirement. Because `all(_is_valid_install_token(...))` then fails, we fall back to returning `[normalized]`, sending the full string (including the comment) to pip and likely causing a parse error. Multiline inputs already strip `#` comments, but single-line inputs don’t. Consider either reusing the multiline comment-stripping for single-line inputs or relaxing the `all(...)` condition so we still return the split tokens even if one token is invalid, letting pip handle it.
</issue_to_address>
### Comment 2
<location path="tests/test_pip_installer.py" line_range="272-281" />
<code_context>
+
+
+@pytest.mark.asyncio
+async def test_install_falls_back_to_raw_input_for_invalid_token_string(monkeypatch):
+ run_pip = AsyncMock(return_value=0)
+
+ monkeypatch.setattr(PipInstaller, "_run_pip_in_process", run_pip)
+
+ installer = PipInstaller("")
+ raw_input = "demo-package !!! another-package"
+ await installer.install(package_name=raw_input)
+
+ run_pip.assert_awaited_once()
+ recorded_args = run_pip.await_args_list[0].args[0]
+
+ assert recorded_args[0:2] == ["install", raw_input]
</code_context>
<issue_to_address>
**suggestion (testing):** You may want a small test for empty/whitespace-only package input to lock in `_split_package_install_input` behavior.
One remaining gap is when `package_name` is empty or whitespace-only:
- `_split_package_install_input` returns `[]`.
- `install` then skips the `package_name` branch and, without a `requirements_path`, effectively runs a bare `pip install` with only index args.
A test like `test_install_ignores_empty_package_string` (e.g. `package_name=' '`) that asserts only index-related args are passed would lock in this behavior and catch regressions in input normalization.
</issue_to_address>Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- In
reload_failed_pluginyou now call_install_plugin_requirementswithout atry/except, which is inconsistent with_check_plugin_dept_updateand means a dependency install error will abort the reload path entirely; consider wrapping this call in the same error-logging pattern so a bad requirements install doesn’t unexpectedly break recovery logic. - The index override detection in
_package_specs_override_indexdoesn’t catch short-form options like-ihttps://example.com/simple, so users can still inadvertently get your default index even when they think they’ve specified their own; consider handling the concatenated-i<url>form as well.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `reload_failed_plugin` you now call `_install_plugin_requirements` without a `try/except`, which is inconsistent with `_check_plugin_dept_update` and means a dependency install error will abort the reload path entirely; consider wrapping this call in the same error-logging pattern so a bad requirements install doesn’t unexpectedly break recovery logic.
- The index override detection in `_package_specs_override_index` doesn’t catch short-form options like `-ihttps://example.com/simple`, so users can still inadvertently get your default index even when they think they’ve specified their own; consider handling the concatenated `-i<url>` form as well.
## Individual Comments
### Comment 1
<location path="astrbot/core/utils/pip_installer.py" line_range="137-146" />
<code_context>
return names
+def _split_package_install_input(raw_input: str) -> list[str]:
+ normalized = raw_input.strip()
+ if not normalized:
+ return []
+
+ if "\n" in normalized or "\r" in normalized:
+ return _split_multiline_package_input(normalized)
+
+ if _is_valid_install_requirement(normalized):
+ return [normalized]
+
+ split_tokens = shlex.split(normalized)
</code_context>
<issue_to_address>
**issue (bug_risk):** Single-line inputs with inline comments are treated as a single invalid requirement rather than being parsed token-wise.
For a single-line input like `"requests==2.31.0 # latest"`, we first fail `_is_valid_install_requirement`, then `shlex.split` returns tokens where the `#`/comment part is not a valid requirement. Because `all(_is_valid_install_token(...))` then fails, we fall back to returning `[normalized]`, sending the full string (including the comment) to pip and likely causing a parse error. Multiline inputs already strip `#` comments, but single-line inputs don’t. Consider either reusing the multiline comment-stripping for single-line inputs or relaxing the `all(...)` condition so we still return the split tokens even if one token is invalid, letting pip handle it.
</issue_to_address>
### Comment 2
<location path="tests/test_pip_installer.py" line_range="272-281" />
<code_context>
+
+
+@pytest.mark.asyncio
+async def test_install_falls_back_to_raw_input_for_invalid_token_string(monkeypatch):
+ run_pip = AsyncMock(return_value=0)
+
+ monkeypatch.setattr(PipInstaller, "_run_pip_in_process", run_pip)
+
+ installer = PipInstaller("")
+ raw_input = "demo-package !!! another-package"
+ await installer.install(package_name=raw_input)
+
+ run_pip.assert_awaited_once()
+ recorded_args = run_pip.await_args_list[0].args[0]
+
+ assert recorded_args[0:2] == ["install", raw_input]
</code_context>
<issue_to_address>
**suggestion (testing):** You may want a small test for empty/whitespace-only package input to lock in `_split_package_install_input` behavior.
One remaining gap is when `package_name` is empty or whitespace-only:
- `_split_package_install_input` returns `[]`.
- `install` then skips the `package_name` branch and, without a `requirements_path`, effectively runs a bare `pip install` with only index args.
A test like `test_install_ignores_empty_package_string` (e.g. `package_name=' '`) that asserts only index-related args are passed would lock in this behavior and catch regressions in input normalization.
</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
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/core/utils/pip_installer.py" line_range="137" />
<code_context>
return names
+def _split_package_install_input(raw_input: str) -> list[str]:
+ normalized = raw_input.strip()
+ if not normalized:
</code_context>
<issue_to_address>
**issue (complexity):** 请考虑通过使索引覆盖检查更加显式、内联简单的辅助函数，以及集中处理行清理逻辑来简化新的 pip 参数解析辅助函数，从而降低理解复杂度。
你可以在不改变行为的前提下降低这里的认知负担，方式包括：
1. **让 `_package_specs_override_index` 逻辑线性且显式**
语义保持不变，但更容易浏览和理解：
```python
def _package_specs_override_index(package_specs: list[str]) -> bool:
for spec in package_specs:
if spec in {"-i", "--index-url"}:
return True
if spec.startswith("--index-url="):
return True
if spec.startswith("-i"):
return True
return False
```
这样可以保持原有逻辑（`spec.startswith("-i")` 仍然会匹配 `-ignore-something`），但去掉了复合的 `any(...)` 表达式。
---
2. **移除对 `_is_valid_install_token` 的细微间接调用**
`_is_valid_install_token` 是一个只在单一位置使用的一行函数。将其逻辑内联可以减少在辅助函数之间跳转，同时不改变行为：
```python
def _split_package_install_input(raw_input: str) -> list[str]:
normalized = raw_input.strip()
if not normalized:
return []
if "\n" in normalized or "\r" in normalized:
return _split_multiline_package_input(normalized)
normalized = _strip_inline_requirement_comment(normalized)
if not normalized:
return []
if _is_valid_install_requirement(normalized):
return [normalized]
split_tokens = shlex.split(normalized)
if split_tokens and split_tokens[0].startswith("-"):
return split_tokens
if split_tokens and all(
token.startswith("-") or _is_valid_install_requirement(token)
for token in split_tokens
):
return split_tokens
return [normalized]
```
然后你就可以完全删除 `_is_valid_install_token`。
---
3. **为单行和多行输入共享“去注释/拆分”的处理路径**
你已经有 `_strip_inline_requirement_comment`；可以把通用的“逐行 -> 清洗后的候选值”逻辑集中到一起，并在 `_split_package_install_input` 和 `_split_multiline_package_input` 中复用，使行为在单一位置定义：
```python
def _iter_clean_requirement_lines(raw_input: str) -> list[str]:
for line in raw_input.splitlines():
candidate = _strip_inline_requirement_comment(line)
if not candidate or candidate.startswith("#"):
continue
yield candidate
def _split_multiline_package_input(raw_input: str) -> list[str]:
requirements: list[str] = []
for candidate in _iter_clean_requirement_lines(raw_input):
if candidate.startswith("-"):
requirements.extend(shlex.split(candidate))
else:
requirements.append(candidate)
return requirements
```
这样 `_split_package_install_input` 的多行处理路径就可以变成：
```python
if "\n" in normalized or "\r" in normalized:
return _split_multiline_package_input(normalized)
```
行为保持不变，但逐行清理和注释处理的规则集中在一个辅助函数中，使整体流程更易理解。
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据这些反馈改进后续的评审。
Original comment in English
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/pip_installer.py" line_range="137" />
<code_context>
return names
+def _split_package_install_input(raw_input: str) -> list[str]:
+ normalized = raw_input.strip()
+ if not normalized:
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the new pip argument‑parsing helpers by making index override checks explicit, inlining trivial helpers, and centralizing line-cleaning logic to reduce cognitive load.
You can reduce the cognitive load here without changing behavior by:
1. **Making `_package_specs_override_index` linear and explicit**
Same semantics, but easier to scan and reason about:
```python
def _package_specs_override_index(package_specs: list[str]) -> bool:
for spec in package_specs:
if spec in {"-i", "--index-url"}:
return True
if spec.startswith("--index-url="):
return True
if spec.startswith("-i"):
return True
return False
```
This keeps the exact logic (`spec.startswith("-i")` still matches `-ignore-something`) but removes the compound `any(...)` expression.
---
2. **Removing tiny indirection for `_is_valid_install_token`**
`_is_valid_install_token` is a one-liner used only in one place. Inlining the logic reduces helper hopping without changing behavior:
```python
def _split_package_install_input(raw_input: str) -> list[str]:
normalized = raw_input.strip()
if not normalized:
return []
if "\n" in normalized or "\r" in normalized:
return _split_multiline_package_input(normalized)
normalized = _strip_inline_requirement_comment(normalized)
if not normalized:
return []
if _is_valid_install_requirement(normalized):
return [normalized]
split_tokens = shlex.split(normalized)
if split_tokens and split_tokens[0].startswith("-"):
return split_tokens
if split_tokens and all(
token.startswith("-") or _is_valid_install_requirement(token)
for token in split_tokens
):
return split_tokens
return [normalized]
```
Then you can drop `_is_valid_install_token` completely.
---
3. **Sharing the comment‑stripping/splitting path for single‑line and multi‑line input**
You already have `_strip_inline_requirement_comment`; you can centralize the common “per-line -> cleaned candidate” logic and reuse it in both `_split_package_install_input` and `_split_multiline_package_input` so the behavior is defined in one place:
```python
def _iter_clean_requirement_lines(raw_input: str) -> list[str]:
for line in raw_input.splitlines():
candidate = _strip_inline_requirement_comment(line)
if not candidate or candidate.startswith("#"):
continue
yield candidate
def _split_multiline_package_input(raw_input: str) -> list[str]:
requirements: list[str] = []
for candidate in _iter_clean_requirement_lines(raw_input):
if candidate.startswith("-"):
requirements.extend(shlex.split(candidate))
else:
requirements.append(candidate)
return requirements
```
Then `_split_package_install_input`’s multiline path becomes:
```python
if "\n" in normalized or "\r" in normalized:
return _split_multiline_package_input(normalized)
```
Behavior stays the same, but the rules for per-line cleaning and comment handling live in one helper, which makes the flow easier to follow.
</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
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并给出了一些高层反馈：
- 在
_strip_inline_requirement_comment中，按[ \t]+#进行 split 会把 URL 中合法的#片段也去掉（例如：--extra-index-url https://example.com/simple#frag）；建议将注释剥离限制在“不是选项”的行上，或者使用更精确的启发式规则。 - 新的
_install_plugin_requirementshelper 在部分调用路径（_check_plugin_dept_update、reload_failed_plugin）中被 try/except 包裹，而在其他路径（install_plugin、install_plugin_from_file）中则没有，从而改变了失败时的行为；建议对这些调用点的错误处理逻辑进行统一，以便在依赖安装失败时插件安装行为保持一致。
AI 代理使用的提示词
Please address the comments from this code review:
## Overall Comments
- In `_strip_inline_requirement_comment`, splitting on `[ \t]+#` will also strip legitimate `#` fragments in URLs (e.g. `--extra-index-url https://example.com/simple#frag`); consider limiting comment stripping to lines that are not options or using a more precise heuristic.
- The new `_install_plugin_requirements` helper is wrapped in try/except in some flows (`_check_plugin_dept_update`, `reload_failed_plugin`) but not in others (`install_plugin`, `install_plugin_from_file`), which changes failure behavior; consider aligning error handling across these call sites so plugin installs behave consistently when dependency installation fails.
## Individual Comments
### Comment 1
<location path="astrbot/core/utils/pip_installer.py" line_range="183" />
<code_context>
+ return requirements
+
+
+def _package_specs_override_index(package_specs: list[str]) -> bool:
+ for spec in package_specs:
+ if spec in {"-i", "--index-url"}:
</code_context>
<issue_to_address>
**issue (complexity):** 请考虑收紧 `_package_specs_override_index` 的实现，并将多行解析 helper 内联，这样可以让“index 覆盖逻辑”和“多行 requirement 处理”更加清晰、易于理解。
你可以在两个关键位置降低复杂度，同时无需改变 `install` 的对外行为，也不需要移除任何特性。
### 1. 收紧并简化 `_package_specs_override_index`
当前基于前缀的 `-i` 检测既不直观也过于宽泛：
```python
def _package_specs_override_index(package_specs: list[str]) -> bool:
for spec in package_specs:
if spec in {"-i", "--index-url"}:
return True
if spec.startswith("--index-url="):
return True
if spec.startswith("-i"):
return True
return False
```
你可以通过基于索引的扫描逻辑，仅处理合法的 `pip` 形态，使表达更明确、更安全：
```python
def _package_specs_override_index(package_specs: list[str]) -> bool:
for i, spec in enumerate(package_specs):
if spec in {"-i", "--index-url"}:
# 像 pip 一样要求后面必须跟一个值
if i + 1 < len(package_specs):
return True
continue
# 内联值：--index-url=https://... 或 -ihttps://...
if spec.startswith("--index-url="):
return True
if spec.startswith("-i") and spec != "-i":
return True
return False
```
这样可以保留所有能力（短/长选项、内联形式），同时消除“任何以 `-i` 开头的字符串都匹配”的含糊之处，让意图更易于推断。
### 2. 合并 `_split_multiline_package_input` 和 `_iter_clean_requirement_lines`
目前多行路径被拆成两层：
```python
def _split_multiline_package_input(raw_input: str) -> list[str]:
requirements: list[str] = []
for candidate in _iter_clean_requirement_lines(raw_input):
if candidate.startswith("-"):
requirements.extend(shlex.split(candidate))
continue
requirements.append(candidate)
return requirements
def _iter_clean_requirement_lines(raw_input: str):
for line in raw_input.splitlines():
candidate = _strip_inline_requirement_comment(line)
if not candidate or candidate.startswith("#"):
continue
yield candidate
```
你可以将 `_iter_clean_requirement_lines` 内联到 `_split_multiline_package_input` 中，在不改变行为的前提下减少间接层和分支：
```python
def _split_multiline_package_input(raw_input: str) -> list[str]:
requirements: list[str] = []
for line in raw_input.splitlines():
candidate = _strip_inline_requirement_comment(line)
if not candidate or candidate.startswith("#"):
continue
if candidate.startswith("-"):
requirements.extend(shlex.split(candidate))
else:
requirements.append(candidate)
return requirements
```
这会保留相同语义（空行、注释、行内注释、选项支持），同时让流程更易理解，并把所有多行处理逻辑集中在一个函数里。
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会用这些反馈来改进后续的 review。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- In
_strip_inline_requirement_comment, splitting on[ \t]+#will also strip legitimate#fragments in URLs (e.g.--extra-index-url https://example.com/simple#frag); consider limiting comment stripping to lines that are not options or using a more precise heuristic. - The new
_install_plugin_requirementshelper is wrapped in try/except in some flows (_check_plugin_dept_update,reload_failed_plugin) but not in others (install_plugin,install_plugin_from_file), which changes failure behavior; consider aligning error handling across these call sites so plugin installs behave consistently when dependency installation fails.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_strip_inline_requirement_comment`, splitting on `[ \t]+#` will also strip legitimate `#` fragments in URLs (e.g. `--extra-index-url https://example.com/simple#frag`); consider limiting comment stripping to lines that are not options or using a more precise heuristic.
- The new `_install_plugin_requirements` helper is wrapped in try/except in some flows (`_check_plugin_dept_update`, `reload_failed_plugin`) but not in others (`install_plugin`, `install_plugin_from_file`), which changes failure behavior; consider aligning error handling across these call sites so plugin installs behave consistently when dependency installation fails.
## Individual Comments
### Comment 1
<location path="astrbot/core/utils/pip_installer.py" line_range="183" />
<code_context>
+ return requirements
+
+
+def _package_specs_override_index(package_specs: list[str]) -> bool:
+ for spec in package_specs:
+ if spec in {"-i", "--index-url"}:
</code_context>
<issue_to_address>
**issue (complexity):** Consider tightening `_package_specs_override_index` and inlining the multiline parsing helper to make the index-override logic and multiline requirement handling more explicit and easier to follow.
You can reduce complexity in two focused spots without changing the public behavior of `install` or removing any features.
### 1. Tighten and simplify `_package_specs_override_index`
The current prefix-based `-i` detection is both non-obvious and over-general:
```python
def _package_specs_override_index(package_specs: list[str]) -> bool:
for spec in package_specs:
if spec in {"-i", "--index-url"}:
return True
if spec.startswith("--index-url="):
return True
if spec.startswith("-i"):
return True
return False
```
You can make this more explicit and safer by scanning with index-aware logic and handling only valid `pip` shapes:
```python
def _package_specs_override_index(package_specs: list[str]) -> bool:
for i, spec in enumerate(package_specs):
if spec in {"-i", "--index-url"}:
# require a following value, like pip does
if i + 1 < len(package_specs):
return True
continue
# inline values: --index-url=https://... or -ihttps://...
if spec.startswith("--index-url="):
return True
if spec.startswith("-i") and spec != "-i":
return True
return False
```
This keeps all capabilities (short/long options, inline forms) but removes the “any string starting with `-i`” ambiguity and makes the intent easier to reason about.
### 2. Merge `_split_multiline_package_input` and `_iter_clean_requirement_lines`
Right now the multiline path has two layers:
```python
def _split_multiline_package_input(raw_input: str) -> list[str]:
requirements: list[str] = []
for candidate in _iter_clean_requirement_lines(raw_input):
if candidate.startswith("-"):
requirements.extend(shlex.split(candidate))
continue
requirements.append(candidate)
return requirements
def _iter_clean_requirement_lines(raw_input: str):
for line in raw_input.splitlines():
candidate = _strip_inline_requirement_comment(line)
if not candidate or candidate.startswith("#"):
continue
yield candidate
```
You can inline `_iter_clean_requirement_lines` into `_split_multiline_package_input` to reduce indirection and branching without changing behavior:
```python
def _split_multiline_package_input(raw_input: str) -> list[str]:
requirements: list[str] = []
for line in raw_input.splitlines():
candidate = _strip_inline_requirement_comment(line)
if not candidate or candidate.startswith("#"):
continue
if candidate.startswith("-"):
requirements.extend(shlex.split(candidate))
else:
requirements.append(candidate)
return requirements
```
This keeps the same semantics (blank lines, comments, inline comments, options support) while making the flow easier to follow and localizing all multiline handling to one function.
</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
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - 我发现了两个问题，并给出了一些高层次的反馈：
_install_plugin_requirements_with_logging中的错误日志总是显示更新插件 {plugin_label} 的依赖失败，当从安装或重新加载路径调用时，这会产生误导；建议将该消息改为更通用的，或者传入操作类型，使日志能准确反映当前工作流程。- 在
PipInstaller.install中，_package_specs_override_index只在追加pip_install_arg之前检查args[1:]，因此用户在pip_install_arg中指定的--index-url不会覆盖默认的-i/--trusted-host；如果这不是有意为之，建议在覆盖检查中同时考虑pip_install_arg。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- `_install_plugin_requirements_with_logging` 中的错误日志总是显示 `更新插件 {plugin_label} 的依赖失败`，当从安装或重新加载路径调用时，这会产生误导；建议将该消息改为更通用的，或者传入操作类型，使日志能准确反映当前工作流程。
- 在 `PipInstaller.install` 中，`_package_specs_override_index` 只在追加 `pip_install_arg` 之前检查 `args[1:]`，因此用户在 `pip_install_arg` 中指定的 `--index-url` 不会覆盖默认的 `-i`/`--trusted-host`；如果这不是有意为之，建议在覆盖检查中同时考虑 `pip_install_arg`。
## Individual Comments
### Comment 1
<location path="astrbot/core/utils/pip_installer.py" line_range="173-182" />
<code_context>
+ return True
+
+
+def _split_multiline_package_input(raw_input: str) -> list[str]:
+ requirements: list[str] = []
+ for line in raw_input.splitlines():
+ candidate = _strip_inline_requirement_comment(line)
+ if not candidate or candidate.startswith("#"):
+ continue
+ if candidate.startswith("-"):
+ requirements.extend(_split_option_input(candidate))
+ continue
+ requirements.append(candidate)
+ return requirements
+
</code_context>
<issue_to_address>
**issue (bug_risk):** 多行输入中，当同一行包含多个包时，其处理方式与单行输入不一致。
在这里，像 `"foo bar"` 这样的行会被当作单个 requirement，而单行路径会使用 `shlex.split` 将 `foo` 和 `bar` 视为两个独立的 requirement，并对每个 token 进行校验。对于类似 `"foo bar\nspam"` 的输入，第一行会以 `"foo bar"` 的形式传给 pip，这很可能是无效的。为了保持行为一致，要么在这里也对非选项行使用 `shlex.split`，要么拒绝包含多个 token 的行。
</issue_to_address>
### Comment 2
<location path="astrbot/core/star/star_manager.py" line_range="216-224" />
<code_context>
+ logger.info(f"正在安装插件 {plugin_label} 所需的依赖库: {requirements_path}")
+ await pip_installer.install(requirements_path=requirements_path)
+
+ async def _install_plugin_requirements_with_logging(
+ self,
+ plugin_dir_path: str,
+ plugin_label: str,
+ ) -> None:
+ try:
+ await self._install_plugin_requirements(plugin_dir_path, plugin_label)
+ except Exception as e:
+ logger.error(f"更新插件 {plugin_label} 的依赖失败。Code: {e!s}")
+
async def _import_plugin_with_dependency_recovery(
</code_context>
<issue_to_address>
**issue (bug_risk):** 在依赖安装过程中吞掉所有异常会隐藏可操作的错误，并且让调试更加困难。
`_install_plugin_requirements_with_logging` 捕获了 `Exception` 却只记录了 `e!s`，之后调用方会假定安装成功。在 `reload_failed_plugin` 或 `install_plugin` 等流程中，这可能导致插件处于部分更新的状态，却没有清晰的诊断信息。至少应使用 `logger.exception` 来保留 traceback，并考虑重新抛出异常或返回结构化错误，以便调用方在依赖安装失败时中止插件加载/更新。
</issue_to_address>帮我变得更有用！请对每条评论点 👍 或 👎，我会根据你的反馈改进后续评审。
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- The error log in
_install_plugin_requirements_with_loggingalways says更新插件 {plugin_label} 的依赖失败, which is misleading when called from install or reload paths; consider making the message generic or passing in the operation type so logs accurately reflect the workflow. - In
PipInstaller.install,_package_specs_override_indexonly inspectsargs[1:]beforepip_install_argis appended, so a user-specified--index-urlinpip_install_argwill not suppress the default-i/--trusted-host; if that’s not intentional, consider includingpip_install_argin the override check.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The error log in `_install_plugin_requirements_with_logging` always says `更新插件 {plugin_label} 的依赖失败`, which is misleading when called from install or reload paths; consider making the message generic or passing in the operation type so logs accurately reflect the workflow.
- In `PipInstaller.install`, `_package_specs_override_index` only inspects `args[1:]` before `pip_install_arg` is appended, so a user-specified `--index-url` in `pip_install_arg` will not suppress the default `-i`/`--trusted-host`; if that’s not intentional, consider including `pip_install_arg` in the override check.
## Individual Comments
### Comment 1
<location path="astrbot/core/utils/pip_installer.py" line_range="173-182" />
<code_context>
+ return True
+
+
+def _split_multiline_package_input(raw_input: str) -> list[str]:
+ requirements: list[str] = []
+ for line in raw_input.splitlines():
+ candidate = _strip_inline_requirement_comment(line)
+ if not candidate or candidate.startswith("#"):
+ continue
+ if candidate.startswith("-"):
+ requirements.extend(_split_option_input(candidate))
+ continue
+ requirements.append(candidate)
+ return requirements
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Multiline input with multiple packages on the same line is handled inconsistently vs single-line input.
Here, a line like `"foo bar"` becomes a single requirement, while the single-line path uses `shlex.split` to treat `foo` and `bar` as separate requirements and validate each token. For input like `"foo bar\nspam"`, the first line would be passed to pip as `"foo bar"`, which is likely invalid. To keep behavior consistent, either shlex-split non-option lines here as well or reject lines that contain multiple tokens.
</issue_to_address>
### Comment 2
<location path="astrbot/core/star/star_manager.py" line_range="216-224" />
<code_context>
+ logger.info(f"正在安装插件 {plugin_label} 所需的依赖库: {requirements_path}")
+ await pip_installer.install(requirements_path=requirements_path)
+
+ async def _install_plugin_requirements_with_logging(
+ self,
+ plugin_dir_path: str,
+ plugin_label: str,
+ ) -> None:
+ try:
+ await self._install_plugin_requirements(plugin_dir_path, plugin_label)
+ except Exception as e:
+ logger.error(f"更新插件 {plugin_label} 的依赖失败。Code: {e!s}")
+
async def _import_plugin_with_dependency_recovery(
</code_context>
<issue_to_address>
**issue (bug_risk):** Swallowing all exceptions during requirement installation may hide actionable errors and makes debugging harder.
`_install_plugin_requirements_with_logging` catches `Exception` and only logs `e!s`, after which callers assume installation succeeded. In flows like `reload_failed_plugin` or `install_plugin`, this can leave a plugin partially updated with no clear diagnostics. At least use `logger.exception` to retain the traceback, and consider re‑raising or returning a structured error so callers can abort plugin load/update when dependencies fail.
</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
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并给出了一些整体性的反馈：
- 若干新的插件管理器测试都遵循相同的模式：针对成功 vs 依赖安装失败记录
events（例如 install、install_from_file、reload_failed_plugin、update_plugin），并断言相同的事件序列；可以考虑把这些用例参数化，这样既能减少重复，又便于在一个地方新增/修改测试场景。
给 AI Agent 的提示
请根据这次代码评审中的评论进行修改：
## 整体性评论
- 若干新的插件管理器测试都遵循相同的模式：针对成功 vs 依赖安装失败记录 `events`（例如 install、install_from_file、reload_failed_plugin、update_plugin），并断言相同的事件序列；可以考虑把这些用例参数化，这样既能减少重复，又便于在一个地方新增/修改测试场景。
## 单独评论
### 评论 1
<location path="astrbot/core/star/star_manager.py" line_range="216-224" />
<code_context>
+ logger.info(f"正在安装插件 {plugin_label} 所需的依赖库: {requirements_path}")
+ await pip_installer.install(requirements_path=requirements_path)
+
+ async def _install_plugin_requirements_with_logging(
+ self,
+ plugin_dir_path: str,
+ plugin_label: str,
+ ) -> None:
+ try:
+ await self._install_plugin_requirements(plugin_dir_path, plugin_label)
+ except Exception as e:
+ logger.exception(f"插件 {plugin_label} 依赖安装失败: {e!s}")
+
async def _import_plugin_with_dependency_recovery(
</code_context>
<issue_to_address>
**issue (bug_risk):** 在这里捕获宽泛的 `Exception` 可能会无意中吞掉 `asyncio.CancelledError` 以及其他关键异常。
由于这个包装器会记录并屏蔽 `_install_plugin_requirements` 抛出的所有 `Exception`，它可能会把任务取消当作普通的插件安装失败来处理并忽略，从而破坏关机/超时行为。建议在通用的 `Exception` 处理之前，显式地重新抛出 `asyncio.CancelledError`（以及你关心的其他关键异常），例如：
```python
try:
await self._install_plugin_requirements(...)
except asyncio.CancelledError:
raise
except Exception as e:
logger.exception("插件 %s 依赖安装失败: %s", plugin_label, e)
```
这样既能保留取消语义，又能继续记录真实的安装错误。
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- Several of the new plugin manager tests follow the pattern of recording
eventsfor success vs. dependency-install failure (e.g., install, install_from_file, reload_failed_plugin, update_plugin) and assert the same sequence; consider parametrizing these cases to reduce duplication and make it easier to add/modify scenarios in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several of the new plugin manager tests follow the pattern of recording `events` for success vs. dependency-install failure (e.g., install, install_from_file, reload_failed_plugin, update_plugin) and assert the same sequence; consider parametrizing these cases to reduce duplication and make it easier to add/modify scenarios in one place.
## Individual Comments
### Comment 1
<location path="astrbot/core/star/star_manager.py" line_range="216-224" />
<code_context>
+ logger.info(f"正在安装插件 {plugin_label} 所需的依赖库: {requirements_path}")
+ await pip_installer.install(requirements_path=requirements_path)
+
+ async def _install_plugin_requirements_with_logging(
+ self,
+ plugin_dir_path: str,
+ plugin_label: str,
+ ) -> None:
+ try:
+ await self._install_plugin_requirements(plugin_dir_path, plugin_label)
+ except Exception as e:
+ logger.exception(f"插件 {plugin_label} 依赖安装失败: {e!s}")
+
async def _import_plugin_with_dependency_recovery(
</code_context>
<issue_to_address>
**issue (bug_risk):** Catching broad `Exception` here may inadvertently swallow `asyncio.CancelledError` and other critical exceptions.
Because this wrapper logs and suppresses every `Exception` from `_install_plugin_requirements`, it may treat task cancellation as a normal plugin failure and then ignore it, which can break shutdown/timeout behavior. Consider explicitly re-raising `asyncio.CancelledError` (and any other critical exceptions you care about) before the generic `Exception` handler, for example:
```python
try:
await self._install_plugin_requirements(...)
except asyncio.CancelledError:
raise
except Exception as e:
logger.exception("插件 %s 依赖安装失败: %s", plugin_label, e)
```
This preserves cancellation semantics while still logging real install errors.
</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
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - 我已经留下了一些整体性的反馈：
- 在插件流程（
install_plugin、install_plugin_from_file、reload_failed_plugin、update_plugin）中，_install_plugin_requirements_with_logging会有意吞掉依赖安装失败（CancelledError之外）的错误，并仍然继续执行load/reload；如果依赖安装失败应该阻止插件激活，建议从这个辅助函数返回一个状态值，并在失败时提前中止后续的 load/reload，以便让这种行为更加明确。 - 新增的包解析辅助函数
_split_package_install_input和_split_multiline_package_input会相互调用（例如多行 → 按行 → 再次调用_split_package_install_input），虽然能工作，但逻辑有点难以跟踪；建议重构为一个小型的、非递归的单行解析器，再配合一个顶层的多行处理器，从而让控制流以及优先级规则（marker vs options vs comments）更容易理解。
供 AI 代理使用的提示词
Please address the comments from this code review:
## Overall Comments
- In the plugin flows (`install_plugin`, `install_plugin_from_file`, `reload_failed_plugin`, `update_plugin`), `_install_plugin_requirements_with_logging` intentionally swallows dependency installation failures (other than `CancelledError`) and still proceeds to `load`/`reload`; if dependency install failure should block plugin activation, consider returning a status from the helper and short‑circuiting the load/reload on failure so the behavior is explicit.
- The new package parsing helpers `_split_package_install_input` and `_split_multiline_package_input` call each other (e.g. multiline → per‑line → `_split_package_install_input` again), which works but is a bit hard to follow; consider refactoring into a small, non‑recursive line parser plus a top‑level multiline handler to make the control flow and precedence rules (marker vs options vs comments) easier to reason about.帮我变得更有用！请在每条评论上点击 👍 或 👎，我会根据你的反馈改进后续的评审。
Original comment in English
Hey - I've left some high level feedback:
- In the plugin flows (
install_plugin,install_plugin_from_file,reload_failed_plugin,update_plugin),_install_plugin_requirements_with_loggingintentionally swallows dependency installation failures (other thanCancelledError) and still proceeds toload/reload; if dependency install failure should block plugin activation, consider returning a status from the helper and short‑circuiting the load/reload on failure so the behavior is explicit. - The new package parsing helpers
_split_package_install_inputand_split_multiline_package_inputcall each other (e.g. multiline → per‑line →_split_package_install_inputagain), which works but is a bit hard to follow; consider refactoring into a small, non‑recursive line parser plus a top‑level multiline handler to make the control flow and precedence rules (marker vs options vs comments) easier to reason about.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the plugin flows (`install_plugin`, `install_plugin_from_file`, `reload_failed_plugin`, `update_plugin`), `_install_plugin_requirements_with_logging` intentionally swallows dependency installation failures (other than `CancelledError`) and still proceeds to `load`/`reload`; if dependency install failure should block plugin activation, consider returning a status from the helper and short‑circuiting the load/reload on failure so the behavior is explicit.
- The new package parsing helpers `_split_package_install_input` and `_split_multiline_package_input` call each other (e.g. multiline → per‑line → `_split_package_install_input` again), which works but is a bit hard to follow; consider refactoring into a small, non‑recursive line parser plus a top‑level multiline handler to make the control flow and precedence rules (marker vs options vs comments) easier to reason about.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并留下了一些总体反馈：
PipInstaller同时暴露了模块级的find_missing_requirements*辅助函数和同名的实例方法，而这些实例方法只是简单地委托调用；可以考虑去掉这些实例包装器，在StarManager中直接使用模块级 API，以避免多余的间接调用以及对逻辑实际所在位置的困惑。- 在
_get_core_constraints中，发行版名称被硬编码为"AstrBot"；如果已安装包的名字发生变化（例如变成小写、加 extras 或者重命名），这一保护机制会在无声无息中失效——可以考虑从一个共享常量或者__package__/元数据中推导核心包名，以提高健壮性。
给 AI Agent 的提示
Please address the comments from this code review:
## Overall Comments
- The `PipInstaller` exposes both module-level `find_missing_requirements*` helpers and identically named instance methods that just delegate; consider dropping the instance wrappers and using the module API directly in `StarManager` to avoid redundant indirection and confusion about where the logic lives.
- In `_get_core_constraints` the distribution name is hard-coded as `"AstrBot"`; if the installed package name ever diverges (e.g., lowercase, extras, or rebranding), this protection silently disables itself—consider deriving the core package name from a single shared constant or from `__package__`/metadata to keep this robust.
## Individual Comments
### Comment 1
<location path="astrbot/core/utils/pip_installer.py" line_range="254" />
<code_context>
return names
+def _iter_requirement_lines(
+ requirements_path: str,
+ *,
</code_context>
<issue_to_address>
**issue (complexity):** 请考虑重构 requirements 解析辅助函数，使分词只在单个迭代器中进行一次，然后下游函数基于这些 token 操作，而不是反复对行进行拆分。
你可以通过避免双重 `shlex.split` 调用并将分词逻辑集中在一起，来降低当前 requirements 解析流程新增的复杂度。
目前的情况是：
- `_iter_requirement_lines` 使用 `shlex.split(line)` 来检测嵌套的 `-r/--requirement`，但最终返回的是**原始字符串行**。
- `_iter_requirement_specs` 会**再次**调用 `shlex.split(line)`，然后可能回退到 `_extract_requirement_name(line)`，而后者本身又会重新去除注释。
你可以让 `_iter_requirement_lines` 成为分词和处理嵌套的单一真源，而 `_iter_requirement_specs` 只在 token 上工作。这样可以消除重复解析，并让整体流程更易理解。
### 建议的重构（小范围、局部修改）
将 `_iter_requirement_lines` 修改为返回 token 而不是文本行：
```python
def _iter_requirement_tokens(
requirements_path: str,
*,
_visited_paths: set[str] | None = None,
) -> Iterator[list[str]]:
visited_paths = _visited_paths or set()
resolved_path = os.path.realpath(requirements_path)
if resolved_path in visited_paths:
logger.warning(
"检测到循环依赖的 requirements 包含: %s，将跳过该文件", resolved_path
)
return
visited_paths.add(resolved_path)
with open(resolved_path, encoding="utf-8") as requirements_file:
for raw_line in requirements_file:
line = _strip_inline_requirement_comment(raw_line)
if not line or line.startswith("#"):
continue
tokens = shlex.split(line)
if not tokens:
continue
nested_path: str | None = None
if tokens[0] in {"-r", "--requirement"} and len(tokens) > 1:
nested_path = tokens[1]
elif tokens[0].startswith("--requirement="):
nested_path = tokens[0].split("=", 1)[1]
if nested_path:
if not os.path.isabs(nested_path):
nested_path = os.path.join(os.path.dirname(resolved_path), nested_path)
yield from _iter_requirement_tokens(nested_path, _visited_paths=visited_paths)
continue
yield tokens
```
然后将 `_iter_requirement_specs` 简化为在这些 token 上工作，并只在需要时重建整行字符串：
```python
def _iter_requirement_specs(
requirements_path: str,
) -> Iterator[tuple[str, SpecifierSet | None]]:
for tokens in _iter_requirement_tokens(requirements_path):
# constraints and pure options
if tokens[0] in {"-c", "--constraint"}:
continue
if tokens[0].startswith("-"):
requirement_name = _extract_requirement_name(" ".join(tokens))
if requirement_name:
yield requirement_name, None
continue
line = " ".join(tokens)
try:
requirement = Requirement(line)
except InvalidRequirement:
requirement_name = _extract_requirement_name(line)
if requirement_name:
yield requirement_name, None
continue
if requirement.marker and not requirement.marker.evaluate():
continue
yield (
_canonicalize_distribution_name(requirement.name),
requirement.specifier,
)
```
通过这一改动：
- 分词（`shlex.split`）和嵌套 `-r` 的处理被集中在同一个地方。
- `_iter_requirement_specs` 在 token 上做一次线性遍历，并且拥有清晰的回退路径。
- `_extract_requirement_name` 仍然可以用于处理异常的行，但你避免了对同一字符串重复去注释和重复解析。
这样可以保持当前所有功能（嵌套 requirements、选项、markers、specifiers），同时减少另一位审查者指出的模块间耦合和重复解析问题。
</issue_to_address>帮我变得更有用！请对每条评论点 👍 或 👎，我会根据反馈改进后续的审查。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The
PipInstallerexposes both module-levelfind_missing_requirements*helpers and identically named instance methods that just delegate; consider dropping the instance wrappers and using the module API directly inStarManagerto avoid redundant indirection and confusion about where the logic lives. - In
_get_core_constraintsthe distribution name is hard-coded as"AstrBot"; if the installed package name ever diverges (e.g., lowercase, extras, or rebranding), this protection silently disables itself—consider deriving the core package name from a single shared constant or from__package__/metadata to keep this robust.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `PipInstaller` exposes both module-level `find_missing_requirements*` helpers and identically named instance methods that just delegate; consider dropping the instance wrappers and using the module API directly in `StarManager` to avoid redundant indirection and confusion about where the logic lives.
- In `_get_core_constraints` the distribution name is hard-coded as `"AstrBot"`; if the installed package name ever diverges (e.g., lowercase, extras, or rebranding), this protection silently disables itself—consider deriving the core package name from a single shared constant or from `__package__`/metadata to keep this robust.
## Individual Comments
### Comment 1
<location path="astrbot/core/utils/pip_installer.py" line_range="254" />
<code_context>
return names
+def _iter_requirement_lines(
+ requirements_path: str,
+ *,
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the requirements parsing helpers so that tokenization happens once in a single iterator and downstream functions operate on those tokens instead of repeatedly re-splitting lines.
You can reduce some of the new complexity in the requirements parsing pipeline by avoiding the double `shlex.split` pass and centralizing tokenization.
Right now:
- `_iter_requirement_lines` does `shlex.split(line)` to detect nested `-r/--requirement`, but ultimately yields the **string line**.
- `_iter_requirement_specs` calls `shlex.split(line)` **again** and may then fall back to `_extract_requirement_name(line)`, which itself re-strips comments.
You can make `_iter_requirement_lines` the single source of truth for tokenization and nesting, and let `_iter_requirement_specs` work purely on tokens. That removes the repeated parsing and makes the flow easier to follow.
### Suggested refactor (small and localized)
Change `_iter_requirement_lines` to yield tokens instead of lines:
```python
def _iter_requirement_tokens(
requirements_path: str,
*,
_visited_paths: set[str] | None = None,
) -> Iterator[list[str]]:
visited_paths = _visited_paths or set()
resolved_path = os.path.realpath(requirements_path)
if resolved_path in visited_paths:
logger.warning(
"检测到循环依赖的 requirements 包含: %s，将跳过该文件", resolved_path
)
return
visited_paths.add(resolved_path)
with open(resolved_path, encoding="utf-8") as requirements_file:
for raw_line in requirements_file:
line = _strip_inline_requirement_comment(raw_line)
if not line or line.startswith("#"):
continue
tokens = shlex.split(line)
if not tokens:
continue
nested_path: str | None = None
if tokens[0] in {"-r", "--requirement"} and len(tokens) > 1:
nested_path = tokens[1]
elif tokens[0].startswith("--requirement="):
nested_path = tokens[0].split("=", 1)[1]
if nested_path:
if not os.path.isabs(nested_path):
nested_path = os.path.join(os.path.dirname(resolved_path), nested_path)
yield from _iter_requirement_tokens(nested_path, _visited_paths=visited_paths)
continue
yield tokens
```
Then simplify `_iter_requirement_specs` to operate on those tokens and only reconstruct the line once when needed:
```python
def _iter_requirement_specs(
requirements_path: str,
) -> Iterator[tuple[str, SpecifierSet | None]]:
for tokens in _iter_requirement_tokens(requirements_path):
# constraints and pure options
if tokens[0] in {"-c", "--constraint"}:
continue
if tokens[0].startswith("-"):
requirement_name = _extract_requirement_name(" ".join(tokens))
if requirement_name:
yield requirement_name, None
continue
line = " ".join(tokens)
try:
requirement = Requirement(line)
except InvalidRequirement:
requirement_name = _extract_requirement_name(line)
if requirement_name:
yield requirement_name, None
continue
if requirement.marker and not requirement.marker.evaluate():
continue
yield (
_canonicalize_distribution_name(requirement.name),
requirement.specifier,
)
```
With this change:
- Tokenization (`shlex.split`) and nested `-r` handling live in one place.
- `_iter_requirement_specs` has a single linear pass over tokens, with a clear fallback path.
- `_extract_requirement_name` is still available for strange lines, but you avoid re-stripping / re-parsing the same string multiple times.
This keeps all current functionality (nested requirements, options, markers, specifiers) while reducing the cross‑coupling and repeated parsing that the other reviewer flagged.
</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
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - 我发现了两个问题，并给出了一些总体反馈：
_StreamingLogWriter.write()目前会丢弃仅包含空白字符的行（通过line.strip()），这可能会隐藏有意义的 pip 输出（例如缩进或特殊格式的行）；建议保留这些行，或者至少只跳过真正的空字符串。_get_core_constraints()会扫描所有已安装的 distribution，并对每一个调用read_text('top_level.txt')，在大型环境中这会非常耗时；你可以先直接尝试默认的'AstrBot'distribution，如果该查找失败，再回退到扫描top_level.txt的逻辑。
给 AI Agents 的提示
请根据以下代码审查意见进行修改：
## 总体意见
- `_StreamingLogWriter.write()` 目前会丢弃仅包含空白字符的行（通过 `line.strip()`），这可能会隐藏有意义的 pip 输出（例如缩进或特殊格式的行）；建议保留这些行，或者至少只跳过真正的空字符串。
- `_get_core_constraints()` 会扫描所有已安装的 distribution，并对每一个调用 `read_text('top_level.txt')`，在大型环境中这会非常耗时；你可以先直接尝试默认的 `'AstrBot'` distribution，如果该查找失败，再回退到扫描 `top_level.txt` 的逻辑。
## 单独评论
### 评论 1
<location path="astrbot/core/star/star_manager.py" line_range="211-206" />
<code_context>
+ async def _ensure_plugin_requirements(
</code_context>
<issue_to_address>
**issue (bug_risk):** 吞掉 `DependencyConflictError` 和通用异常可能会让调用方看不到严重错误。
由于 `_ensure_plugin_requirements` 只是在记录日志后就吞掉 `DependencyConflictError` 和其他异常并直接返回，像 `install_plugin`、`update_plugin` 和 `reload_failed_plugin` 这样的调用方会把依赖检查视为成功。这可能导致插件在依赖冲突未解决的情况下被加载，并在之后以晦涩的导入错误形式失败。建议要么重新抛出一个具体的错误（或包一层后再抛出），让调用方可以中止并展示清晰的错误信息；要么返回一个明确的成功/失败结果，要求调用方在继续之前必须检查该结果。
</issue_to_address>
### 评论 2
<location path="astrbot/core/utils/pip_installer.py" line_range="254" />
<code_context>
return names
+def _iter_requirement_tokens(
+ requirements_path: str,
+ *,
</code_context>
<issue_to_address>
**issue (complexity):** 建议通过统一为单个迭代器来解析 requirements，并让核心 distribution 名称显式且可注入，从而简化 requirements 处理和 core-constraints 的发现逻辑。
你可以在不损失现有行为的前提下，降低新增的复杂度：
---
### 1. 合并 requirements 解析流程
目前，在预检查和名称提取时，处理流程是：
- `_extract_requirement_names` → `_iter_requirement_specs` → `_iter_requirement_tokens` → `_strip_inline_requirement_comment`
- `_find_missing_requirements` → `_iter_requirement_specs` → `_iter_requirement_tokens` → `_strip_inline_requirement_comment`
你可以把它们折叠为一个迭代器，该迭代器：
- 读取文件（支持 `-r` 递归和循环检测），
- 去除行内注释，
- 使用 `Requirement` 解析，
- 直接产出 `(name, specifier | None)`。
这样 `_extract_requirement_names` 和 `_find_missing_requirements` 基本都可以简化成一两行代码，并且可以删除 `_iter_requirement_tokens`、`_iter_requirement_specs`、`_strip_inline_requirement_comment`（针对文件的版本），以及多个位置上的正则回退逻辑。
例如：
```python
def _iter_requirements(
requirements_path: str,
_visited: set[str] | None = None,
) -> Iterator[tuple[str, SpecifierSet | None]]:
visited = _visited or set()
resolved_path = os.path.realpath(requirements_path)
if resolved_path in visited:
logger.warning("检测到循环依赖的 requirements 包含: %s，将跳过该文件", resolved_path)
return
visited.add(resolved_path)
with open(resolved_path, encoding="utf-8") as f:
for raw_line in f:
# 统一：文件里的行统一在这里做注释和空行过滤
line = re.split(r"[ \t]+#", raw_line, maxsplit=1)[0].strip()
if not line or line.startswith("#"):
continue
tokens = shlex.split(line)
if not tokens:
continue
# 处理嵌套 -r
nested: str | None = None
if tokens[0] in {"-r", "--requirement"} and len(tokens) > 1:
nested = tokens[1]
elif tokens[0].startswith("--requirement="):
nested = tokens[0].split("=", 1)[1]
if nested:
if not os.path.isabs(nested):
nested = os.path.join(os.path.dirname(resolved_path), nested)
yield from _iter_requirements(nested, _visited=visited)
continue
# 纯选项：跳过
if tokens[0] in {"-c", "--constraint"}:
continue
if tokens[0].startswith("-"):
# 选项形式但依然可能带有包名，比如 -e git+...#egg=xxx
name = _extract_requirement_name(line)
if name:
yield name, None
continue
try:
req = Requirement(line)
if req.marker and not req.marker.evaluate():
continue
yield _canonicalize_distribution_name(req.name), req.specifier or None
except InvalidRequirement:
name = _extract_requirement_name(line)
if name:
yield name, None
```
然后：
```python
def _extract_requirement_names(requirements_path: str) -> set[str]:
try:
return {name for name, _ in _iter_requirements(requirements_path)}
except Exception as exc:
logger.warning("读取依赖文件失败，跳过冲突检测: %s", exc)
return set()
```
`_find_missing_requirements` 也会变得更线性：
```python
def _find_missing_requirements(requirements_path: str) -> set[str] | None:
try:
required = list(_iter_requirements(requirements_path))
except Exception as exc:
logger.warning("预检查缺失依赖失败，将回退到完整安装: %s", exc)
return None
if not required:
return set()
installed = _collect_installed_distribution_versions(_get_requirement_check_paths())
if installed is None:
return None
missing: set[str] = set()
for name, specifier in required:
installed_version = installed.get(name)
if not installed_version:
missing.add(name)
continue
if specifier and not _specifier_contains_version(specifier, installed_version):
missing.add(name)
return missing
```
这样可以保留当前的所有行为（递归 `-r`、markers、egg 片段等），但去掉多步骤的 token/spec 处理管线，让核心的“requirements → (name, spec)” 操作更加直观。
---
### 2. 让核心约束解析显式且可注入
当前 `_get_core_constraints`：
- 通过扫描 `top_level.txt` 来“猜测” AstrBot 的 distribution，
- 然后再解析 requirements 和已安装版本。
这种“猜测”逻辑增加了环境复杂度，也不利于测试。你可以保留现有行为，但让“核心 distribution 名称”变得显式且可覆盖，同时保留当前的启发式作为兜底方案。
示例：
```python
class PipInstaller:
def __init__(
self,
pip_install_arg: str,
pypi_index_url: str | None = None,
core_dist_name: str | None = "AstrBot",
) -> None:
self.pip_install_arg = pip_install_arg
self.pypi_index_url = pypi_index_url
self.core_dist_name = core_dist_name
```
然后将其传入 `_core_constraints_file` / `_get_core_constraints`：
```python
@contextlib.contextmanager
def _core_constraints_file(core_dist_name: str | None) -> Iterator[str | None]:
constraints = _get_core_constraints(core_dist_name)
...
```
```python
def _get_core_constraints(core_dist_name: str | None) -> list[str]:
constraints: list[str] = []
try:
if core_dist_name is None:
core_dist_name = "AstrBot"
try:
if __package__:
top_pkg = __package__.split(".")[0]
for dist in importlib_metadata.distributions():
if top_pkg in (dist.read_text("top_level.txt") or "").splitlines():
core_dist_name = dist.metadata["Name"]
break
except Exception:
pass
try:
dist = importlib_metadata.distribution(core_dist_name)
except importlib_metadata.PackageNotFoundError:
return []
if not dist.requires:
return []
installed = _collect_installed_distribution_versions(
_get_requirement_check_paths()
)
if not installed:
return []
for req_str in dist.requires:
try:
req = Requirement(req_str)
if req.marker and not req.marker.evaluate():
continue
name = _canonicalize_distribution_name(req.name)
if name in installed:
constraints.append(f"{name}=={installed[name]}")
except Exception:
continue
except Exception as exc:
logger.warning("获取核心依赖约束失败: %s", exc)
return constraints
```
在 `install` 里：
```python
with _core_constraints_file(self.core_dist_name) as constraints_file_path:
...
```
这样可以：
- 保留当前的动态发现逻辑作为回退，
- 让核心 distribution 显式可测，
- 允许部署环境配置核心 distribution 名称，从而避免扫描所有 distributions。
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续的 review。
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- _StreamingLogWriter.write() currently discards lines that are only whitespace (via line.strip()), which can hide meaningful pip output (e.g., indented or formatted lines); consider preserving such lines or at least only skipping truly empty strings.
- _get_core_constraints() scans all installed distributions and calls read_text('top_level.txt') on each, which can be expensive in large environments; you could first try the default 'AstrBot' distribution directly and only fall back to the top_level.txt scan if that lookup fails.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- _StreamingLogWriter.write() currently discards lines that are only whitespace (via line.strip()), which can hide meaningful pip output (e.g., indented or formatted lines); consider preserving such lines or at least only skipping truly empty strings.
- _get_core_constraints() scans all installed distributions and calls read_text('top_level.txt') on each, which can be expensive in large environments; you could first try the default 'AstrBot' distribution directly and only fall back to the top_level.txt scan if that lookup fails.
## Individual Comments
### Comment 1
<location path="astrbot/core/star/star_manager.py" line_range="211-206" />
<code_context>
+ async def _ensure_plugin_requirements(
</code_context>
<issue_to_address>
**issue (bug_risk):** Swallowing `DependencyConflictError` and generic exceptions may hide hard failures from callers.
Because `_ensure_plugin_requirements` only logs `DependencyConflictError` and other exceptions and then returns, callers like `install_plugin`, `update_plugin`, and `reload_failed_plugin` will treat dependency checks as successful. This can lead to plugins loading with unresolved dependency conflicts and failing later with opaque import errors. Consider either re-raising a specific error (or wrapping it) so callers can abort and show a clear message, or returning an explicit success/failure result that callers must check before proceeding.
</issue_to_address>
### Comment 2
<location path="astrbot/core/utils/pip_installer.py" line_range="254" />
<code_context>
return names
+def _iter_requirement_tokens(
+ requirements_path: str,
+ *,
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the requirements handling and core-constraints discovery by unifying parsing into a single iterator and making the core distribution name explicit and injectable.
You can reduce the new complexity without losing any behavior by:
---
### 1. Collapse the requirements parsing pipeline
Right now, for pre-checks and name extraction, you go through:
- `_extract_requirement_names` → `_iter_requirement_specs` → `_iter_requirement_tokens` → `_strip_inline_requirement_comment`
- `_find_missing_requirements` → `_iter_requirement_specs` → `_iter_requirement_tokens` → `_strip_inline_requirement_comment`
You can collapse this into a single iterator that:
- Reads a file (with `-r` recursion and cycle detection),
- Strips inline comments,
- Parses with `Requirement`,
- Yields `(name, specifier | None)` directly.
That makes both `_extract_requirement_names` and `_find_missing_requirements` essentially one-liners, and allows you to delete `_iter_requirement_tokens`, `_iter_requirement_specs`, `_strip_inline_requirement_comment` (for files), and the regex fallback in multiple places.
For example:
```python
def _iter_requirements(
requirements_path: str,
_visited: set[str] | None = None,
) -> Iterator[tuple[str, SpecifierSet | None]]:
visited = _visited or set()
resolved_path = os.path.realpath(requirements_path)
if resolved_path in visited:
logger.warning("检测到循环依赖的 requirements 包含: %s，将跳过该文件", resolved_path)
return
visited.add(resolved_path)
with open(resolved_path, encoding="utf-8") as f:
for raw_line in f:
# 统一：文件里的行统一在这里做注释和空行过滤
line = re.split(r"[ \t]+#", raw_line, maxsplit=1)[0].strip()
if not line or line.startswith("#"):
continue
tokens = shlex.split(line)
if not tokens:
continue
# 处理嵌套 -r
nested: str | None = None
if tokens[0] in {"-r", "--requirement"} and len(tokens) > 1:
nested = tokens[1]
elif tokens[0].startswith("--requirement="):
nested = tokens[0].split("=", 1)[1]
if nested:
if not os.path.isabs(nested):
nested = os.path.join(os.path.dirname(resolved_path), nested)
yield from _iter_requirements(nested, _visited=visited)
continue
# 纯选项：跳过
if tokens[0] in {"-c", "--constraint"}:
continue
if tokens[0].startswith("-"):
# 选项形式但依然可能带有包名，比如 -e git+...#egg=xxx
name = _extract_requirement_name(line)
if name:
yield name, None
continue
try:
req = Requirement(line)
if req.marker and not req.marker.evaluate():
continue
yield _canonicalize_distribution_name(req.name), req.specifier or None
except InvalidRequirement:
name = _extract_requirement_name(line)
if name:
yield name, None
```
Then:
```python
def _extract_requirement_names(requirements_path: str) -> set[str]:
try:
return {name for name, _ in _iter_requirements(requirements_path)}
except Exception as exc:
logger.warning("读取依赖文件失败，跳过冲突检测: %s", exc)
return set()
```
and `_find_missing_requirements` becomes much more linear:
```python
def _find_missing_requirements(requirements_path: str) -> set[str] | None:
try:
required = list(_iter_requirements(requirements_path))
except Exception as exc:
logger.warning("预检查缺失依赖失败，将回退到完整安装: %s", exc)
return None
if not required:
return set()
installed = _collect_installed_distribution_versions(_get_requirement_check_paths())
if installed is None:
return None
missing: set[str] = set()
for name, specifier in required:
installed_version = installed.get(name)
if not installed_version:
missing.add(name)
continue
if specifier and not _specifier_contains_version(specifier, installed_version):
missing.add(name)
return missing
```
This keeps all current behavior (recursive `-r`, markers, egg fragments, etc.) but removes the multi-step token/spec pipeline and makes the core “requirements → (name, spec)” operation straightforward.
---
### 2. Make core constraints resolution explicit and injectable
`_get_core_constraints` currently:
- Guess-detects the AstrBot distribution via `top_level.txt` scanning,
- Then resolves requirements and installed versions.
That guessing logic adds ambient complexity and is hard to test. You can keep the behavior but make the “core distribution name” explicit and overrideable, while keeping the current heuristic as a fallback.
Example:
```python
class PipInstaller:
def __init__(
self,
pip_install_arg: str,
pypi_index_url: str | None = None,
core_dist_name: str | None = "AstrBot",
) -> None:
self.pip_install_arg = pip_install_arg
self.pypi_index_url = pypi_index_url
self.core_dist_name = core_dist_name
```
Then pass it into `_core_constraints_file` / `_get_core_constraints`:
```python
@contextlib.contextmanager
def _core_constraints_file(core_dist_name: str | None) -> Iterator[str | None]:
constraints = _get_core_constraints(core_dist_name)
...
```
```python
def _get_core_constraints(core_dist_name: str | None) -> list[str]:
constraints: list[str] = []
try:
if core_dist_name is None:
core_dist_name = "AstrBot"
try:
if __package__:
top_pkg = __package__.split(".")[0]
for dist in importlib_metadata.distributions():
if top_pkg in (dist.read_text("top_level.txt") or "").splitlines():
core_dist_name = dist.metadata["Name"]
break
except Exception:
pass
try:
dist = importlib_metadata.distribution(core_dist_name)
except importlib_metadata.PackageNotFoundError:
return []
if not dist.requires:
return []
installed = _collect_installed_distribution_versions(
_get_requirement_check_paths()
)
if not installed:
return []
for req_str in dist.requires:
try:
req = Requirement(req_str)
if req.marker and not req.marker.evaluate():
continue
name = _canonicalize_distribution_name(req.name)
if name in installed:
constraints.append(f"{name}=={installed[name]}")
except Exception:
continue
except Exception as exc:
logger.warning("获取核心依赖约束失败: %s", exc)
return constraints
```
And in `install`:
```python
with _core_constraints_file(self.core_dist_name) as constraints_file_path:
...
```
This:
- Keeps the current dynamic discovery as a fallback,
- Makes the core dist explicit and testable,
- Allows deployments to configure the core dist name and avoid scanning all distributions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…and improve exception propagation
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并留下了一些总体反馈：
- 在
PipInstaller._build_pip_args/install中，当package_name和requirements_path都为空或只包含空白字符时，你仍然会只带上install和索引相关的参数来调用 pip；可以考虑在这种情况下直接短路，不去运行 pip，这样真正的空输入就会变成一个 no-op，而不是一次网络调用。 _StreamingLogWriter.write目前会完全跳过空行（if not line: continue），因此你仍然会丢失 pip 输出中的空白行；如果目标是尽可能保留 pip 的原始格式，可以考虑记录并日志输出空行，而不是直接丢弃它们。
面向 AI 代理的提示
Please address the comments from this code review:
## Overall Comments
- 在 `PipInstaller._build_pip_args`/`install` 中，当 `package_name` 和 `requirements_path` 都为空或只包含空白字符时，你仍然会只带上 `install` 和索引相关的参数来调用 pip；可以考虑在这种情况下直接短路，不去运行 pip，这样真正的空输入就会变成一个 no-op，而不是一次网络调用。
- `_StreamingLogWriter.write` 目前会完全跳过空行（`if not line: continue`），因此你仍然会丢失 pip 输出中的空白行；如果目标是尽可能保留 pip 的原始格式，可以考虑记录并日志输出空行，而不是直接丢弃它们。
## Individual Comments
### Comment 1
<location path="astrbot/core/utils/pip_installer.py" line_range="107" />
<code_context>
+ return specifier.contains(parsed_version, prereleases=True)
+
+
+def _split_package_install_input(raw_input: str) -> list[str]:
+ """
+ Normalize the user-provided package string into a list of pip args.
</code_context>
<issue_to_address>
**issue (complexity):** 考虑引入一个共享迭代器来规范化每一行 requirement，然后在不同的解析辅助函数之间复用它，以消除重复并简化逻辑。
你可以通过把“去掉注释 + 跳过空行 + 按逻辑单元拆分”这套逻辑集中到一个统一的迭代器里，并在下面这些地方复用它，从而显著降低 requirement 解析相关的复杂度和重复：
- `_split_package_install_input`
- `_extract_requested_requirements_from_package_input`
- `_iter_requirements`（当前重新实现了一套注释剥离逻辑）
例如：
```python
def _iter_normalized_requirement_lines(raw: str) -> Iterator[str]:
"""Yield non-empty, comment-stripped lines from user/package input."""
normalized = raw.strip()
if not normalized:
return
for line in normalized.splitlines():
stripped = _strip_inline_requirement_comment(line)
if stripped:
yield stripped
```
这样，现有的辅助函数就会变得简单得多，也更一致：
```python
def _split_package_install_input(raw_input: str) -> list[str]:
specs: list[str] = []
for line in _iter_normalized_requirement_lines(raw_input):
# keep the "single Requirement vs shlex.split" behavior
try:
Requirement(line)
except InvalidRequirement:
specs.extend(shlex.split(line))
else:
specs.append(line)
return specs
```
```python
def _extract_requested_requirements_from_package_input(raw_input: str) -> set[str]:
requirements: set[str] = set()
for line in _iter_normalized_requirement_lines(raw_input):
try:
req = Requirement(line)
except InvalidRequirement:
tokens = _split_package_install_line(line)
if not tokens:
continue
if tokens[0] in {"-e", "--editable"} or tokens[0].startswith("--editable="):
requirement_name = _extract_requirement_name(line)
if requirement_name:
requirements.add(requirement_name)
continue
if tokens[0].startswith("-"):
continue
for token in tokens:
requirement_name = _extract_requirement_name(token)
if requirement_name:
requirements.add(requirement_name)
else:
requirements.add(_canonicalize_distribution_name(req.name))
return requirements
```
并且 `_iter_requirements` 可以复用同一个规范化函数，而不是重新实现一套正则逻辑：
```python
def _iter_requirements(
requirements_path: str,
_visited: set[str] | None = None,
) -> Iterator[tuple[str, SpecifierSet | None]]:
...
try:
with open(resolved_path, encoding="utf-8") as f:
for raw_line in f:
for line in _iter_normalized_requirement_lines(raw_line):
tokens = shlex.split(line)
if not tokens:
continue
...
...
```
这样可以保持当前所有行为（包括 Requirement 与 `shlex.split` 的分支以及 editable 处理），但同时为你提供一条统一且可预测的规范化路径，用于：
- 注释剥离，
- 空行跳过，
- 多行输入处理。
如果以后需要调整解析规则（例如微调注释语义），这样也更容易在一个地方统一修改，而不是维护三套实现。
</issue_to_address>帮我变得更有用！请在每条评论上点击 👍 或 👎，我会根据你的反馈改进之后的评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- In
PipInstaller._build_pip_args/install, when bothpackage_nameandrequirements_pathare empty or whitespace, you still invoke pip with justinstalland index flags; consider short‑circuiting and not running pip at all in this case so that truly empty input is a no‑op rather than a network call. - _StreamingLogWriter.write currently skips empty lines entirely (
if not line: continue), so you still lose blank lines from pip’s output; if the goal is to preserve pip’s original formatting as much as possible, consider logging and recording empty lines instead of dropping them.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `PipInstaller._build_pip_args`/`install`, when both `package_name` and `requirements_path` are empty or whitespace, you still invoke pip with just `install` and index flags; consider short‑circuiting and not running pip at all in this case so that truly empty input is a no‑op rather than a network call.
- _StreamingLogWriter.write currently skips empty lines entirely (`if not line: continue`), so you still lose blank lines from pip’s output; if the goal is to preserve pip’s original formatting as much as possible, consider logging and recording empty lines instead of dropping them.
## Individual Comments
### Comment 1
<location path="astrbot/core/utils/pip_installer.py" line_range="107" />
<code_context>
+ return specifier.contains(parsed_version, prereleases=True)
+
+
+def _split_package_install_input(raw_input: str) -> list[str]:
+ """
+ Normalize the user-provided package string into a list of pip args.
</code_context>
<issue_to_address>
**issue (complexity):** Consider introducing a shared iterator to normalize requirement lines and reuse it across the different parsing helpers to remove duplication and simplify their logic.
You can reduce a fair bit of complexity and duplication around requirement parsing by centralizing the “strip comments + skip empty lines + split into logical units” logic into a single iterator and reusing it in:
- `_split_package_install_input`
- `_extract_requested_requirements_from_package_input`
- `_iter_requirements` (which currently re‑implements comment stripping)
For example:
```python
def _iter_normalized_requirement_lines(raw: str) -> Iterator[str]:
"""Yield non-empty, comment-stripped lines from user/package input."""
normalized = raw.strip()
if not normalized:
return
for line in normalized.splitlines():
stripped = _strip_inline_requirement_comment(line)
if stripped:
yield stripped
```
Then the existing helpers become much simpler and less divergent:
```python
def _split_package_install_input(raw_input: str) -> list[str]:
specs: list[str] = []
for line in _iter_normalized_requirement_lines(raw_input):
# keep the "single Requirement vs shlex.split" behavior
try:
Requirement(line)
except InvalidRequirement:
specs.extend(shlex.split(line))
else:
specs.append(line)
return specs
```
```python
def _extract_requested_requirements_from_package_input(raw_input: str) -> set[str]:
requirements: set[str] = set()
for line in _iter_normalized_requirement_lines(raw_input):
try:
req = Requirement(line)
except InvalidRequirement:
tokens = _split_package_install_line(line)
if not tokens:
continue
if tokens[0] in {"-e", "--editable"} or tokens[0].startswith("--editable="):
requirement_name = _extract_requirement_name(line)
if requirement_name:
requirements.add(requirement_name)
continue
if tokens[0].startswith("-"):
continue
for token in tokens:
requirement_name = _extract_requirement_name(token)
if requirement_name:
requirements.add(requirement_name)
else:
requirements.add(_canonicalize_distribution_name(req.name))
return requirements
```
And `_iter_requirements` can reuse the same normalizer instead of re‑implementing the regex:
```python
def _iter_requirements(
requirements_path: str,
_visited: set[str] | None = None,
) -> Iterator[tuple[str, SpecifierSet | None]]:
...
try:
with open(resolved_path, encoding="utf-8") as f:
for raw_line in f:
for line in _iter_normalized_requirement_lines(raw_line):
tokens = shlex.split(line)
if not tokens:
continue
...
...
```
This keeps all current behaviors (including the Requirement vs. `shlex.split` branches and editable handling) but gives you a single, predictable normalization path for:
- comment stripping,
- empty line skipping,
- multi‑line input handling.
It also makes it easier to evolve the parsing rules (e.g., if you need to tweak comment semantics) in one place instead of three.
</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
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - 我发现了 4 个问题
AI Agents 使用的提示词
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/core/utils/pip_installer.py" line_range="964-966" />
<code_context>
+ if not args:
+ return [], requested_requirements
+
+ if not _package_specs_override_index([*args[1:], *pip_install_args]):
+ index_url = mirror or self.pypi_index_url or "https://pypi.org/simple"
+ args.extend(["--trusted-host", "mirrors.aliyun.com", "-i", index_url])
+
+ if pip_install_args:
</code_context>
<issue_to_address>
**🚨 issue (security):** 即使使用了不同的 index URL，也对 mirrors.aliyun.com 进行了硬编码的 trusted-host 设置
当有效的 `index_url` 是 PyPI 或其他自定义索引时，依然会添加 `--trusted-host mirrors.aliyun.com`。这既不必要地将信任范围扩展到一个无关的主机，也无法在使用其他镜像主机且其 TLS 出现问题时提供帮助。请只在索引主机为 `mirrors.aliyun.com` 时有条件地添加 `--trusted-host`，或者从 `index_url` 中动态解析出需要信任的主机。
</issue_to_address>
### Comment 2
<location path="astrbot/core/utils/pip_installer.py" line_range="407-410" />
<code_context>
+ os.remove(path)
+
+
+class _StreamingLogWriter(io.TextIOBase):
+ def __init__(self, log_func) -> None:
+ self._log_func = log_func
+ self._buffer = ""
+ self.lines: list[str] = []
+
</code_context>
<issue_to_address>
**suggestion:** 在流式日志 writer 中进行 CRLF 归一化时，可能会产生多余的空行
在分割之前使用 `text.replace("\r", "\n")` 做归一化时，`"\r\n"` 会被替换成 `"\n\n"`，因此每个 `"\r\n"` 序列都会产生额外的空行，并让 `lines` 变得更大。相反，应当将 CRLF 当作一个单独的换行符处理（例如，先将 `"\r\n"` 替换为 `"\n"`，或者用一个小的状态机正确处理 `\r`/`\n` 的组合）。
建议的实现：
```python
# Normalize CRLF and lone CR to LF without producing extra empty lines
text = text.replace("\r\n", "\n").replace("\r", "\n")
```
在你的代码片段中，我只看到了 `_StreamingLogWriter` 的类定义头部，而没有看到完整实现。上面的 `SEARCH` 区块是假设你的 `write`（或类似）方法当前在分行之前使用了 `text = text.replace("\r", "\n")` 这样的语句。如果变量名或缩进不同（例如用 `s` 而不是 `text`），或者归一化是在一个辅助函数中完成的，请在对应的行上应用同样的替换逻辑：
1. 先把 `"\r\n"` 替换为 `"\n"`。
2. 再把剩余的所有 `"\r"` 替换为 `"\n"`。
其余的分行逻辑以及对 `self.lines` 的累积都可以保持不变。
</issue_to_address>
### Comment 3
<location path="astrbot/core/utils/pip_installer.py" line_range="107" />
<code_context>
- module = importlib.import_module(module_name)
- except Exception:
- return False
+def _iter_normalized_requirement_lines(raw_input: str) -> Iterator[str]:
+ normalized = raw_input.strip()
+ if not normalized:
</code_context>
<issue_to_address>
**issue (complexity):** 建议把重复的“依赖行解析”逻辑抽到一个统一的 helper 中，让它同时返回 tokens 和依赖名称，以简化控制流并避免重复工作。
在处理包输入时（`_iter_normalized_requirement_lines`、`_split_package_install_line`、`_split_package_install_input`、`_extract_requested_requirements_from_package_input`），你做了很多重复的解析工作。针对同一行，多次调用 `Requirement(...)`、`shlex.split` 和 `_extract_requirement_name`，这增加了分支数量，也让控制流变得更难理解。
在保持当前行为不变的前提下，你可以把这些逻辑折叠到一个“按行”的 helper 中，该 helper 解析一次，并返回该行的 tokens 以及解析出来的依赖名称。然后 `_split_package_install_input` 和 `_extract_requested_requirements_from_package_input` 都可以复用它。
重构示例：
```python
def _parse_requirement_line(
line: str,
) -> tuple[list[str], set[str]]:
"""Parse a single requirement line into pip tokens and requirement names.
- Tries packaging.Requirement first.
- Falls back to shlex.split + _extract_requirement_name heuristics.
- Handles editable installs & options.
"""
requirement_names: set[str] = set()
# First try as a single Requirement
try:
req = Requirement(line)
except InvalidRequirement:
# Fallback: tokenized parsing
tokens = shlex.split(line)
if not tokens:
return [], set()
# Editable install
if tokens[0] in {"-e", "--editable"} or tokens[0].startswith("--editable="):
name = _extract_requirement_name(line)
if name:
requirement_names.add(name)
return tokens, requirement_names
# Global options only
if tokens[0].startswith("-"):
# Try to extract any embedded name (e.g. url#egg=...)
name = _extract_requirement_name(line)
if name:
requirement_names.add(name)
return tokens, requirement_names
# Plain spec(s)
for token in tokens:
name = _extract_requirement_name(token)
if name:
requirement_names.add(name)
return tokens, requirement_names
else:
# Valid Requirement object
requirement_names.add(_canonicalize_distribution_name(req.name))
# For a valid Requirement we keep the original line as a single token
return [line], requirement_names
```
然后，你现有的 helper 就可以变得更简单，并共用同一套解析逻辑：
```python
def _split_package_install_input(raw_input: str) -> list[str]:
specs: list[str] = []
for line in _iter_normalized_requirement_lines(raw_input):
tokens, _ = _parse_requirement_line(line)
specs.extend(tokens)
return specs
def _extract_requested_requirements_from_package_input(raw_input: str) -> set[str]:
requirements: set[str] = set()
for line in _iter_normalized_requirement_lines(raw_input):
_, names = _parse_requirement_line(line)
requirements.update(names)
return requirements
```
这样可以减少：
- 针对同一行多次独立调用 `Requirement(...)`、`shlex.split`、`_extract_requirement_name`；
- 分散在多个函数中的分支逻辑。
在保持现有行为不变（包括支持 editable 安装、选项和行内注释）的同时，使数据流（输入行 → `{tokens, names}`）更清晰、更易于维护。
</issue_to_address>
### Comment 4
<location path="astrbot/core/star/star_manager.py" line_range="211" />
<code_context>
+ await self._ensure_plugin_requirements(plugin_path, p)
return True
+ async def _ensure_plugin_requirements(
+ self,
+ plugin_dir_path: str,
</code_context>
<issue_to_address>
**issue (complexity):** 建议通过提取“策略决策” helper（以及可选的小型上下文对象），来压平 `_ensure_plugin_requirements`，从而把决策逻辑与执行和日志分离开。
你可以在不改变行为的前提下压平 `_ensure_plugin_requirements`，并把“决定做什么”与“执行并记录日志”拆开。
**1. 移除内层 try，使预检查结果以显式方式呈现**
与其在内部 `try` 中捕获 `RequirementsPrecheckFailed`，不如把“预检查失败”映射到一个哨兵值，然后在一条线性的流程中处理所有安装逻辑：
```python
from enum import Enum, auto
class _ReqStrategy(Enum):
FULL_INSTALL = auto()
SKIP = auto()
INSTALL_MISSING = auto()
def _determine_requirements_strategy(requirements_path: str) -> _ReqStrategy | tuple[_ReqStrategy, set[str]]:
try:
missing = find_missing_requirements_or_raise(requirements_path)
except RequirementsPrecheckFailed:
return _ReqStrategy.FULL_INSTALL
if not missing:
return _ReqStrategy.SKIP
return _ReqStrategy.INSTALL_MISSING, missing
```
然后，`_ensure_plugin_requirements` 就会更加扁平：
```python
async def _ensure_plugin_requirements(
self,
plugin_dir_path: str,
plugin_label: str,
) -> None:
requirements_path = os.path.join(plugin_dir_path, "requirements.txt")
if not os.path.exists(requirements_path):
return
try:
strategy = _determine_requirements_strategy(requirements_path)
if strategy == _ReqStrategy.FULL_INSTALL:
logger.info(
f"正在安装插件 {plugin_label} 的依赖库（预检查失败，回退到完整安装）: "
f"{requirements_path}"
)
await pip_installer.install(requirements_path=requirements_path)
return
if strategy == _ReqStrategy.SKIP:
logger.info(f"插件 {plugin_label} 的依赖已满足，跳过安装。")
return
_, missing = strategy
logger.info(
f"正在安装插件 {plugin_label} 缺失的依赖库: "
f"{requirements_path} -> {sorted(missing)}"
)
await pip_installer.install(requirements_path=requirements_path)
except asyncio.CancelledError:
raise
except DependencyConflictError as e:
logger.error(f"插件 {plugin_label} 依赖冲突: {e!s}")
raise
except Exception as e:
logger.exception(f"插件 {plugin_label} 依赖安装失败: {e!s}")
raise
```
这在保留所有现有行为和日志的同时，移除了嵌套的 `try`，并让三种状态更加明确。
**2. 可选：通过一个小型上下文对象减少调用方的重复**
如果你想简化在调用处重复构造 `(plugin_dir_path, plugin_label)` 的代码，可以加一个很小的 helper，且对现有逻辑影响较小：
```python
@dataclass
class PluginReqContext:
dir_path: str
label: str
async def _ensure_plugin_requirements_ctx(self, ctx: PluginReqContext) -> None:
await self._ensure_plugin_requirements(ctx.dir_path, ctx.label)
```
使用示例（保持行为完全一致，只是减少重复 / 不一致的风险）：
```python
plugin_path = os.path.join(self.plugin_store_path, dir_name)
await self._ensure_plugin_requirements_ctx(PluginReqContext(plugin_path, dir_name))
```
这两处调整都能在不回退你新增功能的情况下，降低复杂度并提升可读性。
</issue_to_address>帮我变得更有用！请对每条评论点 👍 或 👎，我会根据你的反馈改进后续的评审。
Original comment in English
Hey - I've found 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/core/utils/pip_installer.py" line_range="964-966" />
<code_context>
+ if not args:
+ return [], requested_requirements
+
+ if not _package_specs_override_index([*args[1:], *pip_install_args]):
+ index_url = mirror or self.pypi_index_url or "https://pypi.org/simple"
+ args.extend(["--trusted-host", "mirrors.aliyun.com", "-i", index_url])
+
+ if pip_install_args:
</code_context>
<issue_to_address>
**🚨 issue (security):** Hard-coded trusted-host for mirrors.aliyun.com even when using a different index URL
`--trusted-host mirrors.aliyun.com` is added even when the effective `index_url` is PyPI or some other custom index. This both unnecessarily expands the trust scope to an unrelated host and fails to help when a different mirror host has TLS issues. Please either conditionally add `--trusted-host` only when the index host is `mirrors.aliyun.com`, or derive the trusted host from `index_url` dynamically.
</issue_to_address>
### Comment 2
<location path="astrbot/core/utils/pip_installer.py" line_range="407-410" />
<code_context>
+ os.remove(path)
+
+
+class _StreamingLogWriter(io.TextIOBase):
+ def __init__(self, log_func) -> None:
+ self._log_func = log_func
+ self._buffer = ""
+ self.lines: list[str] = []
+
</code_context>
<issue_to_address>
**suggestion:** CRLF normalization in the streaming log writer can produce spurious empty lines
Normalizing with `text.replace("\r", "\n")` before splitting means `"\r\n"` turns into `"\n\n"`, so `"\r\n"` sequences produce extra empty lines and inflate `lines`. Instead, handle CRLF as a single newline (e.g., first replace `"\r\n"` with `"\n"`, or use a small state machine to process `\r`/`\n` pairs correctly).
Suggested implementation:
```python
# Normalize CRLF and lone CR to LF without producing extra empty lines
text = text.replace("\r\n", "\n").replace("\r", "\n")
```
I only saw the class definition header for `_StreamingLogWriter` in your snippet, not the full implementation. The `SEARCH` block above assumes your `write` (or equivalent) method currently uses a line like `text = text.replace("\r", "\n")` before splitting into lines. If the variable name or indentation differs (e.g., `s` instead of `text`), or if normalization is done in a helper method, apply the same replacement logic to the relevant line(s):
1. First replace `"\r\n"` with `"\n"`.
2. Then replace any remaining `"\r"` with `"\n"`.
The rest of the line-splitting and `self.lines` accumulation logic can remain unchanged.
</issue_to_address>
### Comment 3
<location path="astrbot/core/utils/pip_installer.py" line_range="107" />
<code_context>
- module = importlib.import_module(module_name)
- except Exception:
- return False
+def _iter_normalized_requirement_lines(raw_input: str) -> Iterator[str]:
+ normalized = raw_input.strip()
+ if not normalized:
</code_context>
<issue_to_address>
**issue (complexity):** Consider factoring the repeated requirement-line parsing logic into a single helper that returns both tokens and requirement names to simplify control flow and avoid duplicate work.
You’re doing a lot of duplicate parsing work in the package input handling (`_iter_normalized_requirement_lines`, `_split_package_install_line`, `_split_package_install_input`, `_extract_requested_requirements_from_package_input`). You call `Requirement(...)`, `shlex.split`, and `_extract_requirement_name` multiple times for the same line, which increases branching and makes the control flow harder to follow.
You can keep all existing behavior but collapse this into a single line‑level helper that parses once and returns both tokens and any requirement names it discovers. Then both `_split_package_install_input` and `_extract_requested_requirements_from_package_input` can reuse it.
Example refactor:
```python
def _parse_requirement_line(
line: str,
) -> tuple[list[str], set[str]]:
"""Parse a single requirement line into pip tokens and requirement names.
- Tries packaging.Requirement first.
- Falls back to shlex.split + _extract_requirement_name heuristics.
- Handles editable installs & options.
"""
requirement_names: set[str] = set()
# First try as a single Requirement
try:
req = Requirement(line)
except InvalidRequirement:
# Fallback: tokenized parsing
tokens = shlex.split(line)
if not tokens:
return [], set()
# Editable install
if tokens[0] in {"-e", "--editable"} or tokens[0].startswith("--editable="):
name = _extract_requirement_name(line)
if name:
requirement_names.add(name)
return tokens, requirement_names
# Global options only
if tokens[0].startswith("-"):
# Try to extract any embedded name (e.g. url#egg=...)
name = _extract_requirement_name(line)
if name:
requirement_names.add(name)
return tokens, requirement_names
# Plain spec(s)
for token in tokens:
name = _extract_requirement_name(token)
if name:
requirement_names.add(name)
return tokens, requirement_names
else:
# Valid Requirement object
requirement_names.add(_canonicalize_distribution_name(req.name))
# For a valid Requirement we keep the original line as a single token
return [line], requirement_names
```
Then your existing helpers become simpler and share the same parsing logic:
```python
def _split_package_install_input(raw_input: str) -> list[str]:
specs: list[str] = []
for line in _iter_normalized_requirement_lines(raw_input):
tokens, _ = _parse_requirement_line(line)
specs.extend(tokens)
return specs
def _extract_requested_requirements_from_package_input(raw_input: str) -> set[str]:
requirements: set[str] = set()
for line in _iter_normalized_requirement_lines(raw_input):
_, names = _parse_requirement_line(line)
requirements.update(names)
return requirements
```
This reduces:
- Multiple independent calls to `Requirement(...)`, `shlex.split`, `_extract_requirement_name` for the same line.
- Branching spread across several functions.
The behavior stays the same (including editable installs, options, and inline comments) while making the data flow (input line → `{tokens, names}`) much easier to reason about and maintain.
</issue_to_address>
### Comment 4
<location path="astrbot/core/star/star_manager.py" line_range="211" />
<code_context>
+ await self._ensure_plugin_requirements(plugin_path, p)
return True
+ async def _ensure_plugin_requirements(
+ self,
+ plugin_dir_path: str,
</code_context>
<issue_to_address>
**issue (complexity):** Consider flattening `_ensure_plugin_requirements` by extracting a strategy-determination helper and (optionally) a small context object to separate decision logic from execution and logging.
You can flatten `_ensure_plugin_requirements` and separate “decide what to do” from “do it and log” without changing behavior.
**1. Remove the nested try and make precheck result explicit**
Instead of catching `RequirementsPrecheckFailed` inside an inner `try`, you can map “precheck failed” to a sentinel and handle all install logic in one linear flow:
```python
from enum import Enum, auto
class _ReqStrategy(Enum):
FULL_INSTALL = auto()
SKIP = auto()
INSTALL_MISSING = auto()
def _determine_requirements_strategy(requirements_path: str) -> _ReqStrategy | tuple[_ReqStrategy, set[str]]:
try:
missing = find_missing_requirements_or_raise(requirements_path)
except RequirementsPrecheckFailed:
return _ReqStrategy.FULL_INSTALL
if not missing:
return _ReqStrategy.SKIP
return _ReqStrategy.INSTALL_MISSING, missing
```
Then `_ensure_plugin_requirements` becomes flatter:
```python
async def _ensure_plugin_requirements(
self,
plugin_dir_path: str,
plugin_label: str,
) -> None:
requirements_path = os.path.join(plugin_dir_path, "requirements.txt")
if not os.path.exists(requirements_path):
return
try:
strategy = _determine_requirements_strategy(requirements_path)
if strategy == _ReqStrategy.FULL_INSTALL:
logger.info(
f"正在安装插件 {plugin_label} 的依赖库（预检查失败，回退到完整安装）: "
f"{requirements_path}"
)
await pip_installer.install(requirements_path=requirements_path)
return
if strategy == _ReqStrategy.SKIP:
logger.info(f"插件 {plugin_label} 的依赖已满足，跳过安装。")
return
_, missing = strategy
logger.info(
f"正在安装插件 {plugin_label} 缺失的依赖库: "
f"{requirements_path} -> {sorted(missing)}"
)
await pip_installer.install(requirements_path=requirements_path)
except asyncio.CancelledError:
raise
except DependencyConflictError as e:
logger.error(f"插件 {plugin_label} 依赖冲突: {e!s}")
raise
except Exception as e:
logger.exception(f"插件 {plugin_label} 依赖安装失败: {e!s}")
raise
```
This preserves all existing behavior and logging, but removes the nested `try` and makes the three states explicit.
**2. Optional: reduce repetition in callers with a small context**
If you want to simplify the repeated `(plugin_dir_path, plugin_label)` construction, a tiny helper stays non-invasive:
```python
@dataclass
class PluginReqContext:
dir_path: str
label: str
async def _ensure_plugin_requirements_ctx(self, ctx: PluginReqContext) -> None:
await self._ensure_plugin_requirements(ctx.dir_path, ctx.label)
```
Usage examples (keeps behavior identical, just reduces duplication / inconsistency risk):
```python
plugin_path = os.path.join(self.plugin_store_path, dir_name)
await self._ensure_plugin_requirements_ctx(PluginReqContext(plugin_path, dir_name))
```
Both changes lower complexity and improve readability without reverting your new functionality.
</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
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并给出了一些整体反馈：
_classify_pip_failure中对 pip 失败原因的分类，和 pip 输出里的特定英文子串（"ERROR:","The user requested","ResolutionImpossible"）耦合得比较紧；建议把这些匹配模式集中管理，和/或让匹配逻辑对 pip 版本或本地化（locale）变化更健壮一些，避免在非标准输出下悄悄漏掉依赖冲突。- 在
PipInstaller._run_pip_in_process中，_run_pip_main_streaming的第二个返回值被命名为error_lines，但实际上包含了所有 pip 输出行；把这个变量（以及相关参数）重命名为类似output_lines的名字，会让围绕_classify_pip_failure的处理流程更清晰，也能降低后续被误用的概率。
给 AI Agent 的提示词
请根据这次代码评审中的评论进行修改：
## 整体评论
- `_classify_pip_failure` 中对 pip 失败原因的分类，和 pip 输出里的特定英文子串（`"ERROR:"`, `"The user requested"`, `"ResolutionImpossible"`）耦合得比较紧；建议把这些匹配模式集中管理，和/或让匹配逻辑对 pip 版本或本地化（locale）变化更健壮一些，避免在非标准输出下悄悄漏掉依赖冲突。
- 在 `PipInstaller._run_pip_in_process` 中，`_run_pip_main_streaming` 的第二个返回值被命名为 `error_lines`，但实际上包含了所有 pip 输出行；把这个变量（以及相关参数）重命名为类似 `output_lines` 的名字，会让围绕 `_classify_pip_failure` 的处理流程更清晰，也能降低后续被误用的概率。
## 单条评论
### 评论 1
<location path="astrbot/core/utils/pip_installer.py" line_range="152" />
<code_context>
+ return [line], requirement_names
+
+
+def _split_package_install_input(raw_input: str) -> list[str]:
+ """
+ Normalize the user-provided package string into a list of pip args.
</code_context>
<issue_to_address>
**issue (complexity):** 建议为依赖需求处理引入共享的解析结果结构，并为 pip 运行结果定义一个专用类型，以统一解析逻辑并封装错误处理细节。
你可以在保留所有新行为的前提下，通过两个局部的小重构，减少当前分散在各个 helper 之间的复杂“网状”关系：
---
### 1. 统一依赖需求的解析流程
目前 `_iter_normalized_requirement_lines`、`_parse_package_install_line`、`_split_package_install_input`、`_extract_requested_requirements_from_package_input` 和 `_iter_requirements` 都在不同程度上重复实现了解析/归一化逻辑。
你可以引入一个小的解析结果结构，并在这些调用方之间复用，从而去掉重复的分支逻辑，把各种边界情况的处理集中在一个地方。
例如：
```python
from dataclasses import dataclass
@dataclass
class ParsedRequirementLine:
raw: str
tokens: list[str]
requirement_names: set[str]
```
然后：
```python
def _iter_parsed_requirement_lines(raw_input: str) -> Iterator[ParsedRequirementLine]:
for line in _iter_normalized_requirement_lines(raw_input):
tokens, names = _parse_package_install_line(line)
if tokens:
yield ParsedRequirementLine(raw=line, tokens=tokens, requirement_names=names)
```
现在两个 helper 就可以被简化并共享逻辑：
```python
def _split_package_install_input(raw_input: str) -> list[str]:
specs: list[str] = []
for parsed in _iter_parsed_requirement_lines(raw_input):
specs.extend(parsed.tokens)
return specs
def _extract_requested_requirements_from_package_input(raw_input: str) -> set[str]:
requirements: set[str] = set()
for parsed in _iter_parsed_requirement_lines(raw_input):
requirements.update(parsed.requirement_names)
return requirements
```
如果之后需要额外信息（markers、类型等），只需要扩展 `ParsedRequirementLine`，而不用再增加更多平行的 helper。
---
### 2. 在 `_run_pip_in_process` 中集中处理 pip 错误分类
`_run_pip_in_process` 返回 `(int, list[str])`，把底层细节（退出码 + 原始输出行）泄露给了 `PipInstaller.install`。你可以保持 `_StreamingLogWriter` 不变，但让 `_run_pip_in_process` 返回一个领域对象，这样调用方就不需要直接处理“输出行”了。
一个改动最小的方式是引入一个简单的结果类型，并将 `_classify_pip_failure` 移入 `_run_pip_in_process`：
```python
from dataclasses import dataclass
@dataclass
class PipRunResult:
code: int
conflict: DependencyConflictError | None
```
然后：
```python
async def _run_pip_in_process(self, args: list[str]) -> PipRunResult:
pip_main = _get_pip_main()
_patch_distlib_finder_for_frozen_runtime()
original_handlers = list(logging.getLogger().handlers)
try:
result_code, lines = await asyncio.to_thread(
_run_pip_main_streaming, pip_main, args
)
finally:
_cleanup_added_root_handlers(original_handlers)
conflict = _classify_pip_failure(lines)
return PipRunResult(code=result_code, conflict=conflict)
```
这样 `install` 就会更简单，也不再需要接触 `lines`：
```python
result = await self._run_pip_in_process(args)
if result.code != 0:
if result.conflict:
raise result.conflict
raise Exception(f"安装失败，错误码：{result.code}")
```
这在保持当前所有行为（包括冲突分类）的前提下，减少了调用端一层错误处理的复杂度。
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The pip failure classification in
_classify_pip_failureis tightly coupled to specific English substrings from pip ("ERROR:","The user requested","ResolutionImpossible"); consider centralizing these patterns and/or making the matching more resilient to pip version or locale changes to avoid silently missing conflicts in non‑standard outputs. - In
PipInstaller._run_pip_in_processthe second return value from_run_pip_main_streamingis namederror_linesbut actually contains all pip output lines; renaming this variable (and related parameters) to something likeoutput_lineswould make the flow around_classify_pip_failureclearer and reduce the chance of future misuse.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The pip failure classification in `_classify_pip_failure` is tightly coupled to specific English substrings from pip (`"ERROR:"`, `"The user requested"`, `"ResolutionImpossible"`); consider centralizing these patterns and/or making the matching more resilient to pip version or locale changes to avoid silently missing conflicts in non‑standard outputs.
- In `PipInstaller._run_pip_in_process` the second return value from `_run_pip_main_streaming` is named `error_lines` but actually contains all pip output lines; renaming this variable (and related parameters) to something like `output_lines` would make the flow around `_classify_pip_failure` clearer and reduce the chance of future misuse.
## Individual Comments
### Comment 1
<location path="astrbot/core/utils/pip_installer.py" line_range="152" />
<code_context>
+ return [line], requirement_names
+
+
+def _split_package_install_input(raw_input: str) -> list[str]:
+ """
+ Normalize the user-provided package string into a list of pip args.
</code_context>
<issue_to_address>
**issue (complexity):** Consider introducing shared parsed structures for requirement handling and a dedicated pip run result type to unify parsing logic and encapsulate error handling details.
You can keep all the new behavior but reduce the “web” of helpers in two places with small, local refactors:
---
### 1. Unify the requirement parsing pipeline
Right now `_iter_normalized_requirement_lines`, `_parse_package_install_line`, `_split_package_install_input`, `_extract_requested_requirements_from_package_input` and `_iter_requirements` all partially re‑implement parsing/normalization.
You can introduce a single small parsed structure and re‑use it across these callers, which removes the duplicated branching and keeps edge‑case handling in one place.
For example:
```python
from dataclasses import dataclass
@dataclass
class ParsedRequirementLine:
raw: str
tokens: list[str]
requirement_names: set[str]
```
Then:
```python
def _iter_parsed_requirement_lines(raw_input: str) -> Iterator[ParsedRequirementLine]:
for line in _iter_normalized_requirement_lines(raw_input):
tokens, names = _parse_package_install_line(line)
if tokens:
yield ParsedRequirementLine(raw=line, tokens=tokens, requirement_names=names)
```
Now the two helpers can be simplified and share logic:
```python
def _split_package_install_input(raw_input: str) -> list[str]:
specs: list[str] = []
for parsed in _iter_parsed_requirement_lines(raw_input):
specs.extend(parsed.tokens)
return specs
def _extract_requested_requirements_from_package_input(raw_input: str) -> set[str]:
requirements: set[str] = set()
for parsed in _iter_parsed_requirement_lines(raw_input):
requirements.update(parsed.requirement_names)
return requirements
```
If you later need extra info (markers, kinds, etc.), you extend `ParsedRequirementLine` instead of adding more parallel helpers.
---
### 2. Centralize pip error classification in `_run_pip_in_process`
`_run_pip_in_process` returning `(int, list[str])` leaks low‑level concerns (exit codes + raw lines) into `PipInstaller.install`. You can keep `_StreamingLogWriter` as is but have `_run_pip_in_process` return a domain object so callers don’t have to reason about “lines” at all.
One minimal change is to introduce a simple result type and move `_classify_pip_failure` inside `_run_pip_in_process`:
```python
from dataclasses import dataclass
@dataclass
class PipRunResult:
code: int
conflict: DependencyConflictError | None
```
Then:
```python
async def _run_pip_in_process(self, args: list[str]) -> PipRunResult:
pip_main = _get_pip_main()
_patch_distlib_finder_for_frozen_runtime()
original_handlers = list(logging.getLogger().handlers)
try:
result_code, lines = await asyncio.to_thread(
_run_pip_main_streaming, pip_main, args
)
finally:
_cleanup_added_root_handlers(original_handlers)
conflict = _classify_pip_failure(lines)
return PipRunResult(code=result_code, conflict=conflict)
```
`install` then becomes simpler and doesn’t need to touch `lines`:
```python
result = await self._run_pip_in_process(args)
if result.code != 0:
if result.conflict:
raise result.conflict
raise Exception(f"安装失败，错误码：{result.code}")
```
This keeps all current behavior (including conflict classification) but collapses one layer of error‑handling complexity at call sites.
</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
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题
给 AI 代理的提示
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/core/utils/pip_installer.py" line_range="1099" />
<code_context>
- logger.info(f"Pip 包管理器: pip {' '.join(args)}")
- result_code = await self._run_pip_in_process(args)
+ logger.info("Pip 包管理器 argv: %s", ["pip", *args])
+ run_result = _coerce_pip_run_result(await self._run_pip_in_process(args))
</code_context>
<issue_to_address>
**🚨 issue (security):** 避免记录完整的 pip argv，因为其中可能包含凭据或令牌。
记录完整的 `argv` 会在用户通过类似 `--index-url` 这类参数传递内嵌凭据时泄露机密信息。相应地，要么在日志中对敏感部分进行脱敏处理（例如去除 URL 中的认证信息），要么只记录更安全的摘要信息（比如参数数量或 requirements 文件路径），而不是原始参数列表。
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续的审查建议。
Original comment in English
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/pip_installer.py" line_range="1099" />
<code_context>
- logger.info(f"Pip 包管理器: pip {' '.join(args)}")
- result_code = await self._run_pip_in_process(args)
+ logger.info("Pip 包管理器 argv: %s", ["pip", *args])
+ run_result = _coerce_pip_run_result(await self._run_pip_in_process(args))
</code_context>
<issue_to_address>
**🚨 issue (security):** Avoid logging full pip argv because it may contain credentials or tokens.
Logging the full `argv` can expose secrets when users pass options like `--index-url` with embedded credentials. Instead, either redact sensitive pieces (e.g., auth info in URLs) before logging, or log a safer summary (such as argument count or the requirements file path) rather than the raw arguments.
</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
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并留下了一些整体性的反馈：
_StreamingLogWriter.flush的实现会丢弃 pip 输出末尾的尾随空行（因为它忽略了空的line），这意味着虽然中间的空行会被保留，但最后那个空行不会被保留；如果你希望真正保留所有行（包括末尾的空行），可以考虑在 flush 时也追加空的line值。- 在
PluginManager._ensure_plugin_requirements以及相关的调用处，对于通用失败你总是重新抛出Exception，这会让调用方很难区分依赖安装错误和其他插件错误；可以考虑改为抛出一个更具体的异常类型（或包装原始异常），这样插件生命周期代码和 UI 就能对依赖失败做出不同的响应。
给 AI Agent 的 Prompt
请根据以下代码评审意见进行修改：
## 总体意见
- `_StreamingLogWriter.flush` 的实现会丢弃 pip 输出末尾的尾随空行（因为它忽略了空的 `line`），这意味着虽然中间的空行会被保留，但最后那个空行不会被保留；如果你希望真正保留所有行（包括末尾的空行），可以考虑在 flush 时也追加空的 `line` 值。
- 在 `PluginManager._ensure_plugin_requirements` 以及相关的调用处，对于通用失败你总是重新抛出 `Exception`，这会让调用方很难区分依赖安装错误和其他插件错误；可以考虑改为抛出一个更具体的异常类型（或包装原始异常），这样插件生命周期代码和 UI 就能对依赖失败做出不同的响应。
## 逐条评论
### 评论 1
<location path="astrbot/core/utils/pip_installer.py" line_range="597" />
<code_context>
+ )
+
+
+def _coerce_pip_run_result(
+ result: PipRunResult | tuple[int, list[str]],
+) -> PipRunResult:
</code_context>
<issue_to_address>
**issue (complexity):** 考虑移除 `_coerce_pip_run_result`，直接从 `_run_pip_in_process` 返回并使用 `PipRunResult`，以简化 pip 结果处理流程。
你可以通过移除未被真正使用的“类型规整层”，并让 `_run_pip_in_process` 成为 `PipRunResult` 的唯一来源来简化 pip 结果流转。
目前的情况：
- `_run_pip_main_streaming` → `(code, lines)`
- `_run_pip_in_process` → `PipRunResult`（并且已经调用了 `_classify_pip_failure`）
- `_coerce_pip_run_result` 可以接受一个 tuple 或 `PipRunResult`，但实际上只会从 `_run_pip_in_process` 接收到 `PipRunResult`。
这让 `_coerce_pip_run_result` 变成了一个“空壳”抽象，并让你在跟踪控制流时多了一个不必要的概念负担。
你可以在不改变行为的前提下把它折叠掉：
```python
# before
with _core_constraints_file(self.core_dist_name) as constraints_file_path:
if constraints_file_path:
args.extend(["-c", constraints_file_path])
logger.info(
"Pip 包管理器 argv: %s",
["pip", *_redact_pip_args_for_logging(args)],
)
run_result = _coerce_pip_run_result(await self._run_pip_in_process(args))
if run_result.code != 0:
if run_result.conflict:
raise run_result.conflict
raise Exception(f"安装失败，错误码：{run_result.code}")
```
```python
# after – use PipRunResult directly, drop _coerce_pip_run_result
with _core_constraints_file(self.core_dist_name) as constraints_file_path:
if constraints_file_path:
args.extend(["-c", constraints_file_path])
logger.info(
"Pip 包管理器 argv: %s",
["pip", *_redact_pip_args_for_logging(args)],
)
run_result = await self._run_pip_in_process(args)
if run_result.code != 0:
if run_result.conflict:
raise run_result.conflict
raise Exception(f"安装失败，错误码：{run_result.code}")
```
然后你就可以安全地完全删除 `_coerce_pip_run_result`：
```python
# delete this helper; its behavior is now fully handled in _run_pip_in_process
def _coerce_pip_run_result(
result: PipRunResult | tuple[int, list[str]],
) -> PipRunResult:
if isinstance(result, PipRunResult):
return result
code, output_lines = result
conflict = _classify_pip_failure(output_lines) if code != 0 else None
return PipRunResult(code=code, output_lines=output_lines, conflict=conflict)
```
这样可以在保持全部功能（包括冲突检测）不变的前提下：
- 让 `_run_pip_in_process` 成为唯一把原始 pip 输出包装成 `PipRunResult` 并调用 `_classify_pip_failure` 的地方。
- 确保 `install` 始终只处理一个单一且定义清晰的结果类型（`PipRunResult`），降低认知负担。
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The
_StreamingLogWriter.flushimplementation drops a trailing blank line at the end of pip output (because it ignores an emptyline), which means the final empty line is not preserved even though intermediate blank lines are; if you want to truly preserve all lines, including a terminating blank one, consider appending emptylinevalues on flush as well. - In
PluginManager._ensure_plugin_requirementsand the surrounding call sites you always re-raiseExceptionfor generic failures, which makes it hard for callers to distinguish dependency-installation errors from other plugin errors; consider rethrowing a more specific exception type (or wrapping the original) so plugin lifecycle code and UI can react differently to dependency failures.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_StreamingLogWriter.flush` implementation drops a trailing blank line at the end of pip output (because it ignores an empty `line`), which means the final empty line is not preserved even though intermediate blank lines are; if you want to truly preserve all lines, including a terminating blank one, consider appending empty `line` values on flush as well.
- In `PluginManager._ensure_plugin_requirements` and the surrounding call sites you always re-raise `Exception` for generic failures, which makes it hard for callers to distinguish dependency-installation errors from other plugin errors; consider rethrowing a more specific exception type (or wrapping the original) so plugin lifecycle code and UI can react differently to dependency failures.
## Individual Comments
### Comment 1
<location path="astrbot/core/utils/pip_installer.py" line_range="597" />
<code_context>
+ )
+
+
+def _coerce_pip_run_result(
+ result: PipRunResult | tuple[int, list[str]],
+) -> PipRunResult:
</code_context>
<issue_to_address>
**issue (complexity):** Consider removing `_coerce_pip_run_result` and using `PipRunResult` directly from `_run_pip_in_process` to simplify the pip result handling flow.
You can simplify the pip result flow by removing the unused coercion layer and letting `_run_pip_in_process` be the single authority for `PipRunResult`.
Right now:
- `_run_pip_main_streaming` → `(code, lines)`
- `_run_pip_in_process` → `PipRunResult` (and already calls `_classify_pip_failure`)
- `_coerce_pip_run_result` can accept either a tuple or `PipRunResult`, but is only ever given a `PipRunResult` from `_run_pip_in_process`.
This makes `_coerce_pip_run_result` a dead abstraction and doubles the number of concepts you need to keep in mind when following the control flow.
You can collapse this without changing behavior:
```python
# before
with _core_constraints_file(self.core_dist_name) as constraints_file_path:
if constraints_file_path:
args.extend(["-c", constraints_file_path])
logger.info(
"Pip 包管理器 argv: %s",
["pip", *_redact_pip_args_for_logging(args)],
)
run_result = _coerce_pip_run_result(await self._run_pip_in_process(args))
if run_result.code != 0:
if run_result.conflict:
raise run_result.conflict
raise Exception(f"安装失败，错误码：{run_result.code}")
```
```python
# after – use PipRunResult directly, drop _coerce_pip_run_result
with _core_constraints_file(self.core_dist_name) as constraints_file_path:
if constraints_file_path:
args.extend(["-c", constraints_file_path])
logger.info(
"Pip 包管理器 argv: %s",
["pip", *_redact_pip_args_for_logging(args)],
)
run_result = await self._run_pip_in_process(args)
if run_result.code != 0:
if run_result.conflict:
raise run_result.conflict
raise Exception(f"安装失败，错误码：{run_result.code}")
```
Then you can safely remove `_coerce_pip_run_result` entirely:
```python
# delete this helper; its behavior is now fully handled in _run_pip_in_process
def _coerce_pip_run_result(
result: PipRunResult | tuple[int, list[str]],
) -> PipRunResult:
if isinstance(result, PipRunResult):
return result
code, output_lines = result
conflict = _classify_pip_failure(output_lines) if code != 0 else None
return PipRunResult(code=code, output_lines=output_lines, conflict=conflict)
```
This keeps all functionality (including conflict detection) intact, while:
- Making `_run_pip_in_process` the only place that wraps raw pip output into `PipRunResult` and calls `_classify_pip_failure`.
- Ensuring `install` always deals with a single, well-defined result type (`PipRunResult`), reducing cognitive load.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
astrbot/core/utils/pip_installer.py
Outdated
| ) | ||
|
|
||
|
|
||
| def _coerce_pip_run_result( |
Contributor
There was a problem hiding this comment.
issue (complexity): 考虑移除 _coerce_pip_run_result，直接从 _run_pip_in_process 返回并使用 PipRunResult，以简化 pip 结果处理流程。
你可以通过移除未被真正使用的“类型规整层”，并让 _run_pip_in_process 成为 PipRunResult 的唯一来源来简化 pip 结果流转。
目前的情况：
_run_pip_main_streaming→(code, lines)_run_pip_in_process→PipRunResult（并且已经调用了_classify_pip_failure）_coerce_pip_run_result可以接受一个 tuple 或PipRunResult，但实际上只会从_run_pip_in_process接收到PipRunResult。
这让 _coerce_pip_run_result 变成了一个“空壳”抽象，并让你在跟踪控制流时多了一个不必要的概念负担。
你可以在不改变行为的前提下把它折叠掉：
# before
with _core_constraints_file(self.core_dist_name) as constraints_file_path:
if constraints_file_path:
args.extend(["-c", constraints_file_path])
logger.info(
"Pip 包管理器 argv: %s",
["pip", *_redact_pip_args_for_logging(args)],
)
run_result = _coerce_pip_run_result(await self._run_pip_in_process(args))
if run_result.code != 0:
if run_result.conflict:
raise run_result.conflict
raise Exception(f"安装失败，错误码：{run_result.code}")# after – use PipRunResult directly, drop _coerce_pip_run_result
with _core_constraints_file(self.core_dist_name) as constraints_file_path:
if constraints_file_path:
args.extend(["-c", constraints_file_path])
logger.info(
"Pip 包管理器 argv: %s",
["pip", *_redact_pip_args_for_logging(args)],
)
run_result = await self._run_pip_in_process(args)
if run_result.code != 0:
if run_result.conflict:
raise run_result.conflict
raise Exception(f"安装失败，错误码：{run_result.code}")然后你就可以安全地完全删除 _coerce_pip_run_result：
# delete this helper; its behavior is now fully handled in _run_pip_in_process
def _coerce_pip_run_result(
result: PipRunResult | tuple[int, list[str]],
) -> PipRunResult:
if isinstance(result, PipRunResult):
return result
code, output_lines = result
conflict = _classify_pip_failure(output_lines) if code != 0 else None
return PipRunResult(code=code, output_lines=output_lines, conflict=conflict)这样可以在保持全部功能（包括冲突检测）不变的前提下：
- 让
_run_pip_in_process成为唯一把原始 pip 输出包装成PipRunResult并调用_classify_pip_failure的地方。 - 确保
install始终只处理一个单一且定义清晰的结果类型（PipRunResult），降低认知负担。
Original comment in English
issue (complexity): Consider removing _coerce_pip_run_result and using PipRunResult directly from _run_pip_in_process to simplify the pip result handling flow.
You can simplify the pip result flow by removing the unused coercion layer and letting _run_pip_in_process be the single authority for PipRunResult.
Right now:
_run_pip_main_streaming→(code, lines)_run_pip_in_process→PipRunResult(and already calls_classify_pip_failure)_coerce_pip_run_resultcan accept either a tuple orPipRunResult, but is only ever given aPipRunResultfrom_run_pip_in_process.
This makes _coerce_pip_run_result a dead abstraction and doubles the number of concepts you need to keep in mind when following the control flow.
You can collapse this without changing behavior:
# before
with _core_constraints_file(self.core_dist_name) as constraints_file_path:
if constraints_file_path:
args.extend(["-c", constraints_file_path])
logger.info(
"Pip 包管理器 argv: %s",
["pip", *_redact_pip_args_for_logging(args)],
)
run_result = _coerce_pip_run_result(await self._run_pip_in_process(args))
if run_result.code != 0:
if run_result.conflict:
raise run_result.conflict
raise Exception(f"安装失败，错误码：{run_result.code}")# after – use PipRunResult directly, drop _coerce_pip_run_result
with _core_constraints_file(self.core_dist_name) as constraints_file_path:
if constraints_file_path:
args.extend(["-c", constraints_file_path])
logger.info(
"Pip 包管理器 argv: %s",
["pip", *_redact_pip_args_for_logging(args)],
)
run_result = await self._run_pip_in_process(args)
if run_result.code != 0:
if run_result.conflict:
raise run_result.conflict
raise Exception(f"安装失败，错误码：{run_result.code}")Then you can safely remove _coerce_pip_run_result entirely:
# delete this helper; its behavior is now fully handled in _run_pip_in_process
def _coerce_pip_run_result(
result: PipRunResult | tuple[int, list[str]],
) -> PipRunResult:
if isinstance(result, PipRunResult):
return result
code, output_lines = result
conflict = _classify_pip_failure(output_lines) if code != 0 else None
return PipRunResult(code=code, output_lines=output_lines, conflict=conflict)This keeps all functionality (including conflict detection) intact, while:
- Making
_run_pip_in_processthe only place that wraps raw pip output intoPipRunResultand calls_classify_pip_failure. - Ensuring
installalways deals with a single, well-defined result type (PipRunResult), reducing cognitive load.
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并给出了一些整体层面的反馈：
- 当策略为
INSTALL_MISSING时，_determine_requirement_install_strategy与_ensure_plugin_requirements这条路径会记录日志说“只安装缺失的依赖”，但实际上仍然对完整的依赖文件调用pip_installer.install(requirements_path=...)；如果真正的意图是只安装缺失的包，那么你可能需要只传入缺失的包名（或者调整日志消息/策略命名，使其与当前行为保持一致）。 _classify_pip_failure中的 pip 失败分类依赖于诸如The user requested和(constraint)之类的英文子串；这在不同的 pip 版本或本地化输出下可能会比较脆弱，因此建议考虑防止误判，和/或在这些标记不存在时添加回退逻辑。- 当无法直接找到
AstrBot时，_get_core_constraints最终可能会扫描所有已安装的发行版及其元数据；为了避免在多次安装过程中重复付出开销，可以考虑在进程生命周期内缓存推导出的core_dist_name以及计算出的 constraints。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_determine_requirement_install_strategy` + `_ensure_plugin_requirements` path logs that it "只安装缺失的依赖" when strategy is `INSTALL_MISSING`, but still calls `pip_installer.install(requirements_path=...)` on the full requirements file; if the intent is to install only missing packages you may want to pass just the missing names (or adjust the log message/strategy naming to match current behavior).
- The pip failure classification in `_classify_pip_failure` relies on English substrings like `The user requested` and `(constraint)`; this may be brittle across pip versions or localized outputs, so consider guarding against false positives and/or adding a fallback when these markers are not present.
- `_get_core_constraints` can end up scanning all installed distributions and their metadata when `AstrBot` is not directly found; to avoid repeated cost during multiple installs, consider caching the derived `core_dist_name` and the computed constraints for the process lifetime.
## Individual Comments
### Comment 1
<location path="astrbot/core/utils/pip_installer.py" line_range="131" />
<code_context>
- module = importlib.import_module(module_name)
- except Exception:
- return False
+def _iter_normalized_requirement_lines(raw_input: str) -> Iterator[str]:
+ normalized = raw_input.strip()
+ if not normalized:
</code_context>
<issue_to_address>
**issue (complexity):** 可以考虑将需求行规范化和解析的辅助函数合并到一个 `_iter_parsed_requirement_lines` 生成器中，以简化数据流并让解析逻辑更容易理解。
你可以在不丢失任何新行为的前提下，去掉需求解析流程中的整整一层间接调用。
目前，一行输入会依次经过：
`_iter_normalized_requirement_lines` → `_parse_requirement_line` → `_iter_parsed_requirement_lines`
但你真正对外（相对）可见、且实际使用的 API 只有 `_iter_parsed_requirement_lines`。你可以把前两个函数内联到一个生成器中，这个生成器负责：
- 去掉注释/空行，
- 处理 `Requirement(line)` 与 `shlex` 解析两种分支，
- 填充 `tokens` 和 `requirement_names`，
都在同一个地方完成。
例如，可以用一个自包含的生成器替换这三个函数：
```py
def _iter_normalized_requirement_lines(raw_input: str) -> Iterator[str]:
normalized = raw_input.strip()
if not normalized:
return
for line in normalized.splitlines():
stripped = _strip_inline_requirement_comment(line)
if stripped:
yield stripped
def _parse_requirement_line(line: str) -> ParsedRequirementLine:
requirement_names: set[str] = set()
try:
req = Requirement(line)
except InvalidRequirement:
tokens = shlex.split(line)
if not tokens:
return ParsedRequirementLine(
raw=line,
tokens=(),
requirement_names=frozenset(),
)
if tokens[0] in {"-e", "--editable"} or tokens[0].startswith("--editable="):
name = _extract_requirement_name(line)
if name:
requirement_names.add(name)
return ParsedRequirementLine(
raw=line,
tokens=tuple(tokens),
requirement_names=frozenset(requirement_names),
)
if tokens[0].startswith("-"):
name = _extract_requirement_name(line)
if name:
requirement_names.add(name)
return ParsedRequirementLine(
raw=line,
tokens=tuple(tokens),
requirement_names=frozenset(requirement_names),
)
for token in tokens:
name = _extract_requirement_name(token)
if name:
requirement_names.add(name)
return ParsedRequirementLine(
raw=line,
tokens=tuple(tokens),
requirement_names=frozenset(requirement_names),
)
requirement_names.add(_canonicalize_distribution_name(req.name))
return ParsedRequirementLine(
raw=line,
tokens=(line,),
requirement_names=frozenset(requirement_names),
)
def _iter_parsed_requirement_lines(raw_input: str) -> Iterator[ParsedRequirementLine]:
for line in _iter_normalized_requirement_lines(raw_input):
parsed = _parse_requirement_line(line)
if parsed.tokens:
yield parsed
```
替换为单个自包含的生成器：
```py
def _iter_parsed_requirement_lines(raw_input: str) -> Iterator[ParsedRequirementLine]:
"""
Yield ParsedRequirementLine instances for each non-empty, non-comment line.
Handles:
- Inline comments
- Editable / option lines
- Requirement() parsing where possible
"""
for raw_line in raw_input.splitlines():
# normalize + strip inline comment
line = _strip_inline_requirement_comment(raw_line)
if not line:
continue
requirement_names: set[str] = set()
try:
req = Requirement(line)
except InvalidRequirement:
tokens = shlex.split(line)
if not tokens:
continue
if tokens[0] in {"-e", "--editable"} or tokens[0].startswith("--editable="):
name = _extract_requirement_name(line)
if name:
requirement_names.add(name)
yield ParsedRequirementLine(
raw=line,
tokens=tuple(tokens),
requirement_names=frozenset(requirement_names),
)
continue
if tokens[0].startswith("-"):
name = _extract_requirement_name(line)
if name:
requirement_names.add(name)
yield ParsedRequirementLine(
raw=line,
tokens=tuple(tokens),
requirement_names=frozenset(requirement_names),
)
continue
for token in tokens:
name = _extract_requirement_name(token)
if name:
requirement_names.add(name)
yield ParsedRequirementLine(
raw=line,
tokens=tuple(tokens),
requirement_names=frozenset(requirement_names),
)
continue
# Successfully parsed as a Requirement
requirement_names.add(_canonicalize_distribution_name(req.name))
yield ParsedRequirementLine(
raw=line,
tokens=(line,),
requirement_names=frozenset(requirement_names),
)
```
然后你就可以删除 `_iter_normalized_requirement_lines` 和 `_parse_requirement_line`。
这样做可以：
- 保留当前所有行为（`Requirement` 解析、`shlex` 回退、注释剥离），
- 去掉两个辅助函数以及一层“line → normalized → parsed”的调用链，
- 让调用方（如 `_split_package_install_input`、`_extract_requested_requirements_from_package_input` 和 `_iter_requirements`）更容易追踪解析路径。
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据反馈改进后续评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The
_determine_requirement_install_strategy+_ensure_plugin_requirementspath logs that it "只安装缺失的依赖" when strategy isINSTALL_MISSING, but still callspip_installer.install(requirements_path=...)on the full requirements file; if the intent is to install only missing packages you may want to pass just the missing names (or adjust the log message/strategy naming to match current behavior). - The pip failure classification in
_classify_pip_failurerelies on English substrings likeThe user requestedand(constraint); this may be brittle across pip versions or localized outputs, so consider guarding against false positives and/or adding a fallback when these markers are not present. _get_core_constraintscan end up scanning all installed distributions and their metadata whenAstrBotis not directly found; to avoid repeated cost during multiple installs, consider caching the derivedcore_dist_nameand the computed constraints for the process lifetime.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_determine_requirement_install_strategy` + `_ensure_plugin_requirements` path logs that it "只安装缺失的依赖" when strategy is `INSTALL_MISSING`, but still calls `pip_installer.install(requirements_path=...)` on the full requirements file; if the intent is to install only missing packages you may want to pass just the missing names (or adjust the log message/strategy naming to match current behavior).
- The pip failure classification in `_classify_pip_failure` relies on English substrings like `The user requested` and `(constraint)`; this may be brittle across pip versions or localized outputs, so consider guarding against false positives and/or adding a fallback when these markers are not present.
- `_get_core_constraints` can end up scanning all installed distributions and their metadata when `AstrBot` is not directly found; to avoid repeated cost during multiple installs, consider caching the derived `core_dist_name` and the computed constraints for the process lifetime.
## Individual Comments
### Comment 1
<location path="astrbot/core/utils/pip_installer.py" line_range="131" />
<code_context>
- module = importlib.import_module(module_name)
- except Exception:
- return False
+def _iter_normalized_requirement_lines(raw_input: str) -> Iterator[str]:
+ normalized = raw_input.strip()
+ if not normalized:
</code_context>
<issue_to_address>
**issue (complexity):** Consider inlining the requirement-line normalization and parsing helpers into a single `_iter_parsed_requirement_lines` generator to simplify the data flow and make the parsing logic easier to understand.
You can remove a full layer of indirection from the requirement parsing pipeline without losing any of the new behavior.
Right now a single line flows through:
`_iter_normalized_requirement_lines` → `_parse_requirement_line` → `_iter_parsed_requirement_lines`
but the only public-ish API you actually use is `_iter_parsed_requirement_lines`. You can inline the other two into one generator that:
- strips comments / empty lines,
- handles `Requirement(line)` vs. `shlex` parsing,
- fills `tokens` and `requirement_names`,
all in one place.
For example, replace the trio:
```py
def _iter_normalized_requirement_lines(raw_input: str) -> Iterator[str]:
normalized = raw_input.strip()
if not normalized:
return
for line in normalized.splitlines():
stripped = _strip_inline_requirement_comment(line)
if stripped:
yield stripped
def _parse_requirement_line(line: str) -> ParsedRequirementLine:
requirement_names: set[str] = set()
try:
req = Requirement(line)
except InvalidRequirement:
tokens = shlex.split(line)
if not tokens:
return ParsedRequirementLine(
raw=line,
tokens=(),
requirement_names=frozenset(),
)
if tokens[0] in {"-e", "--editable"} or tokens[0].startswith("--editable="):
name = _extract_requirement_name(line)
if name:
requirement_names.add(name)
return ParsedRequirementLine(
raw=line,
tokens=tuple(tokens),
requirement_names=frozenset(requirement_names),
)
if tokens[0].startswith("-"):
name = _extract_requirement_name(line)
if name:
requirement_names.add(name)
return ParsedRequirementLine(
raw=line,
tokens=tuple(tokens),
requirement_names=frozenset(requirement_names),
)
for token in tokens:
name = _extract_requirement_name(token)
if name:
requirement_names.add(name)
return ParsedRequirementLine(
raw=line,
tokens=tuple(tokens),
requirement_names=frozenset(requirement_names),
)
requirement_names.add(_canonicalize_distribution_name(req.name))
return ParsedRequirementLine(
raw=line,
tokens=(line,),
requirement_names=frozenset(requirement_names),
)
def _iter_parsed_requirement_lines(raw_input: str) -> Iterator[ParsedRequirementLine]:
for line in _iter_normalized_requirement_lines(raw_input):
parsed = _parse_requirement_line(line)
if parsed.tokens:
yield parsed
```
with a single, self‑contained generator:
```py
def _iter_parsed_requirement_lines(raw_input: str) -> Iterator[ParsedRequirementLine]:
"""
Yield ParsedRequirementLine instances for each non-empty, non-comment line.
Handles:
- Inline comments
- Editable / option lines
- Requirement() parsing where possible
"""
for raw_line in raw_input.splitlines():
# normalize + strip inline comment
line = _strip_inline_requirement_comment(raw_line)
if not line:
continue
requirement_names: set[str] = set()
try:
req = Requirement(line)
except InvalidRequirement:
tokens = shlex.split(line)
if not tokens:
continue
if tokens[0] in {"-e", "--editable"} or tokens[0].startswith("--editable="):
name = _extract_requirement_name(line)
if name:
requirement_names.add(name)
yield ParsedRequirementLine(
raw=line,
tokens=tuple(tokens),
requirement_names=frozenset(requirement_names),
)
continue
if tokens[0].startswith("-"):
name = _extract_requirement_name(line)
if name:
requirement_names.add(name)
yield ParsedRequirementLine(
raw=line,
tokens=tuple(tokens),
requirement_names=frozenset(requirement_names),
)
continue
for token in tokens:
name = _extract_requirement_name(token)
if name:
requirement_names.add(name)
yield ParsedRequirementLine(
raw=line,
tokens=tuple(tokens),
requirement_names=frozenset(requirement_names),
)
continue
# Successfully parsed as a Requirement
requirement_names.add(_canonicalize_distribution_name(req.name))
yield ParsedRequirementLine(
raw=line,
tokens=(line,),
requirement_names=frozenset(requirement_names),
)
```
Then you can delete `_iter_normalized_requirement_lines` and `_parse_requirement_line`.
This:
- keeps all current behaviors (`Requirement` parsing, `shlex` fallback, comment stripping),
- removes two helper functions and one level of “line → normalized → parsed” flow,
- makes the parsing path easier to trace for callers like `_split_package_install_input`, `_extract_requested_requirements_from_package_input`, and `_iter_requirements`.
</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
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
嗨——我已经审查了你的更改，一切看起来都很棒！
帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续的评审。
Original comment in English
Hey - I've reviewed your changes and they look great!
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
核心目标
解决插件安装/更新/上传流程中，依赖安装滞后于首次加载尝试的问题。同时优化 Dashboard 手动 pip 安装的输入解析能力，并新增核心依赖防降级保护机制与精准冲突告警。
主要变更
1. 核心依赖防降级保护与精准告警
pip install前，自动提取核心依赖。支持快速路径（直接查找 "AstrBot"）与动态包名发现回退机制。-c constraints.txt) 传递给pip，确保插件安装过程中不会意外降级 AstrBot 的核心库。pip输出中自动提取具体的冲突版本信息，并直接展示在错误消息中。2. 代码架构重构与逻辑精简 (Final Refined)
_iter_requirements迭代器，将文件读取、注释剥离、Token 拆分和Requirement解析整合为单一高效管线，消除了重复分词并修复了 marker 引号丢失问题。StarManager现在会捕获并正确重新抛出依赖安装异常。这意味着依赖冲突将硬性阻止受损插件的加载，避免了运行时的不可预知错误。PipInstaller的冗余包装方法，对外直接暴露模块级 API，降低了调用方 (StarManager) 的认知负担。strip()丢弃空白行，从而完整保留了pip输出的缩进和原始格式。3. 安全扫描与测试增强
PluginManager构造函数签名、异常透传行为以及日志缓冲区合并逻辑。验证与测试
ruff格式化与 Lint 检查。关联工作
基于
docs/plans/2026-03-09-plugin-dependency-install-design.md完成实现。由 Sourcery 提供的摘要
引入更健壮的插件依赖安装机制，包括核心库降级保护、更智能的 pip 参数解析，以及在插件生命周期操作中，
PluginManager与PipInstaller之间更紧密、更安全的集成。新功能：
错误修复：
增强优化：
PipInstaller，实现 pip 输出的流式处理，对依赖冲突进行分类并提供详细错误信息，同时尊重用户提供的索引参数。PluginManager中，使安装、更新、重载以及插件失败后的恢复都在工作流中执行依赖检查和依赖安装。测试：
PipInstaller测试范围，涵盖流式输出行为、依赖预检查逻辑，以及多种 pip 输入格式和索引参数。PluginManager测试，用于验证依赖安装顺序、错误传播，以及在依赖预检查成功、失败或未发现缺失依赖时的行为。Original summary in English
由 Sourcery 生成的总结
通过预安装检查、核心库版本保护，以及将基于 pip 的安装与插件生命周期更紧密地集成，改进插件依赖管理。
新功能：
缺陷修复：
增强改进：
测试：
Original summary in English
Summary by Sourcery
改进插件依赖安装流程和与 pip 的集成，引入核心依赖降级保护、更智能的依赖解析，以及在
PluginManager与PipInstaller之间更紧密的协作。新特性：
缺陷修复：
增强改进：
PluginManager中，使安装、更新、重载以及失败插件恢复都执行一致的依赖检查和安装流程。测试：
PipInstaller测试范围，覆盖流式输出行为、依赖预检查逻辑以及多种 pip 输入/index 格式。PluginManager测试，用于验证依赖安装顺序、错误传播机制，以及在安装、更新、重载和失败插件恢复路径中的行为。Original summary in English
Summary by Sourcery
Improve plugin dependency installation flow and pip integration, adding core dependency downgrade protection, smarter requirement parsing, and tighter coupling between PluginManager and PipInstaller.
New Features:
Bug Fixes:
Enhancements:
Tests:
Original summary in English
由 Sourcery 生成的总结
通过预安装检查、核心库版本保护，以及将基于 pip 的安装与插件生命周期更紧密地集成，改进插件依赖管理。
新功能：
缺陷修复：
增强改进：
测试：
Original summary in English
Summary by Sourcery
改进插件依赖安装流程和与 pip 的集成，引入核心依赖降级保护、更智能的依赖解析，以及在
PluginManager与PipInstaller之间更紧密的协作。新特性：
缺陷修复：
增强改进：
PluginManager中，使安装、更新、重载以及失败插件恢复都执行一致的依赖检查和安装流程。测试：
PipInstaller测试范围，覆盖流式输出行为、依赖预检查逻辑以及多种 pip 输入/index 格式。PluginManager测试，用于验证依赖安装顺序、错误传播机制，以及在安装、更新、重载和失败插件恢复路径中的行为。Original summary in English
Summary by Sourcery
Improve plugin dependency installation flow and pip integration, adding core dependency downgrade protection, smarter requirement parsing, and tighter coupling between PluginManager and PipInstaller.
New Features:
Bug Fixes:
Enhancements:
Tests:
Original summary in English
由 Sourcery 生成的总结
通过预安装检查、核心库版本保护，以及将基于 pip 的安装与插件生命周期更紧密地集成，改进插件依赖管理。
新功能：
缺陷修复：
增强改进：
测试：
Original summary in English
Summary by Sourcery
改进插件依赖安装流程和与 pip 的集成，引入核心依赖降级保护、更智能的依赖解析，以及在
PluginManager与PipInstaller之间更紧密的协作。新特性：
缺陷修复：
增强改进：
PluginManager中，使安装、更新、重载以及失败插件恢复都执行一致的依赖检查和安装流程。测试：
PipInstaller测试范围，覆盖流式输出行为、依赖预检查逻辑以及多种 pip 输入/index 格式。PluginManager测试，用于验证依赖安装顺序、错误传播机制，以及在安装、更新、重载和失败插件恢复路径中的行为。Original summary in English
Summary by Sourcery
Improve plugin dependency installation flow and pip integration, adding core dependency downgrade protection, smarter requirement parsing, and tighter coupling between PluginManager and PipInstaller.
New Features:
Bug Fixes:
Enhancements:
Tests:
Original summary in English
由 Sourcery 生成的总结
通过预安装检查、核心库版本保护，以及将基于 pip 的安装与插件生命周期更紧密地集成，改进插件依赖管理。
新功能：
缺陷修复：
增强改进：
测试：
Original summary in English
Summary by Sourcery
改进插件依赖安装流程和与 pip 的集成，引入核心依赖降级保护、更智能的依赖解析，以及在
PluginManager与PipInstaller之间更紧密的协作。新特性：
缺陷修复：
增强改进：
PluginManager中，使安装、更新、重载以及失败插件恢复都执行一致的依赖检查和安装流程。测试：
PipInstaller测试范围，覆盖流式输出行为、依赖预检查逻辑以及多种 pip 输入/index 格式。PluginManager测试，用于验证依赖安装顺序、错误传播机制，以及在安装、更新、重载和失败插件恢复路径中的行为。Original summary in English
Summary by Sourcery
Improve plugin dependency installation flow and pip integration, adding core dependency downgrade protection, smarter requirement parsing, and tighter coupling between PluginManager and PipInstaller.
New Features:
Bug Fixes:
Enhancements:
Tests:
新功能：
pip安装过程中降级 AstrBot 核心库。错误修复：
pip时按行流式输出stdout和stderr，不会丢失空行，也不会改变其顺序。pip包输入字符串的解析，包括 markers、可编辑的 VCS URL、嵌套的依赖文件以及 index URL 选项，以避免错误安装。增强：
PluginManager工作流程，使安装、更新、重载和失败插件恢复都在生命周期中执行依赖检查和安装。pip依赖冲突的结构化分类，提供清晰的错误消息，并对核心库冲突进行显式处理。pip参数构造逻辑，更好地尊重用户提供的 index 覆盖设置，并更安全地定位桌面客户端的site-packages目录。测试：
PipInstaller测试，覆盖流式输出行为、依赖预检查逻辑以及多种pip输入格式和 index 选项。PluginManager测试，以验证依赖安装顺序、错误传播，以及在预检查成功、失败或没有缺失依赖时的行为。Original summary in English
由 Sourcery 生成的总结
通过预安装检查、核心库版本保护，以及将基于 pip 的安装与插件生命周期更紧密地集成，改进插件依赖管理。
新功能：
缺陷修复：
增强改进：
测试：
Original summary in English
Summary by Sourcery
改进插件依赖安装流程和与 pip 的集成，引入核心依赖降级保护、更智能的依赖解析，以及在
PluginManager与PipInstaller之间更紧密的协作。新特性：
缺陷修复：
增强改进：
PluginManager中，使安装、更新、重载以及失败插件恢复都执行一致的依赖检查和安装流程。测试：
PipInstaller测试范围，覆盖流式输出行为、依赖预检查逻辑以及多种 pip 输入/index 格式。PluginManager测试，用于验证依赖安装顺序、错误传播机制，以及在安装、更新、重载和失败插件恢复路径中的行为。Original summary in English
Summary by Sourcery
Improve plugin dependency installation flow and pip integration, adding core dependency downgrade protection, smarter requirement parsing, and tighter coupling between PluginManager and PipInstaller.
New Features:
Bug Fixes:
Enhancements:
Tests:
Original summary in English
由 Sourcery 生成的总结
通过预安装检查、核心库版本保护，以及将基于 pip 的安装与插件生命周期更紧密地集成，改进插件依赖管理。
新功能：
缺陷修复：
增强改进：
测试：
Original summary in English
Summary by Sourcery
改进插件依赖安装流程和与 pip 的集成，引入核心依赖降级保护、更智能的依赖解析，以及在
PluginManager与PipInstaller之间更紧密的协作。新特性：
缺陷修复：
增强改进：
PluginManager中，使安装、更新、重载以及失败插件恢复都执行一致的依赖检查和安装流程。测试：
PipInstaller测试范围，覆盖流式输出行为、依赖预检查逻辑以及多种 pip 输入/index 格式。PluginManager测试，用于验证依赖安装顺序、错误传播机制，以及在安装、更新、重载和失败插件恢复路径中的行为。Original summary in English
Summary by Sourcery
Improve plugin dependency installation flow and pip integration, adding core dependency downgrade protection, smarter requirement parsing, and tighter coupling between PluginManager and PipInstaller.
New Features:
Bug Fixes:
Enhancements:
Tests:
Original summary in English
由 Sourcery 生成的总结
通过预安装检查、核心库版本保护，以及将基于 pip 的安装与插件生命周期更紧密地集成，改进插件依赖管理。
新功能：
缺陷修复：
增强改进：
测试：
Original summary in English
Summary by Sourcery
改进插件依赖安装流程和与 pip 的集成，引入核心依赖降级保护、更智能的依赖解析，以及在
PluginManager与PipInstaller之间更紧密的协作。新特性：
缺陷修复：
增强改进：
PluginManager中，使安装、更新、重载以及失败插件恢复都执行一致的依赖检查和安装流程。测试：
PipInstaller测试范围，覆盖流式输出行为、依赖预检查逻辑以及多种 pip 输入/index 格式。PluginManager测试，用于验证依赖安装顺序、错误传播机制，以及在安装、更新、重载和失败插件恢复路径中的行为。Original summary in English
Summary by Sourcery
Improve plugin dependency installation flow and pip integration, adding core dependency downgrade protection, smarter requirement parsing, and tighter coupling between PluginManager and PipInstaller.
New Features:
Bug Fixes:
Enhancements:
Tests:
Original summary in English
由 Sourcery 生成的总结
通过预安装检查、核心库版本保护，以及将基于 pip 的安装与插件生命周期更紧密地集成，改进插件依赖管理。
新功能：
缺陷修复：
增强改进：
测试：
Original summary in English
Summary by Sourcery
改进插件依赖安装流程和与 pip 的集成，引入核心依赖降级保护、更智能的依赖解析，以及在
PluginManager与PipInstaller之间更紧密的协作。新特性：
缺陷修复：
增强改进：
PluginManager中，使安装、更新、重载以及失败插件恢复都执行一致的依赖检查和安装流程。测试：
PipInstaller测试范围，覆盖流式输出行为、依赖预检查逻辑以及多种 pip 输入/index 格式。PluginManager测试，用于验证依赖安装顺序、错误传播机制，以及在安装、更新、重载和失败插件恢复路径中的行为。Original summary in English
Summary by Sourcery
Improve plugin dependency installation flow and pip integration, adding core dependency downgrade protection, smarter requirement parsing, and tighter coupling between PluginManager and PipInstaller.
New Features:
Bug Fixes:
Enhancements:
Tests:
新功能：
PluginManager对外提供。错误修复：
增强项：
PipInstaller，从用户输入和配置中构建 pip 参数，同时尊重显式的索引覆盖设置以及面向桌面环境的安装需求。PluginManager中集中统一插件依赖检查，使安装、更新、重载以及失败插件恢复等流程都统一执行依赖检查与安装。测试：
PipInstaller测试范围，以覆盖流式输出行为、依赖解析以及索引处理。PluginManager测试，以验证依赖预检查行为、错误传播机制，以及依赖安装相对于插件加载的正确执行顺序。Original summary in English
由 Sourcery 生成的总结
通过预安装检查、核心库版本保护，以及将基于 pip 的安装与插件生命周期更紧密地集成，改进插件依赖管理。
新功能：
缺陷修复：
增强改进：
测试：
Original summary in English
Summary by Sourcery
改进插件依赖安装流程和与 pip 的集成，引入核心依赖降级保护、更智能的依赖解析，以及在
PluginManager与PipInstaller之间更紧密的协作。新特性：
缺陷修复：
增强改进：
PluginManager中，使安装、更新、重载以及失败插件恢复都执行一致的依赖检查和安装流程。测试：
PipInstaller测试范围，覆盖流式输出行为、依赖预检查逻辑以及多种 pip 输入/index 格式。PluginManager测试，用于验证依赖安装顺序、错误传播机制，以及在安装、更新、重载和失败插件恢复路径中的行为。Original summary in English
Summary by Sourcery
Improve plugin dependency installation flow and pip integration, adding core dependency downgrade protection, smarter requirement parsing, and tighter coupling between PluginManager and PipInstaller.
New Features:
Bug Fixes:
Enhancements:
Tests:
Original summary in English
由 Sourcery 生成的总结
通过预安装检查、核心库版本保护，以及将基于 pip 的安装与插件生命周期更紧密地集成，改进插件依赖管理。
新功能：
缺陷修复：
增强改进：
测试：
Original summary in English
Summary by Sourcery
改进插件依赖安装流程和与 pip 的集成，引入核心依赖降级保护、更智能的依赖解析，以及在
PluginManager与PipInstaller之间更紧密的协作。新特性：
缺陷修复：
增强改进：
PluginManager中，使安装、更新、重载以及失败插件恢复都执行一致的依赖检查和安装流程。测试：
PipInstaller测试范围，覆盖流式输出行为、依赖预检查逻辑以及多种 pip 输入/index 格式。PluginManager测试，用于验证依赖安装顺序、错误传播机制，以及在安装、更新、重载和失败插件恢复路径中的行为。Original summary in English
Summary by Sourcery
Improve plugin dependency installation flow and pip integration, adding core dependency downgrade protection, smarter requirement parsing, and tighter coupling between PluginManager and PipInstaller.
New Features:
Bug Fixes:
Enhancements:
Tests:
Original summary in English
由 Sourcery 生成的总结
通过预安装检查、核心库版本保护，以及将基于 pip 的安装与插件生命周期更紧密地集成，改进插件依赖管理。
新功能：
缺陷修复：
增强改进：
测试：
Original summary in English
Summary by Sourcery
改进插件依赖安装流程和与 pip 的集成，引入核心依赖降级保护、更智能的依赖解析，以及在
PluginManager与PipInstaller之间更紧密的协作。新特性：
缺陷修复：
增强改进：
PluginManager中，使安装、更新、重载以及失败插件恢复都执行一致的依赖检查和安装流程。测试：
PipInstaller测试范围，覆盖流式输出行为、依赖预检查逻辑以及多种 pip 输入/index 格式。PluginManager测试，用于验证依赖安装顺序、错误传播机制，以及在安装、更新、重载和失败插件恢复路径中的行为。Original summary in English
Summary by Sourcery
Improve plugin dependency installation flow and pip integration, adding core dependency downgrade protection, smarter requirement parsing, and tighter coupling between PluginManager and PipInstaller.
New Features:
Bug Fixes:
Enhancements:
Tests:
Original summary in English
由 Sourcery 生成的总结
通过预安装检查、核心库版本保护，以及将基于 pip 的安装与插件生命周期更紧密地集成，改进插件依赖管理。
新功能：
缺陷修复：
增强改进：
测试：
Original summary in English
Summary by Sourcery
改进插件依赖安装流程和与 pip 的集成，引入核心依赖降级保护、更智能的依赖解析，以及在
PluginManager与PipInstaller之间更紧密的协作。新特性：
缺陷修复：
增强改进：
PluginManager中，使安装、更新、重载以及失败插件恢复都执行一致的依赖检查和安装流程。测试：
PipInstaller测试范围，覆盖流式输出行为、依赖预检查逻辑以及多种 pip 输入/index 格式。PluginManager测试，用于验证依赖安装顺序、错误传播机制，以及在安装、更新、重载和失败插件恢复路径中的行为。Original summary in English
Summary by Sourcery
Improve plugin dependency installation flow and pip integration, adding core dependency downgrade protection, smarter requirement parsing, and tighter coupling between PluginManager and PipInstaller.
New Features:
Bug Fixes:
Enhancements:
Tests:
新功能：
pip安装过程中降级 AstrBot 核心库。错误修复：
pip时按行流式输出stdout和stderr，不会丢失空行，也不会改变其顺序。pip包输入字符串的解析，包括 markers、可编辑的 VCS URL、嵌套的依赖文件以及 index URL 选项，以避免错误安装。增强：
PluginManager工作流程，使安装、更新、重载和失败插件恢复都在生命周期中执行依赖检查和安装。pip依赖冲突的结构化分类，提供清晰的错误消息，并对核心库冲突进行显式处理。pip参数构造逻辑，更好地尊重用户提供的 index 覆盖设置，并更安全地定位桌面客户端的site-packages目录。测试：
PipInstaller测试，覆盖流式输出行为、依赖预检查逻辑以及多种pip输入格式和 index 选项。PluginManager测试，以验证依赖安装顺序、错误传播，以及在预检查成功、失败或没有缺失依赖时的行为。Original summary in English
由 Sourcery 生成的总结
通过预安装检查、核心库版本保护，以及将基于 pip 的安装与插件生命周期更紧密地集成，改进插件依赖管理。
新功能：
缺陷修复：
增强改进：
测试：
Original summary in English
Summary by Sourcery
改进插件依赖安装流程和与 pip 的集成，引入核心依赖降级保护、更智能的依赖解析，以及在
PluginManager与PipInstaller之间更紧密的协作。新特性：
缺陷修复：
增强改进：
PluginManager中，使安装、更新、重载以及失败插件恢复都执行一致的依赖检查和安装流程。测试：
PipInstaller测试范围，覆盖流式输出行为、依赖预检查逻辑以及多种 pip 输入/index 格式。PluginManager测试，用于验证依赖安装顺序、错误传播机制，以及在安装、更新、重载和失败插件恢复路径中的行为。Original summary in English
Summary by Sourcery
Improve plugin dependency installation flow and pip integration, adding core dependency downgrade protection, smarter requirement parsing, and tighter coupling between PluginManager and PipInstaller.
New Features:
Bug Fixes:
Enhancements:
Tests:
Original summary in English
由 Sourcery 生成的总结
通过预安装检查、核心库版本保护，以及将基于 pip 的安装与插件生命周期更紧密地集成，改进插件依赖管理。
新功能：
缺陷修复：
增强改进：
测试：
Original summary in English
Summary by Sourcery
改进插件依赖安装流程和与 pip 的集成，引入核心依赖降级保护、更智能的依赖解析，以及在
PluginManager与PipInstaller之间更紧密的协作。新特性：
缺陷修复：
增强改进：
PluginManager中，使安装、更新、重载以及失败插件恢复都执行一致的依赖检查和安装流程。测试：
PipInstaller测试范围，覆盖流式输出行为、依赖预检查逻辑以及多种 pip 输入/index 格式。PluginManager测试，用于验证依赖安装顺序、错误传播机制，以及在安装、更新、重载和失败插件恢复路径中的行为。Original summary in English
Summary by Sourcery
Improve plugin dependency installation flow and pip integration, adding core dependency downgrade protection, smarter requirement parsing, and tighter coupling between PluginManager and PipInstaller.
New Features:
Bug Fixes:
Enhancements:
Tests:
Original summary in English
由 Sourcery 生成的总结
通过预安装检查、核心库版本保护，以及将基于 pip 的安装与插件生命周期更紧密地集成，改进插件依赖管理。
新功能：
缺陷修复：
增强改进：
测试：
Original summary in English
Summary by Sourcery
改进插件依赖安装流程和与 pip 的集成，引入核心依赖降级保护、更智能的依赖解析，以及在
PluginManager与PipInstaller之间更紧密的协作。新特性：
缺陷修复：
增强改进：
PluginManager中，使安装、更新、重载以及失败插件恢复都执行一致的依赖检查和安装流程。测试：
PipInstaller测试范围，覆盖流式输出行为、依赖预检查逻辑以及多种 pip 输入/index 格式。PluginManager测试，用于验证依赖安装顺序、错误传播机制，以及在安装、更新、重载和失败插件恢复路径中的行为。Original summary in English
Summary by Sourcery
Improve plugin dependency installation flow and pip integration, adding core dependency downgrade protection, smarter requirement parsing, and tighter coupling between PluginManager and PipInstaller.
New Features:
Bug Fixes:
Enhancements:
Tests:
Original summary in English
由 Sourcery 生成的总结
通过预安装检查、核心库版本保护，以及将基于 pip 的安装与插件生命周期更紧密地集成，改进插件依赖管理。
新功能：
缺陷修复：
增强改进：
测试：
Original summary in English
Summary by Sourcery
改进插件依赖安装流程和与 pip 的集成，引入核心依赖降级保护、更智能的依赖解析，以及在
PluginManager与PipInstaller之间更紧密的协作。新特性：
缺陷修复：
增强改进：
PluginManager中，使安装、更新、重载以及失败插件恢复都执行一致的依赖检查和安装流程。测试：
PipInstaller测试范围，覆盖流式输出行为、依赖预检查逻辑以及多种 pip 输入/index 格式。PluginManager测试，用于验证依赖安装顺序、错误传播机制，以及在安装、更新、重载和失败插件恢复路径中的行为。Original summary in English
Summary by Sourcery
Improve plugin dependency installation flow and pip integration, adding core dependency downgrade protection, smarter requirement parsing, and tighter coupling between PluginManager and PipInstaller.
New Features:
Bug Fixes:
Enhancements:
Tests: