Skip to content

fix(skills): preserve cached sandbox skill paths #6072

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
stablegenius49 wants to merge 1 commit into
base: master
Choose a base branch
from
+35 −10
Open

fix(skills): preserve cached sandbox skill paths #6072

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 14 additions & 9 deletions astrbot/core/skills/skill_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,10 +154,18 @@ def build_skills_prompt(skills: list[SkillInfo]) -> str:
description = "Read SKILL.md for details."

if skill.source_type == "sandbox_only":
rendered_path = (
f"{str(SANDBOX_WORKSPACE_ROOT)}/{str(SANDBOX_SKILLS_ROOT)}/"
f"{display_name}/SKILL.md"
)
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"
)
Comment on lines +157 to +168
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Mar 11, 2026

Choose a reason for hiding this comment

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

medium

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_path

else:
rendered_path = _sanitize_prompt_path_for_prompt(skill.path)
if not rendered_path:
Expand Down Expand Up @@ -389,12 +397,9 @@ def list_skills(
if active_only and not active:
continue
description = sandbox_cached_descriptions.get(skill_name, "")
if show_sandbox_path:
path_str = sandbox_cached_paths.get(skill_name, "")
if not path_str:
path_str = f"{SANDBOX_WORKSPACE_ROOT}/{SANDBOX_SKILLS_ROOT}/{skill_name}/SKILL.md"
else:
path_str = sandbox_cached_paths.get(skill_name, "")
if not path_str:
path_str = f"{SANDBOX_WORKSPACE_ROOT}/{SANDBOX_SKILLS_ROOT}/{skill_name}/SKILL.md"
skills_by_name[skill_name] = SkillInfo(
name=skill_name,
description=description,
Expand Down
2 changes: 1 addition & 1 deletion tests/test_skill_manager_sandbox_cache.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ def test_list_skills_merges_local_and_sandbox_cache(monkeypatch, tmp_path: Path)
assert by_name["custom-local"].description == "local description"
assert by_name["custom-local"].path == "skills/custom-local/SKILL.md"
assert by_name["python-sandbox"].description == "ship built-in"
assert by_name["python-sandbox"].path == "/workspace/skills/python-sandbox/SKILL.md"
assert by_name["python-sandbox"].path == "/app/skills/python-sandbox/SKILL.md"


def test_sandbox_cached_skill_respects_active_and_display_path(
Expand Down
20 changes: 20 additions & 0 deletions tests/test_skill_metadata_enrichment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,26 @@ def test_build_skills_prompt_sanitizes_sandbox_skill_metadata_in_inventory():
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
Comment on lines +272 to +289
Copy link
Contributor

@sourcery-ai sourcery-ai bot Mar 11, 2026

Choose a reason for hiding this comment

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

suggestion (testing): 通过断言回退的 /workspace/... 路径没有出现在 prompt 中来加强回归测试

为了收紧回归测试范围，可以额外断言旧的回退路径在 prompt 中是 不存在 的（例如 "/workspace/skills/docx/SKILL.md" not in prompt）。这能确保如果实现将路径重新改写回 /workspace/skills/...，即使绝对路径仍然偶然存在，测试也会失败。

Suggested change
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.

Suggested change
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



def test_build_skills_prompt_sanitizes_invalid_sandbox_skill_name_in_path():
skills = [
SkillInfo(
Expand Down