-
Notifications
You must be signed in to change notification settings - Fork 11.1k
fix(skills): safely tokenize portable allowed-tools patterns #4984
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ | |
| 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,45 @@ 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:*)\n---\nBody\n", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Exercise the new spaced-pattern path at activation level. |
||
| 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")], | ||
| 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" | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("active_source", ["slash", "skill_context"]) | ||
| def test_restrictive_skill_explicitly_allows_task_schema_and_execution(active_source): | ||
| skill = _skill("delegating", ["task"]) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P1] Do not map exact lowercase custom tools onto different built-ins. Because the alias lookup happens after
casefold(), a scalar declaration such asallowed-tools: writeis converted towrite_file. The runtime policy then removes an installed tool actually namedwriteand exposes DeerFlow's real file-writing tool instead. This is an authority substitution, not only a failed exact match. Please restrictRead/Write/Editaliases to the intended portable spellings before case folding, or resolve normalization without granting a second tool when the literal name is an exact runtime tool. Add a policy regression case with bothwriteandwrite_filepresent.