Merge pull request #19 from John-Lin/wip/align-shell-arch

refactor: align shell and logging architecture with agentic-slackbot

Author: John-Lin

README.md

@@ -12,7 +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`)
+- Optional local shell via `ShellTool`, controlled by `SHELL_ENABLED` and `SHELL_SKILLS_DIR`
 
 ## Install Dependencies
 
@@ -43,8 +43,12 @@ export TELEGRAM_BOT_TOKEN=""
 export OPENAI_API_KEY=""
 export OPENAI_MODEL="gpt-5.4"
 
-# Shell skills (disabled by default)
-# export SHELL_SKILLS_ENABLED=1
+# Local shell (disabled by default)
+# export SHELL_ENABLED=1
+# export SHELL_SKILLS_DIR="./skills"  # optional; mount skills alongside the shell
+
+# Optional verbose OpenAI Agents SDK logging
+# export AGENT_VERBOSE_LOG=1
 ```
 
 ## Agent Instructions
@@ -173,6 +177,38 @@ uv run bot access group remove <GROUP_ID>
 
 Group members do not need to pair individually — access is controlled at the group level.
 
+## Local Shell (Optional)
+
+The bot can expose a local `ShellTool`. This is **disabled by default**. Enable it with:
+
+```
+export SHELL_ENABLED=1
+```
+
+With just `SHELL_ENABLED=1`, the agent gets bare local shell access with no pre-defined skills.
+
+### Shell Skills (Optional)
+
+You can optionally mount a skills directory alongside the shell. Each immediate subdirectory containing a `SKILL.md` file is registered as a skill and exposed to the agent as a hint (skills are advisory metadata — they do **not** sandbox command execution).
+
+```
+export SHELL_ENABLED=1
+export SHELL_SKILLS_DIR="./skills"
+```
+
+`SHELL_SKILLS_DIR` is ignored unless `SHELL_ENABLED` is set. If the directory is missing or contains no valid skills, the bot falls back to a bare shell and logs a warning.
+
+The `SKILL.md` file should have YAML frontmatter with `name` and `description` fields:
+
+```markdown
+---
+name: my-skill
+description: A brief description of what this skill does
+---
+
+Detailed instructions for the agent...
+```
+
 ## Docker
 
 ```bash

app.py

@@ -6,6 +6,8 @@
 import logging
 import sys
 
+from agents import enable_verbose_stdout_logging
+
 from bot.agents import OpenAIAgent
 from bot.auth import add_group
 from bot.auth import allow_user
@@ -15,15 +17,22 @@
 from bot.auth import remove_user
 from bot.auth import set_dm_policy
 from bot.config import Configuration
+from bot.config import env_flag
 from bot.telegram import TelegramMCPBot
 
 
-async def start_bot() -> None:
-    """Initialize and run the Telegram bot."""
+def _configure_logging() -> None:
     logging.basicConfig(
         format="%(asctime)s - %(name)s - %(levelname)s - %(message)s",
         level=logging.INFO,
     )
+    if env_flag("AGENT_VERBOSE_LOG"):
+        enable_verbose_stdout_logging()
+
+
+async def start_bot() -> None:
+    """Initialize and run the Telegram bot."""
+    _configure_logging()
     config = Configuration()
 
     server_config = config.load_config("servers_config.json")

bot/agents.py

@@ -20,12 +20,13 @@
 from agents.tracing import set_tracing_disabled
 from openai import AsyncOpenAI
 
+from .config import env_flag
+
 INSTRUCTIONS_FILE = Path("instructions.md")
 
 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)
 
@@ -80,17 +81,17 @@ def _parse_skill_description(content: str) -> str:
     return ""
 
 
-def _load_shell_skills() -> list[ShellToolLocalSkill]:
-    """Discover local shell skills under SKILLS_DIR.
+def _load_shell_skills(skills_dir: Path) -> list[ShellToolLocalSkill]:
+    """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.
+    Each immediate subdirectory containing a ``SKILL.md`` file 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():
+    if not skills_dir.is_dir():
         return []
     skills: list[ShellToolLocalSkill] = []
-    for skill_dir in sorted(SKILLS_DIR.iterdir()):
+    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
@@ -109,6 +110,37 @@ def _load_shell_skills() -> list[ShellToolLocalSkill]:
     return skills
 
 
+def _get_shell_environment() -> ShellToolLocalEnvironment | None:
+    """Return the configured local shell environment, if enabled.
+
+    Controlled by two independent environment variables:
+
+    * ``SHELL_ENABLED`` — when truthy, the ``ShellTool`` is attached to the agent.
+    * ``SHELL_SKILLS_DIR`` — optional path; when set, skills discovered under it
+      are mounted alongside the shell. Ignored if ``SHELL_ENABLED`` is not set.
+    """
+    skills_dir_env = os.getenv("SHELL_SKILLS_DIR")
+    if not env_flag("SHELL_ENABLED"):
+        if skills_dir_env:
+            logging.warning(
+                "SHELL_SKILLS_DIR=%r is set but SHELL_ENABLED is not; ignoring skills dir.",
+                skills_dir_env,
+            )
+        return None
+
+    environment: ShellToolLocalEnvironment = {"type": "local"}
+    if skills_dir_env:
+        skills = _load_shell_skills(Path(skills_dir_env))
+        if skills:
+            environment["skills"] = skills
+        else:
+            logging.warning(
+                "SHELL_SKILLS_DIR=%r yielded no skills; attaching bare local shell.",
+                skills_dir_env,
+            )
+    return environment
+
+
 async def _shell_executor(request: ShellCommandRequest) -> str:
     """Run each shell command from the request and return combined output.
 
@@ -215,11 +247,9 @@ def from_dict(cls, name: str, config: dict[str, Any]) -> OpenAIAgent:
                     )
                 )
         tools: list[Any] = []
-        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))
+        environment = _get_shell_environment()
+        if environment is not None:
+            tools.append(ShellTool(executor=_shell_executor, environment=environment))
 
         instructions = _load_instructions()
         return cls(name, instructions=instructions, mcp_servers=mcp_servers, tools=tools)

bot/config.py

@@ -8,6 +8,21 @@
 
 logger = logging.getLogger(__name__)
 
+_FALSY_ENV_VALUES = frozenset({"", "0", "false", "no", "off"})
+
+
+def env_flag(name: str) -> bool:
+    """Return True if env var ``name`` is set to a truthy value.
+
+    Common falsy spellings (empty, "0", "false", "no", "off") are treated as
+    disabled so that ``FOO=0`` behaves as users intuitively expect rather than
+    as Python's default "non-empty string is truthy" rule.
+    """
+    raw = os.getenv(name)
+    if raw is None:
+        return False
+    return raw.strip().lower() not in _FALSY_ENV_VALUES
+
 
 class Configuration:
     """Manages configuration and environment variables for the MCP Telegram bot."""