diff --git a/nanobot/agent/context.py b/nanobot/agent/context.py index a6c3eea..e47fcb8 100644 --- a/nanobot/agent/context.py +++ b/nanobot/agent/context.py @@ -46,7 +46,7 @@ class ContextBuilder: if skills_summary: parts.append(f"""# Skills -The following skills extend your capabilities. To use a skill, call the load_skill tool with its name. +The following skills extend your capabilities. To use a skill, read its SKILL.md file using the read_file tool. Skills with available="false" need dependencies installed first - you can try installing them with apt/brew. {skills_summary}""") diff --git a/nanobot/agent/loop.py b/nanobot/agent/loop.py index 9cbdaf8..2c0d29a 100644 --- a/nanobot/agent/loop.py +++ b/nanobot/agent/loop.py @@ -17,7 +17,8 @@ from nanobot.agent.context import ContextBuilder from nanobot.agent.memory import MemoryConsolidator from nanobot.agent.subagent import SubagentManager from nanobot.agent.tools.cron import CronTool -from nanobot.agent.tools.filesystem import EditFileTool, ListDirTool, LoadSkillTool, ReadFileTool, WriteFileTool +from nanobot.agent.skills import BUILTIN_SKILLS_DIR +from nanobot.agent.tools.filesystem import EditFileTool, ListDirTool, ReadFileTool, WriteFileTool from nanobot.agent.tools.message import MessageTool from nanobot.agent.tools.registry import ToolRegistry from nanobot.agent.tools.shell import ExecTool @@ -114,7 +115,9 @@ class AgentLoop: def _register_default_tools(self) -> None: """Register the default set of tools.""" allowed_dir = self.workspace if self.restrict_to_workspace else None - for cls in (ReadFileTool, WriteFileTool, EditFileTool, ListDirTool): + extra_read = [BUILTIN_SKILLS_DIR] if allowed_dir else None + self.tools.register(ReadFileTool(workspace=self.workspace, allowed_dir=allowed_dir, extra_allowed_dirs=extra_read)) + for cls in (WriteFileTool, EditFileTool, ListDirTool): self.tools.register(cls(workspace=self.workspace, allowed_dir=allowed_dir)) self.tools.register(ExecTool( working_dir=str(self.workspace), @@ -128,7 +131,6 @@ class AgentLoop: self.tools.register(SpawnTool(manager=self.subagents)) if self.cron_service: self.tools.register(CronTool(self.cron_service)) - self.tools.register(LoadSkillTool(skills_loader=self.context.skills)) async def _connect_mcp(self) -> None: """Connect to configured MCP servers (one-time, lazy).""" diff --git a/nanobot/agent/skills.py b/nanobot/agent/skills.py index 2b869fa..9afee82 100644 --- a/nanobot/agent/skills.py +++ b/nanobot/agent/skills.py @@ -118,6 +118,7 @@ class SkillsLoader: lines = [""] for s in all_skills: name = escape_xml(s["name"]) + path = s["path"] desc = escape_xml(self._get_skill_description(s["name"])) skill_meta = self._get_skill_meta(s["name"]) available = self._check_requirements(skill_meta) @@ -125,6 +126,7 @@ class SkillsLoader: lines.append(f" ") lines.append(f" {name}") lines.append(f" {desc}") + lines.append(f" {path}") # Show missing requirements for unavailable skills if not available: diff --git a/nanobot/agent/subagent.py b/nanobot/agent/subagent.py index cdde30d..063b54c 100644 --- a/nanobot/agent/subagent.py +++ b/nanobot/agent/subagent.py @@ -8,6 +8,7 @@ from typing import Any from loguru import logger +from nanobot.agent.skills import BUILTIN_SKILLS_DIR from nanobot.agent.tools.filesystem import EditFileTool, ListDirTool, ReadFileTool, WriteFileTool from nanobot.agent.tools.registry import ToolRegistry from nanobot.agent.tools.shell import ExecTool @@ -92,7 +93,8 @@ class SubagentManager: # Build subagent tools (no message tool, no spawn tool) tools = ToolRegistry() allowed_dir = self.workspace if self.restrict_to_workspace else None - tools.register(ReadFileTool(workspace=self.workspace, allowed_dir=allowed_dir)) + extra_read = [BUILTIN_SKILLS_DIR] if allowed_dir else None + tools.register(ReadFileTool(workspace=self.workspace, allowed_dir=allowed_dir, extra_allowed_dirs=extra_read)) tools.register(WriteFileTool(workspace=self.workspace, allowed_dir=allowed_dir)) tools.register(EditFileTool(workspace=self.workspace, allowed_dir=allowed_dir)) tools.register(ListDirTool(workspace=self.workspace, allowed_dir=allowed_dir)) @@ -213,7 +215,7 @@ Stay focused on the assigned task. Your final response will be reported back to skills_summary = SkillsLoader(self.workspace).build_skills_summary() if skills_summary: - parts.append(f"## Skills\n\nUse load_skill tool to load a skill by name.\n\n{skills_summary}") + parts.append(f"## Skills\n\nRead SKILL.md with read_file to use a skill.\n\n{skills_summary}") return "\n\n".join(parts) diff --git a/nanobot/agent/tools/filesystem.py b/nanobot/agent/tools/filesystem.py index 95bc980..6443f28 100644 --- a/nanobot/agent/tools/filesystem.py +++ b/nanobot/agent/tools/filesystem.py @@ -8,7 +8,10 @@ from nanobot.agent.tools.base import Tool def _resolve_path( - path: str, workspace: Path | None = None, allowed_dir: Path | None = None + path: str, + workspace: Path | None = None, + allowed_dir: Path | None = None, + extra_allowed_dirs: list[Path] | None = None, ) -> Path: """Resolve path against workspace (if relative) and enforce directory restriction.""" p = Path(path).expanduser() @@ -16,22 +19,35 @@ def _resolve_path( p = workspace / p resolved = p.resolve() if allowed_dir: - try: - resolved.relative_to(allowed_dir.resolve()) - except ValueError: + all_dirs = [allowed_dir] + (extra_allowed_dirs or []) + if not any(_is_under(resolved, d) for d in all_dirs): raise PermissionError(f"Path {path} is outside allowed directory {allowed_dir}") return resolved +def _is_under(path: Path, directory: Path) -> bool: + try: + path.relative_to(directory.resolve()) + return True + except ValueError: + return False + + class _FsTool(Tool): """Shared base for filesystem tools — common init and path resolution.""" - def __init__(self, workspace: Path | None = None, allowed_dir: Path | None = None): + def __init__( + self, + workspace: Path | None = None, + allowed_dir: Path | None = None, + extra_allowed_dirs: list[Path] | None = None, + ): self._workspace = workspace self._allowed_dir = allowed_dir + self._extra_allowed_dirs = extra_allowed_dirs def _resolve(self, path: str) -> Path: - return _resolve_path(path, self._workspace, self._allowed_dir) + return _resolve_path(path, self._workspace, self._allowed_dir, self._extra_allowed_dirs) # --------------------------------------------------------------------------- @@ -363,35 +379,3 @@ class ListDirTool(_FsTool): return f"Error: {e}" except Exception as e: return f"Error listing directory: {e}" - - -class LoadSkillTool(Tool): - """Tool to load a skill by name, bypassing workspace restriction.""" - - def __init__(self, skills_loader): - self._skills_loader = skills_loader - - @property - def name(self) -> str: - return "load_skill" - - @property - def description(self) -> str: - return "Load a skill by name. Returns the full SKILL.md content." - - @property - def parameters(self) -> dict[str, Any]: - return { - "type": "object", - "properties": {"name": {"type": "string", "description": "The skill name to load"}}, - "required": ["name"], - } - - async def execute(self, name: str, **kwargs: Any) -> str: - try: - content = self._skills_loader.load_skill(name) - if content is None: - return f"Error: Skill not found: {name}" - return content - except Exception as e: - return f"Error loading skill: {str(e)}" diff --git a/tests/test_filesystem_tools.py b/tests/test_filesystem_tools.py index 0f0ba78..620aa75 100644 --- a/tests/test_filesystem_tools.py +++ b/tests/test_filesystem_tools.py @@ -251,3 +251,114 @@ class TestListDirTool: result = await tool.execute(path=str(tmp_path / "nope")) assert "Error" in result assert "not found" in result + + +# --------------------------------------------------------------------------- +# Workspace restriction + extra_allowed_dirs +# --------------------------------------------------------------------------- + +class TestWorkspaceRestriction: + + @pytest.mark.asyncio + async def test_read_blocked_outside_workspace(self, tmp_path): + workspace = tmp_path / "ws" + workspace.mkdir() + outside = tmp_path / "outside" + outside.mkdir() + secret = outside / "secret.txt" + secret.write_text("top secret") + + tool = ReadFileTool(workspace=workspace, allowed_dir=workspace) + result = await tool.execute(path=str(secret)) + assert "Error" in result + assert "outside" in result.lower() + + @pytest.mark.asyncio + async def test_read_allowed_with_extra_dir(self, tmp_path): + workspace = tmp_path / "ws" + workspace.mkdir() + skills_dir = tmp_path / "skills" + skills_dir.mkdir() + skill_file = skills_dir / "test_skill" / "SKILL.md" + skill_file.parent.mkdir() + skill_file.write_text("# Test Skill\nDo something.") + + tool = ReadFileTool( + workspace=workspace, allowed_dir=workspace, + extra_allowed_dirs=[skills_dir], + ) + result = await tool.execute(path=str(skill_file)) + assert "Test Skill" in result + assert "Error" not in result + + @pytest.mark.asyncio + async def test_extra_dirs_does_not_widen_write(self, tmp_path): + from nanobot.agent.tools.filesystem import WriteFileTool + + workspace = tmp_path / "ws" + workspace.mkdir() + outside = tmp_path / "outside" + outside.mkdir() + + tool = WriteFileTool(workspace=workspace, allowed_dir=workspace) + result = await tool.execute(path=str(outside / "hack.txt"), content="pwned") + assert "Error" in result + assert "outside" in result.lower() + + @pytest.mark.asyncio + async def test_read_still_blocked_for_unrelated_dir(self, tmp_path): + workspace = tmp_path / "ws" + workspace.mkdir() + skills_dir = tmp_path / "skills" + skills_dir.mkdir() + unrelated = tmp_path / "other" + unrelated.mkdir() + secret = unrelated / "secret.txt" + secret.write_text("nope") + + tool = ReadFileTool( + workspace=workspace, allowed_dir=workspace, + extra_allowed_dirs=[skills_dir], + ) + result = await tool.execute(path=str(secret)) + assert "Error" in result + assert "outside" in result.lower() + + @pytest.mark.asyncio + async def test_workspace_file_still_readable_with_extra_dirs(self, tmp_path): + """Adding extra_allowed_dirs must not break normal workspace reads.""" + workspace = tmp_path / "ws" + workspace.mkdir() + ws_file = workspace / "README.md" + ws_file.write_text("hello from workspace") + skills_dir = tmp_path / "skills" + skills_dir.mkdir() + + tool = ReadFileTool( + workspace=workspace, allowed_dir=workspace, + extra_allowed_dirs=[skills_dir], + ) + result = await tool.execute(path=str(ws_file)) + assert "hello from workspace" in result + assert "Error" not in result + + @pytest.mark.asyncio + async def test_edit_blocked_in_extra_dir(self, tmp_path): + """edit_file must not be able to modify files in extra_allowed_dirs.""" + workspace = tmp_path / "ws" + workspace.mkdir() + skills_dir = tmp_path / "skills" + skills_dir.mkdir() + skill_file = skills_dir / "weather" / "SKILL.md" + skill_file.parent.mkdir() + skill_file.write_text("# Weather\nOriginal content.") + + tool = EditFileTool(workspace=workspace, allowed_dir=workspace) + result = await tool.execute( + path=str(skill_file), + old_text="Original content.", + new_text="Hacked content.", + ) + assert "Error" in result + assert "outside" in result.lower() + assert skill_file.read_text() == "# Weather\nOriginal content."