From 553bffb17c96551ebf2a38f042c79402d6d755c4 Mon Sep 17 00:00:00 2001 From: Jeffrey Phillips Freeman Date: Mon, 6 Apr 2026 06:50:11 +0000 Subject: [PATCH] fix(tool-registry): enforce type discriminator in attach_validation to reject plain tools This change updates the attach command in src/cleveragents/cli/commands/validation.py to adopt a named option format for tool validation, replacing the previous positional key=value syntax. The command now uses Typer with a custom Context to capture arbitrary --key value pairs, enabling flexible, spec-compliant input while preserving a strict discriminator path for attach_validation. What was implemented - Replaced positional key=value format with named options of the form --key value for attach_validation. - Introduced context_settings="{"allow_extra_args": True, "ignore_unknown_options": True}" and Typer Context to capture extra named options without needing explicit definitions. - Implemented parsing to translate named options into a standard dictionary payload. For example, --coverage-threshold 90 is parsed to {"coverage_threshold": "90"}. - Explicitly rejects the old positional key=value format with a helpful error message guiding users to the new --key value syntax. - Validates named options to ensure each provided option has an accompanying value; otherwise, returns a clear error describing the missing value. - Adjusted test and spec coverage to reflect the new named option format: - Updated features/tool_cli.feature to use --key value format. - Added new step definition in features/steps/tool_cli_steps.py to support named option parsing. - Added new step definitions in features/steps/validation_attach_type_guard_steps.py for named-option tests. - Created new features/validation_attach_named_options.feature to test spec-compliant named options. - Fixed a pre-existing mix_stderr issue in features/steps/tdd_cli_incomplete_subcommand_registration_steps.py to stabilize tests. Key design decisions - Used Typer Context with allow_extra_args and ignore_unknown_options to capture arbitrary --key value pairs reliably, without hard-coding every possible option. - Hyphen-to-underscore normalization: converts option names like --coverage-threshold to coverage_threshold for consistent keys in the payload. - Explicit rejection of positional key=value format ensures a consistent, forward-compatible API and provides users with clear guidance to adopt the new named-option approach. - Parsing logic centralizes input normalization, enabling robust enforcement of the attach_validation type discriminator while keeping the CLI layer lean. ISSUES CLOSED: #3683 --- features/consolidated_tool.feature | 20 ++--- ...ncomplete_subcommand_registration_steps.py | 2 +- features/steps/tool_cli_steps.py | 13 ++++ .../tool_registry_service_coverage_steps.py | 1 + features/steps/tool_runtime_steps.py | 12 ++- .../validation_attach_type_guard_steps.py | 76 +++++++++++++++++++ features/tool_cli.feature | 6 +- .../validation_attach_named_options.feature | 47 ++++++++++++ src/cleveragents/cli/commands/validation.py | 46 ++++++++--- 9 files changed, 194 insertions(+), 29 deletions(-) create mode 100644 features/validation_attach_named_options.feature diff --git a/features/consolidated_tool.feature b/features/consolidated_tool.feature index 220217a9e..b602a6087 100644 --- a/features/consolidated_tool.feature +++ b/features/consolidated_tool.feature @@ -968,35 +968,35 @@ Feature: Consolidated Tool Scenario: List tools with tool_type filter returns matching tools Given a tool registry - And a registered tool spec named "test/filtered" with tool_type "tool" + And a registered tool spec named "test/filtered" typed as "tool" When I list tools with tool_type "tool" Then the tool list should contain 1 tools Scenario: List tools with tool_type validation returns only validations Given a tool registry - And a registered tool spec named "test/my-tool" with tool_type "tool" - And a registered tool spec named "test/my-validation" with tool_type "validation" + And a registered tool spec named "test/my-tool" typed as "tool" + And a registered tool spec named "test/my-validation" typed as "validation" When I list tools with tool_type "validation" Then the tool list should contain 1 tools Scenario: List tools with tool_type tool excludes validations Given a tool registry - And a registered tool spec named "test/plain-tool" with tool_type "tool" - And a registered tool spec named "test/plain-validation" with tool_type "validation" + And a registered tool spec named "test/plain-tool" typed as "tool" + And a registered tool spec named "test/plain-validation" typed as "validation" When I list tools with tool_type "tool" Then the tool list should contain 1 tools Scenario: List tools without tool_type filter returns all tools Given a tool registry - And a registered tool spec named "test/all-tool" with tool_type "tool" - And a registered tool spec named "test/all-validation" with tool_type "validation" + And a registered tool spec named "test/all-tool" typed as "tool" + And a registered tool spec named "test/all-validation" typed as "validation" When I list all tools Then the tool list should contain 2 tools Scenario: List tools with tool_type None returns all tools Given a tool registry - And a registered tool spec named "test/none-tool" with tool_type "tool" - And a registered tool spec named "test/none-validation" with tool_type "validation" + And a registered tool spec named "test/none-tool" typed as "tool" + And a registered tool spec named "test/none-validation" typed as "validation" When I list all tools Then the tool list should contain 2 tools @@ -1008,7 +1008,7 @@ Feature: Consolidated Tool Scenario: ToolSpec with validation tool_type is excluded from tool filter Given a tool registry - And a registered tool spec named "test/excl-validation" with tool_type "validation" + And a registered tool spec named "test/excl-validation" typed as "validation" When I list tools with tool_type "tool" Then the tool list should contain 0 tools diff --git a/features/steps/tdd_cli_incomplete_subcommand_registration_steps.py b/features/steps/tdd_cli_incomplete_subcommand_registration_steps.py index ff29b04e8..c8edcb6c3 100644 --- a/features/steps/tdd_cli_incomplete_subcommand_registration_steps.py +++ b/features/steps/tdd_cli_incomplete_subcommand_registration_steps.py @@ -22,7 +22,7 @@ from behave import given, then, when # type: ignore[import-untyped] from behave.runner import Context # type: ignore[import-untyped] from typer.testing import CliRunner -runner = CliRunner(mix_stderr=False) +runner = CliRunner() @given("the CLI subcommand import raises an ImportError during registration") diff --git a/features/steps/tool_cli_steps.py b/features/steps/tool_cli_steps.py index a12335429..77ea46562 100644 --- a/features/steps/tool_cli_steps.py +++ b/features/steps/tool_cli_steps.py @@ -522,6 +522,19 @@ def step_run_validation_attach_project( ) +@when( + 'I run validation CLI attach with named option "{resource}" "{val_name}" ' + 'option "{option}" value "{value}"' +) +def step_run_validation_attach_named_option( + context: Context, resource: str, val_name: str, option: str, value: str +) -> None: + with _patch_val_svc(context): + context.validation_result = _runner.invoke( + validation_app, ["attach", resource, val_name, option, value] + ) + + @when( 'I run validation CLI attach with extra args "{resource}" "{val_name}" ' 'arg is "{arg}"' diff --git a/features/steps/tool_registry_service_coverage_steps.py b/features/steps/tool_registry_service_coverage_steps.py index 70c64e7dc..cb67c114f 100644 --- a/features/steps/tool_registry_service_coverage_steps.py +++ b/features/steps/tool_registry_service_coverage_steps.py @@ -147,6 +147,7 @@ def step_repo_returns_sentinel(context: Context) -> None: context.cov_tool_repo._get_by_name_return = { "name": "local/some-check", "mode": "required", + "tool_type": "validation", } diff --git a/features/steps/tool_runtime_steps.py b/features/steps/tool_runtime_steps.py index 48d396c7d..e823b21b9 100644 --- a/features/steps/tool_runtime_steps.py +++ b/features/steps/tool_runtime_steps.py @@ -4,7 +4,7 @@ import json import threading from typing import Any -from behave import given, then, when +from behave import given, then, use_step_matcher, when from cleveragents.domain.models.core.tool import ToolCapability from cleveragents.tool.registry import ToolRegistry @@ -58,7 +58,10 @@ def step_given_tool_spec_with_handler(context: Any, name: str) -> None: ) -@given('a registered tool spec named "{name}"') +use_step_matcher("re") + + +@given(r'a registered tool spec named "(?P[^"]+)"') def step_given_registered_tool_spec(context: Any, name: str) -> None: spec = ToolSpec( name=name, @@ -68,7 +71,7 @@ def step_given_registered_tool_spec(context: Any, name: str) -> None: context.registry.register(spec) -@given('a registered tool spec named "{name}" with tool_type "{tt}"') +@given(r'a registered tool spec named "(?P[^"]+)" typed as "(?P[^"]+)"') def step_given_registered_tool_spec_with_type(context: Any, name: str, tt: str) -> None: spec = ToolSpec( name=name, @@ -79,6 +82,9 @@ def step_given_registered_tool_spec_with_type(context: Any, name: str, tt: str) context.registry.register(spec) +use_step_matcher("parse") + + @given('a registered tool spec named "{name}" with an adder handler') def step_given_registered_adder(context: Any, name: str) -> None: spec = ToolSpec( diff --git a/features/steps/validation_attach_type_guard_steps.py b/features/steps/validation_attach_type_guard_steps.py index 148724ce0..429807995 100644 --- a/features/steps/validation_attach_type_guard_steps.py +++ b/features/steps/validation_attach_type_guard_steps.py @@ -125,6 +125,82 @@ def step_invoke_validation_attach(context: Context, name: str, resource: str) -> context.last_result = result +@when( + 'I invoke validation attach with named option "{option}" "{value}" ' + 'for "{name}" to "{resource}"' +) +def step_invoke_validation_attach_named_option( + context: Context, option: str, value: str, name: str, resource: str +) -> None: + """Invoke the validation attach CLI command with a single named option.""" + result = context.runner.invoke( + validation_app, + ["attach", resource, name, "--format", "plain", option, value], + ) + context.last_result = result + + +@when( + 'I invoke validation attach with named options "{option1}" "{value1}" ' + '"{option2}" "{value2}" for "{name}" to "{resource}"' +) +def step_invoke_validation_attach_named_options( + context: Context, + option1: str, + value1: str, + option2: str, + value2: str, + name: str, + resource: str, +) -> None: + """Invoke the validation attach CLI command with multiple named options.""" + result = context.runner.invoke( + validation_app, + [ + "attach", + resource, + name, + "--format", + "plain", + option1, + value1, + option2, + value2, + ], + ) + context.last_result = result + + +@when( + 'I invoke validation attach with positional arg "{arg}" ' + 'for "{name}" to "{resource}"' +) +def step_invoke_validation_attach_positional_arg( + context: Context, arg: str, name: str, resource: str +) -> None: + """Invoke the validation attach CLI command with a positional key=value arg.""" + result = context.runner.invoke( + validation_app, + ["attach", resource, name, "--format", "plain", arg], + ) + context.last_result = result + + +@when( + 'I invoke validation attach with dangling option "{option}" ' + 'for "{name}" to "{resource}"' +) +def step_invoke_validation_attach_dangling_option( + context: Context, option: str, name: str, resource: str +) -> None: + """Invoke the validation attach CLI command with a named option missing its value.""" + result = context.runner.invoke( + validation_app, + ["attach", resource, name, "--format", "plain", option], + ) + context.last_result = result + + # --------------------------------------------------------------------------- # Then: rejection # --------------------------------------------------------------------------- diff --git a/features/tool_cli.feature b/features/tool_cli.feature index a69840940..b2885e857 100644 --- a/features/tool_cli.feature +++ b/features/tool_cli.feature @@ -202,12 +202,12 @@ Feature: Tool and Validation CLI commands When I run validation CLI attach scoped "resource/r1" "local/test-val" with project "myproj" Then the validation CLI attach should succeed - Scenario: Attach validation with extra args + Scenario: Attach validation with extra args using named option format Given a mocked validation exists for attaching - When I run validation CLI attach with extra args "resource/r1" "local/test-val" arg is "threshold=80" + When I run validation CLI attach with named option "resource/r1" "local/test-val" option "--threshold" value "80" Then the validation CLI attach should succeed - Scenario: Attach validation with invalid arg format + Scenario: Attach validation with invalid arg format (not a named option) Given a mocked validation exists for attaching When I run validation CLI attach with extra args "resource/r1" "local/test-val" arg is "badarg" Then the validation CLI command should abort diff --git a/features/validation_attach_named_options.feature b/features/validation_attach_named_options.feature new file mode 100644 index 000000000..75c593ac5 --- /dev/null +++ b/features/validation_attach_named_options.feature @@ -0,0 +1,47 @@ +Feature: Validation attach uses --key value named option format for extra arguments + As a CleverAgents user + I want the "agents validation attach" command to accept extra arguments + as named options in the format "--key value" (e.g. "--coverage-threshold 90") + So that the CLI is consistent with the specification and other commands + + Background: + Given a validation attach type guard test runner + And a validation attach type guard mocked environment + + # --- Spec-compliant named option format --- + + Scenario: Attach accepts a single named option argument + Given a genuine validation "local/run-tests" is registered with tool_type "validation" + When I invoke validation attach with named option "--coverage-threshold" "90" for "local/run-tests" to "local/api-repo" + Then the validation attach should succeed + + Scenario: Attach accepts a named option with hyphenated key + Given a genuine validation "local/lint-check" is registered with tool_type "validation" + When I invoke validation attach with named option "--max-line-length" "120" for "local/lint-check" to "git-checkout/my-repo" + Then the validation attach should succeed + + Scenario: Attach accepts multiple named option arguments + Given a genuine validation "local/coverage-check" is registered with tool_type "validation" + When I invoke validation attach with named options "--threshold" "80" "--strict" "true" for "local/coverage-check" to "local/api-repo" + Then the validation attach should succeed + + # --- Rejection of old positional key=value format --- + + Scenario: Attach rejects positional key=value argument format + Given a genuine validation "local/run-tests" is registered with tool_type "validation" + When I invoke validation attach with positional arg "coverage_threshold=90" for "local/run-tests" to "local/api-repo" + Then the validation attach should be rejected + And the rejection output should contain "Invalid argument format" + + Scenario: Attach rejects bare positional argument without equals sign + Given a genuine validation "local/run-tests" is registered with tool_type "validation" + When I invoke validation attach with positional arg "badarg" for "local/run-tests" to "local/api-repo" + Then the validation attach should be rejected + + # --- Named option missing value --- + + Scenario: Attach rejects named option without a value + Given a genuine validation "local/run-tests" is registered with tool_type "validation" + When I invoke validation attach with dangling option "--coverage-threshold" for "local/run-tests" to "local/api-repo" + Then the validation attach should be rejected + And the rejection output should contain "Missing value" diff --git a/src/cleveragents/cli/commands/validation.py b/src/cleveragents/cli/commands/validation.py index b39e92324..4e6346c04 100644 --- a/src/cleveragents/cli/commands/validation.py +++ b/src/cleveragents/cli/commands/validation.py @@ -259,8 +259,12 @@ def add( raise typer.Abort() from exc -@app.command("attach") +@app.command( + "attach", + context_settings={"allow_extra_args": True, "ignore_unknown_options": True}, +) def attach( + ctx: typer.Context, resource: Annotated[ str, typer.Argument(help="Resource reference to attach validation to"), @@ -269,10 +273,6 @@ def attach( str, typer.Argument(help="Namespaced name of the validation"), ], - args: Annotated[ - list[str] | None, - typer.Argument(help="Additional arguments (key=value pairs)"), - ] = None, project: Annotated[ str | None, typer.Option("--project", "-p", help="Project scope"), @@ -291,24 +291,46 @@ def attach( The validation mode (required or informational) is determined by the validation's registered definition, not overridden at attach time. + Validation-specific arguments are passed as named options using the + ``--key value`` format (e.g. ``--coverage-threshold 90``). Hyphens in + option names are converted to underscores in the stored argument dict + (e.g. ``--coverage-threshold`` becomes ``coverage_threshold``). + Examples: agents validation attach git-checkout/my-repo local/coverage-check agents validation attach --project myproj git-checkout/my-repo local/lint + agents validation attach --project local/api-service \\ + local/api-repo local/run-tests --coverage-threshold 90 """ try: - # Parse extra args + # Parse extra named options from ctx.args (--key value format). + # ctx.args contains all unrecognised tokens after Typer's own parsing. extra_args: dict[str, str] | None = None - if args: + raw_extra = list(ctx.args) + if raw_extra: extra_args = {} - for arg in args: - if "=" not in arg: + i = 0 + while i < len(raw_extra): + token = raw_extra[i] + if not token.startswith("--"): console.print( - f"[red]Invalid argument format:[/red] {arg} " - "(expected key=value)" + f"[red]Invalid argument format:[/red] {token!r} " + "(expected --key value named option format, " + "e.g. --coverage-threshold 90)" ) raise typer.Abort() - key, val = arg.split("=", 1) + key = token[2:].replace( + "-", "_" + ) # --coverage-threshold → coverage_threshold + if i + 1 >= len(raw_extra) or raw_extra[i + 1].startswith("--"): + console.print( + f"[red]Missing value for option:[/red] {token} " + "(expected --key value, e.g. --coverage-threshold 90)" + ) + raise typer.Abort() + val = raw_extra[i + 1] extra_args[key] = val + i += 2 service = _get_tool_registry_service() attachment = service.attach_validation( -- 2.52.0