fix(skills): preserve cached sandbox skill paths#6072
fix(skills): preserve cached sandbox skill paths#6072stablegenius49 wants to merge 1 commit intoAstrBotDevs:masterfrom
Conversation
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 resolves an issue where sandbox-only skill paths were being consistently rewritten to a default workspace path, overriding their cached absolute locations. The changes ensure that the system now correctly utilizes and preserves these cached paths for sandbox skills, thereby improving the accuracy and efficiency of skill path resolution. Concurrently, the existing sanitization logic for invalid skill names is maintained to uphold security standards. 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
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并给出了一些整体反馈：
- 新的
list_skills逻辑似乎完全忽略了show_sandbox_path标志，并且在缓存路径存在时总是优先使用缓存路径；如果你的意图是保留强制使用/workspace/...路径的能力，建议要么把这个标志重新接入控制流程，要么移除这个参数以避免混淆。 - 在
build_skills_prompt中，仅沙箱（sandbox-only）的回退路径构造在display_name != skill.name分支和not rendered_path分支中被重复实现；建议提取一个小的辅助函数，或者复用单一代码路径，以减少重复并保持沙箱路径格式的一致性。 - 新的测试
test_build_skills_prompt_preserves_cached_absolute_sandbox_path把一个非常具体的绝对路径（/home/ship_0d0fba63/...）写死在代码中；你可能会希望使用更通用或参数化的假绝对路径，以避免让测试与特定用户名或目录结构耦合。
供 AI Agent 使用的提示词
Please address the comments from this code review:
## Overall Comments
- 新的 `list_skills` 逻辑似乎完全忽略了 `show_sandbox_path` 标志，并且在缓存路径存在时总是优先使用缓存路径；如果你的意图是保留强制使用 `/workspace/...` 路径的能力，建议要么把这个标志重新接入控制流程，要么移除这个参数以避免混淆。
- 在 `build_skills_prompt` 中，仅沙箱（sandbox-only）的回退路径构造在 `display_name != skill.name` 分支和 `not rendered_path` 分支中被重复实现；建议提取一个小的辅助函数，或者复用单一代码路径，以减少重复并保持沙箱路径格式的一致性。
- 新的测试 `test_build_skills_prompt_preserves_cached_absolute_sandbox_path` 把一个非常具体的绝对路径（`/home/ship_0d0fba63/...`）写死在代码中；你可能会希望使用更通用或参数化的假绝对路径，以避免让测试与特定用户名或目录结构耦合。
## Individual Comments
### Comment 1
<location path="tests/test_skill_metadata_enrichment.py" line_range="272-289" />
<code_context>
assert "`/workspace/skills/sandbox-skill/SKILL.md`" in prompt
+def test_build_skills_prompt_preserves_cached_absolute_sandbox_path():
+ skills = [
+ SkillInfo(
+ name="docx",
+ description="Read and write Word documents.",
+ path="/home/ship_0d0fba63/workspace/skills/docx/SKILL.md",
+ active=True,
+ source_type="sandbox_only",
+ source_label="sandbox_preset",
+ local_exists=False,
+ sandbox_exists=True,
+ )
+ ]
+
+ prompt = build_skills_prompt(skills)
+
+ assert "`/home/ship_0d0fba63/workspace/skills/docx/SKILL.md`" in prompt
+ assert "cat /home/ship_0d0fba63/workspace/skills/docx/SKILL.md" in prompt
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** 通过断言回退的 `/workspace/...` 路径没有出现在 prompt 中来加强回归测试
为了收紧回归测试范围，可以额外断言旧的回退路径在 prompt 中是 *不存在* 的（例如 `"/workspace/skills/docx/SKILL.md" not in prompt`）。这能确保如果实现将路径重新改写回 `/workspace/skills/...`，即使绝对路径仍然偶然存在，测试也会失败。
```suggestion
def test_build_skills_prompt_preserves_cached_absolute_sandbox_path():
skills = [
SkillInfo(
name="docx",
description="Read and write Word documents.",
path="/home/ship_0d0fba63/workspace/skills/docx/SKILL.md",
active=True,
source_type="sandbox_only",
source_label="sandbox_preset",
local_exists=False,
sandbox_exists=True,
)
]
prompt = build_skills_prompt(skills)
assert "`/home/ship_0d0fba63/workspace/skills/docx/SKILL.md`" in prompt
assert "cat /home/ship_0d0fba63/workspace/skills/docx/SKILL.md" in prompt
assert "`/workspace/skills/docx/SKILL.md`" not in prompt
```
</issue_to_address>帮我变得更有用！请在每条评论上点击 👍 或 👎，我会根据反馈改进后续的审查。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The new
list_skillslogic appears to ignore theshow_sandbox_pathflag entirely and always prefer the cached path when present; if the intent is to preserve the ability to force/workspace/...paths, consider wiring that flag back into the control flow or removing the parameter to avoid confusion. - In
build_skills_prompt, the sandbox-only fallback path construction is duplicated in both thedisplay_name != skill.nameandnot rendered_pathbranches; consider extracting a small helper or reusing a single code path to reduce repetition and keep the sandbox path format consistent. - The new test
test_build_skills_prompt_preserves_cached_absolute_sandbox_pathhardcodes a very specific absolute path (/home/ship_0d0fba63/...); you might want to use a more generic or parametrized fake absolute path to avoid coupling the test to a particular username or directory layout.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `list_skills` logic appears to ignore the `show_sandbox_path` flag entirely and always prefer the cached path when present; if the intent is to preserve the ability to force `/workspace/...` paths, consider wiring that flag back into the control flow or removing the parameter to avoid confusion.
- In `build_skills_prompt`, the sandbox-only fallback path construction is duplicated in both the `display_name != skill.name` and `not rendered_path` branches; consider extracting a small helper or reusing a single code path to reduce repetition and keep the sandbox path format consistent.
- The new test `test_build_skills_prompt_preserves_cached_absolute_sandbox_path` hardcodes a very specific absolute path (`/home/ship_0d0fba63/...`); you might want to use a more generic or parametrized fake absolute path to avoid coupling the test to a particular username or directory layout.
## Individual Comments
### Comment 1
<location path="tests/test_skill_metadata_enrichment.py" line_range="272-289" />
<code_context>
assert "`/workspace/skills/sandbox-skill/SKILL.md`" in prompt
+def test_build_skills_prompt_preserves_cached_absolute_sandbox_path():
+ skills = [
+ SkillInfo(
+ name="docx",
+ description="Read and write Word documents.",
+ path="/home/ship_0d0fba63/workspace/skills/docx/SKILL.md",
+ active=True,
+ source_type="sandbox_only",
+ source_label="sandbox_preset",
+ local_exists=False,
+ sandbox_exists=True,
+ )
+ ]
+
+ prompt = build_skills_prompt(skills)
+
+ assert "`/home/ship_0d0fba63/workspace/skills/docx/SKILL.md`" in prompt
+ assert "cat /home/ship_0d0fba63/workspace/skills/docx/SKILL.md" in prompt
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen regression by asserting that the fallback `/workspace/...` path is not present in the prompt
To tighten the regression, also assert that the old fallback path is *absent* from the prompt (e.g. `"/workspace/skills/docx/SKILL.md" not in prompt`). This ensures the test will fail if the implementation ever reverts to rewriting paths back to `/workspace/skills/...`, even if the absolute path is still present incidentally.
```suggestion
def test_build_skills_prompt_preserves_cached_absolute_sandbox_path():
skills = [
SkillInfo(
name="docx",
description="Read and write Word documents.",
path="/home/ship_0d0fba63/workspace/skills/docx/SKILL.md",
active=True,
source_type="sandbox_only",
source_label="sandbox_preset",
local_exists=False,
sandbox_exists=True,
)
]
prompt = build_skills_prompt(skills)
assert "`/home/ship_0d0fba63/workspace/skills/docx/SKILL.md`" in prompt
assert "cat /home/ship_0d0fba63/workspace/skills/docx/SKILL.md" in prompt
assert "`/workspace/skills/docx/SKILL.md`" not in prompt
```
</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_build_skills_prompt_preserves_cached_absolute_sandbox_path(): | ||
| skills = [ | ||
| SkillInfo( | ||
| name="docx", | ||
| description="Read and write Word documents.", | ||
| path="/home/ship_0d0fba63/workspace/skills/docx/SKILL.md", | ||
| active=True, | ||
| source_type="sandbox_only", | ||
| source_label="sandbox_preset", | ||
| local_exists=False, | ||
| sandbox_exists=True, | ||
| ) | ||
| ] | ||
|
|
||
| prompt = build_skills_prompt(skills) | ||
|
|
||
| assert "`/home/ship_0d0fba63/workspace/skills/docx/SKILL.md`" in prompt | ||
| assert "cat /home/ship_0d0fba63/workspace/skills/docx/SKILL.md" in prompt |
There was a problem hiding this comment.
suggestion (testing): 通过断言回退的 /workspace/... 路径没有出现在 prompt 中来加强回归测试
为了收紧回归测试范围，可以额外断言旧的回退路径在 prompt 中是 不存在 的（例如 "/workspace/skills/docx/SKILL.md" not in prompt）。这能确保如果实现将路径重新改写回 /workspace/skills/...，即使绝对路径仍然偶然存在，测试也会失败。
| def test_build_skills_prompt_preserves_cached_absolute_sandbox_path(): | |
| skills = [ | |
| SkillInfo( | |
| name="docx", | |
| description="Read and write Word documents.", | |
| path="/home/ship_0d0fba63/workspace/skills/docx/SKILL.md", | |
| active=True, | |
| source_type="sandbox_only", | |
| source_label="sandbox_preset", | |
| local_exists=False, | |
| sandbox_exists=True, | |
| ) | |
| ] | |
| prompt = build_skills_prompt(skills) | |
| assert "`/home/ship_0d0fba63/workspace/skills/docx/SKILL.md`" in prompt | |
| assert "cat /home/ship_0d0fba63/workspace/skills/docx/SKILL.md" in prompt | |
| def test_build_skills_prompt_preserves_cached_absolute_sandbox_path(): | |
| skills = [ | |
| SkillInfo( | |
| name="docx", | |
| description="Read and write Word documents.", | |
| path="/home/ship_0d0fba63/workspace/skills/docx/SKILL.md", | |
| active=True, | |
| source_type="sandbox_only", | |
| source_label="sandbox_preset", | |
| local_exists=False, | |
| sandbox_exists=True, | |
| ) | |
| ] | |
| prompt = build_skills_prompt(skills) | |
| assert "`/home/ship_0d0fba63/workspace/skills/docx/SKILL.md`" in prompt | |
| assert "cat /home/ship_0d0fba63/workspace/skills/docx/SKILL.md" in prompt | |
| assert "`/workspace/skills/docx/SKILL.md`" not in prompt |
Original comment in English
suggestion (testing): Strengthen regression by asserting that the fallback /workspace/... path is not present in the prompt
To tighten the regression, also assert that the old fallback path is absent from the prompt (e.g. "/workspace/skills/docx/SKILL.md" not in prompt). This ensures the test will fail if the implementation ever reverts to rewriting paths back to /workspace/skills/..., even if the absolute path is still present incidentally.
| def test_build_skills_prompt_preserves_cached_absolute_sandbox_path(): | |
| skills = [ | |
| SkillInfo( | |
| name="docx", | |
| description="Read and write Word documents.", | |
| path="/home/ship_0d0fba63/workspace/skills/docx/SKILL.md", | |
| active=True, | |
| source_type="sandbox_only", | |
| source_label="sandbox_preset", | |
| local_exists=False, | |
| sandbox_exists=True, | |
| ) | |
| ] | |
| prompt = build_skills_prompt(skills) | |
| assert "`/home/ship_0d0fba63/workspace/skills/docx/SKILL.md`" in prompt | |
| assert "cat /home/ship_0d0fba63/workspace/skills/docx/SKILL.md" in prompt | |
| def test_build_skills_prompt_preserves_cached_absolute_sandbox_path(): | |
| skills = [ | |
| SkillInfo( | |
| name="docx", | |
| description="Read and write Word documents.", | |
| path="/home/ship_0d0fba63/workspace/skills/docx/SKILL.md", | |
| active=True, | |
| source_type="sandbox_only", | |
| source_label="sandbox_preset", | |
| local_exists=False, | |
| sandbox_exists=True, | |
| ) | |
| ] | |
| prompt = build_skills_prompt(skills) | |
| assert "`/home/ship_0d0fba63/workspace/skills/docx/SKILL.md`" in prompt | |
| assert "cat /home/ship_0d0fba63/workspace/skills/docx/SKILL.md" in prompt | |
| assert "`/workspace/skills/docx/SKILL.md`" not in prompt |
![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 pull request correctly addresses an issue where cached absolute paths for sandbox-only skills were not being preserved. The changes in build_skills_prompt and list_skills ensure that cached paths are now used, with appropriate fallbacks. The added regression tests provide good coverage for these fixes. I've included one suggestion to refactor a small piece of logic to improve code clarity and reduce duplication.
| if display_name != skill.name: | ||
| rendered_path = ( | ||
| f"{str(SANDBOX_WORKSPACE_ROOT)}/{str(SANDBOX_SKILLS_ROOT)}/" | ||
| f"{display_name}/SKILL.md" | ||
| ) | ||
| else: | ||
| rendered_path = _sanitize_prompt_path_for_prompt(skill.path) | ||
| if not rendered_path: | ||
| rendered_path = ( | ||
| f"{str(SANDBOX_WORKSPACE_ROOT)}/{str(SANDBOX_SKILLS_ROOT)}/" | ||
| f"{display_name}/SKILL.md" | ||
| ) |
There was a problem hiding this comment.
The logic for determining rendered_path for sandbox_only skills has some repeated code for constructing the fallback path. You can improve maintainability by defining the fallback path once and reusing it, which also makes the code more concise.
fallback_path = (
f"{str(SANDBOX_WORKSPACE_ROOT)}/{str(SANDBOX_SKILLS_ROOT)}/"
f"{display_name}/SKILL.md"
)
if display_name != skill.name:
rendered_path = fallback_path
else:
rendered_path = _sanitize_prompt_path_for_prompt(skill.path) or fallback_path2 participants
Closes #5922
Modifications / 改动点
preserve cached sandbox skill paths for sandbox-only skills instead of always rewriting them to
/workspace/skills/...keep the existing fallback for invalid sandbox skill names so prompt-path sanitization still wins on untrusted metadata
add regression coverage for both
SkillManager.list_skills(runtime="sandbox")andbuild_skills_prompt(...)using cached absolute sandbox pathsThis is NOT a breaking change. / 这不是一个破坏性变更。
Screenshots or Test Results / 运行截图或测试结果
Verification steps run locally:
python3.11 -m py_compile astrbot/core/skills/skill_manager.py tests/test_skill_metadata_enrichment.py tests/test_skill_manager_sandbox_cache.pybuild_skills_prompt(...)preserves/home/ship_*/workspace/skills/.../SKILL.mdfor sandbox-only skills/workspace/skills/<invalid_skill_name>/SKILL.mdSkillManager.list_skills(runtime="sandbox")keeps the cached absolute sandbox path for cache-only skillsRegression script output:
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 修复：
测试：
build_skills_prompt会保留缓存的绝对沙盒路径，同时仍然会清理无效的沙盒技能名称。SkillManager.list_skills(runtime="sandbox")会为仅限沙盒使用的技能返回缓存的沙盒路径。Original summary in English
Summary by Sourcery
Preserve and correctly surface cached sandbox skill paths while maintaining safe fallbacks for invalid sandbox metadata.
Bug Fixes:
Tests:
build_skills_promptpreserves cached absolute sandbox paths and still sanitizes invalid sandbox skill names.SkillManager.list_skills(runtime="sandbox")returns the cached sandbox path for sandbox-only skills.