fix(cli): repair _parse_option_overrides and actor BDD test setup
The prior insertion of _resolve_actor_name split the body of
_parse_option_overrides: lines 333-336 only initialised `overrides`
and returned early on empty input, while the for-loop that processes
key=value pairs ended up indented inside _resolve_actor_name AFTER
its own return statement, making it unreachable dead code. The net
effect was that `agents actor add --option key=value` silently
discarded every override. Moved _resolve_actor_name below
_parse_option_overrides so each function's body is intact.
Also addressed three test-only issues uncovered while verifying the
fix:
- features/actor_add_name_from_config.feature: the last scenario
read "...as positional argument and --config flag" which did not
match the registered step "...as positional argument" and raised
Behave UndefinedStep. Trimmed the suffix.
- features/steps/actor_add_name_from_config_steps.py: removed a
duplicate `@given("an actor CLI runner")` definition (already
registered in features/steps/actor_cli_steps.py) that triggered
behave.step_registry.AmbiguousStep at module load. Dropped the
now-unused CliRunner import. Switched four NamedTemporaryFile
call sites to context managers (SIM115).
- Test YAML for `actor add reads name from YAML config` lacked the
`description` field that ActorConfigSchema requires whenever v3
detection fires (the `type: llm` line triggers it). Added it so
the v3 schema validates instead of erroring with exit_code=2.
Verified locally with the targeted gates flagged by CI:
lint PASS
typecheck PASS
unit_tests features/actor_add_name_from_config.feature PASS
(5/5 scenarios, 22 steps)
ISSUES CLOSED: #11047
This commit is contained in:
Forgejo
@@ -35,5 +35,5 @@ Feature: agents actor add reads actor name from YAML config file
|
||||
@tdd_issue @tdd_issue_11047
|
||||
Scenario: actor update still accepts positional NAME (names registered actor to modify)
|
||||
Given a registered actor "local/existing-actor" already exists in the registry
|
||||
When I run actor update with "local/existing-actor" as positional argument and --config flag
|
||||
When I run actor update with "local/existing-actor" as positional argument
|
||||
Then the actor update should succeed for that registered actor
|
||||
@@ -10,7 +10,6 @@ from unittest.mock import MagicMock, patch
|
||||
|
||||
import yaml
|
||||
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
|
||||
@@ -47,21 +46,22 @@ def _register_cleanup(context: Any, path: Path) -> None:
|
||||
|
||||
# Given Steps
|
||||
|
||||
@given("an actor CLI runner")
|
||||
def step_actor_cli_runner(context):
|
||||
context.runner = CliRunner()
|
||||
|
||||
|
||||
@given('I have an actor YAML config file with name field "{name}"')
|
||||
def step_yaml_config_with_name(context, name):
|
||||
yaml_content = f"name: {name}\nprovider: openai\nmodel: gpt-4o-mini\ntype: llm\n"
|
||||
context.actor_config_data = {"name": name}
|
||||
handle = tempfile.NamedTemporaryFile(
|
||||
delete=False, suffix=".yaml", mode="w", encoding="utf-8"
|
||||
yaml_content = (
|
||||
f"name: {name}\n"
|
||||
"type: llm\n"
|
||||
"description: Test actor created by BDD scenario\n"
|
||||
"provider: openai\n"
|
||||
"model: gpt-4o-mini\n"
|
||||
)
|
||||
handle.write(yaml_content)
|
||||
handle.flush()
|
||||
context.actor_config_path = Path(handle.name)
|
||||
context.actor_config_data = {"name": name}
|
||||
with tempfile.NamedTemporaryFile(
|
||||
delete=False, suffix=".yaml", mode="w", encoding="utf-8"
|
||||
) as handle:
|
||||
handle.write(yaml_content)
|
||||
context.actor_config_path = Path(handle.name)
|
||||
_register_cleanup(context, context.actor_config_path)
|
||||
|
||||
|
||||
@@ -72,12 +72,11 @@ def step_json_config_with_name(context, name):
|
||||
"provider": "openai",
|
||||
"model": "gpt-4o-mini",
|
||||
}
|
||||
handle = tempfile.NamedTemporaryFile(
|
||||
with tempfile.NamedTemporaryFile(
|
||||
delete=False, suffix=".json", mode="w", encoding="utf-8"
|
||||
)
|
||||
json.dump(context.actor_config_data, handle)
|
||||
handle.flush()
|
||||
context.actor_config_path = Path(handle.name)
|
||||
) as handle:
|
||||
json.dump(context.actor_config_data, handle)
|
||||
context.actor_config_path = Path(handle.name)
|
||||
_register_cleanup(context, context.actor_config_path)
|
||||
|
||||
|
||||
@@ -87,44 +86,43 @@ def step_mock_registry_ready(context):
|
||||
pass
|
||||
|
||||
|
||||
@given('I have an actor YAML config file without a name field')
|
||||
@given("I have an actor YAML config file without a name field")
|
||||
def step_yaml_no_name_field(context):
|
||||
context.actor_config_data = {
|
||||
"provider": "openai",
|
||||
"model": "gpt-4o-mini",
|
||||
}
|
||||
handle = tempfile.NamedTemporaryFile(
|
||||
with tempfile.NamedTemporaryFile(
|
||||
delete=False, suffix=".yaml", mode="w", encoding="utf-8"
|
||||
)
|
||||
handle.write(yaml.dump(context.actor_config_data))
|
||||
handle.flush()
|
||||
context.actor_config_path = Path(handle.name)
|
||||
) as handle:
|
||||
handle.write(yaml.dump(context.actor_config_data))
|
||||
context.actor_config_path = Path(handle.name)
|
||||
_register_cleanup(context, context.actor_config_path)
|
||||
|
||||
|
||||
@given('I have an actor YAML config file with null name')
|
||||
@given("I have an actor YAML config file with null name")
|
||||
def step_yaml_null_name_field(context):
|
||||
context.actor_config_data = {
|
||||
"name": None,
|
||||
"provider": "openai",
|
||||
"model": "gpt-4o-mini",
|
||||
}
|
||||
handle = tempfile.NamedTemporaryFile(
|
||||
with tempfile.NamedTemporaryFile(
|
||||
delete=False, suffix=".yaml", mode="w", encoding="utf-8"
|
||||
)
|
||||
handle.write(yaml.dump(context.actor_config_data))
|
||||
handle.flush()
|
||||
context.actor_config_path = Path(handle.name)
|
||||
) as handle:
|
||||
handle.write(yaml.dump(context.actor_config_data))
|
||||
context.actor_config_path = Path(handle.name)
|
||||
_register_cleanup(context, context.actor_config_path)
|
||||
|
||||
|
||||
@given('a pre-existing {name} actor is in the registry')
|
||||
@given("a pre-existing {name} actor is in the registry")
|
||||
def step_existing_actor_registered(context, name):
|
||||
context.existing_actor_name = name
|
||||
|
||||
|
||||
# When Steps
|
||||
|
||||
|
||||
@when("I run actor add with only --config flag pointing to the config file")
|
||||
def step_run_add_with_config_only(context):
|
||||
mock_actor = _make_actor(
|
||||
@@ -192,9 +190,8 @@ def step_run_update_with_name(context, name):
|
||||
|
||||
# Then Steps
|
||||
|
||||
@then(
|
||||
'the actor add should succeed and register "{name}" from config filename'
|
||||
)
|
||||
|
||||
@then('the actor add should succeed and register "{name}" from config filename')
|
||||
def step_add_succeeds_from_config_name(context, name):
|
||||
assert context.result.exit_code == 0, (
|
||||
f"Expected exit_code=0, got {context.result.exit_code}.\n"
|
||||
@@ -202,9 +199,7 @@ def step_add_succeeds_from_config_name(context, name):
|
||||
)
|
||||
call_kwargs = context.mock_registry.upsert_actor.call_args.kwargs
|
||||
actual_name = call_kwargs.get("name")
|
||||
assert actual_name == name, (
|
||||
f"Expected name={name!r}, got {actual_name!r}"
|
||||
)
|
||||
assert actual_name == name, f"Expected name={name!r}, got {actual_name!r}"
|
||||
|
||||
|
||||
@then('the actor command should fail with "{text}"')
|
||||
@@ -214,9 +209,7 @@ def step_add_fails_with(context, text):
|
||||
f"Output:\n{context.result.output}"
|
||||
)
|
||||
output = context.result.output.lower()
|
||||
assert text.lower() in output, (
|
||||
f"Expected '{text}' in output but got:\n{output}"
|
||||
)
|
||||
assert text.lower() in output, f"Expected '{text}' in output but got:\n{output}"
|
||||
|
||||
|
||||
@then("the actor update should succeed for that registered actor")
|
||||
|
||||
Reference in New Issue
Block a user