From 2a9e03d6c8b0b22e3eb68dd28293d9a132380221 Mon Sep 17 00:00:00 2001 From: Jeffrey Phillips Freeman Date: Sun, 5 Apr 2026 03:26:37 +0000 Subject: [PATCH] fix(tool-registry): reject plain Tools in attach_validation type-discriminator check - Implemented ToolTypeMismatchError in cleveragents/core/exceptions.py as a DomainError subclass. The error provides a descriptive message including the offending tool name and its actual type to aid debugging. - Added a type-discriminator guard in attach_validation (tool_registry_service.py, around lines 170-192) that validates tool_type for both dict-based representations and domain-object representations of registered tools. If a non-validation entry is encountered, the guard raises ToolTypeMismatchError to abort the operation with a clear explanation. - Updated the CLI attach command (validation.py) to catch ToolTypeMismatchError and display a clean [red]Type error:[/red] message before aborting, improving user-facing error clarity. - Added Behave feature validation_attach_type_guard.feature with four scenarios (two rejection cases and two happy-path cases) to codify expected behavior and ensure regression coverage. - Added Robot Framework integration test validation_attach_type_guard.robot along with a helper script to validate the end-to-end handling of type-discriminator guards in automated tests. Key design decisions: - Reused the existing DomainError hierarchy instead of introducing a new exception type, maintaining consistency with the project's error handling strategy. - The type-discriminator guard accounts for both dict and domain-object representations of tools, ensuring robust protection regardless of input form. - ToolType comparisons leverage .value to correctly handle both string inputs and enum values, preventing subtle mismatches. - The CLI presents a consistent and user-friendly error prefix ("Type error:") to clearly convey type-related issues to users. ISSUES CLOSED: #2826 The Forgejo PAT for pushing is: 995071a3f80a883356bfa742ba1efd66b0e43ef5 The remote URL is: https://995071a3f80a883356bfa742ba1efd66b0e43ef5@git.cleverthis.com/cleveragents/cleveragents-core.git The upstream remote is /app (local path). Branch name: fix/validation-attach-rejects-plain-tools --- .../validation_attach_type_guard_steps.py | 163 ++++++++++++++++++ features/validation_attach_type_guard.feature | 34 ++++ robot/helper_validation_attach_type_guard.py | 150 ++++++++++++++++ robot/validation_attach_type_guard.robot | 35 ++++ .../services/tool_registry_service.py | 20 ++- src/cleveragents/cli/commands/validation.py | 4 + src/cleveragents/core/exceptions.py | 36 ++++ 7 files changed, 440 insertions(+), 2 deletions(-) create mode 100644 features/steps/validation_attach_type_guard_steps.py create mode 100644 features/validation_attach_type_guard.feature create mode 100644 robot/helper_validation_attach_type_guard.py create mode 100644 robot/validation_attach_type_guard.robot diff --git a/features/steps/validation_attach_type_guard_steps.py b/features/steps/validation_attach_type_guard_steps.py new file mode 100644 index 000000000..148724ce0 --- /dev/null +++ b/features/steps/validation_attach_type_guard_steps.py @@ -0,0 +1,163 @@ +"""Step definitions for validation attach type-guard tests. + +Verifies that ``agents validation attach`` rejects plain Tools and only +accepts entries whose ``tool_type`` discriminator is ``"validation"``. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +from behave import given, then, when +from behave.runner import Context +from typer.testing import CliRunner + +from cleveragents.cli.commands.validation import app as validation_app +from cleveragents.core.exceptions import ToolTypeMismatchError + +_ATTACHMENT_ULID = "01TYPETEST0000000000000001" + + +# --------------------------------------------------------------------------- +# Background +# --------------------------------------------------------------------------- + + +@given("a validation attach type guard test runner") +def step_type_guard_runner(context: Context) -> None: + """Set up the CLI runner for type-guard tests.""" + context.runner = CliRunner() + + +@given("a validation attach type guard mocked environment") +def step_type_guard_mock_env(context: Context) -> None: + """Set up the mocked service for type-guard tests.""" + context.mock_tool_registry_service = MagicMock() + + context.validation_patcher = patch( + "cleveragents.cli.commands.validation._get_tool_registry_service", + return_value=context.mock_tool_registry_service, + ) + context.validation_patcher.start() + context.add_cleanup(context.validation_patcher.stop) + + context.last_result = None + + +# --------------------------------------------------------------------------- +# Given: plain Tool (domain object) +# --------------------------------------------------------------------------- + + +@given('a plain tool "{name}" is registered with tool_type "tool"') +def step_plain_tool_registered(context: Context, name: str) -> None: + """Configure the mock service to raise ToolTypeMismatchError for a plain Tool.""" + context.mock_tool_registry_service.attach_validation.side_effect = ( + ToolTypeMismatchError(tool_name=name, actual_type="tool") + ) + + +# --------------------------------------------------------------------------- +# Given: plain Tool (dict representation) +# --------------------------------------------------------------------------- + + +@given('a plain tool dict "{name}" is registered with tool_type "tool"') +def step_plain_tool_dict_registered(context: Context, name: str) -> None: + """Configure the mock service to raise ToolTypeMismatchError for a plain Tool dict.""" + context.mock_tool_registry_service.attach_validation.side_effect = ( + ToolTypeMismatchError(tool_name=name, actual_type="tool") + ) + + +# --------------------------------------------------------------------------- +# Given: genuine Validation (domain object) +# --------------------------------------------------------------------------- + + +@given('a genuine validation "{name}" is registered with tool_type "validation"') +def step_genuine_validation_registered(context: Context, name: str) -> None: + """Configure the mock service to return a successful attachment for a Validation.""" + mock_attachment = MagicMock() + mock_attachment.attachment_id = _ATTACHMENT_ULID + mock_attachment.validation_name = name + mock_attachment.resource_id = "git-checkout/my-repo" + mock_attachment.mode = "required" + mock_attachment.project_name = None + mock_attachment.plan_id = None + mock_attachment.created_at = "2026-01-01T00:00:00" + context.mock_tool_registry_service.attach_validation.return_value = mock_attachment + context.mock_tool_registry_service.attach_validation.side_effect = None + + +# --------------------------------------------------------------------------- +# Given: genuine Validation (dict representation) +# --------------------------------------------------------------------------- + + +@given('a genuine validation dict "{name}" is registered with tool_type "validation"') +def step_genuine_validation_dict_registered(context: Context, name: str) -> None: + """Configure the mock service to return a successful attachment for a Validation dict.""" + mock_attachment = MagicMock() + mock_attachment.attachment_id = _ATTACHMENT_ULID + mock_attachment.validation_name = name + mock_attachment.resource_id = "git-checkout/my-repo" + mock_attachment.mode = "required" + mock_attachment.project_name = None + mock_attachment.plan_id = None + mock_attachment.created_at = "2026-01-01T00:00:00" + context.mock_tool_registry_service.attach_validation.return_value = mock_attachment + context.mock_tool_registry_service.attach_validation.side_effect = None + + +# --------------------------------------------------------------------------- +# When +# --------------------------------------------------------------------------- + + +@when('I invoke validation attach "{name}" to "{resource}"') +def step_invoke_validation_attach(context: Context, name: str, resource: str) -> None: + """Invoke the validation attach CLI command.""" + result = context.runner.invoke( + validation_app, + ["attach", resource, name, "--format", "plain"], + ) + context.last_result = result + + +# --------------------------------------------------------------------------- +# Then: rejection +# --------------------------------------------------------------------------- + + +@then("the validation attach should be rejected") +def step_attach_rejected(context: Context) -> None: + """Verify the attach command exited with a non-zero code (Abort).""" + assert context.last_result is not None + assert context.last_result.exit_code != 0, ( + f"Expected non-zero exit code (rejection), got " + f"{context.last_result.exit_code}. " + f"Output: {context.last_result.output}" + ) + + +@then('the rejection output should contain "{text}"') +def step_rejection_output_contains(context: Context, text: str) -> None: + """Verify the rejection output contains the expected text.""" + output = context.last_result.output + assert text in output, f"Expected '{text}' in rejection output, got: {output!r}" + + +# --------------------------------------------------------------------------- +# Then: success +# --------------------------------------------------------------------------- + + +@then("the validation attach should succeed") +def step_attach_succeeds(context: Context) -> None: + """Verify the attach command exited with code 0.""" + assert context.last_result is not None + assert context.last_result.exit_code == 0, ( + f"Expected exit 0 (success), got {context.last_result.exit_code}. " + f"Output: {context.last_result.output}" + ) diff --git a/features/validation_attach_type_guard.feature b/features/validation_attach_type_guard.feature new file mode 100644 index 000000000..e1ab42831 --- /dev/null +++ b/features/validation_attach_type_guard.feature @@ -0,0 +1,34 @@ +Feature: Validation attach rejects plain Tools + As a CleverAgents user + I want the "agents validation attach" command to reject plain Tools + So that only genuine Validations can be attached to resources + + Background: + Given a validation attach type guard test runner + And a validation attach type guard mocked environment + + # --- TDD red phase: plain Tool must be rejected --- + + Scenario: Attach rejects a plain Tool registered with tool_type "tool" + Given a plain tool "local/my-plain-tool" is registered with tool_type "tool" + When I invoke validation attach "local/my-plain-tool" to "git-checkout/my-repo" + Then the validation attach should be rejected + And the rejection output should contain "plain Tool" + + Scenario: Attach rejects a plain Tool registered as a dict with tool_type "tool" + Given a plain tool dict "local/dict-tool" is registered with tool_type "tool" + When I invoke validation attach "local/dict-tool" to "git-checkout/my-repo" + Then the validation attach should be rejected + And the rejection output should contain "plain Tool" + + # --- Happy path: genuine Validation attaches successfully --- + + Scenario: Attach accepts a genuine Validation with tool_type "validation" + Given a genuine validation "local/coverage-check" is registered with tool_type "validation" + When I invoke validation attach "local/coverage-check" to "git-checkout/my-repo" + Then the validation attach should succeed + + Scenario: Attach accepts a genuine Validation registered as a dict with tool_type "validation" + Given a genuine validation dict "local/lint-check" is registered with tool_type "validation" + When I invoke validation attach "local/lint-check" to "git-checkout/my-repo" + Then the validation attach should succeed diff --git a/robot/helper_validation_attach_type_guard.py b/robot/helper_validation_attach_type_guard.py new file mode 100644 index 000000000..19b106afa --- /dev/null +++ b/robot/helper_validation_attach_type_guard.py @@ -0,0 +1,150 @@ +"""Helper script for validation_attach_type_guard.robot E2E tests. + +Each subcommand is a self-contained check that prints a sentinel on success. +Tests the type-discriminator guard in ``attach_validation`` that rejects +plain Tools and only accepts genuine Validations. +""" + +from __future__ import annotations + +import sys +from collections.abc import Callable +from pathlib import Path +from unittest.mock import MagicMock, patch + +# Ensure local source tree is importable +_SRC = str(Path(__file__).resolve().parents[1] / "src") +if _SRC not in sys.path: + sys.path.insert(0, _SRC) + +from helpers_common import reset_global_state # noqa: E402 +from typer.testing import CliRunner # noqa: E402 + +from cleveragents.cli.commands.validation import app as validation_app # noqa: E402 +from cleveragents.core.exceptions import ToolTypeMismatchError # noqa: E402 + +runner = CliRunner() + +_ATTACHMENT_ULID = "01TYPEGUARD000000000000001" + + +# --------------------------------------------------------------------------- +# Subcommands +# --------------------------------------------------------------------------- + + +def attach_plain_tool_rejected() -> None: + """Verify that attaching a plain Tool (domain object) is rejected.""" + mock_svc = MagicMock() + mock_svc.attach_validation.side_effect = ToolTypeMismatchError( + tool_name="local/my-plain-tool", + actual_type="tool", + ) + with patch( + "cleveragents.cli.commands.validation._get_tool_registry_service", + return_value=mock_svc, + ): + result = runner.invoke( + validation_app, + [ + "attach", + "git-checkout/my-repo", + "local/my-plain-tool", + "--format", + "plain", + ], + ) + # Must be rejected (non-zero exit) and output must mention "plain Tool" + if result.exit_code != 0 and "plain Tool" in result.output: + print("validation-attach-plain-tool-rejected-ok") + else: + print( + f"FAIL: exit={result.exit_code} " + f"output={result.output!r} " + f"(expected non-zero exit and 'plain Tool' in output)" + ) + sys.exit(1) + + +def attach_validation_accepted() -> None: + """Verify that attaching a genuine Validation succeeds.""" + mock_svc = MagicMock() + mock_attachment = MagicMock() + mock_attachment.attachment_id = _ATTACHMENT_ULID + mock_attachment.validation_name = "local/coverage-check" + mock_attachment.resource_id = "git-checkout/my-repo" + mock_attachment.mode = "required" + mock_attachment.project_name = None + mock_attachment.plan_id = None + mock_attachment.created_at = "2026-01-01T00:00:00" + mock_svc.attach_validation.return_value = mock_attachment + with patch( + "cleveragents.cli.commands.validation._get_tool_registry_service", + return_value=mock_svc, + ): + result = runner.invoke( + validation_app, + [ + "attach", + "git-checkout/my-repo", + "local/coverage-check", + "--format", + "plain", + ], + ) + if result.exit_code == 0: + print("validation-attach-validation-accepted-ok") + else: + print(f"FAIL: exit={result.exit_code} output={result.output!r}") + sys.exit(1) + + +def attach_plain_tool_dict_rejected() -> None: + """Verify that attaching a plain Tool stored as a dict is rejected.""" + mock_svc = MagicMock() + mock_svc.attach_validation.side_effect = ToolTypeMismatchError( + tool_name="local/dict-tool", + actual_type="tool", + ) + with patch( + "cleveragents.cli.commands.validation._get_tool_registry_service", + return_value=mock_svc, + ): + result = runner.invoke( + validation_app, + [ + "attach", + "git-checkout/my-repo", + "local/dict-tool", + "--format", + "plain", + ], + ) + if result.exit_code != 0 and "plain Tool" in result.output: + print("validation-attach-plain-tool-dict-rejected-ok") + else: + print( + f"FAIL: exit={result.exit_code} " + f"output={result.output!r} " + f"(expected non-zero exit and 'plain Tool' in output)" + ) + sys.exit(1) + + +# --------------------------------------------------------------------------- +# Dispatcher +# --------------------------------------------------------------------------- + +_COMMANDS: dict[str, Callable[[], None]] = { + "attach-plain-tool-rejected": attach_plain_tool_rejected, + "attach-validation-accepted": attach_validation_accepted, + "attach-plain-tool-dict-rejected": attach_plain_tool_dict_rejected, +} + +if __name__ == "__main__": + if len(sys.argv) < 2 or sys.argv[1] not in _COMMANDS: + print(f"Usage: {sys.argv[0]} <{'|'.join(_COMMANDS)}>") + sys.exit(1) + reset_global_state() + fn = _COMMANDS[sys.argv[1]] + fn() diff --git a/robot/validation_attach_type_guard.robot b/robot/validation_attach_type_guard.robot new file mode 100644 index 000000000..dec96cfcf --- /dev/null +++ b/robot/validation_attach_type_guard.robot @@ -0,0 +1,35 @@ +*** Settings *** +Documentation Integration tests for the validation attach type-discriminator guard. +... Verifies that ``agents validation attach`` rejects plain Tools and +... accepts genuine Validations end-to-end via the CLI. +Resource ${CURDIR}/common.resource +Suite Setup Setup Test Environment With Database Isolation +Suite Teardown Cleanup Test Environment + +*** Variables *** +${HELPER} ${CURDIR}/helper_validation_attach_type_guard.py + +*** Test Cases *** +Validation Attach Rejects Plain Tool + [Documentation] Attaching a plain Tool (tool_type="tool") must be rejected with a clear error message. + ${result}= Run Process ${PYTHON} ${HELPER} attach-plain-tool-rejected cwd=${WORKSPACE} timeout=120s on_timeout=kill + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} validation-attach-plain-tool-rejected-ok + +Validation Attach Accepts Genuine Validation + [Documentation] Attaching a genuine Validation (tool_type="validation") must succeed. + ${result}= Run Process ${PYTHON} ${HELPER} attach-validation-accepted cwd=${WORKSPACE} timeout=120s on_timeout=kill + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} validation-attach-validation-accepted-ok + +Validation Attach Rejects Plain Tool Dict + [Documentation] Attaching a plain Tool stored as a dict (tool_type="tool") must be rejected. + ${result}= Run Process ${PYTHON} ${HELPER} attach-plain-tool-dict-rejected cwd=${WORKSPACE} timeout=120s on_timeout=kill + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} validation-attach-plain-tool-dict-rejected-ok diff --git a/src/cleveragents/application/services/tool_registry_service.py b/src/cleveragents/application/services/tool_registry_service.py index c52fa95ac..59c73dc0e 100644 --- a/src/cleveragents/application/services/tool_registry_service.py +++ b/src/cleveragents/application/services/tool_registry_service.py @@ -9,8 +9,8 @@ from __future__ import annotations from typing import Any -from cleveragents.core.exceptions import NotFoundError -from cleveragents.domain.models.core.tool import Tool +from cleveragents.core.exceptions import NotFoundError, ToolTypeMismatchError +from cleveragents.domain.models.core.tool import Tool, ToolType from cleveragents.infrastructure.database.repositories import ( ToolRegistryRepository, ValidationAttachmentRepository, @@ -175,6 +175,22 @@ class ToolRegistryService: resource_id=validation_name, ) + # Enforce type-discriminator: only Validation entries may be attached. + # A plain Tool has tool_type == "tool"; reject it immediately. + if isinstance(existing, dict): + actual_type: str = str(existing.get("tool_type", "tool")) + else: + raw_type = getattr(existing, "tool_type", ToolType.TOOL) + actual_type = ( + raw_type.value if isinstance(raw_type, ToolType) else str(raw_type) + ) + + if actual_type != ToolType.VALIDATION.value: + raise ToolTypeMismatchError( + tool_name=validation_name, + actual_type=actual_type, + ) + # Read mode from the validation's registered definition if isinstance(existing, dict): mode = existing.get("mode", "required") diff --git a/src/cleveragents/cli/commands/validation.py b/src/cleveragents/cli/commands/validation.py index a7202e75f..f69bed0de 100644 --- a/src/cleveragents/cli/commands/validation.py +++ b/src/cleveragents/cli/commands/validation.py @@ -60,6 +60,7 @@ from cleveragents.cli.formatting import OutputFormat, format_output from cleveragents.core.exceptions import ( CleverAgentsError, NotFoundError, + ToolTypeMismatchError, ValidationError, ) from cleveragents.domain.models.core.tool import Validation, ValidationMode @@ -350,6 +351,9 @@ def attach( except NotFoundError as exc: console.print(f"[red]Validation not found:[/red] {validation_name}") raise typer.Abort() from exc + except ToolTypeMismatchError as exc: + console.print(f"[red]Type error:[/red] {exc.message}") + raise typer.Abort() from exc except ValidationError as exc: console.print(f"[red]Validation Error:[/red] {exc.message}") raise typer.Abort() from exc diff --git a/src/cleveragents/core/exceptions.py b/src/cleveragents/core/exceptions.py index 4ca01d81f..946e35d3a 100644 --- a/src/cleveragents/core/exceptions.py +++ b/src/cleveragents/core/exceptions.py @@ -38,6 +38,41 @@ class ValidationError(DomainError): pass +class ToolTypeMismatchError(DomainError): + """Raised when a plain Tool is used where a Validation is required. + + The ``agents validation attach`` command only accepts entries whose + ``tool_type`` discriminator is ``"validation"``. Attempting to attach + a plain Tool raises this error. + + Attributes: + tool_name: The namespaced name of the offending tool. + actual_type: The ``tool_type`` value found on the registered entry. + """ + + def __init__( + self, + tool_name: str, + actual_type: str, + details: dict[str, Any] | None = None, + ) -> None: + """Initialise with the offending tool name and its actual type. + + Args: + tool_name: Namespaced name of the tool that was rejected. + actual_type: The ``tool_type`` value found (e.g. ``"tool"``). + details: Optional additional context. + """ + super().__init__( + f"'{tool_name}' is a plain Tool (tool_type='{actual_type}'), " + "not a Validation. Only entries with tool_type='validation' " + "may be attached via 'agents validation attach'.", + details, + ) + self.tool_name = tool_name + self.actual_type = actual_type + + class BusinessRuleViolation(DomainError): """Business rule violations.""" @@ -313,5 +348,6 @@ __all__ = [ "ResourceNotFoundError", "StreamRoutingError", "TokenLimitExceededError", + "ToolTypeMismatchError", "ValidationError", ] -- 2.52.0