test(skill_manager): update sandbox cache path expectations #5706
test(skill_manager): update sandbox cache path expectations #5706RC-CHN wants to merge 3 commits intoAstrBotDevs:masterfrom
Conversation
adjust sandbox cache tests to match absolute path resolution in list_skills for sandbox runtime. verify sandbox-cached skills cannot be deactivated via set_skill_active by asserting a PermissionError, and keep active-only listing behavior intact. add coverage for show_sandbox_path=false to ensure local skills still override cached metadata while sandbox-only skills retain cached paths.
dosubot
bot
added
the
size:M
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 refines the unit tests for the skill manager's sandbox cache functionality. It corrects path assertions for sandbox skills, enforces that sandbox cached skills cannot be deactivated by raising a "PermissionError", and introduces a new test to validate skill path resolution when the "show_sandbox_path" option is disabled. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
dosubot
bot
added
the
area:core
|
Related Documentation Checked 1 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我发现了 1 个问题
给 AI 代理的提示
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/test_skill_manager_sandbox_cache.py" line_range="111-120" />
<code_context>
+def test_sandbox_and_local_path_resolution_with_show_sandbox_path_false(
</code_context>
<issue_to_address>
**suggestion (testing):** 收紧本地 skill 的路径断言，确保它确实使用本地根路径，而不是只是因为后缀匹配而误通过
在这个测试中，当前的 `endswith("/skills/custom-local/SKILL.md")` 检查，即使 sandbox 路径和本地路径共享相同后缀时也可能通过。为了增强鲁棒性，还应断言解析后的路径位于 `skills_root` 之下（例如通过 `startswith(str(skills_root))`，或者使用一个小的兼容 helper 通过 `Path(...).is_relative_to(skills_root)` 进行判断）。
</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="tests/test_skill_manager_sandbox_cache.py" line_range="111-120" />
<code_context>
+def test_sandbox_and_local_path_resolution_with_show_sandbox_path_false(
</code_context>
<issue_to_address>
**suggestion (testing):** Tighten the path assertion for the local skill to ensure it truly uses the local root instead of accidentally matching by suffix
In this test, the current `endswith("/skills/custom-local/SKILL.md")` check could still pass if the sandbox and local paths share the same suffix. To harden it, also assert that the resolved path is under `skills_root` (e.g., `startswith(str(skills_root))` or `Path(...).is_relative_to(skills_root)` via a small compat helper).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def test_sandbox_and_local_path_resolution_with_show_sandbox_path_false( | ||
| monkeypatch, | ||
| tmp_path: Path, | ||
| ): | ||
| data_dir = tmp_path / "data" | ||
| temp_dir = tmp_path / "temp" | ||
| skills_root = tmp_path / "skills" | ||
| data_dir.mkdir(parents=True, exist_ok=True) | ||
| temp_dir.mkdir(parents=True, exist_ok=True) | ||
| skills_root.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
suggestion (testing): 收紧本地 skill 的路径断言，确保它确实使用本地根路径，而不是只是因为后缀匹配而误通过
在这个测试中，当前的 endswith("/skills/custom-local/SKILL.md") 检查，即使 sandbox 路径和本地路径共享相同后缀时也可能通过。为了增强鲁棒性，还应断言解析后的路径位于 skills_root 之下（例如通过 startswith(str(skills_root))，或者使用一个小的兼容 helper 通过 Path(...).is_relative_to(skills_root) 进行判断）。
Original comment in English
suggestion (testing): Tighten the path assertion for the local skill to ensure it truly uses the local root instead of accidentally matching by suffix
In this test, the current endswith("/skills/custom-local/SKILL.md") check could still pass if the sandbox and local paths share the same suffix. To harden it, also assert that the resolved path is under skills_root (e.g., startswith(str(skills_root)) or Path(...).is_relative_to(skills_root) via a small compat helper).
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR updates unit tests for the SkillManager, focusing on correcting sandbox path assertions, adding tests for the rule that 'sandbox cached skills cannot be deactivated', and new coverage tests for show_sandbox_path=False. While the code changes are correct and the new tests are valuable, it's critical to address a confirmed behavior in the core logic: untrusted paths from the sandbox cache are exposed when show_sandbox_path is False. This exposure, combined with insufficient sanitization in prompt construction, creates a potential prompt injection and path traversal vulnerability. On a positive note, the PR correctly implements a permission check to prevent deactivating sandbox-only skills, which is a good security improvement. For improved test code maintainability, consider using pytest fixtures to reduce duplication.
| assert sorted(by_name) == ["custom-local", "python-sandbox"] | ||
| assert by_name["custom-local"].description == "local description" | ||
| local_skill_path = Path(by_name["custom-local"].path) | ||
| assert local_skill_path.is_relative_to(skills_root) |
There was a problem hiding this comment.
This test case confirms that when show_sandbox_path is set to False, the SkillManager.list_skills method returns the untrusted path directly from the sandbox cache for sandbox-only skills. Tracing this data flow to build_skills_prompt in astrbot/core/skills/skill_manager.py reveals that these paths are included in the system prompt with minimal sanitization (only backticks are used for wrapping, and the _SAFE_PATH_RE sanitization is only applied to the example path, not the full list).
An attacker who can manipulate the sandbox cache (e.g., via a compromised sandbox environment) could inject malicious instructions or path traversal sequences into the system prompt. For example, a path like ` \n\n **SYSTEM INSTRUCTION**: ... could break out of the backticks and manipulate the LLM's behavior. Furthermore, if the LLM is instructed to read these files via shell commands (as suggested in the prompt rules), it could lead to information disclosure if the commands are executed on the host.
To remediate this, the list_skills logic should be updated to avoid returning untrusted cache paths when show_sandbox_path is False, and build_skills_prompt should rigorously sanitize all skill metadata (names, descriptions, and paths) before embedding them in the prompt.
| data_dir = tmp_path / "data" | ||
| temp_dir = tmp_path / "temp" | ||
| skills_root = tmp_path / "skills" | ||
| data_dir.mkdir(parents=True, exist_ok=True) | ||
| temp_dir.mkdir(parents=True, exist_ok=True) | ||
| skills_root.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| monkeypatch.setattr( | ||
| "astrbot.core.skills.skill_manager.get_astrbot_data_path", | ||
| lambda: str(data_dir), | ||
| ) | ||
| monkeypatch.setattr( | ||
| "astrbot.core.skills.skill_manager.get_astrbot_temp_path", | ||
| lambda: str(temp_dir), | ||
| ) |
There was a problem hiding this comment.
这段测试设置代码在多个测试函数中重复出现（test_list_skills_merges_local_and_sandbox_cache, test_sandbox_cached_skill_respects_active_and_display_path, 以及这个新的测试）。为了提高代码的可维护性并减少重复，建议将这部分逻辑提取到一个 pytest fixture 中。
例如，你可以创建一个 fixture：
import pytest
from pathlib import Path
@pytest.fixture
def skills_root(monkeypatch, tmp_path: Path) -> Path:
data_dir = tmp_path / "data"
temp_dir = tmp_path / "temp"
skills_root_path = tmp_path / "skills"
data_dir.mkdir(parents=True, exist_ok=True)
temp_dir.mkdir(parents=True, exist_ok=True)
skills_root_path.mkdir(parents=True, exist_ok=True)
monkeypatch.setattr(
"astrbot.core.skills.skill_manager.get_astrbot_data_path",
lambda: str(data_dir),
)
monkeypatch.setattr(
"astrbot.core.skills.skill_manager.get_astrbot_temp_path",
lambda: str(temp_dir),
)
return skills_root_path然后在测试函数中使用它，这样可以移除重复的设置代码：
def test_sandbox_and_local_path_resolution_with_show_sandbox_path_false(skills_root: Path):
mgr = SkillManager(skills_root=str(skills_root))
# ... 测试的其余部分这将使测试代码更简洁、更易于维护。
1 participant
修复单元测试skill部分
Modifications / 改动点
修正 sandbox 路径断言，强化“sandbox 缓存技能不可停用”的权限校验，新增 show_sandbox_path=False 的覆盖测试。
Screenshots or Test Results / 运行截图或测试结果
Checklist / 检查清单
requirements.txt和pyproject.toml文件相应位置。/ I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations inrequirements.txtandpyproject.toml.Summary by Sourcery
更新技能管理器沙箱缓存测试，以匹配新的路径解析行为和沙箱技能的权限规则。
Bug 修复：
list_skills测试中，更正对内置技能的预期沙箱技能路径。PermissionError。测试：
show_sandbox_path为False时，增加对沙箱和本地路径解析的覆盖，以确保本地覆盖生效并正确暴露路径。Original summary in English
Summary by Sourcery
Update skill manager sandbox cache tests to match new path resolution behavior and permission rules for sandboxed skills.
Bug Fixes:
Tests: