diff --git a/libs/deepagents/deepagents/middleware/filesystem.py b/libs/deepagents/deepagents/middleware/filesystem.py index 6ea29998816..a0381636277 100644 --- a/libs/deepagents/deepagents/middleware/filesystem.py +++ b/libs/deepagents/deepagents/middleware/filesystem.py @@ -1330,16 +1330,20 @@ def __init__( ) self._glob_slots = threading.BoundedSemaphore(_SYNC_GLOB_WORKERS) - self.tools = [ - self._create_ls_tool(), - self._create_read_file_tool(), - self._create_write_file_tool(), - self._create_edit_file_tool(), - self._create_delete_tool(), - self._create_glob_tool(), - self._create_grep_tool(), - self._create_execute_tool(), - ] + tool_factories: tuple[tuple[str, Callable[[], BaseTool]], ...] = ( + ("ls", self._create_ls_tool), + ("read_file", self._create_read_file_tool), + ("write_file", self._create_write_file_tool), + ("edit_file", self._create_edit_file_tool), + ("delete", self._create_delete_tool), + ("glob", self._create_glob_tool), + ("grep", self._create_grep_tool), + ("execute", self._create_execute_tool), + ) + # Excluded tools are omitted here entirely, not just hidden from the + # model's schema, so a tool name outside `tools=` never reaches the + # dispatchable tool node + self.tools = [factory() for name, factory in tool_factories if self._enabled_tools is None or name in self._enabled_tools] def _build_dynamic_system_prompt(self, *, include_execution: bool) -> str: """Build (and memoize) the dynamic system prompt. @@ -2289,11 +2293,11 @@ def _unsupported_tools_and_execution_state( runtime: Runtime[ContextT], ) -> tuple[set[str | None], bool, BackendProtocol | None]: """Return unsupported filesystem tools and whether execute remains active.""" - unsupported: set[str | None] = ( - {name for name in tool_names if name in _ALL_FS_TOOL_NAMES and name not in self._enabled_tools} - if self._enabled_tools is not None - else set() - ) + # `tools=` exclusions are enforced at `__init__` (absent from + # `self.tools` entirely), so only backend-capability gating + # `execute`/`delete` on a backend that doesn't support them is + # computed here. + unsupported: set[str | None] = set() execution_active = False backend = None has_execute_tool = "execute" in tool_names diff --git a/libs/deepagents/tests/unit_tests/test_end_to_end.py b/libs/deepagents/tests/unit_tests/test_end_to_end.py index eb2e4fa65d9..8ec8dbd4542 100644 --- a/libs/deepagents/tests/unit_tests/test_end_to_end.py +++ b/libs/deepagents/tests/unit_tests/test_end_to_end.py @@ -4661,3 +4661,43 @@ def test_allowlist_removes_tools_from_request_and_system_prompt(self) -> None: assert "`ls`" in prompt for disabled in ("write_file", "edit_file", "delete", "glob"): assert f"`{disabled}`" not in prompt, f"`{disabled}` should not appear in system prompt tool list" + + def test_excluded_tool_call_fails_instead_of_executing(self) -> None: + """An excluded tool referenced in a `ToolCall` errors instead of executing. + + Simulates an out-of-schema `write_file` call despite + `tools=["ls", "read_file"]`. Before the fix, `write_file` was still + registered on `ToolNode` and would execute. After the fix, it's no longer + registered, so `ToolNode` returns an error `ToolMessage` instead. + """ + model = FixedGenericFakeChatModel( + messages=iter( + [ + AIMessage( + content="", + tool_calls=[ + { + "name": "write_file", + "args": {"file_path": "/pwned.txt", "content": "hi"}, + "id": "call_1", + "type": "tool_call", + } + ], + ), + AIMessage(content="done"), + ] + ) + ) + agent = create_deep_agent( + model=model, + middleware=[FilesystemMiddleware(backend=StateBackend(), tools=["ls", "read_file"])], + ) + + result = agent.invoke({"messages": [HumanMessage(content="hi")]}) + + tool_messages = [m for m in result["messages"] if isinstance(m, ToolMessage)] + assert len(tool_messages) == 1 + assert tool_messages[0].status == "error" + assert "write_file" in tool_messages[0].content + # The excluded tool must not have actually run. + assert "/pwned.txt" not in result.get("files", {}) diff --git a/libs/deepagents/tests/unit_tests/test_middleware.py b/libs/deepagents/tests/unit_tests/test_middleware.py index 02fc73f05de..f8245fa552d 100644 --- a/libs/deepagents/tests/unit_tests/test_middleware.py +++ b/libs/deepagents/tests/unit_tests/test_middleware.py @@ -2122,117 +2122,53 @@ def test_enabled_tools_raises_when_read_file_excluded(self): with pytest.raises(ValueError, match="read_file must be included in tools"): FilesystemMiddleware(backend=StateBackend(), tools=["write_file"]) - def test_enabled_tools_filters_unlisted_tool(self): - """A tool not in tools is filtered out of the model request.""" - middleware = FilesystemMiddleware( - backend=StateBackend(), - system_prompt="", - tools=["read_file", "ls"], - ) - write_tool = MagicMock() - write_tool.name = "write_file" - ls_tool = MagicMock() - ls_tool.name = "ls" - request = MagicMock() - request.tools = [ls_tool, write_tool] - request.override.return_value = request - - middleware._filter_unsupported_tools_and_apply_prompt(request) - - filtered_names = {tool.name for tool in request.override.call_args.kwargs["tools"]} - assert "write_file" not in filtered_names - assert "ls" in filtered_names - def test_enabled_tools_filters_multiple_unlisted(self): - """Only tools in the allowlist survive; the rest are filtered.""" + """Only tools in the allowlist are registered; the rest are absent from `self.tools`.""" middleware = FilesystemMiddleware( backend=StateBackend(), system_prompt="", tools=["read_file", "ls", "grep"], ) - tools = [MagicMock(name=n) for n in ("ls", "write_file", "delete", "grep")] - for t in tools: - t.name = t._mock_name - request = MagicMock() - request.tools = tools - request.override.return_value = request - - middleware._filter_unsupported_tools_and_apply_prompt(request) + names = {tool.name for tool in middleware.tools} + assert "write_file" not in names + assert "delete" not in names + assert "ls" in names + assert "grep" in names - filtered_names = {tool.name for tool in request.override.call_args.kwargs["tools"]} - assert "write_file" not in filtered_names - assert "delete" not in filtered_names - assert "ls" in filtered_names - assert "grep" in filtered_names + def test_enabled_tools_does_not_double_filter_user_provided_tools(self): + """`wrap_model_call` doesn't re-filter or touch non-filesystem tools. - def test_enabled_tools_does_not_filter_user_provided_tools(self): - """User-provided (non-filesystem) tools are never removed by the allowlist.""" + `request.tools` here mirrors what `create_agent` actually assembles: + the middleware's own (already-restricted) `self.tools`, plus a + separate user-provided tool it never owns. + """ middleware = FilesystemMiddleware( backend=StateBackend(), system_prompt="", tools=["read_file", "ls"], ) - ls_tool = MagicMock() - ls_tool.name = "ls" - write_tool = MagicMock() - write_tool.name = "write_file" custom_tool = MagicMock() custom_tool.name = "search" request = MagicMock() - request.tools = [ls_tool, write_tool, custom_tool] + request.tools = [*middleware.tools, custom_tool] request.override.return_value = request middleware._filter_unsupported_tools_and_apply_prompt(request) - filtered_names = {tool.name for tool in request.override.call_args.kwargs["tools"]} - assert "ls" in filtered_names - assert "search" in filtered_names # user tool untouched - assert "write_file" not in filtered_names # FS tool not in allowlist - - def test_enabled_tools_passes_listed_tools_through(self): - """Tools in the allowlist survive; an unlisted tool is dropped.""" - middleware = FilesystemMiddleware( - backend=StateBackend(), - system_prompt="", - tools=["read_file", "ls", "grep"], - ) - ls_tool = MagicMock() - ls_tool.name = "ls" - grep_tool = MagicMock() - grep_tool.name = "grep" - edit_tool = MagicMock() - edit_tool.name = "edit_file" - request = MagicMock() - request.tools = [ls_tool, grep_tool, edit_tool] - request.override.return_value = request - - middleware._filter_unsupported_tools_and_apply_prompt(request) - - filtered_names = {tool.name for tool in request.override.call_args.kwargs["tools"]} - assert "ls" in filtered_names - assert "grep" in filtered_names - assert "edit_file" not in filtered_names + # Nothing left to filter as the allowlist was already applied at + # construction, so no `tools=...` override should occur. + tools_overrides = [c for c in request.override.call_args_list if "tools" in c.kwargs] + assert tools_overrides == [] def test_enabled_tools_read_file_only_filters_everything_else(self): - """tools=["read_file"] hides all other tools.""" - all_other_tools = {"ls", "write_file", "edit_file", "delete", "glob", "grep", "execute"} + """tools=["read_file"] leaves only read_file registered on `self.tools`.""" middleware = FilesystemMiddleware( backend=StateBackend(), system_prompt="", tools=["read_file"], ) - tools = [MagicMock() for _ in range(len(all_other_tools) + 1)] - for t, name in zip(tools, [*all_other_tools, "read_file"], strict=True): - t.name = name - request = MagicMock() - request.tools = tools - request.override.return_value = request - - middleware._filter_unsupported_tools_and_apply_prompt(request) - - filtered_names = {tool.name for tool in request.override.call_args.kwargs["tools"]} - assert "read_file" in filtered_names - assert filtered_names == {"read_file"} + names = {tool.name for tool in middleware.tools} + assert names == {"read_file"} def test_enabled_tools_none_default_passes_all_tools(self): """tools=None (default) does not filter any tools.""" @@ -2253,6 +2189,34 @@ def test_enabled_tools_none_default_passes_all_tools(self): tools_overrides = [c for c in request.override.call_args_list if "tools" in c.kwargs] assert tools_overrides == [] + def test_enabled_tools_excluded_from_self_tools_not_just_request(self): + """Excluded tools are absent from `self.tools` itself, not just `request.tools`. + + `self.tools` is what `create_agent` registers on the dispatchable + `ToolNode` — filtering only `request.tools` in `wrap_model_call` would + leave an excluded tool callable if a `ToolCall` for it ever appeared + (e.g. a stray/hallucinated tool call), since `ToolNode` dispatches by + name lookup independent of what was bound for a given request. + """ + middleware = FilesystemMiddleware( + backend=StateBackend(), + tools=["read_file", "ls"], + ) + names = {tool.name for tool in middleware.tools} + assert names == {"ls", "read_file"} + + def test_enabled_tools_none_default_keeps_all_self_tools(self): + """tools=None (default) still registers every filesystem tool.""" + middleware = FilesystemMiddleware(backend=StateBackend()) + names = {tool.name for tool in middleware.tools} + assert names == {"ls", "read_file", "write_file", "edit_file", "delete", "glob", "grep", "execute"} + + def test_enabled_tools_all_keeps_all_self_tools(self): + """tools="all" registers every filesystem tool, same as the default.""" + middleware = FilesystemMiddleware(backend=StateBackend(), tools="all") + names = {tool.name for tool in middleware.tools} + assert names == {"ls", "read_file", "write_file", "edit_file", "delete", "glob", "grep", "execute"} + def test_enabled_tools_execute_listed_but_backend_unsupported_is_noop(self): """Execute in tools list is still filtered when the backend doesn't support execution.""" middleware = FilesystemMiddleware( @@ -2374,17 +2338,13 @@ def test_grep_description_swap_copies_dict_tool_specs(self): assert "LITERAL text pattern" in rewritten_grep["description"] def test_enabled_tools_system_prompt_lists_only_enabled_tools(self): - """Dynamic system prompt only mentions the tools that survived filtering.""" + """Dynamic system prompt only mentions tools registered on `self.tools`.""" middleware = FilesystemMiddleware( backend=StateBackend(), tools=["read_file", "ls"], ) - ls_tool = MagicMock() - ls_tool.name = "ls" - write_tool = MagicMock() - write_tool.name = "write_file" request = MagicMock() - request.tools = [ls_tool, write_tool] + request.tools = middleware.tools request.system_message = None request.override.return_value = request