-
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 3 commits
6403d0a
dc30227
316809f
3979b31
7b62d0f
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.
Contributor
Author
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. Updated the activation regression to use Bash(git add *) and include a tool named add in the model request. The policy keeps the parenthesized declaration as one inactive literal token, so both bash and the add fragment are rejected while the explicitly normalized web_fetch tool remains available. |
||
| 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.Uh oh!
There was an error while loading. Please reload this page.
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.
Addressed in commit 7b62d0f.
The portable alias table now matches exact documented spellings only; alias lookup no longer case-folds arbitrary scalar names. Therefore Write maps to write_file, while a literal write remains write. Added an activation-level regression with both write and write_file present to verify that the declared authority is not substituted.