fix(actors): enforce --update flag in agents actor add - reject re-adding existing actor without --update
CI / lint (pull_request) Successful in 22s
CI / typecheck (pull_request) Successful in 57s
CI / security (pull_request) Successful in 54s
CI / quality (pull_request) Successful in 35s
CI / build (pull_request) Successful in 25s
CI / helm (pull_request) Successful in 23s
CI / unit_tests (pull_request) Failing after 6m58s
CI / docker (pull_request) Has been skipped
CI / e2e_tests (pull_request) Successful in 18m30s
CI / integration_tests (pull_request) Failing after 23m24s
CI / coverage (pull_request) Successful in 10m59s
CI / status-check (pull_request) Failing after 1s
CI / benchmark-publish (pull_request) Has been skipped
CI / benchmark-regression (pull_request) Successful in 58m21s
CI / lint (pull_request) Successful in 22s
CI / typecheck (pull_request) Successful in 57s
CI / security (pull_request) Successful in 54s
CI / quality (pull_request) Successful in 35s
CI / build (pull_request) Successful in 25s
CI / helm (pull_request) Successful in 23s
CI / unit_tests (pull_request) Failing after 6m58s
CI / docker (pull_request) Has been skipped
CI / e2e_tests (pull_request) Successful in 18m30s
CI / integration_tests (pull_request) Failing after 23m24s
CI / coverage (pull_request) Successful in 10m59s
CI / status-check (pull_request) Failing after 1s
CI / benchmark-publish (pull_request) Has been skipped
CI / benchmark-regression (pull_request) Successful in 58m21s
Before calling upsert_actor(), check if the actor already exists using registry.get_actor() or service.get_actor(). If the actor exists and update_existing is False, print the spec-required error panel and exit with error code 1. Error panel includes: - Actor name - Registration timestamp (formatted as YYYY-MM-DD HH:MM) - Hint to use --update flag Adds Behave tests for: - Duplicate-without-update failure case (exit code 1, error panel shown) - Duplicate-with-update success case (exit code 0, actor updated) - New actor without --update success case (exit code 0, actor added) ISSUES CLOSED: #2609
This commit is contained in:
@@ -0,0 +1,37 @@
|
||||
# Regression tests for bug #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
|
||||
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
|
||||
Then the actor-add-enforcement exit code should be 1
|
||||
And the actor-add-enforcement output should contain an error panel with "Actor already exists"
|
||||
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
|
||||
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
|
||||
Then the actor-add-enforcement exit code should be 1
|
||||
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
|
||||
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
|
||||
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
|
||||
Then the actor-add-enforcement exit code should be 0
|
||||
And the actor-add-enforcement output should contain "Actor added"
|
||||
@@ -0,0 +1,156 @@
|
||||
"""Step definitions for actor add --update flag enforcement (issue #2609).
|
||||
|
||||
Tests that `agents actor add` rejects re-adding an existing actor without
|
||||
the --update flag, and succeeds when --update is provided.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
import tempfile
|
||||
from datetime import datetime
|
||||
from pathlib import Path
|
||||
from typing import Any
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
from behave import given, then, when
|
||||
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 _register_cleanup(context: Any, path: Path) -> None:
|
||||
context._cleanup_handlers.append(lambda: path.unlink(missing_ok=True))
|
||||
|
||||
|
||||
def _write_actor_config(name: str = "local/existing-actor") -> Path:
|
||||
config_data = {
|
||||
"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)
|
||||
|
||||
|
||||
@given("an actor add CLI runner where the actor already exists")
|
||||
def step_given_actor_exists(context: Any) -> None:
|
||||
context.runner = CliRunner()
|
||||
context.actor_name = "local/existing-actor"
|
||||
context.actor_config_path = _write_actor_config(context.actor_name)
|
||||
_register_cleanup(context, context.actor_config_path)
|
||||
|
||||
# The existing actor that will be returned by get_actor
|
||||
context.existing_actor = _make_actor(
|
||||
name=context.actor_name,
|
||||
updated_at=datetime(2026, 2, 7, 14, 22, 0),
|
||||
)
|
||||
# The actor returned after a successful upsert (for --update case)
|
||||
context.updated_actor = _make_actor(
|
||||
name=context.actor_name,
|
||||
updated_at=datetime(2026, 2, 7, 14, 22, 0),
|
||||
)
|
||||
|
||||
|
||||
@given("an actor add CLI runner where the actor does not exist")
|
||||
def step_given_actor_not_exists(context: Any) -> None:
|
||||
context.runner = CliRunner()
|
||||
context.actor_name = "local/new-actor"
|
||||
context.actor_config_path = _write_actor_config(context.actor_name)
|
||||
_register_cleanup(context, context.actor_config_path)
|
||||
|
||||
context.new_actor = _make_actor(name=context.actor_name)
|
||||
|
||||
|
||||
@when("I run actor add without the --update flag")
|
||||
def step_when_add_without_update(context: Any) -> None:
|
||||
with patch("cleveragents.cli.commands.actor._get_services") as mock_svc:
|
||||
registry = MagicMock()
|
||||
if hasattr(context, "existing_actor"):
|
||||
# Actor exists — get_actor returns it, triggering the error path
|
||||
registry.get_actor.return_value = context.existing_actor
|
||||
else:
|
||||
# Actor does not exist — get_actor raises NotFoundError
|
||||
registry.get_actor.side_effect = NotFoundError(
|
||||
resource_type="actor", resource_id=context.actor_name
|
||||
)
|
||||
registry.upsert_actor.return_value = context.new_actor
|
||||
mock_svc.return_value = (MagicMock(), registry)
|
||||
context.result = context.runner.invoke(
|
||||
actor_app,
|
||||
["add", "--config", str(context.actor_config_path)],
|
||||
)
|
||||
|
||||
|
||||
@when("I run actor add with the --update flag")
|
||||
def step_when_add_with_update(context: Any) -> None:
|
||||
with patch("cleveragents.cli.commands.actor._get_services") as mock_svc:
|
||||
registry = MagicMock()
|
||||
# get_actor returns the existing actor (simulating actor already exists)
|
||||
registry.get_actor.return_value = context.existing_actor
|
||||
# upsert_actor returns the updated actor
|
||||
registry.upsert_actor.return_value = context.updated_actor
|
||||
mock_svc.return_value = (MagicMock(), registry)
|
||||
context.result = context.runner.invoke(
|
||||
actor_app,
|
||||
["add", "--config", str(context.actor_config_path), "--update"],
|
||||
)
|
||||
|
||||
|
||||
@then("the actor-add-enforcement exit code should be {code:d}")
|
||||
def step_then_exit_code(context: Any, code: int) -> None:
|
||||
assert context.result.exit_code == code, (
|
||||
f"Expected exit code {code}, got {context.result.exit_code}.\n"
|
||||
f"Output:\n{context.result.output}"
|
||||
)
|
||||
|
||||
|
||||
@then('the actor-add-enforcement output should contain an error panel with "{text}"')
|
||||
def step_then_output_contains_error_panel(context: Any, text: str) -> None:
|
||||
output = context.result.output
|
||||
assert text in output, f"Expected '{text}' in output:\n{output}"
|
||||
assert "Error" in output, f"Expected 'Error' panel title in output:\n{output}"
|
||||
|
||||
|
||||
@then('the actor-add-enforcement output should contain "{text}"')
|
||||
def step_then_output_contains(context: Any, text: str) -> None:
|
||||
output = context.result.output
|
||||
assert text in output, f"Expected '{text}' in output:\n{output}"
|
||||
|
||||
|
||||
@then("the actor-add-enforcement output should contain the registration timestamp")
|
||||
def step_then_output_contains_timestamp(context: Any) -> None:
|
||||
output = context.result.output
|
||||
# The timestamp should be formatted as "YYYY-MM-DD HH:MM"
|
||||
assert "2026-02-07 14:22" in output, (
|
||||
f"Expected registration timestamp '2026-02-07 14:22' in output:\n{output}"
|
||||
)
|
||||
@@ -493,6 +493,28 @@ def add(
|
||||
|
||||
_print_role_warnings(canonical_blob)
|
||||
|
||||
# ── Enforce --update flag: reject re-adding an existing actor ─────────────
|
||||
if not update_existing:
|
||||
try:
|
||||
existing = registry.get_actor(name) if registry else service.get_actor(name)
|
||||
# Actor already exists and --update was not provided — reject with error
|
||||
registered_ts = existing.updated_at.strftime("%Y-%m-%d %H:%M")
|
||||
error_details = (
|
||||
f"Actor already exists: {name}\n"
|
||||
f"Registered: {registered_ts}\n"
|
||||
f"Use --update to replace the existing actor definition."
|
||||
)
|
||||
console.print(Panel(error_details, title="Error", border_style="red"))
|
||||
console.print(
|
||||
"[red bold]✗ ERROR[/red bold] Actor already registered"
|
||||
" \u2014 use --update to replace"
|
||||
)
|
||||
raise typer.Exit(code=1)
|
||||
except typer.Exit:
|
||||
raise
|
||||
except NotFoundError:
|
||||
pass # Actor does not exist yet — proceed with add
|
||||
|
||||
try:
|
||||
if registry:
|
||||
actor = registry.upsert_actor(
|
||||
|
||||
Reference in New Issue
Block a user