fix(skills): render an explicit empty allowed-tools as no tools, not as all (#5593)

* fix(skills): render an empty allowed-tools as no tools, not as all

`_render_skill_metadata` truthiness-tested `Skill.allowed_tools`, so an
explicitly empty allowlist (`allowed-tools: []`, parsed to `()` by
`parse_allowed_tools`) rendered "Allowed tools: (all)" -- the same text an
omitted field (`None`, unrestricted) produces. `allowed_tool_names_for_skills`
distinguishes the two and strips every business tool for `()`, so the
describe_skill output contradicted the policy applied to the same skill.

* docs(skills): scope the rendered (all) to the skill's own declaration

Review note: allowed_tool_names_for_skills makes a legacy None skill
contribute no tools once any loaded skill declares allowed-tools, so
"(all)" on that line reports the frontmatter of this skill rather than
the tool set the middleware will allow in a mixed set. Say so where the
tri-state is rendered; behaviour is unchanged.
This commit is contained in:
Grapette.L 2026-09-22 17:27:01 +08:00 committed by GitHub
parent 825e742eb6
commit dde0595ec8
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 29 additions and 1 deletions

View File

@ -133,7 +133,13 @@ def _render_skill_metadata(skills: list, container_base_path: str) -> str:
blocks: list[str] = [] blocks: list[str] = []
for s in skills: for s in skills:
mutability = "[custom, editable]" if s.category == SkillCategory.CUSTOM else "[built-in]" mutability = "[custom, editable]" if s.category == SkillCategory.CUSTOM else "[built-in]"
tools_line = ", ".join(s.allowed_tools) if s.allowed_tools else "(all)" # `()` is an explicit empty allowlist — the policy middleware strips every
# business tool for it — so only an omitted field (`None`) means unrestricted.
# `(all)` describes this skill's own frontmatter, not the enforced union: once
# any loaded skill declares allowed-tools,
# ``allowed_tool_names_for_skills`` (tool_policy.py) gives a `None` skill no
# tools, so a mixed set can render `(all)` while the middleware restricts it.
tools_line = "(all)" if s.allowed_tools is None else (", ".join(s.allowed_tools) or "(none)")
location = s.get_container_file_path(container_base_path) location = s.get_container_file_path(container_base_path)
# name/description/allowed-tools come from untrusted ``.skill`` frontmatter; # name/description/allowed-tools come from untrusted ``.skill`` frontmatter;
# escape so a value cannot forge a framework tag in the describe_skill output. # escape so a value cannot forge a framework tag in the describe_skill output.

View File

@ -11,6 +11,7 @@ from deerflow.skills.describe import (
build_skill_search_setup, build_skill_search_setup,
get_skill_index_prompt_section, get_skill_index_prompt_section,
) )
from deerflow.skills.tool_policy import allowed_tool_names_for_skills
from deerflow.skills.types import Skill, SkillCategory from deerflow.skills.types import Skill, SkillCategory
# ── Helpers ──────────────────────────────────────────────────────────────────── # ── Helpers ────────────────────────────────────────────────────────────────────
@ -281,3 +282,24 @@ def test_describe_tool_select_uncapped():
content = result.update["messages"][0].content content = result.update["messages"][0].content
for s in many_skills: for s in many_skills:
assert s.name in content, f"select: truncated — {s.name} missing from result" assert s.name in content, f"select: truncated — {s.name} missing from result"
# ── Explicitly empty allowed-tools ─────────────────────────────────────────────
def test_render_explicit_empty_allowed_tools_does_not_claim_all():
restricted = _make_skill("locked-down", allowed_tools=())
rendered = _render_skill_metadata([restricted], "/mnt/skills")
assert "Allowed tools: (all)" not in rendered
assert "Allowed tools: (none)" in rendered
def test_rendered_allowed_tools_agree_with_skill_tool_policy():
"""`(all)` must mean unrestricted, which is the one state that renders it."""
omitted = _make_skill("legacy")
assert allowed_tool_names_for_skills([omitted]) is None
assert "Allowed tools: (all)" in _render_skill_metadata([omitted], "/mnt/skills")
restricted = _make_skill("locked-down", allowed_tools=())
assert allowed_tool_names_for_skills([restricted]) == set()
assert "Allowed tools: (all)" not in _render_skill_metadata([restricted], "/mnt/skills")