mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-09-10 05:58:36 +00:00
fix(skills): safely tokenize portable allowed-tools patterns (#4984)
* fix(skills): accept portable frontmatter forms * fix(skills): normalize portable tool names * Safely preserve parenthesized portable skill tool patterns Portable Agent Skills declarations such as Bash(tvly *) contain spaces inside a command pattern. Keep those patterns as single literal entries while preserving exact names from the existing YAML-list form, so skill loading no longer fragments valid metadata or rewrites mixed-case MCP tools. Constraint: DeerFlow's current skill policy matches exact tool names and does not inspect Bash arguments Constraint: Agent Skills scalar syntax uses whitespace-separated entries with parenthesized command patterns Rejected: raw.split() | fragments Bash(tvly *) into unrelated tool names Rejected: normalize YAML-list entries | breaks case-sensitive MCP/runtime tool names Rejected: map Bash(...) to bash | broadens command-scoped declarations into unrestricted shell access Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep Bash(...) entries literal and inactive until DeerFlow has an explicit command-pattern authorization model Tested: 175 focused parser, validation, installer, review, loader, and tool-policy tests; Ruff check and format; compileall; git diff --check Not-tested: Full backend suite stopped at pre-existing Windows mode assertion test_runtime_config_store_file_is_owner_only Related: #4912 * Preserve exact custom tool names in portable skill parsing Portable scalar frontmatter needs alias normalization for known DeerFlow-compatible names, but generic case conversion corrupts MCP and custom tool identifiers. The tokenizer also treated quoted or escaped parentheses as structural delimiters, rejecting valid command patterns. Preserve unknown names and parse quoted or escaped patterns without broadening Bash(...) into bash. Constraint: Runtime skill policy uses exact tool-name matching Constraint: Parenthesized patterns remain literal because argument-level authorization is not implemented Rejected: Generic CamelCase-to-snake_case for every scalar | rewrites custom/MCP names Rejected: Map Bash(...) to bash | broadens command-scoped declarations into unrestricted shell access Confidence: high Scope-risk: narrow Reversibility: clean Directive: Add an explicit alias before supporting another portable tool name; keep command-pattern authorization separate Tested: 225 skills tests passed, 1 skipped; Ruff check; Ruff format --check; compileall; git diff --check Not-tested: Full backend suite remains affected by unrelated Windows permissions/path and missing Lark CLI tests Related: #4984; #4912 * Preserve case-sensitive exact tool authorities Case-folding a scalar declaration before alias lookup can turn literal write into write_file, substituting a different runtime authority. Keep exact portable spellings as aliases and preserve lowercase, custom, and MCP names; strengthen activation coverage for spaced Bash patterns and command fragments. Constraint: Runtime skill policy uses exact tool-name matching Constraint: Bash(...) remains literal and inactive because command-pattern authorization is not implemented Rejected: Case-insensitive alias lookup | maps lowercase runtime tools onto built-in authorities Rejected: Broaden the parser into command-pattern authorization | outside this PR's scope Confidence: high Scope-risk: narrow Reversibility: clean Directive: Add aliases only for documented portable spellings; preserve all other scalar names verbatim Tested: 226 skills tests passed, 1 skipped; Ruff check; Ruff format --check; compileall; git diff --check Not-tested: Full backend suite remains affected by unrelated Windows permissions/path and missing Lark CLI tests; GitNexus index refresh remains stale Related: #4984; #5016297602 * Support portable Glob and Grep skill aliases Portable Agent Skills commonly declare Glob and Grep, but DeerFlow exposes the runtime tools as glob and grep. Add explicit exact-spelling aliases and activation coverage so imported skills retain search-tool access without broad normalization. Constraint: Runtime skill policy uses exact tool-name matching Constraint: Alias conversion is limited to documented portable spellings Rejected: Case-fold all scalar names | can substitute custom or MCP authorities Rejected: Map arbitrary names by convention | breaks exact runtime compatibility Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep the alias table explicit and preserve unknown scalar names verbatim Tested: 228 skills tests passed, 1 skipped; Ruff check; Ruff format --check; compileall; git diff --check Not-tested: Full backend suite has unrelated environment failures on Windows; GitNexus index reports stale line mappings Related: #4984; #5026257899 --------- Co-authored-by: kriptoburak <kriptoburak@users.noreply.github.com>
This commit is contained in:
georgelichen
GitHub
parent
0d97fdc770
commit
24001e80b7
@ -812,7 +812,7 @@ Users can explicitly activate an enabled skill for a single turn by starting the
|
||||
|
||||
An enabled skill's `allowed-tools` policy applies only after that skill is explicitly slash-activated or captured in the agent's active skill context after a `read_file` load. Merely enabling, advertising, or listing a skill in a custom agent or subagent `skills` allowlist does not reduce that agent's normal toolset; subagents use the same progressive discovery and activation policy as the lead agent. During a slash-activated run, that explicit skill's policy is authoritative: reading another `SKILL.md` may provide instructions but cannot widen the slash skill's tools. Without slash activation, policies from skills actually loaded into active context retain their union semantics. Once active, the policy filters both model-visible tool schemas and tool execution. Framework discovery tools (`tool_search` and `describe_skill`) remain available so an allowed deferred tool or installed skill can still be discovered, but discovery and promotion never grant permission to execute a business tool omitted from `allowed-tools`. `task` is not framework-exempt; a restrictive skill must list it explicitly to delegate to a subagent. Per-step policy decisions are internal runtime context and are removed from observable or persisted context copies. Registry failures and an active set with no remaining valid skill fail closed to framework-safe tools; individual stale paths are ignored only when another valid active skill remains. This is best-effort behavioral scoping, not a hard security boundary: loading skill instructions through another tool is not captured, and active-skill entries can be evicted from bounded context.
|
||||
|
||||
When you install `.skill` archives through the Gateway, DeerFlow accepts standard optional frontmatter metadata such as `version`, `author`, and `compatibility` instead of rejecting otherwise valid external skills.
|
||||
When you install `.skill` archives through the Gateway, DeerFlow accepts standard space-separated `allowed-tools`, optional frontmatter metadata, and the Claude-compatible `argument-hint` field instead of rejecting otherwise valid external skills. YAML lists remain supported for `allowed-tools` and preserve exact runtime names. Exact portable spellings such as `WebFetch`, `WebSearch`, `Glob`, `Grep`, and `Read` map to DeerFlow's `web_fetch`, `web_search`, `glob`, `grep`, and `read_file` tools; lowercase or otherwise unknown scalar names remain unchanged so custom and MCP tools keep their exact runtime spelling. Parenthesized entries such as `Bash(tvly *)` are tokenized as one literal entry, including spaces, quoted text, and escaped parentheses, but remain inactive because DeerFlow does not inspect tool arguments; declare `bash` only when the skill may use the full Bash tool.
|
||||
|
||||
Disabling a skill also removes it from the sandbox filesystem view, so shell commands and structured file tools follow the same enabled state. Local, Docker/AIO, hostPath provisioner, and newly created E2B sandboxes source `/mnt/skills` from enabled-only projections that update when public, custom, legacy, or managed integration skills are toggled, edited, created, deleted, or installed. Structured `read_file` calls (including line ranges and read-before-write checks) use the sandbox provider's mount mapping, so the user identity captured when the sandbox was acquired remains authoritative. Managed integration packages remain shared, while their projected filesystem visibility follows each user's enabled state. Multi-worker Gateways re-read on-disk enable state while rebuilding user projections, so a toggle handled by one worker is honored by another worker's next sandbox acquire. Existing E2B sandboxes retain their creation-time snapshot until they are recreated. PVC-backed provisioner skills keep their configured PVC snapshot/layout for now; dynamic PVC materialization is tracked separately.
|
||||
|
||||
|
||||
@ -1,7 +1,7 @@
|
||||
### Skills System (`packages/harness/deerflow/skills/`)
|
||||
|
||||
- **Location**: global public skills live under `deer-flow/skills/public/`; user-authored custom skills live under `{DEER_FLOW_HOME}/users/{user_id}/skills/custom/`; globally managed integration skills live under `{DEER_FLOW_HOME}/integrations/skills/{provider}/`; per-user integration credentials remain under `{DEER_FLOW_HOME}/users/{user_id}/integrations/{provider}/{config,data}`
|
||||
- **Format**: Directory with `SKILL.md` (YAML frontmatter: name, description, license, allowed-tools, required-secrets)
|
||||
- **Format**: Directory with `SKILL.md` (YAML frontmatter: name, description, license, allowed-tools as a spec-compatible string or YAML list, argument-hint, required-secrets). Exact portable spellings such as `Bash`, `WebFetch`, `WebSearch`, `Glob`, `Grep`, `Read`, `Write`, and `Edit` map to `bash`, `web_fetch`, `web_search`, `glob`, `grep`, `read_file`, `write_file`, and `str_replace`; lowercase or otherwise unknown scalar names and YAML-list entries preserve their exact runtime spelling. Argument-scoped entries remain literal and inactive because the tool policy does not inspect arguments; the scalar tokenizer keeps spaces, quotes, and escaped parentheses inside patterns intact.
|
||||
- **Loading**: `load_skills()` recursively scans public, per-user custom, global integration, and legacy custom locations for `SKILL.md`, parses metadata, and reads enabled state from extensions_config.json plus per-user skill state for non-public categories; that directory is a package boundary, so no nested `SKILL.md` is registered as a runtime skill. A custom skill directory may be a one-level symlink to an external directory for compatibility with operator-managed skill trees; activation still rejects a symlinked `SKILL.md` or deeper path escape. SkillScan has a deliberately narrower packaging rule: known eval fixtures are permitted as support data, while other nested `SKILL.md` files are reported as package defects. It parses runtime metadata and reads enabled state from extensions_config.json.
|
||||
- **External reload**: `POST /api/skills/reload` is an admin-only, process-local invalidation hook for trusted MinIO/NFS/CSI writes. `SkillStorage` instances do not cache a catalog — `load_skills()` scans on every call — so the route clears all `(app_config, user_id)` entries and the rendered prompt-section LRU, then waits up to the shared refresh timeout for the existing off-loop single-flight refresh. Each invalidation receives a generation-bound result handle; a successful scan atomically replaces the global enabled-skills cache, while a loader-level failure propagates to the HTTP waiter and preserves the last-known-good global cache. Per-user/config scans capture the refresh version and cannot repopulate shared caches if invalidation occurs while they are loading. A timed-out HTTP wait fails generically while the daemon refresh worker continues. Subsequent runs rescan after a successful reload; active runs keep their existing snapshot. Each Uvicorn worker/Kubernetes Pod must be targeted separately. Direct mount writes bypass install/edit validation, SkillScan, and history, so mounted roots are an operator-controlled trust boundary.
|
||||
- **Tool policy**: Agent `allowed-tools` declarations apply dynamically only to slash-activated skills and skills captured in `ThreadState.skill_context` through configured `read_file` loads; passive enabled skills and custom-agent/subagent skill allowlists remain discoverable without clamping the baseline toolset. Subagents render only skill discovery metadata at startup and reuse the same adjacent `SkillActivationMiddleware` + `SkillToolPolicyMiddleware` pair as the lead; their configured `skills` field limits discovery and activation instead of eagerly loading bodies or unioning policies. Slash policy is dominant for its run, preventing subsequently read skills from widening explicit authority; autonomous captured skills use the existing union only when no slash source exists. `tool_search` and `describe_skill` stay available as framework discovery infrastructure, while every discovered or promoted business tool still requires active-policy permission for schema visibility and execution; `task`, `list_background_tasks`, and `cancel_background_task` likewise require explicit declarations. Each active model call intentionally reloads the full live registry so enable/disable changes, frontmatter edits, and custom/public name-shadow winners take effect without a stale TTL or unsafe direct-path cache; all tool calls produced by that model step reuse the resulting source-and-path-signed decision. Registry failures and all-invalid active sets fail closed, while stale individual paths are skipped when another valid skill remains. This is best-effort behavioral scoping, not a hard security boundary: alternate loading paths are not captured and bounded autonomous context may evict entries.
|
||||
|
||||
@ -17,6 +17,7 @@ ALLOWED_FRONTMATTER_PROPERTIES = {
|
||||
"description",
|
||||
"license",
|
||||
"allowed-tools",
|
||||
"argument-hint",
|
||||
"required-secrets",
|
||||
"secrets-autonomous",
|
||||
"metadata",
|
||||
|
||||
@ -10,6 +10,71 @@ logger = logging.getLogger(__name__)
|
||||
|
||||
# Valid POSIX environment-variable name.
|
||||
_ENV_VAR_NAME_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$")
|
||||
_PORTABLE_TOOL_ALIASES = {
|
||||
"Bash": "bash",
|
||||
"Edit": "str_replace",
|
||||
"Glob": "glob",
|
||||
"Grep": "grep",
|
||||
"Read": "read_file",
|
||||
"WebFetch": "web_fetch",
|
||||
"WebSearch": "web_search",
|
||||
"Write": "write_file",
|
||||
}
|
||||
|
||||
|
||||
def _normalize_unscoped_allowed_tool(tool_name: str) -> str:
|
||||
"""Map known portable aliases while preserving unknown runtime names."""
|
||||
if "(" in tool_name or ")" in tool_name:
|
||||
return tool_name
|
||||
return _PORTABLE_TOOL_ALIASES.get(tool_name, tool_name)
|
||||
|
||||
|
||||
def _split_portable_allowed_tools(raw: str, skill_file: Path) -> list[str]:
|
||||
"""Split a portable scalar while preserving quoted parenthesized patterns."""
|
||||
tokens: list[str] = []
|
||||
current: list[str] = []
|
||||
depth = 0
|
||||
quote: str | None = None
|
||||
escaped = False
|
||||
|
||||
for char in raw:
|
||||
if escaped:
|
||||
current.append(char)
|
||||
escaped = False
|
||||
continue
|
||||
if char == "\\":
|
||||
current.append(char)
|
||||
escaped = True
|
||||
continue
|
||||
if quote is not None:
|
||||
current.append(char)
|
||||
if char == quote:
|
||||
quote = None
|
||||
continue
|
||||
if char in {"'", '"'}:
|
||||
quote = char
|
||||
current.append(char)
|
||||
continue
|
||||
if char.isspace() and depth == 0:
|
||||
if current:
|
||||
tokens.append("".join(current))
|
||||
current = []
|
||||
continue
|
||||
if char == "(":
|
||||
depth += 1
|
||||
elif char == ")":
|
||||
if depth == 0:
|
||||
raise ValueError(f"allowed-tools in {skill_file} contains an unmatched closing parenthesis")
|
||||
depth -= 1
|
||||
current.append(char)
|
||||
|
||||
if quote is not None:
|
||||
raise ValueError(f"allowed-tools in {skill_file} contains an unclosed quote")
|
||||
if depth:
|
||||
raise ValueError(f"allowed-tools in {skill_file} contains an unclosed parenthesized pattern")
|
||||
if current:
|
||||
tokens.append("".join(current))
|
||||
return tokens
|
||||
|
||||
|
||||
def _format_yaml_error(skill_file: Path, exc: yaml.YAMLError, source: str) -> str:
|
||||
@ -41,14 +106,22 @@ def _format_yaml_error(skill_file: Path, exc: yaml.YAMLError, source: str) -> st
|
||||
def parse_allowed_tools(raw: object, skill_file: Path) -> tuple[str, ...] | None:
|
||||
"""Parse the optional allowed-tools frontmatter field.
|
||||
|
||||
Returns None when the field is omitted. Returns a tuple when the field is a
|
||||
YAML sequence of strings, including an empty tuple for explicit no-tool
|
||||
skills. Raises ValueError for malformed values.
|
||||
Returns None when the field is omitted. Accepts the Agent Skills standard
|
||||
space-separated string or a YAML sequence of strings. Known portable client
|
||||
aliases normalize to DeerFlow runtime names. Unknown names and
|
||||
command-scoped patterns remain literal because DeerFlow does not inspect
|
||||
tool arguments. Returns an empty tuple for an explicit empty value. Raises
|
||||
ValueError for malformed values.
|
||||
"""
|
||||
if raw is None:
|
||||
return None
|
||||
if not isinstance(raw, list):
|
||||
raise ValueError(f"allowed-tools in {skill_file} must be a list of strings")
|
||||
if isinstance(raw, str):
|
||||
raw = _split_portable_allowed_tools(raw, skill_file)
|
||||
normalize_tools = True
|
||||
elif not isinstance(raw, list):
|
||||
raise ValueError(f"allowed-tools in {skill_file} must be a space-separated string or list of strings")
|
||||
else:
|
||||
normalize_tools = False
|
||||
|
||||
allowed_tools: list[str] = []
|
||||
for item in raw:
|
||||
@ -57,7 +130,7 @@ def parse_allowed_tools(raw: object, skill_file: Path) -> tuple[str, ...] | None
|
||||
tool_name = item.strip()
|
||||
if not tool_name:
|
||||
raise ValueError(f"allowed-tools in {skill_file} cannot contain empty tool names")
|
||||
allowed_tools.append(tool_name)
|
||||
allowed_tools.append(_normalize_unscoped_allowed_tool(tool_name) if normalize_tools else tool_name)
|
||||
return tuple(allowed_tools)
|
||||
|
||||
|
||||
|
||||
@ -240,7 +240,7 @@ def _analyze_skill_md(content: str, *, profile: ProfileName, findings: list[dict
|
||||
severity="error",
|
||||
path="SKILL.md",
|
||||
message=str(exc),
|
||||
remediation="Declare allowed-tools as a YAML list of non-empty strings.",
|
||||
remediation="Declare allowed-tools as a space-separated string or YAML list of non-empty strings.",
|
||||
)
|
||||
)
|
||||
|
||||
|
||||
@ -12,6 +12,7 @@ from langgraph.prebuilt.tool_node import ToolCallRequest
|
||||
from langgraph.runtime import Runtime
|
||||
|
||||
from deerflow.runtime.secret_context import SKILL_TOOL_POLICY_DECISION_CONTEXT_KEY, write_slash_skill_source_path
|
||||
from deerflow.skills.parser import parse_skill_file
|
||||
from deerflow.skills.types import Skill, SkillCategory
|
||||
|
||||
_SLASH_SOURCE_OWNER_TOKEN = "test-slash-source-owner"
|
||||
@ -169,6 +170,131 @@ def test_slash_activated_skill_filters_first_model_call_and_task():
|
||||
assert _tool_names(filtered) == ["read_file", "review_skill_package"]
|
||||
|
||||
|
||||
def test_slash_activation_normalizes_unscoped_portable_tools_but_not_command_patterns(tmp_path):
|
||||
skill_dir = tmp_path / "portable"
|
||||
skill_dir.mkdir()
|
||||
skill_file = skill_dir / "SKILL.md"
|
||||
skill_file.write_text(
|
||||
"---\nname: portable\ndescription: Portable tools\nallowed-tools: WebFetch Bash(git add *)\n---\nBody\n",
|
||||
encoding="utf-8",
|
||||
)
|
||||
skill = parse_skill_file(skill_file, category=SkillCategory.CUSTOM)
|
||||
assert skill is not None
|
||||
context = {}
|
||||
write_slash_skill_source_path(
|
||||
context,
|
||||
skill.get_container_file_path(),
|
||||
owner_token=_SLASH_SOURCE_OWNER_TOKEN,
|
||||
)
|
||||
middleware = _middleware([skill])
|
||||
request = ModelRequestStub(
|
||||
[NamedTool("bash"), NamedTool("web_fetch"), NamedTool("web_search"), NamedTool("add")],
|
||||
context=context,
|
||||
)
|
||||
|
||||
filtered = middleware.wrap_model_call(request, lambda model_request: model_request)
|
||||
|
||||
assert _tool_names(filtered) == ["web_fetch"]
|
||||
assert (
|
||||
middleware.wrap_tool_call(
|
||||
ToolRequestStub("web_fetch", context=context),
|
||||
lambda _: "executed",
|
||||
)
|
||||
== "executed"
|
||||
)
|
||||
blocked = middleware.wrap_tool_call(
|
||||
ToolRequestStub("bash", context=context),
|
||||
lambda _: "executed",
|
||||
)
|
||||
assert blocked.status == "error"
|
||||
blocked_fragment = middleware.wrap_tool_call(
|
||||
ToolRequestStub("add", context=context),
|
||||
lambda _: "executed",
|
||||
)
|
||||
assert blocked_fragment.status == "error"
|
||||
|
||||
|
||||
def test_slash_activation_preserves_scalar_custom_tool_names(tmp_path):
|
||||
skill_dir = tmp_path / "custom-tool"
|
||||
skill_dir.mkdir()
|
||||
skill_file = skill_dir / "SKILL.md"
|
||||
skill_file.write_text(
|
||||
"---\nname: custom-tool\ndescription: Custom tool\nallowed-tools: mcp__arxiv__SearchPapers\n---\nBody\n",
|
||||
encoding="utf-8",
|
||||
)
|
||||
skill = parse_skill_file(skill_file, category=SkillCategory.CUSTOM)
|
||||
assert skill is not None
|
||||
context = {}
|
||||
write_slash_skill_source_path(
|
||||
context,
|
||||
skill.get_container_file_path(),
|
||||
owner_token=_SLASH_SOURCE_OWNER_TOKEN,
|
||||
)
|
||||
middleware = _middleware([skill])
|
||||
request = ModelRequestStub(
|
||||
[NamedTool("mcp__arxiv__SearchPapers"), NamedTool("mcp__arxiv__search_papers")],
|
||||
context=context,
|
||||
)
|
||||
|
||||
filtered = middleware.wrap_model_call(request, lambda model_request: model_request)
|
||||
|
||||
assert _tool_names(filtered) == ["mcp__arxiv__SearchPapers"]
|
||||
|
||||
|
||||
def test_slash_activation_preserves_lowercase_exact_tool_authority(tmp_path):
|
||||
skill_dir = tmp_path / "lowercase-tool"
|
||||
skill_dir.mkdir()
|
||||
skill_file = skill_dir / "SKILL.md"
|
||||
skill_file.write_text(
|
||||
"---\nname: lowercase-tool\ndescription: Lowercase tool\nallowed-tools: write\n---\nBody\n",
|
||||
encoding="utf-8",
|
||||
)
|
||||
skill = parse_skill_file(skill_file, category=SkillCategory.CUSTOM)
|
||||
assert skill is not None
|
||||
context = {}
|
||||
write_slash_skill_source_path(
|
||||
context,
|
||||
skill.get_container_file_path(),
|
||||
owner_token=_SLASH_SOURCE_OWNER_TOKEN,
|
||||
)
|
||||
middleware = _middleware([skill])
|
||||
request = ModelRequestStub(
|
||||
[NamedTool("write"), NamedTool("write_file")],
|
||||
context=context,
|
||||
)
|
||||
|
||||
filtered = middleware.wrap_model_call(request, lambda model_request: model_request)
|
||||
|
||||
assert _tool_names(filtered) == ["write"]
|
||||
|
||||
|
||||
def test_slash_activation_normalizes_glob_and_grep_aliases(tmp_path):
|
||||
skill_dir = tmp_path / "search-tools"
|
||||
skill_dir.mkdir()
|
||||
skill_file = skill_dir / "SKILL.md"
|
||||
skill_file.write_text(
|
||||
"---\nname: search-tools\ndescription: Search tools\nallowed-tools: Glob Grep\n---\nBody\n",
|
||||
encoding="utf-8",
|
||||
)
|
||||
skill = parse_skill_file(skill_file, category=SkillCategory.CUSTOM)
|
||||
assert skill is not None
|
||||
context = {}
|
||||
write_slash_skill_source_path(
|
||||
context,
|
||||
skill.get_container_file_path(),
|
||||
owner_token=_SLASH_SOURCE_OWNER_TOKEN,
|
||||
)
|
||||
middleware = _middleware([skill])
|
||||
request = ModelRequestStub(
|
||||
[NamedTool("glob"), NamedTool("grep"), NamedTool("Glob"), NamedTool("Grep")],
|
||||
context=context,
|
||||
)
|
||||
|
||||
filtered = middleware.wrap_model_call(request, lambda model_request: model_request)
|
||||
|
||||
assert _tool_names(filtered) == ["glob", "grep"]
|
||||
|
||||
|
||||
@pytest.mark.parametrize("active_source", ["slash", "skill_context"])
|
||||
def test_restrictive_skill_explicitly_allows_task_schema_and_execution(active_source):
|
||||
skill = _skill("delegating", ["task"])
|
||||
|
||||
@ -87,6 +87,21 @@ def test_local_storage_accepts_external_custom_skill_directory_symlink(tmp_path:
|
||||
assert storage.validate_skill_file_path(linked_file) == external_file
|
||||
|
||||
|
||||
def test_load_skills_discovers_parenthesized_portable_allowed_tools(tmp_path: Path):
|
||||
skills_root = tmp_path / "skills"
|
||||
skill_dir = skills_root / "custom" / "tavily-cli"
|
||||
skill_dir.mkdir(parents=True)
|
||||
(skill_dir / "SKILL.md").write_text(
|
||||
"---\nname: tavily-cli\ndescription: Tavily CLI\nallowed-tools: Bash(tvly *)\n---\n\n# Tavily CLI\n",
|
||||
encoding="utf-8",
|
||||
)
|
||||
|
||||
skills = get_or_new_skill_storage(skills_path=skills_root).load_skills(enabled_only=False)
|
||||
|
||||
skill = next(skill for skill in skills if skill.name == "tavily-cli")
|
||||
assert skill.allowed_tools == ("Bash(tvly *)",)
|
||||
|
||||
|
||||
def test_load_skills_stops_at_skill_package_boundary(tmp_path: Path):
|
||||
"""SKILL.md files inside an existing skill package are support data, not skills."""
|
||||
skills_root = tmp_path / "skills"
|
||||
|
||||
@ -13,6 +13,8 @@ from __future__ import annotations
|
||||
import logging
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
from deerflow.skills.parser import parse_skill_file
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
@ -108,8 +110,113 @@ def test_parse_empty_allowed_tools_list(tmp_path):
|
||||
assert skill.allowed_tools == ()
|
||||
|
||||
|
||||
def test_parse_allowed_tools_string(tmp_path):
|
||||
skill_file = _write_skill(
|
||||
tmp_path,
|
||||
"name: my-skill\ndescription: Test\nallowed-tools: Bash WebFetch Read Write Edit Bash(git:*)",
|
||||
)
|
||||
skill = parse_skill_file(skill_file, category="custom")
|
||||
assert skill is not None
|
||||
assert skill.allowed_tools == (
|
||||
"bash",
|
||||
"web_fetch",
|
||||
"read_file",
|
||||
"write_file",
|
||||
"str_replace",
|
||||
"Bash(git:*)",
|
||||
)
|
||||
|
||||
|
||||
def test_parse_allowed_tools_string_preserves_parenthesized_spaces(tmp_path):
|
||||
skill_file = _write_skill(
|
||||
tmp_path,
|
||||
"name: my-skill\ndescription: Test\nallowed-tools: Bash(tvly *) Bash(playwright-cli:*) Bash(npx:*) Bash(npm:*)",
|
||||
)
|
||||
|
||||
skill = parse_skill_file(skill_file, category="custom")
|
||||
|
||||
assert skill is not None
|
||||
assert skill.allowed_tools == (
|
||||
"Bash(tvly *)",
|
||||
"Bash(playwright-cli:*)",
|
||||
"Bash(npx:*)",
|
||||
"Bash(npm:*)",
|
||||
)
|
||||
|
||||
|
||||
def test_parse_allowed_tools_string_preserves_scalar_custom_tool_names(tmp_path):
|
||||
skill_file = _write_skill(
|
||||
tmp_path,
|
||||
"name: my-skill\ndescription: Test\nallowed-tools: mcp__arxiv__SearchPapers MyCustomTool write",
|
||||
)
|
||||
|
||||
skill = parse_skill_file(skill_file, category="custom")
|
||||
|
||||
assert skill is not None
|
||||
assert skill.allowed_tools == ("mcp__arxiv__SearchPapers", "MyCustomTool", "write")
|
||||
|
||||
|
||||
def test_parse_allowed_tools_string_normalizes_glob_and_grep_aliases(tmp_path):
|
||||
skill_file = _write_skill(
|
||||
tmp_path,
|
||||
"name: my-skill\ndescription: Test\nallowed-tools: Glob Grep",
|
||||
)
|
||||
|
||||
skill = parse_skill_file(skill_file, category="custom")
|
||||
|
||||
assert skill is not None
|
||||
assert skill.allowed_tools == ("glob", "grep")
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"allowed_tools",
|
||||
[
|
||||
'Bash("echo )")',
|
||||
"Bash('echo )')",
|
||||
r"Bash(echo \) literal)",
|
||||
],
|
||||
)
|
||||
def test_parse_allowed_tools_string_preserves_quoted_and_escaped_parentheses(tmp_path, allowed_tools):
|
||||
skill_file = _write_skill(tmp_path, f"name: my-skill\ndescription: Test\nallowed-tools: {allowed_tools}")
|
||||
|
||||
skill = parse_skill_file(skill_file, category="custom")
|
||||
|
||||
assert skill is not None
|
||||
assert skill.allowed_tools == (allowed_tools,)
|
||||
|
||||
|
||||
def test_parse_allowed_tools_list_preserves_exact_runtime_names(tmp_path):
|
||||
skill_file = _write_skill(
|
||||
tmp_path,
|
||||
"name: my-skill\ndescription: Test\nallowed-tools: [mcp__arxiv__SearchPapers, Read, 'Bash(tvly *)']\n",
|
||||
)
|
||||
|
||||
skill = parse_skill_file(skill_file, category="custom")
|
||||
|
||||
assert skill is not None
|
||||
assert skill.allowed_tools == ("mcp__arxiv__SearchPapers", "Read", "Bash(tvly *)")
|
||||
|
||||
|
||||
def test_parse_allowed_tools_string_rejects_unbalanced_parentheses(tmp_path):
|
||||
skill_file = _write_skill(
|
||||
tmp_path,
|
||||
"name: my-skill\ndescription: Test\nallowed-tools: Bash(tvly *",
|
||||
)
|
||||
|
||||
assert parse_skill_file(skill_file, category="custom") is None
|
||||
|
||||
|
||||
def test_parse_allowed_tools_string_rejects_unmatched_closing_parenthesis(tmp_path):
|
||||
skill_file = _write_skill(
|
||||
tmp_path,
|
||||
"name: my-skill\ndescription: Test\nallowed-tools: Bash(tvly *) )",
|
||||
)
|
||||
|
||||
assert parse_skill_file(skill_file, category="custom") is None
|
||||
|
||||
|
||||
def test_parse_invalid_allowed_tools_returns_none(tmp_path):
|
||||
skill_file = _write_skill(tmp_path, "name: my-skill\ndescription: Test\nallowed-tools: bash")
|
||||
skill_file = _write_skill(tmp_path, "name: my-skill\ndescription: Test\nallowed-tools: {bash: true}")
|
||||
skill = parse_skill_file(skill_file, category="custom")
|
||||
assert skill is None
|
||||
|
||||
|
||||
@ -47,17 +47,25 @@ class TestValidateSkillFrontmatter:
|
||||
assert msg == "Skill is valid!"
|
||||
assert name == "my-skill"
|
||||
|
||||
def test_rejects_allowed_tools_string(self, tmp_path):
|
||||
def test_allows_argument_hint(self, tmp_path):
|
||||
skill_dir = _write_skill(
|
||||
tmp_path,
|
||||
"---\nname: my-skill\ndescription: A skill\nallowed-tools: bash\n---\n\nBody\n",
|
||||
"---\nname: my-skill\ndescription: A skill\nargument-hint: '[issue-number]'\n---\n\nBody\n",
|
||||
)
|
||||
valid, msg, name = _validate_skill_frontmatter(skill_dir)
|
||||
assert valid is False
|
||||
assert "allowed-tools" in msg
|
||||
assert str(tmp_path) not in msg
|
||||
assert "SKILL.md" in msg
|
||||
assert name is None
|
||||
assert valid is True
|
||||
assert msg == "Skill is valid!"
|
||||
assert name == "my-skill"
|
||||
|
||||
def test_allows_allowed_tools_string(self, tmp_path):
|
||||
skill_dir = _write_skill(
|
||||
tmp_path,
|
||||
"---\nname: my-skill\ndescription: A skill\nallowed-tools: Bash(tvly *) Bash(playwright-cli:*)\n---\n\nBody\n",
|
||||
)
|
||||
valid, msg, name = _validate_skill_frontmatter(skill_dir)
|
||||
assert valid is True
|
||||
assert msg == "Skill is valid!"
|
||||
assert name == "my-skill"
|
||||
|
||||
def test_rejects_allowed_tools_non_string_entry(self, tmp_path):
|
||||
skill_dir = _write_skill(
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user