From 4099d4aa82e5c06516a7454daaf5899fff691460 Mon Sep 17 00:00:00 2001 From: Daniel Date: Sun, 26 Jul 2026 14:11:07 +1000 Subject: [PATCH] feat: add MCP tool integration via MCPO (#11) --- .agents/skills/steward-steering/SKILL.md | 126 +++++++++++++++++++++++ .gitignore | 3 + CONFIGURATION.md | 20 ++++ docker-compose.dev.yml | 21 +++- docker-compose.yml | 17 +++ mcp.json.example | 25 +++++ steward/bot/telegram.py | 39 +++++-- steward/main.py | 2 + steward/tools/client.py | 108 ++++++++++++++++--- tests/test_bot.py | 23 +++++ tests/test_tools.py | 63 ++++++++++++ 11 files changed, 420 insertions(+), 27 deletions(-) create mode 100644 .agents/skills/steward-steering/SKILL.md create mode 100644 mcp.json.example diff --git a/.agents/skills/steward-steering/SKILL.md b/.agents/skills/steward-steering/SKILL.md new file mode 100644 index 0000000..d5815d8 --- /dev/null +++ b/.agents/skills/steward-steering/SKILL.md @@ -0,0 +1,126 @@ +--- +name: steward-steering +description: Critical project steering rules for Steward development. Prevents accidental commits of sensitive files and ensures git pushes are explicitly approved before executing. +--- + +# Steward Project Steering + +This skill enforces critical guardrails for working on the Steward project. Follow these rules strictly. + +## Rule 1: Never Commit Sensitive Files + +### Files That Must NEVER Be Committed to Git + +These files contain secrets, tokens, API keys, or personal configuration: + +- `mcp.json` - Contains MCP server tokens and credentials +- `.env` - Contains environment variables with secrets +- `config.yaml` - If it contains actual (non-example) values with secrets +- Any file containing: API keys, tokens, passwords, private URLs, auth credentials + +### Files That ARE Safe to Commit + +- `.env.example` - Template showing which env vars are needed (no actual values) +- `mcp.json.example` - Template showing MCP structure (no actual tokens) +- `config.example.yaml` - Template/example configuration (no real values) +- `CONFIGURATION.md` - Documentation +- Source code, tests, docker-compose templates + +### Before Committing + +1. Run `git status` and review all staged files +2. For each file, ask: "Does this contain secrets, tokens, or sensitive config?" +3. If YES: Do NOT commit it. Either: + - Remove it from staging: `git reset ` + - Add it to `.gitignore` if not already there + - Create an `.example` version without secrets +4. If NO: Safe to commit + +### If You Accidentally Commit Secrets + +Do NOT try to fix with `git filter-repo` or history rewrites. Instead: +1. Tell the user immediately +2. User must manually rotate/revoke any compromised tokens +3. Use `git rm --cached ` to stop tracking it +4. Add to `.gitignore` +5. Amend the commit or create a new one +6. Force push to origin only if the branch hasn't been merged to main + +## Rule 2: Git Push Requires Explicit Approval + +### When You Can Commit WITHOUT Approval + +You can commit to the current branch anytime (this is local, safe): + +```bash +git add +git commit -m "message" +``` + +### When You MUST Ask Before Pushing + +Any `git push` command requires explicit user approval first: + +1. **Tell the user:** + ``` + Ready to push branch feature/my-feature to origin. Should I proceed? + ``` + +2. **Wait for user to explicitly say "yes" or "push it"** (or equivalent approval) + +3. **Only then execute:** + ```bash + git push origin feature/my-feature + ``` + +### Exceptions (No Approval Needed) + +- `git push --tags` to push individual tags (assuming the tag commit is already approved) +- Tags that point to already-approved commits + +### Why This Rule Exists + +- Git pushes are permanent and trigger CI/CD +- Force pushes rewrite history +- Accidental pushes before review waste CI resources +- Better to ask than regret + +## Rule 3: Use `.gitignore` for Sensitive Patterns + +When sensitive files are generated locally (like `mcp.json`, `.env`), ensure they're in `.gitignore`: + +```bash +# .gitignore +mcp.json # Actual config with secrets +.env # Local environment file +config.yaml # If it contains real values +``` + +Then create `.example` versions: + +```bash +mcp.json.example # Template with placeholders +.env.example # Shows required variables +config.example.yaml # Shows structure +``` + +Users copy: `cp mcp.json.example mcp.json` then edit locally. + +## Checklist Before Each Push + +- [ ] All staged files reviewed for secrets +- [ ] `.gitignore` includes sensitive files +- [ ] `.example` files created where needed +- [ ] All tests pass (`pytest`) +- [ ] Ruff checks pass (`ruff check`) +- [ ] Commit messages follow conventional commits +- [ ] User has explicitly approved the push + +## When Unsure + +If you're unsure whether a file should be committed: +- Ask the user +- Error on the side of caution (don't commit) +- Create an `.example` version instead + +This is a low-cost insurance policy against leaking secrets. diff --git a/.gitignore b/.gitignore index 2c88c73..ad40f1e 100644 --- a/.gitignore +++ b/.gitignore @@ -35,3 +35,6 @@ htmlcov/ # OS .DS_Store + +# MCP configuration (contains sensitive tokens) +mcp.json diff --git a/CONFIGURATION.md b/CONFIGURATION.md index 3fde322..ae5498d 100644 --- a/CONFIGURATION.md +++ b/CONFIGURATION.md @@ -249,3 +249,23 @@ To migrate from `.env`: 1. Create a `config.yaml` with your configuration 2. Set `CONFIG_FILE` environment variable 3. Set secrets via `STEWARD__*` environment variables + +### Setting up GitHub MCP + +Configure the GitHub MCP server in `mcp.json` using `mcp-remote` to connect to the remote server: + +```json +"github": { + "command": "npx", + "args": ["mcp-remote", "https://api.githubcopilot.com/mcp/", "--header", "Authorization: Bearer YOUR_GITHUB_PAT"] +} +``` + +Replace `YOUR_GITHUB_PAT` with your GitHub Personal Access Token from https://github.com/settings/personal-access-tokens/new + +**Available GitHub Tools via MCP:** +- Repository management (list, create, read files, branches, commits) +- Issues and pull requests (create, update, comment, search) +- GitHub Actions workflows +- Code security and scanning +- And many more - see https://github.com/github/github-mcp-server for full docs diff --git a/docker-compose.dev.yml b/docker-compose.dev.yml index a9e88b4..0809aed 100644 --- a/docker-compose.dev.yml +++ b/docker-compose.dev.yml @@ -1,10 +1,27 @@ services: + mcpo: + image: ghcr.io/open-webui/mcpo:main + environment: + MCPO_API_KEY: "${MCPO_API_KEY:-openwebui}" + volumes: + - ./mcp.json:/mcp.json + command: + - --api-key + - "${MCPO_API_KEY:-openwebui}" + - --config + - /mcp.json + restart: unless-stopped + steward: build: . - user: "1000:1000" # matches the UID/GID created in the Dockerfile - env_file: .env # copy .env.example → .env and fill in your values + depends_on: + - mcpo + user: "1000:1000" # matches the UID/GID created in the Dockerfile + env_file: .env # copy .env.example → .env and fill in your values environment: THREAD_MEMORY_PATH: /data/thread_memory.json + STEWARD__TOOLS__MCP_SERVER_URL: "http://mcpo:8000" + STEWARD__TOOLS__MCP_SERVER_API_KEY: "${MCPO_API_KEY:-openwebui}" volumes: # Bind-mount a local ./data directory for persistent storage. # Create it before the first run: mkdir -p data diff --git a/docker-compose.yml b/docker-compose.yml index 1b38650..2007194 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -1,10 +1,27 @@ services: + mcpo: + image: ghcr.io/open-webui/mcpo:main + environment: + MCPO_API_KEY: "${MCPO_API_KEY:-openwebui}" + volumes: + - ./mcp.json:/mcp.json + command: + - --api-key + - "${MCPO_API_KEY:-openwebui}" + - --config + - /mcp.json + restart: unless-stopped + steward: image: ghcr.io/djw4/steward:latest + depends_on: + - mcpo user: "1000:1000" # matches the UID/GID created in the Dockerfile env_file: .env # copy .env.example → .env and fill in your values environment: THREAD_MEMORY_PATH: /data/thread_memory.json + STEWARD__TOOLS__MCP_SERVER_URL: "http://mcpo:8000" + STEWARD__TOOLS__MCP_SERVER_API_KEY: "${MCPO_API_KEY:-openwebui}" volumes: # Bind-mount a local ./data directory for persistent storage. # Create it before the first run: mkdir -p data diff --git a/mcp.json.example b/mcp.json.example new file mode 100644 index 0000000..0b262fe --- /dev/null +++ b/mcp.json.example @@ -0,0 +1,25 @@ +{ + "mcpServers": { + "github": { + "command": "npx", + "args": ["mcp-remote", "https://api.githubcopilot.com/mcp/", "--header", "Authorization: Bearer YOUR_GITHUB_PAT"] + }, + "memory": { + "command": "npx", + "args": ["-y", "@modelcontextprotocol/server-memory"] + }, + "nocodb": { + "command": "npx", + "args": [ + "mcp-remote", + "https://nocodb.k8sx.com/mcp/ncvd658010h9gx76", + "--header", + "xc-mcp-token: YOUR_NOCODB_TOKEN" + ] + }, + "trilium": { + "command": "npx", + "args": ["mcp-remote", "https://your-trilium-instance.com/mcp"] + } + } +} diff --git a/steward/bot/telegram.py b/steward/bot/telegram.py index c7f6bcd..0565dff 100644 --- a/steward/bot/telegram.py +++ b/steward/bot/telegram.py @@ -72,7 +72,10 @@ def _is_allowed(user_id: int, settings: Settings) -> bool: """Return True if the user is in the allow-list (or no list is configured).""" if not settings.telegram_allowed_user_ids: return True - return user_id in settings.telegram_allowed_user_ids + allowed = user_id in settings.telegram_allowed_user_ids + if not allowed: + logger.info("Ignoring update from unauthorized user %s", user_id) + return allowed def _is_group_enabled(chat_id: int, settings: Settings) -> bool: @@ -82,6 +85,26 @@ def _is_group_enabled(chat_id: int, settings: Settings) -> bool: return chat_id in settings.telegram_group_ids +def _is_chat_enabled(chat: Any, settings: Settings) -> bool: + """Return True if the chat is allowed. + + Private chats are governed only by the user allow-list. Group/channel allow-listing + applies only to non-private chats. + """ + if getattr(chat, "type", None) == "private": + return True + + allowed = _is_group_enabled(chat.id, settings) + if not allowed: + logger.info( + "Ignoring update in unauthorized chat %s (type=%s); configured group IDs: %s", + chat.id, + getattr(chat, "type", None), + settings.telegram_group_ids, + ) + return allowed + + async def _send_long(update: Update, text: str) -> None: """Send text, splitting across messages if it exceeds Telegram's 4096-char limit.""" limit = 4096 @@ -100,7 +123,7 @@ async def start_handler(update: Update, context: ContextTypes.DEFAULT_TYPE) -> N return chat = update.effective_chat - if chat is None or not _is_group_enabled(chat.id, settings): + if chat is None or not _is_chat_enabled(chat, settings): return await update.message.reply_text( # type: ignore[union-attr] @@ -124,7 +147,7 @@ async def help_handler(update: Update, context: ContextTypes.DEFAULT_TYPE) -> No return chat = update.effective_chat - if chat is None or not _is_group_enabled(chat.id, settings): + if chat is None or not _is_chat_enabled(chat, settings): return await update.message.reply_text( # type: ignore[union-attr] @@ -152,7 +175,7 @@ async def clear_handler(update: Update, context: ContextTypes.DEFAULT_TYPE) -> N return chat = update.effective_chat - if chat is None or not _is_group_enabled(chat.id, settings): + if chat is None or not _is_chat_enabled(chat, settings): return key = _thread_key(update) @@ -187,7 +210,7 @@ async def flush_handler(update: Update, context: ContextTypes.DEFAULT_TYPE) -> N return chat = update.effective_chat - if chat is None or not _is_group_enabled(chat.id, settings): + if chat is None or not _is_chat_enabled(chat, settings): return key = _thread_key(update) @@ -270,7 +293,7 @@ async def recall_handler(update: Update, context: ContextTypes.DEFAULT_TYPE) -> return chat = update.effective_chat - if chat is None or not _is_group_enabled(chat.id, settings): + if chat is None or not _is_chat_enabled(chat, settings): return # If the user supplied a keyword query, search the knowledge base @@ -334,7 +357,7 @@ async def analyse_handler(update: Update, context: ContextTypes.DEFAULT_TYPE) -> return chat = update.effective_chat - if chat is None or not _is_group_enabled(chat.id, settings): + if chat is None or not _is_chat_enabled(chat, settings): return await update.message.reply_text("Running analysis, please wait...") # type: ignore[union-attr] @@ -389,7 +412,7 @@ async def message_handler(update: Update, context: ContextTypes.DEFAULT_TYPE) -> chat = update.effective_chat if user is None or not _is_allowed(user.id, settings): return - if chat is None or not _is_group_enabled(chat.id, settings): + if chat is None or not _is_chat_enabled(chat, settings): return text = update.message.text # type: ignore[union-attr] diff --git a/steward/main.py b/steward/main.py index 93260cb..927b982 100644 --- a/steward/main.py +++ b/steward/main.py @@ -17,6 +17,8 @@ logging.basicConfig( level=logging.INFO, format="%(asctime)s %(levelname)s %(name)s: %(message)s", ) +logging.getLogger("httpx").setLevel(logging.WARNING) +logging.getLogger("httpcore").setLevel(logging.WARNING) logger = logging.getLogger(__name__) diff --git a/steward/tools/client.py b/steward/tools/client.py index d8e70b7..da703df 100644 --- a/steward/tools/client.py +++ b/steward/tools/client.py @@ -9,6 +9,7 @@ from __future__ import annotations import json import logging +import re from typing import Any import httpx @@ -31,6 +32,8 @@ class ToolClient: self._api_key = api_key self._timeout = timeout self._spec: dict[str, Any] | None = None + self._server_specs: dict[str, dict[str, Any]] | None = None + self._tool_routes: dict[str, tuple[str, str]] = {} self._tools: list[dict[str, Any]] | None = None @property @@ -59,11 +62,36 @@ class ToolClient: return self._spec async def get_tools(self) -> list[dict[str, Any]]: - """Return OpenAI function-calling tool definitions from the OpenAPI spec.""" + """Return OpenAI function-calling tool definitions from the OpenAPI spec. + + MCPO config-file mode exposes each configured MCP server under its own subpath + (for example ``/github/openapi.json``), while the root schema has no paths and + only links to the per-server docs. In that mode, tool names are namespaced as + ``server__operation`` so similarly named tools from different MCP servers do not + collide. + """ if self._tools is not None: return self._tools - spec = await self.get_spec() - self._tools = _spec_to_openai_tools(spec) + + specs = await self._get_server_specs() + tools: list[dict[str, Any]] = [] + self._tool_routes = {} + + for server_name, spec in specs.items(): + server_tools = _spec_to_openai_tools(spec) + for tool in server_tools: + function = tool["function"] + operation_name = function["name"] + if server_name: + namespaced_name = f"{server_name}__{operation_name}" + function["name"] = namespaced_name + function["description"] = f"[{server_name}] {function.get('description', '')}" + else: + namespaced_name = operation_name + self._tool_routes[namespaced_name] = (server_name, operation_name) + tools.append(tool) + + self._tools = tools logger.info("Registered %d tools from %s", len(self._tools), self._base_url) return self._tools @@ -74,12 +102,19 @@ class ToolClient: partitions *arguments* into path params, query params, and request body, then makes the HTTP request. """ - spec = await self.get_spec() + await self.get_tools() + if tool_name not in self._tool_routes: + raise ValueError(f"Operation '{tool_name}' not found in spec") + + server_name, operation_name = self._tool_routes[tool_name] + specs = await self._get_server_specs() + spec = specs[server_name] path, method, path_params, query_params, body = _resolve_operation( - spec, tool_name, arguments + spec, operation_name, arguments ) - url = self._base_url + path + base_path = f"/{server_name}" if server_name else "" + url = self._base_url + base_path + path for key, value in path_params.items(): url = url.replace(f"{{{key}}}", str(value)) @@ -100,15 +135,59 @@ class ToolClient: pass return resp.text + async def _get_server_specs(self) -> dict[str, dict[str, Any]]: + """Return OpenAPI specs keyed by MCPO server name. + + ``""`` denotes the root OpenAPI server. Non-empty keys denote MCPO + config-file subservers such as ``github`` or ``memory``. + """ + if self._server_specs is not None: + return self._server_specs + + root_spec = await self.get_spec() + if root_spec.get("paths"): + self._server_specs = {"": root_spec} + return self._server_specs + + server_names = _discover_mcpo_server_names(root_spec) + if not server_names: + self._server_specs = {"": root_spec} + return self._server_specs + + specs: dict[str, dict[str, Any]] = {} + async with httpx.AsyncClient(timeout=self._timeout) as http: + for server_name in server_names: + resp = await http.get( + f"{self._base_url}/{server_name}/openapi.json", + headers=self._headers(), + ) + resp.raise_for_status() + spec = resp.json() + specs[server_name] = spec + logger.info( + "Loaded OpenAPI spec from %s/%s (%d paths)", + self._base_url, + server_name, + len(spec.get("paths", {})), + ) + + self._server_specs = specs + return self._server_specs + # --------------------------------------------------------------------------- # OpenAPI → OpenAI tool-definition helpers # --------------------------------------------------------------------------- -def _resolve_ref( - schema: dict[str, Any], components: dict[str, Any] -) -> dict[str, Any]: +def _discover_mcpo_server_names(spec: dict[str, Any]) -> list[str]: + """Extract MCPO config-file server names from the root OpenAPI description.""" + description = str(spec.get("info", {}).get("description", "")) + names = re.findall(r"\[([^\]]+)]\(/([^/]+)/docs\)", description) + return [name for name, path_name in names if name == path_name] + + +def _resolve_ref(schema: dict[str, Any], components: dict[str, Any]) -> dict[str, Any]: """Recursively resolve a ``$ref`` inside an OpenAPI schema.""" if "$ref" not in schema: return schema @@ -138,8 +217,7 @@ def _schema_to_json_schema( result["items"] = _schema_to_json_schema(resolved["items"], components) if "properties" in resolved: result["properties"] = { - k: _schema_to_json_schema(v, components) - for k, v in resolved["properties"].items() + k: _schema_to_json_schema(v, components) for k, v in resolved["properties"].items() } if "required" in resolved: result["required"] = resolved["required"] @@ -175,9 +253,7 @@ def _spec_to_openai_tools(spec: dict[str, Any]) -> list[dict[str, Any]]: # URL / query parameters for param in op.get("parameters", []): name: str = param["name"] - schema = _schema_to_json_schema( - param.get("schema", {"type": "string"}), components - ) + schema = _schema_to_json_schema(param.get("schema", {"type": "string"}), components) if param.get("description"): schema["description"] = param["description"] properties[name] = schema @@ -188,9 +264,7 @@ def _spec_to_openai_tools(spec: dict[str, Any]) -> list[dict[str, Any]]: rb: dict[str, Any] = op.get("requestBody", {}) if rb: json_content = rb.get("content", {}).get("application/json", {}) - body_schema = _schema_to_json_schema( - json_content.get("schema", {}), components - ) + body_schema = _schema_to_json_schema(json_content.get("schema", {}), components) if body_schema.get("type") == "object": for prop_name, prop_schema in body_schema.get("properties", {}).items(): properties[prop_name] = prop_schema diff --git a/tests/test_bot.py b/tests/test_bot.py index 17e1fb9..b081289 100644 --- a/tests/test_bot.py +++ b/tests/test_bot.py @@ -9,6 +9,7 @@ from telegram.ext import CallbackContext from steward.bot.telegram import ( _history, _is_allowed, + _is_chat_enabled, _thread_history, clear_handler, message_handler, @@ -35,6 +36,7 @@ def _make_update( text: str = "hello", chat_id: int | None = None, thread_id: int | None = None, + chat_type: str = "private", ) -> Update: user = MagicMock(spec=User) user.id = user_id @@ -46,6 +48,7 @@ def _make_update( chat = MagicMock(spec=Chat) chat.id = chat_id if chat_id is not None else user_id + chat.type = chat_type update = MagicMock(spec=Update) update.effective_user = user @@ -87,6 +90,26 @@ class TestIsAllowed: assert _is_allowed(999, settings) is False +class TestIsChatEnabled: + def test_private_chat_ignores_group_allowlist(self): + settings = _make_settings(telegram_group_ids=[-100123]) + update = _make_update(user_id=123, chat_type="private") + + assert _is_chat_enabled(update.effective_chat, settings) is True + + def test_group_chat_accepts_configured_group(self): + settings = _make_settings(telegram_group_ids=[-5308306472]) + update = _make_update(chat_id=-5308306472, chat_type="group") + + assert _is_chat_enabled(update.effective_chat, settings) is True + + def test_group_chat_rejects_unconfigured_group(self): + settings = _make_settings(telegram_group_ids=[-1005308306472]) + update = _make_update(chat_id=-5308306472, chat_type="group") + + assert _is_chat_enabled(update.effective_chat, settings) is False + + @pytest.mark.asyncio async def test_start_handler_replies(monkeypatch): settings = _make_settings() diff --git a/tests/test_tools.py b/tests/test_tools.py index 2af1aba..464bb62 100644 --- a/tests/test_tools.py +++ b/tests/test_tools.py @@ -14,6 +14,7 @@ from steward.config import Settings from steward.llm.client import LLMClient from steward.tools.client import ( ToolClient, + _discover_mcpo_server_names, _operation_id, _resolve_operation, _schema_to_json_schema, @@ -25,6 +26,22 @@ from tests.conftest import make_settings # Fixtures # --------------------------------------------------------------------------- +MCPO_ROOT_SPEC: dict[str, Any] = { + "openapi": "3.1.0", + "info": { + "title": "MCP OpenAPI Proxy", + "version": "1.0", + "description": ( + "Automatically generated API from MCP Tool Schemas\n\n" + "- **available tools**:\n" + " - [github](/github/docs)\n" + " - [memory](/memory/docs)\n" + ), + }, + "paths": {}, +} + + SIMPLE_SPEC: dict[str, Any] = { "openapi": "3.0.0", "info": {"title": "Test", "version": "1.0.0"}, @@ -248,6 +265,10 @@ def test_resolve_operation_not_found(): # --------------------------------------------------------------------------- +def test_discover_mcpo_server_names(): + assert _discover_mcpo_server_names(MCPO_ROOT_SPEC) == ["github", "memory"] + + @pytest.mark.asyncio async def test_tool_client_get_spec(): """get_spec() fetches from /openapi.json and caches the result.""" @@ -277,6 +298,48 @@ async def test_tool_client_get_tools(): assert tools2 is tools +@pytest.mark.asyncio +async def test_tool_client_get_tools_from_mcpo_subservers(): + with respx.mock: + respx.get("http://tools.local/openapi.json").mock( + return_value=Response(200, json=MCPO_ROOT_SPEC) + ) + respx.get("http://tools.local/github/openapi.json").mock( + return_value=Response(200, json=SIMPLE_SPEC) + ) + respx.get("http://tools.local/memory/openapi.json").mock( + return_value=Response(200, json=SIMPLE_SPEC) + ) + client = ToolClient("http://tools.local") + tools = await client.get_tools() + + tool_names = {tool["function"]["name"] for tool in tools} + assert "github__list_items" in tool_names + assert "memory__list_items" in tool_names + assert len(tools) == 6 + + +@pytest.mark.asyncio +async def test_tool_client_call_mcpo_namespaced_tool(): + with respx.mock: + respx.get("http://tools.local/openapi.json").mock( + return_value=Response(200, json=MCPO_ROOT_SPEC) + ) + respx.get("http://tools.local/github/openapi.json").mock( + return_value=Response(200, json=SIMPLE_SPEC) + ) + respx.get("http://tools.local/memory/openapi.json").mock( + return_value=Response(200, json=SIMPLE_SPEC) + ) + respx.get("http://tools.local/github/items").mock( + return_value=Response(200, json=[{"id": 1}]) + ) + client = ToolClient("http://tools.local") + result = await client.call("github__list_items", {"limit": 10}) + + assert "id" in result + + @pytest.mark.asyncio async def test_tool_client_call_get_with_query(): with respx.mock: