From d4fe661f05bc17c9a05b33b936dc9287c24bf3a0 Mon Sep 17 00:00:00 2001 From: Alpamys Date: Sat, 4 Jul 2026 16:45:47 +0500 Subject: [PATCH] feat(mcp): plan-only mutating tools + --allow-mutating gate (v0.71.28 Part C) --- src/soup_cli/mcp_server/registry.py | 166 ++++++++++++++++++++++++---- tests/test_v07128.py | 65 +++++++++++ 2 files changed, 210 insertions(+), 21 deletions(-) diff --git a/src/soup_cli/mcp_server/registry.py b/src/soup_cli/mcp_server/registry.py index 17b6ad6..03d5e4c 100644 --- a/src/soup_cli/mcp_server/registry.py +++ b/src/soup_cli/mcp_server/registry.py @@ -66,8 +66,8 @@ def _sanitize(obj: Any) -> Any: return obj -def _read_json_under_cwd(path: str, field: str, *, max_bytes: int = _MAX_JSON_BYTES) -> dict: - """Load a JSON object argument (cwd-contained, symlink-rejected, size-capped). +def _read_text_under_cwd(path: str, field: str, *, max_bytes: int = _MAX_JSON_BYTES) -> str: + """Read a text file argument (cwd-contained, symlink-rejected, size-capped). Opens with ``O_NOFOLLOW`` (where available) and fstats the open fd so a symlink swapped in after the containment check cannot redirect the read @@ -87,11 +87,18 @@ def _read_json_under_cwd(path: str, field: str, *, max_bytes: int = _MAX_JSON_BY with os.fdopen(handle_fd, "r", encoding="utf-8") as handle: if os.fstat(handle.fileno()).st_size > max_bytes: raise McpToolError(f"{field} exceeds {max_bytes} bytes") - payload = json.load(handle) + return handle.read() + except (OSError, UnicodeDecodeError) as exc: + raise McpToolError(f"{field} is unreadable ({type(exc).__name__})") from exc + + +def _read_json_under_cwd(path: str, field: str, *, max_bytes: int = _MAX_JSON_BYTES) -> dict: + """Read a JSON *object* argument (delegates to :func:`_read_text_under_cwd`).""" + text = _read_text_under_cwd(path, field, max_bytes=max_bytes) + try: + payload = json.loads(text) except json.JSONDecodeError as exc: raise McpToolError(f"{field} is not valid JSON") from exc - except OSError as exc: - raise McpToolError(f"{field} is unreadable ({type(exc).__name__})") from exc if not isinstance(payload, dict): raise McpToolError(f"{field} must contain a JSON object") return payload @@ -353,14 +360,23 @@ def _resolve_gpu_memory_mcp(gpu: "str | None") -> float: return 24.0 +def _load_config_under_cwd(config: str): + """Read + validate a soup.yaml via the API-safe loader. + + Uses ``load_config_from_string`` (raises ``ValueError``) NOT ``load_config`` + (which prints to stdout + ``sys.exit`` — both fatal to the MCP stdio stream). + """ + from soup_cli.config.loader import load_config_from_string + + text = _read_text_under_cwd(config, "config") + try: + return load_config_from_string(text) + except ValueError as exc: + raise McpToolError(f"invalid config ({type(exc).__name__})") from exc + + def tool_profile(args: dict) -> dict: """`soup profile` — memory / speed / GPU estimate from a soup.yaml (no model load).""" - from pathlib import Path - - import yaml - from pydantic import ValidationError - - from soup_cli.config.loader import load_config from soup_cli.utils.gpu import model_size_from_name from soup_cli.utils.profiler import ( estimate_speed, @@ -369,13 +385,8 @@ def tool_profile(args: dict) -> dict: recommend_gpu, ) - config = _require_str(args, "config") + cfg = _load_config_under_cwd(_require_str(args, "config")) gpu = _opt_str(args, "gpu") - _enforce_data_path(config, "config") - try: - cfg = load_config(Path(config)) - except (OSError, ValueError, yaml.YAMLError, ValidationError) as exc: - raise McpToolError(f"invalid config ({type(exc).__name__})") from exc model_params_b = model_size_from_name(cfg.base) batch_size = cfg.training.batch_size @@ -493,6 +504,49 @@ def tool_ship_evidence(args: dict) -> dict: return verdict_to_dict(verdict) +# --------------------------------------------------------------------------- +# Mutating tool handlers — PLAN-ONLY in v1: they validate + render the exact +# command that WOULD run, but never execute. Live execution is a follow-up. +# --------------------------------------------------------------------------- + +_MUTATING_NOTE = ( + "plan-only: 'soup mcp serve' does not execute this. Run the command " + "yourself to proceed." +) + + +def tool_train_start(args: dict) -> dict: + """`soup train` (plan-only) — validate a soup.yaml + render the command.""" + import shlex + + config = _require_str(args, "config") + cfg = _load_config_under_cwd(config) + return { + "config_valid": True, + "task": cfg.task, + "base": cfg.base, + "would_run": f"soup train --config {shlex.quote(config)}", + "note": _MUTATING_NOTE, + } + + +def tool_export(args: dict) -> dict: + """`soup export` (plan-only) — validate format + render the command.""" + import shlex + + from soup_cli.commands.export import SUPPORTED_FORMATS + + model = _require_str(args, "model") + fmt = _require_str(args, "format") + if fmt not in SUPPORTED_FORMATS: + raise McpToolError("unsupported export format (see 'soup export --help')") + output = _opt_str(args, "output") + cmd = f"soup export --model {shlex.quote(model)} --format {fmt}" + if output: + cmd += f" --output {shlex.quote(output)}" + return {"format": fmt, "would_run": cmd, "note": _MUTATING_NOTE} + + # --------------------------------------------------------------------------- # Tool table # --------------------------------------------------------------------------- @@ -749,11 +803,81 @@ def _readonly_specs() -> "list[ToolSpec]": ] +def _refuse_mutating(name: str) -> Callable[[dict], dict]: + """Handler used for a mutating tool when ``--allow-mutating`` is off.""" + + def _handler(args: dict) -> dict: + raise McpToolError( + f"'{name}' can change state and is disabled; restart with " + "'soup mcp serve --allow-mutating' to enable (still plan-only in v1)." + ) + + return _handler + + +def _mutating_specs(*, allow_mutating: bool) -> "list[ToolSpec]": + """The plan-only mutating tools. + + Always LISTED (so clients can discover them), but their handler refuses + unless ``allow_mutating`` is set. Even when enabled they only render the + command that would run — v1 never executes training or export. + """ + entries = [ + ( + "train_start", + "Start training (plan-only)", + "Validate a soup.yaml and render the 'soup train' command (does not execute).", + { + "type": "object", + "properties": { + "config": { + "type": "string", + "description": "Path to a soup.yaml under cwd.", + } + }, + "required": ["config"], + "additionalProperties": False, + }, + tool_train_start, + ), + ( + "export", + "Export model (plan-only)", + "Validate a format and render the 'soup export' command (does not execute).", + { + "type": "object", + "properties": { + "model": {"type": "string", "description": "Adapter/model path or id."}, + "format": {"type": "string", "description": "gguf / onnx / awq / gptq / ..."}, + "output": {"type": "string", "description": "Optional output path."}, + }, + "required": ["model", "format"], + "additionalProperties": False, + }, + tool_export, + ), + ] + specs = [] + for name, title, description, schema, real in entries: + handler = real if allow_mutating else _refuse_mutating(name) + specs.append( + ToolSpec( + name=name, + title=title, + description=description, + input_schema=schema, + handler=handler, + mutating=True, + ) + ) + return specs + + def build_registry(*, allow_mutating: bool) -> "list[ToolSpec]": """Assemble the MCP tool table. - The read-only tools are always present. ``allow_mutating`` gates the - plan-only mutating tools (added in a later part). + The read-only tools are always present and executable. The mutating tools + are always listed but refuse unless ``allow_mutating`` is set (and are + plan-only even then). """ - specs = _readonly_specs() - return specs + return _readonly_specs() + _mutating_specs(allow_mutating=allow_mutating) diff --git a/tests/test_v07128.py b/tests/test_v07128.py index f0bb1a3..77b1e76 100644 --- a/tests/test_v07128.py +++ b/tests/test_v07128.py @@ -413,3 +413,68 @@ class TestShipEvidenceHandler: loose = reg.tool_ship_evidence({"evidence": "ev.json", "forgetting_threshold": 0.10}) assert strict["decision"] == "DON'T SHIP" assert loose["decision"] == "SHIP" + + +# --------------------------------------------------------------------------- +# Mutating tools (Part C): plan-only train_start / export + --allow-mutating gate +# --------------------------------------------------------------------------- + + +def _spec(name, *, allow_mutating): + return {s.name: s for s in reg.build_registry(allow_mutating=allow_mutating)}[name] + + +class TestMutatingTools: + def test_present_and_marked_in_both_registries(self): + for allow in (False, True): + specs = {s.name: s for s in reg.build_registry(allow_mutating=allow)} + assert "train_start" in specs and "export" in specs + assert specs["train_start"].mutating is True + assert specs["export"].mutating is True + + def test_train_start_refused_without_allow(self, tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + (tmp_path / "soup.yaml").write_text(_MIN_CONFIG, encoding="utf-8") + with pytest.raises(reg.McpToolError) as exc: + _spec("train_start", allow_mutating=False).handler({"config": "soup.yaml"}) + assert "allow-mutating" in str(exc.value) + + def test_export_refused_without_allow(self, tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + with pytest.raises(reg.McpToolError) as exc: + _spec("export", allow_mutating=False).handler({"model": "m", "format": "gguf"}) + assert "allow-mutating" in str(exc.value) + + def test_train_start_plan_only_with_allow(self, tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + (tmp_path / "soup.yaml").write_text(_MIN_CONFIG, encoding="utf-8") + out = _spec("train_start", allow_mutating=True).handler({"config": "soup.yaml"}) + assert out["config_valid"] is True + assert out["would_run"].startswith("soup train") + assert "plan-only" in out["note"] + + def test_train_start_invalid_config_raises(self, tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + (tmp_path / "bad.yaml").write_text("task: sft\n", encoding="utf-8") # missing base + with pytest.raises(reg.McpToolError): + _spec("train_start", allow_mutating=True).handler({"config": "bad.yaml"}) + + def test_export_plan_only_with_allow(self, tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + out = _spec("export", allow_mutating=True).handler( + {"model": "out/adapter", "format": "gguf"} + ) + assert out["would_run"].startswith("soup export") + assert out["format"] == "gguf" + assert "plan-only" in out["note"] + + def test_export_bad_format_raises(self, tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + with pytest.raises(reg.McpToolError): + _spec("export", allow_mutating=True).handler( + {"model": "out/adapter", "format": "nonsense-format"} + ) + + def test_registry_count_is_16_with_mutating(self): + assert len(reg.build_registry(allow_mutating=True)) == 16 + assert len(reg.build_registry(allow_mutating=False)) == 16