mirror of https://github.com/razor-ai/soup.git
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).
This commit is contained in:
Alpamys
parent
0bce9b4dc1
commit
2cb2b03d62
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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]
|
||||
|
|
|
|||
Loading…
Reference in New Issue