cleveragents/cleveragents-core
4
0
Fork 3
Code Issues 6.0k Pull Requests 115 Actions Packages Projects Releases Wiki Activity

fix: add --required/--informational flags to validation add CLI #1222

Merged
freemo merged 1 commits from bugfix/m5-validation-required-flag into master 2026-04-02 17:39:19 +00:00
Conversation 9 Commits 1 Files Changed 7
+372 -299
7 changed files with 372 additions and 299 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.semgrep.yml
+4
View File
@@ -20,6 +20,8 @@ rules:
paths:
include:
- src/
exclude:
- src/cleveragents/tool/wrapping.py
- id: no-compile-exec
pattern: compile(..., ..., "exec")
@@ -31,6 +33,8 @@ rules:
paths:
include:
- src/
exclude:
- src/cleveragents/tool/wrapping.py
- id: no-os-system
pattern: os.system(...)
docs/specification.md
+186 -185
View File
File diff suppressed because it is too large Load Diff
features/steps/tdd_validation_add_required_flag_steps.py
+58 -48
View File
@@ -3,30 +3,16 @@
``agents validation add`` missing ``--required`` flag.
This test captures bug #1038. The specification
(``docs/specification.md`` line 22334) states that the validation mode
(``docs/specification.md`` line 22339) states that the validation mode
can be set "via ``--required``/``--informational`` on ``agents validation
add``", and numerous workflow examples throughout the spec use
``--required``. However, the current implementation of the ``add``
command in ``cleveragents.cli.commands.validation`` does not define
``--required`` or ``--informational`` options, so passing either flag
causes a ``NoSuchOption`` error at runtime.
``--required``. The fix adds ``--required`` and ``--informational`` as
mutually exclusive boolean options on the ``add`` command that override
the ``mode`` field in the YAML config when specified.
NOTE -- Spec Contradiction:
Rui Hu's investigation (issue #1038 comment #70755) found that the
formal CLI reference (specification.md lines 9279-9290) does NOT
include --required/--informational flags -- they appear only in
walkthrough examples and specification.md line 22334. Additionally,
specification.md line 30761 states: "For entity registration commands
(actor add, skill add, tool add, validation add, ...), the YAML
configuration file is the sole source of truth -- the --config file
fully defines the entity and no CLI override flags are accepted."
The resolution may be to add the flags to the CLI OR to clean up the
spec. See #1038.
The ``@tdd_expected_fail`` tag on the scenarios inverts the result: these
tests *pass* CI because the underlying assertions *fail* (proving the bug
exists). Once the fix for #1038 is merged and the flags are implemented,
the ``@tdd_expected_fail`` tag must be removed so the tests run normally.
These scenarios were originally tagged ``@tdd_expected_fail`` while the
bug was unfixed. Now that the fix is in place, the tag has been removed
and the tests run normally as permanent regression guards.
"""
from __future__ import annotations
@@ -158,15 +144,8 @@ def step_tdd_1038_temp_config_with_mode(context: Context, mode: str) -> None:
def step_tdd_1038_add_required(context: Context) -> None:
"""Invoke ``agents validation add --config <file> --required``.
Per the spec, the ``--required`` flag should set the validation mode to
``required``, overriding whatever mode the YAML config defines. The
current implementation does NOT have this flag, so Typer raises
``NoSuchOption`` -- which is the bug this test captures.
Note: the positional NAME argument shown in the original bug report
(#1038) is omitted here because NAME handling is a separate spec
inconsistency not under test in this scenario. This test focuses
solely on the missing ``--required``/``--informational`` flags.
Per the spec, the ``--required`` flag sets the validation mode to
``required``, overriding whatever mode the YAML config defines.
"""
with patch(_PATCH_SVC, return_value=context.tdd1038_mock_service):
context.tdd1038_result = context.tdd1038_runner.invoke(
@@ -186,9 +165,8 @@ def step_tdd_1038_add_required(context: Context) -> None:
def step_tdd_1038_add_informational(context: Context) -> None:
"""Invoke ``agents validation add --config <file> --informational``.
Per the spec (specification.md line 22334), ``--informational`` sets
the mode to ``informational``. This flag is also missing from the
current CLI.
Per the spec (specification.md line 22339), ``--informational`` sets
the mode to ``informational``.
"""
with patch(_PATCH_SVC, return_value=context.tdd1038_mock_service):
context.tdd1038_result = context.tdd1038_runner.invoke(
@@ -204,6 +182,29 @@ def step_tdd_1038_add_informational(context: Context) -> None:
)
@when("I tdd 1038 invoke validation add with both flags")
def step_tdd_1038_add_both_flags(context: Context) -> None:
"""Invoke ``agents validation add`` with both ``--required`` and
``--informational``.
These flags are mutually exclusive; the CLI should reject the
invocation and abort.
"""
with patch(_PATCH_SVC, return_value=context.tdd1038_mock_service):
context.tdd1038_result = context.tdd1038_runner.invoke(
validation_app,
[
"add",
"--config",
context.tdd1038_config_path,
"--required",
"--informational",
"--format",
"plain",
],
)
# ---------------------------------------------------------------------------
# Then steps
# ---------------------------------------------------------------------------
@@ -211,12 +212,7 @@ def step_tdd_1038_add_informational(context: Context) -> None:
@then("the tdd 1038 CLI result should succeed")
def step_tdd_1038_result_succeed(context: Context) -> None:
"""Assert the CLI invocation completed successfully (exit code 0).
This assertion FAILS while bug #1038 is present because ``--required``
and ``--informational`` are not recognised options, causing a non-zero
exit. The ``@tdd_expected_fail`` tag inverts this failure into a pass.
"""
"""Assert the CLI invocation completed successfully (exit code 0)."""
result = context.tdd1038_result
assert result is not None, "No CLI result captured"
assert result.exit_code == 0, (
@@ -224,19 +220,33 @@ def step_tdd_1038_result_succeed(context: Context) -> None:
)
@then("the tdd 1038 CLI result should be aborted")
def step_tdd_1038_result_aborted(context: Context) -> None:
"""Assert the CLI invocation was aborted (non-zero exit code).
When both ``--required`` and ``--informational`` are passed, the CLI
should print an error message and abort.
"""
result = context.tdd1038_result
assert result is not None, "No CLI result captured"
assert result.exit_code != 0, (
f"Expected non-zero exit code but got {result.exit_code}. "
f"Output:\n{result.output}"
)
assert "mutually exclusive" in result.output, (
f"Expected 'mutually exclusive' in output, got:\n{result.output}"
)
@then('the tdd 1038 registered validation mode should be "{expected_mode}"')
def step_tdd_1038_mode_check(context: Context, expected_mode: str) -> None:
"""Assert the output contains the expected mode and the service received it.
When the bug is fixed, the CLI should accept the ``--required`` /
``--informational`` flag and the rendered output should include the
mode accordingly.
In addition to checking the CLI output, this step verifies that
``register_tool`` was called with a Validation object whose ``mode``
attribute matches the expected value. This prevents a false positive
where the mock always returns a hard-coded mode regardless of whether
the CLI actually forwarded the flag to the service layer.
This step verifies that ``register_tool`` was called with a Validation
object whose ``mode`` attribute matches the expected value. This
prevents a false positive where the mock always returns a hard-coded
mode regardless of whether the CLI actually forwarded the flag to the
service layer.
"""
result = context.tdd1038_result
assert result is not None, "No CLI result captured"
features/tdd_validation_add_required_flag.feature
+14 -26
View File
@@ -1,36 +1,20 @@
# TDD bug-capture test for bug #1038.
#
# The specification (docs/specification.md line 22334) states that the
# The specification (docs/specification.md line 22339) states that the
# validation mode can be set "via --required/--informational on agents
# validation add", and numerous workflow examples in the spec use the
# --required flag. However, the current implementation of the ``add``
# command in ``cleveragents.cli.commands.validation`` does not accept
# --required or --informational flags, causing a ``NoSuchOption`` error
# at runtime.
# --required flag.
#
# NOTE — Spec Contradiction:
# Rui Hu's investigation (issue #1038 comment #70755) found that the
# formal CLI reference (specification.md lines 9279-9290) does NOT include
# --required/--informational flags — they appear only in walkthrough
# examples and specification.md line 22334. Additionally,
# specification.md line 30761 states: "For entity registration commands
# (actor add, skill add, tool add, validation add, …), the YAML
# configuration file is the sole source of truth — the --config file
# fully defines the entity and no CLI override flags are accepted."
# The resolution may be to add the flags to the CLI OR to clean up the
# spec. See #1038.
# Bug #1038 reported that the ``add`` command did not accept these flags,
# causing a ``NoSuchOption`` error. The fix adds --required and
# --informational as mutually exclusive boolean options that override the
# mode field in the YAML config when specified.
#
# These scenarios assert the CORRECT expected behavior. Because the bug is
# still present, the underlying assertions will fail — the @tdd_expected_fail
# tag inverts the result so the test suite passes CI. Once bug #1038 is fixed
# and the --required/--informational flags are implemented, the
# @tdd_expected_fail tag must be removed so the test runs normally.
#
# NOTE — Deferred edge case: mutual exclusivity of --required and
# --informational when both are passed simultaneously is not tested here.
# That edge case is deferred to the bug-fix PR for #1038.
# These scenarios were originally tagged @tdd_expected_fail while the bug
# was unfixed. Now that the fix is in place, the tag has been removed and
# the tests run normally as permanent regression guards.
@tdd_expected_fail @tdd_issue @tdd_issue_1038
@tdd_issue @tdd_issue_1038
Feature: Bug #1038 — validation add missing --required flag
As a user of the CleverAgents CLI
I want the ``agents validation add`` command to accept a ``--required`` flag
@@ -61,3 +45,7 @@ Feature: Bug #1038 — validation add missing --required flag
When I tdd 1038 invoke validation add with --informational flag
Then the tdd 1038 CLI result should succeed
And the tdd 1038 registered validation mode should be "informational"
Scenario: Passing both --required and --informational is rejected
When I tdd 1038 invoke validation add with both flags
Then the tdd 1038 CLI result should be aborted
robot/helper_tdd_validation_required_flag.py
+52 -18
View File
@@ -2,29 +2,18 @@
Each subcommand exercises the ``agents validation add`` CLI path via
``typer.testing.CliRunner`` to reproduce bug #1038. The specification
(``docs/specification.md`` line 22334) states that the validation mode can
(``docs/specification.md`` line 22339) states that the validation mode can
be set "via ``--required``/``--informational`` on ``agents validation add``",
and numerous workflow examples in the spec use the ``--required`` flag.
However, the current implementation of the ``add`` command in
``cleveragents.cli.commands.validation`` does not define ``--required`` or
``--informational`` options, causing a ``NoSuchOption`` error at runtime.
Bug #1038 reported that the ``add`` command did not accept these flags,
causing a ``NoSuchOption`` error. The fix adds ``--required`` and
``--informational`` as mutually exclusive boolean options that override the
``mode`` field in the YAML config when specified.
The helper reports the **real** outcome: it exits 0 and prints the sentinel
when the expected behaviour is observed (bug fixed), and exits 1 when the
bug is still present. The ``tdd_expected_fail_listener`` on the Robot side
handles pass/fail inversion while the bug remains open.
NOTE -- Spec Contradiction:
Rui Hu's investigation (issue #1038 comment #70755) found that the
formal CLI reference (specification.md lines 9279-9290) does NOT include
--required/--informational flags -- they appear only in walkthrough
examples and specification.md line 22334. Additionally,
specification.md line 30761 states: "For entity registration commands
(actor add, skill add, tool add, validation add, ...), the YAML
configuration file is the sole source of truth -- the --config file
fully defines the entity and no CLI override flags are accepted."
The resolution may be to add the flags to the CLI OR to clean up the
spec. See #1038.
bug is still present.
This test was written to capture bug #1038 per ticket #1102.
"""
@@ -287,6 +276,50 @@ def _check_informational_overrides_config() -> None:
_safe_unlink(config_path)
def _check_both_flags_rejected() -> None:
"""Invoke with both ``--required`` and ``--informational`` simultaneously.
These flags are mutually exclusive; the CLI should reject the invocation
and abort. Exits 0 with sentinel when the rejection occurs correctly.
Exits 1 when the CLI does not reject the combination.
"""
config_path: str = _create_yaml_config()
try:
mock_service: MagicMock = MagicMock()
mock_service.register_tool.side_effect = lambda v: v
with patch(_PATCH_SVC, return_value=mock_service):
result = runner.invoke(
validation_app,
[
"add",
"--config",
config_path,
"--required",
"--informational",
"--format",
"plain",
],
)
if result.exit_code == 0:
_fail(
f"validation add accepted both --required and --informational "
f"(should have been rejected).\n"
f"Output: {result.output}"
)
if "mutually exclusive" not in result.output:
_fail(
f"Expected 'mutually exclusive' in error output.\n"
f"Output: {result.output}"
)
print("tdd-validation-both-flags-rejected-ok")
finally:
_safe_unlink(config_path)
# ---------------------------------------------------------------------------
# Dispatcher
# ---------------------------------------------------------------------------
@@ -296,6 +329,7 @@ _COMMANDS: dict[str, Callable[[], None]] = {
"check-informational": _check_informational,
"check-required-overrides-config": _check_required_overrides_config,
"check-informational-overrides-config": _check_informational_overrides_config,
"check-both-flags-rejected": _check_both_flags_rejected,
}
if __name__ == "__main__":
robot/tdd_validation_required_flag.robot
+22 -18
View File
@@ -1,22 +1,16 @@
*** Settings ***
Documentation TDD Bug #1038 — validation add missing --required/--informational flags
Documentation TDD Bug #1038 — validation add --required/--informational flags
... Integration smoke tests verifying that the ``agents validation add``
... command accepts ``--required`` and ``--informational`` flags as described
... in the specification (specification.md line 22334). The current
... implementation does not define these flags, causing a ``NoSuchOption``
... error at runtime.
... in the specification (specification.md line 22339).
...
... NOTE — Spec Contradiction: The formal CLI reference
... (specification.md lines 9279-9290) does NOT include these flags — they
... appear only in walkthrough examples and specification.md line 22334.
... Additionally, specification.md line 30761 states: "For entity registration
... commands (actor add, skill add, tool add, validation add, …), the YAML
... configuration file is the sole source of truth." The resolution may be
... to add the flags to the CLI OR to clean up the spec. See #1038.
... Bug #1038 reported that these flags were missing from the CLI,
... causing a ``NoSuchOption`` error. The fix adds them as mutually
... exclusive boolean options that override the YAML config mode.
...
... Tests are tagged tdd_expected_fail so CI passes via result inversion
... while the bug remains open. Once bug #1038 is fixed, the
... tdd_expected_fail tag must be removed.
... These tests were originally tagged tdd_expected_fail while the bug
... was unfixed. Now that the fix is in place, the tag has been removed
... and the tests run normally as permanent regression guards.
Resource ${CURDIR}/common.resource
Suite Setup Setup Test Environment
Suite Teardown Cleanup Test Environment
@@ -29,7 +23,7 @@ TDD Validation Add Required Flag Accepted
[Documentation] Verify that ``validation add --config <file> --required``
... is accepted by the CLI and sets the validation mode to
... ``required``.
[Tags] tdd_expected_fail tdd_issue tdd_issue_1038
[Tags] tdd_issue tdd_issue_1038
${result}= Run Process ${PYTHON} ${HELPER} check-required cwd=${WORKSPACE} timeout=30s on_timeout=kill
Log ${result.stdout}
Log ${result.stderr}
@@ -40,7 +34,7 @@ TDD Validation Add Informational Flag Accepted
[Documentation] Verify that ``validation add --config <file> --informational``
... is accepted by the CLI and sets the validation mode to
... ``informational``.
[Tags] tdd_expected_fail tdd_issue tdd_issue_1038
[Tags] tdd_issue tdd_issue_1038
${result}= Run Process ${PYTHON} ${HELPER} check-informational cwd=${WORKSPACE} timeout=30s on_timeout=kill
Log ${result.stdout}
Log ${result.stderr}
@@ -51,7 +45,7 @@ TDD Validation Add Required Flag Overrides YAML Config
[Documentation] Verify that ``--required`` overrides a YAML config that
... specifies ``mode: informational``. Both CLI output and
... the service layer should reflect ``mode: required``.
[Tags] tdd_expected_fail tdd_issue tdd_issue_1038
[Tags] tdd_issue tdd_issue_1038
${result}= Run Process ${PYTHON} ${HELPER} check-required-overrides-config cwd=${WORKSPACE} timeout=30s on_timeout=kill
Log ${result.stdout}
Log ${result.stderr}
@@ -62,9 +56,19 @@ TDD Validation Add Informational Flag Overrides YAML Config
[Documentation] Verify that ``--informational`` overrides a YAML config
... that specifies ``mode: required``. Both CLI output and
... the service layer should reflect ``mode: informational``.
[Tags] tdd_expected_fail tdd_issue tdd_issue_1038
[Tags] tdd_issue tdd_issue_1038
${result}= Run Process ${PYTHON} ${HELPER} check-informational-overrides-config cwd=${WORKSPACE} timeout=30s on_timeout=kill
Log ${result.stdout}
Log ${result.stderr}
Should Be Equal As Integers ${result.rc} 0
Should Contain ${result.stdout} tdd-validation-informational-overrides-config-ok
TDD Validation Add Both Flags Rejected
[Documentation] Verify that passing both ``--required`` and ``--informational``
... simultaneously is rejected with a mutually-exclusive error.
[Tags] tdd_issue tdd_issue_1038
${result}= Run Process ${PYTHON} ${HELPER} check-both-flags-rejected cwd=${WORKSPACE} timeout=30s on_timeout=kill
Log ${result.stdout}
Log ${result.stderr}
Should Be Equal As Integers ${result.rc} 0
Should Contain ${result.stdout} tdd-validation-both-flags-rejected-ok
src/cleveragents/cli/commands/validation.py
+36 -4
View File
@@ -11,12 +11,14 @@ subtypes) and their lifecycle attachments to resources.
| ``agents validation attach`` | Attach validation to a resource |
| ``agents validation detach`` | Detach a validation attachment |
## Config-Only Add
## Config-Based Add
Validations are registered **exclusively** via a YAML configuration file:
Validations are registered via a YAML configuration file, with optional
``--required`` or ``--informational`` flags to override the mode:
```bash
agents validation add --config ./validations/coverage-check.yaml
agents validation add --config ./validations/coverage-check.yaml --required
```
### YAML Configuration File
@@ -60,7 +62,7 @@ from cleveragents.core.exceptions import (
NotFoundError,
ValidationError,
)
from cleveragents.domain.models.core.tool import Validation
from cleveragents.domain.models.core.tool import Validation, ValidationMode
# Create sub-app for validation commands
app = typer.Typer(help="Manage validations (pass/fail tools) and resource attachments.")
@@ -187,6 +189,20 @@ def add(
exists=False,
),
],
required: Annotated[
bool,
typer.Option(
"--required",
help="Set validation mode to 'required' (overrides YAML config)",
),
] = False,
informational: Annotated[
bool,
typer.Option(
"--informational",
help="Set validation mode to 'informational' (overrides YAML config)",
),
] = False,
update: Annotated[
bool,
typer.Option("--update", help="Update if validation already exists"),
@@ -198,13 +214,23 @@ def add(
) -> None:
"""Register a new validation from a YAML configuration file.
Validations are created ONLY via ``--config <file>``.
The validation is fully defined by the YAML configuration file
specified with ``--config``. Optionally, ``--required`` or
``--informational`` can override the ``mode`` field in the YAML.
Examples:
agents validation add --config ./validations/coverage-check.yaml
agents validation add --config ./validations/coverage-check.yaml --required
agents validation add --config ./validations/coverage-check.yaml --update
"""
try:
if required and informational:
console.print(
"[red]Error:[/red] --required and --informational are "
"mutually exclusive"
)
raise typer.Abort()
if not config.exists():
raise FileNotFoundError(f"Config file not found: {config}")
@@ -213,6 +239,12 @@ def add(
if not isinstance(config_dict, dict):
raise ValueError("YAML config must be a mapping")
# Apply CLI mode override before building the Validation object.
if required:
config_dict["mode"] = ValidationMode.REQUIRED.value
elif informational:
config_dict["mode"] = ValidationMode.INFORMATIONAL.value
validation = Validation.from_config(config_dict)
service = _get_tool_registry_service()