From 1579b248155288b424c6470894401b574c60c872 Mon Sep 17 00:00:00 2001 From: Petr Date: Sat, 28 Feb 2026 16:49:57 +0100 Subject: [PATCH] Phase 6: Model validation & dead code cleanup - Remove silent defaults from TokenVerifyResponse required fields (token_id, token_description, project_name, owner_name now required) - Change project_id default from 0 to None in both TokenVerifyResponse and ProjectConfig to avoid masking parsing failures - Validate MCP tool name exists before calling (raises ConfigError) - Add deduplication to _extract_ids (preserves order, removes dupes) - Remove unnecessary McpService.__init__ override - Move inline imports to top-level in output.py (json, datetime, Panel) - Add tests for all changes (13 new tests) --- src/keboola_agent_cli/client.py | 2 +- src/keboola_agent_cli/models.py | 12 +- src/keboola_agent_cli/output.py | 9 +- src/keboola_agent_cli/services/mcp_service.py | 26 +++-- tests/test_client.py | 2 +- tests/test_mcp_service.py | 103 ++++++++++++++++++ tests/test_models.py | 86 ++++++++++++++- 7 files changed, 213 insertions(+), 27 deletions(-) diff --git a/src/keboola_agent_cli/client.py b/src/keboola_agent_cli/client.py index bcadc645..3f2710a8 100644 --- a/src/keboola_agent_cli/client.py +++ b/src/keboola_agent_cli/client.py @@ -105,7 +105,7 @@ def verify_token(self) -> TokenVerifyResponse: return TokenVerifyResponse( token_id=str(data.get("id", "")), token_description=data.get("description", ""), - project_id=data.get("owner", {}).get("id", 0), + project_id=data.get("owner", {}).get("id"), project_name=data.get("owner", {}).get("name", ""), owner_name=data.get("owner", {}).get("name", ""), ) diff --git a/src/keboola_agent_cli/models.py b/src/keboola_agent_cli/models.py index faf53beb..7b0f831d 100644 --- a/src/keboola_agent_cli/models.py +++ b/src/keboola_agent_cli/models.py @@ -13,7 +13,7 @@ class ProjectConfig(BaseModel): project_name: str = Field( default="", description="Human-readable project name (populated on add)" ) - project_id: int = Field(default=0, description="Keboola project ID (populated on add)") + project_id: int | None = Field(default=None, description="Keboola project ID (populated on add)") @field_validator("stack_url") @classmethod @@ -46,11 +46,11 @@ class AppConfig(BaseModel): class TokenVerifyResponse(BaseModel): """Response from the Keboola token verification endpoint.""" - token_id: str = Field(default="", description="Token identifier") - token_description: str = Field(default="", description="Human-readable token description") - project_id: int = Field(default=0, description="Keboola project numeric ID") - project_name: str = Field(default="", description="Keboola project name") - owner_name: str = Field(default="", description="Project owner name") + token_id: str = Field(description="Token identifier") + token_description: str = Field(description="Human-readable token description") + project_id: int | None = Field(default=None, description="Keboola project numeric ID") + project_name: str = Field(description="Keboola project name") + owner_name: str = Field(description="Project owner name") class ErrorResponse(BaseModel): diff --git a/src/keboola_agent_cli/output.py b/src/keboola_agent_cli/output.py index 1a63977e..7bc48433 100644 --- a/src/keboola_agent_cli/output.py +++ b/src/keboola_agent_cli/output.py @@ -3,6 +3,7 @@ import json import sys from collections.abc import Callable +from datetime import datetime from typing import Any from rich.console import Console @@ -192,9 +193,7 @@ def format_config_detail(console: Console, data: dict[str, Any]) -> None: # Show configuration parameters if present configuration = data.get("configuration", {}) if configuration: - import json as _json - - config_str = _json.dumps(configuration, indent=2) + config_str = json.dumps(configuration, indent=2) lines.append(f"\n[bold]Configuration:[/bold]\n{config_str}") # Show rows if present @@ -298,8 +297,6 @@ def _format_duration(job: dict[str, Any]) -> str: start = job.get("startTime") end = job.get("endTime") if start and end: - from datetime import datetime - try: start_dt = datetime.fromisoformat(start) end_dt = datetime.fromisoformat(end) @@ -699,6 +696,4 @@ def format_doctor_panel(console: Console, data: dict[str, Any]) -> None: lines.append("") lines.append(f" Summary: {', '.join(parts)}") - from rich.panel import Panel - console.print(Panel("\n".join(lines), title="kbagent doctor", expand=False)) diff --git a/src/keboola_agent_cli/services/mcp_service.py b/src/keboola_agent_cli/services/mcp_service.py index 97827694..a9f71b1b 100644 --- a/src/keboola_agent_cli/services/mcp_service.py +++ b/src/keboola_agent_cli/services/mcp_service.py @@ -302,9 +302,10 @@ async def _connect_and_auto_expand( def _extract_ids(content_items: list[Any], key: str) -> list[str]: - """Extract ID values from parsed MCP tool content. + """Extract unique ID values from parsed MCP tool content. Handles both list-of-dicts format and single-dict-with-list format. + Deduplicates while preserving insertion order. """ ids = [] for item in content_items: @@ -321,7 +322,7 @@ def _extract_ids(content_items: list[Any], key: str) -> list[str]: for sub in value: if isinstance(sub, dict) and key in sub: ids.append(str(sub[key])) - return ids + return list(dict.fromkeys(ids)) class McpService(BaseService): @@ -334,13 +335,6 @@ class McpService(BaseService): Uses the same DI pattern as JobService/ConfigService. """ - def __init__( - self, - config_store: ConfigStore, - client_factory: ClientFactory | None = None, - ) -> None: - super().__init__(config_store, client_factory) - def resolve_project(self, alias: str | None = None) -> tuple[str, ProjectConfig]: """Resolve a single project alias (or the default project). @@ -456,7 +450,7 @@ def validate_tool_input( """ schema = self.get_tool_schema(tool_name, aliases=aliases) if schema is None: - return [] # Can't validate - tool not found, let it fail at call time + return [] # Tool not found; call_tool will raise ConfigError required = schema.get("required", []) missing = [param for param in required if param not in tool_input] @@ -490,10 +484,22 @@ def call_tool( Returns: Dict with "results" list and "errors" list. + + Raises: + ConfigError: If tool_name is not found in the available tool list. """ if tool_input is None: tool_input = {} + # Validate tool name exists in the MCP tool list + tool_list_result = self.list_tools(aliases=[alias] if alias else None) + known_tools = {t["name"] for t in tool_list_result.get("tools", [])} + if known_tools and tool_name not in known_tools: + raise ConfigError( + f"Unknown MCP tool '{tool_name}'. " + f"Use 'kbagent tool list' to see available tools." + ) + is_write = _is_write_tool(tool_name) if is_write: diff --git a/tests/test_client.py b/tests/test_client.py index 0375a2b6..1059db97 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -579,7 +579,7 @@ def test_verify_token_minimal_response(self, httpx_mock) -> None: result = client.verify_token() assert result.token_id == "1" assert result.project_name == "" - assert result.project_id == 0 + assert result.project_id is None class TestLargeResponse: diff --git a/tests/test_mcp_service.py b/tests/test_mcp_service.py index b4b0099a..b2878118 100644 --- a/tests/test_mcp_service.py +++ b/tests/test_mcp_service.py @@ -10,6 +10,7 @@ from keboola_agent_cli.models import ProjectConfig from keboola_agent_cli.services.mcp_service import ( McpService, + _extract_ids, _is_write_tool, detect_mcp_server_command, ) @@ -741,3 +742,105 @@ def test_mcp_timeout_defaults(self) -> None: assert mcp_mod.MCP_INIT_TIMEOUT_SECONDS == DEFAULT_MCP_INIT_TIMEOUT finally: importlib.reload(mcp_mod) + + +# --------------------------------------------------------------------------- +# TestUnknownToolName +# --------------------------------------------------------------------------- + + +class TestUnknownToolName: + """Tests for Phase 6: Unknown MCP tool name validation.""" + + @patch("keboola_agent_cli.services.mcp_service.asyncio.run") + def test_unknown_tool_name_error( + self, mock_run: MagicMock, tmp_path: Path + ) -> None: + """Calling a nonexistent tool raises ConfigError with a clear message.""" + mock_run.return_value = _sample_tools() + + store = _setup_store( + tmp_path, + projects={"prod": {"token": "tok-prod"}}, + ) + svc = McpService(config_store=store) + + with pytest.raises(ConfigError, match="Unknown MCP tool 'nonexistent_tool'"): + svc.call_tool("nonexistent_tool", {}) + + @patch("keboola_agent_cli.services.mcp_service.asyncio.run") + def test_known_tool_name_passes_validation( + self, mock_run: MagicMock, tmp_path: Path + ) -> None: + """Calling a known tool does not raise ConfigError for tool name.""" + call_count = 0 + tools = _sample_tools() + + def side_effect(coro): + nonlocal call_count + call_count += 1 + if call_count == 1: + # First call is list_tools + return tools + # Second call is the actual tool call + return {"content": [{"data": "ok"}], "isError": False} + + mock_run.side_effect = side_effect + + store = _setup_store( + tmp_path, + projects={"prod": {"token": "tok-prod"}}, + default_project="prod", + ) + svc = McpService(config_store=store) + # create_config is in the sample tools and is a write tool + result = svc.call_tool("create_config", {"name": "test"}) + assert result["results"] or result["errors"] # No ConfigError raised + + +# --------------------------------------------------------------------------- +# TestExtractIdsDeduplication +# --------------------------------------------------------------------------- + + +class TestExtractIdsDeduplication: + """Tests for Phase 6: _extract_ids deduplication.""" + + def test_extract_ids_deduplication(self) -> None: + """_extract_ids removes duplicate IDs while preserving order.""" + content = [ + [ + {"id": "bucket-1"}, + {"id": "bucket-2"}, + {"id": "bucket-1"}, # duplicate + {"id": "bucket-3"}, + {"id": "bucket-2"}, # duplicate + ] + ] + result = _extract_ids(content, "id") + assert result == ["bucket-1", "bucket-2", "bucket-3"] + + def test_extract_ids_no_duplicates(self) -> None: + """_extract_ids works normally when there are no duplicates.""" + content = [ + [ + {"id": "a"}, + {"id": "b"}, + {"id": "c"}, + ] + ] + result = _extract_ids(content, "id") + assert result == ["a", "b", "c"] + + def test_extract_ids_empty_input(self) -> None: + """_extract_ids returns empty list for empty input.""" + result = _extract_ids([], "id") + assert result == [] + + def test_extract_ids_nested_dict_deduplication(self) -> None: + """_extract_ids deduplicates across nested dict formats.""" + content = [ + {"id": "x", "items": [{"id": "y"}, {"id": "x"}]}, + ] + result = _extract_ids(content, "id") + assert result == ["x", "y"] diff --git a/tests/test_models.py b/tests/test_models.py index 348aa403..772bd695 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -5,7 +5,13 @@ import pytest from pydantic import ValidationError -from keboola_agent_cli.models import AppConfig, ErrorResponse, ProjectConfig, SuccessResponse +from keboola_agent_cli.models import ( + AppConfig, + ErrorResponse, + ProjectConfig, + SuccessResponse, + TokenVerifyResponse, +) class TestProjectConfig: @@ -31,7 +37,7 @@ def test_default_values(self) -> None: token="901-token", ) assert config.project_name == "" - assert config.project_id == 0 + assert config.project_id is None def test_json_round_trip(self) -> None: """ProjectConfig can be serialized to JSON and deserialized back.""" @@ -255,6 +261,82 @@ def test_project_add_accepts_https_gcp(self) -> None: assert config.stack_url == "https://connection.europe-west3.gcp.keboola.com" +class TestTokenVerifyResponseValidation: + """Tests for Phase 6: TokenVerifyResponse required fields and project_id default.""" + + def test_token_verify_response_rejects_missing_fields(self) -> None: + """TokenVerifyResponse with missing required fields raises ValidationError.""" + with pytest.raises(ValidationError): + TokenVerifyResponse( + token_id="123", + token_description="My Token", + # project_name missing + # owner_name missing + ) + + def test_token_verify_response_rejects_missing_owner_name(self) -> None: + """TokenVerifyResponse with missing owner_name raises ValidationError.""" + with pytest.raises(ValidationError, match="owner_name"): + TokenVerifyResponse( + token_id="123", + token_description="My Token", + project_name="Test Project", + # owner_name missing + ) + + def test_token_verify_response_rejects_missing_token_id(self) -> None: + """TokenVerifyResponse with missing token_id raises ValidationError.""" + with pytest.raises(ValidationError, match="token_id"): + TokenVerifyResponse( + token_description="My Token", + project_name="Test Project", + owner_name="Test Owner", + ) + + def test_token_verify_response_rejects_missing_token_description(self) -> None: + """TokenVerifyResponse with missing token_description raises ValidationError.""" + with pytest.raises(ValidationError, match="token_description"): + TokenVerifyResponse( + token_id="123", + project_name="Test Project", + owner_name="Test Owner", + ) + + def test_token_verify_response_rejects_missing_project_name(self) -> None: + """TokenVerifyResponse with missing project_name raises ValidationError.""" + with pytest.raises(ValidationError, match="project_name"): + TokenVerifyResponse( + token_id="123", + token_description="My Token", + owner_name="Test Owner", + ) + + def test_project_id_default_none(self) -> None: + """TokenVerifyResponse project_id defaults to None, not 0.""" + response = TokenVerifyResponse( + token_id="123", + token_description="My Token", + project_name="Test Project", + owner_name="Test Owner", + ) + assert response.project_id is None + + def test_token_verify_response_with_all_fields(self) -> None: + """TokenVerifyResponse with all fields specified works correctly.""" + response = TokenVerifyResponse( + token_id="123", + token_description="My Token", + project_id=4567, + project_name="Test Project", + owner_name="Test Owner", + ) + assert response.token_id == "123" + assert response.token_description == "My Token" + assert response.project_id == 4567 + assert response.project_name == "Test Project" + assert response.owner_name == "Test Owner" + + class TestMaxParallelWorkersValidation: """Tests for max_parallel_workers upper bound validation."""