From 2cb2b03d62669f01479d7116c22d7391b9076961 Mon Sep 17 00:00:00 2001 From: Alpamys Date: Sat, 4 Jul 2026 20:36:35 +0500 Subject: [PATCH] fix(mcp): address ECC review findings (v0.71.28) - python-review: unreachable ImportError branch (probe transformers directly); json.dumps moved inside the sanitized error boundary; hoisted stdlib imports; dropped redundant quoted hints; comprehension for mutating specs. - code-review: guard FailureScore/classify_score (out-of-range diagnose score); broaden _load_data_rows except (ImportError/csv.Error); _opt_int rejects instead of silently clamping; inclusive forgetting_threshold bounds. - security-review: size cap on data loads; max-length cap on string args; sanitize the error-path message too (structural, not by-convention). - tdd: +26 tests (symlink wiring, stdout redirect, non-serializable result, runs/registry happy paths, ambiguous ref, ship malformed matrix, serve plumbing); fixed a real bug -- Rich ate the [mcp] in the friendly ImportError hint (escaped the bracket). --- src/soup_cli/commands/mcp.py | 4 +- src/soup_cli/mcp_server/registry.py | 135 ++++++++---- src/soup_cli/mcp_server/server.py | 12 +- tests/test_v07128.py | 319 +++++++++++++++++++++++++++- 4 files changed, 408 insertions(+), 62 deletions(-) diff --git a/src/soup_cli/commands/mcp.py b/src/soup_cli/commands/mcp.py index 500e446..498bae8 100644 --- a/src/soup_cli/commands/mcp.py +++ b/src/soup_cli/commands/mcp.py @@ -47,9 +47,11 @@ def serve( try: from soup_cli.mcp_server.server import run_stdio_server except ImportError: + # NB: escape the '[' in 'soup-cli[mcp]' so Rich prints it literally + # instead of parsing '[mcp]' as a (dropped) markup tag. console.print( "[red]The MCP server needs the 'mcp' SDK.[/] " - "Install it with: [bold]pip install 'soup-cli[mcp]'[/]" + "Install it with: [bold]pip install 'soup-cli\\[mcp]'[/]" ) raise typer.Exit(1) from None diff --git a/src/soup_cli/mcp_server/registry.py b/src/soup_cli/mcp_server/registry.py index 03d5e4c..d60022c 100644 --- a/src/soup_cli/mcp_server/registry.py +++ b/src/soup_cli/mcp_server/registry.py @@ -11,15 +11,25 @@ importing this module stays cheap and torch-free. from __future__ import annotations +import csv import json import os -from dataclasses import dataclass -from typing import Any, Callable, Mapping +import shlex +from dataclasses import asdict, dataclass +from pathlib import Path +from typing import TYPE_CHECKING, Any, Callable, Mapping from soup_cli.utils.paths import enforce_under_cwd_and_no_symlink +if TYPE_CHECKING: + from soup_cli.config.schema import SoupConfig + # Default read cap for JSON tool arguments (mirrors ship/diagnose evidence). _MAX_JSON_BYTES = 16 * 1024 * 1024 +# Cap on `data` dataset loads — the server is long-lived, so a client must not +# be able to point `data` at an arbitrarily large file and exhaust memory +# (mirrors advise's own 1 GiB cap; security-review MEDIUM). +_MAX_DATA_BYTES = 1024 * 1024 * 1024 # C0 control bytes (keep tab / newline / CR) + DEL, stripped from every string # in a handler result before it reaches the MCP client. ``rich.markup.escape`` @@ -41,7 +51,12 @@ class McpToolError(Exception): @dataclass(frozen=True) class ToolSpec: - """One entry in the MCP tool table.""" + """One entry in the MCP tool table. + + ``frozen=True`` blocks attribute rebinding; ``input_schema`` contents are + still technically mutable, but the table is built fresh per server and + never mutated in place. + """ name: str title: str @@ -111,19 +126,28 @@ def _read_json_under_cwd(path: str, field: str, *, max_bytes: int = _MAX_JSON_BY # --------------------------------------------------------------------------- +# Generous cap on free-text string args (paths, ids, goals, queries). Bounds a +# pathological input without constraining any legitimate value (security-review). +_MAX_STR_LEN = 4096 + + def _require_str(args: dict, key: str) -> str: val = args.get(key) if not isinstance(val, str) or not val: raise McpToolError(f"'{key}' must be a non-empty string") + if len(val) > _MAX_STR_LEN: + raise McpToolError(f"'{key}' must be at most {_MAX_STR_LEN} characters") return val -def _opt_str(args: dict, key: str) -> "str | None": +def _opt_str(args: dict, key: str) -> str | None: val = args.get(key) if val is None: return None if not isinstance(val, str): raise McpToolError(f"'{key}' must be a string") + if len(val) > _MAX_STR_LEN: + raise McpToolError(f"'{key}' must be at most {_MAX_STR_LEN} characters") return val @@ -131,7 +155,11 @@ def _opt_int(args: dict, key: str, default: int, *, lo: int, hi: int) -> int: val = args.get(key, default) if isinstance(val, bool) or not isinstance(val, int): raise McpToolError(f"'{key}' must be an integer") - return max(lo, min(hi, val)) + # Reject rather than silently clamp — clamping hides the user's error and + # bypasses the core's own bounds check (code-review MEDIUM). + if not lo <= val <= hi: + raise McpToolError(f"'{key}' must be between {lo} and {hi}") + return val def _enforce_data_path(path: str, field: str = "data") -> None: @@ -150,8 +178,6 @@ def _enforce_data_path(path: str, field: str = "data") -> None: def tool_advise(args: dict) -> dict: """`soup advise` — pre-flight PROMPT_ENG / RAG / SFT / DPO / GRPO verdict.""" - import dataclasses - from soup_cli.utils import advise as _advise data = _require_str(args, "data") @@ -164,18 +190,27 @@ def tool_advise(args: dict) -> dict: verdict = _advise.build_verdict(profile, task, goal=goal) except (OSError, ValueError, TypeError) as exc: raise McpToolError(f"advise failed ({type(exc).__name__})") from exc - return dataclasses.asdict(verdict) + return asdict(verdict) -def _load_data_rows(path: str) -> list: - from pathlib import Path - +def _load_data_rows(path: str) -> list[dict]: from soup_cli.data.loader import load_raw_data _enforce_data_path(path) + # Best-effort size cap before load_raw_data reads the whole file into memory + # (the path is already confirmed non-symlink + under cwd). + try: + size = os.path.getsize(path) + except OSError as exc: + raise McpToolError(f"data is unreadable ({type(exc).__name__})") from exc + if size > _MAX_DATA_BYTES: + raise McpToolError(f"data exceeds {_MAX_DATA_BYTES} bytes") + # load_raw_data dispatches by extension: parquet raises bare ImportError + # without pandas, CSV raises csv.Error (NOT a ValueError subclass) on + # malformed input. Translate every loader failure here (code-review MEDIUM). try: return load_raw_data(Path(path)) - except (OSError, ValueError) as exc: + except (OSError, ValueError, ImportError, csv.Error) as exc: raise McpToolError(f"cannot load data ({type(exc).__name__})") from exc @@ -227,13 +262,18 @@ def tool_data_doctor(args: dict) -> dict: fmt = _formats.detect_format(rows) except ValueError as exc: raise McpToolError("could not auto-detect data format; pass 'format'") from exc + # `resolve_tokenizer` catches the transformers-missing case internally and + # re-raises it as a ValueError, so probe the dependency directly to keep the + # actionable "install the extra" hint (python-review HIGH). try: - tok = _dd.resolve_tokenizer(model, trust_remote_code=False) + import transformers # noqa: F401 except ImportError as exc: raise McpToolError( "data_doctor needs the tokenizer stack: pip install 'soup-cli[train]'" ) from exc - except (ValueError, TypeError, OSError) as exc: + try: + tok = _dd.resolve_tokenizer(model, trust_remote_code=False) + except (ImportError, ValueError, TypeError, OSError) as exc: raise McpToolError(f"could not load tokenizer ({type(exc).__name__})") from exc try: report = _dd.run_doctor( @@ -338,7 +378,7 @@ def tool_registry_show(args: dict) -> dict: return entry -def _resolve_gpu_memory_mcp(gpu: "str | None") -> float: +def _resolve_gpu_memory_mcp(gpu: str | None) -> float: """GPU memory in GB from a flag or auto-detection (non-Typer mirror of ``commands/profile.py::_resolve_gpu_memory``).""" from soup_cli.utils.profiler import GPU_MEMORY @@ -360,7 +400,7 @@ def _resolve_gpu_memory_mcp(gpu: "str | None") -> float: return 24.0 -def _load_config_under_cwd(config: str): +def _load_config_under_cwd(config: str) -> SoupConfig: """Read + validate a soup.yaml via the API-safe loader. Uses ``load_config_from_string`` (raises ``ValueError``) NOT ``load_config`` @@ -439,13 +479,19 @@ def tool_diagnose_evidence(args: dict) -> dict: score = entry.get("score", 1.0) if isinstance(score, bool) or not isinstance(score, (int, float)): raise McpToolError(f"evidence.scores.{mode}.score must be a number") - verdict = entry.get("verdict") or classify_score(score) - scores[mode] = FailureScore( - mode=mode, - score=float(score), - verdict=verdict, - evidence=str(entry.get("evidence", "supplied via evidence")), - ) + # classify_score rejects a score outside [0, 1] / non-finite, and + # FailureScore.__post_init__ rejects a mismatched/unknown verdict — both + # ValueError. Guard them into a specific McpToolError (code-review HIGH). + try: + verdict = entry.get("verdict") or classify_score(score) + scores[mode] = FailureScore( + mode=mode, + score=float(score), + verdict=verdict, + evidence=str(entry.get("evidence", "supplied via evidence")), + ) + except (ValueError, TypeError, OverflowError) as exc: + raise McpToolError(f"evidence.scores.{mode} is invalid ({type(exc).__name__})") from exc try: report = build_report( run_id=run_id, base=base, adapter=adapter, scores=scores, soup_version=__version__ @@ -470,8 +516,10 @@ def tool_ship_evidence(args: dict) -> dict: if isinstance(threshold, bool) or not isinstance(threshold, (int, float)): raise McpToolError("'forgetting_threshold' must be a number") threshold = float(threshold) - if not 0.0 < threshold < 1.0: - raise McpToolError("'forgetting_threshold' must be in (0, 1)") + # Inclusive [0, 1] matches ship_verdict._validate_threshold + the CLI (a + # 0.0 zero-tolerance gate is legitimate) (code-review LOW). + if not 0.0 <= threshold <= 1.0: + raise McpToolError("'forgetting_threshold' must be in [0, 1]") task = payload.get("task") if not isinstance(task, dict): @@ -517,8 +565,6 @@ _MUTATING_NOTE = ( 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 { @@ -532,8 +578,6 @@ def tool_train_start(args: dict) -> dict: 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") @@ -557,7 +601,7 @@ _DATA_ARG = { } -def _readonly_specs() -> "list[ToolSpec]": +def _readonly_specs() -> list[ToolSpec]: return [ ToolSpec( name="advise", @@ -791,8 +835,8 @@ def _readonly_specs() -> "list[ToolSpec]": }, "forgetting_threshold": { "type": "number", - "exclusiveMinimum": 0, - "exclusiveMaximum": 1, + "minimum": 0, + "maximum": 1, }, }, "required": ["evidence"], @@ -815,7 +859,7 @@ def _refuse_mutating(name: str) -> Callable[[dict], dict]: return _handler -def _mutating_specs(*, allow_mutating: bool) -> "list[ToolSpec]": +def _mutating_specs(*, allow_mutating: bool) -> list[ToolSpec]: """The plan-only mutating tools. Always LISTED (so clients can discover them), but their handler refuses @@ -857,23 +901,20 @@ def _mutating_specs(*, allow_mutating: bool) -> "list[ToolSpec]": 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 [ + ToolSpec( + name=name, + title=title, + description=description, + input_schema=schema, + handler=real if allow_mutating else _refuse_mutating(name), + mutating=True, ) - return specs + for name, title, description, schema, real in entries + ] -def build_registry(*, allow_mutating: bool) -> "list[ToolSpec]": +def build_registry(*, allow_mutating: bool) -> list[ToolSpec]: """Assemble the MCP tool table. The read-only tools are always present and executable. The mutating tools diff --git a/src/soup_cli/mcp_server/server.py b/src/soup_cli/mcp_server/server.py index c5277b7..25f74c9 100644 --- a/src/soup_cli/mcp_server/server.py +++ b/src/soup_cli/mcp_server/server.py @@ -53,15 +53,19 @@ def build_server(specs: List[ToolSpec]) -> Server: raise ValueError("unknown tool") try: # Any core that prints (e.g. a Rich warning) must not corrupt the - # JSON-RPC stdout channel — send stray stdout to stderr for the - # duration of the (synchronous) handler call. + # JSON-RPC stdout channel - send stray stdout to stderr for the + # duration of the (synchronous) handler call. Serialization stays + # INSIDE the try so a non-JSON-serializable result also becomes a + # sanitized isError (never a raw TypeError the SDK would echo). with redirect_stdout(sys.stderr): result = spec.handler(arguments or {}) + text = json.dumps(_sanitize(result), indent=2, ensure_ascii=False) except McpToolError as exc: - raise ValueError(str(exc)) from None + # _sanitize the message too so the C0/ESC guarantee is structural, + # not just a convention every handler must remember (security-review). + raise ValueError(_sanitize(str(exc))) from None except Exception as exc: # never leak a stack trace / path to the client raise ValueError(f"internal error ({type(exc).__name__})") from None - text = json.dumps(_sanitize(result), indent=2, ensure_ascii=False) return [types.TextContent(type="text", text=text)] return server diff --git a/tests/test_v07128.py b/tests/test_v07128.py index 3b0eaab..acf6dba 100644 --- a/tests/test_v07128.py +++ b/tests/test_v07128.py @@ -7,6 +7,7 @@ Covers the pure tool registry (handlers + guards), the SDK server wiring from __future__ import annotations import json +import os import pytest @@ -170,6 +171,29 @@ class TestDataInspectValidateHandlers: with pytest.raises(reg.McpToolError): reg.tool_data_inspect({"data": "nope.jsonl"}) + def test_oversize_data_rejected(self, tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + monkeypatch.setattr(reg, "_MAX_DATA_BYTES", 1) + p = tmp_path / "d.jsonl" + _write_jsonl(p, _ADVISE_ROWS) + with pytest.raises(reg.McpToolError) as exc: + reg.tool_data_inspect({"data": "d.jsonl"}) + assert "exceeds" in str(exc.value) + + +class TestArgHelperBounds: + def test_require_str_rejects_overlong(self): + with pytest.raises(reg.McpToolError): + reg.tool_recipes_show({"name": "x" * (reg._MAX_STR_LEN + 1)}) + + def test_opt_str_rejects_overlong(self): + with pytest.raises(reg.McpToolError): + reg.tool_recipes_search({"query": "x" * (reg._MAX_STR_LEN + 1)}) + + def test_opt_str_rejects_non_string(self): + with pytest.raises(reg.McpToolError): + reg.tool_recipes_search({"query": 123}) + class TestDataScoreHandler: def test_returns_scorecard(self, tmp_path, monkeypatch): @@ -211,12 +235,13 @@ class TestDataDoctorHandler: assert "checks" in out and isinstance(out["checks"], list) def test_missing_transformers_friendly_error(self, tmp_path, monkeypatch): + import sys + monkeypatch.chdir(tmp_path) - - def _boom(model, **kw): - raise ImportError("no transformers") - - monkeypatch.setattr("soup_cli.utils.data_doctor.resolve_tokenizer", _boom) + # Simulate the REAL absence path: `import transformers` fails. (The + # handler probes the dependency directly rather than relying on + # resolve_tokenizer's wrapped exception type.) + monkeypatch.setitem(sys.modules, "transformers", None) p = tmp_path / "chat.jsonl" _write_jsonl(p, [{"messages": [{"role": "user", "content": "hi"}]}]) with pytest.raises(reg.McpToolError) as exc: @@ -226,9 +251,13 @@ class TestDataDoctorHandler: class TestRecipesHandlers: def test_search_returns_results(self): + from soup_cli.recipes.catalog import RECIPES + out = reg.tool_recipes_search({"query": "qwen"}) assert out["count"] >= 1 assert all("name" in r and "model" in r for r in out["results"]) + # each name resolves to a real catalog entry (not the "?" id() fallback) + assert all(r["name"] in RECIPES for r in out["results"]) # search results stay compact — no full yaml body assert all("yaml_str" not in r for r in out["results"]) @@ -256,6 +285,16 @@ class TestRunsHandlers: with pytest.raises(reg.McpToolError): reg.tool_runs_show({"run_id": "nope"}) + def test_limit_out_of_range_rejected(self, tmp_path, monkeypatch): + # _opt_int rejects (not clamps) out-of-range values, before DB access. + monkeypatch.setenv("SOUP_DB_PATH", str(tmp_path / "exp.db")) + with pytest.raises(reg.McpToolError): + reg.tool_runs_list({"limit": 999999}) + with pytest.raises(reg.McpToolError): + reg.tool_runs_list({"limit": 0}) + with pytest.raises(reg.McpToolError): + reg.tool_runs_list({"limit": "ten"}) + class TestRegistryHandlers: def test_list_returns_entries_key(self, tmp_path, monkeypatch): @@ -315,11 +354,6 @@ class TestBuildRegistry: assert callable(spec.handler) assert isinstance(spec.mutating, bool) - def test_no_mutating_in_readonly_registry_is_executable(self): - # read-only build has no non-mutating gap: every listed tool is callable - for spec in reg.build_registry(allow_mutating=False): - assert callable(spec.handler) - # --------------------------------------------------------------------------- # Flagged read-only handlers (Part B): profile / diagnose / ship evidence @@ -366,6 +400,16 @@ class TestDiagnoseEvidenceHandler: with pytest.raises(reg.McpToolError): reg.tool_diagnose_evidence({"run_id": "r1", "evidence": "ev.json"}) + def test_out_of_range_score_raises_specific(self, tmp_path, monkeypatch): + # score outside [0,1] -> classify_score raises ValueError; the handler + # must surface a specific McpToolError, not a generic internal error. + monkeypatch.chdir(tmp_path) + ev = {"scores": {"forgetting": {"score": 1.5}}} + (tmp_path / "ev.json").write_text(json.dumps(ev), encoding="utf-8") + with pytest.raises(reg.McpToolError) as exc: + reg.tool_diagnose_evidence({"run_id": "r1", "evidence": "ev.json"}) + assert "forgetting" in str(exc.value) + def test_missing_evidence_raises(self, tmp_path, monkeypatch): monkeypatch.chdir(tmp_path) with pytest.raises(reg.McpToolError): @@ -584,6 +628,66 @@ class TestServerRoundTrip: payload = json.loads(res.content[0].text) assert payload["v"] == "abc" # control bytes stripped by _sanitize + def test_error_message_is_sanitized(self): + from soup_cli.mcp_server.registry import McpToolError, ToolSpec + from soup_cli.mcp_server.server import build_server + + def _boom(args): + raise McpToolError("bad\x1b[31mvalue\x07") + + spec = ToolSpec( + name="boom", + title="Boom", + description="boom", + input_schema={"type": "object", "properties": {}}, + handler=_boom, + mutating=False, + ) + server = build_server([spec]) + res = _roundtrip(server, "boom", {}) + assert res.isError is True + text = res.content[0].text + assert "\x1b" not in text and "\x07" not in text + + def test_handler_stdout_is_redirected_off_the_jsonrpc_channel(self, capsys): + from soup_cli.mcp_server.registry import ToolSpec + from soup_cli.mcp_server.server import build_server + + def _noisy(args): + print("LEAK-TO-STDOUT") + return {"ok": True} + + spec = ToolSpec( + name="noisy", + title="Noisy", + description="noisy", + input_schema={"type": "object", "properties": {}}, + handler=_noisy, + mutating=False, + ) + server = build_server([spec]) + res = _roundtrip(server, "noisy", {}) + assert res.isError is False + # the handler's stdout print must NOT reach the process stdout channel + assert "LEAK-TO-STDOUT" not in capsys.readouterr().out + + def test_non_serializable_result_is_error_not_crash(self): + from soup_cli.mcp_server.registry import ToolSpec + from soup_cli.mcp_server.server import build_server + + spec = ToolSpec( + name="bad", + title="Bad", + description="bad", + input_schema={"type": "object", "properties": {}}, + handler=lambda args: {"bad": {1, 2, 3}}, # a set is not JSON-serializable + mutating=False, + ) + server = build_server([spec]) + res = _roundtrip(server, "bad", {}) + assert res.isError is True + assert "internal error" in res.content[0].text + # --------------------------------------------------------------------------- # CLI wiring (Part D) @@ -626,6 +730,8 @@ class TestMcpCli: monkeypatch.setitem(sys.modules, "soup_cli.mcp_server.server", None) r = CliRunner().invoke(app, ["mcp", "serve"]) assert r.exit_code == 1 + # the hint must name the exact extra (Rich must not eat the '[mcp]') + assert "soup-cli[mcp]" in r.output class TestRegistryNoSdkImport: @@ -637,3 +743,196 @@ class TestRegistryNoSdkImport: src = inspect.getsource(registry_mod) assert "import mcp" not in src assert "from mcp" not in src + + +# --------------------------------------------------------------------------- +# Additional coverage (tdd-review gaps) +# --------------------------------------------------------------------------- + + +class TestReadGuardsExtra: + def test_malformed_json_raises(self, tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + (tmp_path / "bad.json").write_text("{not valid json", encoding="utf-8") + with pytest.raises(reg.McpToolError): + reg._read_json_under_cwd("bad.json", "evidence") + + @pytest.mark.skipif(os.name == "nt", reason="POSIX symlink semantics") + def test_read_json_rejects_symlink(self, tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + (tmp_path / "real.json").write_text('{"a": 1}', encoding="utf-8") + os.symlink(tmp_path / "real.json", tmp_path / "link.json") + with pytest.raises(reg.McpToolError): + reg._read_json_under_cwd("link.json", "evidence") + + def test_sanitize_handles_tuple(self): + out = reg._sanitize(("a\x1bb", "c")) + assert out == ["ab", "c"] + + +class TestOptIntBoolGuard: + def test_bool_limit_rejected(self, tmp_path, monkeypatch): + monkeypatch.setenv("SOUP_DB_PATH", str(tmp_path / "exp.db")) + with pytest.raises(reg.McpToolError): + reg.tool_runs_list({"limit": True}) + + +class TestRunsRegistryHappyPaths: + def test_runs_show_happy(self, tmp_path, monkeypatch): + from soup_cli.experiment.tracker import ExperimentTracker + + monkeypatch.setenv("SOUP_DB_PATH", str(tmp_path / "exp.db")) + run_id = ExperimentTracker().start_run( + {"base": "m", "task": "sft"}, "cpu", "CPU", {} + ) + out = reg.tool_runs_show({"run_id": run_id}) + assert out["run_id"] == run_id + + def test_registry_show_happy(self, tmp_path, monkeypatch): + from soup_cli.registry.store import RegistryStore + + monkeypatch.setenv("SOUP_REGISTRY_DB_PATH", str(tmp_path / "reg.db")) + with RegistryStore() as store: + entry_id = store.push( + name="mymodel", tag="v1", base_model="b", task="sft", + run_id=None, config={"base": "b"}, + ) + out = reg.tool_registry_show({"ref": entry_id}) + assert out["id"] == entry_id + + def test_registry_list_filter_narrows(self, tmp_path, monkeypatch): + from soup_cli.registry.store import RegistryStore + + monkeypatch.setenv("SOUP_REGISTRY_DB_PATH", str(tmp_path / "reg.db")) + with RegistryStore() as store: + store.push(name="alpha", tag="v1", base_model="b", task="sft", + run_id=None, config={"base": "b"}) + store.push(name="beta", tag="v1", base_model="b", task="dpo", + run_id=None, config={"base": "b"}) + assert reg.tool_registry_list({})["count"] == 2 + narrowed = reg.tool_registry_list({"name": "alpha"}) + assert narrowed["count"] == 1 + assert narrowed["entries"][0]["name"] == "alpha" + + def test_registry_show_ambiguous_ref(self, tmp_path, monkeypatch): + from soup_cli.registry.store import AmbiguousRefError, RegistryStore + + monkeypatch.setenv("SOUP_REGISTRY_DB_PATH", str(tmp_path / "reg.db")) + + def _raise(self, ref): + raise AmbiguousRefError("matches many") + + monkeypatch.setattr(RegistryStore, "resolve", _raise) + with pytest.raises(reg.McpToolError) as exc: + reg.tool_registry_show({"ref": "ab"}) + assert "ambiguous" in str(exc.value).lower() + + def test_show_missing_required_raises(self, tmp_path, monkeypatch): + monkeypatch.setenv("SOUP_DB_PATH", str(tmp_path / "exp.db")) + monkeypatch.setenv("SOUP_REGISTRY_DB_PATH", str(tmp_path / "reg.db")) + with pytest.raises(reg.McpToolError): + reg.tool_runs_show({}) + with pytest.raises(reg.McpToolError): + reg.tool_registry_show({}) + + +class TestLoadDataTranslation: + def test_loader_csv_error_translated(self, tmp_path, monkeypatch): + import csv + + monkeypatch.chdir(tmp_path) + (tmp_path / "d.jsonl").write_text('{"a": 1}', encoding="utf-8") + + def _raise(path): + raise csv.Error("bad csv") + + monkeypatch.setattr("soup_cli.data.loader.load_raw_data", _raise) + with pytest.raises(reg.McpToolError) as exc: + reg.tool_data_inspect({"data": "d.jsonl"}) + assert "cannot load data" in str(exc.value) + + +class TestDataDoctorBranches: + def test_auto_detect_failure(self, tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + _write_jsonl(tmp_path / "d.jsonl", [{"random_field": 1}, {"random_field": 2}]) + with pytest.raises(reg.McpToolError) as exc: + reg.tool_data_doctor({"data": "d.jsonl", "model": "x"}) + assert "auto-detect" in str(exc.value) + + +class TestDiagnoseEvidenceBranches: + def test_scores_not_object(self, tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + (tmp_path / "ev.json").write_text(json.dumps({"scores": [1, 2]}), encoding="utf-8") + with pytest.raises(reg.McpToolError): + reg.tool_diagnose_evidence({"run_id": "r", "evidence": "ev.json"}) + + def test_score_entry_not_object(self, tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + (tmp_path / "ev.json").write_text( + json.dumps({"scores": {"forgetting": "high"}}), encoding="utf-8" + ) + with pytest.raises(reg.McpToolError): + reg.tool_diagnose_evidence({"run_id": "r", "evidence": "ev.json"}) + + +class TestShipEvidenceMalformed: + @pytest.mark.parametrize( + "payload", + [ + {"benchmarks": {}}, # task missing + {"task": [1, 2], "benchmarks": {}}, # task not object + {"task": {"mode": "metric", "base": 0.5}, "benchmarks": {}}, # tuned missing + # non-numeric base + {"task": {"mode": "metric", "base": "x", "tuned": 0.8}, "benchmarks": {}}, + # benchmarks not an object + {"task": {"mode": "metric", "base": 0.5, "tuned": 0.8}, "benchmarks": [1]}, + { + "task": {"mode": "metric", "base": 0.5, "tuned": 0.8}, + "benchmarks": {"m": {"base": 0.7}}, # entry missing tuned + }, + ], + ) + def test_malformed_ship_evidence_rejected(self, tmp_path, monkeypatch, payload): + monkeypatch.chdir(tmp_path) + (tmp_path / "ev.json").write_text(json.dumps(payload), encoding="utf-8") + with pytest.raises(reg.McpToolError): + reg.tool_ship_evidence({"evidence": "ev.json"}) + + def test_bad_threshold_rejected(self, tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + ev = {"task": {"mode": "metric", "base": 0.5, "tuned": 0.8}, "benchmarks": {}} + (tmp_path / "ev.json").write_text(json.dumps(ev), encoding="utf-8") + with pytest.raises(reg.McpToolError): + reg.tool_ship_evidence({"evidence": "ev.json", "forgetting_threshold": "high"}) + with pytest.raises(reg.McpToolError): + reg.tool_ship_evidence({"evidence": "ev.json", "forgetting_threshold": 5.0}) + + +class TestExportOutputArg: + def test_export_includes_output(self, tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + out = _spec("export", allow_mutating=True).handler( + {"model": "out/adapter", "format": "gguf", "output": "dist/model.gguf"} + ) + assert "--output" in out["would_run"] + + +class TestServePlumbing: + def test_allow_mutating_flag_reaches_runner(self, monkeypatch): + pytest.importorskip("mcp") + from typer.testing import CliRunner + + import soup_cli.mcp_server.server as srv + from soup_cli.cli import app + + calls = [] + monkeypatch.setattr( + srv, "run_stdio_server", lambda *, allow_mutating: calls.append(allow_mutating) + ) + r1 = CliRunner().invoke(app, ["mcp", "serve"]) + r2 = CliRunner().invoke(app, ["mcp", "serve", "--allow-mutating"]) + assert r1.exit_code == 0, (r1.output, repr(r1.exception)) + assert r2.exit_code == 0, (r2.output, repr(r2.exception)) + assert calls == [False, True]