fix(validation): replace positional key=value args with --key value named options in validation attach command

Refactors the 'agents validation attach' command to accept extra validation arguments as named CLI options using '--key value' format (e.g. '--coverage-threshold 90'), as required by the specification. Changes: - validation.py: Replace positional 'key=value' Argument with Typer context settings (allow_extra_args=True, ignore_unknown_options=True) to capture '--key value' named options from ctx.args. Strips '--' prefix and maps tokens to {key: value} dict entries. Rejects bare tokens (not '--key value') with a clear error message. - features/tdd_validation_attach_named_options.feature: New TDD Behave scenarios covering single/multiple named options, no-args case, and rejection of old positional key=value format. - features/steps/tdd_validation_attach_named_options_steps.py: Step definitions for the new feature. - robot/validation_attach_named_options.robot: Robot Framework integration tests verifying spec-compliant '--key value' option format. - robot/helper_validation_attach_named_options.py: Helper script for the Robot Framework tests. - features/steps/tdd_cli_incomplete_subcommand_registration_steps.py: Fix pre-existing CliRunner(mix_stderr=False) incompatibility with current Typer. - features/steps/tool_runtime_steps.py: Fix pre-existing AmbiguousStep error by converting conflicting parse-based step definitions to regex matchers. - features/consolidated_tool.feature: Update step text to match renamed step. Closes #3684
2026-04-06 07:06:57 +00:00
parent 8f8493197e
commit 58c9760856
5 changed files with 505 additions and 25 deletions
@@ -0,0 +1,224 @@
+"""Helper script for validation_attach_named_options.robot E2E tests.
+
+Each subcommand is a self-contained check that prints a sentinel on success.
+Tests that ``agents validation attach`` accepts ``--key value`` named option
+format for extra validation arguments, as required by the spec.
+"""
+
+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
+
+runner = CliRunner()
+
+_ATTACHMENT_ULID = "01NAMEDOPT000000000000001"
+
+
+def _make_mock_attachment(validation_name: str, resource_id: str) -> MagicMock:
+    """Create a mock attachment object."""
+    mock_attachment = MagicMock()
+    mock_attachment.attachment_id = _ATTACHMENT_ULID
+    mock_attachment.validation_name = validation_name
+    mock_attachment.resource_id = resource_id
+    mock_attachment.mode = "required"
+    mock_attachment.project_name = None
+    mock_attachment.plan_id = None
+    mock_attachment.created_at = "2026-01-01T00:00:00"
+    return mock_attachment
+
+
+# ---------------------------------------------------------------------------
+# Subcommands
+# ---------------------------------------------------------------------------
+
+
+def attach_with_coverage_threshold() -> None:
+    """Verify that --coverage-threshold 90 named option is accepted and forwarded."""
+    mock_svc = MagicMock()
+    mock_svc.attach_validation.return_value = _make_mock_attachment(
+        "local/coverage-check", "git-checkout/my-repo"
+    )
+    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",
+                "--coverage-threshold",
+                "90",
+                "--format",
+                "plain",
+            ],
+        )
+
+    if result.exit_code != 0:
+        print(
+            f"FAIL: exit={result.exit_code} output={result.output!r} "
+            f"exception={result.exception!r}"
+        )
+        sys.exit(1)
+
+    # Verify the service received the correct args dict
+    call_kwargs = mock_svc.attach_validation.call_args
+    if call_kwargs is None:
+        print("FAIL: attach_validation was not called")
+        sys.exit(1)
+
+    args_dict = call_kwargs.kwargs.get("args") or {}
+    if args_dict.get("coverage-threshold") != "90":
+        print(f"FAIL: expected args={{'coverage-threshold': '90'}}, got {args_dict!r}")
+        sys.exit(1)
+
+    print("validation-attach-named-option-coverage-threshold-ok")
+
+
+def attach_with_multiple_named_options() -> None:
+    """Verify that multiple --key value named options are all forwarded."""
+    mock_svc = MagicMock()
+    mock_svc.attach_validation.return_value = _make_mock_attachment(
+        "local/lint-check", "git-checkout/my-repo"
+    )
+    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/lint-check",
+                "--threshold",
+                "70",
+                "--strict",
+                "true",
+                "--format",
+                "plain",
+            ],
+        )
+
+    if result.exit_code != 0:
+        print(
+            f"FAIL: exit={result.exit_code} output={result.output!r} "
+            f"exception={result.exception!r}"
+        )
+        sys.exit(1)
+
+    call_kwargs = mock_svc.attach_validation.call_args
+    if call_kwargs is None:
+        print("FAIL: attach_validation was not called")
+        sys.exit(1)
+
+    args_dict = call_kwargs.kwargs.get("args") or {}
+    if args_dict.get("threshold") != "70":
+        print(f"FAIL: expected threshold='70', got {args_dict!r}")
+        sys.exit(1)
+    if args_dict.get("strict") != "true":
+        print(f"FAIL: expected strict='true', got {args_dict!r}")
+        sys.exit(1)
+
+    print("validation-attach-named-option-multiple-ok")
+
+
+def attach_without_extra_args() -> None:
+    """Verify that attach without extra named options still works."""
+    mock_svc = MagicMock()
+    mock_svc.attach_validation.return_value = _make_mock_attachment(
+        "local/basic-check", "git-checkout/my-repo"
+    )
+    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/basic-check",
+                "--format",
+                "plain",
+            ],
+        )
+
+    if result.exit_code != 0:
+        print(
+            f"FAIL: exit={result.exit_code} output={result.output!r} "
+            f"exception={result.exception!r}"
+        )
+        sys.exit(1)
+
+    print("validation-attach-named-option-no-extra-args-ok")
+
+
+def attach_with_positional_key_value_rejected() -> None:
+    """Verify that old-style positional key=value format is rejected."""
+    mock_svc = MagicMock()
+    mock_svc.attach_validation.return_value = _make_mock_attachment(
+        "local/coverage-check", "git-checkout/my-repo"
+    )
+    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",
+                "coverage-threshold=90",
+                "--format",
+                "plain",
+            ],
+        )
+
+    # Must be rejected (non-zero exit) and output must mention "Invalid argument format"
+    if result.exit_code != 0 and "Invalid argument format" in result.output:
+        print("validation-attach-positional-key-value-rejected-ok")
+    else:
+        print(
+            f"FAIL: exit={result.exit_code} "
+            f"output={result.output!r} "
+            f"(expected non-zero exit and 'Invalid argument format' in output)"
+        )
+        sys.exit(1)
+
+
+# ---------------------------------------------------------------------------
+# Dispatcher
+# ---------------------------------------------------------------------------
+
+_COMMANDS: dict[str, Callable[[], None]] = {
+    "attach-with-coverage-threshold": attach_with_coverage_threshold,
+    "attach-with-multiple-named-options": attach_with_multiple_named_options,
+    "attach-without-extra-args": attach_without_extra_args,
+    "attach-with-positional-key-value-rejected": (
+        attach_with_positional_key_value_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()
@@ -0,0 +1,44 @@
+*** Settings ***
+Documentation    Integration tests for the validation attach named option format.
+...              Verifies that ``agents validation attach`` accepts ``--key value``
+...              named option format for extra validation arguments, as required
+...              by the specification.
+Resource         ${CURDIR}/common.resource
+Suite Setup      Setup Test Environment With Database Isolation
+Suite Teardown   Cleanup Test Environment
+
+*** Variables ***
+${HELPER}    ${CURDIR}/helper_validation_attach_named_options.py
+
+*** Test Cases ***
+Validation Attach Accepts Named Option Coverage Threshold
+    [Documentation]    Attaching with --coverage-threshold 90 must succeed and forward the arg.
+    ${result}=    Run Process    ${PYTHON}    ${HELPER}    attach-with-coverage-threshold    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-named-option-coverage-threshold-ok
+
+Validation Attach Accepts Multiple Named Options
+    [Documentation]    Attaching with multiple --key value pairs must succeed and forward all args.
+    ${result}=    Run Process    ${PYTHON}    ${HELPER}    attach-with-multiple-named-options    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-named-option-multiple-ok
+
+Validation Attach Works Without Extra Named Options
+    [Documentation]    Attaching without extra named options must still succeed.
+    ${result}=    Run Process    ${PYTHON}    ${HELPER}    attach-without-extra-args    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-named-option-no-extra-args-ok
+
+Validation Attach Rejects Positional Key Equals Value Format
+    [Documentation]    Old-style positional key=value format must be rejected with a clear error.
+    ${result}=    Run Process    ${PYTHON}    ${HELPER}    attach-with-positional-key-value-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-positional-key-value-rejected-ok