From c0562b5d250217889e48bd85221b76ae5f512c16 Mon Sep 17 00:00:00 2001 From: Brian Krafft Date: Fri, 3 Apr 2026 13:46:57 -0700 Subject: [PATCH] feat(4.7): extract MCPToolHandlers into openspace/mcp/tool_handlers.py Extract all 4 MCP tool handlers (execute_task, search_skills, fix_skill, upload_skill) and their 12 helper functions from mcp_server.py into a new openspace/mcp/tool_handlers.py module. Changes: - Create openspace/mcp/ package with __init__.py - Create openspace/mcp/tool_handlers.py (799 lines) with: - 4 tool handlers + register_handlers(mcp) wiring function - All lifecycle helpers (_get_openspace, _get_store, etc.) - Upload metadata helpers (_write/_read_upload_meta) - Cloud import helpers (_cloud_search_and_import, _do_import_cloud_skill) - Result formatting (_format_task_result, _json_ok, _json_error) - Slim mcp_server.py to 220 lines (was 992): bootstrap, FastMCP, server entry - Update test imports: test_auto_import_disabled, test_integration, test_traceback_safety now reference openspace.mcp.tool_handlers - Add 20 new tests in test_mcp_tool_handlers.py All 1,376 tests pass, 117 skipped. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- openspace/mcp/__init__.py | 1 + openspace/mcp/tool_handlers.py | 799 +++++++++++++++++++++++++++++ openspace/mcp_server.py | 778 +--------------------------- tests/test_auto_import_disabled.py | 54 +- tests/test_integration.py | 44 +- tests/test_mcp_tool_handlers.py | 213 ++++++++ tests/test_traceback_safety.py | 22 +- 7 files changed, 1076 insertions(+), 835 deletions(-) create mode 100644 openspace/mcp/__init__.py create mode 100644 openspace/mcp/tool_handlers.py create mode 100644 tests/test_mcp_tool_handlers.py diff --git a/openspace/mcp/__init__.py b/openspace/mcp/__init__.py new file mode 100644 index 00000000..5629d810 --- /dev/null +++ b/openspace/mcp/__init__.py @@ -0,0 +1 @@ +"""OpenSpace MCP sub-package — decomposed from mcp_server.py (P4 Epics 4.7-4.9).""" diff --git a/openspace/mcp/tool_handlers.py b/openspace/mcp/tool_handlers.py new file mode 100644 index 00000000..93d9f586 --- /dev/null +++ b/openspace/mcp/tool_handlers.py @@ -0,0 +1,799 @@ +"""MCP tool handler implementations for OpenSpace. + +Extracted from ``mcp_server.py`` (Epic 4.7) to isolate tool handler logic +from server bootstrap and transport concerns. + +Four MCP tools: + - execute_task — Full grounding engine delegation + - search_skills — Standalone skill search/discovery + - fix_skill — Manual skill fix (FIX evolution only) + - upload_skill — Upload local skill to cloud + +Call ``register_handlers(mcp)`` to wire all handlers to a FastMCP instance. +""" + +from __future__ import annotations + +import asyncio +import json +import logging +import os +from pathlib import Path +from typing import Any, Dict, List, Optional + +logger = logging.getLogger("openspace.mcp_server") + +# --------------------------------------------------------------------------- +# Module-level state (singleton pattern, same as original mcp_server.py) +# --------------------------------------------------------------------------- +_openspace_instance = None +_openspace_lock = asyncio.Lock() +_standalone_store = None + +# Tracks bot skill directories already registered this session. +_registered_skill_dirs: set = set() + +_UPLOAD_META_FILENAME = ".upload_meta.json" + + +# --------------------------------------------------------------------------- +# Lifecycle helpers +# --------------------------------------------------------------------------- +async def _get_openspace(): + """Lazy-initialise the OpenSpace engine.""" + global _openspace_instance + if _openspace_instance is not None and _openspace_instance.is_initialized(): + return _openspace_instance + + async with _openspace_lock: + if _openspace_instance is not None and _openspace_instance.is_initialized(): + return _openspace_instance + + logger.info("Initializing OpenSpace engine ...") + from openspace.host_detection import build_grounding_config_path, build_llm_kwargs + from openspace.tool_layer import OpenSpace, OpenSpaceConfig + + env_model = os.environ.get("OPENSPACE_MODEL", "") + workspace = os.environ.get("OPENSPACE_WORKSPACE") + max_iter = int(os.environ.get("OPENSPACE_MAX_ITERATIONS", "20")) + enable_rec = os.environ.get("OPENSPACE_ENABLE_RECORDING", "true").lower() in ("true", "1", "yes") + + backend_scope_raw = os.environ.get("OPENSPACE_BACKEND_SCOPE") + backend_scope = [b.strip() for b in backend_scope_raw.split(",") if b.strip()] if backend_scope_raw else None + + config_path = build_grounding_config_path() + model, llm_kwargs = build_llm_kwargs(env_model) + + _pkg_root = str(Path(__file__).resolve().parent.parent.parent) + recording_base = workspace or _pkg_root + recording_log_dir = str(Path(recording_base) / "logs" / "recordings") + + config = OpenSpaceConfig( + llm_model=model, + llm_kwargs=llm_kwargs, + workspace_dir=workspace, + grounding_max_iterations=max_iter, + enable_recording=enable_rec, + recording_backends=["shell"] if enable_rec else None, + recording_log_dir=recording_log_dir, + backend_scope=backend_scope, + grounding_config_path=config_path, + ) + + _openspace_instance = OpenSpace(config=config) + await _openspace_instance.initialize() + logger.info("OpenSpace engine ready (model=%s).", model) + + # Auto-register host bot skill directories from env + host_skill_dirs_raw = os.environ.get("OPENSPACE_HOST_SKILL_DIRS", "") + if host_skill_dirs_raw: + dirs = [d.strip() for d in host_skill_dirs_raw.split(",") if d.strip()] + if dirs: + await _auto_register_skill_dirs(dirs) + logger.info("Auto-registered host skill dirs from OPENSPACE_HOST_SKILL_DIRS: %s", dirs) + + return _openspace_instance + + +def _get_store(): + """Get SkillStore — reuses OpenSpace's internal instance when available.""" + global _standalone_store + if _openspace_instance and _openspace_instance.is_initialized(): + internal = getattr(_openspace_instance, "_skill_store", None) + if internal and not internal._closed: + return internal + if _standalone_store is None or _standalone_store._closed: + from openspace.skill_engine import SkillStore + + _standalone_store = SkillStore() + return _standalone_store + + +def _is_auto_import_enabled() -> bool: + """Check whether cloud auto-import is enabled in SkillConfig. + + Returns ``False`` (safe default) if the config is unavailable or the + flag is not explicitly set to ``True``. + """ + if _openspace_instance and _openspace_instance.is_initialized(): + gc = getattr(_openspace_instance, "_grounding_config", None) + if gc and gc.skills: + return gc.skills.auto_import_enabled + return False + + +def _get_cloud_client(): + """Get a OpenSpaceClient instance (raises CloudError if not configured).""" + from openspace.cloud.auth import get_openspace_auth + from openspace.cloud.client import OpenSpaceClient + + auth_headers, api_base = get_openspace_auth() + return OpenSpaceClient(auth_headers, api_base) + + +# --------------------------------------------------------------------------- +# Upload metadata helpers +# --------------------------------------------------------------------------- +def _write_upload_meta(skill_dir: Path, info: Dict[str, Any]) -> None: + """Write ``.upload_meta.json`` so ``upload_skill`` can read pre-saved metadata.""" + meta = { + "origin": info.get("origin", "imported"), + "parent_skill_ids": info.get("parent_skill_ids", []), + "change_summary": info.get("change_summary", ""), + "created_by": info.get("created_by", "openspace"), + "tags": info.get("tags", []), + } + meta_path = skill_dir / _UPLOAD_META_FILENAME + try: + meta_path.write_text( + json.dumps(meta, ensure_ascii=False, indent=2) + "\n", + encoding="utf-8", + ) + logger.debug(f"Wrote upload metadata to {meta_path}") + except Exception as e: + logger.warning(f"Failed to write upload metadata: {e}") + + +def _read_upload_meta(skill_dir: Path) -> Dict[str, Any]: + """Read upload metadata with three-tier fallback. + + Resolution order: + 1. ``.upload_meta.json`` sidecar file + 2. SkillStore DB lookup by path + 3. Empty dict (caller applies defaults) + """ + # Tier 1: sidecar file + meta_path = skill_dir / _UPLOAD_META_FILENAME + if meta_path.exists(): + try: + data = json.loads(meta_path.read_text(encoding="utf-8")) + if data: + return data + except (json.JSONDecodeError, OSError) as e: + logger.warning(f"Failed to read upload metadata file: {e}") + + # Tier 2: DB lookup + try: + store = _get_store() + rec = store.load_record_by_path(str(skill_dir)) + if rec: + logger.debug(f"Upload metadata resolved from DB for {skill_dir}") + return { + "origin": rec.lineage.origin.value, + "parent_skill_ids": rec.lineage.parent_skill_ids, + "change_summary": rec.lineage.change_summary, + "created_by": rec.lineage.created_by or "", + "tags": rec.tags, + } + except Exception as e: + logger.debug(f"DB upload metadata lookup failed: {e}") + + return {} + + +# --------------------------------------------------------------------------- +# Auto-registration / cloud import helpers +# --------------------------------------------------------------------------- +async def _auto_register_skill_dirs(skill_dirs: List[str]) -> int: + """Register bot skill directories into OpenSpace's SkillRegistry + DB.""" + global _registered_skill_dirs + + valid_dirs = [Path(d) for d in skill_dirs if Path(d).is_dir()] + if not valid_dirs: + return 0 + + openspace = await _get_openspace() + registry = openspace._skill_registry + if not registry: + logger.warning("_auto_register_skill_dirs: SkillRegistry not initialized") + return 0 + + added = registry.discover_from_dirs(valid_dirs) + + db_created = 0 + if added: + store = _get_store() + db_created = await store.sync_from_registry(added) + + is_first = any(d not in _registered_skill_dirs for d in skill_dirs) + for d in skill_dirs: + _registered_skill_dirs.add(d) + + if added: + action = "Auto-registered" if is_first else "Re-scanned & found" + logger.info(f"{action} {len(added)} skill(s) from {len(valid_dirs)} dir(s), {db_created} new DB record(s)") + return len(added) + + +async def _cloud_search_and_import(task: str, limit: int = 8) -> List[Dict[str, Any]]: + """Search cloud for skills relevant to *task* and auto-import top hits. + + Stage 1 of two-stage pipeline: cloud BM25+embedding → import top-N locally. + Returns empty list when ``auto_import_enabled`` is ``False``. + """ + if not _is_auto_import_enabled(): + logger.debug("Cloud auto-import is disabled (auto_import_enabled=False)") + return [] + try: + from openspace.cloud.embedding import generate_embedding, resolve_embedding_api + from openspace.cloud.search import ( + SkillSearchEngine, + build_cloud_candidates, + ) + + client = _get_cloud_client() + embedding_api_key, _ = resolve_embedding_api() + has_embedding = bool(embedding_api_key) + + items = await asyncio.to_thread( + client.fetch_metadata, + include_embedding=has_embedding, + limit=200, + ) + if not items: + return [] + + candidates = build_cloud_candidates(items) + if not candidates: + return [] + + query_embedding: Optional[List[float]] = None + if has_embedding: + query_embedding = await asyncio.to_thread( + generate_embedding, + task, + ) + + engine = SkillSearchEngine() + results = engine.search(task, candidates, query_embedding=query_embedding, limit=limit * 2) + + cloud_hits = [ + r + for r in results + if r.get("source") == "cloud" and r.get("visibility", "public") == "public" and r.get("skill_id") + ][:limit] + + import_results: List[Dict[str, Any]] = [] + for hit in cloud_hits: + try: + imp = await _do_import_cloud_skill(skill_id=hit["skill_id"]) + import_results.append( + { + "skill_id": hit["skill_id"], + "name": hit.get("name", ""), + "import_status": imp.get("status", "error"), + "local_path": imp.get("local_path", ""), + } + ) + except Exception as e: + logger.warning(f"Cloud import failed for {hit['skill_id']}: {e}") + + if import_results: + logger.info(f"Cloud search imported {len(import_results)} skill(s)") + return import_results + + except Exception as e: + logger.warning(f"_cloud_search_and_import failed (non-fatal): {e}") + return [] + + +async def _do_import_cloud_skill(skill_id: str, target_dir: Optional[str] = None) -> Dict[str, Any]: + """Download a cloud skill and register it locally. + + Refuses to proceed when ``auto_import_enabled`` is ``False``. + """ + if not _is_auto_import_enabled(): + return {"status": "blocked", "reason": "auto_import_enabled is False"} + client = _get_cloud_client() + + if target_dir: + base_dir = Path(target_dir) + else: + host_ws = os.environ.get("NANOBOT_WORKSPACE") or os.environ.get("OPENCLAW_STATE_DIR") + if host_ws: + base_dir = Path(host_ws) / "skills" + base_dir.mkdir(parents=True, exist_ok=True) + else: + openspace = await _get_openspace() + skill_cfg = openspace._grounding_config.skills if openspace._grounding_config else None + if skill_cfg and skill_cfg.skill_dirs: + base_dir = Path(skill_cfg.skill_dirs[0]) + else: + base_dir = Path(__file__).resolve().parent.parent / "skills" + + result = await asyncio.to_thread(client.import_skill, skill_id, base_dir) + + skill_dir = Path(result.get("local_path", "")) + if skill_dir.exists(): + openspace = await _get_openspace() + registry = openspace._skill_registry + if registry: + meta = registry.register_skill_dir(skill_dir) + if meta: + store = _get_store() + await store.sync_from_registry([meta]) + result["registered"] = True + + result.setdefault("registered", False) + return result + + +# --------------------------------------------------------------------------- +# Result formatting +# --------------------------------------------------------------------------- +def _format_task_result(result: Dict[str, Any]) -> Dict[str, Any]: + """Format an OpenSpace execution result for MCP transport.""" + tool_execs = result.get("tool_executions", []) + tool_summary = [ + { + "tool": te.get("tool_name", te.get("tool", "")), + "status": te.get("status", ""), + "error": te.get("error", "")[:200] if te.get("error") else None, + } + for te in tool_execs[:20] + ] + + output: Dict[str, Any] = { + "status": result.get("status", "unknown"), + "response": result.get("response", ""), + "execution_time": round(result.get("execution_time", 0), 2), + "iterations": result.get("iterations", 0), + "skills_used": result.get("skills_used", []), + "task_id": result.get("task_id", ""), + "tool_call_count": len(tool_execs), + "tool_summary": tool_summary, + } + if result.get("warning"): + output["warning"] = result["warning"] + + # Format evolved_skills with skill_dir and upload_ready flag + raw_evolved = result.get("evolved_skills", []) + if raw_evolved: + formatted_evolved = [] + for es in raw_evolved: + skill_path = es.get("path", "") + skill_dir = str(Path(skill_path).parent) if skill_path else "" + formatted_evolved.append( + { + "skill_dir": skill_dir, + "name": es.get("name", ""), + "origin": es.get("origin", ""), + "change_summary": es.get("change_summary", ""), + "upload_ready": bool(skill_dir), + } + ) + output["evolved_skills"] = formatted_evolved + names = [es["name"] for es in formatted_evolved if es.get("upload_ready")] + if names: + output["action_required"] = ( + f"OpenSpace auto-evolved {len(names)} skill(s): {', '.join(names)}. " + f"Follow the 'When to upload' rules in your delegate-task skill to " + f"decide visibility, then upload via upload_skill. " + f"Tell the user what you evolved and what you uploaded." + ) + + return output + + +def _json_ok(data: Any) -> str: + return json.dumps(data, ensure_ascii=False, indent=2) + + +def _json_error(error_msg: str, *, error_code: str = "VALIDATION_ERROR") -> str: + """Return a structured error for validation / not-found cases.""" + from openspace.errors import safe_error_response + + return safe_error_response(error_code, error_msg) + + +# --------------------------------------------------------------------------- +# MCP Tool Handlers (4 tools) +# --------------------------------------------------------------------------- +async def execute_task( + task: str, + workspace_dir: str | None = None, + max_iterations: int | None = None, + skill_dirs: list[str] | None = None, + search_scope: str = "all", +) -> str: + """Execute a task with OpenSpace's full grounding engine. + + OpenSpace will: + 1. Auto-register bot skills from skill_dirs (if provided) + 2. Search for relevant skills (scope controls local vs cloud+local) + 3. Attempt skill-guided execution → fallback to pure tools + 4. Auto-analyze → auto-evolve (FIX/DERIVED/CAPTURED) if needed + + If skills are auto-evolved, the response includes ``evolved_skills`` + with ``upload_ready: true``. Call ``upload_skill`` with just the + ``skill_dir`` + ``visibility`` to upload — metadata is pre-saved. + + Note: This call blocks until the task completes (may take minutes). + Set MCP client tool-call timeout ≥ 600 seconds. + + Args: + task: The task instruction (natural language). + workspace_dir: Working directory. Defaults to OPENSPACE_WORKSPACE env. + max_iterations: Max agent iterations (default: 20). + skill_dirs: Bot's skill directories to auto-register so OpenSpace + can select and track them. Directories are re-scanned + on every call to discover skills created since the last + invocation. + search_scope: Skill search scope before execution. + "all" (default) — local + cloud; falls back to local + if no API key is configured. + "local" — local SkillRegistry only (fast, no cloud). + """ + try: + openspace = await _get_openspace() + + # Re-scan host skill directories (from env) + host_skill_dirs_raw = os.environ.get("OPENSPACE_HOST_SKILL_DIRS", "") + if host_skill_dirs_raw: + env_dirs = [d.strip() for d in host_skill_dirs_raw.split(",") if d.strip()] + if env_dirs: + await _auto_register_skill_dirs(env_dirs) + + # Auto-register bot skill directories (from call parameter) + if skill_dirs: + await _auto_register_skill_dirs(skill_dirs) + + # Cloud search + import (if requested) + imported_skills: List[Dict[str, Any]] = [] + if search_scope == "all": + imported_skills = await _cloud_search_and_import(task) + + # Execute + result = await openspace.execute( + task=task, + workspace_dir=workspace_dir, + max_iterations=max_iterations, + ) + + # Write .upload_meta.json for each evolved skill + for es in result.get("evolved_skills", []): + skill_path = es.get("path", "") + if skill_path: + _write_upload_meta(Path(skill_path).parent, es) + + formatted = _format_task_result(result) + if imported_skills: + formatted["imported_skills"] = imported_skills + return _json_ok(formatted) + + except Exception as e: + from openspace.errors import EXECUTION_ERROR, handle_mcp_exception + + return handle_mcp_exception(e, tool_name="execute_task", error_code=EXECUTION_ERROR) + + +async def search_skills( + query: str, + source: str = "all", + limit: int = 20, + auto_import: bool = True, +) -> str: + """Search skills across local registry and cloud community. + + Standalone search for browsing / discovery. Use this when the bot + wants to find available skills, then decide whether to handle the + task locally or delegate to ``execute_task``. + + **Scope difference from execute_task**: + - ``search_skills`` returns results to the bot for decision-making. + - ``execute_task``'s internal search feeds directly into execution + (the bot never sees the search results). + + Uses hybrid ranking: BM25 → embedding re-rank → lexical boost. + Embedding requires OPENAI_API_KEY; falls back to lexical-only without it. + + Args: + query: Search query text (natural language or keywords). + source: "all" (cloud + local), "local", or "cloud". Default: "all". + limit: Maximum results to return (default: 20). + auto_import: Auto-download top public cloud skills (default: True). + """ + try: + from openspace.cloud.search import hybrid_search_skills + + q = query.strip() + if not q: + return _json_ok({"results": [], "count": 0}) + + # Re-scan host skill directories so newly created skills are searchable. + local_skills = None + store = None + if source in ("all", "local"): + openspace = await _get_openspace() + + host_skill_dirs_raw = os.environ.get("OPENSPACE_HOST_SKILL_DIRS", "") + if host_skill_dirs_raw: + env_dirs = [d.strip() for d in host_skill_dirs_raw.split(",") if d.strip()] + if env_dirs: + await _auto_register_skill_dirs(env_dirs) + + registry = openspace._skill_registry + if registry: + local_skills = registry.list_skills() + store = _get_store() + + results = await hybrid_search_skills( + query=q, + local_skills=local_skills, + store=store, + source=source, + limit=limit, + ) + + _AUTO_IMPORT_MAX = 3 + import_summary: List[Dict[str, Any]] = [] + if auto_import and _is_auto_import_enabled(): + cloud_results = [ + r + for r in results + if r.get("source") == "cloud" and r.get("visibility", "public") == "public" and r.get("skill_id") + ][:_AUTO_IMPORT_MAX] + for cr in cloud_results: + try: + imp_result = await _do_import_cloud_skill(skill_id=cr["skill_id"]) + status = imp_result.get("status", "error") + import_summary.append( + { + "skill_id": cr["skill_id"], + "name": cr.get("name", ""), + "import_status": status, + "local_path": imp_result.get("local_path", ""), + } + ) + if status in ("success", "already_exists"): + cr["auto_imported"] = True + cr["local_path"] = imp_result.get("local_path", "") + except Exception as imp_err: + logger.warning(f"auto_import failed for {cr['skill_id']}: {imp_err}", exc_info=True) + import_summary.append( + { + "skill_id": cr["skill_id"], + "import_status": "error", + "error": "Cloud skill import failed", + } + ) + + output: Dict[str, Any] = {"results": results, "count": len(results)} + if import_summary: + output["auto_import_summary"] = import_summary + return _json_ok(output) + + except Exception as e: + from openspace.errors import EXECUTION_ERROR, handle_mcp_exception + + return handle_mcp_exception(e, tool_name="search_skills", error_code=EXECUTION_ERROR) + + +async def fix_skill( + skill_dir: str, + direction: str, +) -> str: + """Manually fix a broken skill. + + This is the **only** manual evolution entry point. DERIVED and + CAPTURED evolutions are triggered automatically by ``execute_task`` + (they need a task to run). Use ``fix_skill`` when: + + - A skill's instructions are wrong or outdated + - The bot knows exactly which skill is broken and what to fix + - Auto-evolution inside ``execute_task`` didn't catch the issue + + The skill does NOT need to be pre-registered in OpenSpace — + provide the skill directory path and OpenSpace will register it + automatically before fixing. + + After fixing, the new skill is saved locally and ``.upload_meta.json`` + is pre-written. Call ``upload_skill`` with just ``skill_dir`` + + ``visibility`` to upload. + + Args: + skill_dir: Path to the broken skill directory (must contain SKILL.md). + direction: What's broken and how to fix it. Be specific: + e.g. "The API endpoint changed from v1 to v2" or + "Add retry logic for HTTP 429 rate limit errors". + """ + try: + from openspace.skill_engine.evolver import EvolutionContext, EvolutionTrigger + from openspace.skill_engine.types import EvolutionSuggestion, EvolutionType + + if not direction: + return _json_error("direction is required — describe what to fix.") + + skill_path = Path(skill_dir) + skill_md = skill_path / "SKILL.md" + if not skill_md.exists(): + return _json_error("SKILL.md not found in the specified skill directory", error_code="SKILL_NOT_FOUND") + + openspace = await _get_openspace() + registry = openspace._skill_registry + if not registry: + return _json_error("SkillRegistry not initialized", error_code="INTERNAL_ERROR") + if not openspace._skill_evolver: + return _json_error("Skill evolution is not enabled", error_code="INTERNAL_ERROR") + + # Step 1: Register the skill (idempotent) + meta = registry.register_skill_dir(skill_path) + if not meta: + return _json_error("Failed to register skill from the specified directory", error_code="SKILL_NOT_FOUND") + + store = _get_store() + await store.sync_from_registry([meta]) + + # Step 2: Load record + content + rec = store.load_record(meta.skill_id) + if not rec: + return _json_error("Failed to load skill record", error_code="SKILL_NOT_FOUND") + + evolver = openspace._skill_evolver + content = evolver._load_skill_content(rec) + if not content: + return _json_error("Cannot load content for the specified skill", error_code="SKILL_NOT_FOUND") + + # Step 3: Run FIX evolution + recent = store.load_analyses(skill_id=meta.skill_id, limit=5) + + ctx = EvolutionContext( + trigger=EvolutionTrigger.ANALYSIS, + suggestion=EvolutionSuggestion( + evolution_type=EvolutionType.FIX, + target_skill_ids=[meta.skill_id], + direction=direction, + ), + skill_records=[rec], + skill_contents=[content], + skill_dirs=[skill_path], + recent_analyses=recent, + available_tools=evolver._available_tools, + ) + + logger.info(f"fix_skill: {meta.skill_id} — {direction[:100]}") + new_record = await evolver.evolve(ctx) + + if not new_record: + return _json_ok( + { + "status": "failed", + "error": "Evolution did not produce a new skill.", + } + ) + + # Step 4: Write .upload_meta.json + new_skill_dir = Path(new_record.path).parent if new_record.path else skill_path + _write_upload_meta( + new_skill_dir, + { + "origin": new_record.lineage.origin.value, + "parent_skill_ids": new_record.lineage.parent_skill_ids, + "change_summary": new_record.lineage.change_summary, + "created_by": new_record.lineage.created_by or "openspace", + "tags": new_record.tags, + }, + ) + + return _json_ok( + { + "status": "success", + "new_skill": { + "skill_dir": str(new_skill_dir), + "name": new_record.name, + "origin": new_record.lineage.origin.value, + "change_summary": new_record.lineage.change_summary, + "upload_ready": True, + }, + } + ) + + except Exception as e: + from openspace.errors import EXECUTION_ERROR, handle_mcp_exception + + return handle_mcp_exception(e, tool_name="fix_skill", error_code=EXECUTION_ERROR) + + +async def upload_skill( + skill_dir: str, + visibility: str = "public", + origin: str | None = None, + parent_skill_ids: list[str] | None = None, + tags: list[str] | None = None, + created_by: str | None = None, + change_summary: str | None = None, +) -> str: + """Upload a local skill to the cloud. + + For evolved skills (from ``execute_task`` or ``fix_skill``), most + metadata is **pre-saved** in ``.upload_meta.json``. The bot only + needs to provide: + + - ``skill_dir`` — path to the skill directory + - ``visibility`` — "public" or "private" + + All other parameters are optional overrides. If omitted, pre-saved + values are used. If no pre-saved values exist, sensible defaults + are applied. + + **origin + parent_skill_ids constraints** (enforced by cloud): + - imported / captured → parent_skill_ids must be empty + - derived → at least 1 parent + - fixed → exactly 1 parent + + Args: + skill_dir: Path to skill directory (must contain SKILL.md). + visibility: "public" or "private". This is the one thing the + bot MUST decide. + origin: Override origin. Default: from .upload_meta.json or "imported". + parent_skill_ids: Override parents. Default: from .upload_meta.json. + tags: Override tags. Default: from .upload_meta.json. + created_by: Override creator. Default: from .upload_meta.json. + change_summary: Override summary. Default: from .upload_meta.json. + """ + try: + skill_path = Path(skill_dir) + if not (skill_path / "SKILL.md").exists(): + return _json_error("SKILL.md not found in the specified skill directory", error_code="SKILL_NOT_FOUND") + + # Read pre-saved metadata (written by execute_task/fix_skill) + meta = _read_upload_meta(skill_path) + + # Merge: explicit params override pre-saved values + final_origin = origin if origin is not None else meta.get("origin", "imported") + final_parents = parent_skill_ids if parent_skill_ids is not None else meta.get("parent_skill_ids", []) + final_tags = tags if tags is not None else meta.get("tags", []) + final_created_by = created_by if created_by is not None else meta.get("created_by", "") + final_change_summary = change_summary if change_summary is not None else meta.get("change_summary", "") + + client = _get_cloud_client() + result = await asyncio.to_thread( + client.upload_skill, + skill_path, + visibility=visibility, + origin=final_origin, + parent_skill_ids=final_parents, + tags=final_tags, + created_by=final_created_by, + change_summary=final_change_summary, + ) + return _json_ok(result) + + except Exception as e: + from openspace.errors import EXECUTION_ERROR, handle_mcp_exception + + return handle_mcp_exception(e, tool_name="upload_skill", error_code=EXECUTION_ERROR) + + +# --------------------------------------------------------------------------- +# Registration +# --------------------------------------------------------------------------- +def register_handlers(mcp) -> None: + """Wire all tool handlers to the given FastMCP instance. + + Called by ``mcp_server.py`` after creating the FastMCP app. + """ + mcp.tool()(execute_task) + mcp.tool()(search_skills) + mcp.tool()(fix_skill) + mcp.tool()(upload_skill) diff --git a/openspace/mcp_server.py b/openspace/mcp_server.py index aa09a5d6..0291f04f 100644 --- a/openspace/mcp_server.py +++ b/openspace/mcp_server.py @@ -16,15 +16,12 @@ from __future__ import annotations -import asyncio import atexit import inspect -import json import logging import os import sys from pathlib import Path -from typing import Any, Dict, List, Optional class _MCPSafeStdout: @@ -130,779 +127,10 @@ def _cleanup_file_handles(): mcp = FastMCP("OpenSpace", **_fastmcp_kwargs) -_openspace_instance = None -_openspace_lock = asyncio.Lock() -_standalone_store = None +# Register tool handlers from extracted module (Epic 4.7) +from openspace.mcp.tool_handlers import register_handlers -# Internal state: tracks bot skill directories already registered this session. -_registered_skill_dirs: set = set() - -_UPLOAD_META_FILENAME = ".upload_meta.json" - - -async def _get_openspace(): - """Lazy-initialise the OpenSpace engine.""" - global _openspace_instance - if _openspace_instance is not None and _openspace_instance.is_initialized(): - return _openspace_instance - - async with _openspace_lock: - if _openspace_instance is not None and _openspace_instance.is_initialized(): - return _openspace_instance - - logger.info("Initializing OpenSpace engine ...") - from openspace.host_detection import build_grounding_config_path, build_llm_kwargs - from openspace.tool_layer import OpenSpace, OpenSpaceConfig - - env_model = os.environ.get("OPENSPACE_MODEL", "") - workspace = os.environ.get("OPENSPACE_WORKSPACE") - max_iter = int(os.environ.get("OPENSPACE_MAX_ITERATIONS", "20")) - enable_rec = os.environ.get("OPENSPACE_ENABLE_RECORDING", "true").lower() in ("true", "1", "yes") - - backend_scope_raw = os.environ.get("OPENSPACE_BACKEND_SCOPE") - backend_scope = [b.strip() for b in backend_scope_raw.split(",") if b.strip()] if backend_scope_raw else None - - config_path = build_grounding_config_path() - model, llm_kwargs = build_llm_kwargs(env_model) - - _pkg_root = str(Path(__file__).resolve().parent.parent) - recording_base = workspace or _pkg_root - recording_log_dir = str(Path(recording_base) / "logs" / "recordings") - - config = OpenSpaceConfig( - llm_model=model, - llm_kwargs=llm_kwargs, - workspace_dir=workspace, - grounding_max_iterations=max_iter, - enable_recording=enable_rec, - recording_backends=["shell"] if enable_rec else None, # ["shell", "mcp", "web"] if enable_rec else None - recording_log_dir=recording_log_dir, - backend_scope=backend_scope, - grounding_config_path=config_path, - ) - - _openspace_instance = OpenSpace(config=config) - await _openspace_instance.initialize() - logger.info("OpenSpace engine ready (model=%s).", model) - - # Auto-register host bot skill directories from env (set once by human) - host_skill_dirs_raw = os.environ.get("OPENSPACE_HOST_SKILL_DIRS", "") - if host_skill_dirs_raw: - dirs = [d.strip() for d in host_skill_dirs_raw.split(",") if d.strip()] - if dirs: - await _auto_register_skill_dirs(dirs) - logger.info("Auto-registered host skill dirs from OPENSPACE_HOST_SKILL_DIRS: %s", dirs) - - return _openspace_instance - - -def _get_store(): - """Get SkillStore — reuses OpenSpace's internal instance when available.""" - global _standalone_store - if _openspace_instance and _openspace_instance.is_initialized(): - internal = getattr(_openspace_instance, "_skill_store", None) - if internal and not internal._closed: - return internal - if _standalone_store is None or _standalone_store._closed: - from openspace.skill_engine import SkillStore - - _standalone_store = SkillStore() - return _standalone_store - - -def _is_auto_import_enabled() -> bool: - """Check whether cloud auto-import is enabled in SkillConfig. - - Returns ``False`` (safe default) if the config is unavailable or the - flag is not explicitly set to ``True``. This ensures that untrusted - cloud skills are never imported without an explicit opt-in. - """ - if _openspace_instance and _openspace_instance.is_initialized(): - gc = getattr(_openspace_instance, "_grounding_config", None) - if gc and gc.skills: - return gc.skills.auto_import_enabled - return False - - -def _get_cloud_client(): - """Get a OpenSpaceClient instance (raises CloudError if not configured).""" - from openspace.cloud.auth import get_openspace_auth - from openspace.cloud.client import OpenSpaceClient - - auth_headers, api_base = get_openspace_auth() - return OpenSpaceClient(auth_headers, api_base) - - -def _write_upload_meta(skill_dir: Path, info: Dict[str, Any]) -> None: - """Write ``.upload_meta.json`` so ``upload_skill`` can read pre-saved metadata. - - Called after evolution (execute_task auto-evolve or fix_skill). - The bot then only needs to provide ``skill_dir`` + ``visibility`` - when uploading — everything else is pre-filled. - """ - meta = { - "origin": info.get("origin", "imported"), - "parent_skill_ids": info.get("parent_skill_ids", []), - "change_summary": info.get("change_summary", ""), - "created_by": info.get("created_by", "openspace"), - "tags": info.get("tags", []), - } - meta_path = skill_dir / _UPLOAD_META_FILENAME - try: - meta_path.write_text( - json.dumps(meta, ensure_ascii=False, indent=2) + "\n", - encoding="utf-8", - ) - logger.debug(f"Wrote upload metadata to {meta_path}") - except Exception as e: - logger.warning(f"Failed to write upload metadata: {e}") - - -def _read_upload_meta(skill_dir: Path) -> Dict[str, Any]: - """Read upload metadata with three-tier fallback. - - Resolution order: - 1. ``.upload_meta.json`` sidecar file (written right after evolution) - 2. SkillStore DB lookup by path (long-term persistence) - 3. Empty dict (caller applies defaults) - - This ensures metadata survives even if the sidecar file is deleted - or the user comes back to upload much later. - """ - # Tier 1: sidecar file - meta_path = skill_dir / _UPLOAD_META_FILENAME - if meta_path.exists(): - try: - data = json.loads(meta_path.read_text(encoding="utf-8")) - if data: - return data - except (json.JSONDecodeError, OSError) as e: - logger.warning(f"Failed to read upload metadata file: {e}") - - # Tier 2: DB lookup - try: - store = _get_store() - rec = store.load_record_by_path(str(skill_dir)) - if rec: - logger.debug(f"Upload metadata resolved from DB for {skill_dir}") - return { - "origin": rec.lineage.origin.value, - "parent_skill_ids": rec.lineage.parent_skill_ids, - "change_summary": rec.lineage.change_summary, - "created_by": rec.lineage.created_by or "", - "tags": rec.tags, - } - except Exception as e: - logger.debug(f"DB upload metadata lookup failed: {e}") - - return {} - - -async def _auto_register_skill_dirs(skill_dirs: List[str]) -> int: - """Register bot skill directories into OpenSpace's SkillRegistry + DB. - - Called automatically by ``execute_task`` on every invocation. Directories - are re-scanned each time so that skills created by the host bot since the last call are discovered immediately. - """ - global _registered_skill_dirs - - valid_dirs = [Path(d) for d in skill_dirs if Path(d).is_dir()] - if not valid_dirs: - return 0 - - openspace = await _get_openspace() - registry = openspace._skill_registry - if not registry: - logger.warning("_auto_register_skill_dirs: SkillRegistry not initialized") - return 0 - - added = registry.discover_from_dirs(valid_dirs) - - db_created = 0 - if added: - store = _get_store() - db_created = await store.sync_from_registry(added) - - is_first = any(d not in _registered_skill_dirs for d in skill_dirs) - for d in skill_dirs: - _registered_skill_dirs.add(d) - - if added: - action = "Auto-registered" if is_first else "Re-scanned & found" - logger.info(f"{action} {len(added)} skill(s) from {len(valid_dirs)} dir(s), {db_created} new DB record(s)") - return len(added) - - -async def _cloud_search_and_import(task: str, limit: int = 8) -> List[Dict[str, Any]]: - """Search cloud for skills relevant to *task* and auto-import top hits. - - This is **stage 1** of a two-stage pipeline: - Stage 1 (here): cloud BM25+embedding → pick top-N to import locally. - Stage 2 (tool_layer): local BM25 + LLM → select from ALL local skills - (including ones just imported) for injection. - - Stage 1 intentionally imports more than will be used (default: 8) so - that stage 2 has a larger pool to choose from. The two BM25 passes - are NOT redundant — stage 1 filters thousands of cloud candidates down - to a manageable import set; stage 2 makes the final task-specific choice. - - Returns an empty list immediately when ``auto_import_enabled`` is - ``False`` (the default) — untrusted cloud code is never imported - without an explicit opt-in. - """ - if not _is_auto_import_enabled(): - logger.debug("Cloud auto-import is disabled (auto_import_enabled=False)") - return [] - try: - from openspace.cloud.embedding import generate_embedding, resolve_embedding_api - from openspace.cloud.search import ( - SkillSearchEngine, - build_cloud_candidates, - ) - - client = _get_cloud_client() - embedding_api_key, _ = resolve_embedding_api() - has_embedding = bool(embedding_api_key) - - items = await asyncio.to_thread( - client.fetch_metadata, - include_embedding=has_embedding, - limit=200, - ) - if not items: - return [] - - candidates = build_cloud_candidates(items) - if not candidates: - return [] - - query_embedding: Optional[List[float]] = None - if has_embedding: - query_embedding = await asyncio.to_thread( - generate_embedding, - task, - ) - - engine = SkillSearchEngine() - results = engine.search(task, candidates, query_embedding=query_embedding, limit=limit * 2) - - cloud_hits = [ - r - for r in results - if r.get("source") == "cloud" and r.get("visibility", "public") == "public" and r.get("skill_id") - ][:limit] - - import_results: List[Dict[str, Any]] = [] - for hit in cloud_hits: - try: - imp = await _do_import_cloud_skill(skill_id=hit["skill_id"]) - import_results.append( - { - "skill_id": hit["skill_id"], - "name": hit.get("name", ""), - "import_status": imp.get("status", "error"), - "local_path": imp.get("local_path", ""), - } - ) - except Exception as e: - logger.warning(f"Cloud import failed for {hit['skill_id']}: {e}") - - if import_results: - logger.info(f"Cloud search imported {len(import_results)} skill(s)") - return import_results - - except Exception as e: - logger.warning(f"_cloud_search_and_import failed (non-fatal): {e}") - return [] - - -async def _do_import_cloud_skill(skill_id: str, target_dir: Optional[str] = None) -> Dict[str, Any]: - """Download a cloud skill and register it locally. - - Refuses to proceed when ``auto_import_enabled`` is ``False``. - """ - if not _is_auto_import_enabled(): - return {"status": "blocked", "reason": "auto_import_enabled is False"} - client = _get_cloud_client() - - if target_dir: - base_dir = Path(target_dir) - else: - host_ws = os.environ.get("NANOBOT_WORKSPACE") or os.environ.get("OPENCLAW_STATE_DIR") - if host_ws: - base_dir = Path(host_ws) / "skills" - base_dir.mkdir(parents=True, exist_ok=True) - else: - openspace = await _get_openspace() - skill_cfg = openspace._grounding_config.skills if openspace._grounding_config else None - if skill_cfg and skill_cfg.skill_dirs: - base_dir = Path(skill_cfg.skill_dirs[0]) - else: - base_dir = Path(__file__).resolve().parent / "skills" - - result = await asyncio.to_thread(client.import_skill, skill_id, base_dir) - - skill_dir = Path(result.get("local_path", "")) - if skill_dir.exists(): - openspace = await _get_openspace() - registry = openspace._skill_registry - if registry: - meta = registry.register_skill_dir(skill_dir) - if meta: - store = _get_store() - await store.sync_from_registry([meta]) - result["registered"] = True - - result.setdefault("registered", False) - return result - - -def _format_task_result(result: Dict[str, Any]) -> Dict[str, Any]: - """Format an OpenSpace execution result for MCP transport.""" - tool_execs = result.get("tool_executions", []) - tool_summary = [ - { - "tool": te.get("tool_name", te.get("tool", "")), - "status": te.get("status", ""), - "error": te.get("error", "")[:200] if te.get("error") else None, - } - for te in tool_execs[:20] - ] - - output: Dict[str, Any] = { - "status": result.get("status", "unknown"), - "response": result.get("response", ""), - "execution_time": round(result.get("execution_time", 0), 2), - "iterations": result.get("iterations", 0), - "skills_used": result.get("skills_used", []), - "task_id": result.get("task_id", ""), - "tool_call_count": len(tool_execs), - "tool_summary": tool_summary, - } - if result.get("warning"): - output["warning"] = result["warning"] - - # Format evolved_skills with skill_dir and upload_ready flag - raw_evolved = result.get("evolved_skills", []) - if raw_evolved: - formatted_evolved = [] - for es in raw_evolved: - skill_path = es.get("path", "") - skill_dir = str(Path(skill_path).parent) if skill_path else "" - formatted_evolved.append( - { - "skill_dir": skill_dir, - "name": es.get("name", ""), - "origin": es.get("origin", ""), - "change_summary": es.get("change_summary", ""), - "upload_ready": bool(skill_dir), - } - ) - output["evolved_skills"] = formatted_evolved - # Prompt the calling agent to upload evolved skills - names = [es["name"] for es in formatted_evolved if es.get("upload_ready")] - if names: - output["action_required"] = ( - f"OpenSpace auto-evolved {len(names)} skill(s): {', '.join(names)}. " - f"Follow the 'When to upload' rules in your delegate-task skill to " - f"decide visibility, then upload via upload_skill. " - f"Tell the user what you evolved and what you uploaded." - ) - - return output - - -def _json_ok(data: Any) -> str: - return json.dumps(data, ensure_ascii=False, indent=2) - - -def _json_error(error_msg: str, *, error_code: str = "VALIDATION_ERROR") -> str: - """Return a structured error for validation / not-found cases.""" - from openspace.errors import safe_error_response - - return safe_error_response(error_code, error_msg) - - -# MCP Tools (4 tools) -@mcp.tool() -async def execute_task( - task: str, - workspace_dir: str | None = None, - max_iterations: int | None = None, - skill_dirs: list[str] | None = None, - search_scope: str = "all", -) -> str: - """Execute a task with OpenSpace's full grounding engine. - - OpenSpace will: - 1. Auto-register bot skills from skill_dirs (if provided) - 2. Search for relevant skills (scope controls local vs cloud+local) - 3. Attempt skill-guided execution → fallback to pure tools - 4. Auto-analyze → auto-evolve (FIX/DERIVED/CAPTURED) if needed - - If skills are auto-evolved, the response includes ``evolved_skills`` - with ``upload_ready: true``. Call ``upload_skill`` with just the - ``skill_dir`` + ``visibility`` to upload — metadata is pre-saved. - - Note: This call blocks until the task completes (may take minutes). - Set MCP client tool-call timeout ≥ 600 seconds. - - Args: - task: The task instruction (natural language). - workspace_dir: Working directory. Defaults to OPENSPACE_WORKSPACE env. - max_iterations: Max agent iterations (default: 20). - skill_dirs: Bot's skill directories to auto-register so OpenSpace - can select and track them. Directories are re-scanned - on every call to discover skills created since the last - invocation. - search_scope: Skill search scope before execution. - "all" (default) — local + cloud; falls back to local - if no API key is configured. - "local" — local SkillRegistry only (fast, no cloud). - """ - try: - openspace = await _get_openspace() - - # Re-scan host skill directories (from env) to pick up skills - # created by the host bot since the last call. - host_skill_dirs_raw = os.environ.get("OPENSPACE_HOST_SKILL_DIRS", "") - if host_skill_dirs_raw: - env_dirs = [d.strip() for d in host_skill_dirs_raw.split(",") if d.strip()] - if env_dirs: - await _auto_register_skill_dirs(env_dirs) - - # Auto-register bot skill directories (from call parameter) - if skill_dirs: - await _auto_register_skill_dirs(skill_dirs) - - # Cloud search + import (if requested) - imported_skills: List[Dict[str, Any]] = [] - if search_scope == "all": - imported_skills = await _cloud_search_and_import(task) - - # Execute - result = await openspace.execute( - task=task, - workspace_dir=workspace_dir, - max_iterations=max_iterations, - ) - - # Write .upload_meta.json for each evolved skill - for es in result.get("evolved_skills", []): - skill_path = es.get("path", "") - if skill_path: - _write_upload_meta(Path(skill_path).parent, es) - - formatted = _format_task_result(result) - if imported_skills: - formatted["imported_skills"] = imported_skills - return _json_ok(formatted) - - except Exception as e: - from openspace.errors import EXECUTION_ERROR, handle_mcp_exception - - return handle_mcp_exception(e, tool_name="execute_task", error_code=EXECUTION_ERROR) - - -@mcp.tool() -async def search_skills( - query: str, - source: str = "all", - limit: int = 20, - auto_import: bool = True, -) -> str: - """Search skills across local registry and cloud community. - - Standalone search for browsing / discovery. Use this when the bot - wants to find available skills, then decide whether to handle the - task locally or delegate to ``execute_task``. - - **Scope difference from execute_task**: - - ``search_skills`` returns results to the bot for decision-making. - - ``execute_task``'s internal search feeds directly into execution - (the bot never sees the search results). - - Uses hybrid ranking: BM25 → embedding re-rank → lexical boost. - Embedding requires OPENAI_API_KEY; falls back to lexical-only without it. - - Args: - query: Search query text (natural language or keywords). - source: "all" (cloud + local), "local", or "cloud". Default: "all". - limit: Maximum results to return (default: 20). - auto_import: Auto-download top public cloud skills (default: True). - """ - try: - from openspace.cloud.search import hybrid_search_skills - - q = query.strip() - if not q: - return _json_ok({"results": [], "count": 0}) - - # Re-scan host skill directories so newly created skills are searchable. - local_skills = None - store = None - if source in ("all", "local"): - openspace = await _get_openspace() - - host_skill_dirs_raw = os.environ.get("OPENSPACE_HOST_SKILL_DIRS", "") - if host_skill_dirs_raw: - env_dirs = [d.strip() for d in host_skill_dirs_raw.split(",") if d.strip()] - if env_dirs: - await _auto_register_skill_dirs(env_dirs) - - registry = openspace._skill_registry - if registry: - local_skills = registry.list_skills() - store = _get_store() - - results = await hybrid_search_skills( - query=q, - local_skills=local_skills, - store=store, - source=source, - limit=limit, - ) - - _AUTO_IMPORT_MAX = 3 - import_summary: List[Dict[str, Any]] = [] - if auto_import and _is_auto_import_enabled(): - cloud_results = [ - r - for r in results - if r.get("source") == "cloud" and r.get("visibility", "public") == "public" and r.get("skill_id") - ][:_AUTO_IMPORT_MAX] - for cr in cloud_results: - try: - imp_result = await _do_import_cloud_skill(skill_id=cr["skill_id"]) - status = imp_result.get("status", "error") - import_summary.append( - { - "skill_id": cr["skill_id"], - "name": cr.get("name", ""), - "import_status": status, - "local_path": imp_result.get("local_path", ""), - } - ) - if status in ("success", "already_exists"): - cr["auto_imported"] = True - cr["local_path"] = imp_result.get("local_path", "") - except Exception as imp_err: - logger.warning(f"auto_import failed for {cr['skill_id']}: {imp_err}", exc_info=True) - import_summary.append( - { - "skill_id": cr["skill_id"], - "import_status": "error", - "error": "Cloud skill import failed", - } - ) - - output: Dict[str, Any] = {"results": results, "count": len(results)} - if import_summary: - output["auto_import_summary"] = import_summary - return _json_ok(output) - - except Exception as e: - from openspace.errors import EXECUTION_ERROR, handle_mcp_exception - - return handle_mcp_exception(e, tool_name="search_skills", error_code=EXECUTION_ERROR) - - -@mcp.tool() -async def fix_skill( - skill_dir: str, - direction: str, -) -> str: - """Manually fix a broken skill. - - This is the **only** manual evolution entry point. DERIVED and - CAPTURED evolutions are triggered automatically by ``execute_task`` - (they need a task to run). Use ``fix_skill`` when: - - - A skill's instructions are wrong or outdated - - The bot knows exactly which skill is broken and what to fix - - Auto-evolution inside ``execute_task`` didn't catch the issue - - The skill does NOT need to be pre-registered in OpenSpace — - provide the skill directory path and OpenSpace will register it - automatically before fixing. - - After fixing, the new skill is saved locally and ``.upload_meta.json`` - is pre-written. Call ``upload_skill`` with just ``skill_dir`` + - ``visibility`` to upload. - - Args: - skill_dir: Path to the broken skill directory (must contain SKILL.md). - direction: What's broken and how to fix it. Be specific: - e.g. "The API endpoint changed from v1 to v2" or - "Add retry logic for HTTP 429 rate limit errors". - """ - try: - from openspace.skill_engine.evolver import EvolutionContext, EvolutionTrigger - from openspace.skill_engine.types import EvolutionSuggestion, EvolutionType - - if not direction: - return _json_error("direction is required — describe what to fix.") - - skill_path = Path(skill_dir) - skill_md = skill_path / "SKILL.md" - if not skill_md.exists(): - return _json_error("SKILL.md not found in the specified skill directory", error_code="SKILL_NOT_FOUND") - - openspace = await _get_openspace() - registry = openspace._skill_registry - if not registry: - return _json_error("SkillRegistry not initialized", error_code="INTERNAL_ERROR") - if not openspace._skill_evolver: - return _json_error("Skill evolution is not enabled", error_code="INTERNAL_ERROR") - - # Step 1: Register the skill (idempotent) - meta = registry.register_skill_dir(skill_path) - if not meta: - return _json_error("Failed to register skill from the specified directory", error_code="SKILL_NOT_FOUND") - - store = _get_store() - await store.sync_from_registry([meta]) - - # Step 2: Load record + content - rec = store.load_record(meta.skill_id) - if not rec: - return _json_error("Failed to load skill record", error_code="SKILL_NOT_FOUND") - - evolver = openspace._skill_evolver - content = evolver._load_skill_content(rec) - if not content: - return _json_error("Cannot load content for the specified skill", error_code="SKILL_NOT_FOUND") - - # Step 3: Run FIX evolution - recent = store.load_analyses(skill_id=meta.skill_id, limit=5) - - ctx = EvolutionContext( - trigger=EvolutionTrigger.ANALYSIS, - suggestion=EvolutionSuggestion( - evolution_type=EvolutionType.FIX, - target_skill_ids=[meta.skill_id], - direction=direction, - ), - skill_records=[rec], - skill_contents=[content], - skill_dirs=[skill_path], - recent_analyses=recent, - available_tools=evolver._available_tools, - ) - - logger.info(f"fix_skill: {meta.skill_id} — {direction[:100]}") - new_record = await evolver.evolve(ctx) - - if not new_record: - return _json_ok( - { - "status": "failed", - "error": "Evolution did not produce a new skill.", - } - ) - - # Step 4: Write .upload_meta.json - new_skill_dir = Path(new_record.path).parent if new_record.path else skill_path - _write_upload_meta( - new_skill_dir, - { - "origin": new_record.lineage.origin.value, - "parent_skill_ids": new_record.lineage.parent_skill_ids, - "change_summary": new_record.lineage.change_summary, - "created_by": new_record.lineage.created_by or "openspace", - "tags": new_record.tags, - }, - ) - - return _json_ok( - { - "status": "success", - "new_skill": { - "skill_dir": str(new_skill_dir), - "name": new_record.name, - "origin": new_record.lineage.origin.value, - "change_summary": new_record.lineage.change_summary, - "upload_ready": True, - }, - } - ) - - except Exception as e: - from openspace.errors import EXECUTION_ERROR, handle_mcp_exception - - return handle_mcp_exception(e, tool_name="fix_skill", error_code=EXECUTION_ERROR) - - -@mcp.tool() -async def upload_skill( - skill_dir: str, - visibility: str = "public", - origin: str | None = None, - parent_skill_ids: list[str] | None = None, - tags: list[str] | None = None, - created_by: str | None = None, - change_summary: str | None = None, -) -> str: - """Upload a local skill to the cloud. - - For evolved skills (from ``execute_task`` or ``fix_skill``), most - metadata is **pre-saved** in ``.upload_meta.json``. The bot only - needs to provide: - - - ``skill_dir`` — path to the skill directory - - ``visibility`` — "public" or "private" - - All other parameters are optional overrides. If omitted, pre-saved - values are used. If no pre-saved values exist, sensible defaults - are applied. - - **origin + parent_skill_ids constraints** (enforced by cloud): - - imported / captured → parent_skill_ids must be empty - - derived → at least 1 parent - - fixed → exactly 1 parent - - Args: - skill_dir: Path to skill directory (must contain SKILL.md). - visibility: "public" or "private". This is the one thing the - bot MUST decide. - origin: Override origin. Default: from .upload_meta.json or "imported". - parent_skill_ids: Override parents. Default: from .upload_meta.json. - tags: Override tags. Default: from .upload_meta.json. - created_by: Override creator. Default: from .upload_meta.json. - change_summary: Override summary. Default: from .upload_meta.json. - """ - try: - skill_path = Path(skill_dir) - if not (skill_path / "SKILL.md").exists(): - return _json_error("SKILL.md not found in the specified skill directory", error_code="SKILL_NOT_FOUND") - - # Read pre-saved metadata (written by execute_task/fix_skill) - meta = _read_upload_meta(skill_path) - - # Merge: explicit params override pre-saved values - final_origin = origin if origin is not None else meta.get("origin", "imported") - final_parents = parent_skill_ids if parent_skill_ids is not None else meta.get("parent_skill_ids", []) - final_tags = tags if tags is not None else meta.get("tags", []) - final_created_by = created_by if created_by is not None else meta.get("created_by", "") - final_change_summary = change_summary if change_summary is not None else meta.get("change_summary", "") - - client = _get_cloud_client() - result = await asyncio.to_thread( - client.upload_skill, - skill_path, - visibility=visibility, - origin=final_origin, - parent_skill_ids=final_parents, - tags=final_tags, - created_by=final_created_by, - change_summary=final_change_summary, - ) - return _json_ok(result) - - except Exception as e: - from openspace.errors import EXECUTION_ERROR, handle_mcp_exception - - return handle_mcp_exception(e, tool_name="upload_skill", error_code=EXECUTION_ERROR) +register_handlers(mcp) def run_mcp_server() -> None: diff --git a/tests/test_auto_import_disabled.py b/tests/test_auto_import_disabled.py index 930625d8..6ce69404 100644 --- a/tests/test_auto_import_disabled.py +++ b/tests/test_auto_import_disabled.py @@ -56,7 +56,7 @@ class TestIsAutoImportEnabled: """_is_auto_import_enabled() must reflect SkillConfig state.""" def test_returns_false_when_no_instance(self): - import openspace.mcp_server as srv + import openspace.mcp.tool_handlers as srv original = srv._openspace_instance try: @@ -66,7 +66,7 @@ def test_returns_false_when_no_instance(self): srv._openspace_instance = original def test_returns_false_when_not_initialized(self): - import openspace.mcp_server as srv + import openspace.mcp.tool_handlers as srv original = srv._openspace_instance try: @@ -78,7 +78,7 @@ def test_returns_false_when_not_initialized(self): srv._openspace_instance = original def test_returns_false_when_config_missing(self): - import openspace.mcp_server as srv + import openspace.mcp.tool_handlers as srv original = srv._openspace_instance try: @@ -91,7 +91,7 @@ def test_returns_false_when_config_missing(self): srv._openspace_instance = original def test_returns_false_when_skills_config_missing(self): - import openspace.mcp_server as srv + import openspace.mcp.tool_handlers as srv original = srv._openspace_instance try: @@ -106,7 +106,7 @@ def test_returns_false_when_skills_config_missing(self): srv._openspace_instance = original def test_returns_false_when_flag_is_false(self): - import openspace.mcp_server as srv + import openspace.mcp.tool_handlers as srv from openspace.config.grounding import SkillConfig original = srv._openspace_instance @@ -122,7 +122,7 @@ def test_returns_false_when_flag_is_false(self): srv._openspace_instance = original def test_returns_true_when_flag_is_true(self): - import openspace.mcp_server as srv + import openspace.mcp.tool_handlers as srv from openspace.config.grounding import SkillConfig original = srv._openspace_instance @@ -148,19 +148,19 @@ class TestCloudSearchAndImportGating: @pytest.fixture(autouse=True) def _patch_auto_import(self): - with patch("openspace.mcp_server._is_auto_import_enabled", return_value=False): + with patch("openspace.mcp.tool_handlers._is_auto_import_enabled", return_value=False): yield async def test_returns_empty_when_disabled(self): - from openspace.mcp_server import _cloud_search_and_import + from openspace.mcp.tool_handlers import _cloud_search_and_import result = await _cloud_search_and_import("build a web scraper") assert result == [] async def test_never_calls_cloud_when_disabled(self): """Cloud search module should never be imported when disabled.""" - with patch("openspace.mcp_server._is_auto_import_enabled", return_value=False): - from openspace.mcp_server import _cloud_search_and_import + with patch("openspace.mcp.tool_handlers._is_auto_import_enabled", return_value=False): + from openspace.mcp.tool_handlers import _cloud_search_and_import # If cloud modules were imported, this would fail on missing deps result = await _cloud_search_and_import("anything") @@ -173,8 +173,8 @@ class TestCloudSearchAndImportEnabled: async def test_proceeds_when_enabled(self): """Verify the guard allows through when enabled (will fail on missing cloud module, proving the guard was passed).""" - with patch("openspace.mcp_server._is_auto_import_enabled", return_value=True): - from openspace.mcp_server import _cloud_search_and_import + with patch("openspace.mcp.tool_handlers._is_auto_import_enabled", return_value=True): + from openspace.mcp.tool_handlers import _cloud_search_and_import # Cloud modules won't be available in test env, so this should # return [] via the except branch, but it should NOT return @@ -193,8 +193,8 @@ class TestDoImportCloudSkillGating: """_do_import_cloud_skill must refuse when auto-import is disabled.""" async def test_blocked_when_disabled(self): - with patch("openspace.mcp_server._is_auto_import_enabled", return_value=False): - from openspace.mcp_server import _do_import_cloud_skill + with patch("openspace.mcp.tool_handlers._is_auto_import_enabled", return_value=False): + from openspace.mcp.tool_handlers import _do_import_cloud_skill result = await _do_import_cloud_skill("some-skill-id") assert result["status"] == "blocked" @@ -203,8 +203,8 @@ async def test_blocked_when_disabled(self): async def test_allowed_when_enabled(self): """When enabled, should attempt to actually import (and fail on missing cloud client — proving the guard was passed).""" - with patch("openspace.mcp_server._is_auto_import_enabled", return_value=True): - from openspace.mcp_server import _do_import_cloud_skill + with patch("openspace.mcp.tool_handlers._is_auto_import_enabled", return_value=True): + from openspace.mcp.tool_handlers import _do_import_cloud_skill with pytest.raises(Exception): # Will fail because cloud client isn't configured @@ -237,11 +237,11 @@ def _patch_openspace(self): ) with ( - patch("openspace.mcp_server._get_openspace", new_callable=AsyncMock, return_value=mock_os), - patch("openspace.mcp_server._get_store") as mock_store, + patch("openspace.mcp.tool_handlers._get_openspace", new_callable=AsyncMock, return_value=mock_os), + patch("openspace.mcp.tool_handlers._get_store") as mock_store, patch("openspace.cloud.search.hybrid_search_skills", mock_hybrid, create=True), - patch("openspace.mcp_server._is_auto_import_enabled", return_value=False), - patch("openspace.mcp_server._do_import_cloud_skill", new_callable=AsyncMock) as mock_import, + patch("openspace.mcp.tool_handlers._is_auto_import_enabled", return_value=False), + patch("openspace.mcp.tool_handlers._do_import_cloud_skill", new_callable=AsyncMock) as mock_import, ): mock_store.return_value = MagicMock() self.mock_import = mock_import @@ -250,7 +250,7 @@ def _patch_openspace(self): async def test_auto_import_param_true_but_config_false(self): """Even with auto_import=True in the call, config flag blocks import.""" - from openspace.mcp_server import search_skills + from openspace.mcp.tool_handlers import search_skills result_json = await search_skills(query="web scraper", auto_import=True) result = json.loads(result_json) @@ -261,7 +261,7 @@ async def test_auto_import_param_true_but_config_false(self): assert len(result["results"]) == 1 async def test_no_import_summary_when_disabled(self): - from openspace.mcp_server import search_skills + from openspace.mcp.tool_handlers import search_skills result_json = await search_skills(query="web scraper", auto_import=True) result = json.loads(result_json) @@ -281,13 +281,13 @@ async def test_cloud_import_blocked_in_execute_task(self): """Cloud import in execute_task goes through _cloud_search_and_import, which is gated. Verify the chain works.""" with ( - patch("openspace.mcp_server._is_auto_import_enabled", return_value=False), - patch("openspace.mcp_server._cloud_search_and_import", new_callable=AsyncMock) as mock_cloud, + patch("openspace.mcp.tool_handlers._is_auto_import_enabled", return_value=False), + patch("openspace.mcp.tool_handlers._cloud_search_and_import", new_callable=AsyncMock) as mock_cloud, ): # Even though _cloud_search_and_import has its own guard, # verify that when called, it returns [] without side effects mock_cloud.return_value = [] - from openspace.mcp_server import _cloud_search_and_import + from openspace.mcp.tool_handlers import _cloud_search_and_import result = await _cloud_search_and_import("any task") assert result == [] @@ -303,7 +303,7 @@ class TestConfigToGatingIntegration: to _is_auto_import_enabled() and gates all import paths.""" def test_config_false_gates_helper(self): - import openspace.mcp_server as srv + import openspace.mcp.tool_handlers as srv from openspace.config.grounding import SkillConfig original = srv._openspace_instance @@ -320,7 +320,7 @@ def test_config_false_gates_helper(self): srv._openspace_instance = original def test_config_true_enables_helper(self): - import openspace.mcp_server as srv + import openspace.mcp.tool_handlers as srv from openspace.config.grounding import SkillConfig original = srv._openspace_instance diff --git a/tests/test_integration.py b/tests/test_integration.py index d6c73601..0f303178 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -197,7 +197,7 @@ class TestSkillExecutionIntegration: @pytest.mark.asyncio async def test_execute_task_returns_formatted_result(self): """execute_task produces MCP-formatted result when engine succeeds.""" - from openspace.mcp_server import execute_task + from openspace.mcp.tool_handlers import execute_task mock_os = MagicMock() mock_os.is_initialized.return_value = True @@ -210,11 +210,11 @@ async def test_execute_task_returns_formatted_result(self): ) with ( - patch("openspace.mcp_server._get_openspace", new_callable=AsyncMock, return_value=mock_os), - patch("openspace.mcp_server._auto_register_skill_dirs", new_callable=AsyncMock), - patch("openspace.mcp_server._cloud_search_and_import", new_callable=AsyncMock, return_value=[]), - patch("openspace.mcp_server._format_task_result", return_value={"result": "formatted"}), - patch("openspace.mcp_server._write_upload_meta"), + patch("openspace.mcp.tool_handlers._get_openspace", new_callable=AsyncMock, return_value=mock_os), + patch("openspace.mcp.tool_handlers._auto_register_skill_dirs", new_callable=AsyncMock), + patch("openspace.mcp.tool_handlers._cloud_search_and_import", new_callable=AsyncMock, return_value=[]), + patch("openspace.mcp.tool_handlers._format_task_result", return_value={"result": "formatted"}), + patch("openspace.mcp.tool_handlers._write_upload_meta"), ): result = await execute_task(task="test task") mock_os.execute.assert_called_once() @@ -222,7 +222,7 @@ async def test_execute_task_returns_formatted_result(self): @pytest.mark.asyncio async def test_execute_task_auto_registers_skill_dirs(self): """execute_task calls _auto_register_skill_dirs when skill_dirs provided.""" - from openspace.mcp_server import execute_task + from openspace.mcp.tool_handlers import execute_task mock_os = MagicMock() mock_os.is_initialized.return_value = True @@ -235,11 +235,11 @@ async def test_execute_task_auto_registers_skill_dirs(self): ) with ( - patch("openspace.mcp_server._get_openspace", new_callable=AsyncMock, return_value=mock_os), - patch("openspace.mcp_server._auto_register_skill_dirs", new_callable=AsyncMock) as mock_register, - patch("openspace.mcp_server._cloud_search_and_import", new_callable=AsyncMock, return_value=[]), - patch("openspace.mcp_server._format_task_result", return_value={"result": "ok"}), - patch("openspace.mcp_server._write_upload_meta"), + patch("openspace.mcp.tool_handlers._get_openspace", new_callable=AsyncMock, return_value=mock_os), + patch("openspace.mcp.tool_handlers._auto_register_skill_dirs", new_callable=AsyncMock) as mock_register, + patch("openspace.mcp.tool_handlers._cloud_search_and_import", new_callable=AsyncMock, return_value=[]), + patch("openspace.mcp.tool_handlers._format_task_result", return_value={"result": "ok"}), + patch("openspace.mcp.tool_handlers._write_upload_meta"), ): await execute_task(task="test", skill_dirs=["/tmp/skills"]) mock_register.assert_called() @@ -247,7 +247,7 @@ async def test_execute_task_auto_registers_skill_dirs(self): @pytest.mark.asyncio async def test_execute_task_respects_search_scope_local(self): """execute_task skips cloud import when search_scope='local'.""" - from openspace.mcp_server import execute_task + from openspace.mcp.tool_handlers import execute_task mock_os = MagicMock() mock_os.is_initialized.return_value = True @@ -260,11 +260,11 @@ async def test_execute_task_respects_search_scope_local(self): ) with ( - patch("openspace.mcp_server._get_openspace", new_callable=AsyncMock, return_value=mock_os), - patch("openspace.mcp_server._auto_register_skill_dirs", new_callable=AsyncMock), - patch("openspace.mcp_server._cloud_search_and_import", new_callable=AsyncMock) as mock_import, - patch("openspace.mcp_server._format_task_result", return_value={"result": "ok"}), - patch("openspace.mcp_server._write_upload_meta"), + patch("openspace.mcp.tool_handlers._get_openspace", new_callable=AsyncMock, return_value=mock_os), + patch("openspace.mcp.tool_handlers._auto_register_skill_dirs", new_callable=AsyncMock), + patch("openspace.mcp.tool_handlers._cloud_search_and_import", new_callable=AsyncMock) as mock_import, + patch("openspace.mcp.tool_handlers._format_task_result", return_value={"result": "ok"}), + patch("openspace.mcp.tool_handlers._write_upload_meta"), ): await execute_task(task="test", search_scope="local") mock_import.assert_not_called() @@ -357,16 +357,16 @@ async def test_error_responses_contain_no_tracebacks(self): @pytest.mark.asyncio async def test_execute_task_exception_returns_safe_error(self): """Internal exception in execute_task returns safe error, no traceback.""" - from openspace.mcp_server import execute_task + from openspace.mcp.tool_handlers import execute_task mock_os = MagicMock() mock_os.is_initialized.return_value = True mock_os.execute = AsyncMock(side_effect=RuntimeError("internal boom")) with ( - patch("openspace.mcp_server._get_openspace", new_callable=AsyncMock, return_value=mock_os), - patch("openspace.mcp_server._auto_register_skill_dirs", new_callable=AsyncMock), - patch("openspace.mcp_server._cloud_search_and_import", new_callable=AsyncMock), + patch("openspace.mcp.tool_handlers._get_openspace", new_callable=AsyncMock, return_value=mock_os), + patch("openspace.mcp.tool_handlers._auto_register_skill_dirs", new_callable=AsyncMock), + patch("openspace.mcp.tool_handlers._cloud_search_and_import", new_callable=AsyncMock), ): result = await execute_task(task="crash test") result_str = str(result) diff --git a/tests/test_mcp_tool_handlers.py b/tests/test_mcp_tool_handlers.py new file mode 100644 index 00000000..c9a6d3ef --- /dev/null +++ b/tests/test_mcp_tool_handlers.py @@ -0,0 +1,213 @@ +"""Tests for openspace.mcp.tool_handlers — Epic 4.7 extraction.""" +from __future__ import annotations + +import json +import sys +from pathlib import Path +from typing import Any, Dict +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +# Guard: skip all tests if the tool_handlers module can't be imported +try: + from openspace.mcp.tool_handlers import ( + _format_task_result, + _is_auto_import_enabled, + _json_error, + _json_ok, + _read_upload_meta, + _write_upload_meta, + register_handlers, + ) + + _HAS_MODULE = True +except ImportError: + _HAS_MODULE = False + +pytestmark = pytest.mark.skipif(not _HAS_MODULE, reason="openspace.mcp.tool_handlers not importable") + + +# --------------------------------------------------------------------------- +# register_handlers +# --------------------------------------------------------------------------- +class TestRegisterHandlers: + """Verify register_handlers wires all 4 tools to the FastMCP instance.""" + + def test_registers_four_tools(self): + mock_mcp = MagicMock() + mock_decorator = MagicMock(side_effect=lambda fn: fn) + mock_mcp.tool.return_value = mock_decorator + + register_handlers(mock_mcp) + + assert mock_mcp.tool.call_count == 4 + registered_names = {call.args[0].__name__ for call in mock_decorator.call_args_list} + assert registered_names == {"execute_task", "search_skills", "fix_skill", "upload_skill"} + + def test_handlers_preserve_docstrings(self): + mock_mcp = MagicMock() + registered_fns = [] + mock_mcp.tool.return_value = lambda fn: (registered_fns.append(fn), fn)[1] + + register_handlers(mock_mcp) + + for fn in registered_fns: + assert fn.__doc__ is not None, f"{fn.__name__} has no docstring" + assert len(fn.__doc__) > 20, f"{fn.__name__} docstring too short" + + def test_idempotent_double_register(self): + """Calling register_handlers twice should not raise.""" + mock_mcp = MagicMock() + mock_mcp.tool.return_value = lambda fn: fn + + register_handlers(mock_mcp) + register_handlers(mock_mcp) + assert mock_mcp.tool.call_count == 8 # 4 + 4 + + +# --------------------------------------------------------------------------- +# _format_task_result +# --------------------------------------------------------------------------- +class TestFormatTaskResult: + """Verify result formatting for MCP transport.""" + + def test_minimal_result(self): + result = _format_task_result({}) + assert result["status"] == "unknown" + assert result["response"] == "" + assert result["execution_time"] == 0 + assert result["iterations"] == 0 + assert result["skills_used"] == [] + assert result["tool_call_count"] == 0 + assert result["tool_summary"] == [] + + def test_full_result(self): + result = _format_task_result({ + "status": "success", + "response": "Done!", + "execution_time": 3.456789, + "iterations": 5, + "skills_used": ["skill_a"], + "task_id": "t1", + "tool_executions": [ + {"tool_name": "bash", "status": "ok", "error": None}, + {"tool_name": "python", "status": "error", "error": "x" * 300}, + ], + }) + assert result["status"] == "success" + assert result["execution_time"] == 3.46 + assert result["tool_call_count"] == 2 + assert len(result["tool_summary"][1]["error"]) <= 200 + + def test_evolved_skills_formatting(self): + result = _format_task_result({ + "evolved_skills": [ + {"path": "/skills/foo/SKILL.md", "name": "foo", "origin": "derived", "change_summary": "test"}, + ], + }) + assert result["evolved_skills"][0]["skill_dir"] == str(Path("/skills/foo")) + assert result["evolved_skills"][0]["upload_ready"] is True + assert "action_required" in result + + def test_warning_passthrough(self): + result = _format_task_result({"warning": "timeout approaching"}) + assert result["warning"] == "timeout approaching" + + def test_no_warning_when_absent(self): + result = _format_task_result({}) + assert "warning" not in result + + def test_tool_summary_caps_at_20(self): + execs = [{"tool_name": f"t{i}", "status": "ok"} for i in range(30)] + result = _format_task_result({"tool_executions": execs}) + assert len(result["tool_summary"]) == 20 + assert result["tool_call_count"] == 30 + + +# --------------------------------------------------------------------------- +# _json_ok / _json_error +# --------------------------------------------------------------------------- +class TestJsonHelpers: + def test_json_ok_roundtrip(self): + data = {"key": "value", "num": 42} + raw = _json_ok(data) + assert json.loads(raw) == data + + def test_json_ok_unicode(self): + raw = _json_ok({"emoji": "🚀"}) + assert "🚀" in raw # ensure_ascii=False + + def test_json_error_structured(self): + raw = _json_error("something broke") + parsed = json.loads(raw) + assert "error" in parsed or "message" in parsed or "error_code" in parsed + + +# --------------------------------------------------------------------------- +# _write_upload_meta / _read_upload_meta +# --------------------------------------------------------------------------- +class TestUploadMeta: + def test_write_then_read_roundtrip(self, tmp_path): + info = { + "origin": "derived", + "parent_skill_ids": ["s1"], + "change_summary": "Fixed it", + "created_by": "bot", + "tags": ["test"], + } + _write_upload_meta(tmp_path, info) + result = _read_upload_meta(tmp_path) + assert result["origin"] == "derived" + assert result["parent_skill_ids"] == ["s1"] + assert result["tags"] == ["test"] + + def test_read_empty_dir_returns_empty(self, tmp_path): + """No sidecar, no DB → empty dict.""" + with patch("openspace.mcp.tool_handlers._get_store") as mock_store: + mock_store.return_value.load_record_by_path.return_value = None + result = _read_upload_meta(tmp_path) + assert result == {} + + def test_write_missing_fields_defaults(self, tmp_path): + _write_upload_meta(tmp_path, {}) + result = _read_upload_meta(tmp_path) + assert result["origin"] == "imported" + assert result["parent_skill_ids"] == [] + + def test_corrupt_sidecar_falls_through(self, tmp_path): + """Corrupt JSON → falls to DB tier → empty dict.""" + (tmp_path / ".upload_meta.json").write_text("{bad json", encoding="utf-8") + with patch("openspace.mcp.tool_handlers._get_store") as mock_store: + mock_store.return_value.load_record_by_path.return_value = None + result = _read_upload_meta(tmp_path) + assert result == {} + + +# --------------------------------------------------------------------------- +# _is_auto_import_enabled +# --------------------------------------------------------------------------- +class TestAutoImportEnabled: + def test_returns_false_when_no_instance(self): + with patch("openspace.mcp.tool_handlers._openspace_instance", None): + assert _is_auto_import_enabled() is False + + def test_returns_false_when_not_initialized(self): + mock_os = MagicMock() + mock_os.is_initialized.return_value = False + with patch("openspace.mcp.tool_handlers._openspace_instance", mock_os): + assert _is_auto_import_enabled() is False + + def test_returns_true_when_enabled(self): + mock_os = MagicMock() + mock_os.is_initialized.return_value = True + mock_os._grounding_config.skills.auto_import_enabled = True + with patch("openspace.mcp.tool_handlers._openspace_instance", mock_os): + assert _is_auto_import_enabled() is True + + def test_returns_false_when_no_skills_config(self): + mock_os = MagicMock() + mock_os.is_initialized.return_value = True + mock_os._grounding_config = None + with patch("openspace.mcp.tool_handlers._openspace_instance", mock_os): + assert _is_auto_import_enabled() is False diff --git a/tests/test_traceback_safety.py b/tests/test_traceback_safety.py index cae47a90..ffdf126d 100644 --- a/tests/test_traceback_safety.py +++ b/tests/test_traceback_safety.py @@ -228,7 +228,7 @@ def _make_openspace_mock(): @pytest.fixture(autouse=True) def _patch_openspace_init(monkeypatch): """Prevent real OpenSpace initialization in every test.""" - import openspace.mcp_server as srv + import openspace.mcp.tool_handlers as srv mock = _make_openspace_mock() monkeypatch.setattr(srv, "_openspace_instance", mock) @@ -240,7 +240,7 @@ def _patch_openspace_init(monkeypatch): @pytest.mark.asyncio async def test_execute_task_error_no_traceback(): """execute_task: exception → structured error, no traceback leak.""" - import openspace.mcp_server as srv + import openspace.mcp.tool_handlers as srv with patch.object( srv, @@ -258,7 +258,7 @@ async def test_execute_task_error_no_traceback(): @pytest.mark.asyncio async def test_execute_task_deep_traceback_not_leaked(): """execute_task: real traceback chain → nothing leaks.""" - import openspace.mcp_server as srv + import openspace.mcp.tool_handlers as srv def _blow_up(): raise KeyError("secret_key") @@ -282,10 +282,10 @@ async def _explode(): @pytest.mark.asyncio async def test_search_skills_error_no_traceback(): """search_skills: exception → structured error, no traceback leak.""" - import openspace.mcp_server as srv + import openspace.mcp.tool_handlers as srv with patch( - "openspace.mcp_server.hybrid_search_skills", + "openspace.mcp.tool_handlers.hybrid_search_skills", create=True, side_effect=ImportError("No module named 'openspace.cloud.search'"), ): @@ -303,7 +303,7 @@ async def test_search_skills_error_no_traceback(): @pytest.mark.asyncio async def test_fix_skill_missing_skill_md(): """fix_skill: missing SKILL.md → SKILL_NOT_FOUND, no path leak.""" - import openspace.mcp_server as srv + import openspace.mcp.tool_handlers as srv result = await srv.fix_skill( skill_dir="/secret/internal/path/my-skill", @@ -323,7 +323,7 @@ async def test_fix_skill_exception_no_traceback(): """fix_skill: runtime exception → structured error.""" import os - import openspace.mcp_server as srv + import openspace.mcp.tool_handlers as srv # Create a temporary directory with SKILL.md so we pass validation tmpdir = os.path.join(os.path.dirname(__file__), "_test_skill_tmp") @@ -356,7 +356,7 @@ async def test_fix_skill_exception_no_traceback(): @pytest.mark.asyncio async def test_upload_skill_missing_skill_md(): """upload_skill: missing SKILL.md → SKILL_NOT_FOUND, no path leak.""" - import openspace.mcp_server as srv + import openspace.mcp.tool_handlers as srv result = await srv.upload_skill(skill_dir="/opt/secret/skills/broken") @@ -372,7 +372,7 @@ async def test_upload_skill_exception_no_traceback(): """upload_skill: cloud auth failure → structured error, no traceback.""" import os - import openspace.mcp_server as srv + import openspace.mcp.tool_handlers as srv tmpdir = os.path.join(os.path.dirname(__file__), "_test_upload_tmp") os.makedirs(tmpdir, exist_ok=True) @@ -403,7 +403,7 @@ async def test_upload_skill_exception_no_traceback(): @pytest.mark.asyncio async def test_server_logs_full_exception(): """Verify that full exception details are logged server-side.""" - import openspace.mcp_server as srv + import openspace.mcp.tool_handlers as srv with patch("openspace.errors.logger") as mock_logger: with patch.object( @@ -439,7 +439,7 @@ async def test_server_logs_full_exception(): ) async def test_all_tools_structured_error_on_failure(tool_name, call): """Every MCP tool returns structured error JSON on failure — never raw tracebacks.""" - import openspace.mcp_server as srv + import openspace.mcp.tool_handlers as srv # Sabotage everything to force errors with patch.object(