From d28c0f04f04eb2cf3bddc102f670ab81274b846f Mon Sep 17 00:00:00 2001 From: Michael Lieberman Date: Mon, 17 Aug 2026 19:31:36 -0400 Subject: [PATCH 1/2] feat(sieve): add `mcp` handler for external MCP servers as observation sources Adds a new built-in sieve handler `mcp` symmetric with `exec`/`api_call`. TOML controls declare `handler = "mcp"` passes referencing an allowlisted server (`[mcp_servers.]` block on the framework TOML or `.baseline.toml`); the handler dispatches over stdio, exposes the response as `result.*` to CEL `expr`, and records the raw response plus trust label in evidence. Trust is allowlist-required (no allowlist entry -> ERROR without spawn); optional `trusted_publisher` triggers Sigstore sidecar verification. Child processes inherit only a curated env safe-set (PATH/HOME/LANG/LC_*/XDG_*/SSL_CERT_FILE) plus the operator's TOML `env` block. Sessions are spawned lazily on first use, pooled across the audit run via a persistent asyncio loop in a daemon thread, and torn down in `verify_batch`'s finally block. Absent binary defaults to INCONCLUSIVE (FAIL if `optional = false`); tool-side `isError=True` is ERROR without marking the session broken; a session crash triggers exactly one respawn. The orchestrator emits `[N/M] dispatching_mcp .` on `darnit.harness` at INFO before dispatch, matching feature 026's `dispatching_llm` pattern. Zero new runtime deps: uses existing `mcp>=1.23,<2` client-side APIs and the already-declared `darnit-core[attestation]` sigstore extra for the optional verification path. Scoped entirely to `packages/darnit/` core; no plugin package touched. Sandboxing beyond env curation (bubblewrap, nono.sh, landlock, nsjail) tracked as issue #375; `mcp_trust.verify` is deliberately isolated so that follow-up can extend the pre-spawn hooks without touching the pool. Spec: `specs/031-mcp-server-handler/` (52 tasks, all closed). --- .specify/feature.json | 2 +- CLAUDE.md | 2 +- .../src/darnit/config/framework_schema.py | 86 +++ packages/darnit/src/darnit/config/merger.py | 16 + .../darnit/src/darnit/config/user_schema.py | 6 + packages/darnit/src/darnit/core/models.py | 8 + .../src/darnit/sieve/builtin_handlers.py | 271 ++++++++ .../src/darnit/sieve/handler_registry.py | 5 + packages/darnit/src/darnit/sieve/mcp_pool.py | 568 +++++++++++++++ packages/darnit/src/darnit/sieve/mcp_trust.py | 130 ++++ .../darnit/src/darnit/sieve/orchestrator.py | 80 ++- packages/darnit/src/darnit/tools/audit.py | 38 + .../checklists/requirements.md | 38 + .../contracts/mcp-handler-contract.md | 136 ++++ specs/031-mcp-server-handler/data-model.md | 129 ++++ specs/031-mcp-server-handler/plan.md | 187 +++++ specs/031-mcp-server-handler/quickstart.md | 113 +++ specs/031-mcp-server-handler/research.md | 101 +++ specs/031-mcp-server-handler/spec.md | 145 ++++ specs/031-mcp-server-handler/tasks.md | 256 +++++++ tests/darnit/config/test_mcp_server_config.py | 86 +++ .../darnit/config/test_merger_mcp_servers.py | 49 ++ tests/darnit/sieve/conftest.py | 29 + .../fixtures/mock_mcp_server/__init__.py | 27 + .../fixtures/mock_mcp_server/__main__.py | 80 +++ tests/darnit/sieve/test_mcp_handler.py | 652 ++++++++++++++++++ tests/darnit/sieve/test_mcp_pool.py | 139 ++++ tests/darnit/sieve/test_mcp_trust.py | 82 +++ 28 files changed, 3454 insertions(+), 7 deletions(-) create mode 100644 packages/darnit/src/darnit/sieve/mcp_pool.py create mode 100644 packages/darnit/src/darnit/sieve/mcp_trust.py create mode 100644 specs/031-mcp-server-handler/checklists/requirements.md create mode 100644 specs/031-mcp-server-handler/contracts/mcp-handler-contract.md create mode 100644 specs/031-mcp-server-handler/data-model.md create mode 100644 specs/031-mcp-server-handler/plan.md create mode 100644 specs/031-mcp-server-handler/quickstart.md create mode 100644 specs/031-mcp-server-handler/research.md create mode 100644 specs/031-mcp-server-handler/spec.md create mode 100644 specs/031-mcp-server-handler/tasks.md create mode 100644 tests/darnit/config/test_mcp_server_config.py create mode 100644 tests/darnit/config/test_merger_mcp_servers.py create mode 100644 tests/darnit/sieve/conftest.py create mode 100644 tests/darnit/sieve/fixtures/mock_mcp_server/__init__.py create mode 100644 tests/darnit/sieve/fixtures/mock_mcp_server/__main__.py create mode 100644 tests/darnit/sieve/test_mcp_handler.py create mode 100644 tests/darnit/sieve/test_mcp_pool.py create mode 100644 tests/darnit/sieve/test_mcp_trust.py diff --git a/.specify/feature.json b/.specify/feature.json index cbe8efdf..2270c5c0 100644 --- a/.specify/feature.json +++ b/.specify/feature.json @@ -1 +1 @@ -{"feature_directory": "specs/030-dot-project-spec-sync"} +{"feature_directory": "specs/031-mcp-server-handler"} diff --git a/CLAUDE.md b/CLAUDE.md index 3f8783b7..74f21f77 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -381,5 +381,5 @@ else: For additional context about technologies to be used, project structure, shell commands, and other important information, read the current plan: -[`specs/030-dot-project-spec-sync/plan.md`](specs/030-dot-project-spec-sync/plan.md) +[`specs/031-mcp-server-handler/plan.md`](specs/031-mcp-server-handler/plan.md) diff --git a/packages/darnit/src/darnit/config/framework_schema.py b/packages/darnit/src/darnit/config/framework_schema.py index 5b21ae82..2962f3ae 100644 --- a/packages/darnit/src/darnit/config/framework_schema.py +++ b/packages/darnit/src/darnit/config/framework_schema.py @@ -1023,6 +1023,86 @@ class PluginConfig(BaseModel): model_config = ConfigDict(extra="allow") +class McpServerConfig(BaseModel): + """One allowlist entry describing an external MCP server darnit may spawn. + + A control's ``handler = "mcp"`` pass declares ``server = ""``; + that name MUST match a key under ``[mcp_servers.]`` in the + effective framework config (or in ``.baseline.toml``, which wins + per-name per spec FR-016). Absence of the entry produces ERROR at + audit time without spawning anything -- allowlist is the primary + trust boundary. + + Fields locked at the schema layer: + + * ``command`` is required and non-empty; the first element is the + executable name (resolved via ``PATH``) or an absolute path. + * ``env`` values MAY contain ``$VAR`` placeholders, substituted from + the operator's shell at spawn time. Unset variables substitute as + empty string, matching the ``exec`` handler. + * ``trusted_publisher`` triggers Sigstore sidecar verification when + set; failure to verify produces ERROR without ever entering + evidence (spec FR-007). + * ``optional = true`` (default) turns a missing binary into + INCONCLUSIVE; ``optional = false`` promotes it to FAIL. + * ``install_hint`` surfaces in the INCONCLUSIVE/FAIL message. + + ``extra = "forbid"`` locks spec FR-015: unknown fields such as a + hypothetical future ``transport = "http"`` MUST raise + :class:`ValidationError` at load time rather than silently accepting. + v0 supports stdio only; a future transport addition changes the + schema at that time. + """ + + command: list[str] = Field(..., min_length=1) + env: dict[str, str] = Field(default_factory=dict) + trusted_publisher: str | None = None + optional: bool = True + install_hint: str = "" + + model_config = ConfigDict(extra="forbid") + + @field_validator("command") + @classmethod + def _validate_command_nonempty(cls, v: list[str]) -> list[str]: + if not v: + raise ValueError( + "mcp_servers[*].command must be a non-empty list; the first " + "element names the executable (resolved via PATH) or is an " + "absolute path." + ) + return v + + @field_validator("trusted_publisher") + @classmethod + def _validate_trusted_publisher_shape(cls, v: str | None) -> str | None: + if v is None: + return None + stripped = v.strip() + if not stripped: + raise ValueError( + "mcp_servers[*].trusted_publisher, when set, must not be blank." + ) + # Advisory-only shape check: accept github.com URLs and bare + # owner/repo strings. Any other shape is permitted at the schema + # layer but produces a runtime verification failure at spawn. + import logging as _logging + + looks_like_github = ( + stripped.startswith("https://github.com/") + or stripped.startswith("http://github.com/") + or "/" in stripped.strip("/") + ) + if not looks_like_github: + _logging.getLogger(__name__).warning( + "mcp_servers[*].trusted_publisher=%r does not look like a " + "github.com URL or owner/repo pair; verification will " + "likely fail at spawn time.", + stripped, + ) + return stripped + + class PluginsConfig(BaseModel): """Container for plugin configurations. @@ -1420,6 +1500,12 @@ class FrameworkConfig(BaseModel): # Plugin configurations (for extending framework with additional handlers) plugins: PluginsConfig = Field(default_factory=PluginsConfig) + # Allowlist of external MCP servers this framework may consult via the + # built-in ``mcp`` sieve handler. Keyed by operator-chosen server name; + # the pass references it as ``server = ""``. Empty dict preserves + # backward-compatible behavior for every existing framework TOML. + mcp_servers: dict[str, McpServerConfig] = Field(default_factory=dict) + # Named audit profiles (optional, for multi-scenario implementations) audit_profiles: dict[str, AuditProfileConfig] = Field(default_factory=dict) diff --git a/packages/darnit/src/darnit/config/merger.py b/packages/darnit/src/darnit/config/merger.py index fe6c70c5..b8221fd7 100644 --- a/packages/darnit/src/darnit/config/merger.py +++ b/packages/darnit/src/darnit/config/merger.py @@ -73,6 +73,7 @@ ControlConfig, FrameworkConfig, FrameworkDefaults, + McpServerConfig, ) from .user_schema import ( ControlOverride, @@ -160,6 +161,11 @@ class EffectiveConfig: cache_ttl: int = 300 timeout: int = 300 + # Merged MCP-server allowlist: framework + .baseline.toml, with + # per-name replacement (spec FR-016). Empty dict is the pre-feature + # default and preserves backward compatibility. + mcp_servers: dict[str, "McpServerConfig"] = field(default_factory=dict) + # Source configs (for reference) _framework_config: FrameworkConfig | None = None _user_config: UserConfig | None = None @@ -376,6 +382,16 @@ def merge_configs( for name, adapter in user.adapters.items(): effective.adapters[name] = adapter + # Merge MCP-server allowlist (spec FR-016). + # Precedence: framework provides the base; each key present in + # `.baseline.toml` REPLACES the framework's block for that name + # entirely (no deep merge within a block; the operator's entry is + # authoritative). Disjoint names coexist. + effective.mcp_servers = dict(framework.mcp_servers) + if user: + for name, srv in user.mcp_servers.items(): + effective.mcp_servers[name] = srv + # Apply user settings if user: effective.cache_results = user.settings.cache_results diff --git a/packages/darnit/src/darnit/config/user_schema.py b/packages/darnit/src/darnit/config/user_schema.py index 0587a047..f86a1beb 100644 --- a/packages/darnit/src/darnit/config/user_schema.py +++ b/packages/darnit/src/darnit/config/user_schema.py @@ -44,6 +44,7 @@ CheckConfig, ControlConfig, HandlerInvocation, + McpServerConfig, RemediationConfig, ) @@ -257,6 +258,11 @@ class UserConfig(BaseModel): # Control groups for batch configuration control_groups: dict[str, ControlGroup] = Field(default_factory=dict) + # Per-fleet MCP-server allowlist entries. Keys here fully replace the + # framework's ``[mcp_servers.]`` block of the same name at merge + # time (spec FR-016); disjoint names coexist. + mcp_servers: dict[str, McpServerConfig] = Field(default_factory=dict) + model_config = ConfigDict(extra="allow") # ========================================================================= diff --git a/packages/darnit/src/darnit/core/models.py b/packages/darnit/src/darnit/core/models.py index e4642b83..2b492144 100644 --- a/packages/darnit/src/darnit/core/models.py +++ b/packages/darnit/src/darnit/core/models.py @@ -98,6 +98,14 @@ class ExecutionContext: # Already-computed check results cached_results: dict[str, CheckResult] = field(default_factory=dict) + # Feature 031: allowlist of external MCP servers (merged framework + + # `.baseline.toml`, per-name replacement) available to the built-in + # ``mcp`` sieve handler. Values are ``McpServerConfig`` instances but + # typed as ``Any`` here to avoid a config->core import cycle. Empty + # dict is the pre-feature default -- audits that never consult an + # MCP server pay zero cost. + mcp_servers: dict[str, Any] = field(default_factory=dict) + # Threading locks _lock: threading.Lock = field(default_factory=threading.Lock, init=False, repr=False) _tool_locks: dict[str, threading.Lock] = field(default_factory=dict, init=False, repr=False) diff --git a/packages/darnit/src/darnit/sieve/builtin_handlers.py b/packages/darnit/src/darnit/sieve/builtin_handlers.py index 88689e9f..607cf0e0 100644 --- a/packages/darnit/src/darnit/sieve/builtin_handlers.py +++ b/packages/darnit/src/darnit/sieve/builtin_handlers.py @@ -33,6 +33,18 @@ logger = logging.getLogger(__name__) +# ============================================================================= +# Feature 031: mcp handler constants +# ============================================================================= + +MCP_DEFAULT_TIMEOUT_SECONDS: float = 60.0 +"""Per-call timeout for `handler = "mcp"` passes when the pass omits `timeout`. + +Spec FR-002 (clarified 2026-08-16). Individual passes MAY override via +``timeout = ``. Kept as a module constant so tests can monkeypatch +it without stubbing the whole handler. +""" + # ============================================================================= # Verification Handlers @@ -941,6 +953,254 @@ def yaml_inject_handler(config: dict[str, Any], context: HandlerContext) -> Hand ) +# ============================================================================= +# Feature 031: mcp handler +# ============================================================================= + + +def mcp_handler(config: dict[str, Any], context: HandlerContext) -> HandlerResult: + """Call a tool on an allowlisted MCP server and evaluate ``expr`` over ``result.*``. + + Config fields: + server: Name of an allowlisted ``[mcp_servers.]`` block. + tool: Name of the tool to invoke on that server. + args: Dict of tool arguments; ``$OWNER``, ``$REPO``, ``$BRANCH``, + and ``$PATH`` placeholders in string values are substituted + from the ``HandlerContext``. + expr: Optional CEL expression evaluated over ``{"result": }``. + When absent, PASS iff the tool returned successfully. + timeout: Optional per-call timeout override in seconds. Defaults + to :data:`MCP_DEFAULT_TIMEOUT_SECONDS`. + + Emits :class:`HandlerResult` per the failure-mode table in + ``docs/architecture/feature-031/mcp-handler-contract.md``. Does NOT + emit the ``dispatching_mcp`` progress line -- the orchestrator's + dispatch site owns that so ``[N/M]`` counter state is available. + """ + server_name = config.get("server") + tool_name = config.get("tool") + args = dict(config.get("args") or {}) + expr = config.get("expr") + timeout = float(config.get("timeout", MCP_DEFAULT_TIMEOUT_SECONDS)) + + if not isinstance(server_name, str) or not server_name: + return HandlerResult( + status=HandlerResultStatus.ERROR, + message="mcp handler pass missing 'server' field", + ) + if not isinstance(tool_name, str) or not tool_name: + return HandlerResult( + status=HandlerResultStatus.ERROR, + message="mcp handler pass missing 'tool' field", + ) + + pool = context.mcp_pool + if pool is None: + return HandlerResult( + status=HandlerResultStatus.ERROR, + message="mcp handler invoked without pool wiring (internal error)", + ) + + server_config = _lookup_mcp_server(context, server_name) + substituted_args = _substitute_mcp_args(args, context) + + import time as _time + + from .mcp_pool import ( + McpServerBinaryMissing, + McpServerHandshakeFailed, + McpServerUnusable, + McpServerVerificationFailed, + McpToolError, + McpToolResponseNotJson, + McpToolTimeout, + UnknownMcpServer, + ) + + call_start = _time.time() + error_info: tuple[HandlerResultStatus, str] | None = None + raw_response: dict[str, Any] | None = None + trust_label: str + + try: + raw_response = pool.call_tool(server_name, tool_name, substituted_args, timeout) + except UnknownMcpServer as err: + error_info = (HandlerResultStatus.ERROR, str(err)) + except McpServerBinaryMissing as err: + # optional=true (default) -> INCONCLUSIVE; optional=false -> FAIL + optional = True + if server_config is not None: + optional = bool(getattr(server_config, "optional", True)) + status = HandlerResultStatus.INCONCLUSIVE if optional else HandlerResultStatus.FAIL + message = str(err) if optional else f"Required MCP server binary not found. {err}" + error_info = (status, message) + except McpServerVerificationFailed as err: + error_info = (HandlerResultStatus.ERROR, str(err)) + except McpServerHandshakeFailed as err: + # Contract: INCONCLUSIVE by default; FAIL when the operator marked + # the server as required (optional=false). + optional = True + if server_config is not None: + optional = bool(getattr(server_config, "optional", True)) + status = HandlerResultStatus.INCONCLUSIVE if optional else HandlerResultStatus.FAIL + error_info = (status, str(err)) + except McpServerUnusable as err: + # Broken twice -- treat like an unusable binary: INCONCLUSIVE unless + # the operator marked the server required (optional=false), then FAIL. + optional = True + if server_config is not None: + optional = bool(getattr(server_config, "optional", True)) + status = HandlerResultStatus.INCONCLUSIVE if optional else HandlerResultStatus.FAIL + error_info = (status, str(err)) + except McpToolTimeout as err: + error_info = (HandlerResultStatus.ERROR, str(err)) + except McpToolError as err: + error_info = (HandlerResultStatus.ERROR, str(err)) + except McpToolResponseNotJson as err: + error_info = (HandlerResultStatus.ERROR, str(err)) + except Exception as err: # noqa: BLE001 - final safety net + error_info = ( + HandlerResultStatus.ERROR, + f"MCP handler unexpected error: {type(err).__name__}: {err}", + ) + + elapsed_ms = int((_time.time() - call_start) * 1000) + + if server_config is not None: + trust_label = ( + "sigstore-verified" + if getattr(server_config, "trusted_publisher", None) + else "operator-trusted-path" + ) + else: + trust_label = "operator-trusted-path" + + session = pool._sessions.get(server_name) if hasattr(pool, "_sessions") else None + if session is not None: + trust_label = session.trust_label + + if error_info is not None: + status, message = error_info + invocation_record = { + "server": server_name, + "tool": tool_name, + "args_after_substitution": substituted_args, + "error": message, + "trust_label": trust_label, + "elapsed_ms": elapsed_ms, + } + evidence: dict[str, Any] = {"mcp_calls": [invocation_record]} + return HandlerResult(status=status, message=message, evidence=evidence) + + assert raw_response is not None + invocation_record = { + "server": server_name, + "tool": tool_name, + "args_after_substitution": substituted_args, + "raw_response": raw_response, + "trust_label": trust_label, + "elapsed_ms": elapsed_ms, + } + evidence = {"mcp_calls": [invocation_record], "result": raw_response} + + if expr: + cel_ok, cel_value, cel_error = _eval_cel_over_result(expr, raw_response) + if not cel_ok: + return HandlerResult( + status=HandlerResultStatus.ERROR, + message=f"MCP expr evaluation failed: {cel_error}", + evidence=evidence, + ) + if cel_value: + return HandlerResult( + status=HandlerResultStatus.PASS, + message=f"MCP {server_name}.{tool_name} expr matched", + confidence=1.0, + evidence=evidence, + ) + return HandlerResult( + status=HandlerResultStatus.FAIL, + message=f"MCP {server_name}.{tool_name} expr did not match", + evidence=evidence, + ) + + # No expr -> presence of a successful tool response is PASS. + return HandlerResult( + status=HandlerResultStatus.PASS, + message=f"MCP {server_name}.{tool_name} returned successfully", + confidence=1.0, + evidence=evidence, + ) + + +def _lookup_mcp_server(context: HandlerContext, server_name: str) -> Any | None: + """Return the ``McpServerConfig`` for ``server_name`` on the exec context, if any.""" + execution_context = context.execution_context + if execution_context is None: + return None + servers = getattr(execution_context, "mcp_servers", None) + if not isinstance(servers, dict): + return None + return servers.get(server_name) + + +def _substitute_mcp_args(args: dict[str, Any], context: HandlerContext) -> dict[str, Any]: + """Substitute ``$OWNER``/``$REPO``/``$BRANCH``/``$PATH`` in string values.""" + replacements = { + "OWNER": context.owner or "", + "REPO": context.repo or "", + "BRANCH": context.default_branch or "main", + "PATH": context.local_path or "", + } + out: dict[str, Any] = {} + for key, value in args.items(): + if isinstance(value, str): + out[key] = _apply_replacements(value, replacements) + else: + out[key] = value + return out + + +def _apply_replacements(template: str, replacements: dict[str, str]) -> str: + result: list[str] = [] + i = 0 + while i < len(template): + ch = template[i] + if ch == "$" and i + 1 < len(template): + end = i + 1 + while end < len(template) and (template[end].isalnum() or template[end] == "_"): + end += 1 + if end > i + 1: + name = template[i + 1 : end] + if name in replacements: + result.append(replacements[name]) + i = end + continue + result.append(ch) + i += 1 + return "".join(result) + + +def _eval_cel_over_result( + expr: str, raw_response: dict[str, Any] +) -> tuple[bool, Any, str | None]: + """Evaluate ``expr`` against ``{"result": raw_response}``. + + Returns ``(ok, value, error)``. ``ok=False`` means evaluation itself + failed (surface as ERROR); ``ok=True`` means it produced a value that + the caller interprets as truthy/falsy. + """ + try: + from .cel_evaluator import evaluate_cel + except Exception as err: # noqa: BLE001 - CEL evaluator import surprise + return False, None, f"CEL evaluator unavailable: {err}" + + cel_result = evaluate_cel(expr, {"result": raw_response}) + if not cel_result.success: + return False, None, str(cel_result.error) + return True, cel_result.value, None + + # ============================================================================= # Registration # ============================================================================= @@ -1015,6 +1275,17 @@ def register_builtin_handlers() -> None: description="Alias for manual_steps handler (human verification checklist)", default_authority="asserted", ) + # Feature 031: external MCP server as observation source. Dispositive + # because the tool observes ground truth (a real subprocess reports + # its state); the trust label separately surfaces whether the binary + # was Sigstore-verified or operator-trusted-on-PATH. + registry.register( + "mcp", + phase="deterministic", + handler_fn=mcp_handler, + description="Call a tool on an external MCP server; evaluate CEL over result.*", + default_authority="dispositive", + ) # Remediation handlers registry.register( diff --git a/packages/darnit/src/darnit/sieve/handler_registry.py b/packages/darnit/src/darnit/sieve/handler_registry.py index 8143ec32..30a396b7 100644 --- a/packages/darnit/src/darnit/sieve/handler_registry.py +++ b/packages/darnit/src/darnit/sieve/handler_registry.py @@ -112,6 +112,11 @@ class HandlerContext: shared_cache: dict[str, HandlerResult] = field(default_factory=dict) dependency_results: dict[str, Any] = field(default_factory=dict) execution_context: Any | None = None + # Feature 031: assigned by the orchestrator's dispatch site when the + # invocation targets the built-in ``mcp`` handler. None for every + # other handler kind. A ``None`` value inside the mcp handler + # indicates a plumbing bug and MUST resolve the pass ERROR. + mcp_pool: Any | None = None # Handler callable signature: (config, context) -> HandlerResult diff --git a/packages/darnit/src/darnit/sieve/mcp_pool.py b/packages/darnit/src/darnit/sieve/mcp_pool.py new file mode 100644 index 00000000..e6115131 --- /dev/null +++ b/packages/darnit/src/darnit/sieve/mcp_pool.py @@ -0,0 +1,568 @@ +"""Per-audit MCP client-session pool. + +Spawn-lazy: no session is created until a control's mcp-handler pass actually +references its server. Teardown-in-finally: verify_batch guarantees every +session is closed before returning, even on exceptions. + +Sync-over-async bridge: MCP's stdio client requires a live event loop for +the subprocess's read/write pumps. Darnit's sieve orchestrator is +synchronous, so the pool owns one background asyncio loop running in a +daemon thread for the pool's whole lifetime. Every sync ``call_tool`` / +``acquire`` / ``teardown_all`` call submits a coroutine via +``run_coroutine_threadsafe`` and blocks on the future. + +Public surface used by the handler: :class:`McpPool.call_tool`. The pool +owns the allowlist (dict of server name -> McpServerConfig) and the session +cache. The exception hierarchy at the bottom of this module names every +distinct failure mode the handler maps to a HandlerResult status (see the +failure-mode table in ``contracts/mcp-handler-contract.md``). + +The sandbox follow-up (issue #375) will extend :func:`McpPool._spawn`'s +pre-spawn hooks (env curation, Sigstore verification) without touching the +pool's session cache or the handler's public shape. +""" + +from __future__ import annotations + +import asyncio +import concurrent.futures +import json +import logging +import os +import shutil +import sys +import threading +from dataclasses import dataclass +from datetime import datetime +from pathlib import Path +from typing import TYPE_CHECKING, Any, Literal + +if TYPE_CHECKING: + from darnit.config.framework_schema import McpServerConfig # noqa: F401 + +logger = logging.getLogger(__name__) + + +# ============================================================================= +# Constants +# ============================================================================= + +MCP_ENV_SAFE_KEYS: tuple[str, ...] = ("PATH", "HOME", "LANG", "SSL_CERT_FILE") +MCP_ENV_SAFE_PREFIXES: tuple[str, ...] = ("LC_", "XDG_") +MCP_ENV_SAFE_KEYS_WINDOWS: tuple[str, ...] = ("SYSTEMROOT", "SYSTEMDRIVE") +MCP_PROGRESS_VERB: str = "dispatching_mcp" + + +# ============================================================================= +# Exception hierarchy +# +# Each concrete exception names the failure mode the handler maps to a +# HandlerResult status. Kept flat and specific so a caller can distinguish +# "binary missing" (INCONCLUSIVE/FAIL depending on `optional`) from +# "handshake failed" (INCONCLUSIVE always) without string-matching messages. +# ============================================================================= + + +class McpPoolError(Exception): + """Base class for every pool-side failure.""" + + +class McpServerBinaryMissing(McpPoolError): + """The allowlisted `command[0]` is not resolvable on PATH.""" + + +class McpServerVerificationFailed(McpPoolError): + """`trusted_publisher` was set and Sigstore verification did not pass.""" + + +class McpServerHandshakeFailed(McpPoolError): + """The MCP `initialize` handshake failed or the child exited early.""" + + +class McpServerUnusable(McpPoolError): + """The session was broken and a single respawn also failed.""" + + +class UnknownMcpServer(McpPoolError): + """No allowlist entry exists for the referenced server name.""" + + +class McpToolTimeout(McpPoolError): + """`session.call_tool` did not return within the configured timeout.""" + + +class McpToolError(McpPoolError): + """The MCP tool explicitly returned isError=True with a message.""" + + +class McpToolResponseNotJson(McpPoolError): + """Tool response content was non-text or not JSON-parseable.""" + + +# ============================================================================= +# PooledSession +# ============================================================================= + + +@dataclass +class PooledSession: + """One live MCP client session, cached in the pool by server name. + + Lifecycle: FRESH -> USED -> TEARDOWN. Any call that raises a + session-level error (crash, timeout, handshake death) transitions the + session to BROKEN; the next reference respawns exactly once. A double- + broken session raises :class:`McpServerUnusable` on further references. + Tool-side errors (`isError=True`) are NOT session-level failures and do + not mark the session broken. + """ + + server_name: str + config: Any # McpServerConfig -- typed as Any to avoid the import cycle + session: Any | None # mcp.ClientSession, or None while broken + trust_label: Literal["sigstore-verified", "operator-trusted-path"] + spawn_ts: datetime + broken: bool = False + # Owner task: holds the stdio_client + ClientSession contexts open on + # the pool's bridge loop. Closes on _shutdown_event.set(). + _owner_task: Any = None + _shutdown_event: Any = None + + def mark_broken(self) -> None: + self.broken = True + + def is_healthy(self) -> bool: + return not self.broken and self.session is not None + + +# ============================================================================= +# _LoopBridge -- sync-over-async +# ============================================================================= + + +class _LoopBridge: + """One long-lived asyncio loop running in a daemon thread. + + The pool submits coroutines here so stdio-client subprocesses stay + alive across successive sync ``call_tool`` invocations. Kept private + to this module; not part of the reader contract. + """ + + def __init__(self) -> None: + self._loop = asyncio.new_event_loop() + self._ready = threading.Event() + self._thread = threading.Thread( + target=self._runner, + name="darnit-mcp-pool-loop", + daemon=True, + ) + self._thread.start() + self._ready.wait() + + def _runner(self) -> None: + asyncio.set_event_loop(self._loop) + self._ready.set() + try: + self._loop.run_forever() + finally: + try: + self._loop.close() + except Exception: # noqa: BLE001 - shutdown best-effort + pass + + def run(self, coro: Any, timeout: float | None = None) -> Any: + future = asyncio.run_coroutine_threadsafe(coro, self._loop) + try: + return future.result(timeout=timeout) + except concurrent.futures.TimeoutError as err: + future.cancel() + raise McpToolTimeout(f"MCP call exceeded {timeout:g}s") from err + + def close(self) -> None: + if not self._loop.is_running(): + return + self._loop.call_soon_threadsafe(self._loop.stop) + self._thread.join(timeout=5) + + +# ============================================================================= +# McpPool +# ============================================================================= + + +class McpPool: + """Per-audit-run pool of MCP client sessions keyed by server name. + + Owns the allowlist (a dict of ``server_name -> McpServerConfig``) and + the cache of live sessions. Constructed lazily by the orchestrator on + first reference to an mcp-handler pass; torn down in the orchestrator's + ``verify_batch`` finally block. + """ + + def __init__( + self, + servers: dict[str, Any] | None = None, + trust_verifier: Any = None, + ) -> None: + """Initialise the pool. + + Args: + servers: Allowlist mapping ``server_name -> McpServerConfig``. + A missing entry at ``call_tool`` time raises + :class:`UnknownMcpServer`. + trust_verifier: Callable ``(binary_path, trusted_publisher) + -> (ok: bool, reason: str)`` used when a server's config + declares ``trusted_publisher``. Defaults to + :func:`darnit.sieve.mcp_trust.verify` -- injected here so + tests can substitute a monkeypatched verifier without + reaching into module globals. + """ + self._servers: dict[str, Any] = dict(servers or {}) + self._sessions: dict[str, PooledSession] = {} + self._bridge: _LoopBridge | None = None + if trust_verifier is None: + from darnit.sieve import mcp_trust + + trust_verifier = mcp_trust.verify + self._verify = trust_verifier + + # ------------------------------------------------------------------ + # Public API + # ------------------------------------------------------------------ + + def call_tool( + self, + server_name: str, + tool: str, + args: dict[str, Any], + timeout: float, + ) -> dict[str, Any]: + """Call ``tool`` on ``server_name`` and return the parsed response. + + Raises the specific pool exception on any failure. The mcp handler + is responsible for mapping each exception to a HandlerResult status + per the failure-mode table. + """ + config = self._servers.get(server_name) + if config is None: + raise UnknownMcpServer(f"unknown MCP server: {server_name}") + + session = self.acquire(server_name, config) + assert session.session is not None # acquire returns healthy session + + try: + result = self._bridge_run( + self._call_tool_async(session.session, tool, args, timeout), + timeout=timeout + 5, + ) + except McpToolTimeout: + session.mark_broken() + raise + except McpPoolError: + raise + except Exception as err: # noqa: BLE001 - session-level surprise + session.mark_broken() + raise McpServerHandshakeFailed( + f"MCP session error during {server_name}.{tool}: {err}" + ) from err + + # mcp.CallToolResult carries `.isError` and `.content` (list of + # content parts). Success responses are expected to carry one text + # part containing JSON. + is_error = bool(getattr(result, "isError", False)) + content = getattr(result, "content", None) or [] + if is_error: + message = _first_text(content) or "tool reported isError without message" + raise McpToolError(f"MCP tool error: {message}") + + text = _first_text(content) + if text is None: + raise McpToolResponseNotJson( + f"MCP tool response was non-text content on {server_name}.{tool}" + ) + try: + parsed = json.loads(text) + except json.JSONDecodeError as err: + raise McpToolResponseNotJson( + f"MCP tool response not JSON-parseable on {server_name}.{tool}: {err}" + ) from err + if not isinstance(parsed, dict): + raise McpToolResponseNotJson( + f"MCP tool response was not a JSON object on {server_name}.{tool}" + ) + return parsed + + def acquire(self, server_name: str, config: Any) -> PooledSession: + """Return a healthy :class:`PooledSession`, respawning at most once.""" + session = self._sessions.get(server_name) + if session is not None and session.is_healthy(): + return session + + if session is not None and session.broken: + self._teardown_one(session) + self._sessions.pop(server_name, None) + fresh = self._spawn(server_name, config) + if not fresh.is_healthy(): + raise McpServerUnusable( + f"MCP server session broken and respawn failed for {server_name}" + ) + return fresh + + return self._spawn(server_name, config) + + def teardown_all(self) -> None: + """Close every cached session, best-effort, then clear the cache. + + Also stops the background asyncio loop -- the pool is single-use; + callers who need a fresh pool construct a new one. + """ + for session in list(self._sessions.values()): + self._teardown_one(session) + self._sessions.clear() + if self._bridge is not None: + self._bridge.close() + self._bridge = None + + # ------------------------------------------------------------------ + # Env curation + # ------------------------------------------------------------------ + + @staticmethod + def build_child_env(server_config: Any) -> dict[str, str]: + """Compose the child process env: safe-set + operator TOML block. + + The safe-set is a small allowlist (PATH, HOME, LANG, LC_*, XDG_*, + SSL_CERT_FILE). ``$VAR`` placeholders in the operator's TOML + ``env`` block are substituted from the parent shell at spawn time; + unset variables substitute as empty string (matching the ``exec`` + handler behavior). + """ + parent = dict(os.environ) + env: dict[str, str] = {} + allow_keys = set(MCP_ENV_SAFE_KEYS) + if sys.platform == "win32": + allow_keys.update(MCP_ENV_SAFE_KEYS_WINDOWS) + for key, value in parent.items(): + if key in allow_keys or any(key.startswith(p) for p in MCP_ENV_SAFE_PREFIXES): + env[key] = value + + for key, template in (getattr(server_config, "env", {}) or {}).items(): + env[key] = _substitute_dollar_vars(str(template), parent) + return env + + # ------------------------------------------------------------------ + # Spawn / teardown internals + # ------------------------------------------------------------------ + + def _spawn(self, server_name: str, config: Any) -> PooledSession: + command = list(getattr(config, "command", []) or []) + if not command: + raise McpServerHandshakeFailed( + f"MCP server '{server_name}' has empty command" + ) + program = command[0] + binary_path: Path + if os.path.isabs(program): + binary_path = Path(program) + if not binary_path.exists(): + raise McpServerBinaryMissing(_absent_binary_message(program, config)) + else: + resolved = shutil.which(program) + if resolved is None: + raise McpServerBinaryMissing(_absent_binary_message(program, config)) + binary_path = Path(resolved) + + trust_label: Literal["sigstore-verified", "operator-trusted-path"] + trusted_publisher = getattr(config, "trusted_publisher", None) + if trusted_publisher: + ok, reason = self._verify(binary_path, trusted_publisher) + if not ok: + raise McpServerVerificationFailed( + f"Sigstore verification failed for {program}: {reason}" + ) + trust_label = "sigstore-verified" + else: + trust_label = "operator-trusted-path" + + env = self.build_child_env(config) + + # Ensure the bridge loop is running before we schedule the owner + # task on it. + if self._bridge is None: + self._bridge = _LoopBridge() + loop = self._bridge._loop + + ready_future: concurrent.futures.Future[Any] = concurrent.futures.Future() + + async def _run_session_owner() -> None: + shutdown = asyncio.Event() + try: + session, _stack = await _open_session_async(command, env) + except Exception as err: # noqa: BLE001 -- surface to caller + ready_future.set_exception(err) + return + ready_future.set_result((session, shutdown)) + try: + await shutdown.wait() + finally: + # _open_session_async's AsyncExitStack was returned to us + # but keeping the with-scope in this task ensures cancel + # scopes stay bound to this same task. + await _stack.aclose() + + owner_task_future = asyncio.run_coroutine_threadsafe( + _run_session_owner(), loop + ) + + try: + session, shutdown = ready_future.result(timeout=30) + except concurrent.futures.TimeoutError as err: + owner_task_future.cancel() + raise McpServerHandshakeFailed( + f"MCP handshake for {server_name} exceeded 30s" + ) from err + except Exception as err: # noqa: BLE001 -- handshake surprises + owner_task_future.cancel() + raise McpServerHandshakeFailed( + f"MCP handshake failed for {server_name}: {err}" + ) from err + + pooled = PooledSession( + server_name=server_name, + config=config, + session=session, + trust_label=trust_label, + spawn_ts=datetime.now(), + _owner_task=owner_task_future, + _shutdown_event=shutdown, + ) + self._sessions[server_name] = pooled + return pooled + + def _teardown_one(self, session: PooledSession) -> None: + shutdown = session._shutdown_event + owner = session._owner_task + if shutdown is None or owner is None: + return + loop = self._bridge._loop if self._bridge is not None else None + try: + if loop is not None: + loop.call_soon_threadsafe(shutdown.set) + # Wait for the owner task to finish (which closes the async + # exit stack in the task that opened it, avoiding anyio's + # "different-task" cancel-scope error). + try: + owner.result(timeout=5) + except concurrent.futures.TimeoutError: + logger.warning( + "MCP session teardown for %s did not complete within 5s", + session.server_name, + ) + except Exception as err: # noqa: BLE001 - best-effort close + logger.warning( + "MCP session teardown for %s raised %s: %s", + session.server_name, + type(err).__name__, + err, + ) + finally: + session._shutdown_event = None + session._owner_task = None + session.session = None + + # ------------------------------------------------------------------ + # Sync-over-async bridge access (private) + # ------------------------------------------------------------------ + + def _bridge_run(self, coro: Any, timeout: float | None = None) -> Any: + if self._bridge is None: + self._bridge = _LoopBridge() + return self._bridge.run(coro, timeout=timeout) + + @staticmethod + async def _call_tool_async( + session: Any, tool: str, args: dict[str, Any], timeout: float + ) -> Any: + return await asyncio.wait_for(session.call_tool(tool, args), timeout=timeout) + + +# ============================================================================= +# Module helpers +# ============================================================================= + + +def _absent_binary_message(program: str, config: Any) -> str: + """Build the operator-facing message for a missing MCP server binary.""" + install_hint = getattr(config, "install_hint", "") or "" + message = f"MCP server binary not found: {program}" + if install_hint: + message = f"{message}. {install_hint}" + return message + + +def _substitute_dollar_vars(template: str, env: dict[str, str]) -> str: + """Replace ``$VAR`` occurrences with values from ``env``; empty if unset.""" + result: list[str] = [] + i = 0 + while i < len(template): + ch = template[i] + if ch == "$" and i + 1 < len(template): + end = i + 1 + while end < len(template) and (template[end].isalnum() or template[end] == "_"): + end += 1 + if end > i + 1: + name = template[i + 1 : end] + result.append(env.get(name, "")) + i = end + continue + result.append(ch) + i += 1 + return "".join(result) + + +def _first_text(parts: list[Any]) -> str | None: + for part in parts: + text = getattr(part, "text", None) + if isinstance(text, str): + return text + return None + + +async def _open_session_async( + command: list[str], env: dict[str, str] +) -> tuple[Any, Any]: + """Enter stdio_client + ClientSession contexts; return (session, stack). + + The returned :class:`contextlib.AsyncExitStack` owns both context + managers and MUST be closed on the same event loop that opened it. + """ + from contextlib import AsyncExitStack + + from mcp import ClientSession + from mcp.client.stdio import StdioServerParameters, stdio_client + + params = StdioServerParameters(command=command[0], args=command[1:], env=env) + + stack = AsyncExitStack() + streams = await stack.enter_async_context(stdio_client(params)) + read, write = streams + session = await stack.enter_async_context(ClientSession(read, write)) + await session.initialize() + return session, stack + + +__all__ = [ + "MCP_ENV_SAFE_KEYS", + "MCP_ENV_SAFE_PREFIXES", + "MCP_PROGRESS_VERB", + "McpPool", + "McpPoolError", + "McpServerBinaryMissing", + "McpServerHandshakeFailed", + "McpServerUnusable", + "McpServerVerificationFailed", + "McpToolError", + "McpToolResponseNotJson", + "McpToolTimeout", + "PooledSession", + "UnknownMcpServer", +] diff --git a/packages/darnit/src/darnit/sieve/mcp_trust.py b/packages/darnit/src/darnit/sieve/mcp_trust.py new file mode 100644 index 00000000..2e965edf --- /dev/null +++ b/packages/darnit/src/darnit/sieve/mcp_trust.py @@ -0,0 +1,130 @@ +"""Sigstore sidecar verification for MCP server binaries. + +Isolated so the sandboxing follow-up (issue #375) can extend the pre-spawn +hooks (bubblewrap, nono.sh, landlock, nsjail) without touching the pool's +session cache or the handler. + +Verification model: an operator declares ``trusted_publisher`` in a +``[mcp_servers.]`` block; the pool looks for +``.sigstore`` or ``.sigstore.json`` next to the resolved +binary path and verifies it against a GitHub-workflow identity policy +derived from ``trusted_publisher``. Failure returns a ``(False, reason)`` +tuple; the pool maps that to :class:`McpServerVerificationFailed`, which +the handler resolves ERROR. There is NO code path from verification failure +to PASS. + +Alternatives considered: + +* Fetching a transparency-log attestation by SHA-256 at spawn time was + rejected because Constitution II ("conservative-by-default") forbids + silently requiring network I/O during an audit. +* Chaining verification into the plugin-signing surface was rejected + because MCP server binaries are external tools, not darnit plugins; + the two trust domains are different. +""" + +from __future__ import annotations + +import logging +from pathlib import Path + +logger = logging.getLogger(__name__) + + +def verify(binary_path: Path, trusted_publisher: str) -> tuple[bool, str]: + """Verify a Sigstore sidecar next to ``binary_path``. + + Args: + binary_path: The resolved path of the MCP server binary. + trusted_publisher: Either a full GitHub identity URL + (``https://github.com/[/]``) or a bare + ```` / ``/`` string. + + Returns: + ``(True, reason)`` if verification succeeds against a policy + derived from ``trusted_publisher``; ``(False, reason)`` on any + failure -- missing sidecar, malformed bundle, sigstore SDK not + installed, or policy mismatch. The pool never sees an exception + from this function. + """ + sidecar = _find_sidecar(binary_path) + if sidecar is None: + return False, ( + f"no Sigstore sidecar found next to {binary_path} " + f"(looked for .sigstore and .sigstore.json)" + ) + + try: + from sigstore.models import Bundle # type: ignore[import-not-found] + from sigstore.verify import Verifier # type: ignore[import-not-found] + from sigstore.verify.policy import ( # type: ignore[import-not-found] + GitHubWorkflowRepository, + ) + except ImportError: + return False, ( + "sigstore not installed -- install darnit-core[attestation] " + "to enable trusted_publisher verification" + ) + + try: + bundle = Bundle.from_json(sidecar.read_bytes()) + except Exception as err: # noqa: BLE001 - sigstore raises assorted subclasses + return False, f"Sigstore verification failed: could not parse bundle: {err}" + + repo_ref = _extract_repo_ref(trusted_publisher) + if repo_ref is None: + return False, ( + f"Sigstore verification failed: trusted_publisher " + f"{trusted_publisher!r} does not name a GitHub owner/repo" + ) + + try: + policy = GitHubWorkflowRepository(repo_ref) + verifier = Verifier.production() + verifier.verify_dsse(bundle, policy) + except Exception as err: # noqa: BLE001 - sigstore raises assorted subclasses + return False, f"Sigstore verification failed: {err}" + + return True, f"verified against {trusted_publisher}" + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _find_sidecar(binary_path: Path) -> Path | None: + for suffix in (".sigstore", ".sigstore.json"): + candidate = binary_path.with_name(binary_path.name + suffix) + if candidate.exists(): + return candidate + return None + + +def _extract_repo_ref(trusted_publisher: str) -> str | None: + """Extract the ``owner/repo`` reference from a ``trusted_publisher`` value. + + Accepts: + + * ``https://github.com/`` + * ``https://github.com//`` + * ```` (bare) + * ``/`` (bare) + + Returns ``owner/repo`` when a repo is present; ``owner/`` is invalid for + :class:`GitHubWorkflowRepository`, so an owner-only value returns + ``None`` and the caller reports the failure. + """ + stripped = trusted_publisher.strip() + for prefix in ("https://github.com/", "http://github.com/"): + if stripped.startswith(prefix): + stripped = stripped[len(prefix) :] + break + stripped = stripped.strip("/") + parts = stripped.split("/", 1) + if len(parts) == 2 and parts[0] and parts[1]: + return f"{parts[0]}/{parts[1]}" + return None + + +__all__ = ["verify"] diff --git a/packages/darnit/src/darnit/sieve/orchestrator.py b/packages/darnit/src/darnit/sieve/orchestrator.py index 93d9ff99..2a4a748b 100644 --- a/packages/darnit/src/darnit/sieve/orchestrator.py +++ b/packages/darnit/src/darnit/sieve/orchestrator.py @@ -1,5 +1,6 @@ """Sieve orchestrator - runs verification passes in order.""" +import logging import time from enum import Enum from typing import Any @@ -28,6 +29,11 @@ logger = get_logger("sieve.orchestrator") +# ``darnit.harness`` logger is where feature 026 emits ``dispatching_llm`` +# INFO progress lines; feature 031 emits its ``dispatching_mcp`` twin on +# the same logger so a harness watcher subscribes to one channel for both. +_harness_logger = logging.getLogger("darnit.harness") + # ============================================================================= # RFC-0001 Stage 1 (feature 025): per-phase Check execution rule @@ -236,6 +242,10 @@ def __init__(self, stop_on_llm: bool = True): self._shared_cache: dict[str, HandlerResult] = {} # Dependency results: keyed by control ID, populated as controls are verified self._dependency_results: dict[str, SieveResult] = {} + # MCP client-session pool. Constructed lazily on the first dispatch + # that references ``handler = "mcp"``; torn down in + # ``verify_batch``'s finally block or on ``reset_caches``. + self._mcp_pool: Any | None = None def reset_caches(self) -> None: """Reset shared handler cache and dependency results. @@ -244,6 +254,12 @@ def reset_caches(self) -> None: """ self._shared_cache.clear() self._dependency_results.clear() + if self._mcp_pool is not None: + try: + self._mcp_pool.teardown_all() + except Exception as err: # noqa: BLE001 - best-effort teardown + logger.warning("MCP pool teardown during reset raised: %s", err) + self._mcp_pool = None def _evaluate_when(self, control_spec: ControlSpec, context: CheckContext) -> bool: """Evaluate when clause for conditional applicability. @@ -367,6 +383,29 @@ def _dispatch_handler_invocations( handler_config = dict(invocation.model_extra or {}) handler_config["handler"] = invocation.handler + # Feature 031: for the built-in mcp handler, lazily + # construct the pool, assign it to the HandlerContext, and + # emit the [N/M] dispatching_mcp progress line on the + # darnit.harness logger BEFORE the handler runs. Emission + # here (not inside the handler) mirrors feature 026's + # dispatching_llm pattern and gives us the (idx, total) + # counter from the enumerate loop directly. + if invocation.handler == "mcp": + if self._mcp_pool is None: + self._mcp_pool = _build_mcp_pool(handler_ctx) + handler_ctx.mcp_pool = self._mcp_pool + server = handler_config.get("server", "?") + tool_name = handler_config.get("tool", "?") + total = len(handler_invocations) + _harness_logger.info( + "[%d/%d] %s dispatching_mcp %s.%s", + pass_index + 1, + total, + control_spec.control_id, + server, + tool_name, + ) + start_time = time.time() try: handler_result = handler_info.fn(handler_config, handler_ctx) @@ -712,12 +751,24 @@ def verify_batch( # Resolve execution order ordered = _resolve_execution_order(control_specs) - # Execute in dependency order, collect results + # Execute in dependency order, collect results. The finally block + # guarantees the MCP pool (if any was constructed) is torn down on + # every exit path -- success, exception, or interrupt. result_map: dict[str, SieveResult] = {} - for spec in ordered: - context = context_factory(spec.control_id) - result = self.verify(spec, context) - result_map[spec.control_id] = result + try: + for spec in ordered: + context = context_factory(spec.control_id) + result = self.verify(spec, context) + result_map[spec.control_id] = result + finally: + if self._mcp_pool is not None: + try: + self._mcp_pool.teardown_all() + except Exception as err: # noqa: BLE001 - best-effort teardown + logger.warning( + "MCP pool teardown at verify_batch exit raised: %s", err + ) + self._mcp_pool = None # Return in original order return [result_map[spec.control_id] for spec in control_specs if spec.control_id in result_map] @@ -789,6 +840,25 @@ def _apply_on_pass( # ============================================================================= +def _build_mcp_pool(handler_ctx: HandlerContext) -> Any: + """Construct a per-run :class:`McpPool` seeded from the execution context. + + The execution context (assigned by the audit entrypoint) carries an + ``mcp_servers`` mapping when the effective configuration declared any. + An audit run that never encounters an mcp-handler pass never reaches + this function, so the pool cost stays zero for existing consumers. + """ + from .mcp_pool import McpPool + + servers: dict[str, Any] = {} + execution_context = handler_ctx.execution_context + if execution_context is not None: + maybe = getattr(execution_context, "mcp_servers", None) + if isinstance(maybe, dict): + servers = maybe + return McpPool(servers=servers) + + def _handler_status_to_outcome(status: HandlerResultStatus) -> PassOutcome: """Convert HandlerResultStatus to PassOutcome.""" mapping = { diff --git a/packages/darnit/src/darnit/tools/audit.py b/packages/darnit/src/darnit/tools/audit.py index 74422ce1..bbe424b1 100644 --- a/packages/darnit/src/darnit/tools/audit.py +++ b/packages/darnit/src/darnit/tools/audit.py @@ -144,6 +144,31 @@ def _get_framework_config_path(framework_name: str | None = None) -> Path | None return None +def _load_merged_mcp_servers( + local_path: str, framework_name: str | None +) -> dict[str, Any]: + """Return the merged ``mcp_servers`` allowlist for this audit run. + + Composes the framework TOML's block with any ``.baseline.toml`` + overrides via the standard :func:`merge_configs` rule (per-name + replacement, spec FR-016). Returns an empty dict when no framework + is resolved or neither surface declares any servers. + """ + from darnit.config import ( + load_framework_config, + load_user_config, + merge_configs, + ) + + framework_path = _get_framework_config_path(framework_name) + if not framework_path: + return {} + framework = load_framework_config(framework_path) + user = load_user_config(Path(local_path)) + effective = merge_configs(framework, user) + return dict(effective.mcp_servers) + + def load_effective_audit_config(local_path: str, framework_name: str | None = None) -> Any | None: """Load the effective configuration for auditing. @@ -427,6 +452,19 @@ def run_sieve_audit( repo=repo, local_path=local_path, ) + + # Feature 031: populate the MCP-server allowlist on the execution + # context so the sieve orchestrator can lazy-construct the pool when + # a control's pass references `handler = "mcp"`. Failure to load + # merged config here is non-fatal -- audits that don't use MCP + # simply see an empty allowlist and any mcp handler pass resolves + # ERROR ("unknown MCP server: ...") at dispatch time. + try: + execution_context.mcp_servers = _load_merged_mcp_servers( + local_path, resolved_fw + ) + except Exception as err: # noqa: BLE001 - config load must not break audit + logger.debug("MCP allowlist load failed (non-fatal): %s", err) all_results: list[CheckResult] = [] # Build project_context once for all controls. diff --git a/specs/031-mcp-server-handler/checklists/requirements.md b/specs/031-mcp-server-handler/checklists/requirements.md new file mode 100644 index 00000000..6b2e79d0 --- /dev/null +++ b/specs/031-mcp-server-handler/checklists/requirements.md @@ -0,0 +1,38 @@ +# Specification Quality Checklist: mcp handler for calling external MCP servers as observation sources + +**Purpose**: Validate specification completeness and quality before proceeding to planning +**Created**: 2026-08-16 +**Feature**: [spec.md](../spec.md) + +## Content Quality + +- [x] No implementation details (languages, frameworks, APIs) +- [x] Focused on user value and business needs +- [x] Written for non-technical stakeholders +- [x] All mandatory sections completed + +## Requirement Completeness + +- [x] No [NEEDS CLARIFICATION] markers remain +- [x] Requirements are testable and unambiguous +- [x] Success criteria are measurable +- [x] Success criteria are technology-agnostic (no implementation details) +- [x] All acceptance scenarios are defined +- [x] Edge cases are identified +- [x] Scope is clearly bounded +- [x] Dependencies and assumptions identified + +## Feature Readiness + +- [x] All functional requirements have clear acceptance criteria +- [x] User scenarios cover primary flows +- [x] Feature meets measurable outcomes defined in Success Criteria +- [x] No implementation details leak into specification + +## Notes + +- Content Quality: The spec names two entities that live in code (`mcp` handler, `[mcp_servers.]` TOML block) because those are the surface being designed, not incidental stack choices. A stakeholder ignoring implementation details still needs to know that "the new thing" is a handler control authors reference from TOML and that the operator configuration lives in a specific block name. +- Requirement Completeness: No clarification markers were introduced. Three points that could have become clarifications are handled in Assumptions instead because reasonable defaults exist: (a) the Scorecard-backed reference control is deferred to a follow-up feature, (b) HTTP/SSE transport is deferred, (c) cross-audit caching is deferred. The three-option-tradeoff conversation happened in-chat before spec draft; the results are locked into FR-010 (Stage 1 authority), FR-011 (spawn-lazy-per-audit), and FR-005 through FR-009 (allowlist-required, Sigstore-optional trust). +- Success Criteria: All five are technology-agnostic and measurable. SC-002 references a "mock server that counts its own lifecycle events" as a verification method rather than a system requirement; the mock is a testing artifact, not a system component. +- FR-010 (Stage 1 authority) is intentionally distinct from other FRs because it names the constitution property the spec is aligning with; a downstream reviewer can point at that FR when re-checking Constitution IV alignment during the plan phase. +- Non-goals for v0 are enumerated in Assumptions rather than a separate section because the template does not have a Non-Goals section. Every non-goal is phrased "MUST NOT" or "out of scope" so downstream `/speckit-plan` cannot accidentally re-in-scope them without a spec update. diff --git a/specs/031-mcp-server-handler/contracts/mcp-handler-contract.md b/specs/031-mcp-server-handler/contracts/mcp-handler-contract.md new file mode 100644 index 00000000..c9b79524 --- /dev/null +++ b/specs/031-mcp-server-handler/contracts/mcp-handler-contract.md @@ -0,0 +1,136 @@ +# Reader Contract: `mcp` handler + +## Scope + +The public control-author-facing surface of the `mcp` handler, its allowlist declaration, its evidence shape, its progress-log line, and its exhaustive failure-mode table. This is the file a future reconciliation-style feature will diff against to detect breaking changes. + +## TOML pass surface + +Inside a `[[controls..passes]]` block: + +| Field | Type | Required | Notes | +|-------|------|----------|-------| +| `handler` | string | required | Value MUST be `"mcp"`. | +| `server` | string | required | References an `[mcp_servers.]` allowlist entry by name. Unknown name resolves the pass ERROR without spawning. | +| `tool` | string | required | The MCP tool name to invoke on the server. | +| `args` | table | required | Arguments passed to the tool. Values may contain `$OWNER`, `$REPO`, `$BRANCH`, `$PATH` substitution tokens; the handler performs substitution before dispatch, symmetric with `exec` handler. | +| `expr` | string | optional | CEL truth expression evaluated against `result.*` (the tool's response). If omitted, the presence of a non-error response is treated as PASS. Same evaluation model as `exec`'s `expr`. | +| `timeout` | integer (seconds) | optional | Per-call timeout. Defaults to `60` (spec FR-002, clarified 2026-08-16). Timeout expiration resolves the affected pass ERROR. | +| `authority` | string | optional | Standard sieve authority override. May tighten (dispositive → suggestive) but not loosen. Default: `dispositive` (from the handler's registered `default_authority`). | + +## TOML allowlist surface + +Top-level `[mcp_servers.]` block, either in `.baseline.toml` or a framework TOML: + +| Field | Type | Required | Notes | +|-------|------|----------|-------| +| `command` | list of strings | required | argv-style. First element is the executable (resolved via PATH) or an absolute path. | +| `env` | table of string→string | optional | Extra env vars for the child process. Values may contain `$VAR` placeholders; darnit substitutes from its own `os.environ` at spawn time (empty string when unset). | +| `trusted_publisher` | string | optional | GitHub identity URL (`https://github.com/` or `...//`) or OIDC identity. When present, darnit verifies the binary's Sigstore sidecar (`.sigstore` or `.sigstore.json`) against this publisher before spawning. | +| `optional` | bool | optional | Defaults to `true`. When `true`, absence of the binary produces INCONCLUSIVE. When `false`, absence produces FAIL. | +| `install_hint` | string | optional | One-line hint surfaced in the INCONCLUSIVE/FAIL message when the binary is missing (e.g., `"Install with: brew install scorecard-mcp"`). | + +Merge precedence: `.baseline.toml` block wins over framework TOML block of the same name. + +## Child process environment + +At spawn time, the child process env is constructed as: + +``` +child_env = { + k: os.environ[k] for k in os.environ + if k in {"PATH", "HOME", "LANG", "SSL_CERT_FILE"} + or k.startswith("LC_") + or k.startswith("XDG_") +} | { + tk: substitute($VARS_from_os_environ)(tv) + for tk, tv in server_config.env.items() +} +``` + +No other operator-shell env variable is visible to the child. Notably absent by default: `AWS_*`, `GITHUB_TOKEN`, `ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, any user-set variable not in the safe-set. An operator who needs one of these in the child MUST name it in the TOML `env` block explicitly. + +## CEL binding + +The tool's response is bound in CEL as `result.*`. Example: + +```toml +expr = 'result.score >= 7.0 && result.date > "2026-01-01"' +``` + +The response must be JSON-parseable text content in the `CallToolResult`. Non-JSON responses (image content, binary blobs) resolve the pass ERROR with a "non-JSON response" reason. + +## Evidence shape (`McpInvocationRecord`) + +Every invocation writes one record into the control's `evidence["mcp_calls"]` list: + +```json +{ + "server": "scorecard", + "tool": "get_repo_score", + "args_after_substitution": {"repo_url": "github.com/octo/hello"}, + "raw_response": {"score": 8.5, "date": "2026-08-01"}, + "trust_label": "sigstore-verified", + "elapsed_ms": 812 +} +``` + +On error, `raw_response` is omitted and `error` is set: + +```json +{ + "server": "scorecard", + "tool": "get_repo_score", + "args_after_substitution": {"repo_url": "github.com/octo/hello"}, + "error": "tool timeout after 60s", + "trust_label": "sigstore-verified", + "elapsed_ms": 60003 +} +``` + +`trust_label` is one of `"sigstore-verified"` (successful Sigstore verification against `trusted_publisher`) or `"operator-trusted-path"` (spawned based on allowlist entry alone, no verification attempted). + +## Progress-log shape + +At tool dispatch, exactly one INFO log line on the `darnit.harness` logger: + +``` +[{n}/{m}] {control_id} dispatching_mcp {server}.{tool} +``` + +Where `{n}/{m}` is the standard sieve control-progress counter (matching feature 026's `[N/M]` format), `{control_id}` is the current control being resolved, `{server}` is the allowlist key, `{tool}` is the tool name. No corresponding "returned" line; the terminal `resolved_pass` / `resolved_fail` / `resolved_error` / `resolved_inconclusive` line for the control conveys completion. + +## Failure-mode table + +Every failure path and its resulting control status: + +| Failure mode | Control status | Evidence reason | +|--------------|----------------|-----------------| +| Referenced `server` not in `[mcp_servers.*]` allowlist | ERROR | `unknown MCP server: ` | +| Binary from `command` not on PATH (`optional = true`) | INCONCLUSIVE | `MCP server binary not found: . ` | +| Binary from `command` not on PATH (`optional = false`) | FAIL | `Required MCP server binary not found: . ` | +| `trusted_publisher` set, no sidecar or verification fails | ERROR | `Sigstore verification failed for : ` | +| Spawn succeeded but MCP handshake failed or timed out | INCONCLUSIVE (or FAIL if `optional=false`) | `MCP handshake failed: ` | +| Handshake succeeded, tool invocation timed out | ERROR | `MCP tool call timed out after s` | +| Handshake succeeded, tool returned `isError=True` | ERROR | `MCP tool error: ` | +| Handshake succeeded, tool returned non-JSON content | ERROR | `MCP tool response not JSON-parseable` | +| Session crashed mid-audit, respawn attempted, respawn succeeded | (call retries against fresh session) | (evidence records the respawn) | +| Session crashed mid-audit, respawn attempted, respawn failed | INCONCLUSIVE (or FAIL if `optional=false`) | `MCP server session broken and respawn failed: ` | +| Audit exits (success, fail, exception, interrupt) with active session | (control status unchanged) | Session is terminated before darnit process exits. No orphan. | + +## Backward compatibility + +This feature is a strict addition. Every framework TOML and `.baseline.toml` that parsed successfully before this feature MUST continue to parse successfully after it. Zero existing controls change behavior. The `[mcp_servers]` schema section is optional; absence is the pre-feature state. + +## Non-goals for v0 + +The following are deferred and are NOT part of this contract: + +- HTTP or SSE transport. Only stdio is supported. +- Parallel invocation of the same server (v0 is serial). +- Cross-audit result caching. +- `darnit install-mcp ` install-helper subcommand. +- Sandbox tooling beyond env-curation (tracked in issue #375). +- Retrofit of the env-safe-set to the existing `exec` handler (separate feature). + +Any control author who assumes a deferred feature is available should get a clear error (e.g., an `mcp_servers.` block with a hypothetical `transport = "http"` field would fail schema validation, not silently no-op). diff --git a/specs/031-mcp-server-handler/data-model.md b/specs/031-mcp-server-handler/data-model.md new file mode 100644 index 00000000..07817fce --- /dev/null +++ b/specs/031-mcp-server-handler/data-model.md @@ -0,0 +1,129 @@ +# Phase 1 Data Model: mcp handler + +## Purpose + +Enumerate every new type, its fields, its constraints, and its lifecycle. The vocabulary here is what the plan phase locks in for the reader contract, the tasks decomposition, and future reconciliation-style diffs. + +## New types + +### `McpServerConfig` (Pydantic model in `framework_schema.py`) + +An allowlist entry declaring one MCP server darnit may spawn. + +| Field | Type | Default | Constraint | +|-------|------|---------|------------| +| `command` | `list[str]` | required (no default) | Non-empty. First element is the executable name (resolved via PATH) or an absolute path; subsequent elements are argv. | +| `env` | `dict[str, str]` | `{}` | Values MAY contain `$VAR` placeholders. `$VAR` is resolved against darnit's parent `os.environ` at spawn time; unset variables substitute as empty string (matching `exec` handler behavior). | +| `trusted_publisher` | `str \| None` | `None` | When set, MUST be a GitHub identity URL (`https://github.com/` or `https://github.com//`) or an OIDC identity string. Verified against a Sigstore sidecar (`.sigstore` or `.sigstore.json`) at spawn time; verification failure produces ERROR without spawning. | +| `optional` | `bool` | `True` | When `True`, absence of the binary produces INCONCLUSIVE. When `False`, absence produces FAIL. | +| `install_hint` | `str` | `""` | Free-form. Surfaces in the INCONCLUSIVE/FAIL message when the binary is missing. Recommend one line, imperative form (`Install with: brew install scorecard-mcp`). | + +Merge precedence: `.baseline.toml` block wins over framework TOML block of the same name (spec FR-016). + +### `FrameworkConfig.mcp_servers` (new field on existing model) + +```python +mcp_servers: dict[str, McpServerConfig] = Field(default_factory=dict) +``` + +Key is the operator-chosen server name (referenced by controls as `server = ""`). Empty dict is the pre-feature default; any framework or `.baseline.toml` without an `[mcp_servers]` section behaves identically to before this feature. + +### `PooledSession` (runtime dataclass in `mcp_pool.py`) + +```python +@dataclass +class PooledSession: + server_name: str + config: McpServerConfig + session: ClientSession | None # None while broken + trust_label: Literal["sigstore-verified", "operator-trusted-path"] + spawn_ts: datetime + broken: bool = False +``` + +Lifecycle: `FRESH` → `USED` on first call → `TEARDOWN` on audit end. Any call that raises (crash, timeout, MCP-level error other than tool-side isError) transitions to `BROKEN`; a subsequent reference to the same server name triggers exactly one respawn attempt. A double-broken session produces INCONCLUSIVE/FAIL without further respawn attempts, per FR-012. + +Pool holds one `PooledSession` per server name. Uniqueness by `server_name` within the pool's audit run. + +### `McpInvocationRecord` (evidence-dict shape) + +Not a distinct type in code — this is the shape darnit writes into a control's evidence dict per invocation, mirrored here so the reader contract and future maintainers can point at one definition. + +```python +{ + "server": "scorecard", + "tool": "get_repo_score", + "args_after_substitution": {"repo_url": "github.com/octo/hello"}, + "raw_response": {"score": 8.5, "date": "2026-08-01", ...}, # or omitted if error + "error": "tool raise_error: reason 'x'", # or omitted if success + "trust_label": "sigstore-verified", # or "operator-trusted-path" + "elapsed_ms": 812, +} +``` + +## Existing types touched + +### `SieveOrchestrator` (in `orchestrator.py`) + +Add: + +```python +self._mcp_pool: dict[str, PooledSession] = {} # cleared in reset_caches() +``` + +Modify `reset_caches()`: + +```python +def reset_caches(self) -> None: + self._shared_cache.clear() + self._dependency_results.clear() + self._mcp_pool.clear() # NEW +``` + +Modify `verify_batch()` to wrap its per-control loop in a `try/finally` that tears down all live pool sessions before returning. + +Modify `_dispatch_handler_invocations` (or the equivalent dispatch site inside `verify_batch`) to emit `f"[{idx}/{total}] {control_spec.control_id} dispatching_mcp {invocation.server}.{invocation.tool}"` on the `darnit.harness` logger at INFO level **before** invoking a handler where `invocation.handler == "mcp"`. The counter `(idx, total)` is threaded from `verify_batch`'s enumeration loop. The handler function itself stays log-free at its dispatch site because a caller-observed side effect belongs to the caller (matches feature 026's `dispatching_llm` pattern; see spec FR-019). + +### `HandlerContext` (in `handler_registry.py`) + +Add: + +```python +mcp_pool: McpPool | None = None # assigned by the orchestrator's + # dispatch site alongside shared_cache + # and dependency_results when the + # invocation is an mcp handler; None + # for every other handler kind. +``` + +Handler function reads `context.mcp_pool` to obtain the pool. A `None` value at handler-invocation time indicates a plumbing bug and MUST resolve the pass ERROR rather than crash. Default value preserves every existing call site. + +No signature changes on any public callable. + +## Constants introduced + +- `MCP_DEFAULT_TIMEOUT_SECONDS = 60` in `builtin_handlers.py`, referenced by `mcp_handler`. +- `MCP_ENV_SAFE_KEYS = ("PATH", "HOME", "LANG", "SSL_CERT_FILE")` and `MCP_ENV_SAFE_PREFIXES = ("LC_", "XDG_")` in `mcp_pool.py`, referenced by the spawn helper. +- `MCP_PROGRESS_VERB = "dispatching_mcp"` in `mcp_pool.py` (or the handler module), referenced by the progress-log line. + +## Trust-label state machine + +``` + sigstore verify passes spawn succeeds + ---[trusted_publisher set]---> [SIGSTORE-VERIFIED] -----------> PooledSession + / +[McpServerConfig] + \ spawn succeeds + ---[trusted_publisher unset]---> [OPERATOR-TRUSTED-PATH] ---------> PooledSession +``` + +`sigstore verify fails` → no spawn, no session, ERROR on all controls that would use this server for the current audit. + +`binary absent + optional=true` → INCONCLUSIVE. +`binary absent + optional=false` → FAIL. + +The trust label is opaque to authority resolution — both paths produce `dispositive` results (spec FR-010; observation-based). The label appears only in the evidence record for auditors and downstream attestation. + +## Non-model concerns + +Everything else about this feature (CEL binding, arg substitution, timeout enforcement) reuses machinery that already exists in the sieve. No new pydantic models, no new dataclasses, no schema migrations beyond the two additions above. diff --git a/specs/031-mcp-server-handler/plan.md b/specs/031-mcp-server-handler/plan.md new file mode 100644 index 00000000..f649991e --- /dev/null +++ b/specs/031-mcp-server-handler/plan.md @@ -0,0 +1,187 @@ +# Implementation Plan: mcp handler for calling external MCP servers as observation sources + +**Branch**: `031-mcp-server-handler` | **Date**: 2026-08-17 | **Spec**: [spec.md](./spec.md) + +**Input**: Feature specification from `specs/031-mcp-server-handler/spec.md` (with 3 clarifications recorded 2026-08-16: curated env safe-set for child processes; 60-second default per-call timeout; `dispatching_mcp` INFO progress line symmetric with feature 026's `dispatching_llm`). + +## Summary + +Add a new built-in sieve handler named `mcp` that lets TOML controls invoke tools on external MCP (Model Context Protocol) servers as observation sources, symmetric with the existing `exec` and `api_call` handlers. Servers are declared in the framework configuration under `[mcp_servers.]` blocks with a required `command` field (allowlist entry) and optional `env`, `trusted_publisher`, `optional`, `install_hint`. When a control's pass references `server = ""`, the handler dispatches the tool call over stdio, exposes the response as `result.*` to the CEL `expr`, and records the raw response plus trust label in evidence. Server sessions are spawned lazily on first use, pooled across the audit run, and torn down at audit end. Trust is allowlist-required (no allowlist entry → ERROR without spawning) with optional `trusted_publisher` Sigstore verification (verification failure → ERROR, never PASS). Child processes inherit only a curated env safe-set (PATH, HOME, LANG, LC_*, SSL_CERT_FILE, XDG_*) plus the operator's TOML `env` block; other parent-shell vars are dropped. The feature adds no new runtime dependency: `mcp>=1.23,<2` is already declared for darnit-core, and this feature uses its client-side APIs. + +Reference server (`uwu-tools/scorecard-mcp`) is deliberately out of scope for v0; the machinery ships with a mock MCP server driving the integration test. + +## Technical Context + +**Language/Version**: Python 3.11/3.12 (workspace targets - same as the rest of darnit). + +**Primary Dependencies**: `mcp>=1.23,<2` (already a runtime dep, declared for darnit-core; this feature uses its client-side `mcp.client.stdio.stdio_client` + `mcp.ClientSession` APIs). `sigstore` (already declared under `darnit-core[attestation]`) for the optional `trusted_publisher` verification path. No new pip dependencies. + +**Storage**: Filesystem only. `.baseline.toml` and framework TOMLs are the sole persistence surface for `[mcp_servers.]` declarations. Pool state and tool-invocation records are audit-run scoped and never persisted across audits. + +**Testing**: pytest, extending existing `tests/darnit/sieve/` and `tests/darnit/config/` layers. New fixture: an in-repo mock MCP server (a small Python module using `mcp.server` primitives) used exclusively by the feature's integration test to verify spawn-lazy semantics, tool invocation, timeout enforcement, and teardown. The mock server counts its own lifecycle events so SC-002 (spawns == 1, terminations == 1 across 20 controls) is mechanically verifiable. + +**Target Platform**: Same as darnit workspace - any platform Python 3.11+ runs on. Constraint: the MCP client-side stdio transport uses process pipes, which behave identically across Linux, macOS, and Windows-with-WSL. No platform-specific code paths introduced. + +**Project Type**: Library/framework internal change; scoped to `packages/darnit/` core. No new packages, no new plugins. + +**Performance Goals**: Not a hot path; MCP calls are network-bound by definition and dominate their own timing. Spec's SC-002 (single spawn across N controls) is the primary performance property. Non-goal: minimize handshake latency; that belongs to the MCP SDK. + +**Constraints**: +- Zero product-source additions outside `packages/darnit/`. +- No new required arguments on any public callable that existing internal callers pass without modification (matches feature 030 FR-008 in spirit). +- Default per-call timeout MUST be 60 seconds (spec FR-002, clarified 2026-08-16). Handler-level default; per-pass override via `timeout = `. +- Child process environment MUST be constructed as the union of a curated safe-set inherited from darnit's own process AND the TOML `env` block; no other parent-shell env leaks through (spec FR-005, clarified 2026-08-16). +- `dispatching_mcp` INFO log line MUST fire on `darnit.harness` at call dispatch (spec FR-019, clarified 2026-08-16) using the `[N/M] dispatching_mcp .` shape. + +**Scale/Scope**: One new handler module (`darnit_mcp_handler` inside `builtin_handlers.py`) + one new pool module in `packages/darnit/src/darnit/sieve/mcp_pool.py` + a schema extension to `FrameworkConfig` + one integration test with a mock server. Estimated diff: ~600 lines of production code, ~500 lines of test code (mostly the mock server + fixtures). + +## Constitution Check + +*GATE: Must pass before Phase 0 research. Re-check after Phase 1 design.* + +The darnit constitution (5 core principles, plus architecture constraints and workflow rules) evaluated against this feature: + +| Principle | Applies | Assessment | +|-----------|---------|------------| +| I. Plugin Separation | Yes | PASS. The `mcp` handler + pool live in `packages/darnit/src/darnit/sieve/` (core framework). This feature does not import any implementation package. Implementation packages may still register their own domain-specific handlers via `register_handlers()` unchanged. | +| II. Conservative-by-Default | Yes | PASS. Absence of the allowlisted binary produces INCONCLUSIVE by default (never PASS) with a specific install-hint message; `[mcp_servers.].optional = false` promotes absence to FAIL. Sigstore verification failure produces ERROR (never PASS). A tool invocation that errors resolves the affected control ERROR, never leaks into another control's evidence. Every one of these paths is a "silence is safe" posture. | +| III. TOML-First Architecture | Yes | PASS. Control-author-facing surface is entirely in TOML (`handler = "mcp"` on a pass, `[mcp_servers.]` allowlist blocks). No Python-code-only escape hatch. Existing control-loader validation extends to the new `mcp_servers` field via a schema addition to `FrameworkConfig`. | +| IV. Never Guess User Values | Yes | PASS. MCP tool results are observation-based dispositive (external tool observed ground truth) - the handler registers with `default_authority = "dispositive"`. It does NOT synthesize `asserted` authority: an operator-trusted PATH binary and a Sigstore-verified binary both produce `dispositive` results because the tool is observing ground truth; the trust label surfaces separately on the evidence record. | +| V. Sieve Pipeline Integrity | Yes | PASS. The handler returns a single `HandlerResult` per invocation; the orchestrator's phase semantics and disposition logic are unchanged. `dispatching_mcp` progress log is a side effect, not a phase modifier. INCONCLUSIVE from missing binary correctly falls through to the next pass (or resolves the control per the sieve orchestrator's `_dispatch_handler_invocations`, the existing flat-invocation dispatch loop). | + +Architecture constraints (three-layer architecture, package structure): PASS. The change is confined to `packages/darnit/` core. No new layers or packages. + +Development workflow (lint, tests, spec sync, no-emoji rules): PASS. Standard workflow; no new gates required. + +**Gate result: PASS. Proceed to Phase 0.** + +## Project Structure + +### Documentation (this feature) + +```text +specs/031-mcp-server-handler/ +├── plan.md # This file +├── research.md # Phase 0 output — MCP client-API choice, pool lifecycle, mock-server design +├── data-model.md # Phase 1 output — new schema types (McpServerConfig, PooledSession, invocation record) +├── quickstart.md # Phase 1 output — control-author + operator worked example +├── contracts/ +│ └── mcp-handler-contract.md # Phase 1 output — TOML surface + evidence shape + log shape +├── checklists/ +│ └── requirements.md # From /speckit-specify +└── tasks.md # /speckit-tasks output (not created here) +``` + +### Source Code (repository root) + +```text +packages/darnit/src/darnit/sieve/ +├── builtin_handlers.py # Add `mcp_handler` alongside exec/api_call. Registers via register_builtin_handlers(). +├── mcp_pool.py # NEW. Per-audit connection pool: spawn-lazy, session cache, teardown-on-exit. +├── mcp_trust.py # NEW. Sigstore verification for `trusted_publisher`; small module isolated so the sandboxing follow-up (issue #375) can extend it without touching the pool. +├── orchestrator.py # Small edit: initialize the pool on the orchestrator instance; clear it in reset_caches(); teardown in verify_batch() finally-block. +└── (no new modules beyond mcp_pool + mcp_trust) + +packages/darnit/src/darnit/config/ +└── framework_schema.py # Add McpServerConfig BaseModel + `mcp_servers: dict[str, McpServerConfig]` on FrameworkConfig. + +packages/darnit/src/darnit/harness/ +└── (no changes — dispatching_mcp log line is emitted by the handler on darnit.harness logger; harness-side wiring is already generic) + +tests/darnit/sieve/ +├── test_mcp_handler.py # NEW. Unit tests: TOML parsing, arg substitution, expr evaluation over result, timeout, absent-binary paths. +├── test_mcp_pool.py # NEW. Unit tests: spawn-lazy, session reuse, teardown on all exit paths, respawn-on-invalidation, single-retry. +├── test_mcp_trust.py # NEW. Unit tests: Sigstore-verify path, verification failure, trusted_publisher absent (operator-trusted PATH label). +└── fixtures/ + └── mock_mcp_server/ # NEW. Small Python mock MCP server that counts spawn/teardown/tool-call events. Uses the `mcp` package's own server primitives. + +tests/darnit/config/ +└── test_framework_schema.py # Add coverage for [mcp_servers.] block: required command, optional env/trusted_publisher/optional/install_hint, precedence between .baseline.toml and framework TOML. +``` + +**Structure Decision**: Split concerns into three small modules under `sieve/`: the handler itself (in the existing `builtin_handlers.py` alongside its peers), the per-audit pool (`mcp_pool.py`), and the trust verification (`mcp_trust.py`). The trust module is deliberately isolated so issue #375's sandbox exploration can extend the pre-spawn hooks without needing to change the pool. Testing extends existing `tests/darnit/sieve/` and `tests/darnit/config/`; a new `fixtures/mock_mcp_server/` directory holds the mock server used by the integration test. + +## Complexity Tracking + +No constitution violations to justify. The feature is a scoped handler addition with zero new architecture. + +## Phase 0: Research + +Research questions surfaced by Technical Context and the spec's Assumptions/Edge Cases: + +1. **What is the correct MCP client-side API surface to use?** — The `mcp` package (already a darnit-core runtime dep) exposes both server and client primitives. The client-side entry is `mcp.client.stdio.stdio_client` returning a context-manager pair of read/write streams, plus `mcp.ClientSession` wrapping them into a call surface. Research confirms the specific API version (mcp>=1.23) exposes `session.call_tool(name, arguments)` returning a `CallToolResult`, and that this is stable across the 1.x major. Decision: use `stdio_client` + `ClientSession.call_tool`; keep the client-side use behind a thin adapter in `mcp_pool.py` so a future HTTP-transport follow-up can add a parallel adapter without changing the handler. + +2. **Pool lifecycle: which lifecycle hook clears the pool?** — The sieve orchestrator has an existing `reset_caches()` method that clears `_shared_cache` and `_dependency_results` at the start of each `verify_batch`. `verify_batch` is the natural boundary for a single audit run. Decision: pool state lives as `_mcp_pool: dict[str, PooledSession]` on the orchestrator, cleared in `reset_caches()`. Teardown of active sessions happens in a `try/finally` around `verify_batch`'s loop so any exit path (success, failure, exception, interrupt) tears down before returning. Alternative considered: audit-level context-manager wrapping the whole run. Rejected because it requires threading a new lifetime object through every caller of `verify_batch` (agent graph, harness driver, MCP tool wrapper), whereas the orchestrator-owned pool is the shortest correct path. + +3. **What are the exact env-safe-set semantics?** — Spec FR-005 names PATH, HOME, LANG, LC_*, SSL_CERT_FILE, XDG_*. Research resolves the `LC_*` and `XDG_*` glob expansion at spawn time: read `os.environ` once, filter by predicate `k.startswith("LC_") or k.startswith("XDG_") or k in {"PATH", "HOME", "LANG", "SSL_CERT_FILE"}`, then apply the TOML `env` block's `$VAR` substitutions from the operator's shell (looking up the substituted vars in `os.environ` at substitution time; empty string if unset, matching how `exec` handler substitutes `$OWNER`). Decision documented explicitly so the sandbox follow-up (issue #375) can extend the predicate without ambiguity. + +4. **Sigstore verification against a `trusted_publisher` value: what shape?** — Darnit already has sigstore machinery for plugin-wheel verification (`.baseline.toml [plugins].trusted_publishers = [...]`). The mcp verification path can reuse the same underlying `sigstore.verify.Verifier.production()` + `GitHubWorkflowRepository` policy composition. Research: for a locally-installed binary, the darnit-known reference is the binary's bundled `.sigstore` sidecar (produced by GoReleaser's Sigstore step or similar). Decision: `mcp_trust.verify(binary_path, trusted_publisher)` looks for `.sigstore` or `.sigstore.json` sidecar next to the binary; if absent, verification fails (ERROR, per FR-007). Alternative considered: fetching an attestation from the transparency log using the binary's SHA-256 as the reference. Rejected for v0 because it adds a network round-trip that Constitution II says can't be silently required at audit time. + +5. **How does the mock MCP server work in the integration test?** — The `mcp` package exposes server primitives that let us write a mock in <100 lines. The mock counts spawn events (by writing to a shared file the test can inspect), exposes a small tool surface (`echo`, `get_score`, `raise_error`, `sleep_forever`), and is launched via `stdio_client(StdioServerParameters(command=["python", "-m", "tests.darnit.sieve.fixtures.mock_mcp_server"]))`. Decision: mock server module is at `tests/darnit/sieve/fixtures/mock_mcp_server/__init__.py` with a `__main__.py` for `-m` invocation. The counter file path is passed via TOML `env` block so different test cases can isolate their own counts. + +**Output**: `research.md` documenting each decision with rationale and rejected alternatives. + +## Phase 1: Design & Contracts + +**Prerequisites**: `research.md` complete. + +### Data Model (`data-model.md`) + +New schema types and their relationships: + +- **`McpServerConfig`** (Pydantic model in `framework_schema.py`): fields `command: list[str]` (required); `env: dict[str, str]` (default empty); `trusted_publisher: str | None` (default None); `optional: bool` (default True); `install_hint: str` (default empty). Validation rules: `command` must be non-empty; `trusted_publisher`, when set, must look like a URL or GitHub identity string. +- **`FrameworkConfig.mcp_servers: dict[str, McpServerConfig]`** (default empty). Merged from `.baseline.toml` over framework TOML via the existing config-merge path (spec FR-016). +- **`PooledSession`** (runtime-only dataclass in `mcp_pool.py`): fields `server_name: str`; `config: McpServerConfig`; `session: mcp.ClientSession`; `trust_label: Literal["sigstore-verified", "operator-trusted-path"]`; `spawn_ts: datetime`; `broken: bool` (default False). Lifecycle: `spawn()` → `use()` → `teardown()` or `invalidate()`. Distinguishable by `(audit_id, server_name)` — new audit gets fresh sessions. +- **`McpInvocationRecord`** (dict shape in evidence, mirrored in the reader-contract docs): `server`, `tool`, `args_after_substitution`, `raw_response` (or `error`), `trust_label`, `elapsed_ms`. +- **`HandlerContext.mcp_pool: McpPool | None = None`** (added field on the existing `HandlerContext` dataclass in `sieve/handler_registry.py`). The orchestrator assigns this to `self._mcp_pool` when constructing the context for an invocation whose `invocation.handler == "mcp"`; None for every other handler kind. The mcp handler reads `context.mcp_pool` to obtain the pool; a `None` value indicates a plumbing bug and resolves the pass ERROR (not a user-facing failure). + +State transitions for `PooledSession`: + +``` + spawn() first .use() terminated +[ ---- ] ------> [ FRESH ] ------> [ USED ] ------> [ TEARDOWN ] + | | + v v + invalidate() invalidate() + | | + v v + [ BROKEN ] [ BROKEN ] +``` + +`FRESH → BROKEN` (handshake failure) triggers exactly one respawn attempt on next reference; `USED → BROKEN` (crash mid-audit) also triggers one respawn. Two consecutive broken states → all subsequent references for that server produce INCONCLUSIVE (or FAIL if `optional = false`) without further respawn attempts, per FR-012. + +### Contracts (`contracts/mcp-handler-contract.md`) + +The public control-author API. Contents: + +- **TOML pass surface**: exact field list for `handler = "mcp"` (`server`, `tool`, `args`, `expr`, `timeout`, `authority`). Table of accepted CEL context vars (`result.*`, `$OWNER`/`$REPO`/`$BRANCH`/`$PATH`). +- **TOML allowlist surface**: exact field list for `[mcp_servers.]` (`command`, `env`, `trusted_publisher`, `optional`, `install_hint`). Merge precedence between `.baseline.toml` and framework TOML. +- **Evidence shape**: the `McpInvocationRecord` fields a downstream evidence-reader will see per invocation. +- **Progress-log shape**: the exact format string `[{n}/{m}] {control_id} dispatching_mcp {server}.{tool}` fired on the `darnit.harness` logger at INFO level. +- **Failure modes**: table with rows for each named failure (missing allowlist, missing binary + optional=true, missing binary + optional=false, Sigstore verify fail, handshake fail, tool error, timeout, crash mid-audit) and the corresponding control status. +- **Non-goals for v0**: repeats the spec's Assumptions §non-goals list at the contract level so a control author does not accidentally rely on a deferred feature (HTTP transport, cross-audit caching, install-helper subcommand, parallel calls). + +### Quickstart (`quickstart.md`) + +Two worked examples: + +1. **Control author perspective**: write a level-1 OSPS-style pass that consults the mock MCP server. Includes the exact TOML for `[mcp_servers.mock]` and the pass. Shows the expected evidence output and the progress-log line. +2. **Operator perspective**: register a real MCP server (using a real-world example like `uwu-tools/scorecard-mcp` when its interface is known, or a placeholder for now). Show adding a `trusted_publisher` line and what the verification failure message looks like. + +Also includes the "failure-mode diagnostics" section: how to interpret each failure-status message from the contract. + +### Agent Context Update + +Update the reference between `` and `` markers in `CLAUDE.md` to point at `specs/031-mcp-server-handler/plan.md`. + +## Post-Design Constitution Recheck + +The design phase artifacts do not introduce any new principle-touching decisions: + +- **I. Plugin Separation**: unchanged; all new modules are in `packages/darnit/`. +- **II. Conservative-by-Default**: reinforced by the state-transition model — `BROKEN` sessions produce INCONCLUSIVE/FAIL rather than silently retrying, and no code path produces PASS from a Sigstore verification failure. +- **III. TOML-First**: reinforced by the exact TOML surface documented in the reader contract; no Python-code escape hatch introduced. +- **IV. Never Guess User Values**: reinforced by the trust-label being separate from the authority — an "operator-trusted-path" invocation still produces `dispositive` results because the underlying observation is ground-truth (a tool observed the world); the label surfaces the trust level for auditors. +- **V. Sieve Pipeline Integrity**: reinforced by the handler returning a single `HandlerResult` and the pool being invisible to the disposition logic. + +**Post-design gate: PASS.** diff --git a/specs/031-mcp-server-handler/quickstart.md b/specs/031-mcp-server-handler/quickstart.md new file mode 100644 index 00000000..17cfbc64 --- /dev/null +++ b/specs/031-mcp-server-handler/quickstart.md @@ -0,0 +1,113 @@ +# Quickstart: `mcp` handler + +Two worked examples. First is the control-author perspective (writing a new control that consults an external MCP server). Second is the operator perspective (adding a new server to a fleet's `.baseline.toml`). + +## Example 1: Control author writes a pass that consults an external MCP server + +Assume there's an MCP server called `scorecard-mcp` that exposes a `get_repo_score(repo_url) -> {"score": float, ...}` tool. Your framework TOML declares the allowlist entry in one place and references it from any number of controls. + +### Framework TOML + +```toml +[mcp_servers.scorecard] +command = ["scorecard-mcp"] +env = { GITHUB_TOKEN = "$GITHUB_TOKEN" } # substituted from operator's shell at spawn time +trusted_publisher = "https://github.com/uwu-tools" # optional; when set, sidecar-verified +optional = true # binary absent -> INCONCLUSIVE, not FAIL +install_hint = "Install with: brew install scorecard-mcp" + +[controls."OSPS-VM-01.01"] +name = "OpenSSFScorecardOverallScore" +level = 1 +domain = "VM" +description = "Repository's OpenSSF Scorecard aggregate score is at least 7.0." + +[[controls."OSPS-VM-01.01".passes]] +handler = "mcp" +server = "scorecard" +tool = "get_repo_score" +args = { repo_url = "github.com/$OWNER/$REPO" } +expr = 'result.score >= 7.0' +timeout = 120 # override the 60s default; a real Scorecard scan may take longer +``` + +### What happens at audit time + +1. `darnit audit` (or the MCP tool wrapper, or the harness) starts a `verify_batch` run. +2. The first control that references `server = "scorecard"` triggers spawn: darnit resolves `scorecard-mcp` on PATH, verifies its Sigstore sidecar against `https://github.com/uwu-tools`, constructs the child env as (curated safe-set) + (`GITHUB_TOKEN` from operator's shell), spawns the subprocess, performs the MCP `initialize` handshake, and caches the session in the pool. +3. Darnit emits one INFO line on `darnit.harness`: + ``` + [3/62] OSPS-VM-01.01 dispatching_mcp scorecard.get_repo_score + ``` +4. `session.call_tool("get_repo_score", {"repo_url": "github.com/octo/hello"})` runs with 120s timeout. +5. Result comes back as `{"score": 8.5, "date": "2026-08-01"}`. +6. CEL evaluates `result.score >= 7.0` → `True` → pass resolves PASS. +7. Evidence record for this control's `mcp_calls` list captures the whole exchange with `trust_label = "sigstore-verified"` and `elapsed_ms = 812`. +8. Subsequent controls that reference `server = "scorecard"` reuse the same session — no re-spawn, no re-handshake. +9. At audit end, `verify_batch`'s `finally` block terminates the session before returning. No orphaned subprocess. + +### What if the operator hasn't installed `scorecard-mcp`? + +The absent-binary path produces one INFO line and one INCONCLUSIVE result per affected control: + +``` +[3/62] OSPS-VM-01.01 dispatching_mcp scorecard.get_repo_score +[3/62] OSPS-VM-01.01 resolved_inconclusive +``` + +The evidence record for the control names the missing binary and includes the `install_hint`: + +``` +MCP server binary not found: scorecard-mcp. Install with: brew install scorecard-mcp +``` + +## Example 2: Operator adds a new MCP-backed capability without editing plugin code + +Your fleet already runs `darnit audit` against your repos. You want to enrich the audit with a new external tool that exposes an MCP interface. No plugin code, no framework fork. + +Add one block to `.baseline.toml`: + +```toml +extends = "openssf-baseline" + +[mcp_servers.internal_policy] +command = ["/opt/company/policy-mcp"] +env = { COMPANY_POLICY_TOKEN = "$COMPANY_POLICY_TOKEN" } +optional = false # required for our fleet; missing binary is FAIL, not INCONCLUSIVE +install_hint = "See internal wiki page: https://wiki.company.io/policy-mcp" +``` + +Then any control in the extended framework, or in an override you write, can reference `server = "internal_policy"`. If you also want to add controls without forking the framework, you'll typically place them in a small extension package; that's an existing darnit-plugin-composition workflow (feature 013), not something this feature introduces. + +## Failure-mode diagnostics quick reference + +When a control's `mcp`-backed pass resolves to something unexpected, the evidence record tells you why. Reading the evidence for the affected control's `mcp_calls[0]`: + +| Symptom in evidence | What it means | Fix | +|---------------------|---------------|-----| +| `unknown MCP server: ` | Control's `server = ""` has no matching `[mcp_servers.]` block. | Add the block to `.baseline.toml` or the framework TOML. | +| `MCP server binary not found: . ` | Binary is not on PATH. | Install per the hint. | +| `Required MCP server binary not found` (FAIL) | Same as above, but `optional = false` promoted absence to FAIL. | Install, or set `optional = true` if this control class can tolerate absence. | +| `Sigstore verification failed for : ` | `trusted_publisher` was set and the binary's sidecar didn't verify. | Verify the binary's provenance manually. Do NOT remove `trusted_publisher` without understanding why verification failed. | +| `MCP handshake failed: ` | Server binary spawned but its initialization handshake failed or timed out. | Check server-side logs; the binary may be a wrong version or misconfigured. | +| `MCP tool call timed out after s` | Tool didn't respond within `timeout`. | Raise the per-pass `timeout`, or investigate why the server is slow. | +| `MCP tool error: ` | Server explicitly returned `isError=True`. | The message is the tool's own; read it. | +| `MCP tool response not JSON-parseable` | Tool returned non-text or non-JSON content. | Report to the MCP server's authors; darnit v0 requires JSON responses. | +| `MCP server session broken and respawn failed: ` | Session crashed mid-audit and could not respawn. | Same as handshake-failure diagnosis for the respawn attempt. | + +## Non-goals for v0 (what you can't do yet) + +The following are known-deferred: + +- **HTTP or SSE transport**: only stdio is supported. `command = ["python", "server.py"]` works; connecting to `https://mcp.company.io/tool` does not. +- **Parallel calls to the same server**: v0 runs controls serially. If your MCP server can handle concurrent calls, that capability is not yet used. +- **Cross-audit result caching**: every audit spawns fresh sessions. If your MCP tool is expensive and its output is stable across a day, you may want to add a separate cache layer above darnit for now. +- **`darnit install-mcp `**: no install-helper subcommand exists. Operators install MCP-server binaries the same way they install any other CLI tool. +- **Stronger sandboxing**: env-curation is the sandbox in v0. See issue #375 for the exploration on `nono.sh` / bubblewrap / nsjail / landlock integration for real subprocess isolation. + +## Where to look next + +- Contract: `contracts/mcp-handler-contract.md` — exhaustive field, log, and failure-mode table. +- Data model: `data-model.md` — the schema types this feature adds. +- Research decisions: `research.md` — why the pool lives on the orchestrator, why sidecar-based Sigstore verify, etc. +- Follow-up sandboxing: [darnitdevorg/darnit#375](https://github.com/darnitdevorg/darnit/issues/375). diff --git a/specs/031-mcp-server-handler/research.md b/specs/031-mcp-server-handler/research.md new file mode 100644 index 00000000..0a2ecfcc --- /dev/null +++ b/specs/031-mcp-server-handler/research.md @@ -0,0 +1,101 @@ +# Phase 0 Research: mcp handler for calling external MCP servers + +## Decision 1: MCP client-side API surface + +**Decision**: Use `mcp.client.stdio.stdio_client` + `mcp.ClientSession.call_tool` from the `mcp>=1.23,<2` package, wrapped behind a thin adapter in `packages/darnit/src/darnit/sieve/mcp_pool.py` so a future HTTP-transport follow-up can add a parallel adapter without touching the handler. + +**Rationale**: +- The `mcp` package is already declared as a darnit-core runtime dep (from feature 025's `FastMCP` server-side use). Its client-side entry points ship in the same wheel and require no extra installation. +- `mcp.client.stdio.stdio_client(StdioServerParameters(command=[...], args=[...], env={...}))` returns an async context manager yielding `(read_stream, write_stream)`. `mcp.ClientSession(read_stream, write_stream)` wraps those into a session with `initialize()`, `list_tools()`, `call_tool(name, arguments)`, and `close()`. This is the documented, stable surface across all 1.x releases. +- `session.call_tool(name, arguments)` returns a `CallToolResult` object with `.content: list[TextContent | ImageContent | ...]` and `.isError: bool`. For v0, we require and consume text content whose payload is JSON-parseable (this matches how every current MCP server we care about, including Scorecard MCP's design, returns results). +- Isolating the adapter behind `mcp_pool.py` means the handler code sees an object with a `call_tool(name, args, *, timeout: float) -> dict[str, Any]` signature. That single-method interface is a natural seam for the HTTP-transport follow-up: a new adapter satisfies the same interface, and the pool picks it based on a future `transport = "http"` field. + +**Alternatives considered**: +- **Direct JSON-RPC-over-pipes**: rejected. Reimplements what the SDK already does (framing, request/response correlation, initialization handshake) and reintroduces the maintenance burden the SDK exists to remove. +- **Subprocess + wait_for on `session.call_tool` in every handler call**: kept as the actual mechanism, but wrapped behind the pool's `call_tool` so timeout enforcement is centralized (matches spec FR-002 60s default). + +## Decision 2: Pool lifecycle boundary + +**Decision**: The pool is owned by `SieveOrchestrator` as `_mcp_pool: dict[str, PooledSession]`, cleared in the existing `reset_caches()` method, and torn down in a `try/finally` around `verify_batch()`'s per-control loop so any exit path (success, failure, exception, external interrupt) tears down before the method returns. + +**Rationale**: +- The orchestrator already owns two audit-run-scoped caches (`_shared_cache`, `_dependency_results`) with the exact same lifetime as an MCP server pool. Reusing the same lifecycle hook is the shortest correct path. +- `verify_batch()` is the single entry point for a full audit run — every consumer of the sieve (agent graph, `HarnessRun` driver, MCP tool wrapper `audit_openssf_baseline`, CLI `cmd_run`) calls into `verify_batch()`. Adding teardown there covers every caller. +- `try/finally` around the per-control loop guarantees teardown fires on the exception path too (spec FR-013, no orphaned subprocesses). +- Broken sessions are invalidated at their per-call catch site; the next reference to the same server sees `broken=True` and triggers exactly one respawn (spec FR-012). A double-broken session goes to a permanent "unusable this audit" state without further respawn. + +**Alternatives considered**: +- **Audit-level context-manager wrapping the whole run**: rejected. Requires threading a new lifetime object through every `verify_batch()` caller (four call sites). The orchestrator-owned pool needs zero external plumbing. +- **Per-control pool (spawn/teardown per call)**: rejected — spec FR-011 explicitly requires spawn-lazy-per-audit with reuse. + +## Decision 3: Environment safe-set implementation + +**Decision**: The child process env is constructed as: + +```python +_SAFE_KEY_PREDICATE = lambda k: ( + k in {"PATH", "HOME", "LANG", "SSL_CERT_FILE"} + or k.startswith("LC_") + or k.startswith("XDG_") +) + +child_env = {k: v for k, v in os.environ.items() if _SAFE_KEY_PREDICATE(k)} +for tk, tv in server_config.env.items(): + child_env[tk] = _substitute(tv) # $VAR from os.environ, empty string if unset +``` + +**Rationale**: +- The predicate is spelled out with the exact key set from spec FR-005 (2026-08-16 clarification). Any future expansion of the safe-set touches this one line. +- `LC_*` and `XDG_*` are prefix predicates because their key namespace is open-ended (`LC_ALL`, `LC_CTYPE`, `LC_MESSAGES`, ..., `XDG_CONFIG_HOME`, `XDG_DATA_HOME`, `XDG_RUNTIME_DIR`, ...). Matching by prefix is the natural shape. +- `$VAR` substitution inside the TOML `env` values mirrors how `exec` handler substitutes `$OWNER`/`$REPO`/`$BRANCH`/`$PATH` in `command`; consistent semantics reduce control-author surprise. +- An unset substituted var becomes empty string (not KeyError) to match `exec`'s behavior. The MCP server, not darnit, is responsible for treating "no token" as an error condition if it needs one. +- Cross-platform: on Windows, add `SYSTEMROOT` and `SYSTEMDRIVE` to the safe-set at the platform-guard level. Deferred to plan-implementation phase; called out here so it does not get overlooked. + +**Alternatives considered**: +- **Pass full parent env**: rejected. Reintroduces the leak the safe-set exists to prevent. +- **Empty env plus TOML block only**: rejected. Breaks most real MCP servers (which need PATH to find helper binaries, HOME for config). + +## Decision 4: Sigstore verification path + +**Decision**: `mcp_trust.verify(binary_path, trusted_publisher)` looks for a `.sigstore` or `.sigstore.json` sidecar next to the binary, calls `sigstore.verify.Verifier.production().verify_dsse(bundle, policy)` where `policy = AllOf([OIDCIssuer("https://token.actions.githubusercontent.com"), GitHubWorkflowRepository(trusted_publisher.removeprefix("https://github.com/"))])`. Returns `True` on success, `False` on any failure (missing sidecar, malformed bundle, verification error). Failure is loud in the caller: the caller resolves the affected control ERROR with the specific reason surfaced in evidence. + +**Rationale**: +- The `sigstore` package is already declared under `darnit-core[attestation]` and used by the existing plugin-wheel verification path in `packages/darnit/src/darnit/core/plugin.py`. Reusing the same `Verifier.production()` + policy composition means one trust-root, one code path. +- The sidecar approach matches how GoReleaser's Sigstore step produces attestations for binary releases (e.g., cosign attach + a `.sig` and `.sigstore` bundle deposited next to the binary). It's the most common on-disk shape for CLI tools that publish signed releases. +- Failure at any point (missing sidecar, HTTP unreachable while contacting Rekor, mismatched identity) returns `False`; the caller produces ERROR with the specific reason. Constitution II: never a silent PASS from a verification failure. +- Sidecar-based verification does not require a network round-trip *at audit time* if the sidecar contains the full offline-verifiable bundle. Recent sigstore Python versions default to bundle format which does support offline verification for most cases; the fallback that hits Rekor is deferred to `sigstore.verify` internals and only fires when the bundle is old-shape. + +**Alternatives considered**: +- **Fetch attestation from Rekor at audit time using the binary's SHA-256**: rejected for v0. Adds a network round-trip that Constitution II says can't silently be required for a compliance check. Attaching a sidecar to the binary makes the trust story auditable at install time, not audit time. +- **In-process verification with a fixed embedded trust root**: rejected. Duplicates the `sigstore` package's trust-root management, defeating the reason we depend on it. + +## Decision 5: Mock MCP server design for the integration test + +**Decision**: A small Python module at `tests/darnit/sieve/fixtures/mock_mcp_server/__init__.py` implementing a real MCP server using `mcp.server.Server`. Ships with a `__main__.py` for `python -m tests.darnit.sieve.fixtures.mock_mcp_server` invocation. Exposes four tools: + +- `echo(text: str) -> {"text": str}` — trivially exercises the round-trip. +- `get_score(repo_url: str) -> {"score": float}` — returns a canned score parameterizable by env var so different test cases hit different values. +- `raise_error(reason: str) -> raises` — deliberately returns `isError=True` so ERROR-path tests are exercised. +- `sleep_forever() -> hangs indefinitely` — for timeout-path tests. + +Spawn/teardown/tool-call events are logged to a file named by env var `DARNIT_MOCK_MCP_COUNTER_FILE`. The test fixture creates a fresh path per test; the mock appends one line per event; the test reads it back for assertions. + +**Rationale**: +- Using the real `mcp` package's server primitives (rather than hand-rolling JSON-RPC) means the mock exercises the exact wire protocol darnit's client-side code uses. If the SDK's framing changes, both sides update together. +- File-based event counting is the simplest way to observe a subprocess's lifecycle from a test that cannot use in-process introspection (the subprocess is a separate process). +- Four tools cover the four load-bearing behavior paths: success (echo, get_score), server-side error (raise_error), timeout (sleep_forever). No fifth tool needed; edge cases beyond these compose from these primitives. +- Placing the mock under `tests/darnit/sieve/fixtures/` keeps it clearly test-scoped and out of production packaging. + +**Alternatives considered**: +- **Third-party MCP mock library**: nothing usable exists at this feature's estimated implementation date; the `mcp` SDK's own server primitives are the right level. +- **Reuse the `scorecard-mcp` binary as the test fixture**: rejected — it's an external, still-stabilizing project. The spec explicitly defers the reference integration to a follow-up feature. +- **Multi-process mock**: rejected. One mock per test is a distinct file-counter path; simpler than a shared multi-instance mock. + +## Deferred / out of scope for this feature + +- HTTP or SSE MCP transports (deferred per spec Assumptions). +- Parallel invocation of the same server across parallel controls (deferred; v0 is serial). +- Cross-audit caching of tool results. +- `darnit install-mcp ` install-helper subcommand. +- Sandbox tooling beyond env-curation (tracked in issue #375). +- `exec`-handler env-curation retrofit — a separate feature could apply the same predicate to `exec` calls; not in scope here. diff --git a/specs/031-mcp-server-handler/spec.md b/specs/031-mcp-server-handler/spec.md new file mode 100644 index 00000000..70fd0c50 --- /dev/null +++ b/specs/031-mcp-server-handler/spec.md @@ -0,0 +1,145 @@ +# Feature Specification: mcp handler for calling external MCP servers as observation sources + +**Feature Branch**: `031-mcp-server-handler` + +**Created**: 2026-08-16 + +**Status**: Draft + +**Input**: User description: "Add mcp handler to darnit's sieve so TOML controls can call external MCP servers as observation sources, with spawn-lazy-per-audit lifecycle and allowlist-required plus optional Sigstore trust. Motivating reference server: uwu-tools/scorecard-mcp. v0 ships the machinery + mock-server integration test only; real Scorecard-backed controls land in a follow-up feature once uwu-tools/scorecard-mcp stabilizes its tool surface." + +## Clarifications + +### Session 2026-08-16 + +- Q: Child process environment inheritance → A: Curated safe-set (PATH, HOME, LANG, LC_*, SSL_CERT_FILE, XDG_*) plus TOML `env` block. Stronger sandbox tech (e.g., seccomp/landlock/nsjail-style isolation) is a follow-up feature, tracked separately. +- Q: Default per-call timeout → A: 60 seconds. Control authors doing longer-running work opt in with `timeout = ` on the pass. +- Q: Progress-line observability for MCP calls → A: One INFO line per call at dispatch, using a `dispatching_mcp` phase verb symmetric with feature 026's `dispatching_llm`. + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 - Control author writes an OSPS control that consults an external MCP server (Priority: P1) + +A control author writes a compliance control whose most authoritative signal is not observable from the local repo alone. They want to consult a tool that already exposes an MCP interface (OpenSSF Scorecard, a proprietary policy engine, an internal SBOM validator) and use its answer as evidence for the control's PASS/FAIL decision. They declare the tool call in the same flat TOML shape they use for `exec` and `api_call`. The audit runs, the tool answers, the control resolves. + +**Why this priority**: This is the whole reason the feature exists. Without a working control-author flow, the machinery has no consumer. Every other user story presupposes this one works. + +**Independent Test**: Author a small OpenSSF-style TOML control whose only pass is `handler = "mcp"` against a mock MCP server that returns `{"score": 8.0}`. Run the audit against a test repo. The control's status equals PASS; the evidence dict contains `result.score = 8.0`; the CEL `expr` was evaluated against the mock's response. + +**Acceptance Scenarios**: + +1. **Given** an `.baseline.toml` that declares `[mcp_servers.mock]` pointing at a mock MCP server binary on the operator's PATH, **When** an audit runs a control whose pass is `{ handler = "mcp", server = "mock", tool = "get_score", args = {...}, expr = 'result.score >= 7.0' }`, **Then** the control resolves against the mock's response with the CEL `expr` evaluated over `result.*` and the mock's raw response captured in the control's evidence. +2. **Given** the same audit invocation, **When** three separate controls in the same audit each call the `mock` server (possibly for different tools), **Then** the mock server is spawned exactly once for that audit run and reused across all three calls. +3. **Given** the same audit invocation, **When** the audit finishes (success, failure, or interrupted mid-run), **Then** the mock server subprocess is terminated cleanly before the darnit process exits; no orphaned processes remain. + +--- + +### User Story 2 - Operator adds a new MCP-backed capability to their audit (Priority: P2) + +A fleet operator wants to enrich their audits with a new external tool that exposes an MCP interface. They add one block to their `.baseline.toml` describing how to spawn the server. Controls in their framework TOML can now reference the new server by name. No plugin code, no framework fork, no additional darnit installation step beyond making the MCP-server binary available on PATH. + +**Why this priority**: Operator ergonomics is what makes the feature adoptable beyond the reference integration. If adding a new server requires a plugin author to write Python glue, the machinery loses most of its value. This story asserts the pure-configuration path works. + +**Independent Test**: On a fresh checkout with no plugin changes, add a `[mcp_servers.newthing]` block to `.baseline.toml`, place the corresponding binary on PATH, author a control that references it, run the audit, observe the control resolve. + +**Acceptance Scenarios**: + +1. **Given** an operator adds a `[mcp_servers.newthing]` block to `.baseline.toml` with a `command` field naming a binary they installed, **When** the audit starts, **Then** darnit reads that block from the effective framework configuration alongside all other framework TOML. +2. **Given** the operator's TOML block includes `env = { API_TOKEN = "$SOME_ENV_VAR" }`, **When** the server is spawned, **Then** the child process's environment contains (a) the curated safe-set `PATH`, `HOME`, `LANG`, `LC_*`, `SSL_CERT_FILE`, `XDG_*` inherited from darnit's own process, PLUS (b) `API_TOKEN` set to the value of `$SOME_ENV_VAR` from the operator's shell environment at audit-invocation time. NO other operator-shell env variables leak through by default (clarified 2026-08-16); the child does NOT receive credentials-style vars from the parent shell such as `AWS_*`, `GITHUB_TOKEN`, `ANTHROPIC_API_KEY`, or arbitrary user-set variables unless the TOML block names them explicitly. +3. **Given** no framework or `.baseline.toml` block declares an `[mcp_servers.]`, **When** a control's pass references `server = ""`, **Then** the control resolves as ERROR with an evidence field naming the missing server and pointing at the operator's configuration; the audit does not crash or silently swallow the control. + +--- + +### User Story 3 - Trust boundary is respected regardless of the operator's PATH state (Priority: P2) + +A malicious or accidentally-installed binary on the operator's PATH must not be able to run under darnit's identity as an MCP server. The `[mcp_servers.]` block is the allowlist; without an entry, darnit does not spawn. When the block includes a `trusted_publisher`, darnit verifies the binary's Sigstore attestation before spawning and refuses to trust the output on verification failure. + +**Why this priority**: Constitution II bites hard here. A silent PASS from an unverified server contradicts the entire compliance posture darnit exists to enforce. This story is P2 not because it is less important than US1 (it is not) but because the P1 machinery does not compile without an allowlist enforcement path, so this property is exercised even in the reference test path. + +**Independent Test**: Two shell tests. (a) Try to reference a server not declared in `[mcp_servers.*]` from a control; observe ERROR without a subprocess spawn. (b) Declare a server with `trusted_publisher = "https://github.com/example"` pointing at a binary whose Sigstore attestation does not match; observe ERROR with a verification-failed message; the binary's output MUST NOT contribute to any control's evidence. + +**Acceptance Scenarios**: + +1. **Given** an `.baseline.toml` with NO `[mcp_servers.rogue]` block, **When** a control's pass declares `server = "rogue"`, **Then** darnit records an ERROR result naming the missing allowlist entry; the audit does NOT spawn any subprocess. +2. **Given** `[mcp_servers.pinned].trusted_publisher = "https://github.com/example"` and a `pinned-server` binary on PATH whose Sigstore attestation cannot be verified against that publisher, **When** the audit reaches a control that uses `server = "pinned"`, **Then** darnit records an ERROR with the verification failure reason; the audit does NOT trust the binary's output as evidence. +3. **Given** the same block but the binary's Sigstore attestation DOES verify against `https://github.com/example`, **When** the audit runs, **Then** the server is spawned once for the audit run and its tool responses are used as evidence, and verification success is recorded in the run's evidence. +4. **Given** `[mcp_servers.]` with NO `trusted_publisher` field, **When** the audit runs, **Then** the server is spawned based on the allowlist entry alone, no Sigstore verification is attempted, and this is documented as "operator-trusted PATH" in the run's evidence. + +--- + +### User Story 4 - Missing binary is a knowable, non-fatal outcome by default (Priority: P3) + +A control author writes a control that consults an external MCP server. Some operators install that server; some do not. When the server is absent, the audit does not crash and does not silently PASS. The control's status is INCONCLUSIVE with a clear message telling the operator what to install. + +**Why this priority**: Real-world audits run across heterogeneous fleets. A single "install this binary" gate that stops every run is worse than a control that reports "I could not check this because you did not install the tool." P3 because US3 covers the security-critical absence case (rogue reference), while this story covers the ergonomics of the common-case absence (tool not installed). + +**Independent Test**: Configure a control that uses `[mcp_servers.absent]` where the binary is not on PATH. Run the audit. Observe the control resolves INCONCLUSIVE (not PASS, not ERROR) with a message identifying which binary was expected. Then set `[mcp_servers.absent].optional = false` in `.baseline.toml`, re-run, observe the same absence now produces FAIL. + +**Acceptance Scenarios**: + +1. **Given** `[mcp_servers.absent].command = ["absentbin"]` and no `absentbin` on PATH, **When** the audit reaches a control that uses `server = "absent"`, **Then** the control resolves INCONCLUSIVE with a message naming `absentbin` and hinting at how to install it (per the operator's TOML block or a default hint). +2. **Given** the same conditions plus `[mcp_servers.absent].optional = false` in the operator's `.baseline.toml`, **When** the audit runs, **Then** the same absence produces FAIL for every control that references the server. +3. **Given** the server is present and spawns cleanly, but a specific tool call raises an MCP-level error (unknown tool, malformed args, tool-side crash), **When** the audit reaches the affected control, **Then** that individual control resolves ERROR with the MCP error message in evidence; other controls that use the same server continue to function. + +### Edge Cases + +- **Server crashes mid-audit**: the pooled session becomes unusable. Subsequent controls that reference the same server MUST attempt exactly one respawn; if respawn fails, they resolve INCONCLUSIVE (or FAIL if `optional = false`). Do not retry indefinitely. +- **Server hangs (no response within the handler's timeout)**: the individual call times out, that control resolves ERROR, the session is discarded; the next control that references the same server triggers a fresh spawn. +- **Same server name declared in both `.baseline.toml` and a framework TOML**: `.baseline.toml` wins (operator override supersedes framework default), symmetric with how darnit already merges `.baseline.toml` over framework TOMLs. +- **Server spawns but MCP handshake fails or times out**: the session is not cached; the control resolves INCONCLUSIVE (or FAIL if `optional = false`) with a message identifying the handshake failure. Do NOT retry the same broken server for other controls in the same audit; log once and record consistently. +- **Argument substitution (`$OWNER`, `$REPO`, `$BRANCH`, `$PATH`) inside `args`**: MUST behave identically to how `exec` handler substitutes them today. No new substitution surface introduced by this feature. +- **A control that legitimately expects a numeric-zero or empty-list result**: `expr` distinguishes "server returned {}" from "server did not respond" so an empty response does not silently look like a PASS or FAIL depending on which one the operator wanted. Evidence records both the raw response and the CEL truth value. +- **Audit interrupted (Ctrl+C, timeout, harness cancel)**: pooled sessions MUST be terminated before darnit exits. No zombie subprocesses under any exit path. +- **HTTP or SSE transport for the MCP server**: out of scope for this feature; only stdio transport is supported. A control that references an HTTP-only server MUST produce ERROR with a "stdio-only" reason, not a silent hang. + +## Requirements *(mandatory)* + +### Functional Requirements + +- **FR-001**: A new sieve handler named `mcp` MUST be available for control authors to reference from framework TOML alongside `exec`, `api_call`, `file_exists`, and the other existing built-in handlers. +- **FR-002**: The `mcp` handler MUST accept, at minimum, `server` (allowlist name), `tool` (tool to invoke), and `args` (dict of arguments to pass to the tool) from its TOML config block. It MUST accept `expr` (CEL truth expression evaluated against the tool response) with the same semantics as `exec`. It MUST accept an optional `timeout` (per-call, in seconds) that defaults to **60 seconds** when unspecified on the pass (clarified 2026-08-16). A control author whose tool legitimately takes longer (repository clone, deep static analysis) opts in with an explicit larger `timeout` value. +- **FR-003**: The result of a tool invocation MUST be exposed to CEL as `result.*`, symmetric with how `exec` exposes `output.*`, so a control author familiar with `exec` needs no additional CEL knowledge. +- **FR-004**: The handler MUST perform `$OWNER`, `$REPO`, `$BRANCH`, and `$PATH` variable substitution inside `args` values before dispatching the tool call, matching the `exec` handler's substitution behavior. +- **FR-005**: An allowlist entry for a server MUST take the form `[mcp_servers.]` in `.baseline.toml` or a framework TOML, with at minimum a `command` field listing the executable and arguments used to spawn the server. Optional fields MUST include `env`, `trusted_publisher`, `optional`, and `install_hint`. When darnit spawns the server, the child process's environment MUST be constructed as the union of (a) a fixed curated safe-set inherited from darnit's own process (`PATH`, `HOME`, `LANG`, `LC_*`, `SSL_CERT_FILE`, `XDG_*`) and (b) the operator's `env` block. Any other variable from darnit's parent shell MUST NOT be visible to the child (clarified 2026-08-16). +- **FR-006**: A control's pass MUST resolve as ERROR with a message identifying the missing allowlist entry when the referenced server has no `[mcp_servers.]` block; the audit MUST NOT spawn any subprocess as a result of that reference. +- **FR-007**: When `[mcp_servers.].trusted_publisher` is set, darnit MUST verify the server binary's Sigstore attestation against that publisher before treating the binary's output as evidence; verification failure MUST resolve as ERROR (never PASS) and MUST NOT contribute the binary's output to any control's evidence. +- **FR-008**: When `[mcp_servers.].trusted_publisher` is unset, darnit MAY spawn the binary based on the allowlist entry alone; the resulting evidence MUST be labelled "operator-trusted PATH" or equivalent so downstream consumers (attestation, report) can distinguish operator-trust from cryptographic verification. +- **FR-009**: When the named binary is not found on the operator's PATH (or at the absolute path specified in `command`), the affected control MUST resolve INCONCLUSIVE by default, with a message naming the missing binary and including the operator's `install_hint` if set. The `[mcp_servers.].optional = false` field MUST promote absence to FAIL. +- **FR-010**: The `mcp` handler ships under RFC-0001 Stage 1 authority semantics as an observation-based handler; its default authority MUST be `dispositive`. TOML-level authority overrides on individual passes MUST behave identically to how they work for `exec` today (cannot loosen a handler default). +- **FR-011**: Server sessions MUST be pooled across the lifetime of a single audit run: the first control that references a given server triggers the spawn and MCP handshake; subsequent controls in the same audit that reference the same server reuse the same session; sessions are torn down when the audit ends. Session pooling MUST NOT cross audit boundaries. +- **FR-012**: A pooled session that becomes unusable mid-audit (server crashed, socket closed, handshake failure) MUST be invalidated so that the next control's reference to the same server triggers a single respawn attempt; darnit MUST NOT retry the spawn indefinitely on repeated failures within one audit run. +- **FR-013**: On any audit exit path (success, failure, exception, external interrupt), all pooled MCP server subprocesses MUST be terminated before the darnit process exits. No orphaned processes. +- **FR-014**: A tool invocation that returns an MCP-level error (unknown tool, malformed args, or server-side error response) MUST resolve the affected control as ERROR for that call only; other controls in the same audit that reference the same server or the same tool MUST NOT be affected. +- **FR-015**: Transport for v0 MUST be stdio only. A server declared with a transport specification other than stdio (if the schema grows to support one) MUST produce a clear ERROR ("transport not supported") rather than a silent hang. +- **FR-016**: When the same `[mcp_servers.]` block is declared in both `.baseline.toml` and a framework TOML, the `.baseline.toml` block MUST take precedence, symmetric with existing darnit config-merge semantics. +- **FR-017**: The feature MUST NOT introduce a new runtime dependency to any darnit product package. The MCP client library `mcp>=1.23,<2` is already a runtime dependency; no additional dependency is required. +- **FR-018**: Every tool invocation MUST record in the control's evidence: the server name, the tool name, the arguments the tool was called with (after `$` substitution), the raw JSON response (or the error), the trust label (Sigstore-verified vs operator-trusted PATH), and the elapsed time. +- **FR-019**: At the moment the sieve orchestrator is about to dispatch an MCP tool call (a pass whose `handler = "mcp"`), the orchestrator MUST emit one INFO log line on the `darnit.harness` logger using the `[N/M] dispatching_mcp .` shape (clarified 2026-08-16). Emission from the orchestrator (not the handler) matches feature 026's `dispatching_llm` pattern, where the driver iterating controls is the only entity that knows N and M. No corresponding "call returned" line is emitted; the terminal `resolved_*` line for the control conveys completion. + +### Key Entities *(include if feature involves data)* + +- **MCP server allowlist entry** (`[mcp_servers.]`): the operator's or framework author's declaration of a server darnit may spawn. Fields: `command` (required), `env`, `trusted_publisher`, `optional`, `install_hint`. Identity is the block name; uniqueness within the effective merged framework configuration. +- **Pooled session**: the runtime state representing one spawned MCP server subprocess plus its client-side session for the current audit. Ephemeral to the audit run. Distinguishable by (audit id, server name); a fresh audit starts fresh sessions. +- **Tool invocation record**: a per-call record entered into the affected control's evidence, capturing every input, output, and trust label. Persists into the audit's evidence log the same way `exec` output persists today. + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: A control author familiar with `exec` handler can author a working `mcp`-backed pass by writing at most one additional TOML block (the `[mcp_servers.]` allowlist) and using the same CEL and substitution syntax they already know for `exec`. +- **SC-002**: For a hypothetical audit that runs 20 controls all backed by the same MCP server, the server MUST be spawned exactly once and terminated exactly once. This measurable property (spawns == 1, terminations == 1) is verifiable from a mock server that counts its own lifecycle events. +- **SC-003**: An operator who removes a server from their PATH (or never installs it) sees INCONCLUSIVE results with actionable install-hint messages on affected controls, and no ERROR or crash of the audit as a whole, on the very first audit run after the removal. +- **SC-004**: A malicious binary swapped onto PATH under a server name whose allowlist entry declares `trusted_publisher` MUST NOT successfully contribute any evidence to the audit. Result on the affected controls MUST be ERROR with the Sigstore verification failure reason, verifiable by re-running the audit after the swap. +- **SC-005**: A control author who reads the feature's docs plus one worked example can add a new `[mcp_servers.]` block for an arbitrary MCP server they know about in under 15 minutes, without reading any Python source. + +## Assumptions + +- The MCP server producer's own lifecycle guarantees are outside darnit's scope. If the server itself makes non-idempotent state changes on a `tools/list` handshake, that is the server's design bug and darnit does not compensate for it. Darnit's own contract is: single spawn per audit, single handshake per spawn, teardown at audit end. +- Sigstore verification uses the same underlying machinery as darnit's existing plugin-wheel Sigstore path (`.baseline.toml [plugins] allow_unsigned` etc.). No new verification transport, no new trust root. If the operator's environment cannot reach Sigstore's transparency log, `trusted_publisher`-configured servers MUST resolve as ERROR (server unusable) rather than fall through to allowlist-only trust. +- The reference MCP server (`uwu-tools/scorecard-mcp`) is out of scope for this feature. This feature ships the mechanism plus a mock-server-backed integration test; the Scorecard-backed control TOML lands in a follow-up feature once the reference server's tool surface stabilizes. +- HTTP and SSE transports are out of scope. Only stdio is supported in v0. A follow-up feature can add HTTP once the operational story (URLs, TLS trust, cross-network dispatch policy) is scoped. +- Concurrent tool calls against the same server are NOT supported in v0. Controls are dispatched serially; if the future orchestrator adds parallel control execution, a follow-up feature addresses per-server concurrency (e.g., per-server locks or connection multiplexing). +- Cross-audit caching of tool results is NOT supported in v0. Every audit spawns fresh sessions. A follow-up feature can add a cache once the invalidation story is scoped (e.g., "invalidate when the target repo's HEAD SHA changes"). +- The feature does not introduce a `darnit install-mcp ` install-helper subcommand. Making the binary available is the operator's responsibility; the feature only provides `install_hint` messages to the operator when a control fails because the binary is absent. +- Existing scorecard normalizer (`packages/darnit/src/darnit/locate/normalizer.py`) is neither replaced nor extended by this feature. Whether a future Scorecard-backed control routes its raw JSON through that normalizer is decided in the follow-up Scorecard integration feature, not here. +- Stronger sandboxing (seccomp filters, landlock, nsjail-style process isolation, per-server cgroups) is deliberately out of scope for v0. The env-curation posture chosen in the 2026-08-16 clarification is a first line of defense; harder isolation belongs in a follow-up issue that can evaluate the whole sandbox-tool market against darnit's cross-platform requirements. diff --git a/specs/031-mcp-server-handler/tasks.md b/specs/031-mcp-server-handler/tasks.md new file mode 100644 index 00000000..d7118d6c --- /dev/null +++ b/specs/031-mcp-server-handler/tasks.md @@ -0,0 +1,256 @@ +--- +description: "Task list for feature 031-mcp-server-handler" +--- + +# Tasks: mcp handler for calling external MCP servers as observation sources + +**Input**: Design documents in `specs/031-mcp-server-handler/` + +**Prerequisites**: [plan.md](./plan.md), [spec.md](./spec.md), [research.md](./research.md), [data-model.md](./data-model.md), [contracts/mcp-handler-contract.md](./contracts/mcp-handler-contract.md), [quickstart.md](./quickstart.md). + +**Tests**: Included. Spec SC-002 requires mechanical verification of "spawns == 1, terminations == 1 across 20 controls" via a mock server that counts its own lifecycle events. Every user story's Independent Test requires a fixture-driven behavior test. Tests are load-bearing. + +**Organization**: One phase per user story after Setup + Foundational. Every user-story task carries a `[USn]` label. Cross-story files are only touched in Setup / Foundational / Polish. + +## Format: `[ID] [P?] [Story] Description` + +- **[P]**: Can run in parallel (different files, no dependencies on incomplete tasks). +- **[Story]**: `[US1]`, `[US2]`, `[US3]`, `[US4]` matching spec's user stories. +- File paths are absolute-from-repo-root. + +## Path Conventions + +Single workspace repo. All product code under `packages/darnit/src/darnit/sieve/` and `packages/darnit/src/darnit/config/`. Tests under `tests/darnit/sieve/`, `tests/darnit/config/`, and a new `tests/darnit/sieve/fixtures/mock_mcp_server/` package. + +## Phase 1: Setup (Shared Infrastructure) + +**Purpose**: Introduce the two new module files the rest of the feature builds on, plus the mock MCP server fixture used across US1/US2/US4 tests. + +- [X] T001 Create `packages/darnit/src/darnit/sieve/mcp_pool.py` with a module docstring naming its purpose (per-audit pool for MCP client sessions; spawn-lazy, teardown-in-finally), the constants `MCP_ENV_SAFE_KEYS = ("PATH", "HOME", "LANG", "SSL_CERT_FILE")`, `MCP_ENV_SAFE_PREFIXES = ("LC_", "XDG_")`, `MCP_PROGRESS_VERB = "dispatching_mcp"`, and stub declarations for `PooledSession` (dataclass) and `McpPool` (class) with method signatures only. No implementation body yet -- that lands in Phase 4. + +- [X] T002 Create `packages/darnit/src/darnit/sieve/mcp_trust.py` with a module docstring naming its purpose (Sigstore sidecar verification for `trusted_publisher`; deliberately isolated so issue #375 sandbox work can extend the pre-spawn hooks without touching the pool). Stub the single public function `verify(binary_path: Path, trusted_publisher: str) -> tuple[bool, str]` returning `(True, "")` unconditionally as a placeholder. Real implementation lands in Phase 5. + +- [X] T003 Create the mock MCP server package at `tests/darnit/sieve/fixtures/mock_mcp_server/__init__.py` and `tests/darnit/sieve/fixtures/mock_mcp_server/__main__.py`. Use `mcp.server.Server` primitives. Expose four tools: `echo(text: str) -> {"text": str}`, `get_score(repo_url: str) -> {"score": float}` (score parameterizable via env `DARNIT_MOCK_MCP_SCORE`, default 8.5), `raise_error(reason: str)` returning `isError=True`, `sleep_forever()` hanging indefinitely. Every spawn, teardown, and tool-call event MUST append one JSON line to the file named by env `DARNIT_MOCK_MCP_COUNTER_FILE` (if set). + +- [X] T004 Add a pytest fixture `mock_mcp_server_command` in `tests/darnit/sieve/conftest.py` (create the file if it does not exist) that returns a `list[str]` command suitable for a `[mcp_servers.mock]` `command` field: `[sys.executable, "-m", "tests.darnit.sieve.fixtures.mock_mcp_server"]`. Also add a fixture `mcp_counter_file(tmp_path)` returning a fresh path per test so counter files never collide. + +**Checkpoint**: Module skeletons and mock server exist. No behavior yet; nothing wires into the sieve. Phase 2 starts wiring. + +--- + +## Phase 2: Foundational (Blocking Prerequisites) + +**Purpose**: The two schema and orchestrator edits every user story depends on. Nothing US1-through-US4 can be implemented until these land, because both the reader and the pool need to know how to find their config. + +**CRITICAL**: No user story work begins until this phase completes. + +- [X] T005 Add `McpServerConfig(BaseModel)` to `packages/darnit/src/darnit/config/framework_schema.py`. Fields per [data-model.md](./data-model.md): `command: list[str]` (required, min length 1), `env: dict[str, str] = Field(default_factory=dict)`, `trusted_publisher: str | None = None`, `optional: bool = True`, `install_hint: str = ""`. Add `model_config = ConfigDict(extra="forbid")` so unknown fields (e.g., a hypothetical future `transport = "http"` key that v0 does not support) raise `ValidationError` at load time rather than silently accepting. This locks spec FR-015 ("transport specification other than stdio MUST produce a clear ERROR") at the schema layer. Validator on `command` MUST reject empty list with a clear error message. Validator on `trusted_publisher` (when set) MUST accept `https://github.com/` or `https://github.com//` shapes; other shapes are permitted but generate a Pydantic warning. Follow the file's existing docstring + field-comment conventions. + +- [X] T006 Add the field `mcp_servers: dict[str, McpServerConfig] = Field(default_factory=dict)` to the existing `FrameworkConfig` class in `packages/darnit/src/darnit/config/framework_schema.py`. Empty default preserves backward compatibility for every existing framework TOML (spec Backward Compatibility). Place the field next to `plugins` (line ~1421 pre-feature) so a maintainer diffing the schema sees the two extension surfaces together. + +- [X] T007 Update `packages/darnit/src/darnit/config/merger.py` to merge `mcp_servers` blocks with the same precedence rule as other framework-vs-baseline blocks: `.baseline.toml` block for a given `` fully replaces the framework TOML block for that `` (spec FR-016). Do NOT deep-merge fields within a block; a `.baseline.toml` entry is authoritative for that server name. + +- [X] T008 Add `_mcp_pool: McpPool | None = None` field on `SieveOrchestrator` in `packages/darnit/src/darnit/sieve/orchestrator.py`. Initialize as `None` in `__init__`; construct lazily inside the handler dispatch path when first needed (this keeps orchestrators that never call `mcp` handlers zero-cost). Extend `reset_caches()` to call `self._mcp_pool.teardown_all()` if the pool exists then set `self._mcp_pool = None` so a subsequent `verify_batch` starts fresh. Wrap the per-control loop inside `verify_batch()` in a `try / finally` that also calls `teardown_all()` and clears the field on exit (success, failure, or exception). + +- [X] T008a Wire the `dispatching_mcp` progress log AND the pool handoff into the orchestrator's dispatch site (`_dispatch_handler_invocations` in `packages/darnit/src/darnit/sieve/orchestrator.py`). Two edits: **(a)** add `mcp_pool: McpPool | None = None` field to `HandlerContext` in `packages/darnit/src/darnit/sieve/handler_registry.py`; **(b)** in the dispatch loop, when `invocation.handler == "mcp"`, lazily construct `self._mcp_pool` if `None`, assign it into the built `HandlerContext.mcp_pool`, and emit `f"[{idx}/{total}] {control_spec.control_id} dispatching_mcp {invocation.server}.{invocation.tool}"` on the `darnit.harness` logger at INFO level BEFORE calling the handler function. `(idx, total)` are threaded from `verify_batch`'s enumeration loop; add a small `progress: tuple[int, int]` parameter to `_dispatch_handler_invocations` if it does not already receive equivalent state. Matches spec FR-019 emission-from-orchestrator posture; matches feature 026's `dispatching_llm` pattern. + +**Checkpoint**: Framework config accepts `[mcp_servers.]` blocks; orchestrator owns the pool slot; teardown is guaranteed on every exit path; the `dispatching_mcp` INFO log fires at the correct point in the sieve dispatch loop. No handler exists yet. + +--- + +## Phase 3: User Story 1 - Control author writes an OSPS control that consults an external MCP server (Priority: P1) MVP + +**Goal**: A TOML control with `handler = "mcp"` produces a working PASS/FAIL/ERROR against the mock server, with CEL `expr` evaluated over `result.*` and the mock response captured in evidence. This is the whole reason the feature exists; the other three stories layer on top. + +**Independent Test**: Author a control whose only pass is `{ handler = "mcp", server = "mock", tool = "get_score", args = {}, expr = 'result.score >= 7.0' }` pointing at the T003/T004 mock. Run the audit. Control's status = PASS; evidence dict contains `result.score = 8.5`. + +### Implementation for User Story 1 + +- [X] T009 [P] [US1] Implement `PooledSession` dataclass in `packages/darnit/src/darnit/sieve/mcp_pool.py`: fields per [data-model.md](./data-model.md) (`server_name`, `config`, `session`, `trust_label`, `spawn_ts`, `broken`). Add methods `mark_broken(self) -> None` and `is_healthy(self) -> bool`. No I/O in the dataclass; it's just runtime state. + +- [X] T010 [P] [US1] Implement `McpPool.build_child_env(server_config: McpServerConfig) -> dict[str, str]` in `packages/darnit/src/darnit/sieve/mcp_pool.py`. Reads `os.environ`, filters by the predicate `k in MCP_ENV_SAFE_KEYS or any(k.startswith(p) for p in MCP_ENV_SAFE_PREFIXES)`, then overlays substituted values from `server_config.env`. `$VAR` substitution in values MUST look up `VAR` in `os.environ` and substitute empty string if unset (matching `exec` handler). On Windows, additionally allow-list `SYSTEMROOT` and `SYSTEMDRIVE` (guard with `sys.platform`). + +- [X] T011 [US1] Implement `McpPool.acquire(self, server_name: str, config: McpServerConfig) -> PooledSession` in `packages/darnit/src/darnit/sieve/mcp_pool.py`. First lookup in `self._sessions: dict[str, PooledSession]`. If missing, call `self._spawn(server_name, config)`. If present but `broken`, call `self._spawn` once more and cache. If present, `broken`, AND already-respawned-and-broken again, raise `McpServerUnusable` with the reason. Return the session. + +- [X] T012 [US1] Implement `McpPool._spawn(self, server_name: str, config: McpServerConfig) -> PooledSession` in `packages/darnit/src/darnit/sieve/mcp_pool.py`. Steps: (a) resolve `config.command[0]` on PATH via `shutil.which` (unless it's absolute); if missing, raise `McpServerBinaryMissing` with the resolved-name and `install_hint`. (b) If `config.trusted_publisher` is set, call `mcp_trust.verify(binary_path, config.trusted_publisher)`; on `False`, raise `McpServerVerificationFailed` with the reason. (c) Assemble the child env via `build_child_env`. (d) Construct `mcp.client.stdio.StdioServerParameters(command=config.command[0], args=config.command[1:], env=child_env)`. (e) Enter the `stdio_client` context and open a `ClientSession`, call `session.initialize()`. (f) Cache in `self._sessions[server_name]` and return the `PooledSession` with the correct `trust_label`. Any exception from steps (d)-(f) raises `McpServerHandshakeFailed` with the reason and does NOT cache. + +- [X] T013 [US1] Implement `McpPool.call_tool(self, server_name: str, config: McpServerConfig, tool: str, args: dict, timeout: float) -> dict` in `packages/darnit/src/darnit/sieve/mcp_pool.py`. Acquire session via `acquire`. Call `asyncio.run(asyncio.wait_for(session.call_tool(tool, args), timeout=timeout))`. On timeout, mark session broken and raise `McpToolTimeout`. On MCP `CallToolResult` with `isError=True`, raise `McpToolError` with the tool-supplied message; do NOT mark the session broken (this is a tool-level error, not a session-level one). On non-text content, raise `McpToolResponseNotJson`. On success, parse the text content as JSON and return the dict. + +- [X] T014 [US1] Implement `McpPool.teardown_all(self) -> None` in `packages/darnit/src/darnit/sieve/mcp_pool.py`. Iterates `self._sessions.values()`, closes each session with best-effort exception suppression (log a warning; do NOT re-raise), then clears the dict. This is what the orchestrator calls in `reset_caches()` and in `verify_batch`'s finally. + +- [X] T015 [US1] Define the exception hierarchy in `packages/darnit/src/darnit/sieve/mcp_pool.py`: `McpPoolError` (base), `McpServerBinaryMissing`, `McpServerVerificationFailed`, `McpServerHandshakeFailed`, `McpServerUnusable`, `McpToolTimeout`, `McpToolError`, `McpToolResponseNotJson`. Each carries the specific reason string the failure-mode table in `contracts/mcp-handler-contract.md` names. + +- [X] T016 [US1] Add `mcp_handler(config: dict[str, Any], context: HandlerContext) -> HandlerResult` in `packages/darnit/src/darnit/sieve/builtin_handlers.py`, alongside the other built-in handlers. Body: (a) read `server`, `tool`, `args`, `expr`, `timeout=MCP_DEFAULT_TIMEOUT_SECONDS` from `config`; validate `server` and `tool` are non-empty strings. (b) look up the `McpServerConfig` from the effective framework config; if missing, return `HandlerResult(status=ERROR, message="unknown MCP server: ", ...)`. (c) substitute `$OWNER`/`$REPO`/`$BRANCH`/`$PATH` in `args` values using `context`. (d) obtain the orchestrator's pool via `context.mcp_pool` (assigned by the orchestrator's dispatch site in T008a). If `context.mcp_pool is None`, return `HandlerResult(status=ERROR, message="mcp handler invoked without pool wiring (internal error)", ...)` -- this is a plumbing bug, not a user-facing failure. (e) call `pool.call_tool(...)`. Map exceptions to `HandlerResult` per the failure-mode table (ERROR/INCONCLUSIVE/FAIL). (f) evaluate `expr` (if set) against `{"result": }`; PASS/FAIL accordingly. (g) attach a `McpInvocationRecord`-shaped dict into `HandlerResult.evidence["mcp_calls"]` (list append). NOTE: the handler does NOT emit the `dispatching_mcp` INFO log -- that belongs to the orchestrator (T008a) so `[N/M]` is available. + +- [X] T017 [US1] Add `MCP_DEFAULT_TIMEOUT_SECONDS = 60` as a module-level constant in `packages/darnit/src/darnit/sieve/builtin_handlers.py` (spec FR-002, clarified 2026-08-16). Reference it as the default in the `mcp_handler` timeout read. + +- [X] T018 [US1] Register `mcp_handler` in `register_builtin_handlers()` at `packages/darnit/src/darnit/sieve/builtin_handlers.py`. Call signature: `registry.register("mcp", phase="deterministic", handler_fn=mcp_handler, default_authority="dispositive")`. This makes it available to control authors alongside `exec`, `api_call`, `file_exists`, etc. + +- [X] T019 [P] [US1] Write `tests/darnit/sieve/test_mcp_handler.py::test_pass_evaluates_expr_over_result` -- author a `ProjectConfig`-style TOML config with `[mcp_servers.mock] command = ` and a control whose pass uses `handler = "mcp"`, `tool = "get_score"`, `expr = 'result.score >= 7.0'`. Run the sieve orchestrator against it. Assert (a) the control resolves PASS; (b) `evidence["mcp_calls"][0]["raw_response"] == {"score": 8.5}` (matching the mock's default); (c) `evidence["mcp_calls"][0]["trust_label"] == "operator-trusted-path"`. + +- [X] T020 [P] [US1] Write `tests/darnit/sieve/test_mcp_handler.py::test_fail_when_expr_false` -- same fixture with `DARNIT_MOCK_MCP_SCORE=5.0` in the `env` block. Same `expr = 'result.score >= 7.0'`. Assert the control resolves FAIL (not ERROR); `evidence["mcp_calls"][0]["raw_response"]["score"] == 5.0`. + +- [X] T021 [P] [US1] Write `tests/darnit/sieve/test_mcp_handler.py::test_arg_substitution` -- pass `args = { repo_url = "github.com/$OWNER/$REPO" }` and set `context.owner = "octo"`, `context.repo = "hello"`. Use the `echo` tool. Assert `evidence["mcp_calls"][0]["args_after_substitution"]["repo_url"] == "github.com/octo/hello"` and the mock's echoed response reflects the substituted value. + +- [X] T022 [P] [US1] Write `tests/darnit/sieve/test_mcp_handler.py::test_progress_log_line_emitted` -- use `caplog` at INFO level on `darnit.harness`. Run the same config as T019. Assert exactly one log record matches the pattern `r"\[\d+/\d+\] \S+ dispatching_mcp mock\.get_score"`. + +**Checkpoint**: A control author can now write a `handler = "mcp"` pass and have it resolve against the mock server. US1's Independent Test passes. + +--- + +## Phase 4: User Story 2 - Operator adds a new MCP-backed capability without editing plugin code (Priority: P2) + +**Goal**: An operator's edit to `.baseline.toml` is the whole delta needed to enable a new MCP-backed server. No plugin code, no framework fork. + +**Independent Test**: On a fresh checkout with no plugin changes, add a `[mcp_servers.newthing]` block to `.baseline.toml`, place its binary on PATH, author a control that references it, run the audit, observe the control resolve. + +### Implementation for User Story 2 + +- [X] T023 [P] [US2] Write `tests/darnit/config/test_framework_schema.py::test_mcp_servers_block_parses` -- author a small framework TOML with an `[mcp_servers.example]` block including all optional fields. Assert `FrameworkConfig.mcp_servers["example"].command == [...]`, `env == {...}`, `trusted_publisher == "..."`, `optional == False`, `install_hint == "..."`. + +- [X] T024 [P] [US2] Write `tests/darnit/config/test_framework_schema.py::test_mcp_servers_command_required` -- author `[mcp_servers.example]` with only `env = {...}` (missing `command`). Assert `ValidationError` is raised at load time with a message naming `command`. + +- [X] T025 [P] [US2] Write `tests/darnit/config/test_framework_schema.py::test_mcp_servers_command_nonempty` -- author `[mcp_servers.example].command = []`. Assert `ValidationError` with a message noting the empty list. + +- [X] T026 [P] [US2] Write `tests/darnit/config/test_merger.py::test_mcp_servers_baseline_wins` -- author a framework TOML with `[mcp_servers.foo].command = ["fw-cmd"]` and a `.baseline.toml` with `[mcp_servers.foo].command = ["bl-cmd"]`. Merge. Assert the merged config's `mcp_servers["foo"].command == ["bl-cmd"]` (baseline replaces, not deep-merges). + +- [X] T027 [P] [US2] Write `tests/darnit/config/test_merger.py::test_mcp_servers_disjoint_names_coexist` -- framework declares `[mcp_servers.a]`, baseline declares `[mcp_servers.b]`. Assert both keys present in merged config. + +- [X] T028 [P] [US2] Write `tests/darnit/sieve/test_mcp_handler.py::test_env_curation_drops_credentials` -- monkeypatch `os.environ` with `AWS_SECRET_ACCESS_KEY="secret"`, `GITHUB_TOKEN="ghp_x"`, `HOME="/h"`, `PATH="/usr/bin"`, `XDG_CONFIG_HOME="/xdg"`, `LC_ALL="en_US.UTF-8"`. Call `McpPool.build_child_env(McpServerConfig(command=["true"], env={}))`. Assert the result contains `HOME`, `PATH`, `XDG_CONFIG_HOME`, `LC_ALL`; does NOT contain `AWS_SECRET_ACCESS_KEY` or `GITHUB_TOKEN`. + +- [X] T029 [P] [US2] Write `tests/darnit/sieve/test_mcp_handler.py::test_env_toml_block_substitutes_from_parent` -- monkeypatch `os.environ["GH_TOKEN"] = "ghp_realtoken"`. Config `env = {"GITHUB_TOKEN": "$GH_TOKEN", "STATIC_VAL": "literal"}`. Assert `build_child_env` output contains `GITHUB_TOKEN=ghp_realtoken` and `STATIC_VAL=literal`. + +- [X] T030 [P] [US2] Write `tests/darnit/sieve/test_mcp_handler.py::test_env_unset_var_substitutes_empty` -- monkeypatch `os.environ` to NOT contain `UNSET_VAR`. Config `env = {"X": "$UNSET_VAR"}`. Assert `build_child_env` output contains `X=""` (empty string), NOT that the key is missing. + +- [X] T031 [P] [US2] Write `tests/darnit/sieve/test_mcp_handler.py::test_unknown_server_produces_error` -- author a control with `handler = "mcp"`, `server = "rogue"`, no `[mcp_servers.rogue]` block anywhere. Run the sieve. Assert the control resolves ERROR with a message matching `unknown MCP server: rogue`; assert NO subprocess was spawned (verify by asserting the mock server's counter file was never written). + +- [X] T031a [P] [US2] Write `tests/darnit/config/test_framework_schema.py::test_mcp_servers_rejects_unknown_field` -- author a framework TOML with `[mcp_servers.foo] command = ["cmd"], transport = "http"`. Assert loading raises `ValidationError` with a message naming the `transport` key. Locks spec FR-015 ("transport specification other than stdio MUST produce a clear ERROR rather than silent hang") at the schema-load boundary. Pairs with the `extra="forbid"` addition on `McpServerConfig` in T005. + +**Checkpoint**: Operator-facing configuration surface is fully covered by schema, merge-precedence, env-curation, and unknown-field rejection tests. US2's Independent Test passes. + +--- + +## Phase 5: User Story 3 - Trust boundary is respected regardless of the operator's PATH state (Priority: P2) + +**Goal**: An allowlist entry is necessary; when `trusted_publisher` is set, Sigstore verification is additionally required; verification failure never contributes evidence. + +**Independent Test**: Two shell tests. (a) Reference a server without `[mcp_servers.*]` entry -> ERROR without spawn. (b) `trusted_publisher = "..."` with a binary whose sidecar mismatches -> ERROR with verification-failed message; no evidence contribution. + +### Implementation for User Story 3 + +- [X] T032 [US3] Implement `mcp_trust.verify(binary_path: Path, trusted_publisher: str) -> tuple[bool, str]` in `packages/darnit/src/darnit/sieve/mcp_trust.py`. Steps: (a) look for `binary_path.with_suffix(".sigstore")` OR `binary_path.with_suffix(".sigstore.json")` sidecar; if neither exists, return `(False, "no Sigstore sidecar found next to ")`. (b) Read the sidecar bytes; parse as `sigstore.models.Bundle` via `Bundle.from_json`. (c) Construct `policy = AllOf([OIDCIssuer("https://token.actions.githubusercontent.com"), GitHubWorkflowRepository()])`. (d) Call `Verifier.production().verify_dsse(bundle, policy)`; on success return `(True, "verified against ")`; on exception return `(False, "Sigstore verification failed: ")`. (e) Guard the whole function with `try/except ImportError` on sigstore; if unavailable, return `(False, "sigstore not installed - install darnit-core[attestation]")`. + +- [X] T033 [P] [US3] Write `tests/darnit/sieve/test_mcp_trust.py::test_no_sidecar_returns_false_with_reason` -- create a temp binary file, no sidecar. Call `verify(binary_path, "https://github.com/example/example")`. Assert `(False, msg)` where `msg` names the missing sidecar. + +- [X] T034 [P] [US3] Write `tests/darnit/sieve/test_mcp_trust.py::test_malformed_sidecar_returns_false` -- create a temp binary file and a `.sigstore` file containing `{"not": "a bundle"}`. Assert `(False, msg)` where `msg` includes the phrase `Sigstore verification failed` or `not a valid bundle` (mirror whatever the SDK raises). + +- [X] T035 [P] [US3] Write `tests/darnit/sieve/test_mcp_trust.py::test_sigstore_unavailable_returns_false` -- monkeypatch `sigstore` to `None` via `sys.modules`. Call `verify(...)`. Assert `(False, msg)` naming `darnit-core[attestation]`. + +- [X] T036 [US3] Write `tests/darnit/sieve/test_mcp_handler.py::test_verification_failure_produces_error_no_evidence` -- monkeypatch `mcp_trust.verify` to return `(False, "TEST verification failed")`. Config `[mcp_servers.pinned].command = [], trusted_publisher = "https://github.com/example"`. Run a control that uses `server = "pinned"`. Assert the control resolves ERROR with message containing `Sigstore verification failed`; assert `evidence["mcp_calls"]` list is EMPTY or contains only the failure record (no `raw_response`); assert the mock's counter file shows ZERO tool-call events (server was never spawned OR was terminated before a tool call). + +- [X] T037 [US3] Write `tests/darnit/sieve/test_mcp_handler.py::test_verification_success_trust_label` -- monkeypatch `mcp_trust.verify` to return `(True, "verified against https://github.com/example")`. Same fixture. Assert the control resolves PASS (using `expr = 'result.score >= 7.0'`); `evidence["mcp_calls"][0]["trust_label"] == "sigstore-verified"`. + +- [X] T038 [US3] Write `tests/darnit/sieve/test_mcp_handler.py::test_trusted_publisher_absent_label_is_operator_trusted_path` -- config omits `trusted_publisher`. Same PASS assertion as T037 but `trust_label == "operator-trusted-path"`. Assert `mcp_trust.verify` was NOT called (spy via monkeypatch or `unittest.mock`). + +**Checkpoint**: Trust boundary tests all pass. A malicious binary swapped onto PATH under a `trusted_publisher`-declared server name cannot contribute evidence. US3's Independent Test passes. + +--- + +## Phase 6: User Story 4 - Missing binary is a knowable, non-fatal outcome by default (Priority: P3) + +**Goal**: An absent binary defaults to INCONCLUSIVE (not ERROR, not silent PASS) with actionable install-hint text; `optional = false` promotes absence to FAIL. Session-crash/hang/tool-error paths all distinguish INCONCLUSIVE/ERROR/FAIL cleanly. + +**Independent Test**: `[mcp_servers.absent].command = ["absentbin"]` with no `absentbin` on PATH. Control resolves INCONCLUSIVE with `absentbin` and install-hint in the message. Flip `optional = false`; same absence produces FAIL. Then present-but-error paths (raise_error, sleep_forever) exercise their respective statuses. + +### Implementation for User Story 4 + +- [X] T039 [P] [US4] Write `tests/darnit/sieve/test_mcp_handler.py::test_binary_absent_optional_true_inconclusive` -- config `command = ["definitelynotarealthing_xyzq"]`. Run the control. Assert status INCONCLUSIVE; message contains `MCP server binary not found: definitelynotarealthing_xyzq` and any `install_hint`. Assert the mock's counter file is empty. + +- [X] T040 [P] [US4] Write `tests/darnit/sieve/test_mcp_handler.py::test_binary_absent_optional_false_fails` -- same config plus `optional = false`. Assert status FAIL with same message shape. + +- [X] T041 [P] [US4] Write `tests/darnit/sieve/test_mcp_handler.py::test_tool_timeout_produces_error_marks_broken` -- config points at mock; control uses `tool = "sleep_forever"` and `timeout = 1`. Assert control resolves ERROR with message containing `timed out after 1s`. Then run a SECOND control that uses `tool = "get_score"` on the same server. Assert the second control succeeds because the pool respawned after the broken session. Verify the mock's counter file shows exactly TWO spawn events (initial + one respawn); do NOT confuse this with SC-002's spawn-once property, which applies only to the no-crash path (see T044). + +- [X] T042 [P] [US4] Write `tests/darnit/sieve/test_mcp_handler.py::test_tool_side_error_produces_error_no_broken` -- control uses `tool = "raise_error", args = {"reason": "test"}`. Assert control resolves ERROR with message containing `MCP tool error` and the reason string. Then run a SECOND control against the same server using `tool = "echo"`. Assert the second succeeds AND the pool did NOT respawn (session was not marked broken because tool-side error is not a session-level failure). Verify by asserting the mock's counter file shows exactly ONE spawn event. + +- [X] T043 [P] [US4] Write `tests/darnit/sieve/test_mcp_handler.py::test_handshake_failure_produces_inconclusive_no_evidence` -- point the config at a `command = ["python", "-c", "import sys; sys.exit(0)"]` binary that exits immediately after spawn (no MCP handshake). Assert control resolves INCONCLUSIVE with message containing `MCP handshake failed`. Assert `evidence["mcp_calls"]` list is empty of successful invocations. + +- [X] T044 [P] [US4] Write `tests/darnit/sieve/test_mcp_pool.py::test_teardown_on_success_path` -- construct an orchestrator, run a `verify_batch` with two controls that both use the mock server, assert the mock's counter file shows exactly one spawn AND exactly one teardown event (SC-002 property). + +- [X] T045 [P] [US4] Write `tests/darnit/sieve/test_mcp_pool.py::test_teardown_on_exception_path` -- inject an exception at the second control's dispatch (monkeypatch one of its handler chains to raise `RuntimeError`). Assert the exception propagates out of `verify_batch` AND the mock's counter file shows a teardown event (finally-block guarantee). + +**Checkpoint**: All four spec user stories are covered by tests. Every failure mode from `contracts/mcp-handler-contract.md`'s table has a corresponding regression test. + +--- + +## Phase 7: Polish & Cross-Cutting Concerns + +**Purpose**: Full workspace verification, scope guard, lint clean, spec-sync validation, product-scope invariant. + +- [X] T046 Run the full workspace test sweep from repo root: `uv run pytest tests/ -q --deselect tests/darnit/context/test_dot_project_upstream.py::TestUpstreamSpecSync::test_upstream_spec_unchanged`. Confirm exit code 0. (The deselect matches the pattern used by feature 030 to avoid the CNCF-drift test.) + +- [X] T047 [P] Two sub-steps, both MUST pass. **(a) Structure Decision**: verify no file outside `packages/darnit/src/darnit/sieve/`, `packages/darnit/src/darnit/config/framework_schema.py`, `packages/darnit/src/darnit/config/merger.py`, and `tests/darnit/` was modified under `packages/*/src/`: `git diff --name-only main..HEAD | grep -E 'packages/(darnit-baseline|darnit-gittuf|darnit-reproducibility)/src/'` MUST produce zero lines. **(b) FR-017 no-new-runtime-dep guard**: any change to `pyproject.toml` at the repo root or any `packages/*/pyproject.toml` MUST NOT add a new entry to `[project.dependencies]` or `[project.optional-dependencies]` for a published product package. `git diff main..HEAD -- pyproject.toml packages/*/pyproject.toml` MUST either be empty OR be reviewed against FR-017 (`mcp>=1.23,<2` and `sigstore` were both already runtime deps pre-feature; no new package should appear). + +- [X] T048 [P] Run `uv run ruff check .` on repo root; MUST exit 0. Fix any lint issues in the files this feature touched; do NOT auto-format unrelated files. + +- [X] T049 [P] Run `uv run python scripts/validate_sync.py --verbose` if that script exists; MUST exit 0. This is darnit's spec-implementation sync check per constitution's Development Workflow section. It validates that the new `mcp` handler name in TOML schemas matches the code registration. + +- [X] T050 Confirm the module docstring on `packages/darnit/src/darnit/sieve/mcp_pool.py` and `mcp_trust.py` accurately describes the final implementation (specifically the exception hierarchy from T015 and the sidecar-lookup path from T032). Fix any docstring/code drift. Also confirm the `contracts/mcp-handler-contract.md` failure-mode table matches every exception the handler emits (cross-read against T016's exception-to-HandlerResult mapping). + +--- + +## Dependencies + +``` +Phase 1 (T001..T004) ──> Phase 2 (T005..T008) ──> Phase 3 (US1: T009..T022) + │ + ├──> Phase 4 (US2: T023..T031) [all [P] within phase] + │ + ├──> Phase 5 (US3: T032..T038) + │ + ├──> Phase 6 (US4: T039..T045) [all [P] within phase] + │ + └──> Phase 7 (Polish: T046..T050) +``` + +Phase 1 tasks T001, T002 touch different files, T003 and T004 touch different files (mock server package vs conftest.py) — all four are `[P]` in principle but marked sequential in the outline because reviewer readability improves when the four setup steps land in order. + +Within Phase 3 (US1), tasks T009 and T010 touch different regions of `mcp_pool.py` and can be authored `[P]`. Tasks T011–T015 all touch `mcp_pool.py` and MUST serialize on it. T016–T018 touch `builtin_handlers.py` and MUST serialize on it. T019–T022 write independent tests in the same test file — mark `[P]` because pytest itself handles concurrent additions cleanly at review time; commit ordering does not matter. + +Within Phase 5 (US3), T032 must land before T036–T038 (which monkeypatch the real function). T033–T035 test the real function directly and can be authored parallel to T036–T038 as long as T032 lands first. + +## Parallel execution examples + +After Phase 3 (US1) completes, US2/US3/US4 test files are disjoint from each other AND from `mcp_pool.py` — Phase 4, Phase 5, and Phase 6 can be authored concurrently: + +```sh +# Fire the US2 config tests, US3 trust tests, and US4 failure-mode tests concurrently. +# All touch distinct test files; no serialization needed. +uv run pytest tests/darnit/config/test_framework_schema.py tests/darnit/config/test_merger.py -q & # US2 configs +uv run pytest tests/darnit/sieve/test_mcp_trust.py -q & # US3 trust +uv run pytest tests/darnit/sieve/test_mcp_pool.py -q & # US4 lifecycle +wait +``` + +Within Phase 7: + +```sh +uv run pytest tests/ -q --deselect ... # T046 (long-running; start it first) +git diff --name-only main..HEAD | grep -E ... # T047 (fast, [P]) +uv run ruff check . # T048 (fast, [P]) +uv run python scripts/validate_sync.py --verbose # T049 (fast, [P]) +# T050 runs last, requires final state +``` + +## Implementation strategy + +MVP scope = Phase 1 + Phase 2 + Phase 3 (User Story 1 alone). Landing US1 gets the machinery working end-to-end against the mock server and delivers the P1 goal. Everything after that layers additional guarantees onto the same code path. + +Incremental delivery order: + +1. Land T001..T022 (Setup + Foundational + US1) as the MVP PR. At this point a control author can write `handler = "mcp"` and consult the mock. Failure modes are covered by unit tests for T016's exception mapping. +2. Land T023..T031 (US2 config + env curation) as a follow-up commit or separate PR. Independent of US1 code but layered onto its config path. +3. Land T032..T038 (US3 trust boundary) as a follow-up commit. Adds Sigstore verification behind a small new module. +4. Land T039..T045 (US4 failure-mode regressions) as a follow-up commit. Locks the failure-mode table against silent regression. +5. Land T046..T050 (Polish) as the last commit or squash into the MVP. + +All commits belong to the same PR against `main`. If piecewise review is preferred, reviewer order is (foundational + US1 code, US2 tests, US3 tests, US4 tests, polish) so each commit's contract-level effect is legible independently. diff --git a/tests/darnit/config/test_mcp_server_config.py b/tests/darnit/config/test_mcp_server_config.py new file mode 100644 index 00000000..9d65c3f8 --- /dev/null +++ b/tests/darnit/config/test_mcp_server_config.py @@ -0,0 +1,86 @@ +"""Framework/User schema coverage for the ``mcp_servers`` block. + +Locks the operator-facing shape of ``[mcp_servers.]`` at schema load +time. Merger-precedence tests live in ``test_merger.py``. +""" + +from __future__ import annotations + +import pytest +from pydantic import ValidationError + +from darnit.config.framework_schema import ( + FrameworkConfig, + FrameworkMetadata, + McpServerConfig, +) + +# --------------------------------------------------------------------------- +# T023: full-block parse +# --------------------------------------------------------------------------- + + +def test_mcp_servers_block_parses(): + config = McpServerConfig( + command=["scorecard-mcp", "--stdio"], + env={"GITHUB_TOKEN": "$GITHUB_TOKEN"}, + trusted_publisher="https://github.com/uwu-tools/scorecard-mcp", + optional=False, + install_hint="brew install scorecard-mcp", + ) + assert config.command == ["scorecard-mcp", "--stdio"] + assert config.env == {"GITHUB_TOKEN": "$GITHUB_TOKEN"} + assert config.trusted_publisher == "https://github.com/uwu-tools/scorecard-mcp" + assert config.optional is False + assert config.install_hint == "brew install scorecard-mcp" + + +def test_framework_config_carries_mcp_servers_block(): + fw = FrameworkConfig( + metadata=FrameworkMetadata( + name="test", + display_name="Test", + version="0.0.1", + spec_version="v0", + ), + mcp_servers={ + "scorecard": McpServerConfig(command=["scorecard-mcp"]), + }, + ) + assert "scorecard" in fw.mcp_servers + assert fw.mcp_servers["scorecard"].command == ["scorecard-mcp"] + + +# --------------------------------------------------------------------------- +# T024: missing `command` field is a validation error +# --------------------------------------------------------------------------- + + +def test_mcp_servers_command_required(): + with pytest.raises(ValidationError) as exc: + McpServerConfig(env={"X": "1"}) + assert "command" in str(exc.value) + + +# --------------------------------------------------------------------------- +# T025: empty `command` list is a validation error +# --------------------------------------------------------------------------- + + +def test_mcp_servers_command_nonempty(): + with pytest.raises(ValidationError) as exc: + McpServerConfig(command=[]) + # Message should note the empty/too-short list. + msg = str(exc.value).lower() + assert "at least 1" in msg or "empty" in msg or "too_short" in msg + + +# --------------------------------------------------------------------------- +# T031a: unknown field on `McpServerConfig` -> ValidationError (FR-015 lock) +# --------------------------------------------------------------------------- + + +def test_mcp_servers_rejects_unknown_field(): + with pytest.raises(ValidationError) as exc: + McpServerConfig(command=["x"], transport="http") # type: ignore[call-arg] + assert "transport" in str(exc.value) diff --git a/tests/darnit/config/test_merger_mcp_servers.py b/tests/darnit/config/test_merger_mcp_servers.py new file mode 100644 index 00000000..a47a57a1 --- /dev/null +++ b/tests/darnit/config/test_merger_mcp_servers.py @@ -0,0 +1,49 @@ +"""Merger precedence coverage for the ``mcp_servers`` block (spec FR-016).""" + +from __future__ import annotations + +from darnit.config.framework_schema import ( + FrameworkConfig, + FrameworkMetadata, + McpServerConfig, +) +from darnit.config.merger import merge_configs +from darnit.config.user_schema import UserConfig + + +def _framework(**servers: McpServerConfig) -> FrameworkConfig: + return FrameworkConfig( + metadata=FrameworkMetadata( + name="test", + display_name="Test", + version="0.0.1", + spec_version="v0", + ), + mcp_servers=dict(servers), + ) + + +# --------------------------------------------------------------------------- +# T026: baseline replaces per-name (no deep merge) +# --------------------------------------------------------------------------- + + +def test_mcp_servers_baseline_wins(): + fw = _framework(foo=McpServerConfig(command=["fw-cmd"])) + user = UserConfig(mcp_servers={"foo": McpServerConfig(command=["bl-cmd"])}) + eff = merge_configs(fw, user) + assert eff.mcp_servers["foo"].command == ["bl-cmd"] + + +# --------------------------------------------------------------------------- +# T027: disjoint names coexist +# --------------------------------------------------------------------------- + + +def test_mcp_servers_disjoint_names_coexist(): + fw = _framework(a=McpServerConfig(command=["fw-a"])) + user = UserConfig(mcp_servers={"b": McpServerConfig(command=["bl-b"])}) + eff = merge_configs(fw, user) + assert set(eff.mcp_servers) == {"a", "b"} + assert eff.mcp_servers["a"].command == ["fw-a"] + assert eff.mcp_servers["b"].command == ["bl-b"] diff --git a/tests/darnit/sieve/conftest.py b/tests/darnit/sieve/conftest.py new file mode 100644 index 00000000..fd3b58bc --- /dev/null +++ b/tests/darnit/sieve/conftest.py @@ -0,0 +1,29 @@ +"""Shared fixtures for sieve tests. + +The mcp-handler integration tests spawn an in-repo mock MCP server via the +Python interpreter running the test. Fixtures here isolate each test's +counter file so mock lifecycle events do not collide across tests. +""" + +from __future__ import annotations + +import sys +from pathlib import Path + +import pytest + + +@pytest.fixture() +def mock_mcp_server_command() -> list[str]: + """Return the command to launch the mock MCP server as a stdio subprocess. + + Used as the ``command`` field on a ``[mcp_servers.mock]`` block in test + framework configs. + """ + return [sys.executable, "-m", "tests.darnit.sieve.fixtures.mock_mcp_server"] + + +@pytest.fixture() +def mcp_counter_file(tmp_path: Path) -> Path: + """Return a fresh counter-file path unique to this test.""" + return tmp_path / "mcp_counter.jsonl" diff --git a/tests/darnit/sieve/fixtures/mock_mcp_server/__init__.py b/tests/darnit/sieve/fixtures/mock_mcp_server/__init__.py new file mode 100644 index 00000000..d3ee2dc7 --- /dev/null +++ b/tests/darnit/sieve/fixtures/mock_mcp_server/__init__.py @@ -0,0 +1,27 @@ +"""In-repo mock MCP server used by the mcp-handler integration tests. + +Exposes four deterministic tools: + +* ``echo(text)`` -- returns ``{"text": text}`` +* ``get_score(repo_url)`` -- returns ``{"score": float}`` where the score + is parameterisable via env ``DARNIT_MOCK_MCP_SCORE`` (default ``8.5``) +* ``raise_error(reason)`` -- raises ``ToolError`` so the MCP layer sends + the response with ``isError=True`` +* ``sleep_forever()`` -- suspends indefinitely (used to exercise the + handler's per-call timeout) + +The server appends a single JSON line to the file named by env +``DARNIT_MOCK_MCP_COUNTER_FILE`` on every lifecycle event (``spawn``, +``teardown``, ``tool_call``). Tests inspect that file to make mechanical +assertions about the pool's spawn/teardown semantics (spec SC-002). +""" + +from __future__ import annotations + +__all__ = ["main"] + + +def main() -> None: + from .__main__ import main as _run + + _run() diff --git a/tests/darnit/sieve/fixtures/mock_mcp_server/__main__.py b/tests/darnit/sieve/fixtures/mock_mcp_server/__main__.py new file mode 100644 index 00000000..56d79528 --- /dev/null +++ b/tests/darnit/sieve/fixtures/mock_mcp_server/__main__.py @@ -0,0 +1,80 @@ +"""Runnable mock MCP server entrypoint. + +Invoked by the pool as ``[sys.executable, "-m", "tests.darnit.sieve.fixtures.mock_mcp_server"]``. +""" + +from __future__ import annotations + +import asyncio +import json +import os +import sys +import time +from pathlib import Path + + +def _log_event(kind: str, payload: dict[str, object] | None = None) -> None: + """Append a single JSON line to the counter file, if configured.""" + path = os.environ.get("DARNIT_MOCK_MCP_COUNTER_FILE") + if not path: + return + record: dict[str, object] = {"kind": kind, "ts": time.time(), "pid": os.getpid()} + if payload: + record.update(payload) + try: + Path(path).parent.mkdir(parents=True, exist_ok=True) + with open(path, "a", encoding="utf-8") as fh: + fh.write(json.dumps(record) + "\n") + except Exception: # noqa: BLE001 -- counter file MUST NOT crash the mock + pass + + +def _make_server(): # -> FastMCP + from mcp.server.fastmcp import FastMCP + from mcp.server.fastmcp.exceptions import ToolError + + srv = FastMCP("darnit-mock-mcp") + + @srv.tool(description="Echo text back to the caller.") + def echo(text: str) -> dict[str, str]: + _log_event("tool_call", {"tool": "echo", "args": {"text": text}}) + return {"text": text} + + @srv.tool(description="Return a deterministic score for a repo URL.") + def get_score(repo_url: str = "") -> dict: + _log_event("tool_call", {"tool": "get_score", "args": {"repo_url": repo_url}}) + raw = os.environ.get("DARNIT_MOCK_MCP_SCORE", "8.5") + try: + score = float(raw) + except ValueError: + score = 8.5 + return {"score": score, "repo_url": repo_url} + + @srv.tool(description="Deliberately return an isError=True MCP response.") + def raise_error(reason: str = "test") -> dict[str, str]: + _log_event("tool_call", {"tool": "raise_error", "args": {"reason": reason}}) + raise ToolError(f"raise_error: reason {reason!r}") + + @srv.tool(description="Sleep forever; used to exercise per-call timeout.") + async def sleep_forever() -> dict[str, str]: + _log_event("tool_call", {"tool": "sleep_forever"}) + await asyncio.Event().wait() + return {"unreachable": "true"} + + return srv + + +def main() -> None: + _log_event("spawn") + try: + srv = _make_server() + asyncio.run(srv.run_stdio_async()) + finally: + _log_event("teardown") + + +if __name__ == "__main__": + try: + main() + except KeyboardInterrupt: + sys.exit(0) diff --git a/tests/darnit/sieve/test_mcp_handler.py b/tests/darnit/sieve/test_mcp_handler.py new file mode 100644 index 00000000..098d7187 --- /dev/null +++ b/tests/darnit/sieve/test_mcp_handler.py @@ -0,0 +1,652 @@ +"""Tests for the built-in ``mcp`` sieve handler. + +These tests spawn the in-repo mock MCP server (see +``tests/darnit/sieve/fixtures/mock_mcp_server``) as a real subprocess and +drive the handler through :class:`McpPool`. Every test constructs its own +pool and tears it down in a finalizer so the daemon-thread loop and stdio +subprocess do not leak between tests. +""" + +from __future__ import annotations + +import json +import os +import re +from pathlib import Path +from typing import Any + +import pytest + +from darnit.config.framework_schema import McpServerConfig +from darnit.core.models import ExecutionContext +from darnit.sieve.builtin_handlers import mcp_handler +from darnit.sieve.handler_registry import HandlerContext, HandlerResultStatus +from darnit.sieve.mcp_pool import McpPool + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _make_pool_and_ctx( + tmp_path: Path, + mock_mcp_server_command: list[str], + mcp_counter_file: Path, + *, + server_name: str = "mock", + tool_env_score: str | None = None, + trusted_publisher: str | None = None, + trust_verifier: Any = None, + optional: bool = True, + extra_env: dict[str, str] | None = None, +) -> tuple[McpPool, HandlerContext]: + """Return ``(pool, handler_ctx)`` wired to the mock server. + + The mock's counter file path and score are propagated via TOML + ``env`` substitution so the mock's parent env stays curated. + """ + parent_env: dict[str, str] = { + "DARNIT_MOCK_MCP_COUNTER_FILE_SRC": str(mcp_counter_file), + } + if tool_env_score is not None: + parent_env["DARNIT_MOCK_MCP_SCORE_SRC"] = tool_env_score + for key, value in parent_env.items(): + os.environ[key] = value + + server_env: dict[str, str] = { + "DARNIT_MOCK_MCP_COUNTER_FILE": "$DARNIT_MOCK_MCP_COUNTER_FILE_SRC", + } + if tool_env_score is not None: + server_env["DARNIT_MOCK_MCP_SCORE"] = "$DARNIT_MOCK_MCP_SCORE_SRC" + if extra_env: + server_env.update(extra_env) + + config = McpServerConfig( + command=mock_mcp_server_command, + env=server_env, + trusted_publisher=trusted_publisher, + optional=optional, + install_hint="Install the mock (test-only)", + ) + + pool = McpPool( + servers={server_name: config}, + trust_verifier=trust_verifier or (lambda p, tp: (True, "test")), + ) + execution_context = ExecutionContext( + owner="octo", + repo="hello", + local_path=str(tmp_path), + mcp_servers={server_name: config}, + ) + handler_ctx = HandlerContext( + local_path=str(tmp_path), + owner="octo", + repo="hello", + default_branch="main", + control_id="TEST-MCP-01", + execution_context=execution_context, + mcp_pool=pool, + ) + return pool, handler_ctx + + +@pytest.fixture() +def mock_pool_ctx(tmp_path, mock_mcp_server_command, mcp_counter_file, request): + """Yield a pool+ctx pair, guaranteeing teardown after each test.""" + pool, ctx = _make_pool_and_ctx( + tmp_path, mock_mcp_server_command, mcp_counter_file + ) + + def _finalize() -> None: + pool.teardown_all() + + request.addfinalizer(_finalize) + return pool, ctx, mcp_counter_file + + +def _read_counter(counter_file: Path) -> list[dict[str, Any]]: + if not counter_file.exists(): + return [] + return [json.loads(line) for line in counter_file.read_text().splitlines() if line] + + +# --------------------------------------------------------------------------- +# T019: pass evaluates expr over result +# --------------------------------------------------------------------------- + + +def test_pass_evaluates_expr_over_result(mock_pool_ctx): + _, ctx, _ = mock_pool_ctx + result = mcp_handler( + { + "server": "mock", + "tool": "get_score", + "args": {"repo_url": "github.com/octo/hello"}, + "expr": "result.score >= 7.0", + }, + ctx, + ) + assert result.status == HandlerResultStatus.PASS, ( + f"handler said {result.status}: {result.message} " + f"evidence={result.evidence}" + ) + call = result.evidence["mcp_calls"][0] + assert call["raw_response"]["score"] == 8.5 + assert call["trust_label"] == "operator-trusted-path" + + +# --------------------------------------------------------------------------- +# T020: fail when expr false +# --------------------------------------------------------------------------- + + +def test_fail_when_expr_false(tmp_path, mock_mcp_server_command, mcp_counter_file, request): + pool, ctx = _make_pool_and_ctx( + tmp_path, + mock_mcp_server_command, + mcp_counter_file, + tool_env_score="5.0", + ) + request.addfinalizer(pool.teardown_all) + + result = mcp_handler( + { + "server": "mock", + "tool": "get_score", + "args": {}, + "expr": "result.score >= 7.0", + }, + ctx, + ) + assert result.status == HandlerResultStatus.FAIL + assert result.evidence["mcp_calls"][0]["raw_response"]["score"] == 5.0 + + +# --------------------------------------------------------------------------- +# T021: arg substitution against context +# --------------------------------------------------------------------------- + + +def test_arg_substitution(mock_pool_ctx): + _, ctx, _ = mock_pool_ctx + result = mcp_handler( + { + "server": "mock", + "tool": "echo", + "args": {"text": "repo=$OWNER/$REPO branch=$BRANCH"}, + }, + ctx, + ) + assert result.status == HandlerResultStatus.PASS + call = result.evidence["mcp_calls"][0] + assert call["args_after_substitution"]["text"] == "repo=octo/hello branch=main" + # Mock echoed the substituted value + assert call["raw_response"]["text"] == "repo=octo/hello branch=main" + + +# --------------------------------------------------------------------------- +# T022: progress log line emitted by the orchestrator +# --------------------------------------------------------------------------- + + +# --------------------------------------------------------------------------- +# T028: env curation drops credentials +# --------------------------------------------------------------------------- + + +def test_env_curation_drops_credentials(monkeypatch): + for k, v in { + "AWS_SECRET_ACCESS_KEY": "shhh", + "GITHUB_TOKEN": "ghp_x", + "HOME": "/h", + "PATH": "/usr/bin", + "XDG_CONFIG_HOME": "/xdg", + "LC_ALL": "en_US.UTF-8", + }.items(): + monkeypatch.setenv(k, v) + # Clear anything else that might already be set from the parent shell. + for leaky in ("AWS_ACCESS_KEY_ID",): + monkeypatch.delenv(leaky, raising=False) + + env = McpPool.build_child_env(McpServerConfig(command=["true"], env={})) + assert env["HOME"] == "/h" + assert env["PATH"] == "/usr/bin" + assert env["XDG_CONFIG_HOME"] == "/xdg" + assert env["LC_ALL"] == "en_US.UTF-8" + assert "AWS_SECRET_ACCESS_KEY" not in env + assert "GITHUB_TOKEN" not in env + + +# --------------------------------------------------------------------------- +# T029: TOML env block substitutes from parent shell +# --------------------------------------------------------------------------- + + +def test_env_toml_block_substitutes_from_parent(monkeypatch): + monkeypatch.setenv("GH_TOKEN", "ghp_realtoken") + env = McpPool.build_child_env( + McpServerConfig( + command=["true"], + env={"GITHUB_TOKEN": "$GH_TOKEN", "STATIC_VAL": "literal"}, + ) + ) + assert env["GITHUB_TOKEN"] == "ghp_realtoken" + assert env["STATIC_VAL"] == "literal" + + +# --------------------------------------------------------------------------- +# T030: unset variable substitutes as empty string +# --------------------------------------------------------------------------- + + +def test_env_unset_var_substitutes_empty(monkeypatch): + monkeypatch.delenv("DARNIT_TEST_UNSET_VAR", raising=False) + env = McpPool.build_child_env( + McpServerConfig(command=["true"], env={"X": "$DARNIT_TEST_UNSET_VAR"}) + ) + assert env["X"] == "" + + +# --------------------------------------------------------------------------- +# T031: unknown server -> ERROR without any spawn +# --------------------------------------------------------------------------- + + +def test_unknown_server_produces_error(tmp_path, mcp_counter_file): + execution_context = ExecutionContext( + owner="octo", + repo="hello", + local_path=str(tmp_path), + mcp_servers={}, # no `rogue` registered + ) + pool = McpPool(servers={}) + ctx = HandlerContext( + local_path=str(tmp_path), + control_id="TEST-01", + execution_context=execution_context, + mcp_pool=pool, + ) + result = mcp_handler( + {"server": "rogue", "tool": "anything", "args": {}}, ctx + ) + assert result.status == HandlerResultStatus.ERROR + assert "unknown MCP server: rogue" in result.message + assert not mcp_counter_file.exists() or _read_counter(mcp_counter_file) == [] + + +# --------------------------------------------------------------------------- +# T036: trust verification failure -> ERROR + no evidence tool call +# --------------------------------------------------------------------------- + + +def test_verification_failure_produces_error_no_evidence( + tmp_path, mock_mcp_server_command, mcp_counter_file, request +): + def _fail_verify(_binary, _tp): + return False, "TEST verification failed" + + pool, ctx = _make_pool_and_ctx( + tmp_path, + mock_mcp_server_command, + mcp_counter_file, + server_name="pinned", + trusted_publisher="https://github.com/example/example", + trust_verifier=_fail_verify, + ) + request.addfinalizer(pool.teardown_all) + + result = mcp_handler( + { + "server": "pinned", + "tool": "get_score", + "args": {}, + "expr": "result.score >= 7.0", + }, + ctx, + ) + assert result.status == HandlerResultStatus.ERROR + assert "Sigstore verification failed" in result.message + # No successful tool call in evidence + call = result.evidence["mcp_calls"][0] + assert "raw_response" not in call + assert "error" in call + # Counter file records ZERO tool_call events + events = _read_counter(mcp_counter_file) + assert not any(e.get("kind") == "tool_call" for e in events) + + +# --------------------------------------------------------------------------- +# T037: trust verification success -> PASS + trust_label = sigstore-verified +# --------------------------------------------------------------------------- + + +def test_verification_success_trust_label( + tmp_path, mock_mcp_server_command, mcp_counter_file, request +): + def _ok_verify(_binary, _tp): + return True, "verified against https://github.com/example/example" + + pool, ctx = _make_pool_and_ctx( + tmp_path, + mock_mcp_server_command, + mcp_counter_file, + server_name="pinned", + trusted_publisher="https://github.com/example/example", + trust_verifier=_ok_verify, + ) + request.addfinalizer(pool.teardown_all) + + result = mcp_handler( + { + "server": "pinned", + "tool": "get_score", + "args": {}, + "expr": "result.score >= 7.0", + }, + ctx, + ) + assert result.status == HandlerResultStatus.PASS + assert result.evidence["mcp_calls"][0]["trust_label"] == "sigstore-verified" + + +# --------------------------------------------------------------------------- +# T038: trusted_publisher absent -> label = operator-trusted-path, verify NOT called +# --------------------------------------------------------------------------- + + +def test_trusted_publisher_absent_label_is_operator_trusted_path( + tmp_path, mock_mcp_server_command, mcp_counter_file, request +): + verify_called = False + + def _spy_verify(_binary, _tp): + nonlocal verify_called + verify_called = True + return True, "should not run" + + pool, ctx = _make_pool_and_ctx( + tmp_path, + mock_mcp_server_command, + mcp_counter_file, + trusted_publisher=None, # no verification + trust_verifier=_spy_verify, + ) + request.addfinalizer(pool.teardown_all) + + result = mcp_handler( + { + "server": "mock", + "tool": "get_score", + "args": {}, + "expr": "result.score >= 7.0", + }, + ctx, + ) + assert result.status == HandlerResultStatus.PASS + assert result.evidence["mcp_calls"][0]["trust_label"] == "operator-trusted-path" + assert verify_called is False + + +# --------------------------------------------------------------------------- +# T039: binary absent + optional=true -> INCONCLUSIVE with install hint +# --------------------------------------------------------------------------- + + +def test_binary_absent_optional_true_inconclusive(tmp_path, mcp_counter_file): + absent_cmd = ["definitelynotarealthing_xyzq_abc"] + server_config = McpServerConfig( + command=absent_cmd, + install_hint="Install with: brew install thing", + optional=True, + ) + pool = McpPool(servers={"missing": server_config}) + execution_context = ExecutionContext( + owner="octo", + repo="hello", + local_path=str(tmp_path), + mcp_servers={"missing": server_config}, + ) + ctx = HandlerContext( + local_path=str(tmp_path), + control_id="TEST-01", + execution_context=execution_context, + mcp_pool=pool, + ) + result = mcp_handler( + {"server": "missing", "tool": "echo", "args": {}}, ctx + ) + assert result.status == HandlerResultStatus.INCONCLUSIVE + assert "MCP server binary not found: definitelynotarealthing_xyzq_abc" in result.message + assert "Install with: brew install thing" in result.message + assert not mcp_counter_file.exists() or _read_counter(mcp_counter_file) == [] + + +# --------------------------------------------------------------------------- +# T040: binary absent + optional=false -> FAIL with same shape +# --------------------------------------------------------------------------- + + +def test_binary_absent_optional_false_fails(tmp_path, mcp_counter_file): + absent_cmd = ["definitelynotarealthing_xyzq_abc"] + server_config = McpServerConfig( + command=absent_cmd, + install_hint="Install with: brew install thing", + optional=False, + ) + pool = McpPool(servers={"missing": server_config}) + execution_context = ExecutionContext( + owner="octo", + repo="hello", + local_path=str(tmp_path), + mcp_servers={"missing": server_config}, + ) + ctx = HandlerContext( + local_path=str(tmp_path), + control_id="TEST-01", + execution_context=execution_context, + mcp_pool=pool, + ) + result = mcp_handler( + {"server": "missing", "tool": "echo", "args": {}}, ctx + ) + assert result.status == HandlerResultStatus.FAIL + assert "MCP server binary not found: definitelynotarealthing_xyzq_abc" in result.message + + +# --------------------------------------------------------------------------- +# T041: tool timeout -> ERROR + session broken + respawn on next call +# --------------------------------------------------------------------------- + + +def test_tool_timeout_produces_error_marks_broken( + tmp_path, mock_mcp_server_command, mcp_counter_file, request +): + pool, ctx = _make_pool_and_ctx( + tmp_path, mock_mcp_server_command, mcp_counter_file + ) + request.addfinalizer(pool.teardown_all) + + result = mcp_handler( + { + "server": "mock", + "tool": "sleep_forever", + "args": {}, + "timeout": 1, + }, + ctx, + ) + assert result.status == HandlerResultStatus.ERROR + assert "timed out" in result.message.lower() or "exceeded" in result.message.lower() + + # Second call to the same server should succeed because the pool + # respawns after the broken session. + result2 = mcp_handler( + { + "server": "mock", + "tool": "get_score", + "args": {}, + "expr": "result.score >= 7.0", + }, + ctx, + ) + assert result2.status == HandlerResultStatus.PASS, ( + f"expected respawn to succeed, got {result2.status}: {result2.message}" + ) + + # Verify the mock's counter file shows exactly TWO spawn events + # (initial + one respawn). Note: SC-002's spawn-once property applies + # only to the no-crash path (see test_teardown_on_success_path). + events = _read_counter(mcp_counter_file) + spawn_events = [e for e in events if e.get("kind") == "spawn"] + assert len(spawn_events) == 2, ( + f"expected 2 spawn events (initial + respawn), got {len(spawn_events)}" + ) + + +# --------------------------------------------------------------------------- +# T042: tool-side isError -> ERROR + session NOT broken +# --------------------------------------------------------------------------- + + +def test_tool_side_error_produces_error_no_broken( + tmp_path, mock_mcp_server_command, mcp_counter_file, request +): + pool, ctx = _make_pool_and_ctx( + tmp_path, mock_mcp_server_command, mcp_counter_file + ) + request.addfinalizer(pool.teardown_all) + + result = mcp_handler( + { + "server": "mock", + "tool": "raise_error", + "args": {"reason": "test"}, + }, + ctx, + ) + assert result.status == HandlerResultStatus.ERROR + assert "MCP tool error" in result.message + + # Same server, different tool: should NOT respawn (session not marked broken) + result2 = mcp_handler( + {"server": "mock", "tool": "echo", "args": {"text": "hi"}}, + ctx, + ) + assert result2.status == HandlerResultStatus.PASS + events = _read_counter(mcp_counter_file) + spawn_events = [e for e in events if e.get("kind") == "spawn"] + assert len(spawn_events) == 1, ( + f"expected exactly ONE spawn (tool-side error is not session failure), got {len(spawn_events)}" + ) + + +# --------------------------------------------------------------------------- +# T043: handshake failure (binary exits immediately) -> INCONCLUSIVE +# --------------------------------------------------------------------------- + + +def test_handshake_failure_produces_inconclusive_no_evidence(tmp_path, mcp_counter_file): + import sys as _sys + + server_config = McpServerConfig( + command=[_sys.executable, "-c", "import sys; sys.exit(0)"], + optional=True, + ) + pool = McpPool(servers={"deadbin": server_config}) + execution_context = ExecutionContext( + owner="octo", + repo="hello", + local_path=str(tmp_path), + mcp_servers={"deadbin": server_config}, + ) + ctx = HandlerContext( + local_path=str(tmp_path), + control_id="TEST-01", + execution_context=execution_context, + mcp_pool=pool, + ) + try: + result = mcp_handler( + {"server": "deadbin", "tool": "anything", "args": {}}, ctx + ) + finally: + pool.teardown_all() + + assert result.status == HandlerResultStatus.INCONCLUSIVE, ( + f"expected INCONCLUSIVE, got {result.status}: {result.message}" + ) + assert "handshake failed" in result.message.lower() or "handshake" in result.message.lower() + call = result.evidence["mcp_calls"][0] + assert "raw_response" not in call + + +def test_progress_log_line_emitted( + tmp_path, mock_mcp_server_command, mcp_counter_file, caplog, request +): + """The orchestrator emits `[N/M] dispatching_mcp .`.""" + import logging + + from darnit.config.framework_schema import HandlerInvocation + from darnit.sieve.models import CheckContext, ControlSpec + from darnit.sieve.orchestrator import SieveOrchestrator + + parent_env = {"DARNIT_MOCK_MCP_COUNTER_FILE_SRC": str(mcp_counter_file)} + for key, value in parent_env.items(): + os.environ[key] = value + + server_config = McpServerConfig( + command=mock_mcp_server_command, + env={"DARNIT_MOCK_MCP_COUNTER_FILE": "$DARNIT_MOCK_MCP_COUNTER_FILE_SRC"}, + ) + invocation = HandlerInvocation( + handler="mcp", + server="mock", + tool="get_score", + args={"repo_url": "github.com/$OWNER/$REPO"}, + expr="result.score >= 7.0", + ) + control_spec = ControlSpec( + control_id="OSPS-VM-01.01", + level=1, + domain=None, + name="MockScore", + description="", + metadata={"handler_invocations": [invocation]}, + ) + + execution_context = ExecutionContext( + owner="octo", + repo="hello", + local_path=str(tmp_path), + mcp_servers={"mock": server_config}, + ) + check_context = CheckContext( + owner="octo", + repo="hello", + local_path=str(tmp_path), + default_branch="main", + control_id="OSPS-VM-01.01", + execution_context=execution_context, + ) + + orchestrator = SieveOrchestrator(stop_on_llm=False) + + def _finalize() -> None: + if orchestrator._mcp_pool is not None: + orchestrator._mcp_pool.teardown_all() + orchestrator._mcp_pool = None + + request.addfinalizer(_finalize) + + with caplog.at_level(logging.INFO, logger="darnit.harness"): + result = orchestrator.verify(control_spec, check_context) + + assert result.status == "PASS", f"expected PASS got {result.status}: {result.message}" + matched = [ + rec + for rec in caplog.records + if rec.name == "darnit.harness" + and re.match(r"\[\d+/\d+\] \S+ dispatching_mcp mock\.get_score", rec.getMessage()) + ] + assert len(matched) == 1, f"expected 1 dispatching_mcp line, got {len(matched)}" diff --git a/tests/darnit/sieve/test_mcp_pool.py b/tests/darnit/sieve/test_mcp_pool.py new file mode 100644 index 00000000..3fd1a3a6 --- /dev/null +++ b/tests/darnit/sieve/test_mcp_pool.py @@ -0,0 +1,139 @@ +"""Lifecycle tests for the MCP pool through the sieve orchestrator. + +Locks SC-002 (single spawn across N controls; single teardown on the +success path) and the try/finally guarantee that teardown fires even +when the audit loop raises. +""" + +from __future__ import annotations + +import json +import os +from pathlib import Path +from typing import Any + +import pytest + +from darnit.config.framework_schema import HandlerInvocation, McpServerConfig +from darnit.core.models import ExecutionContext +from darnit.sieve.models import CheckContext, ControlSpec +from darnit.sieve.orchestrator import SieveOrchestrator + + +def _read_counter(counter_file: Path) -> list[dict[str, Any]]: + if not counter_file.exists(): + return [] + return [json.loads(line) for line in counter_file.read_text().splitlines() if line] + + +def _build_control(control_id: str) -> ControlSpec: + invocation = HandlerInvocation( + handler="mcp", + server="mock", + tool="get_score", + args={}, + expr="result.score >= 7.0", + ) + return ControlSpec( + control_id=control_id, + level=1, + domain=None, + name=f"Mock-{control_id}", + description="", + metadata={"handler_invocations": [invocation]}, + ) + + +# --------------------------------------------------------------------------- +# T044: teardown on the success path (single spawn, single teardown) +# --------------------------------------------------------------------------- + + +def test_teardown_on_success_path( + tmp_path, mock_mcp_server_command, mcp_counter_file +): + os.environ["DARNIT_MOCK_MCP_COUNTER_FILE_SRC"] = str(mcp_counter_file) + server_config = McpServerConfig( + command=mock_mcp_server_command, + env={"DARNIT_MOCK_MCP_COUNTER_FILE": "$DARNIT_MOCK_MCP_COUNTER_FILE_SRC"}, + ) + + execution_context = ExecutionContext( + owner="octo", + repo="hello", + local_path=str(tmp_path), + mcp_servers={"mock": server_config}, + ) + controls = [_build_control("CTRL-01"), _build_control("CTRL-02")] + + def _factory(_cid: str) -> CheckContext: + return CheckContext( + owner="octo", + repo="hello", + local_path=str(tmp_path), + default_branch="main", + control_id=_cid, + execution_context=execution_context, + ) + + orchestrator = SieveOrchestrator(stop_on_llm=False) + results = orchestrator.verify_batch(controls, _factory) + for r in results: + assert r.status == "PASS", f"{r.control_id}: {r.status} -- {r.message}" + + events = _read_counter(mcp_counter_file) + spawn_events = [e for e in events if e.get("kind") == "spawn"] + teardown_events = [e for e in events if e.get("kind") == "teardown"] + assert len(spawn_events) == 1, ( + f"SC-002: expected exactly ONE spawn across {len(controls)} controls, " + f"got {len(spawn_events)}" + ) + assert len(teardown_events) == 1, ( + f"expected exactly ONE teardown after verify_batch, got {len(teardown_events)}" + ) + + +# --------------------------------------------------------------------------- +# T045: teardown on the exception path +# --------------------------------------------------------------------------- + + +def test_teardown_on_exception_path( + tmp_path, mock_mcp_server_command, mcp_counter_file, monkeypatch +): + os.environ["DARNIT_MOCK_MCP_COUNTER_FILE_SRC"] = str(mcp_counter_file) + server_config = McpServerConfig( + command=mock_mcp_server_command, + env={"DARNIT_MOCK_MCP_COUNTER_FILE": "$DARNIT_MOCK_MCP_COUNTER_FILE_SRC"}, + ) + + execution_context = ExecutionContext( + owner="octo", + repo="hello", + local_path=str(tmp_path), + mcp_servers={"mock": server_config}, + ) + controls = [_build_control("CTRL-01"), _build_control("CTRL-02")] + + def _factory(_cid: str) -> CheckContext: + if _cid == "CTRL-02": + raise RuntimeError("test-injected failure") + return CheckContext( + owner="octo", + repo="hello", + local_path=str(tmp_path), + default_branch="main", + control_id=_cid, + execution_context=execution_context, + ) + + orchestrator = SieveOrchestrator(stop_on_llm=False) + with pytest.raises(RuntimeError, match="test-injected failure"): + orchestrator.verify_batch(controls, _factory) + + events = _read_counter(mcp_counter_file) + teardown_events = [e for e in events if e.get("kind") == "teardown"] + assert len(teardown_events) >= 1, ( + "verify_batch's finally block must tear down the pool even when the " + "control loop raises" + ) diff --git a/tests/darnit/sieve/test_mcp_trust.py b/tests/darnit/sieve/test_mcp_trust.py new file mode 100644 index 00000000..02e51c04 --- /dev/null +++ b/tests/darnit/sieve/test_mcp_trust.py @@ -0,0 +1,82 @@ +"""Tests for the Sigstore sidecar verification helper. + +The ``verify`` function is deliberately isolated so the sandboxing +follow-up (issue #375) can extend it without touching the pool. These +tests lock the four operator-observable outcomes: sidecar absent, +sidecar malformed, sigstore SDK unavailable, and (implicitly, via +mcp_handler coverage) success/failure verification. +""" + +from __future__ import annotations + +import importlib +import sys + +import pytest + +from darnit.sieve.mcp_trust import verify + + +def _has_sigstore() -> bool: + try: + importlib.import_module("sigstore.models") + importlib.import_module("sigstore.verify") + return True + except ImportError: + return False + + +# --------------------------------------------------------------------------- +# T033: no sidecar -> (False, reason) +# --------------------------------------------------------------------------- + + +def test_no_sidecar_returns_false_with_reason(tmp_path): + binary = tmp_path / "scorecard-mcp" + binary.write_bytes(b"fake elf") + ok, reason = verify(binary, "https://github.com/example/example") + assert ok is False + assert "no Sigstore sidecar" in reason + assert str(binary) in reason + + +# --------------------------------------------------------------------------- +# T034: malformed sidecar -> (False, "Sigstore verification failed: ...") +# --------------------------------------------------------------------------- + + +@pytest.mark.skipif( + not _has_sigstore(), + reason="sigstore extra not installed in this environment", +) +def test_malformed_sidecar_returns_false(tmp_path): + binary = tmp_path / "scorecard-mcp" + binary.write_bytes(b"fake elf") + sidecar = tmp_path / "scorecard-mcp.sigstore" + sidecar.write_text('{"not": "a bundle"}') + ok, reason = verify(binary, "https://github.com/example/example") + assert ok is False + assert "Sigstore verification failed" in reason + + +# --------------------------------------------------------------------------- +# T035: sigstore SDK unavailable -> (False, "install darnit-core[attestation]") +# --------------------------------------------------------------------------- + + +def test_sigstore_unavailable_returns_false(tmp_path, monkeypatch): + binary = tmp_path / "scorecard-mcp" + binary.write_bytes(b"fake elf") + sidecar = tmp_path / "scorecard-mcp.sigstore" + sidecar.write_text("{}") + + # Blot out any pre-imported sigstore submodules so the ImportError + # branch runs deterministically even when the extra is installed. + for name in list(sys.modules): + if name == "sigstore" or name.startswith("sigstore."): + monkeypatch.setitem(sys.modules, name, None) + + ok, reason = verify(binary, "https://github.com/example/example") + assert ok is False + assert "sigstore not installed" in reason + assert "darnit-core[attestation]" in reason From cc7431ca13a1f4b5ba3ee3d30d85c5b6dc957caf Mon Sep 17 00:00:00 2001 From: Michael Lieberman Date: Thu, 20 Aug 2026 11:48:54 -0400 Subject: [PATCH 2/2] fix(sieve): bind sigstore bundle to binary, close PATH TOCTOU, fix stderr capture (#380 review) Addresses three findings from Marc-cn's review of PR #380: 1. Sigstore verification now binds the bundle to the on-disk binary. `mcp_trust.verify` computes the binary's SHA-256 and: - tries `verify_artifact(Hashed(...), bundle, policy)` first (direct-artifact / `cosign sign-blob` shape -- the SDK does the binding as part of the signature check), and - falls back to `verify_dsse` + explicit `subject.digest.sha256` match against the binary's SHA-256 (in-toto / SLSA shape). The previous code called `verify_dsse` alone and never touched the binary bytes, so any valid bundle from the trusted publisher's workflow would have passed regardless of which binary sat beside it. Tests lock the mismatch-rejected, digest-match-accepted, and direct-artifact-accepted paths. 2. `_spawn` now execs the resolved absolute path (`str(binary_path)`) rather than handing the caller's relative command name to `StdioServerParameters`. The OS was re-resolving PATH at exec time, so a substituted binary between our `shutil.which` / Sigstore verification and the actual exec would run instead of the one we just verified. 3. Suite-order isolation: MCP `stdio_client` binds its `errlog=sys.stderr` default at module import time. If the mcp module was first imported while a pytest capsys-active test held `sys.stderr` replaced with a non-fd stream, every subsequent subprocess spawn raised `io.UnsupportedOperation: fileno`. Under Marc's repro (`pytest tests/darnit/harness tests/darnit/sieve/test_mcp_handler.py`) this caused 8/16 mcp tests to fail with `MCP handshake failed for mock: fileno`. Fix: resolve child stderr at call time via `_resolve_child_stderr` (prefer live `sys.stderr` with a working `fileno`, fall back to `sys.__stderr__`, then `os.devnull`). New regression test monkeypatches `sys.stderr` to a no-fileno stream and asserts spawn still succeeds. Zero product-package additions; all changes scoped to `packages/darnit/src/darnit/sieve/`. No new runtime dependencies. --- packages/darnit/src/darnit/sieve/mcp_pool.py | 73 +++++++- packages/darnit/src/darnit/sieve/mcp_trust.py | 106 ++++++++++- tests/darnit/sieve/test_mcp_handler.py | 45 +++++ tests/darnit/sieve/test_mcp_trust.py | 167 ++++++++++++++++++ 4 files changed, 376 insertions(+), 15 deletions(-) diff --git a/packages/darnit/src/darnit/sieve/mcp_pool.py b/packages/darnit/src/darnit/sieve/mcp_pool.py index e6115131..23421f4a 100644 --- a/packages/darnit/src/darnit/sieve/mcp_pool.py +++ b/packages/darnit/src/darnit/sieve/mcp_pool.py @@ -145,10 +145,19 @@ class _LoopBridge: The pool submits coroutines here so stdio-client subprocesses stay alive across successive sync ``call_tool`` invocations. Kept private to this module; not part of the reader contract. + + Thread-ownership contract: the loop MUST be constructed inside the + runner thread, not the calling thread. On POSIX, asyncio's child + watcher (used for subprocess pipe management) binds to the thread + that CREATED the loop. If the creating thread had an existing loop + with an incompatible watcher (a common state after other tests + exercise asyncio in the calling thread), stdio-subprocess spawn + fails inside anyio with the tell-tale ``fileno`` error. Creating + the loop in the runner side-steps that entirely. """ def __init__(self) -> None: - self._loop = asyncio.new_event_loop() + self._loop: asyncio.AbstractEventLoop | None = None self._ready = threading.Event() self._thread = threading.Thread( target=self._runner, @@ -156,20 +165,24 @@ def __init__(self) -> None: daemon=True, ) self._thread.start() + # Block until _runner has set self._loop AND started run_forever. self._ready.wait() def _runner(self) -> None: - asyncio.set_event_loop(self._loop) + loop = asyncio.new_event_loop() + asyncio.set_event_loop(loop) + self._loop = loop self._ready.set() try: - self._loop.run_forever() + loop.run_forever() finally: try: - self._loop.close() + loop.close() except Exception: # noqa: BLE001 - shutdown best-effort pass def run(self, coro: Any, timeout: float | None = None) -> Any: + assert self._loop is not None, "bridge not ready" future = asyncio.run_coroutine_threadsafe(coro, self._loop) try: return future.result(timeout=timeout) @@ -178,9 +191,10 @@ def run(self, coro: Any, timeout: float | None = None) -> Any: raise McpToolTimeout(f"MCP call exceeded {timeout:g}s") from err def close(self) -> None: - if not self._loop.is_running(): + loop = self._loop + if loop is None or not loop.is_running(): return - self._loop.call_soon_threadsafe(self._loop.stop) + loop.call_soon_threadsafe(loop.stop) self._thread.join(timeout=5) @@ -385,6 +399,13 @@ def _spawn(self, server_name: str, config: Any) -> PooledSession: env = self.build_child_env(config) + # Close a PATH TOCTOU: exec the resolved absolute path we + # verified above, not the caller's relative command name. If we + # handed StdioServerParameters `command[0]` unchanged, the OS + # would re-resolve PATH at exec time and could pick up a + # different binary than the one we hashed / Sigstore-verified. + exec_command = [str(binary_path), *command[1:]] + # Ensure the bridge loop is running before we schedule the owner # task on it. if self._bridge is None: @@ -396,7 +417,7 @@ def _spawn(self, server_name: str, config: Any) -> PooledSession: async def _run_session_owner() -> None: shutdown = asyncio.Event() try: - session, _stack = await _open_session_async(command, env) + session, _stack = await _open_session_async(exec_command, env) except Exception as err: # noqa: BLE001 -- surface to caller ready_future.set_exception(err) return @@ -542,14 +563,50 @@ async def _open_session_async( params = StdioServerParameters(command=command[0], args=command[1:], env=env) + # Resolve child stderr at call time, not at mcp-module-import time. + # ``stdio_client`` binds its ``errlog=sys.stderr`` default when the + # mcp module first loads; if that happened inside a pytest capsys + # context, the captured stream lacks a ``fileno()`` and every + # subsequent subprocess spawn raises ``io.UnsupportedOperation``. + # Pick a stream that always has a valid OS-level fd. + errlog = _resolve_child_stderr() + stack = AsyncExitStack() - streams = await stack.enter_async_context(stdio_client(params)) + streams = await stack.enter_async_context(stdio_client(params, errlog=errlog)) read, write = streams session = await stack.enter_async_context(ClientSession(read, write)) await session.initialize() return session, stack +def _resolve_child_stderr() -> Any: + """Return a stderr stream the child subprocess can inherit. + + Preference order: + + 1. ``sys.stderr`` if it exposes a working ``fileno()`` -- normal case + for CLI and interactive runs. + 2. ``sys.__stderr__`` (the original pre-capture stderr) if usable. + 3. ``os.devnull`` opened for write as a last resort. + """ + for candidate in (sys.stderr, sys.__stderr__): + if candidate is None: + continue + fileno = getattr(candidate, "fileno", None) + if not callable(fileno): + continue + try: + fileno() + except (OSError, ValueError): + continue + return candidate + # Fall back to devnull so the subprocess still gets a valid fd for + # its stderr. The caller MUST NOT close this stream; the AsyncExitStack + # is not responsible for it, but the fd leak is bounded to one per + # spawn and the OS reclaims it on process exit. + return open(os.devnull, "w") # noqa: SIM115 - lifetime tied to child + + __all__ = [ "MCP_ENV_SAFE_KEYS", "MCP_ENV_SAFE_PREFIXES", diff --git a/packages/darnit/src/darnit/sieve/mcp_trust.py b/packages/darnit/src/darnit/sieve/mcp_trust.py index 2e965edf..802d5d01 100644 --- a/packages/darnit/src/darnit/sieve/mcp_trust.py +++ b/packages/darnit/src/darnit/sieve/mcp_trust.py @@ -13,6 +13,25 @@ the handler resolves ERROR. There is NO code path from verification failure to PASS. +Bundle shapes supported: + +1. Direct-artifact signatures (``cosign sign-blob`` output, GitHub + Actions' Sigstore action against a binary artifact). Verification + uses :func:`sigstore.verify.Verifier.verify_artifact` with the + binary's precomputed SHA-256, which binds the signature to the + bytes on disk. +2. DSSE-wrapped in-toto attestations (the GoReleaser / SLSA shape). + Verification uses :func:`Verifier.verify_dsse` and then checks that + at least one ``subject[].digest.sha256`` in the returned statement + equals the binary's SHA-256. Without this second check, any valid + bundle from the trusted publisher's workflow would pass regardless + of which binary sat beside it. + +Direct-artifact is tried first because it is the cheaper and more +directly bound shape; DSSE fallback runs only when the bundle is not a +direct signature. Both paths reject a bundle whose subject digest does +not match the on-disk binary. + Alternatives considered: * Fetching a transparency-log attestation by SHA-256 at spawn time was @@ -25,6 +44,8 @@ from __future__ import annotations +import hashlib +import json import logging from pathlib import Path @@ -42,10 +63,11 @@ def verify(binary_path: Path, trusted_publisher: str) -> tuple[bool, str]: Returns: ``(True, reason)`` if verification succeeds against a policy - derived from ``trusted_publisher``; ``(False, reason)`` on any - failure -- missing sidecar, malformed bundle, sigstore SDK not - installed, or policy mismatch. The pool never sees an exception - from this function. + derived from ``trusted_publisher`` AND the bundle is bound to + the on-disk binary bytes; ``(False, reason)`` on any failure -- + missing sidecar, malformed bundle, unbound signature, sigstore + SDK not installed, or policy mismatch. The pool never sees an + exception from this function. """ sidecar = _find_sidecar(binary_path) if sidecar is None: @@ -55,6 +77,8 @@ def verify(binary_path: Path, trusted_publisher: str) -> tuple[bool, str]: ) try: + from sigstore._utils import HashAlgorithm # type: ignore[import-not-found] + from sigstore.hashes import Hashed # type: ignore[import-not-found] from sigstore.models import Bundle # type: ignore[import-not-found] from sigstore.verify import Verifier # type: ignore[import-not-found] from sigstore.verify.policy import ( # type: ignore[import-not-found] @@ -78,14 +102,82 @@ def verify(binary_path: Path, trusted_publisher: str) -> tuple[bool, str]: f"{trusted_publisher!r} does not name a GitHub owner/repo" ) + try: + binary_bytes = binary_path.read_bytes() + except OSError as err: + return False, f"Sigstore verification failed: cannot read {binary_path}: {err}" + binary_sha256 = hashlib.sha256(binary_bytes).digest() + binary_sha256_hex = binary_sha256.hex() + try: policy = GitHubWorkflowRepository(repo_ref) verifier = Verifier.production() - verifier.verify_dsse(bundle, policy) - except Exception as err: # noqa: BLE001 - sigstore raises assorted subclasses + except Exception as err: # noqa: BLE001 return False, f"Sigstore verification failed: {err}" - return True, f"verified against {trusted_publisher}" + # Path A: direct-artifact signature. verify_artifact binds the + # signature to the SHA-256 digest we hand it, so a substituted + # binary produces a mismatch here rather than a false accept. + hashed = Hashed(algorithm=HashAlgorithm.SHA2_256, digest=binary_sha256) + try: + verifier.verify_artifact(hashed, bundle, policy) + return True, ( + f"verified against {trusted_publisher} " + f"(direct-artifact signature; sha256={binary_sha256_hex[:16]}...)" + ) + except Exception as artifact_err: # noqa: BLE001 - fall through to DSSE + artifact_reason = str(artifact_err) + + # Path B: DSSE-wrapped attestation. verify_dsse returns the payload; + # we still have to check that the attested subject is our binary. + try: + payload_type, payload = verifier.verify_dsse(bundle, policy) + except Exception as dsse_err: # noqa: BLE001 + return False, ( + f"Sigstore verification failed for both paths: " + f"direct-artifact ({artifact_reason}); " + f"DSSE ({dsse_err})" + ) + + if payload_type != "application/vnd.in-toto+json": + return False, ( + f"Sigstore verification failed: DSSE payload type " + f"{payload_type!r} is not an in-toto statement" + ) + + try: + statement = json.loads(payload) + except json.JSONDecodeError as err: + return False, ( + f"Sigstore verification failed: in-toto statement not JSON: {err}" + ) + + subjects = statement.get("subject") or [] + if not isinstance(subjects, list) or not subjects: + return False, ( + "Sigstore verification failed: in-toto statement declares no " + "subject; cannot bind attestation to this binary" + ) + + for subject in subjects: + if not isinstance(subject, dict): + continue + digest_map = subject.get("digest") + if not isinstance(digest_map, dict): + continue + candidate = digest_map.get("sha256") + if isinstance(candidate, str) and candidate.lower() == binary_sha256_hex: + return True, ( + f"verified against {trusted_publisher} " + f"(DSSE in-toto attestation; sha256={binary_sha256_hex[:16]}...)" + ) + + return False, ( + "Sigstore verification failed: DSSE attestation is valid but no " + f"subject.digest.sha256 matches the on-disk binary " + f"(binary sha256={binary_sha256_hex[:16]}...); attestation may " + "cover a different artifact" + ) # --------------------------------------------------------------------------- diff --git a/tests/darnit/sieve/test_mcp_handler.py b/tests/darnit/sieve/test_mcp_handler.py index 098d7187..9caa2f0d 100644 --- a/tests/darnit/sieve/test_mcp_handler.py +++ b/tests/darnit/sieve/test_mcp_handler.py @@ -581,6 +581,51 @@ def test_handshake_failure_produces_inconclusive_no_evidence(tmp_path, mcp_count assert "raw_response" not in call +# --------------------------------------------------------------------------- +# Regression: captured sys.stderr must not break subprocess spawn +# --------------------------------------------------------------------------- + + +def test_pool_survives_captured_sys_stderr( + tmp_path, mock_mcp_server_command, mcp_counter_file, request, monkeypatch +): + """PR #380 review finding 3. + + ``mcp.client.stdio.stdio_client`` binds its ``errlog=sys.stderr`` + default at module-import time. Under pytest, if the mcp module was + first imported while a capsys-active test held ``sys.stderr`` + replaced with a non-fd stream, every subsequent spawn raised + ``io.UnsupportedOperation: fileno``. Simulate that state directly + (a stderr stream without a working ``fileno()``) and confirm the + pool still spawns cleanly. + """ + import io as _io + + class _NoFilenoStream(_io.StringIO): + def fileno(self): + raise _io.UnsupportedOperation("fileno") + + monkeypatch.setattr("sys.stderr", _NoFilenoStream()) + + pool, ctx = _make_pool_and_ctx( + tmp_path, mock_mcp_server_command, mcp_counter_file + ) + request.addfinalizer(pool.teardown_all) + + result = mcp_handler( + { + "server": "mock", + "tool": "get_score", + "args": {}, + "expr": "result.score >= 7.0", + }, + ctx, + ) + assert result.status == HandlerResultStatus.PASS, ( + f"handler said {result.status}: {result.message}" + ) + + def test_progress_log_line_emitted( tmp_path, mock_mcp_server_command, mcp_counter_file, caplog, request ): diff --git a/tests/darnit/sieve/test_mcp_trust.py b/tests/darnit/sieve/test_mcp_trust.py index 02e51c04..48f87fce 100644 --- a/tests/darnit/sieve/test_mcp_trust.py +++ b/tests/darnit/sieve/test_mcp_trust.py @@ -64,6 +64,173 @@ def test_malformed_sidecar_returns_false(tmp_path): # --------------------------------------------------------------------------- +# --------------------------------------------------------------------------- +# Subject-digest binding: DSSE path must reject bundles whose in-toto +# statement doesn't cover the on-disk binary (review of PR #380, finding 1). +# --------------------------------------------------------------------------- + + +@pytest.mark.skipif( + not _has_sigstore(), + reason="sigstore extra not installed in this environment", +) +def test_dsse_subject_digest_mismatch_rejected(tmp_path, monkeypatch): + """A valid bundle whose in-toto subject digest does not match the binary + on disk MUST be rejected. Prevents the substituted-binary attack Marc + flagged: any valid bundle from the trusted repo's workflow would + otherwise pass regardless of which binary sat beside it.""" + import hashlib as _hashlib + import json as _json + + from darnit.sieve import mcp_trust as _mcp_trust + + binary = tmp_path / "scorecard-mcp" + binary.write_bytes(b"actual on-disk bytes") + sidecar = tmp_path / "scorecard-mcp.sigstore" + sidecar.write_text('{"placeholder": true}') + + other_digest = _hashlib.sha256(b"a completely different artifact").hexdigest() + fake_statement = { + "_type": "https://in-toto.io/Statement/v0.1", + "predicateType": "https://slsa.dev/provenance/v0.2", + "subject": [{"name": "somebinary", "digest": {"sha256": other_digest}}], + "predicate": {}, + } + + class _FakeBundle: + @classmethod + def from_json(cls, _bytes): + return cls() + + class _FakeVerifier: + @classmethod + def production(cls): + return cls() + + def verify_artifact(self, hashed, bundle, policy): + raise RuntimeError("not a direct-artifact signature") + + def verify_dsse(self, bundle, policy): + return "application/vnd.in-toto+json", _json.dumps(fake_statement).encode() + + monkeypatch.setattr(_mcp_trust, "_find_sidecar", lambda p: sidecar) + # Patch the sigstore surfaces that verify() imports lazily. + import sigstore.models as _sm + import sigstore.verify as _sv + + monkeypatch.setattr(_sm, "Bundle", _FakeBundle) + monkeypatch.setattr(_sv, "Verifier", _FakeVerifier) + + ok, reason = _mcp_trust.verify(binary, "https://github.com/example/repo") + assert ok is False + assert "no subject.digest.sha256 matches" in reason + + +@pytest.mark.skipif( + not _has_sigstore(), + reason="sigstore extra not installed in this environment", +) +def test_dsse_subject_digest_match_accepted(tmp_path, monkeypatch): + """A DSSE bundle whose in-toto subject digest equals the binary's + SHA-256 MUST pass; balances the negative test above.""" + import hashlib as _hashlib + import json as _json + + from darnit.sieve import mcp_trust as _mcp_trust + + payload = b"the exact bytes that got signed" + binary = tmp_path / "scorecard-mcp" + binary.write_bytes(payload) + sidecar = tmp_path / "scorecard-mcp.sigstore" + sidecar.write_text('{"placeholder": true}') + + matching_digest = _hashlib.sha256(payload).hexdigest() + good_statement = { + "_type": "https://in-toto.io/Statement/v0.1", + "predicateType": "https://slsa.dev/provenance/v0.2", + "subject": [ + {"name": "somewhere_else", "digest": {"sha256": "0" * 64}}, + {"name": "scorecard-mcp", "digest": {"sha256": matching_digest}}, + ], + "predicate": {}, + } + + class _FakeBundle: + @classmethod + def from_json(cls, _bytes): + return cls() + + class _FakeVerifier: + @classmethod + def production(cls): + return cls() + + def verify_artifact(self, hashed, bundle, policy): + raise RuntimeError("not a direct-artifact signature") + + def verify_dsse(self, bundle, policy): + return "application/vnd.in-toto+json", _json.dumps(good_statement).encode() + + monkeypatch.setattr(_mcp_trust, "_find_sidecar", lambda p: sidecar) + import sigstore.models as _sm + import sigstore.verify as _sv + + monkeypatch.setattr(_sm, "Bundle", _FakeBundle) + monkeypatch.setattr(_sv, "Verifier", _FakeVerifier) + + ok, reason = _mcp_trust.verify(binary, "https://github.com/example/repo") + assert ok is True, reason + assert "DSSE in-toto attestation" in reason + + +@pytest.mark.skipif( + not _has_sigstore(), + reason="sigstore extra not installed in this environment", +) +def test_direct_artifact_signature_accepted(tmp_path, monkeypatch): + """A direct-artifact bundle (cosign sign-blob shape) MUST pass without + entering the DSSE fallback. verify_artifact does the binding for us.""" + from darnit.sieve import mcp_trust as _mcp_trust + + payload = b"artifact bytes" + binary = tmp_path / "cli-tool" + binary.write_bytes(payload) + sidecar = tmp_path / "cli-tool.sigstore" + sidecar.write_text('{"placeholder": true}') + + class _FakeBundle: + @classmethod + def from_json(cls, _bytes): + return cls() + + dsse_called = False + + class _FakeVerifier: + @classmethod + def production(cls): + return cls() + + def verify_artifact(self, hashed, bundle, policy): + return None # success + + def verify_dsse(self, bundle, policy): + nonlocal dsse_called + dsse_called = True + raise AssertionError("should not run when verify_artifact succeeds") + + monkeypatch.setattr(_mcp_trust, "_find_sidecar", lambda p: sidecar) + import sigstore.models as _sm + import sigstore.verify as _sv + + monkeypatch.setattr(_sm, "Bundle", _FakeBundle) + monkeypatch.setattr(_sv, "Verifier", _FakeVerifier) + + ok, reason = _mcp_trust.verify(binary, "https://github.com/example/repo") + assert ok is True, reason + assert "direct-artifact signature" in reason + assert dsse_called is False + + def test_sigstore_unavailable_returns_false(tmp_path, monkeypatch): binary = tmp_path / "scorecard-mcp" binary.write_bytes(b"fake elf")