diff --git a/CHANGELOG.md b/CHANGELOG.md index 99894f63c..2ac7fc1a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -304,6 +304,10 @@ ensuring data is stored with proper parameter values. - **Plan Rollback Command** (#8557): Implemented `agents plan rollback []` for checkpoint-based plan state restoration in Epic #8493. The command restores a plan's sandbox to the state captured at a given checkpoint, discarding all decisions made after that checkpoint. The checkpoint can be specified as an optional positional second argument or via the `--to-checkpoint` named option. Supports `--yes/-y` flag to skip confirmation prompts and `--format/-f` for output format selection (rich/plain/json/yaml). Included with comprehensive BDD test coverage (>= 97%) and spec-aligned output formatting showing rollback summary, changes reverted, impact analysis, and post-rollback state panels. +### Fixed + +- **`agents actor add` enforces `--update` flag for existing actors** (#1500): Added regression tests to `features/actor_add_update_enforcement.feature` and step definitions in `features/steps/actor_add_update_enforcement_steps.py` verifying that re-adding an existing actor without `--update` fails, while re-adding with `--update` succeeds. + ### Changed - **Expanded ruff lint scope to cover `.opencode/`** (#10848): The `lint` nox session diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index 5c06947aa..fe4f1a1f0 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -56,6 +56,9 @@ Below are some of the specific details of various contributions. * HAL 9000 has contributed the decision recording hook for the Strategize phase (issue #8522): captures every decision point with question, chosen option, alternatives, confidence, rationale, and full context snapshot for replay and correction. * HAL 9000 has contributed the ContextStrategy protocol and StrategyRegistry plugin registration system (PR #10590 / issue #8616): implemented the pluggable context assembly strategy protocol with proper type-safe method signatures, created the central thread-safe StrategyRegistry supporting registration, lookup, entry-point discovery, and per-strategy configuration (timeout, fragments limits, workers, circuit breaker threshold). Six built-in strategies implemented and documented: simple-keyword, semantic-embedding, breadth-depth-navigator, arce, temporal-archaeology, and plan-decision-context. Full BDD test coverage including thread safety, boundary validation, and error handling tests. (Part of Epic #8505) * HAL9000 has contributed automated implementation of ACMS context policy configuration loader and plan execution integration. +* HAL 9000 has contributed automated bug fixes, security improvements, and migration safety enhancements including the migration prompt safe-default fix (#7503). +* HAL 9000 has contributed the actor add --update flag enforcement tests and BDD coverage for issue #1500: added regression scenario tests verifying that re-adding an existing actor without `--update` fails, while re-adding with `--update` succeeds. + * This project was made possible thanks to considerable donation of time, money, and resources by CleverThis, Inc. * HAL 9000 has contributed automated bug fixes, CLI output formatting improvements, and ongoing maintenance as part of the CleverAgents automation system. * HAL 9000 has contributed the pr-review-pool-supervisor tracking prefix documentation fix (#7891): aligned all documentation references from the outdated `AUTO-REV-POOL` prefix to the correct `AUTO-REV-SUP` prefix used in production. diff --git a/features/actor_add_update_enforcement.feature b/features/actor_add_update_enforcement.feature index ed554930b..efae3a35f 100644 --- a/features/actor_add_update_enforcement.feature +++ b/features/actor_add_update_enforcement.feature @@ -1,11 +1,11 @@ -# Regression tests for bug #2609: actor add must reject re-adding an existing -# actor unless --update is provided. +# Regression tests for bug #1500/#2609: actor add must reject re-adding an +# existing actor unless --update is provided. Feature: agents actor add enforces --update flag for existing actors As a user of the CleverAgents CLI I want `agents actor add` to fail with a clear error when re-adding an existing actor So that I cannot accidentally overwrite actor configurations without explicit intent - @tdd_issue @tdd_issue_2609 @tdd_issue_4178 + @tdd_issue @tdd_issue_1500 @tdd_issue_2609 @tdd_issue_4178 Scenario: Re-adding an existing actor without --update fails with error panel Given an actor add CLI runner where the actor already exists When I run actor add without the --update flag @@ -14,7 +14,7 @@ Feature: agents actor add enforces --update flag for existing actors And the actor-add-enforcement output should contain "Use --update to replace the existing actor definition" And the actor-add-enforcement output should contain the registration timestamp - @tdd_issue @tdd_issue_2609 @tdd_issue_4178 + @tdd_issue @tdd_issue_1500 @tdd_issue_2609 @tdd_issue_4178 Scenario: Re-adding an existing actor without --update shows error status line Given an actor add CLI runner where the actor already exists When I run actor add without the --update flag @@ -22,14 +22,14 @@ Feature: agents actor add enforces --update flag for existing actors And the actor-add-enforcement output should contain "Actor already registered" And the actor-add-enforcement output should contain "use --update to replace" - @tdd_issue @tdd_issue_2609 @tdd_issue_4178 + @tdd_issue @tdd_issue_1500 @tdd_issue_2609 @tdd_issue_4178 Scenario: Re-adding an existing actor with --update succeeds Given an actor add CLI runner where the actor already exists When I run actor add with the --update flag Then the actor-add-enforcement exit code should be 0 And the actor-add-enforcement output should contain "Actor updated" - @tdd_issue @tdd_issue_2609 @tdd_issue_4178 + @tdd_issue @tdd_issue_1500 @tdd_issue_2609 @tdd_issue_4178 Scenario: Adding a new actor without --update succeeds Given an actor add CLI runner where the actor does not exist When I run actor add without the --update flag diff --git a/features/steps/actor_add_update_enforcement_steps.py b/features/steps/actor_add_update_enforcement_steps.py index f49477733..ddc07a592 100644 --- a/features/steps/actor_add_update_enforcement_steps.py +++ b/features/steps/actor_add_update_enforcement_steps.py @@ -1,7 +1,11 @@ -"""Step definitions for actor add --update flag enforcement (issue #2609). +"""Step definitions for actor add --update flag enforcement (issue #1500/#2609). Tests that `agents actor add` rejects re-adding an existing actor without the --update flag, and succeeds when --update is provided. + +The ``add`` command requires a positional NAME argument followed by +``--config ``: + agents actor add --config [--update] """ from __future__ import annotations @@ -105,6 +109,7 @@ def step_when_add_without_update(context: Any) -> None: ) registry.upsert_actor.return_value = context.new_actor mock_svc.return_value = (MagicMock(), registry) + # The add command requires a positional NAME argument before --config context.result = context.runner.invoke( actor_app, ["add", context.actor_name, "--config", str(context.actor_config_path)], @@ -120,6 +125,7 @@ def step_when_add_with_update(context: Any) -> None: # upsert_actor returns the updated actor registry.upsert_actor.return_value = context.updated_actor mock_svc.return_value = (MagicMock(), registry) + # The add command requires a positional NAME argument before --config context.result = context.runner.invoke( actor_app, [ diff --git a/robot/actor_add_update_enforcement.robot b/robot/actor_add_update_enforcement.robot new file mode 100644 index 000000000..a81f3a3a7 --- /dev/null +++ b/robot/actor_add_update_enforcement.robot @@ -0,0 +1,36 @@ +*** Settings *** +Documentation Integration tests for actor add --update flag enforcement (issue #1500) +Resource ${CURDIR}/common.resource +Suite Setup Setup Test Environment +Suite Teardown Cleanup Test Environment + +*** Variables *** +${HELPER} ${CURDIR}/helper_actor_add_update_enforcement.py + +*** Test Cases *** +Actor Add Existing Actor Without Update Flag Fails + [Documentation] Verify that actor add rejects re-adding an existing actor without --update + [Tags] tdd_issue tdd_issue_1500 + ${result}= Run Process ${PYTHON} ${HELPER} reject-existing-without-update cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} actor-add-reject-existing-ok + +Actor Add Existing Actor With Update Flag Succeeds + [Documentation] Verify that actor add with --update on an existing actor succeeds + [Tags] tdd_issue tdd_issue_1500 + ${result}= Run Process ${PYTHON} ${HELPER} accept-existing-with-update cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} actor-add-accept-update-ok + +Actor Add New Actor Without Update Flag Succeeds + [Documentation] Verify that actor add on a new actor succeeds without --update + [Tags] tdd_issue tdd_issue_1500 + ${result}= Run Process ${PYTHON} ${HELPER} accept-new-without-update cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} actor-add-accept-new-ok diff --git a/robot/helper_actor_add_update_enforcement.py b/robot/helper_actor_add_update_enforcement.py new file mode 100644 index 000000000..826a31e8c --- /dev/null +++ b/robot/helper_actor_add_update_enforcement.py @@ -0,0 +1,169 @@ +"""Helper script for actor add --update flag enforcement Robot tests (issue #1500). + +Tests that ``agents actor add`` rejects re-adding an existing actor without +the --update flag, and succeeds when --update is provided or the actor is new. +""" + +from __future__ import annotations + +import json +import sys +import tempfile +from datetime import datetime +from pathlib import Path +from typing import Any +from unittest.mock import MagicMock, patch + +from typer.testing import CliRunner + +from cleveragents.cli.commands.actor import app as actor_app +from cleveragents.core.exceptions import NotFoundError +from cleveragents.domain.models.core.actor import Actor + + +def _make_actor( + *, + name: str = "local/existing-actor", + provider: str = "openai", + model: str = "gpt-4", + config: dict[str, Any] | None = None, + updated_at: datetime | None = None, +) -> Actor: + blob = config or {} + return Actor( + id=1, + name=name, + provider=provider, + model=model, + config_blob=blob, + config_hash=Actor.compute_hash(blob), + unsafe=False, + is_built_in=False, + is_default=False, + updated_at=updated_at or datetime(2026, 2, 7, 14, 22, 0), + ) + + +def _write_config(name: str) -> Path: + config_data: dict[str, Any] = { + "name": name, + "provider": "openai", + "model": "gpt-4", + } + with tempfile.NamedTemporaryFile( + delete=False, suffix=".json", mode="w", encoding="utf-8" + ) as handle: + json.dump(config_data, handle) + handle.flush() + return Path(handle.name) + + +def test_reject_existing_without_update() -> None: + """Re-adding an existing actor without --update must exit 1 with an error panel.""" + actor_name = "local/existing-actor" + config_path = _write_config(actor_name) + existing = _make_actor(name=actor_name, updated_at=datetime(2026, 2, 7, 14, 22, 0)) + + try: + runner = CliRunner() + with patch("cleveragents.cli.commands.actor._get_services") as mock_svc: + registry = MagicMock() + registry.get_actor.return_value = existing + mock_svc.return_value = (MagicMock(), registry) + result = runner.invoke( + actor_app, + ["add", actor_name, "--config", str(config_path)], + ) + finally: + config_path.unlink(missing_ok=True) + + assert result.exit_code == 1, ( + f"Expected exit code 1 but got {result.exit_code}.\nOutput:\n{result.output}" + ) + assert "Actor already exists" in result.output, ( + f"Expected 'Actor already exists' in output:\n{result.output}" + ) + assert "Use --update to replace" in result.output, ( + f"Expected 'Use --update to replace' in output:\n{result.output}" + ) + print("actor-add-reject-existing-ok") + + +def test_accept_existing_with_update() -> None: + """Re-adding an existing actor with --update must exit 0 and show + 'Actor updated'.""" + actor_name = "local/existing-actor" + config_path = _write_config(actor_name) + existing = _make_actor(name=actor_name, updated_at=datetime(2026, 2, 7, 14, 22, 0)) + updated = _make_actor(name=actor_name, updated_at=datetime(2026, 2, 7, 14, 22, 0)) + + try: + runner = CliRunner() + with patch("cleveragents.cli.commands.actor._get_services") as mock_svc: + registry = MagicMock() + registry.get_actor.return_value = existing + registry.upsert_actor.return_value = updated + mock_svc.return_value = (MagicMock(), registry) + result = runner.invoke( + actor_app, + ["add", actor_name, "--config", str(config_path), "--update"], + ) + finally: + config_path.unlink(missing_ok=True) + + assert result.exit_code == 0, ( + f"Expected exit code 0 but got {result.exit_code}.\nOutput:\n{result.output}" + ) + assert "Actor updated" in result.output, ( + f"Expected 'Actor updated' in output:\n{result.output}" + ) + print("actor-add-accept-update-ok") + + +def test_accept_new_without_update() -> None: + """Adding a brand-new actor without --update must exit 0 and show 'Actor added'.""" + actor_name = "local/new-actor" + config_path = _write_config(actor_name) + new_actor = _make_actor(name=actor_name) + + try: + runner = CliRunner() + with patch("cleveragents.cli.commands.actor._get_services") as mock_svc: + registry = MagicMock() + registry.get_actor.side_effect = NotFoundError( + resource_type="actor", resource_id=actor_name + ) + registry.upsert_actor.return_value = new_actor + mock_svc.return_value = (MagicMock(), registry) + result = runner.invoke( + actor_app, + ["add", actor_name, "--config", str(config_path)], + ) + finally: + config_path.unlink(missing_ok=True) + + assert result.exit_code == 0, ( + f"Expected exit code 0 but got {result.exit_code}.\nOutput:\n{result.output}" + ) + assert "Actor added" in result.output, ( + f"Expected 'Actor added' in output:\n{result.output}" + ) + print("actor-add-accept-new-ok") + + +def main() -> None: + command = sys.argv[1] if len(sys.argv) > 1 else "" + dispatch = { + "reject-existing-without-update": test_reject_existing_without_update, + "accept-existing-with-update": test_accept_existing_with_update, + "accept-new-without-update": test_accept_new_without_update, + } + fn = dispatch.get(command) + if fn is None: + print(f"Unknown command: {command!r}", file=sys.stderr) + sys.exit(1) + fn() + + +if __name__ == "__main__": + main()