diff --git a/features/consolidated_tool.feature b/features/consolidated_tool.feature index 356850273..d13541ab6 100644 --- a/features/consolidated_tool.feature +++ b/features/consolidated_tool.feature @@ -587,6 +587,15 @@ Feature: Consolidated Tool When I attempt to attach validation via the coverage service Then a coverage NotFoundError should be raised with message containing "not found" + # --- attach_validation: type discriminator rejects plain tools ------------- + + + Scenario: attach_validation raises ValidationError when a plain tool is passed + Given a mock-based tool registry service + Given the mock tool repo returns a plain tool entry for get_by_name + When I attempt to attach validation via the coverage service + Then a coverage ValidationError should be raised with message containing "plain tool" + # --- attach_validation: successful delegation ------------------------------ diff --git a/features/steps/tool_registry_service_coverage_steps.py b/features/steps/tool_registry_service_coverage_steps.py index 70c64e7dc..4c7084072 100644 --- a/features/steps/tool_registry_service_coverage_steps.py +++ b/features/steps/tool_registry_service_coverage_steps.py @@ -14,7 +14,7 @@ from behave.runner import Context from cleveragents.application.services.tool_registry_service import ( ToolRegistryService, ) -from cleveragents.core.exceptions import DatabaseError, NotFoundError +from cleveragents.core.exceptions import DatabaseError, NotFoundError, ValidationError from cleveragents.infrastructure.database.repositories import ( DuplicateToolError, ToolInUseError, @@ -146,10 +146,20 @@ def step_repo_returns_none(context: Context) -> None: def step_repo_returns_sentinel(context: Context) -> None: context.cov_tool_repo._get_by_name_return = { "name": "local/some-check", + "tool_type": "validation", "mode": "required", } +@given("the mock tool repo returns a plain tool entry for get_by_name") +def step_repo_returns_plain_tool(context: Context) -> None: + context.cov_tool_repo._get_by_name_return = { + "name": "local/plain-tool", + "tool_type": "tool", + "source": "builtin", + } + + # --------------------------------------------------------------------------- # When # --------------------------------------------------------------------------- @@ -268,3 +278,16 @@ def step_then_no_error(context: Context) -> None: assert context.cov_error is None, ( f"Expected no error, got {type(context.cov_error).__name__}: {context.cov_error}" ) + + +@then('a coverage ValidationError should be raised with message containing "{text}"') +def step_then_validation_error(context: Context, text: str) -> None: + assert context.cov_error is not None, ( + "Expected ValidationError but no error was raised" + ) + assert isinstance(context.cov_error, ValidationError), ( + f"Expected ValidationError, got {type(context.cov_error).__name__}: {context.cov_error}" + ) + assert text in str(context.cov_error), ( + f"Expected '{text}' in error message, got '{context.cov_error}'" + ) diff --git a/robot/helper_tool_cli.py b/robot/helper_tool_cli.py index aa266f49e..2134ff58f 100644 --- a/robot/helper_tool_cli.py +++ b/robot/helper_tool_cli.py @@ -193,6 +193,35 @@ def validation_detach() -> None: print("validation-cli-detach-ok") +def validation_attach_plain_tool() -> None: + """Verify validation attach rejects a plain tool with a non-zero exit code.""" + from cleveragents.core.exceptions import ValidationError + + svc = MagicMock() + svc.attach_validation.side_effect = ValidationError( + "'local/plain-tool' is a plain tool, not a validation — " + "only entries with tool_type='validation' may be attached as validations." + ) + with patch( + "cleveragents.cli.commands.validation._get_tool_registry_service", + return_value=svc, + ): + result = runner.invoke( + validation_app, + ["attach", "resource/r1", "local/plain-tool"], + ) + # typer.Abort() results in exit code 1 + assert result.exit_code != 0, ( + f"Expected non-zero exit code for plain tool, " + f"got {result.exit_code}: {result.output}" + ) + output_lower = result.output.lower() + assert "plain tool" in output_lower or "validation error" in output_lower, ( + f"Expected error message about plain tool, got: {result.output}" + ) + print("validation-cli-attach-plain-tool-rejected") + + _COMMANDS = { "tool-add-config": tool_add_config, "tool-list": tool_list_all, @@ -200,6 +229,7 @@ _COMMANDS = { "tool-remove": tool_remove_name, "validation-add-config": validation_add_config, "validation-attach": validation_attach, + "validation-attach-plain-tool": validation_attach_plain_tool, "validation-detach": validation_detach, } diff --git a/robot/tool_cli.robot b/robot/tool_cli.robot index cca076684..b0a103b0d 100644 --- a/robot/tool_cli.robot +++ b/robot/tool_cli.robot @@ -55,3 +55,9 @@ Validation Detach Removes Attachment ${result}= Run Process ${PYTHON} ${HELPER} validation-detach cwd=${WORKSPACE} Should Be Equal As Integers ${result.rc} 0 Should Contain ${result.stdout} validation-cli-detach-ok + +Validation Attach Rejects Plain Tool + [Documentation] Verify that ``validation attach`` rejects a plain tool name with a non-zero exit code and descriptive error message + ${result}= Run Process ${PYTHON} ${HELPER} validation-attach-plain-tool cwd=${WORKSPACE} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} validation-cli-attach-plain-tool-rejected diff --git a/src/cleveragents/application/services/tool_registry_service.py b/src/cleveragents/application/services/tool_registry_service.py index c52fa95ac..8a2cf8c67 100644 --- a/src/cleveragents/application/services/tool_registry_service.py +++ b/src/cleveragents/application/services/tool_registry_service.py @@ -9,7 +9,7 @@ from __future__ import annotations from typing import Any -from cleveragents.core.exceptions import NotFoundError +from cleveragents.core.exceptions import NotFoundError, ValidationError from cleveragents.domain.models.core.tool import Tool from cleveragents.infrastructure.database.repositories import ( ToolRegistryRepository, @@ -153,6 +153,12 @@ class ToolRegistryService: the validation's registered definition — it is not an attach-time override. + The ``tool_type`` discriminator stored in the Tool Registry is enforced + here: only entries tagged as ``"validation"`` may be attached. Passing + the name of a plain tool (``tool_type="tool"``) raises a + :class:`~cleveragents.core.exceptions.ValidationError` so that the + type-safety guarantee described in the specification is upheld. + Args: validation_name: Name of the validation tool to attach. resource_id: Resource reference string. @@ -165,6 +171,8 @@ class ToolRegistryService: Raises: NotFoundError: If the validation tool does not exist. + ValidationError: If the named entry is a plain tool rather than a + validation (``tool_type != "validation"``). DatabaseError: On persistence failure. """ # Verify validation exists @@ -175,6 +183,18 @@ class ToolRegistryService: resource_id=validation_name, ) + # Enforce the type discriminator: only "validation" entries are allowed. + if isinstance(existing, dict): + tool_type = existing.get("tool_type") + else: + tool_type = getattr(existing, "tool_type", None) + if tool_type != "validation": + raise ValidationError( + f"'{validation_name}' is a plain tool, not a validation — " + "only entries with tool_type='validation' may be attached as " + "validations. Register a proper validation schema first." + ) + # Read mode from the validation's registered definition if isinstance(existing, dict): mode = existing.get("mode", "required")