mcp: validate tool inputs

kcidev/libs/dashboard.py

@@ -400,12 +400,19 @@ def dashboard_fetch_issue_list(origin, days, use_json):
     return dashboard_api_fetch("issue/", params, use_json)
 
 
+def _require_issue_id(issue_id):
+    if not issue_id or not issue_id.strip():
+        raise click.ClickException("Issue id is required")
+
+
 def dashboard_fetch_issue(issue_id, use_json):
+    _require_issue_id(issue_id)
     logging.info(f"Fetching issue details for issue ID: {issue_id}")
     return dashboard_api_fetch(f"issue/{issue_id}", {}, use_json)
 
 
 def dashboard_fetch_issue_builds(origin, issue_id, use_json, error_verbose=True):
+    _require_issue_id(issue_id)
     logging.info(f"Fetching builds for issue ID: {issue_id}")
     params = {"filter_origin": origin} if origin else {}
     return dashboard_api_fetch(
@@ -414,6 +421,7 @@ def dashboard_fetch_issue_builds(origin, issue_id, use_json, error_verbose=True)
 
 
 def dashboard_fetch_issue_tests(origin, issue_id, use_json, error_verbose=True):
+    _require_issue_id(issue_id)
     logging.info(f"Fetching tests for issue ID: {issue_id}")
     params = {"filter_origin": origin} if origin else {}
     return dashboard_api_fetch(

kcidev/mcp/tools_dashboard.py

@@ -7,6 +7,7 @@
 from kcidev.api import KciDevError, KernelCIClient
 from kcidev.libs.filters import StatusFilter
 from kcidev.mcp.errors import tool_errors
+from kcidev.mcp.validation import check_page_args, check_page_bounds, checked_status
 
 _active_client = ContextVar("dashboard_tool_client", default=None)
 
@@ -16,10 +17,11 @@ def _current_client():
 
 
 def _page(data, key, status, limit, offset, fields=None):
+    check_page_bounds(limit, offset)
     items = data[key] if isinstance(data, dict) else data
     total = len(items)
     if status:
-        status_filter = StatusFilter(status)
+        status_filter = StatusFilter(checked_status(status))
         items = [item for item in items if status_filter.matches(item)]
     page = items[offset : offset + limit]
     if fields:
@@ -142,12 +144,13 @@ def list_builds(
     """List kernel builds for one commit of a tree.
 
     Optional filters: arch (e.g. 'arm64'), tree name, ISO date range, and
-    status ('pass', 'fail' or 'inconclusive'). Results are paginated with
+    status ('pass', 'fail', 'inconclusive' or 'all'). Results are paginated with
     limit/offset; the response carries 'total' (before status filtering)
     and 'matched' counts so you know whether to fetch further pages;
     fields projects each entry to only those keys.
     Returns build entries with ids usable with get_build.
     """
+    check_page_args(status, limit, offset)
     data = _current_client().get_builds(
         origin, giturl, branch, commit, arch, tree, start_date, end_date
     )
@@ -173,12 +176,13 @@ def list_boots(
     """List boot test results for one commit of a tree.
 
     Optional filters: arch, tree name, ISO date range, boot origin, and
-    status ('pass', 'fail' or 'inconclusive'). Results are paginated with
+    status ('pass', 'fail', 'inconclusive' or 'all'). Results are paginated with
     limit/offset; the response carries 'total' (before status filtering)
     and 'matched' counts so you know whether to fetch further pages;
     fields projects each entry to only those keys.
     Returns boot entries with ids usable with get_test.
     """
+    check_page_args(status, limit, offset)
     data = _current_client().get_boots(
         origin, giturl, branch, commit, arch, tree, start_date, end_date, boot_origin
     )
@@ -203,13 +207,14 @@ def list_tests(
     """List test results for one commit of a tree.
 
     Optional filters: arch, tree name, ISO date range, and status ('pass',
-    'fail' or 'inconclusive'). A full commit can carry tens of thousands
+    'fail', 'inconclusive' or 'all'). A full commit can carry tens of thousands
     of tests, so filter by status and paginate with limit/offset; the
     response carries 'total' (before status filtering) and 'matched'
     counts so you know whether to fetch further pages; fields projects
     each entry to only those keys.
     Returns test entries with ids usable with get_test.
     """
+    check_page_args(status, limit, offset)
     data = _current_client().get_tests(
         origin, giturl, branch, commit, arch, tree, start_date, end_date
     )
@@ -327,10 +332,11 @@ def get_issue_builds(
     An empty list means the issue has no builds recorded against it, and
     also what an unknown issue id returns, since the dashboard reports
     both the same way; confirm the id with get_issue if it matters.
-    Optional status filter ('pass', 'fail' or 'inconclusive') and
+    Optional status filter ('pass', 'fail', 'inconclusive' or 'all') and
     limit/offset pagination; the response carries 'total' and 'matched'
     counts; fields projects each entry to only those keys.
     """
+    check_page_args(status, limit, offset)
     data = _current_client().get_issue_builds(issue_id, origin)
     return _page(data, "builds", status, limit, offset, fields)
 
@@ -349,10 +355,11 @@ def get_issue_tests(
     An empty list means the issue has no tests recorded against it, and
     also what an unknown issue id returns, since the dashboard reports
     both the same way; confirm the id with get_issue if it matters.
-    Optional status filter ('pass', 'fail' or 'inconclusive') and
+    Optional status filter ('pass', 'fail', 'inconclusive' or 'all') and
     limit/offset pagination; the response carries 'total' and 'matched'
     counts; fields projects each entry to only those keys.
     """
+    check_page_args(status, limit, offset)
     data = _current_client().get_issue_tests(issue_id, origin)
     return _page(data, "tests", status, limit, offset, fields)
 

kcidev/mcp/tools_maestro.py

@@ -2,6 +2,7 @@
 # -*- coding: utf-8 -*-
 
 from kcidev.mcp.errors import tool_errors
+from kcidev.mcp.validation import check_page_bounds, checked_filters
 
 
 def register_tools(server, client, api_url, pipeline_url, token):
@@ -44,7 +45,10 @@ def list_nodes(
             paginate within the window; full nodes are large, so use
             fields to project each node to only those keys.
             """
-            nodes = client.get_nodes(limit=limit, offset=offset, filters=filters or [])
+            check_page_bounds(limit, offset)
+            nodes = client.get_nodes(
+                limit=limit, offset=offset, filters=checked_filters(filters)
+            )
             if fields:
                 return [{k: n[k] for k in fields if k in n} for n in nodes]
             return nodes

kcidev/mcp/validation.py

@@ -0,0 +1,45 @@
+#!/usr/bin/env python3
+# -*- coding: utf-8 -*-
+
+from kcidev.api import KciDevError
+
+STATUS_CHOICES = ("all", "pass", "fail", "inconclusive")
+
+
+def checked_status(status):
+    normalised = status.strip().lower()
+    if normalised not in STATUS_CHOICES:
+        raise KciDevError(
+            f"Unknown status {status!r}: expected one of {', '.join(STATUS_CHOICES)}"
+        )
+    return normalised
+
+
+def check_page_bounds(limit, offset):
+    if limit < 0:
+        raise KciDevError(f"Invalid limit {limit}: must be zero or greater")
+    if offset < 0:
+        raise KciDevError(f"Invalid offset {offset}: must be zero or greater")
+
+
+def checked_filters(filters):
+    for entry in filters or []:
+        if "=" not in entry:
+            raise KciDevError(
+                f"Invalid filter {entry!r}: expected 'field=value', "
+                "for example 'state=done'"
+            )
+    return list(filters or [])
+
+
+def check_page_args(status, limit, offset):
+    """Validate paging arguments before any request is made.
+
+    The tools fetch a whole result set and page it in memory, so
+    validating inside the pager would mean an expensive request for
+    input that was never usable, and a request failure would mask the
+    real complaint.
+    """
+    check_page_bounds(limit, offset)
+    if status:
+        checked_status(status)

tests/test_dashboard.py

@@ -198,3 +198,50 @@ def test_other_dashboard_errors_are_passed_through_unchanged(monkeypatch):
         dashboard.dashboard_api_fetch("build/x", {}, False)
 
     assert excinfo.value.message == "Build not found"
+
+
+def _issue_collection_get(monkeypatch):
+    response = Mock(status_code=200)
+    response.json.return_value = {"issues": [{"id": "maestro:one"}]}
+    get = Mock(return_value=response)
+    monkeypatch.setattr(dashboard.kcidev_session, "get", get)
+    return get
+
+
+def test_dashboard_fetch_issue_rejects_empty_id(monkeypatch):
+    get = _issue_collection_get(monkeypatch)
+
+    with pytest.raises(click.ClickException):
+        dashboard.dashboard_fetch_issue("", False)
+
+    get.assert_not_called()
+
+
+def test_dashboard_fetch_issue_builds_rejects_empty_id(monkeypatch):
+    get = _issue_collection_get(monkeypatch)
+
+    with pytest.raises(click.ClickException):
+        dashboard.dashboard_fetch_issue_builds(None, "", False)
+
+    get.assert_not_called()
+
+
+def test_dashboard_fetch_issue_tests_rejects_empty_id(monkeypatch):
+    get = _issue_collection_get(monkeypatch)
+
+    with pytest.raises(click.ClickException):
+        dashboard.dashboard_fetch_issue_tests(None, "", False)
+
+    get.assert_not_called()
+
+
+def test_cli_issue_command_rejects_empty_id(monkeypatch):
+    from kcidev.main import get_cli
+
+    get = _issue_collection_get(monkeypatch)
+
+    runner = CliRunner()
+    result = runner.invoke(get_cli(), ["results", "issue", "--id", ""])
+
+    assert result.exit_code != 0
+    get.assert_not_called()

tests/test_mcp_server.py

@@ -235,3 +235,35 @@ def test_list_nodes_http_error_keeps_api_detail(monkeypatch):
     result = _call_tool(create_server(CFG, "test"), "list_nodes", {})
     assert result.isError is True
     assert "422" in result.content[0].text
+
+
+def _no_http(monkeypatch):
+    from kcidev.libs import maestro_common
+
+    get = Mock()
+    monkeypatch.setattr(maestro_common.kcidev_session, "get", get)
+    return get
+
+
+def test_list_nodes_rejects_negative_limit(monkeypatch):
+    get = _no_http(monkeypatch)
+    result = _call_tool(create_server(CFG, "test"), "list_nodes", {"limit": -1})
+    assert result.isError is True
+    get.assert_not_called()
+
+
+def test_list_nodes_rejects_negative_offset(monkeypatch):
+    get = _no_http(monkeypatch)
+    result = _call_tool(create_server(CFG, "test"), "list_nodes", {"offset": -1})
+    assert result.isError is True
+    get.assert_not_called()
+
+
+def test_list_nodes_rejects_filter_without_equals(monkeypatch):
+    get = _no_http(monkeypatch)
+    result = _call_tool(
+        create_server(CFG, "test"), "list_nodes", {"filters": ["state done"]}
+    )
+    assert result.isError is True
+    assert "state done" in result.content[0].text
+    get.assert_not_called()

tests/test_mcp_tools_dashboard.py

@@ -282,3 +282,77 @@ def test_get_issue_tests_still_reports_other_errors(monkeypatch):
     _mock_get(monkeypatch, {"error": "Issue not found"})
     with pytest.raises(ToolExecutionError):
         tools_dashboard.get_issue_tests("maestro:nope")
+
+
+def _tree_args(**extra):
+    args = {
+        "giturl": "https://git.example.org/linux.git",
+        "branch": "master",
+        "commit": "deadbeef",
+    }
+    args.update(extra)
+    return args
+
+
+def test_list_tests_accepts_uppercase_status(monkeypatch):
+    _mock_get(
+        monkeypatch,
+        {"tests": [{"id": "p1", "status": "PASS"}, {"id": "f1", "status": "FAIL"}]},
+    )
+    result = tools_dashboard.list_tests(**_tree_args(status="FAIL"))
+    assert result["matched"] == 1
+    assert result["tests"] == [{"id": "f1", "status": "FAIL"}]
+
+
+def test_list_tests_rejects_unknown_status(monkeypatch):
+    _mock_get(monkeypatch, {"tests": [{"id": "f1", "status": "FAIL"}]})
+    with pytest.raises(ToolExecutionError) as excinfo:
+        tools_dashboard.list_tests(**_tree_args(status="borked"))
+    assert "borked" in str(excinfo.value)
+
+
+def test_list_tests_rejects_negative_limit(monkeypatch):
+    _mock_get(monkeypatch, {"tests": [{"id": str(i)} for i in range(5)]})
+    with pytest.raises(ToolExecutionError):
+        tools_dashboard.list_tests(**_tree_args(limit=-1))
+
+
+def test_list_tests_rejects_negative_offset(monkeypatch):
+    _mock_get(monkeypatch, {"tests": [{"id": str(i)} for i in range(5)]})
+    with pytest.raises(ToolExecutionError):
+        tools_dashboard.list_tests(**_tree_args(offset=-1))
+
+
+def test_get_issue_rejects_empty_id(monkeypatch):
+    get = _mock_get(monkeypatch, {"issues": [{"id": "maestro:one"}]})
+    with pytest.raises(ToolExecutionError):
+        tools_dashboard.get_issue("")
+    get.assert_not_called()
+
+
+def test_invalid_status_is_rejected_before_any_request(monkeypatch):
+    get = _mock_get(monkeypatch, {"tests": []})
+    with pytest.raises(ToolExecutionError):
+        tools_dashboard.list_tests(**_tree_args(status="borked"))
+    get.assert_not_called()
+
+
+def test_invalid_limit_is_rejected_before_any_request(monkeypatch):
+    get = _mock_get(monkeypatch, {"tests": []})
+    with pytest.raises(ToolExecutionError):
+        tools_dashboard.list_tests(**_tree_args(limit=-1))
+    get.assert_not_called()
+
+
+def test_invalid_offset_is_rejected_before_any_request(monkeypatch):
+    get = _mock_get(monkeypatch, {"builds": []})
+    with pytest.raises(ToolExecutionError):
+        tools_dashboard.list_builds(**_tree_args(offset=-1))
+    get.assert_not_called()
+
+
+def test_issue_tools_validate_before_any_request(monkeypatch):
+    get = _mock_get(monkeypatch, {"tests": []})
+    with pytest.raises(ToolExecutionError):
+        tools_dashboard.get_issue_tests("maestro:i1", status="borked")
+    get.assert_not_called()