From aa202d5cbe10bcf7d56d89bfc623b64494c5cdf7 Mon Sep 17 00:00:00 2001 From: johnlin Date: Sat, 4 Apr 2026 13:29:23 +0800 Subject: [PATCH 1/7] feat: add local shell skill support via ShellTool Agents can now load local skill directories (e.g. ~/.claude/skills/) via the shellSkills config key. Each skill's description is auto-read from its SKILL.md frontmatter. A 30s-timeout async subprocess executor handles command execution, respecting per-request timeout_ms when provided. Co-Authored-By: Claude Sonnet 4.6 --- bot/agents.py | 82 +++++++++++++++++++++- servers_config.example.json | 16 ++++- tests/test_agents.py | 131 ++++++++++++++++++++++++++++++++++++ 3 files changed, 226 insertions(+), 3 deletions(-) diff --git a/bot/agents.py b/bot/agents.py index eb39823..a8beece 100644 --- a/bot/agents.py +++ b/bot/agents.py @@ -8,6 +8,8 @@ from agents import Agent from agents import Runner +from agents import ShellTool +from agents import ShellToolLocalSkill from agents import TResponseInputItem from agents.mcp import MCPServerStdio from agents.mcp import MCPServerStreamableHttp @@ -20,6 +22,7 @@ MAX_TURNS = 10 MCP_SESSION_TIMEOUT_SECONDS = 30.0 +SHELL_TIMEOUT = 30.0 set_tracing_disabled(True) @@ -58,20 +61,79 @@ def _get_model() -> OpenAIResponsesModel | OpenAIChatCompletionsModel: return OpenAIResponsesModel(model=model_name, openai_client=client) +def _read_skill_description(skill_dir: Path) -> str: + """Read the description field from a skill's SKILL.md frontmatter. + + Returns an empty string if the file is missing or has no description. + """ + skill_md = skill_dir / "SKILL.md" + try: + content = skill_md.read_text(encoding="utf-8") + except FileNotFoundError: + logging.warning("SKILL.md not found in %s", skill_dir) + return "" + + # Parse YAML frontmatter between --- delimiters + if not content.startswith("---"): + return "" + end = content.find("\n---", 3) + if end == -1: + return "" + frontmatter = content[3:end] + for line in frontmatter.splitlines(): + if line.startswith("description:"): + return line[len("description:") :].strip() + return "" + + +async def _shell_executor(request: Any) -> str: + """Execute a shell command from a LocalShellCommandRequest. + + Uses timeout_ms from the request if provided, otherwise falls back + to SHELL_TIMEOUT seconds. + """ + action = request.data.action + command: list[str] = action.command + env: dict[str, str] = action.env or {} + cwd: str | None = action.working_directory + timeout_ms: int | None = action.timeout_ms + + timeout = (timeout_ms / 1000.0) if timeout_ms is not None else SHELL_TIMEOUT + + merged_env = {**os.environ, **env} + + proc = await asyncio.create_subprocess_exec( + *command, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.STDOUT, + env=merged_env, + cwd=cwd, + ) + try: + stdout, _ = await asyncio.wait_for(proc.communicate(), timeout=timeout) + return stdout.decode("utf-8", errors="replace") + except TimeoutError: + proc.kill() + await proc.communicate() + return f"Command timed out after {timeout}s" + + class OpenAIAgent: - """A wrapper for OpenAI Agent with MCP server support.""" + """A wrapper for OpenAI Agent with MCP server and local shell skill support.""" def __init__( self, name: str, instructions: str, mcp_servers: list | None = None, + tools: list | None = None, ) -> None: self.agent = Agent( name=name, instructions=instructions, model=_get_model(), mcp_servers=(mcp_servers if mcp_servers is not None else []), + tools=(tools if tools is not None else []), ) self.name = name self._conversations: dict[int, list[TResponseInputItem]] = {} @@ -127,8 +189,24 @@ def from_dict(cls, name: str, config: dict[str, Any]) -> OpenAIAgent: }, ) ) + tools: list[Any] = [] + skill_configs = config.get("shellSkills", []) + if skill_configs: + skills: list[ShellToolLocalSkill] = [] + for skill_cfg in skill_configs: + skill_path = Path(os.path.expanduser(skill_cfg["path"])) + description = _read_skill_description(skill_path) + skills.append( + ShellToolLocalSkill( + name=skill_cfg["name"], + description=description, + path=str(skill_path), + ) + ) + tools.append(ShellTool(executor=_shell_executor, environment={"type": "local", "skills": skills})) + instructions = _load_instructions() - return cls(name, instructions=instructions, mcp_servers=mcp_servers) + return cls(name, instructions=instructions, mcp_servers=mcp_servers, tools=tools) async def connect(self) -> None: for mcp_server in self.agent.mcp_servers: diff --git a/servers_config.example.json b/servers_config.example.json index 7873ad3..1d00b5b 100644 --- a/servers_config.example.json +++ b/servers_config.example.json @@ -4,5 +4,19 @@ "command": "uvx", "args": ["yfmcp"] } - } + }, + "shellSkills": [ + { + "name": "obsidian-cli", + "path": "~/.claude/skills/obsidian-cli" + }, + { + "name": "obsidian-markdown", + "path": "~/.claude/skills/obsidian-markdown" + }, + { + "name": "obsidian-bases", + "path": "~/.claude/skills/obsidian-bases" + } + ] } diff --git a/tests/test_agents.py b/tests/test_agents.py index 9e8dead..c2fc108 100644 --- a/tests/test_agents.py +++ b/tests/test_agents.py @@ -5,6 +5,7 @@ from unittest.mock import patch import pytest +from agents import ShellTool from agents.mcp import MCPServerStdio from agents.mcp import MCPServerStreamableHttp from agents.models.interface import Model @@ -12,8 +13,10 @@ from agents.models.openai_responses import OpenAIResponsesModel from bot.agents import MAX_TURNS +from bot.agents import SHELL_TIMEOUT from bot.agents import OpenAIAgent from bot.agents import _get_model +from bot.agents import _shell_executor @pytest.fixture @@ -231,3 +234,131 @@ def test_no_truncation_when_under_limit(self): msgs = agent.get_messages(chat_id=100) user_msgs = [m for m in msgs if m["role"] == "user"] assert len(user_msgs) == 3 + + +def _make_shell_request( + command: list[str], + env: dict | None = None, + cwd: str | None = None, + timeout_ms: int | None = None, +): + """Build a mock LocalShellCommandRequest.""" + action = MagicMock() + action.command = command + action.env = env or {} + action.working_directory = cwd + action.timeout_ms = timeout_ms + request = MagicMock() + request.data.action = action + return request + + +class TestShellExecutor: + def test_constant_is_30_seconds(self): + assert SHELL_TIMEOUT == 30.0 + + @pytest.mark.anyio + async def test_returns_stdout(self): + request = _make_shell_request(["echo", "hello"]) + result = await _shell_executor(request) + assert "hello" in result + + @pytest.mark.anyio + async def test_stderr_merged_into_output(self): + request = _make_shell_request(["bash", "-c", "echo err >&2"]) + result = await _shell_executor(request) + assert "err" in result + + @pytest.mark.anyio + async def test_uses_working_directory(self): + request = _make_shell_request(["pwd"], cwd="/tmp") + result = await _shell_executor(request) + assert "/tmp" in result + + @pytest.mark.anyio + async def test_times_out_and_returns_message(self, monkeypatch): + monkeypatch.setattr("bot.agents.SHELL_TIMEOUT", 0.05) + request = _make_shell_request(["sleep", "10"]) + result = await _shell_executor(request) + assert "timed out" in result.lower() + + @pytest.mark.anyio + async def test_timeout_ms_from_request_overrides_default(self, monkeypatch): + monkeypatch.setattr("bot.agents.SHELL_TIMEOUT", 30.0) + # 50ms timeout from request — sleep 10s should time out + request = _make_shell_request(["sleep", "10"], timeout_ms=50) + result = await _shell_executor(request) + assert "timed out" in result.lower() + + +class TestFromDictShellSkills: + def test_no_shell_skills_when_config_missing(self): + agent = OpenAIAgent.from_dict("test", {"mcpServers": {}}) + shell_tools = [t for t in agent.agent.tools if isinstance(t, ShellTool)] + assert len(shell_tools) == 0 + + def test_shell_tool_added_when_skills_configured(self, tmp_path): + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text("---\nname: my-skill\ndescription: A test skill\n---\n") + + config = { + "mcpServers": {}, + "shellSkills": [{"name": "my-skill", "path": str(skill_dir)}], + } + agent = OpenAIAgent.from_dict("test", config) + shell_tools = [t for t in agent.agent.tools if isinstance(t, ShellTool)] + assert len(shell_tools) == 1 + + def test_skill_description_read_from_skill_md(self, tmp_path): + skill_dir = tmp_path / "obs" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text("---\nname: obs\ndescription: Interact with Obsidian\n---\n# Content\n") + config = { + "mcpServers": {}, + "shellSkills": [{"name": "obs", "path": str(skill_dir)}], + } + agent = OpenAIAgent.from_dict("test", config) + shell_tool = next(t for t in agent.agent.tools if isinstance(t, ShellTool)) + skill = shell_tool.environment["skills"][0] + assert skill["description"] == "Interact with Obsidian" + + def test_tilde_expanded_in_path(self, tmp_path, monkeypatch): + skill_dir = tmp_path / "skill" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text("---\nname: s\ndescription: d\n---\n") + monkeypatch.setenv("HOME", str(tmp_path)) + + config = { + "mcpServers": {}, + "shellSkills": [{"name": "s", "path": "~/skill"}], + } + agent = OpenAIAgent.from_dict("test", config) + shell_tool = next(t for t in agent.agent.tools if isinstance(t, ShellTool)) + assert shell_tool.environment["skills"][0]["path"] == str(skill_dir) + + def test_multiple_skills_all_mounted(self, tmp_path): + skills = [] + for name in ["skill-a", "skill-b"]: + d = tmp_path / name + d.mkdir() + (d / "SKILL.md").write_text(f"---\nname: {name}\ndescription: desc\n---\n") + skills.append({"name": name, "path": str(d)}) + + agent = OpenAIAgent.from_dict("test", {"mcpServers": {}, "shellSkills": skills}) + shell_tool = next(t for t in agent.agent.tools if isinstance(t, ShellTool)) + assert len(shell_tool.environment["skills"]) == 2 + + def test_mcp_servers_and_shell_skills_coexist(self, tmp_path): + skill_dir = tmp_path / "s" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text("---\nname: s\ndescription: d\n---\n") + + config = { + "mcpServers": {"my-mcp": {"command": "uvx", "args": ["something"]}}, + "shellSkills": [{"name": "s", "path": str(skill_dir)}], + } + agent = OpenAIAgent.from_dict("test", config) + assert len(agent.agent.mcp_servers) == 1 + shell_tools = [t for t in agent.agent.tools if isinstance(t, ShellTool)] + assert len(shell_tools) == 1 From eb106a085ff3a5b6a6b18a862cdf19894cfb1910 Mon Sep 17 00:00:00 2001 From: johnlin Date: Mon, 6 Apr 2026 14:37:33 +0800 Subject: [PATCH 2/7] refactor: auto-discover local shell skills from ./skills/ - Skills are now loaded from /skills/*/SKILL.md at agent init instead of being listed in servers_config.json. - Skill name is taken from the directory name; description is parsed from the SKILL.md YAML frontmatter. - Rewrote _shell_executor to match the SDK ShellCommandRequest API: iterate action.commands and use create_subprocess_shell. - Removed MagicMock-based executor tests; kept config-to-tool mapping tests against a monkeypatched SKILLS_DIR. - Added skills/.gitignore so users can drop their own skills into the directory without polluting the repo. --- bot/agents.py | 106 +++++++++++++-------------- servers_config.example.json | 16 +---- skills/.gitignore | 4 ++ tests/test_agents.py | 138 +++++++++--------------------------- 4 files changed, 90 insertions(+), 174 deletions(-) create mode 100644 skills/.gitignore diff --git a/bot/agents.py b/bot/agents.py index a8beece..b8413db 100644 --- a/bot/agents.py +++ b/bot/agents.py @@ -9,7 +9,6 @@ from agents import Agent from agents import Runner from agents import ShellTool -from agents import ShellToolLocalSkill from agents import TResponseInputItem from agents.mcp import MCPServerStdio from agents.mcp import MCPServerStreamableHttp @@ -23,6 +22,7 @@ MAX_TURNS = 10 MCP_SESSION_TIMEOUT_SECONDS = 30.0 SHELL_TIMEOUT = 30.0 +SKILLS_DIR = Path(__file__).resolve().parent.parent / "skills" set_tracing_disabled(True) @@ -61,61 +61,68 @@ def _get_model() -> OpenAIResponsesModel | OpenAIChatCompletionsModel: return OpenAIResponsesModel(model=model_name, openai_client=client) -def _read_skill_description(skill_dir: Path) -> str: - """Read the description field from a skill's SKILL.md frontmatter. - - Returns an empty string if the file is missing or has no description. - """ - skill_md = skill_dir / "SKILL.md" - try: - content = skill_md.read_text(encoding="utf-8") - except FileNotFoundError: - logging.warning("SKILL.md not found in %s", skill_dir) - return "" - - # Parse YAML frontmatter between --- delimiters +def _parse_skill_description(content: str) -> str: + """Return the description field from a SKILL.md YAML frontmatter, or "".""" if not content.startswith("---"): return "" end = content.find("\n---", 3) if end == -1: return "" - frontmatter = content[3:end] - for line in frontmatter.splitlines(): + for line in content[3:end].splitlines(): if line.startswith("description:"): return line[len("description:") :].strip() return "" +def _load_shell_skills() -> list[dict[str, str]]: + """Discover local shell skills under SKILLS_DIR. + + Each immediate subdirectory of SKILLS_DIR containing a SKILL.md is mounted + as a ShellToolLocalSkill. The skill name is the directory name; the + description is read from the SKILL.md YAML frontmatter. + """ + if not SKILLS_DIR.is_dir(): + return [] + skills: list[dict[str, str]] = [] + for skill_dir in sorted(SKILLS_DIR.iterdir()): + skill_md = skill_dir / "SKILL.md" + if not skill_dir.is_dir() or not skill_md.is_file(): + continue + skills.append( + { + "name": skill_dir.name, + "description": _parse_skill_description(skill_md.read_text(encoding="utf-8")), + "path": str(skill_dir), + } + ) + return skills + + async def _shell_executor(request: Any) -> str: - """Execute a shell command from a LocalShellCommandRequest. + """Run each shell command from the request and return combined output. - Uses timeout_ms from the request if provided, otherwise falls back - to SHELL_TIMEOUT seconds. + Honours action.timeout_ms when set, otherwise falls back to SHELL_TIMEOUT. + stderr is merged into stdout for simplicity. """ action = request.data.action - command: list[str] = action.command - env: dict[str, str] = action.env or {} - cwd: str | None = action.working_directory - timeout_ms: int | None = action.timeout_ms - - timeout = (timeout_ms / 1000.0) if timeout_ms is not None else SHELL_TIMEOUT - - merged_env = {**os.environ, **env} - - proc = await asyncio.create_subprocess_exec( - *command, - stdout=asyncio.subprocess.PIPE, - stderr=asyncio.subprocess.STDOUT, - env=merged_env, - cwd=cwd, - ) - try: - stdout, _ = await asyncio.wait_for(proc.communicate(), timeout=timeout) - return stdout.decode("utf-8", errors="replace") - except TimeoutError: - proc.kill() - await proc.communicate() - return f"Command timed out after {timeout}s" + timeout = (action.timeout_ms / 1000.0) if action.timeout_ms else SHELL_TIMEOUT + + outputs: list[str] = [] + for command in action.commands: + proc = await asyncio.create_subprocess_shell( + command, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.STDOUT, + ) + try: + stdout, _ = await asyncio.wait_for(proc.communicate(), timeout=timeout) + outputs.append(stdout.decode("utf-8", errors="replace")) + except TimeoutError: + proc.kill() + await proc.communicate() + outputs.append(f"Command timed out after {timeout}s: {command}") + break + return "\n".join(outputs) class OpenAIAgent: @@ -190,19 +197,8 @@ def from_dict(cls, name: str, config: dict[str, Any]) -> OpenAIAgent: ) ) tools: list[Any] = [] - skill_configs = config.get("shellSkills", []) - if skill_configs: - skills: list[ShellToolLocalSkill] = [] - for skill_cfg in skill_configs: - skill_path = Path(os.path.expanduser(skill_cfg["path"])) - description = _read_skill_description(skill_path) - skills.append( - ShellToolLocalSkill( - name=skill_cfg["name"], - description=description, - path=str(skill_path), - ) - ) + skills = _load_shell_skills() + if skills: tools.append(ShellTool(executor=_shell_executor, environment={"type": "local", "skills": skills})) instructions = _load_instructions() diff --git a/servers_config.example.json b/servers_config.example.json index 1d00b5b..7873ad3 100644 --- a/servers_config.example.json +++ b/servers_config.example.json @@ -4,19 +4,5 @@ "command": "uvx", "args": ["yfmcp"] } - }, - "shellSkills": [ - { - "name": "obsidian-cli", - "path": "~/.claude/skills/obsidian-cli" - }, - { - "name": "obsidian-markdown", - "path": "~/.claude/skills/obsidian-markdown" - }, - { - "name": "obsidian-bases", - "path": "~/.claude/skills/obsidian-bases" - } - ] + } } diff --git a/skills/.gitignore b/skills/.gitignore new file mode 100644 index 0000000..1425c6f --- /dev/null +++ b/skills/.gitignore @@ -0,0 +1,4 @@ +# Users drop their own shell skills into this directory; the bot auto-loads +# any subdirectory containing a SKILL.md. Skill content is not tracked. +* +!.gitignore diff --git a/tests/test_agents.py b/tests/test_agents.py index c2fc108..08a0abd 100644 --- a/tests/test_agents.py +++ b/tests/test_agents.py @@ -13,10 +13,8 @@ from agents.models.openai_responses import OpenAIResponsesModel from bot.agents import MAX_TURNS -from bot.agents import SHELL_TIMEOUT from bot.agents import OpenAIAgent from bot.agents import _get_model -from bot.agents import _shell_executor @pytest.fixture @@ -26,9 +24,12 @@ def _stub_instructions(monkeypatch): @pytest.fixture(autouse=True) -def _mock_model(monkeypatch): - """Prevent tests from constructing a real OpenAI client.""" +def _mock_model(monkeypatch, tmp_path_factory): + """Prevent tests from constructing a real OpenAI client and isolate skills.""" monkeypatch.setattr("bot.agents._get_model", lambda: create_autospec(Model)) + # Point SKILLS_DIR at an empty directory so tests do not auto-load the + # real ./skills/ on disk. Tests that need skills can override this. + monkeypatch.setattr("bot.agents.SKILLS_DIR", tmp_path_factory.mktemp("empty_skills")) class TestGetModel: @@ -236,128 +237,57 @@ def test_no_truncation_when_under_limit(self): assert len(user_msgs) == 3 -def _make_shell_request( - command: list[str], - env: dict | None = None, - cwd: str | None = None, - timeout_ms: int | None = None, -): - """Build a mock LocalShellCommandRequest.""" - action = MagicMock() - action.command = command - action.env = env or {} - action.working_directory = cwd - action.timeout_ms = timeout_ms - request = MagicMock() - request.data.action = action - return request - - -class TestShellExecutor: - def test_constant_is_30_seconds(self): - assert SHELL_TIMEOUT == 30.0 - - @pytest.mark.anyio - async def test_returns_stdout(self): - request = _make_shell_request(["echo", "hello"]) - result = await _shell_executor(request) - assert "hello" in result - - @pytest.mark.anyio - async def test_stderr_merged_into_output(self): - request = _make_shell_request(["bash", "-c", "echo err >&2"]) - result = await _shell_executor(request) - assert "err" in result - - @pytest.mark.anyio - async def test_uses_working_directory(self): - request = _make_shell_request(["pwd"], cwd="/tmp") - result = await _shell_executor(request) - assert "/tmp" in result - - @pytest.mark.anyio - async def test_times_out_and_returns_message(self, monkeypatch): - monkeypatch.setattr("bot.agents.SHELL_TIMEOUT", 0.05) - request = _make_shell_request(["sleep", "10"]) - result = await _shell_executor(request) - assert "timed out" in result.lower() - - @pytest.mark.anyio - async def test_timeout_ms_from_request_overrides_default(self, monkeypatch): - monkeypatch.setattr("bot.agents.SHELL_TIMEOUT", 30.0) - # 50ms timeout from request — sleep 10s should time out - request = _make_shell_request(["sleep", "10"], timeout_ms=50) - result = await _shell_executor(request) - assert "timed out" in result.lower() - - -class TestFromDictShellSkills: - def test_no_shell_skills_when_config_missing(self): +class TestLoadShellSkills: + def test_no_shell_tool_when_skills_dir_missing(self, tmp_path, monkeypatch): + monkeypatch.setattr("bot.agents.SKILLS_DIR", tmp_path / "nonexistent") agent = OpenAIAgent.from_dict("test", {"mcpServers": {}}) shell_tools = [t for t in agent.agent.tools if isinstance(t, ShellTool)] assert len(shell_tools) == 0 - def test_shell_tool_added_when_skills_configured(self, tmp_path): + def test_shell_tool_added_when_skill_found(self, tmp_path, monkeypatch): skill_dir = tmp_path / "my-skill" skill_dir.mkdir() (skill_dir / "SKILL.md").write_text("---\nname: my-skill\ndescription: A test skill\n---\n") + monkeypatch.setattr("bot.agents.SKILLS_DIR", tmp_path) - config = { - "mcpServers": {}, - "shellSkills": [{"name": "my-skill", "path": str(skill_dir)}], - } - agent = OpenAIAgent.from_dict("test", config) - shell_tools = [t for t in agent.agent.tools if isinstance(t, ShellTool)] - assert len(shell_tools) == 1 - - def test_skill_description_read_from_skill_md(self, tmp_path): - skill_dir = tmp_path / "obs" - skill_dir.mkdir() - (skill_dir / "SKILL.md").write_text("---\nname: obs\ndescription: Interact with Obsidian\n---\n# Content\n") - config = { - "mcpServers": {}, - "shellSkills": [{"name": "obs", "path": str(skill_dir)}], - } - agent = OpenAIAgent.from_dict("test", config) + agent = OpenAIAgent.from_dict("test", {"mcpServers": {}}) shell_tool = next(t for t in agent.agent.tools if isinstance(t, ShellTool)) skill = shell_tool.environment["skills"][0] - assert skill["description"] == "Interact with Obsidian" - - def test_tilde_expanded_in_path(self, tmp_path, monkeypatch): - skill_dir = tmp_path / "skill" - skill_dir.mkdir() - (skill_dir / "SKILL.md").write_text("---\nname: s\ndescription: d\n---\n") - monkeypatch.setenv("HOME", str(tmp_path)) + assert skill["name"] == "my-skill" + assert skill["description"] == "A test skill" + assert skill["path"] == str(skill_dir) - config = { - "mcpServers": {}, - "shellSkills": [{"name": "s", "path": "~/skill"}], - } - agent = OpenAIAgent.from_dict("test", config) - shell_tool = next(t for t in agent.agent.tools if isinstance(t, ShellTool)) - assert shell_tool.environment["skills"][0]["path"] == str(skill_dir) - - def test_multiple_skills_all_mounted(self, tmp_path): - skills = [] + def test_multiple_skills_all_mounted(self, tmp_path, monkeypatch): for name in ["skill-a", "skill-b"]: d = tmp_path / name d.mkdir() - (d / "SKILL.md").write_text(f"---\nname: {name}\ndescription: desc\n---\n") - skills.append({"name": name, "path": str(d)}) + (d / "SKILL.md").write_text(f"---\nname: {name}\ndescription: desc {name}\n---\n") + monkeypatch.setattr("bot.agents.SKILLS_DIR", tmp_path) - agent = OpenAIAgent.from_dict("test", {"mcpServers": {}, "shellSkills": skills}) + agent = OpenAIAgent.from_dict("test", {"mcpServers": {}}) shell_tool = next(t for t in agent.agent.tools if isinstance(t, ShellTool)) assert len(shell_tool.environment["skills"]) == 2 - def test_mcp_servers_and_shell_skills_coexist(self, tmp_path): + def test_directory_without_skill_md_is_skipped(self, tmp_path, monkeypatch): + (tmp_path / "not-a-skill").mkdir() + good = tmp_path / "real-skill" + good.mkdir() + (good / "SKILL.md").write_text("---\nname: real-skill\ndescription: d\n---\n") + monkeypatch.setattr("bot.agents.SKILLS_DIR", tmp_path) + + agent = OpenAIAgent.from_dict("test", {"mcpServers": {}}) + shell_tool = next(t for t in agent.agent.tools if isinstance(t, ShellTool)) + skills = shell_tool.environment["skills"] + assert len(skills) == 1 + assert skills[0]["name"] == "real-skill" + + def test_mcp_servers_and_shell_skills_coexist(self, tmp_path, monkeypatch): skill_dir = tmp_path / "s" skill_dir.mkdir() (skill_dir / "SKILL.md").write_text("---\nname: s\ndescription: d\n---\n") + monkeypatch.setattr("bot.agents.SKILLS_DIR", tmp_path) - config = { - "mcpServers": {"my-mcp": {"command": "uvx", "args": ["something"]}}, - "shellSkills": [{"name": "s", "path": str(skill_dir)}], - } + config = {"mcpServers": {"my-mcp": {"command": "uvx", "args": ["something"]}}} agent = OpenAIAgent.from_dict("test", config) assert len(agent.agent.mcp_servers) == 1 shell_tools = [t for t in agent.agent.tools if isinstance(t, ShellTool)] From 77f374f63e0fee258ddba32147d988c628e87108 Mon Sep 17 00:00:00 2001 From: johnlin Date: Mon, 6 Apr 2026 14:53:22 +0800 Subject: [PATCH 3/7] fix: type ShellTool environment with SDK TypedDicts mypy could not narrow the dict literal {"type": "local", "skills": ...} to ShellToolLocalEnvironment; use the TypedDict constructors from the agents SDK so the types are explicit. --- bot/agents.py | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/bot/agents.py b/bot/agents.py index b8413db..5a251df 100644 --- a/bot/agents.py +++ b/bot/agents.py @@ -9,6 +9,8 @@ from agents import Agent from agents import Runner from agents import ShellTool +from agents import ShellToolLocalEnvironment +from agents import ShellToolLocalSkill from agents import TResponseInputItem from agents.mcp import MCPServerStdio from agents.mcp import MCPServerStreamableHttp @@ -74,7 +76,7 @@ def _parse_skill_description(content: str) -> str: return "" -def _load_shell_skills() -> list[dict[str, str]]: +def _load_shell_skills() -> list[ShellToolLocalSkill]: """Discover local shell skills under SKILLS_DIR. Each immediate subdirectory of SKILLS_DIR containing a SKILL.md is mounted @@ -83,17 +85,17 @@ def _load_shell_skills() -> list[dict[str, str]]: """ if not SKILLS_DIR.is_dir(): return [] - skills: list[dict[str, str]] = [] + skills: list[ShellToolLocalSkill] = [] for skill_dir in sorted(SKILLS_DIR.iterdir()): skill_md = skill_dir / "SKILL.md" if not skill_dir.is_dir() or not skill_md.is_file(): continue skills.append( - { - "name": skill_dir.name, - "description": _parse_skill_description(skill_md.read_text(encoding="utf-8")), - "path": str(skill_dir), - } + ShellToolLocalSkill( + name=skill_dir.name, + description=_parse_skill_description(skill_md.read_text(encoding="utf-8")), + path=str(skill_dir), + ) ) return skills @@ -199,7 +201,8 @@ def from_dict(cls, name: str, config: dict[str, Any]) -> OpenAIAgent: tools: list[Any] = [] skills = _load_shell_skills() if skills: - tools.append(ShellTool(executor=_shell_executor, environment={"type": "local", "skills": skills})) + environment = ShellToolLocalEnvironment(type="local", skills=skills) + tools.append(ShellTool(executor=_shell_executor, environment=environment)) instructions = _load_instructions() return cls(name, instructions=instructions, mcp_servers=mcp_servers, tools=tools) From cdbbf12689b43a6b78daf43e77be480ae32d2abd Mon Sep 17 00:00:00 2001 From: johnlin Date: Mon, 6 Apr 2026 21:59:14 +0800 Subject: [PATCH 4/7] fix: skip unreadable skill files during discovery --- bot/agents.py | 7 ++++++- tests/test_agents.py | 44 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/bot/agents.py b/bot/agents.py index 5a251df..4cbf58d 100644 --- a/bot/agents.py +++ b/bot/agents.py @@ -90,10 +90,15 @@ def _load_shell_skills() -> list[ShellToolLocalSkill]: skill_md = skill_dir / "SKILL.md" if not skill_dir.is_dir() or not skill_md.is_file(): continue + try: + content = skill_md.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + logging.warning("Skipping unreadable skill file: %s", skill_md, exc_info=True) + continue skills.append( ShellToolLocalSkill( name=skill_dir.name, - description=_parse_skill_description(skill_md.read_text(encoding="utf-8")), + description=_parse_skill_description(content), path=str(skill_dir), ) ) diff --git a/tests/test_agents.py b/tests/test_agents.py index 08a0abd..ebc35de 100644 --- a/tests/test_agents.py +++ b/tests/test_agents.py @@ -1,5 +1,6 @@ from __future__ import annotations +from pathlib import Path from unittest.mock import MagicMock from unittest.mock import create_autospec from unittest.mock import patch @@ -292,3 +293,46 @@ def test_mcp_servers_and_shell_skills_coexist(self, tmp_path, monkeypatch): assert len(agent.agent.mcp_servers) == 1 shell_tools = [t for t in agent.agent.tools if isinstance(t, ShellTool)] assert len(shell_tools) == 1 + + def test_unreadable_utf8_skill_file_is_skipped(self, tmp_path, monkeypatch): + bad = tmp_path / "bad-skill" + bad.mkdir() + (bad / "SKILL.md").write_bytes(b"\xff\xfe\x00\x00") + + good = tmp_path / "good-skill" + good.mkdir() + (good / "SKILL.md").write_text("---\nname: good-skill\ndescription: good\n---\n") + + monkeypatch.setattr("bot.agents.SKILLS_DIR", tmp_path) + + agent = OpenAIAgent.from_dict("test", {"mcpServers": {}}) + shell_tool = next(t for t in agent.agent.tools if isinstance(t, ShellTool)) + skills = shell_tool.environment["skills"] + assert len(skills) == 1 + assert skills[0]["name"] == "good-skill" + + def test_oserror_reading_skill_file_is_skipped(self, tmp_path, monkeypatch): + bad = tmp_path / "bad-skill" + bad.mkdir() + bad_file = bad / "SKILL.md" + bad_file.write_text("---\nname: bad\ndescription: bad\n---\n") + + good = tmp_path / "good-skill" + good.mkdir() + (good / "SKILL.md").write_text("---\nname: good-skill\ndescription: good\n---\n") + + original_read_text = Path.read_text + + def _read_text(self: Path, *args, **kwargs): + if self == bad_file: + raise OSError("permission denied") + return original_read_text(self, *args, **kwargs) + + monkeypatch.setattr("bot.agents.SKILLS_DIR", tmp_path) + monkeypatch.setattr(Path, "read_text", _read_text) + + agent = OpenAIAgent.from_dict("test", {"mcpServers": {}}) + shell_tool = next(t for t in agent.agent.tools if isinstance(t, ShellTool)) + skills = shell_tool.environment["skills"] + assert len(skills) == 1 + assert skills[0]["name"] == "good-skill" From bdcf618ccbe0e924cffd55109d5c43e7ec09230e Mon Sep 17 00:00:00 2001 From: johnlin Date: Mon, 6 Apr 2026 22:14:20 +0800 Subject: [PATCH 5/7] test: stub instructions in shell-skill from_dict tests --- tests/test_agents.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_agents.py b/tests/test_agents.py index ebc35de..75cce3b 100644 --- a/tests/test_agents.py +++ b/tests/test_agents.py @@ -238,6 +238,7 @@ def test_no_truncation_when_under_limit(self): assert len(user_msgs) == 3 +@pytest.mark.usefixtures("_stub_instructions") class TestLoadShellSkills: def test_no_shell_tool_when_skills_dir_missing(self, tmp_path, monkeypatch): monkeypatch.setattr("bot.agents.SKILLS_DIR", tmp_path / "nonexistent") From 5d2ea42d8fe7a3b5236bfa98d9a6e8ea79f1a35b Mon Sep 17 00:00:00 2001 From: johnlin Date: Mon, 6 Apr 2026 23:50:00 +0800 Subject: [PATCH 6/7] fix: harden shell executor and skill description parser - Strip matched quotes from YAML frontmatter description values - Use ShellCommandRequest type instead of Any for executor parameter - Use 'is not None' for timeout_ms check to distinguish 0 from None - Catch OSError from create_subprocess_shell and return error message - Append [exit code: N] to output for non-zero exit codes - Add direct unit tests for _parse_skill_description and _shell_executor --- bot/agents.py | 29 +++++++---- tests/test_agents.py | 115 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 135 insertions(+), 9 deletions(-) diff --git a/bot/agents.py b/bot/agents.py index 4cbf58d..c338145 100644 --- a/bot/agents.py +++ b/bot/agents.py @@ -8,6 +8,7 @@ from agents import Agent from agents import Runner +from agents import ShellCommandRequest from agents import ShellTool from agents import ShellToolLocalEnvironment from agents import ShellToolLocalSkill @@ -72,7 +73,10 @@ def _parse_skill_description(content: str) -> str: return "" for line in content[3:end].splitlines(): if line.startswith("description:"): - return line[len("description:") :].strip() + value = line[len("description:") :].strip() + if len(value) >= 2 and value[0] == value[-1] and value[0] in ('"', "'"): + value = value[1:-1] + return value return "" @@ -105,25 +109,32 @@ def _load_shell_skills() -> list[ShellToolLocalSkill]: return skills -async def _shell_executor(request: Any) -> str: +async def _shell_executor(request: ShellCommandRequest) -> str: """Run each shell command from the request and return combined output. Honours action.timeout_ms when set, otherwise falls back to SHELL_TIMEOUT. stderr is merged into stdout for simplicity. """ action = request.data.action - timeout = (action.timeout_ms / 1000.0) if action.timeout_ms else SHELL_TIMEOUT + timeout = (action.timeout_ms / 1000.0) if action.timeout_ms is not None else SHELL_TIMEOUT outputs: list[str] = [] for command in action.commands: - proc = await asyncio.create_subprocess_shell( - command, - stdout=asyncio.subprocess.PIPE, - stderr=asyncio.subprocess.STDOUT, - ) + try: + proc = await asyncio.create_subprocess_shell( + command, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.STDOUT, + ) + except OSError as e: + outputs.append(f"Failed to run command: {command}: {e}") + break try: stdout, _ = await asyncio.wait_for(proc.communicate(), timeout=timeout) - outputs.append(stdout.decode("utf-8", errors="replace")) + output = stdout.decode("utf-8", errors="replace") + if proc.returncode: + output += f"\n[exit code: {proc.returncode}]" + outputs.append(output) except TimeoutError: proc.kill() await proc.communicate() diff --git a/tests/test_agents.py b/tests/test_agents.py index 75cce3b..9c104a4 100644 --- a/tests/test_agents.py +++ b/tests/test_agents.py @@ -16,6 +16,8 @@ from bot.agents import MAX_TURNS from bot.agents import OpenAIAgent from bot.agents import _get_model +from bot.agents import _parse_skill_description +from bot.agents import _shell_executor @pytest.fixture @@ -337,3 +339,116 @@ def _read_text(self: Path, *args, **kwargs): skills = shell_tool.environment["skills"] assert len(skills) == 1 assert skills[0]["name"] == "good-skill" + + +class TestParseSkillDescription: + def test_unquoted_description(self): + content = "---\nname: my-skill\ndescription: A test skill\n---\nBody text." + assert _parse_skill_description(content) == "A test skill" + + def test_double_quoted_description(self): + content = '---\nname: my-skill\ndescription: "A quoted skill"\n---\n' + assert _parse_skill_description(content) == "A quoted skill" + + def test_single_quoted_description(self): + content = "---\nname: my-skill\ndescription: 'Single quoted'\n---\n" + assert _parse_skill_description(content) == "Single quoted" + + def test_no_frontmatter(self): + content = "Just a plain markdown file." + assert _parse_skill_description(content) == "" + + def test_no_description_field(self): + content = "---\nname: my-skill\n---\nBody." + assert _parse_skill_description(content) == "" + + def test_unclosed_frontmatter(self): + content = "---\nname: my-skill\ndescription: never closed" + assert _parse_skill_description(content) == "" + + def test_empty_description(self): + content = "---\nname: my-skill\ndescription:\n---\n" + assert _parse_skill_description(content) == "" + + def test_description_with_colon_in_value(self): + content = "---\ndescription: Run this: do stuff\n---\n" + assert _parse_skill_description(content) == "Run this: do stuff" + + +def _make_shell_request(commands: list[str], timeout_ms: int | None = None): + """Build a minimal object matching the ShellCommandRequest interface.""" + from types import SimpleNamespace + + action = SimpleNamespace(commands=commands, timeout_ms=timeout_ms) + data = SimpleNamespace(action=action) + return SimpleNamespace(data=data) + + +class TestShellExecutor: + @pytest.mark.anyio + async def test_single_command_returns_stdout(self): + request = _make_shell_request(["echo hello"]) + result = await _shell_executor(request) + assert result.strip() == "hello" + + @pytest.mark.anyio + async def test_multiple_commands_combined(self): + request = _make_shell_request(["echo first", "echo second"]) + result = await _shell_executor(request) + assert "first" in result + assert "second" in result + assert result.index("first") < result.index("second") + + @pytest.mark.anyio + async def test_stderr_merged_into_stdout(self): + request = _make_shell_request(["echo err >&2"]) + result = await _shell_executor(request) + assert result.strip() == "err" + + @pytest.mark.anyio + async def test_command_timeout_kills_process(self): + request = _make_shell_request(["sleep 30"], timeout_ms=100) + result = await _shell_executor(request) + assert "timed out" in result.lower() + + @pytest.mark.anyio + async def test_nonzero_exit_code_appends_exit_code(self): + request = _make_shell_request(["echo failing && exit 1"]) + result = await _shell_executor(request) + assert "failing" in result + assert "[exit code: 1]" in result + + @pytest.mark.anyio + async def test_zero_exit_code_no_suffix(self): + request = _make_shell_request(["echo ok"]) + result = await _shell_executor(request) + assert "exit code" not in result + + @pytest.mark.anyio + async def test_timeout_ms_none_uses_default(self): + """When timeout_ms is None, SHELL_TIMEOUT is used (command completes fine).""" + request = _make_shell_request(["echo ok"], timeout_ms=None) + result = await _shell_executor(request) + assert result.strip() == "ok" + + @pytest.mark.anyio + async def test_timeout_stops_remaining_commands(self): + """After a timeout, subsequent commands are not executed.""" + request = _make_shell_request(["sleep 30", "echo should-not-run"], timeout_ms=100) + result = await _shell_executor(request) + assert "timed out" in result.lower() + assert "should-not-run" not in result + + @pytest.mark.anyio + async def test_subprocess_oserror_returns_error_message(self, monkeypatch): + """When create_subprocess_shell raises OSError, return error text instead of crashing.""" + import asyncio as _asyncio + + async def _failing_shell(*args, **kwargs): + raise OSError("fork failed") + + monkeypatch.setattr(_asyncio, "create_subprocess_shell", _failing_shell) + + request = _make_shell_request(["echo hello"]) + result = await _shell_executor(request) + assert "fork failed" in result From da3e7d404db619e178e990a2d0ae30863b1f61dc Mon Sep 17 00:00:00 2001 From: johnlin Date: Mon, 6 Apr 2026 23:58:57 +0800 Subject: [PATCH 7/7] feat: gate shell skills behind SHELL_SKILLS_ENABLED env var Shell skills and ShellTool are now disabled by default. Set SHELL_SKILLS_ENABLED=1 to opt in. Update README with the new env var, add missing instructions.md mount in Docker example, and list shell skills in the Features section. --- README.md | 19 +++++-------------- bot/agents.py | 9 +++++---- tests/test_agents.py | 30 ++++++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index 26ed9ca..378d3e5 100644 --- a/README.md +++ b/README.md @@ -12,6 +12,7 @@ See also: [agentic-slackbot](https://github.com/John-Lin/agentic-slackbot) — a - Supports OpenAI, Azure OpenAI endpoints - Per-conversation history with automatic truncation - Group reply chain — after `@mention`, anyone can continue by replying +- Local shell skills — let the agent run shell scripts from `skills/` (opt-in via `SHELL_SKILLS_ENABLED`) ## Install Dependencies @@ -41,6 +42,9 @@ export TELEGRAM_BOT_TOKEN="" # OpenAI API export OPENAI_API_KEY="" export OPENAI_MODEL="gpt-5.4" + +# Shell skills (disabled by default) +# export SHELL_SKILLS_ENABLED=1 ``` ## Agent Instructions @@ -180,6 +184,7 @@ docker run -d \ -e TELEGRAM_BOT_TOKEN="" \ -e OPENAI_API_KEY="" \ -e OPENAI_MODEL="gpt-5.4" \ + -v /path/to/instructions.md:/app/instructions.md \ -v /path/to/access.json:/app/access.json \ agentic-telegram-bot ``` @@ -198,17 +203,3 @@ docker run -d \ -v /path/to/access.json:/app/access.json \ agentic-telegram-bot ``` - -If you do not use MCP servers, you still need to mount `instructions.md`: - -```bash -docker run -d \ - --name telegent \ - -e BOT_USERNAME="@your_bot_username" \ - -e TELEGRAM_BOT_TOKEN="" \ - -e OPENAI_API_KEY="" \ - -e OPENAI_MODEL="gpt-5.4" \ - -v /path/to/instructions.md:/app/instructions.md \ - -v /path/to/access.json:/app/access.json \ - agentic-telegram-bot -``` diff --git a/bot/agents.py b/bot/agents.py index c338145..2894267 100644 --- a/bot/agents.py +++ b/bot/agents.py @@ -215,10 +215,11 @@ def from_dict(cls, name: str, config: dict[str, Any]) -> OpenAIAgent: ) ) tools: list[Any] = [] - skills = _load_shell_skills() - if skills: - environment = ShellToolLocalEnvironment(type="local", skills=skills) - tools.append(ShellTool(executor=_shell_executor, environment=environment)) + if os.getenv("SHELL_SKILLS_ENABLED"): + skills = _load_shell_skills() + if skills: + environment = ShellToolLocalEnvironment(type="local", skills=skills) + tools.append(ShellTool(executor=_shell_executor, environment=environment)) instructions = _load_instructions() return cls(name, instructions=instructions, mcp_servers=mcp_servers, tools=tools) diff --git a/tests/test_agents.py b/tests/test_agents.py index 9c104a4..1bf6636 100644 --- a/tests/test_agents.py +++ b/tests/test_agents.py @@ -242,13 +242,38 @@ def test_no_truncation_when_under_limit(self): @pytest.mark.usefixtures("_stub_instructions") class TestLoadShellSkills: + def _make_skill_dir(self, tmp_path): + """Create a valid skill directory and return its parent.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text("---\nname: my-skill\ndescription: A test skill\n---\n") + return tmp_path + + def test_disabled_by_default(self, tmp_path, monkeypatch): + """Skills exist but SHELL_SKILLS_ENABLED is not set — no ShellTool.""" + monkeypatch.delenv("SHELL_SKILLS_ENABLED", raising=False) + monkeypatch.setattr("bot.agents.SKILLS_DIR", self._make_skill_dir(tmp_path)) + agent = OpenAIAgent.from_dict("test", {"mcpServers": {}}) + shell_tools = [t for t in agent.agent.tools if isinstance(t, ShellTool)] + assert len(shell_tools) == 0 + + def test_enabled_with_env_var(self, tmp_path, monkeypatch): + """SHELL_SKILLS_ENABLED=1 and skills exist — ShellTool is added.""" + monkeypatch.setenv("SHELL_SKILLS_ENABLED", "1") + monkeypatch.setattr("bot.agents.SKILLS_DIR", self._make_skill_dir(tmp_path)) + agent = OpenAIAgent.from_dict("test", {"mcpServers": {}}) + shell_tools = [t for t in agent.agent.tools if isinstance(t, ShellTool)] + assert len(shell_tools) == 1 + def test_no_shell_tool_when_skills_dir_missing(self, tmp_path, monkeypatch): + monkeypatch.setenv("SHELL_SKILLS_ENABLED", "1") monkeypatch.setattr("bot.agents.SKILLS_DIR", tmp_path / "nonexistent") agent = OpenAIAgent.from_dict("test", {"mcpServers": {}}) shell_tools = [t for t in agent.agent.tools if isinstance(t, ShellTool)] assert len(shell_tools) == 0 def test_shell_tool_added_when_skill_found(self, tmp_path, monkeypatch): + monkeypatch.setenv("SHELL_SKILLS_ENABLED", "1") skill_dir = tmp_path / "my-skill" skill_dir.mkdir() (skill_dir / "SKILL.md").write_text("---\nname: my-skill\ndescription: A test skill\n---\n") @@ -262,6 +287,7 @@ def test_shell_tool_added_when_skill_found(self, tmp_path, monkeypatch): assert skill["path"] == str(skill_dir) def test_multiple_skills_all_mounted(self, tmp_path, monkeypatch): + monkeypatch.setenv("SHELL_SKILLS_ENABLED", "1") for name in ["skill-a", "skill-b"]: d = tmp_path / name d.mkdir() @@ -273,6 +299,7 @@ def test_multiple_skills_all_mounted(self, tmp_path, monkeypatch): assert len(shell_tool.environment["skills"]) == 2 def test_directory_without_skill_md_is_skipped(self, tmp_path, monkeypatch): + monkeypatch.setenv("SHELL_SKILLS_ENABLED", "1") (tmp_path / "not-a-skill").mkdir() good = tmp_path / "real-skill" good.mkdir() @@ -286,6 +313,7 @@ def test_directory_without_skill_md_is_skipped(self, tmp_path, monkeypatch): assert skills[0]["name"] == "real-skill" def test_mcp_servers_and_shell_skills_coexist(self, tmp_path, monkeypatch): + monkeypatch.setenv("SHELL_SKILLS_ENABLED", "1") skill_dir = tmp_path / "s" skill_dir.mkdir() (skill_dir / "SKILL.md").write_text("---\nname: s\ndescription: d\n---\n") @@ -298,6 +326,7 @@ def test_mcp_servers_and_shell_skills_coexist(self, tmp_path, monkeypatch): assert len(shell_tools) == 1 def test_unreadable_utf8_skill_file_is_skipped(self, tmp_path, monkeypatch): + monkeypatch.setenv("SHELL_SKILLS_ENABLED", "1") bad = tmp_path / "bad-skill" bad.mkdir() (bad / "SKILL.md").write_bytes(b"\xff\xfe\x00\x00") @@ -315,6 +344,7 @@ def test_unreadable_utf8_skill_file_is_skipped(self, tmp_path, monkeypatch): assert skills[0]["name"] == "good-skill" def test_oserror_reading_skill_file_is_skipped(self, tmp_path, monkeypatch): + monkeypatch.setenv("SHELL_SKILLS_ENABLED", "1") bad = tmp_path / "bad-skill" bad.mkdir() bad_file = bad / "SKILL.md"