From dde0595ec87ffa0cc09101d0d7ec02a8801c7cf4 Mon Sep 17 00:00:00 2001 From: "Grapette.L" <90967079+Lesereingrape@users.noreply.github.com> Date: Tue, 22 Sep 2026 17:27:01 +0800 Subject: [PATCH] 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. --- .../harness/deerflow/skills/describe.py | 8 ++++++- backend/tests/test_skill_describe.py | 22 +++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/backend/packages/harness/deerflow/skills/describe.py b/backend/packages/harness/deerflow/skills/describe.py index fe8c1c8b9..4417dbf0e 100644 --- a/backend/packages/harness/deerflow/skills/describe.py +++ b/backend/packages/harness/deerflow/skills/describe.py @@ -133,7 +133,13 @@ def _render_skill_metadata(skills: list, container_base_path: str) -> str: blocks: list[str] = [] for s in skills: 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) # name/description/allowed-tools come from untrusted ``.skill`` frontmatter; # escape so a value cannot forge a framework tag in the describe_skill output. diff --git a/backend/tests/test_skill_describe.py b/backend/tests/test_skill_describe.py index b7bd8176b..f68edbd16 100644 --- a/backend/tests/test_skill_describe.py +++ b/backend/tests/test_skill_describe.py @@ -11,6 +11,7 @@ from deerflow.skills.describe import ( build_skill_search_setup, get_skill_index_prompt_section, ) +from deerflow.skills.tool_policy import allowed_tool_names_for_skills from deerflow.skills.types import Skill, SkillCategory # ── Helpers ──────────────────────────────────────────────────────────────────── @@ -281,3 +282,24 @@ def test_describe_tool_select_uncapped(): content = result.update["messages"][0].content for s in many_skills: 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")