fix: correctness/quality nits roundup + lint (#541)

Fixes #541.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: Shashankss1205

src/sktime_mcp/data/adapters/pandas_adapter.py

@@ -199,13 +199,16 @@ def validate(self, data: pd.DataFrame) -> tuple[bool, dict[str, Any]]:
         target_col = self.config.get("target_column")
         if target_col is None and len(data.columns) > 0:
             target_col = data.columns[0]
-        if target_col is not None and target_col in data.columns:
-            if not pd.api.types.is_numeric_dtype(data[target_col]):
-                warnings.append(
-                    f"Target column '{target_col}' has non-numeric dtype "
-                    f"'{data[target_col].dtype}'. Forecasting requires numeric values; "
-                    "convert the column before fitting."
-                )
+        if (
+            target_col is not None
+            and target_col in data.columns
+            and not pd.api.types.is_numeric_dtype(data[target_col])
+        ):
+            warnings.append(
+                f"Target column '{target_col}' has non-numeric dtype "
+                f"'{data[target_col].dtype}'. Forecasting requires numeric values; "
+                "convert the column before fitting."
+            )
 
         # Check for constant values
         for col in data.columns:

src/sktime_mcp/runtime/executor.py

@@ -638,9 +638,13 @@ def predict(
 
             out = {
                 "success": True,
-                "horizon": len(fh) if hasattr(fh, "__len__") else fh,
                 "mode": mode,
             }
+            # horizon is only meaningful for forecasters; echoing it for
+            # classifiers/regressors/transformers implied a truncation that
+            # didn't happen (N-01).
+            if not (is_classifier_or_regressor or is_transformer):
+                out["horizon"] = len(fh) if hasattr(fh, "__len__") else fh
             if mode == "predict":
                 out["predictions"] = result
             elif mode == "predict_interval":
@@ -1392,7 +1396,9 @@ def format_data_handle(
                 X = X[~X.index.duplicated(keep="first")]
             changes_made["duplicates_removed"] = n_duplicates
 
-        # 2. Sort by index
+        # 2. Sort by index (report it, like the other repairs — NB-08)
+        if not y.index.is_monotonic_increasing:
+            changes_made["sorted"] = True
         y = y.sort_index()
         if X is not None:
             X = X.sort_index()

src/sktime_mcp/server.py

@@ -801,6 +801,7 @@ async def list_tools() -> list[Tool]:
                         "type": "integer",
                         "description": "Resolution in dots per inch (default: 150).",
                         "default": 150,
+                        "minimum": 1,
                     },
                     "markers": {
                         "description": "Marker style(s) for data points (e.g., 'o', ['.', 'x']).",

src/sktime_mcp/tools/inspect_data.py

@@ -87,13 +87,16 @@ def inspect_data_tool(data_handle: str) -> dict[str, Any]:
         if X is not None and isinstance(X, pd.DataFrame):
             columns = columns + [f"X:{c}" for c in X.columns]
 
-        # --- dtypes ---
+        # --- dtypes (include exogenous columns, which `columns` already lists — N-14) ---
         if isinstance(y, pd.DataFrame):
             dtypes = {str(col): str(dtype) for col, dtype in y.dtypes.items()}
         elif isinstance(y, pd.Series):
             dtypes = {y.name if y.name else "target": str(y.dtype)}
         else:
             dtypes = {}
+        if X is not None and isinstance(X, pd.DataFrame):
+            for col, dtype in X.dtypes.items():
+                dtypes[f"X:{col}"] = str(dtype)
 
         # --- index names ---
         if hasattr(y, "index") and hasattr(y.index, "names"):

src/sktime_mcp/tools/instantiate.py

@@ -56,10 +56,13 @@ def release_handle_tool(handle: str) -> dict[str, Any]:
     """
     handle_manager = get_handle_manager()
     released = handle_manager.release_handle(handle)
+    if released:
+        return {"success": True, "handle": handle, "message": "Handle released"}
+    # Failure carries an "error" key like every other tool (NB-02).
     return {
-        "success": released,
+        "success": False,
         "handle": handle,
-        "message": "Handle released" if released else handle_manager.describe_missing(handle),
+        "error": handle_manager.describe_missing(handle),
     }
 
 

src/sktime_mcp/tools/plotting.py

@@ -166,6 +166,14 @@ def plot_series_tool(
             "error": "data_handles must contain at least one data handle ID.",
         }
 
+    # dpi=0 previously slipped through (falsy -> default 150) while dpi=-100
+    # errored — reject all non-positive dpi consistently (N-20).
+    if dpi is not None and dpi <= 0:
+        return {
+            "success": False,
+            "error": f"dpi must be a positive integer, got {dpi}.",
+        }
+
     executor = get_executor()
 
     # --- resolve data handles -----------------------------------------------

src/sktime_mcp/tools/save_model.py

@@ -4,8 +4,8 @@
 Saves estimator instances via sktime's MLflow integration.
 """
 
-import os
 from collections.abc import Callable
+from pathlib import Path
 from typing import Any
 
 from sktime_mcp.runtime.handles import get_handle_manager
@@ -24,7 +24,7 @@ def resolve_model_path(path: str) -> str:
     """
     if path.startswith(_MLFLOW_URI_PREFIXES) or "://" in path:
         return path
-    return os.path.abspath(os.path.expanduser(path))
+    return str(Path(path).expanduser().resolve())
 
 
 def _get_mlflow_save_model() -> Callable[..., Any]:

src/sktime_mcp/tools/transform_data.py

@@ -132,6 +132,8 @@ def _action_format(
     changes_applied: list[str] = []
     changes = result.get("changes_made", {})
 
+    if changes.get("sorted"):
+        changes_applied.append("Sorted rows by time index")
     if changes.get("duplicates_removed", 0) > 0:
         changes_applied.append(f"Removed {changes['duplicates_removed']} duplicate timestamps")
     if changes.get("frequency_set"):

tests/test_nits_roundup.py

@@ -0,0 +1,104 @@
+"""Correctness/quality nits roundup (#541)."""
+
+import contextlib
+
+import pandas as pd
+import pytest
+
+from sktime_mcp.runtime.executor import get_executor
+from sktime_mcp.runtime.handles import get_handle_manager
+from sktime_mcp.tools.fit_predict import fit_tool, predict_tool
+from sktime_mcp.tools.inspect_data import inspect_data_tool
+from sktime_mcp.tools.instantiate import instantiate_tool, release_handle_tool
+from sktime_mcp.tools.plotting import plot_series_tool
+
+
+def _release(h):
+    with contextlib.suppress(KeyError):
+        get_handle_manager().release_handle(h)
+
+
+def test_release_handle_failure_has_error_key():
+    # NB-02: failure must carry an "error" key like other tools
+    res = release_handle_tool("est_never_existed")
+    assert res["success"] is False
+    assert "error" in res
+    assert "not found" in res["error"].lower()
+
+
+def test_classifier_predict_omits_horizon():
+    # N-01: horizon is meaningless for classifiers
+    h = instantiate_tool(spec="KNeighborsTimeSeriesClassifier()")["handle"]
+    try:
+        fit_tool(estimator_handle=h, X_dataset="arrow_head", y_dataset="arrow_head")
+        res = predict_tool(estimator_handle=h, X_dataset="arrow_head")
+        assert res["success"], res
+        assert "horizon" not in res
+    finally:
+        _release(h)
+
+
+def test_forecaster_predict_keeps_horizon():
+    h = instantiate_tool(spec="NaiveForecaster(sp=12)")["handle"]
+    try:
+        fit_tool(estimator_handle=h, y_dataset="airline")
+        res = predict_tool(estimator_handle=h, horizon=6)
+        assert res["horizon"] == 6
+    finally:
+        _release(h)
+
+
+def test_format_reports_sorting():
+    # NB-08: sorting is surfaced in changes
+    ex = get_executor()
+    ex._data_handles["nit_unsorted"] = {
+        "y": pd.Series(
+            [3.0, 1.0, 2.0],
+            index=pd.to_datetime(["2024-03-01", "2024-01-01", "2024-02-01"]),
+        ),
+        "X": None,
+        "metadata": {"frequency": None},
+        "validation": {},
+        "config": {},
+    }
+    try:
+        res = ex.format_data_handle("nit_unsorted", release_original=False)
+        assert res["success"]
+        assert res["changes_made"].get("sorted") is True
+    finally:
+        for h in list(ex._data_handles):
+            if h == "nit_unsorted" or h == res.get("data_handle"):
+                ex._data_handles.pop(h, None)
+
+
+def test_inspect_includes_exog_dtypes():
+    # N-14: dtypes must include exog columns that `columns` lists
+    ex = get_executor()
+    idx = pd.period_range("2020-01", periods=12, freq="M")
+    ex._data_handles["nit_exog"] = {
+        "y": pd.Series(range(12), index=idx, name="target", dtype=float),
+        "X": pd.DataFrame({"temp": range(12)}, index=idx),
+        "metadata": {},
+        "validation": {},
+        "config": {},
+    }
+    try:
+        res = inspect_data_tool(data_handle="nit_exog")
+        assert res["success"], res
+        assert "X:temp" in res["dtypes"]
+        assert "X:temp" in res["columns"]
+    finally:
+        ex._data_handles.pop("nit_exog", None)
+
+
+def test_plot_rejects_nonpositive_dpi():
+    # N-20: dpi=0 must be rejected, not silently defaulted
+    ex = get_executor()
+    idx = pd.period_range("2020-01", periods=12, freq="M")
+    ex._data_handles["nit_plot"] = {"y": pd.Series(range(12), index=idx)}
+    try:
+        res = plot_series_tool(data_handles=["nit_plot"], dpi=0)
+        assert not res["success"]
+        assert "dpi" in res["error"].lower()
+    finally:
+        ex._data_handles.pop("nit_plot", None)