diff --git a/observal-server/api/routes/mcp.py b/observal-server/api/routes/mcp.py index 43d4c7b..2e11b13 100644 --- a/observal-server/api/routes/mcp.py +++ b/observal-server/api/routes/mcp.py @@ -90,12 +90,25 @@ async def submit_mcp( db: AsyncSession = Depends(get_db), current_user: User = Depends(require_role(UserRole.user)), ): - # Prevent duplicate names for the same user - existing = await db.execute( - select(McpListing).where(McpListing.name == req.name, McpListing.submitted_by == current_user.id) + # Prevent duplicate names for the same user. + # Pending/rejected listings are replaced automatically so the user isn't + # blocked when re-submitting after a mistake. Approved listings are + # protected — use the update flow instead. + existing = ( + ( + await db.execute( + select(McpListing).where(McpListing.name == req.name, McpListing.submitted_by == current_user.id) + ) + ) + .scalars() + .first() ) - if existing.scalars().first(): - raise HTTPException(status_code=409, detail=f"You already have a listing named '{req.name}'") + if existing: + if existing.status == ListingStatus.approved: + raise HTTPException(status_code=409, detail=f"You already have an approved listing named '{req.name}'") + # Replace the old pending/rejected listing + await db.delete(existing) + await db.flush() listing = McpListing( name=req.name, diff --git a/observal-server/services/agent_config_generator.py b/observal-server/services/agent_config_generator.py index 7974c49..4f5add0 100644 --- a/observal-server/services/agent_config_generator.py +++ b/observal-server/services/agent_config_generator.py @@ -62,9 +62,9 @@ def _build_mcp_configs( # agent file gets proper mcpServers frontmatter. safe = _sanitize_name(listing.name) mcp_id = str(listing.id) - run_cmd = _build_run_command(safe, listing.framework) + run_cmd = _build_run_command(safe, listing.framework, listing.docker_image, mcp_env) shim_args = ["--mcp-id", mcp_id, "--", *run_cmd] - mcp_configs[safe] = {"command": "observal-shim", "args": shim_args, "env": {}} + mcp_configs[safe] = {"command": "observal-shim", "args": shim_args, "env": mcp_env} for ext in agent.external_mcps or []: name = _sanitize_name(ext.get("name", "")) diff --git a/observal-server/services/config_generator.py b/observal-server/services/config_generator.py index 937d175..6fe6e5e 100644 --- a/observal-server/services/config_generator.py +++ b/observal-server/services/config_generator.py @@ -70,7 +70,7 @@ def _build_run_command( - Python / unknown: python -m """ fw = (framework or "").lower() - if "docker" in fw and docker_image: + if docker_image: cmd = ["docker", "run", "-i", "--rm"] for k, v in (server_env or {}).items(): cmd.extend(["-e", f"{k}={v}"]) diff --git a/observal-server/services/mcp_validator.py b/observal-server/services/mcp_validator.py index 301d118..67a8d8f 100644 --- a/observal-server/services/mcp_validator.py +++ b/observal-server/services/mcp_validator.py @@ -81,11 +81,25 @@ async def _async_clone(clone_url: str, dest: str, depth: int = 1) -> None: ) -_ENV_VAR_PATTERN = re.compile( +_ENV_VAR_PATTERN_PYTHON = re.compile( r"""os\.environ\s*(?:\.get\s*\(\s*|\.?\[?\s*\[?\s*)["']([A-Z][A-Z0-9_]+)["']""" r"""|os\.getenv\s*\(\s*["']([A-Z][A-Z0-9_]+)["']""" ) +_ENV_VAR_PATTERN_GO = re.compile(r"""os\.Getenv\(\s*"([A-Z][A-Z0-9_]+)"\s*\)""") + +_ENV_VAR_PATTERN_TS = re.compile( + r"""process\.env\.([A-Z][A-Z0-9_]+)""" + r"""|process\.env\[\s*["']([A-Z][A-Z0-9_]+)["']\s*\]""" +) + +# README patterns: docker -e flags, export statements, JSON config keys +_README_PATTERNS = [ + re.compile(r"""-e\s+([A-Z][A-Z0-9_]+)"""), + re.compile(r"""export\s+([A-Z][A-Z0-9_]+)="""), + re.compile(r""""([A-Z][A-Z0-9_]+)"\s*:\s*\""""), +] + # Env vars that are internal to the runtime / framework, not user-facing config _INTERNAL_ENV_VARS = frozenset( { @@ -109,6 +123,7 @@ async def _async_clone(clone_url: str, dest: str, depth: int = 1) -> None: "PORT", "HOST", "DEBUG", + "APP", "LOG_LEVEL", "LOGGING_LEVEL", "HOSTNAME", @@ -121,6 +136,15 @@ async def _async_clone(clone_url: str, dest: str, depth: int = 1) -> None: } ) +# User-facing env vars that match a filtered prefix but should still be detected +_ALLOWED_ENV_VARS = frozenset( + { + "GITHUB_TOKEN", + "GITHUB_PERSONAL_ACCESS_TOKEN", + "DOCKER_HOST", + } +) + # Prefix patterns for build/CI/infrastructure env vars that are never user-facing _FILTERED_PREFIXES = ( "CI_", @@ -143,33 +167,107 @@ async def _async_clone(clone_url: str, dest: str, depth: int = 1) -> None: def _is_filtered_env_var(name: str) -> bool: """Return True if the env var is internal/infrastructure and should not be prompted.""" + if name in _ALLOWED_ENV_VARS: + return False if name in _INTERNAL_ENV_VARS: return True return any(name.startswith(prefix) for prefix in _FILTERED_PREFIXES) -def _detect_env_vars(tmp_dir: str) -> list[dict]: - """Scan repo files for required environment variables. +# Directories that contain test / internal / build code — not user-facing config +_SKIP_DIRS = frozenset( + { + "test", + "tests", + "e2e", + "internal", + "testdata", + "vendor", + "node_modules", + "__pycache__", + ".git", + } +) - Scans Python source (os.environ/os.getenv) and .env.example files. - Dockerfile ENV/ARG directives are intentionally skipped — they contain - build-time variables that are not user-facing configuration. - """ - root = Path(tmp_dir) - found: dict[str, str] = {} # name -> description hint - # Scan Python files for os.environ / os.getenv - for py_file in root.rglob("*.py"): +def _is_test_file(path: Path) -> bool: + """Return True if the file is in a test/internal directory or is a test file.""" + if any(part in _SKIP_DIRS for part in path.parts): + return True + name = path.name + return name.endswith("_test.go") or name.startswith("test_") or name.endswith("_test.py") + + +def _scan_files_for_env_vars(root: Path, glob: str, pattern: re.Pattern, found: dict[str, str]) -> None: + """Scan files matching *glob* for env var references using *pattern*.""" + for path in root.rglob(glob): + if _is_test_file(path.relative_to(root)): + continue try: - content = py_file.read_text(errors="ignore") - for m in _ENV_VAR_PATTERN.finditer(content): - name = m.group(1) or m.group(2) + content = path.read_text(errors="ignore") + for m in pattern.finditer(content): + name = next((g for g in m.groups() if g), None) if name and not _is_filtered_env_var(name): found.setdefault(name, "") except Exception: continue - # Scan .env.example / .env.sample for documented env vars + +def _scan_readme_for_env_vars(root: Path, found: dict[str, str]) -> None: + """Extract env vars from README files (docker -e, export, JSON config).""" + for name in ("README.md", "README.rst", "README.txt", "README"): + readme = root / name + if not readme.exists(): + continue + try: + content = readme.read_text(errors="ignore") + except Exception: + continue + for pattern in _README_PATTERNS: + for m in pattern.finditer(content): + var = m.group(1) + if var and not _is_filtered_env_var(var): + found.setdefault(var, "") + break # only scan the first README found + + +def _extract_manifest_env_vars(root: Path, found: dict[str, str]) -> bool: + """Extract env vars from a server.json MCP manifest (authoritative source). + + The manifest is the standard MCP server descriptor. Env vars declared here + are always included — they bypass the prefix filter since the author + explicitly listed them as required. + + Returns True if a valid server.json was found (even if it declares no env vars). + """ + manifest = root / "server.json" + if not manifest.exists(): + return False + try: + data = json.loads(manifest.read_text(errors="ignore")) + except Exception: + return False + # packages[].runtimeArguments — Docker -e flags (e.g. GitHub MCP server) + for pkg in data.get("packages", []): + for arg in pkg.get("runtimeArguments", []): + value = arg.get("value", "") + # Pattern: "ENV_VAR={placeholder}" — extract the var name before '=' + if "=" in value: + var_name = value.split("=", 1)[0] + if var_name and var_name == var_name.upper(): + desc = arg.get("description", "") + found.setdefault(var_name, desc) + + # remotes[].variables — URL-interpolated secrets (e.g. ?api_key={key}) + for remote in data.get("remotes", []): + for var_key, var_meta in (remote.get("variables") or {}).items(): + desc = var_meta.get("description", "") if isinstance(var_meta, dict) else "" + found.setdefault(var_key, desc) + return True + + +def _scan_env_example(root: Path, found: dict[str, str]) -> None: + """Scan .env.example / .env.sample files for documented env vars.""" for env_file in root.glob(".env*"): if env_file.name in (".env", ".env.local"): continue # skip actual secrets @@ -184,6 +282,38 @@ def _detect_env_vars(tmp_dir: str) -> list[dict]: except Exception: continue + +def _detect_env_vars(tmp_dir: str) -> list[dict]: + """Scan repo files for required environment variables. + + Tiered detection (stops at first tier that finds results): + 1. server.json manifest (authoritative — author's explicit declaration) + 2. README + .env.example (author's documentation) + 3. Source code scanning (last resort — catches os.Getenv / process.env / etc.) + """ + root = Path(tmp_dir) + found: dict[str, str] = {} + + # Tier 1: MCP server manifest — authoritative, skip everything else + if _extract_manifest_env_vars(root, found): + return [{"name": k, "description": v, "required": True} for k, v in sorted(found.items())] + + # Tier 2: README — author's documented config (export, docker -e, JSON examples) + _scan_readme_for_env_vars(root, found) + if found: + return [{"name": k, "description": v, "required": True} for k, v in sorted(found.items())] + + # Tier 3: .env.example — explicit config template + _scan_env_example(root, found) + if found: + return [{"name": k, "description": v, "required": True} for k, v in sorted(found.items())] + + # Tier 4: Source code scanning — last resort + _scan_files_for_env_vars(root, "*.py", _ENV_VAR_PATTERN_PYTHON, found) + _scan_files_for_env_vars(root, "*.go", _ENV_VAR_PATTERN_GO, found) + for ext in ("*.ts", "*.js", "*.mts", "*.mjs"): + _scan_files_for_env_vars(root, ext, _ENV_VAR_PATTERN_TS, found) + return [{"name": k, "description": v, "required": True} for k, v in sorted(found.items())] diff --git a/observal_cli/analyzer.py b/observal_cli/analyzer.py index bfe488d..d79a9e3 100644 --- a/observal_cli/analyzer.py +++ b/observal_cli/analyzer.py @@ -35,11 +35,25 @@ r"|Server\(\s*name\s*=" ) -_ENV_VAR_PATTERN = re.compile( +_ENV_VAR_PATTERN_PYTHON = re.compile( r"""os\.environ\s*(?:\.get\s*\(\s*|\.?\[?\s*\[?\s*)["']([A-Z][A-Z0-9_]+)["']""" r"""|os\.getenv\s*\(\s*["']([A-Z][A-Z0-9_]+)["']""" ) +_ENV_VAR_PATTERN_GO = re.compile(r"""os\.Getenv\(\s*"([A-Z][A-Z0-9_]+)"\s*\)""") + +# README patterns: docker -e flags, export statements, JSON config keys +_README_PATTERNS = [ + re.compile(r"""-e\s+([A-Z][A-Z0-9_]+)"""), + re.compile(r"""export\s+([A-Z][A-Z0-9_]+)="""), + re.compile(r""""([A-Z][A-Z0-9_]+)"\s*:\s*\""""), +] + +_ENV_VAR_PATTERN_TS = re.compile( + r"""process\.env\.([A-Z][A-Z0-9_]+)""" + r"""|process\.env\[\s*["']([A-Z][A-Z0-9_]+)["']\s*\]""" +) + _INTERNAL_ENV_VARS = frozenset( { "PATH", @@ -62,6 +76,7 @@ "PORT", "HOST", "DEBUG", + "APP", "LOG_LEVEL", "LOGGING_LEVEL", "HOSTNAME", @@ -74,6 +89,15 @@ } ) +# User-facing env vars that match a filtered prefix but should still be detected +_ALLOWED_ENV_VARS = frozenset( + { + "GITHUB_TOKEN", + "GITHUB_PERSONAL_ACCESS_TOKEN", + "DOCKER_HOST", + } +) + # Prefix patterns for build/CI/infrastructure env vars that are never user-facing _FILTERED_PREFIXES = ( "CI_", @@ -125,33 +149,107 @@ def _clone_repo(git_url: str, dest: str) -> str | None: def _is_filtered_env_var(name: str) -> bool: """Return True if the env var is internal/infrastructure and should not be prompted.""" + if name in _ALLOWED_ENV_VARS: + return False if name in _INTERNAL_ENV_VARS: return True return any(name.startswith(prefix) for prefix in _FILTERED_PREFIXES) -def _detect_env_vars(tmp_dir: str) -> list[dict]: - """Scan repo files for required environment variables. +# Directories that contain test / internal / build code — not user-facing config +_SKIP_DIRS = frozenset( + { + "test", + "tests", + "e2e", + "internal", + "testdata", + "vendor", + "node_modules", + "__pycache__", + ".git", + } +) - Scans Python source (os.environ/os.getenv) and .env.example files. - Dockerfile ENV/ARG directives are intentionally skipped — they contain - build-time variables that are not user-facing configuration. - """ - root = Path(tmp_dir) - found: dict[str, str] = {} - # Scan Python files for os.environ / os.getenv - for py_file in root.rglob("*.py"): +def _is_test_file(path: Path) -> bool: + """Return True if the file is in a test/internal directory or is a test file.""" + if any(part in _SKIP_DIRS for part in path.parts): + return True + name = path.name + return name.endswith("_test.go") or name.startswith("test_") or name.endswith("_test.py") + + +def _scan_files_for_env_vars(root: Path, glob: str, pattern: re.Pattern, found: dict[str, str]) -> None: + """Scan files matching *glob* for env var references using *pattern*.""" + for path in root.rglob(glob): + if _is_test_file(path.relative_to(root)): + continue try: - content = py_file.read_text(errors="ignore") - for m in _ENV_VAR_PATTERN.finditer(content): - name = m.group(1) or m.group(2) + content = path.read_text(errors="ignore") + for m in pattern.finditer(content): + name = next((g for g in m.groups() if g), None) if name and not _is_filtered_env_var(name): found.setdefault(name, "") except Exception: continue - # Scan .env.example / .env.sample for documented env vars + +def _scan_readme_for_env_vars(root: Path, found: dict[str, str]) -> None: + """Extract env vars from README files (docker -e, export, JSON config).""" + for name in ("README.md", "README.rst", "README.txt", "README"): + readme = root / name + if not readme.exists(): + continue + try: + content = readme.read_text(errors="ignore") + except Exception: + continue + for pattern in _README_PATTERNS: + for m in pattern.finditer(content): + var = m.group(1) + if var and not _is_filtered_env_var(var): + found.setdefault(var, "") + break # only scan the first README found + + +def _extract_manifest_env_vars(root: Path, found: dict[str, str]) -> bool: + """Extract env vars from a server.json MCP manifest (authoritative source). + + The manifest is the standard MCP server descriptor. Env vars declared here + are always included — they bypass the prefix filter since the author + explicitly listed them as required. + + Returns True if a valid server.json was found (even if it declares no env vars). + """ + manifest = root / "server.json" + if not manifest.exists(): + return False + try: + data = json.loads(manifest.read_text(errors="ignore")) + except Exception: + return False + # packages[].runtimeArguments — Docker -e flags (e.g. GitHub MCP server) + for pkg in data.get("packages", []): + for arg in pkg.get("runtimeArguments", []): + value = arg.get("value", "") + # Pattern: "ENV_VAR={placeholder}" — extract the var name before '=' + if "=" in value: + var_name = value.split("=", 1)[0] + if var_name and var_name == var_name.upper(): + desc = arg.get("description", "") + found.setdefault(var_name, desc) + + # remotes[].variables — URL-interpolated secrets (e.g. ?api_key={key}) + for remote in data.get("remotes", []): + for var_key, var_meta in (remote.get("variables") or {}).items(): + desc = var_meta.get("description", "") if isinstance(var_meta, dict) else "" + found.setdefault(var_key, desc) + return True + + +def _scan_env_example(root: Path, found: dict[str, str]) -> None: + """Scan .env.example / .env.sample files for documented env vars.""" for env_file in root.glob(".env*"): if env_file.name in (".env", ".env.local"): continue # skip actual secrets @@ -166,6 +264,38 @@ def _detect_env_vars(tmp_dir: str) -> list[dict]: except Exception: continue + +def _detect_env_vars(tmp_dir: str) -> list[dict]: + """Scan repo files for required environment variables. + + Tiered detection (stops at first tier that finds results): + 1. server.json manifest (authoritative — author's explicit declaration) + 2. README + .env.example (author's documentation) + 3. Source code scanning (last resort — catches os.Getenv / process.env / etc.) + """ + root = Path(tmp_dir) + found: dict[str, str] = {} + + # Tier 1: MCP server manifest — authoritative, skip everything else + if _extract_manifest_env_vars(root, found): + return [{"name": k, "description": v, "required": True} for k, v in sorted(found.items())] + + # Tier 2: README — author's documented config (export, docker -e, JSON examples) + _scan_readme_for_env_vars(root, found) + if found: + return [{"name": k, "description": v, "required": True} for k, v in sorted(found.items())] + + # Tier 3: .env.example — explicit config template + _scan_env_example(root, found) + if found: + return [{"name": k, "description": v, "required": True} for k, v in sorted(found.items())] + + # Tier 4: Source code scanning — last resort + _scan_files_for_env_vars(root, "*.py", _ENV_VAR_PATTERN_PYTHON, found) + _scan_files_for_env_vars(root, "*.go", _ENV_VAR_PATTERN_GO, found) + for ext in ("*.ts", "*.js", "*.mts", "*.mjs"): + _scan_files_for_env_vars(root, ext, _ENV_VAR_PATTERN_TS, found) + return [{"name": k, "description": v, "required": True} for k, v in sorted(found.items())] diff --git a/observal_cli/cmd_mcp.py b/observal_cli/cmd_mcp.py index eefff1d..f94327e 100644 --- a/observal_cli/cmd_mcp.py +++ b/observal_cli/cmd_mcp.py @@ -113,11 +113,11 @@ def _review_env_vars(env_vars: list[dict]) -> list[dict]: """Let the developer review, remove, and annotate each env var.""" reviewed: list[dict] = [] - rprint("\n[bold]Review each variable[/bold] [dim](enter to keep, 'r' to remove, 'o' for optional)[/dim]\n") + rprint("\n[bold]Review each variable[/bold]\n") for ev in env_vars: action = typer.prompt( - f" {ev['name']} [required]", + f" {ev['name']} — keep? [Enter=keep / r=remove / o=optional]", default="", show_default=False, ) @@ -306,6 +306,14 @@ def _submit_impl(git_url, name, category, yes): _docker_image = None if _framework == "docker": _docker_image = typer.prompt("Docker image (e.g. registry.example.com/org/mcp-server:latest)") + elif _framework == "go": + _docker_image = ( + typer.prompt( + "Docker image (e.g. ghcr.io/org/server:latest — press Enter to skip if the binary is on PATH)", + default="", + ) + or None + ) _setup = typer.prompt("Setup instructions (optional, press Enter to skip)", default="") _changelog = typer.prompt("Changelog", default="Initial release") diff --git a/tests/test_env_detection_and_config.py b/tests/test_env_detection_and_config.py new file mode 100644 index 0000000..b645ce5 --- /dev/null +++ b/tests/test_env_detection_and_config.py @@ -0,0 +1,713 @@ +"""Tests for env var detection (analyzer + validator), config generation with docker, +and MCP submit auto-replace logic.""" + +import json +import tempfile +import uuid +from datetime import UTC, datetime +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock + +import pytest +from fastapi import FastAPI +from httpx import ASGITransport, AsyncClient + +from api.deps import get_current_user, get_db +from models.mcp import ListingStatus +from models.user import User, UserRole + +# ── Helpers ────────────────────────────────────────────── + + +def _user(**kw): + u = MagicMock(spec=User) + u.id = kw.get("id", uuid.uuid4()) + u.role = kw.get("role", UserRole.user) + return u + + +def _mock_db(): + db = AsyncMock() + db.add = MagicMock() + db.commit = AsyncMock() + db.refresh = AsyncMock() + db.delete = AsyncMock() + db.flush = AsyncMock() + return db + + +def _scalar_result(val): + r = MagicMock() + r.scalar_one_or_none.return_value = val + r.scalars.return_value.all.return_value = [val] if val else [] + r.scalars.return_value.first.return_value = val + return r + + +def _make_tmpdir_with_files(file_map: dict[str, str]) -> str: + """Create a temp directory with the given file tree. Returns path.""" + tmp = tempfile.mkdtemp(prefix="observal_test_") + for relpath, content in file_map.items(): + full = Path(tmp) / relpath + full.parent.mkdir(parents=True, exist_ok=True) + full.write_text(content) + return tmp + + +# ═══════════════════════════════════════════════════════════ +# 1. Env Var Filtering +# ═══════════════════════════════════════════════════════════ + + +class TestEnvVarFiltering: + """Test _is_filtered_env_var for both CLI analyzer and server validator.""" + + def test_internal_vars_filtered(self): + from observal_cli.analyzer import _is_filtered_env_var + + for var in ("PATH", "HOME", "NODE_ENV", "PORT", "APP", "DEBUG"): + assert _is_filtered_env_var(var), f"{var} should be filtered" + + def test_ci_prefix_filtered(self): + from observal_cli.analyzer import _is_filtered_env_var + + for var in ("CI_PIPELINE_ID", "GITHUB_SHA", "GITLAB_CI", "DOCKER_BUILDKIT"): + assert _is_filtered_env_var(var), f"{var} should be filtered" + + def test_allowed_vars_bypass_prefix_filter(self): + from observal_cli.analyzer import _is_filtered_env_var + + for var in ("GITHUB_TOKEN", "GITHUB_PERSONAL_ACCESS_TOKEN", "DOCKER_HOST"): + assert not _is_filtered_env_var(var), f"{var} should NOT be filtered" + + def test_user_facing_vars_pass(self): + from observal_cli.analyzer import _is_filtered_env_var + + for var in ("OPENAI_API_KEY", "SLACK_TOKEN", "DATABASE_URL"): + assert not _is_filtered_env_var(var), f"{var} should pass" + + def test_server_validator_matches_cli(self): + """Server-side filtering must match CLI-side.""" + from observal_cli.analyzer import _is_filtered_env_var as cli_filter + from services.mcp_validator import _is_filtered_env_var as server_filter + + test_vars = [ + "PATH", + "GITHUB_SHA", + "GITHUB_TOKEN", + "OPENAI_API_KEY", + "CI_PIPELINE_ID", + "DOCKER_HOST", + "APP", + "SLACK_TOKEN", + ] + for var in test_vars: + assert cli_filter(var) == server_filter(var), f"Mismatch on {var}" + + +# ═══════════════════════════════════════════════════════════ +# 2. Test File Filtering +# ═══════════════════════════════════════════════════════════ + + +class TestTestFileFiltering: + def test_skip_test_dirs(self): + from observal_cli.analyzer import _is_test_file + + assert _is_test_file(Path("tests/test_foo.py")) + assert _is_test_file(Path("test/main_test.go")) + assert _is_test_file(Path("e2e/integration.ts")) + assert _is_test_file(Path("vendor/lib.go")) + assert _is_test_file(Path("node_modules/pkg/index.js")) + + def test_skip_test_files(self): + from observal_cli.analyzer import _is_test_file + + assert _is_test_file(Path("cmd/server_test.go")) + assert _is_test_file(Path("test_config.py")) + + def test_pass_normal_files(self): + from observal_cli.analyzer import _is_test_file + + assert not _is_test_file(Path("cmd/server.go")) + assert not _is_test_file(Path("src/main.py")) + assert not _is_test_file(Path("lib/index.ts")) + + +# ═══════════════════════════════════════════════════════════ +# 3. Tiered Env Var Detection (CLI analyzer) +# ═══════════════════════════════════════════════════════════ + + +class TestTier1ServerJson: + """Tier 1: server.json manifest is authoritative.""" + + def test_packages_runtime_arguments(self): + from observal_cli.analyzer import _detect_env_vars + + manifest = { + "packages": [ + { + "runtimeArguments": [ + {"value": "GITHUB_PERSONAL_ACCESS_TOKEN={token}", "description": "GitHub PAT"}, + ] + } + ] + } + tmp = _make_tmpdir_with_files({"server.json": json.dumps(manifest)}) + result = _detect_env_vars(tmp) + names = [r["name"] for r in result] + assert "GITHUB_PERSONAL_ACCESS_TOKEN" in names + + def test_remotes_variables(self): + from observal_cli.analyzer import _detect_env_vars + + manifest = { + "remotes": [ + { + "variables": { + "API_KEY": {"description": "The API key"}, + "SECRET": {"description": "A secret"}, + } + } + ] + } + tmp = _make_tmpdir_with_files({"server.json": json.dumps(manifest)}) + result = _detect_env_vars(tmp) + names = [r["name"] for r in result] + assert "API_KEY" in names + assert "SECRET" in names + + def test_manifest_stops_further_scanning(self): + """If server.json exists (even with 0 env vars), skip all other tiers.""" + from observal_cli.analyzer import _detect_env_vars + + tmp = _make_tmpdir_with_files( + { + "server.json": json.dumps({"packages": []}), + "README.md": "export MY_VAR=something", + "src/main.py": 'os.environ["SOME_KEY"]', + } + ) + result = _detect_env_vars(tmp) + assert result == [] + + def test_invalid_json_falls_through(self): + """Malformed server.json should fall through to next tier.""" + from observal_cli.analyzer import _detect_env_vars + + tmp = _make_tmpdir_with_files( + { + "server.json": "not valid json{{{", + "README.md": "export MY_TOKEN=xyz", + } + ) + result = _detect_env_vars(tmp) + names = [r["name"] for r in result] + assert "MY_TOKEN" in names + + +class TestTier2Readme: + """Tier 2: README env var extraction.""" + + def test_docker_e_flag(self): + from observal_cli.analyzer import _detect_env_vars + + tmp = _make_tmpdir_with_files({"README.md": "docker run -e MY_API_KEY -e MY_SECRET image:latest"}) + result = _detect_env_vars(tmp) + names = [r["name"] for r in result] + assert "MY_API_KEY" in names + assert "MY_SECRET" in names + + def test_export_statement(self): + from observal_cli.analyzer import _detect_env_vars + + tmp = _make_tmpdir_with_files({"README.md": "export OPENAI_API_KEY=sk-..."}) + result = _detect_env_vars(tmp) + names = [r["name"] for r in result] + assert "OPENAI_API_KEY" in names + + def test_json_config_key(self): + from observal_cli.analyzer import _detect_env_vars + + tmp = _make_tmpdir_with_files({"README.md": '{\n "SLACK_TOKEN": "xoxb-..."\n}'}) + result = _detect_env_vars(tmp) + names = [r["name"] for r in result] + assert "SLACK_TOKEN" in names + + def test_filters_internal_vars_from_readme(self): + from observal_cli.analyzer import _detect_env_vars + + tmp = _make_tmpdir_with_files( + {"README.md": "export PATH=/usr/bin\nexport NODE_ENV=production\nexport MY_TOKEN=abc"} + ) + result = _detect_env_vars(tmp) + names = [r["name"] for r in result] + assert "MY_TOKEN" in names + assert "PATH" not in names + assert "NODE_ENV" not in names + + def test_readme_stops_further_scanning(self): + from observal_cli.analyzer import _detect_env_vars + + tmp = _make_tmpdir_with_files( + { + "README.md": "export MY_TOKEN=abc", + ".env.example": "EXTRA_VAR=foo", + "src/main.py": 'os.getenv("CODE_VAR")', + } + ) + result = _detect_env_vars(tmp) + names = [r["name"] for r in result] + assert "MY_TOKEN" in names + assert "EXTRA_VAR" not in names + assert "CODE_VAR" not in names + + +class TestTier3EnvExample: + """Tier 3: .env.example file.""" + + def test_env_example_detected(self): + from observal_cli.analyzer import _detect_env_vars + + tmp = _make_tmpdir_with_files({".env.example": "API_KEY=\nDATABASE_URL=postgres://localhost/db\n"}) + result = _detect_env_vars(tmp) + names = [r["name"] for r in result] + assert "API_KEY" in names + assert "DATABASE_URL" in names + + def test_skips_comments_and_blanks(self): + from observal_cli.analyzer import _detect_env_vars + + tmp = _make_tmpdir_with_files({".env.example": "# This is a comment\n\nSECRET_KEY=mysecret\n"}) + result = _detect_env_vars(tmp) + names = [r["name"] for r in result] + assert "SECRET_KEY" in names + assert len(result) == 1 + + def test_skips_env_and_env_local(self): + """Should not scan .env or .env.local (actual secrets).""" + from observal_cli.analyzer import _detect_env_vars + + tmp = _make_tmpdir_with_files( + { + ".env": "REAL_SECRET=abc123", + ".env.local": "LOCAL_SECRET=def456", + } + ) + result = _detect_env_vars(tmp) + assert result == [] + + +class TestTier4SourceCode: + """Tier 4: Source code scanning (last resort).""" + + def test_python_os_environ(self): + from observal_cli.analyzer import _detect_env_vars + + tmp = _make_tmpdir_with_files( + {"src/main.py": 'import os\ntoken = os.environ["MY_TOKEN"]\nkey = os.getenv("MY_KEY")\n'} + ) + result = _detect_env_vars(tmp) + names = [r["name"] for r in result] + assert "MY_TOKEN" in names + assert "MY_KEY" in names + + def test_go_os_getenv(self): + from observal_cli.analyzer import _detect_env_vars + + tmp = _make_tmpdir_with_files( + {"cmd/main.go": 'package main\nimport "os"\nfunc main() { os.Getenv("API_TOKEN") }\n'} + ) + result = _detect_env_vars(tmp) + names = [r["name"] for r in result] + assert "API_TOKEN" in names + + def test_typescript_process_env(self): + from observal_cli.analyzer import _detect_env_vars + + tmp = _make_tmpdir_with_files( + {"src/index.ts": 'const key = process.env.OPENAI_KEY;\nconst s = process.env["MY_SECRET"];\n'} + ) + result = _detect_env_vars(tmp) + names = [r["name"] for r in result] + assert "OPENAI_KEY" in names + assert "MY_SECRET" in names + + def test_skips_test_directories(self): + from observal_cli.analyzer import _detect_env_vars + + tmp = _make_tmpdir_with_files( + { + "tests/test_config.py": 'os.environ["TEST_ONLY_VAR"]', + "src/main.py": 'os.getenv("REAL_VAR")', + } + ) + result = _detect_env_vars(tmp) + names = [r["name"] for r in result] + assert "REAL_VAR" in names + assert "TEST_ONLY_VAR" not in names + + def test_filters_internal_vars_from_code(self): + from observal_cli.analyzer import _detect_env_vars + + tmp = _make_tmpdir_with_files( + {"src/app.py": 'os.environ["PATH"]\nos.getenv("HOME")\nos.getenv("MY_CUSTOM_VAR")\n'} + ) + result = _detect_env_vars(tmp) + names = [r["name"] for r in result] + assert "MY_CUSTOM_VAR" in names + assert "PATH" not in names + assert "HOME" not in names + + +# ═══════════════════════════════════════════════════════════ +# 4. Server-side _detect_env_vars (mirrors CLI) +# ═══════════════════════════════════════════════════════════ + + +class TestServerEnvDetection: + """Verify server-side mcp_validator._detect_env_vars produces same results.""" + + def test_server_json_manifest(self): + from services.mcp_validator import _detect_env_vars + + manifest = { + "packages": [ + { + "runtimeArguments": [ + {"value": "MY_TOKEN={val}", "description": "desc"}, + ] + } + ] + } + tmp = _make_tmpdir_with_files({"server.json": json.dumps(manifest)}) + result = _detect_env_vars(tmp) + names = [r["name"] for r in result] + assert "MY_TOKEN" in names + + def test_readme_tier(self): + from services.mcp_validator import _detect_env_vars + + tmp = _make_tmpdir_with_files({"README.md": "export CUSTOM_KEY=value"}) + result = _detect_env_vars(tmp) + names = [r["name"] for r in result] + assert "CUSTOM_KEY" in names + + def test_source_scanning(self): + from services.mcp_validator import _detect_env_vars + + tmp = _make_tmpdir_with_files({"main.go": 'package main\nimport "os"\nfunc f() { os.Getenv("GO_TOKEN") }\n'}) + result = _detect_env_vars(tmp) + names = [r["name"] for r in result] + assert "GO_TOKEN" in names + + +# ═══════════════════════════════════════════════════════════ +# 5. Config Generation: _build_run_command +# ═══════════════════════════════════════════════════════════ + + +class TestBuildRunCommand: + def test_docker_image_generates_docker_run(self): + from services.config_generator import _build_run_command + + cmd = _build_run_command("my-mcp", "go", docker_image="ghcr.io/org/server:latest") + assert cmd[0] == "docker" + assert cmd[1] == "run" + assert "ghcr.io/org/server:latest" in cmd + + def test_docker_image_with_env_vars(self): + from services.config_generator import _build_run_command + + cmd = _build_run_command( + "my-mcp", + "go", + docker_image="ghcr.io/org/server:latest", + server_env={"GITHUB_PERSONAL_ACCESS_TOKEN": "tok123"}, + ) + assert "-e" in cmd + assert "GITHUB_PERSONAL_ACCESS_TOKEN=tok123" in cmd + # -e flags must come before the image + image_idx = cmd.index("ghcr.io/org/server:latest") + e_idx = cmd.index("-e") + assert e_idx < image_idx + + def test_no_docker_image_typescript(self): + from services.config_generator import _build_run_command + + cmd = _build_run_command("my-mcp", "typescript-mcp-sdk") + assert cmd == ["npx", "-y", "my-mcp"] + + def test_no_docker_image_go(self): + from services.config_generator import _build_run_command + + cmd = _build_run_command("my-mcp", "go-mcp-sdk") + assert cmd == ["my-mcp"] + + def test_no_docker_image_python(self): + from services.config_generator import _build_run_command + + cmd = _build_run_command("my-mcp", "python-mcp") + assert cmd == ["python", "-m", "my-mcp"] + + def test_no_docker_image_none_framework(self): + from services.config_generator import _build_run_command + + cmd = _build_run_command("my-mcp", None) + assert cmd == ["python", "-m", "my-mcp"] + + def test_docker_image_overrides_framework(self): + """Any framework with a docker_image should use docker run.""" + from services.config_generator import _build_run_command + + for fw in ("python-mcp", "typescript-mcp-sdk", "go-mcp-sdk", None): + cmd = _build_run_command("my-mcp", fw, docker_image="img:latest") + assert cmd[0] == "docker", f"Framework {fw} with docker_image should use docker run" + + +# ═══════════════════════════════════════════════════════════ +# 6. Config Generation: generate_config with docker listing +# ═══════════════════════════════════════════════════════════ + + +class TestGenerateConfigDocker: + def _make_listing(self, **kw): + listing = MagicMock() + listing.name = kw.get("name", "github-mcp-server") + listing.id = kw.get("listing_id", "abc-123") + listing.docker_image = kw.get("docker_image", "ghcr.io/github/github-mcp-server") + listing.framework = kw.get("framework", "go") + listing.environment_variables = kw.get( + "environment_variables", + [{"name": "GITHUB_PERSONAL_ACCESS_TOKEN", "description": "", "required": True}], + ) + return listing + + def test_cursor_docker_config(self): + from services.config_generator import generate_config + + listing = self._make_listing() + cfg = generate_config(listing, "cursor", env_values={"GITHUB_PERSONAL_ACCESS_TOKEN": "tok"}) + server = cfg["mcpServers"]["github-mcp-server"] + assert server["command"] == "observal-shim" + # The run command after -- should be docker run + args = server["args"] + sep_idx = args.index("--") + run_cmd = args[sep_idx + 1 :] + assert run_cmd[0] == "docker" + assert "ghcr.io/github/github-mcp-server" in run_cmd + + def test_claude_code_docker_config(self): + from services.config_generator import generate_config + + listing = self._make_listing() + cfg = generate_config(listing, "claude-code", env_values={"GITHUB_PERSONAL_ACCESS_TOKEN": "tok"}) + assert cfg["type"] == "shell_command" + # The command should contain docker run + cmd = cfg["command"] + assert "docker" in cmd + assert "ghcr.io/github/github-mcp-server" in cmd + + +# ═══════════════════════════════════════════════════════════ +# 7. Agent Config Generator: Claude Code with docker MCP +# ═══════════════════════════════════════════════════════════ + + +class TestAgentConfigDockerMcp: + def _make_agent(self, mcp_component_id=None): + agent = MagicMock() + agent.name = "test-agent" + agent.id = "agent-123" + agent.prompt = "You are a test agent." + agent.description = "A test agent" + agent.model_name = None + comp = MagicMock() + comp.component_type = "mcp" + comp.component_id = mcp_component_id or uuid.uuid4() + agent.components = [comp] + agent.external_mcps = [] + return agent + + def _make_listing(self): + listing = MagicMock() + listing.name = "github-mcp-server" + listing.id = uuid.uuid4() + listing.docker_image = "ghcr.io/github/github-mcp-server" + listing.framework = "go" + listing.environment_variables = [{"name": "GITHUB_PERSONAL_ACCESS_TOKEN", "description": "", "required": True}] + return listing + + def test_claude_code_passes_docker_image(self): + from services.agent_config_generator import generate_agent_config + + listing = self._make_listing() + comp_id = uuid.uuid4() + agent = self._make_agent(mcp_component_id=comp_id) + + cfg = generate_agent_config( + agent, + "claude-code", + mcp_listings={comp_id: listing}, + env_values={str(listing.id): {"GITHUB_PERSONAL_ACCESS_TOKEN": "tok"}}, + ) + mcp_config = cfg["mcp_config"] + assert "github-mcp-server" in mcp_config + mcp_entry = mcp_config["github-mcp-server"] + # Args should include docker run + args_str = " ".join(mcp_entry["args"]) + assert "docker" in args_str + assert "ghcr.io/github/github-mcp-server" in args_str + + def test_claude_code_passes_env_vars(self): + from services.agent_config_generator import generate_agent_config + + listing = self._make_listing() + comp_id = uuid.uuid4() + agent = self._make_agent(mcp_component_id=comp_id) + + cfg = generate_agent_config( + agent, + "claude-code", + mcp_listings={comp_id: listing}, + env_values={str(listing.id): {"GITHUB_PERSONAL_ACCESS_TOKEN": "tok"}}, + ) + mcp_entry = cfg["mcp_config"]["github-mcp-server"] + assert mcp_entry["env"]["GITHUB_PERSONAL_ACCESS_TOKEN"] == "tok" + + +# ═══════════════════════════════════════════════════════════ +# 8. MCP Submit: Auto-replace pending/rejected listings +# ═══════════════════════════════════════════════════════════ + + +class TestMcpSubmitAutoReplace: + @pytest.mark.asyncio + async def test_replace_pending_on_resubmit(self): + from api.routes.mcp import router + + user = _user() + db = _mock_db() + app = FastAPI() + app.include_router(router) + app.dependency_overrides[get_current_user] = lambda: user + app.dependency_overrides[get_db] = lambda: db + + # Existing pending listing with same name + existing = MagicMock() + existing.id = uuid.uuid4() + existing.name = "github-mcp-server" + existing.status = ListingStatus.pending + existing.submitted_by = user.id + + # First execute returns existing, second returns results for any other queries + db.execute = AsyncMock(return_value=_scalar_result(existing)) + + def _refresh(obj): + obj.id = uuid.uuid4() + obj.created_at = datetime.now(UTC) + obj.updated_at = datetime.now(UTC) + obj.status = ListingStatus.pending + obj.mcp_validated = False + obj.framework = None + obj.rejection_reason = None + + db.refresh = AsyncMock(side_effect=_refresh) + + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as ac: + r = await ac.post( + "/api/v1/mcps/submit", + json={ + "name": "github-mcp-server", + "version": "1.0.0", + "git_url": "https://github.com/github/github-mcp-server", + "description": "GitHub MCP server", + "category": "version-control", + "owner": "github", + "client_analysis": {"tools": [], "issues": []}, + }, + ) + + assert r.status_code == 200 + # The old listing should have been deleted + db.delete.assert_called_once_with(existing) + db.flush.assert_called_once() + + @pytest.mark.asyncio + async def test_reject_resubmit_of_approved(self): + from api.routes.mcp import router + + user = _user() + db = _mock_db() + app = FastAPI() + app.include_router(router) + app.dependency_overrides[get_current_user] = lambda: user + app.dependency_overrides[get_db] = lambda: db + + existing = MagicMock() + existing.id = uuid.uuid4() + existing.name = "github-mcp-server" + existing.status = ListingStatus.approved + existing.submitted_by = user.id + + db.execute = AsyncMock(return_value=_scalar_result(existing)) + + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as ac: + r = await ac.post( + "/api/v1/mcps/submit", + json={ + "name": "github-mcp-server", + "version": "1.0.0", + "git_url": "https://github.com/github/github-mcp-server", + "description": "GitHub MCP server", + "category": "version-control", + "owner": "github", + "client_analysis": {"tools": [], "issues": []}, + }, + ) + + assert r.status_code == 409 + assert "approved" in r.json()["detail"].lower() + + @pytest.mark.asyncio + async def test_no_existing_submits_normally(self): + from api.routes.mcp import router + + user = _user() + db = _mock_db() + app = FastAPI() + app.include_router(router) + app.dependency_overrides[get_current_user] = lambda: user + app.dependency_overrides[get_db] = lambda: db + + db.execute = AsyncMock(return_value=_scalar_result(None)) + + def _refresh(obj): + obj.id = uuid.uuid4() + obj.created_at = datetime.now(UTC) + obj.updated_at = datetime.now(UTC) + obj.status = ListingStatus.pending + obj.mcp_validated = False + obj.framework = None + obj.rejection_reason = None + + db.refresh = AsyncMock(side_effect=_refresh) + + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as ac: + r = await ac.post( + "/api/v1/mcps/submit", + json={ + "name": "github-mcp-server", + "version": "1.0.0", + "git_url": "https://github.com/github/github-mcp-server", + "description": "GitHub MCP server", + "category": "version-control", + "owner": "github", + "client_analysis": {"tools": [], "issues": []}, + }, + ) + + assert r.status_code == 200 + # No delete should have been called + db.delete.assert_not_called() diff --git a/tests/test_shim_phase3.py b/tests/test_shim_phase3.py index 9dd5178..ac4ced0 100644 --- a/tests/test_shim_phase3.py +++ b/tests/test_shim_phase3.py @@ -255,10 +255,13 @@ async def test_send_fire_and_forget(self): class TestConfigGenerator: - def _make_listing(self, name="my-mcp", listing_id="abc-123"): + def _make_listing(self, name="my-mcp", listing_id="abc-123", **kw): listing = MagicMock() listing.name = name listing.id = listing_id + listing.docker_image = kw.get("docker_image") + listing.framework = kw.get("framework") + listing.environment_variables = kw.get("environment_variables", []) return listing def test_cursor_wraps_with_shim(self):