Repository navigation
fix(skills): render an explicit empty allowed-tools as no tools, not as all - #5593
WillemJiang merged 3 commits into
Conversation
`_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.
|
This is a draft by necessity, not because work is missing: PRs from accounts without write access are force-drafted here, and the create call only succeeds when it asks for a draft. Nothing is still in progress — the one-line change in |
…pty-allowed-tools
github-actions
Bot
added
area:skills
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Reviewed against head 4d7fcb8. The state distinction is correct and matches the enforced semantics at this head: None renders (all), an explicit empty tuple falls through the falsy join to (none), and non-empty declarations render unchanged; the html.escape on the line below is untouched, and nothing in the repo parses the rendered text, so this is prompt-only output. I ran tests/test_skill_describe.py (27 passed) plus test_skill_metadata_prompt_injection.py and test_skill_tool_policy_middleware.py (50 passed) in a worktree at this exact SHA, and ruff check/format are clean on both files. Cross-reference: PR #5659 (explicit empty skill export requirements) touches only the frontend export dialog and i18n at its current head, so there is no file or semantic conflict with this change — both PRs fix different falsy readers of the same allowed_tools tri-state, and this backend half and that frontend half stack cleanly. One suggestion-level residual of the same mismatch family is posted inline on the new tools_line.
| 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. | ||
| tools_line = "(all)" if s.allowed_tools is None else (", ".join(s.allowed_tools) or "(none)") |
There was a problem hiding this comment.
Correct for the pure cases, but note a residual of the same metadata-vs-enforced-state gap in mixed sets: per allowed_tool_names_for_skills (tool_policy.py), once any loaded skill declares allowed-tools, a legacy None skill contributes no tools to the union — yet this line still renders Allowed tools: (all) for it. So describing a legacy skill while a restricted skill is loaded repeats the #5590 mismatch the policy side deliberately implements (None only means allow-all when no loaded skill declares the field). Keeping the per-skill-declaration scope here is defensible and this framing is pre-existing, but the new consistency test pins single-skill lists only, so it cannot catch this drift — worth either a comment scoping (all) to the skill's own frontmatter declaration, or a mixed-list fixture spelling out the intended rendering.
There was a problem hiding this comment.
Scoped in 4d22605 — the comment above tools_line now states that (all) reports this skill's own frontmatter and names allowed_tool_names_for_skills for the mixed-set case; behaviour unchanged. I deliberately stopped short of the mixed-list fixture: pinning the current rendering would turn an open semantics question into a test, since as you say a legacy skill in a mixed set renders (all) while the middleware restricts it. If you would rather this line report the enforced union, the change is to pass the union (or the has_explicit_declaration flag) into _render_skill_metadata, and I will add that fixture as the regression test in this PR. At the new head tests/test_skill_describe.py + test_skill_metadata_prompt_injection.py + test_skill_tool_policy_middleware.py give 77 passed, and ruff check/format are clean on describe.py.
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.
Fixes #5590
Why
Trigger: while reading the skills package for another change I landed on
_render_skill_metadata(skills/describe.py:136) and noticed it truthiness-testss.allowed_tools, two lines below the code that escapes it for prompt safety.Skill.allowed_toolsistuple[str, ...] | None(skills/types.py:51), and those two empty-looking values mean opposite things — so the renderer collapses a restriction into a permission.Pain:
describe_skillis how the agent learns what a loaded skill permits. Forallowed-tools: []it printsAllowed tools: (all)whileSkillToolPolicyMiddlewarehas stripped every business tool from that same skill — the model plans against tools that will be rejected, and the metadata it is reasoning over is the opposite of its enforced state. Reported as #5590.Why it should be treated as a defect rather than a style choice: every other reader of this field already distinguishes the two states, and does so deliberately:
skills/tool_policy.py:41-47—if skill.allowed_tools is None: continue, then a dedicatedlogger.info("Skill %s declared empty allowed-tools", …)for the empty case.skills/parser.py:114— the docstring for the function that produces the value: "Returns an empty tuple for an explicit empty value."skills/export.py:305—… if tools is not None else None.tests/test_skills_validation.py::TestValidateSkillFrontmatter::test_allows_empty_allowed_tools,tests/test_skills_parser.py::test_parse_empty_allowed_tools_list,tests/test_skill_tool_policy_middleware.py::test_explicit_empty_allowed_tools_keeps_only_framework_tools.So
describe.pyis the only reader that gets it wrong, and the mislabel is observable inside one package: the sameSkillobject yieldsset()from the policy and(all)from the renderer.What changed
"(all)"now renders only for the state that means unrestricted (None, field omitted). An explicit empty allowlist renders"(none)", and a non-empty one is unchanged.html.escape(tools_line)) is untouched, so nothing about how untrusted front matter reaches the prompt changes.Surface area
backend/packages/harness/deerflow/skills/describe.py,backend/tests/test_skill_describe.pyallowed-tools:Allowed tools: (all)→Allowed tools: (none). Omitted and non-empty declarations render exactly as before.Bug fix verification
backend/tests/test_skill_describe.py::test_render_explicit_empty_allowed_tools_does_not_claim_allbackend/tests/test_skill_describe.py::test_rendered_allowed_tools_agree_with_skill_tool_policyPYTHONUTF8=1 pytest tests/test_skill_describe.pymain@aa4e43a2+ these tests (source file reverted)2 failed, 25 passed— both fail onassert "Allowed tools: (all)" not in rendered27 passedThe second fixture is a consistency test rather than a text assertion: it pins
allowed_tool_names_for_skills([skill]) is None⇔(all)and== set()⇔ not(all), so the two components cannot drift apart again without failing here. The pre-existingtest_render_no_allowed_tools_shows_allandtest_render_metadata_formatare the controls and pass on both sides.I also ran the reproduction from #5590 verbatim against
aa4e43a2— that is where thedecl='[]' → policy=set() → rendered: (all)/decl=None → policy=None → rendered: (all)table comes from — and re-ran it on this branch, where the empty case prints(none)and the omitted case still prints(all).Validation
PYTHONUTF8=1 pytest tests -k "skill or describe"on this branch:1 failed, 1336 passed, 64 skipped(78 s). The one failure istests/test_lark_cli_integration.py::test_install_lark_integration_installs_managed_gateway_cli_before_skill_pack,which fails identically after reverting
describe.pytoaa4e43a2on this same machine (1 failed). It is an environment failure, not a behaviour one: the test drives an externallarkCLI, and on this host the subprocess probe returns undecodable cp936 bytes, solark_cli.py:474raisesAttributeError: 'NoneType' object has no attribute 'strip'. My diff does not touch that module.PYTHONUTF8=1 pytest tests/test_skill_describe.py tests/test_skill_metadata_prompt_injection.py:31 passed— the injection module asserts the escaped-tag behaviour of this same renderer, so it is the closest neighbour to guard.makeis not available on this host:ruff checkon both changed files →All checks passed!;ruff format --check→2 files already formatted(ruff0.15.12).make lint/make testin full, theliveandblocking_iomarks, the frontend suites, Docker e2e. This is a local source checkout, not CI.AI assistance
Tool(s) used: Qoder (agentic CLI).
How you used it: it wrote the two fixtures, ran them red on
aa4e43a2and green on the branch, ran the sweeps quoted above, and chose(none)as the rendering. It also wrote this description. A maintainer should read the diff with that in mind.Human review status: this PR was opened during an unattended contribution run, so the account owner has not read the diff yet. That is why the box below is unticked, and I will tick it (or amend the PR) once they have.
Notes for the reviewer
allowed-toolsat validation time instead of rendering it. I did not, becausetest_allows_empty_allowed_toolsexists specifically to pin that install accepts the value, andtool_policy.pylogs it as a supported state rather than treating it as an error — the empty list looks intentional ("this skill is metadata only"), which is exactly the case where the rendered text matters. Happy to switch to that variant if you consider the declaration itself a mistake.get_skill_index_prompt_section(same file, below this function) renders only names, so it has no share of the bug;skills/export.pyandtool_policy.pywere checked for the same falsy pattern and are already correct.allowed-toolsand touches neither this function nor its tests, so there is no conflict; if that discussion ends up changing what()means, this rendering follows it with a one-line edit.