Files
temp/features/steps/skill_cli_coverage_boost_steps.py
T
hurui200320 554d6889cc fix(cli): add --skill flag to actor run command (#971)
## Summary

Add the missing `--skill` repeatable flag to `actor run` and `actor-run` CLI commands, aligning the implementation with the specification (CLI Synopsis line 277). The flag enables ad-hoc skill injection at runtime without modifying YAML configuration.

Closes #887

## Changes

### DI Container
- **`container.py`**: Added `_build_skill_service()` factory and `skill_service` Singleton provider, following the established `_build_*` pattern. Falls back to in-memory `SkillService()` when the database is unavailable. Exception handling narrowed to `(ImportError, OperationalError, DatabaseError, OSError)` with `exc_info=True` for traceability.

### CLI Layer
- **`actor.py`**: Added `--skill` Typer option (`list[str] | None`, repeatable, `metavar="NAME"`). Help text notes that skills only augment tool-bearing agents. Wrapped constructor in the existing `try/except` block so `CleverAgentsException` from skill resolution is properly caught.
- **`actor_run.py`**: Same `--skill` option with `metavar="NAME"`. Exception handler catches `CleverAgentsException` (matching master — not broadened to `CleverAgentsError`).
- **`skill.py`**: Removed module-level `_service` cache. `_get_skill_service()` now always delegates to `get_container().skill_service()` so that `reset_container()` correctly invalidates the cached instance. `_reset_skill_service()` now overrides the container's provider via `providers.Object()`. Removed dead `validate_skill_names()` function.

### Runtime Layer
- **`application.py`** (438 lines, down from 625): `ReactiveCleverAgentsApp` gains `skill_names` parameter with automatic deduplication via `dict.fromkeys`. `_resolve_skills()` obtains `SkillService` from the DI container (no CLI layer import). Separate `except KeyError` and `except ValueError` produce distinct error messages (`"not found in registry"` vs `"resolution failed: {exc}"`). Skill tools are only injected into agents that already have tools (`if self._resolved_skill_tools and tools:`), preventing LLM agents from being converted to pass-through `SimpleToolAgent` instances. When skill tools are skipped for tool-less agents, `logger.debug` emits a diagnostic message. `_sanitize_skill_name()` validates skill name format with tightened regex: `^[\w.-]{1,127}/[\w.-]{1,127}$` with `re.ASCII` flag. Zero-tool skill warning now uses `logger.warning` (not `print(stderr)`), ensuring structured log output and proper log-level filtering.
- **`graph_executor.py`** (334 lines): Extracted graph execution logic. Type annotations improved.

### Tests
- 24+ Behave scenarios across feature files covering: single/multiple/unknown skill flags, skill+context combined, duplicate deduplication, skill resolution, ValueError path, zero-tool resolution, error handling, tool merging, default behavior, overrides, LLM agent guard, `_sanitize_skill_name` edge cases (empty string, too-long name, ANSI escape codes, disallowed characters), `_build_skill_service` happy+fallback paths, `_get_skill_service` container delegation.
- CLI "unknown skill" tests for **both** `actor.py` and `actor_run.py` exercise the real error chain (mock only `get_container()`, not the entire `ReactiveCleverAgentsApp`), testing `_resolve_skills()` → `CleverAgentsException` → `except CleverAgentsException` → exit code 2 end-to-end.
- Combined skill+context tests assert `ContextManager` was instantiated and `exists()` was called in dedicated **Then** steps.
- `@coverage` tags added to all new scenarios.
- **Robot Framework smoke tests** added (`robot/skill_actor_run.robot` + `robot/helper_skill_actor_run.py`): unknown-skill error path and valid-skill acceptance path.

### Changelog
- Added entry under `## Unreleased` in `CHANGELOG.md`.

## Review Fixes Applied (Brent Edwards, Rounds 1 & 2)

| # | Finding | Resolution |
|---|---------|------------|
| **P1-1** | `print(stderr)` for zero-tool skill warning | **Fixed** — replaced with `logger.warning("Skill '%s' resolved to zero tools", name)`, removed unused `import sys` |
| **P2-2** | Skill tools silently skipped for tool-less agents | **Fixed** — added `logger.debug` when skipped; updated `--skill` help text to note "only augments tool-bearing agents" |
| **P2-3** | `container.py` at 739 lines | **Acknowledged** — pre-existing growth (+59 lines for `_build_skill_service`); extracting factories is a separate refactoring task |
| **P2-4↑** | `CleverAgentsException` → `CleverAgentsError` broadens catch scope | **Fixed** — reverted `actor_run.py` to `except CleverAgentsException` matching master |
| **P3-5** | No Robot Framework smoke test for `--skill` | **Fixed** — added `skill_actor_run.robot` with 2 test cases (unknown-skill error, valid-skill acceptance) |
| **P3-6** | `GraphExecutor._follow_chained_edges` static-calling-static | **Acknowledged** — cosmetic pattern that doesn't affect correctness; can address in a follow-up |

## Known Limitations / Deferred Items

| Item | Reason |
|------|--------|
| `actor.py` at 679 lines (500-line guideline) | Pre-existing (670 on master), +9 lines for `--skill`. Refactoring the shared `_execute()` closure is a separate task. |
| `container.py` at 739 lines (500-line guideline) | Was 680 lines on master, +59 lines for `_build_skill_service()` and `skill_service` provider. Refactoring into sub-modules is a separate task. |
| Code duplication between `actor.py` and `actor_run.py` `run()` | ~47 lines identical code. Coupled with the line-count issue above — both require extracting shared execution logic into a helper module. |
| `SimpleToolAgent` only executes `tools[0]` | Deferred to #974. Pre-existing architectural limitation, not introduced by this PR. |
| `GraphExecutor._follow_chained_edges` static-calling-static pattern | Cosmetic, doesn't affect behavior. |

## Quality Gates
- `nox -s lint`:  PASS
- `nox -s typecheck`:  PASS (0 errors)
- `nox -s unit_tests`:  PASS (11,130 scenarios, 0 failures)
- `nox -s integration_tests`:  PASS (1,559 tests, 0 failures)
- `nox -s coverage_report`:  97% (meets threshold)
- Branch rebased onto latest `master` (`ab1fd19b`)

Reviewed-on: cleveragents/cleveragents-core#971
Co-authored-by: Rui Hu <rui.hu@cleverthis.com>
Co-committed-by: Rui Hu <rui.hu@cleverthis.com>
2026-03-19 09:30:35 +00:00

309 lines
11 KiB
Python

# pyright: reportRedeclaration=false
"""Step definitions for skill_cli_coverage_boost.feature.
Targets the remaining uncovered lines in
``cleveragents.cli.commands.skill``:
- Lines 82-103: ``_get_skill_service()`` DB initialisation and fallback
- Line 889: ``tools`` non-rich ``source_type = "agent_skills"``
- Lines 984-985: ``refresh`` defensive ``name is None`` guard
All step text uses the ``boost-`` prefix to avoid collisions.
"""
from __future__ import annotations
import json
from typing import Any
from unittest.mock import MagicMock, patch
from behave import given, then, when
from behave.runner import Context
from typer.testing import CliRunner
import cleveragents.cli.commands.skill as skill_mod
from cleveragents.application.services.skill_service import SkillService
from cleveragents.cli.commands.skill import (
_get_skill_service,
_reset_skill_service,
)
from cleveragents.cli.commands.skill import app as skill_app
from cleveragents.domain.models.core.skill import (
ResolvedToolEntry,
Skill,
SkillAgentSource,
)
# ── helpers ─────────────────────────────────────────────────
def _make_skill(
name: str,
description: str = "test skill",
tool_refs: list[str] | None = None,
agent_skills: list[SkillAgentSource] | None = None,
) -> Skill:
"""Create a Skill domain object."""
return Skill(
name=name,
description=description,
tool_refs=tool_refs or [],
includes=[],
mcp_servers=[],
agent_skills=agent_skills or [],
anonymous_tools=[],
)
def _register_skill(context: Context, skill: Skill) -> None:
"""Register a skill in the service with timestamps."""
from datetime import datetime
now = datetime.now()
context.boost_service._skills[skill.name] = skill
context.boost_service._created_at[skill.name] = now
context.boost_service._updated_at[skill.name] = now
# ── Background ──────────────────────────────────────────────
@given("boost- a fresh skill CLI service")
def step_boost_background(context: Context) -> None:
"""Reset module-level singleton and prepare runner/service."""
_reset_skill_service()
context.boost_runner = CliRunner()
context.boost_service = _get_skill_service()
context.boost_result = None
context.boost_patches: list[Any] = []
context.boost_returned_service = None
context.boost_guard_printed = False
context.boost_guard_aborted = False
# ── Given: _get_skill_service delegates to container ────────
@given("boost- the module-level _service is set to None")
def step_boost_set_service_none(context: Context) -> None:
"""No-op — _get_skill_service always delegates to the container now."""
@given("boost- the DI container returns a DB-backed SkillService")
def step_boost_mock_container_db_service(context: Context) -> None:
"""Mock get_container().skill_service() to return a SkillService with a repo."""
mock_skill_repo = MagicMock()
mock_service = SkillService(skill_repo=mock_skill_repo)
mock_container = MagicMock()
mock_container.skill_service.return_value = mock_service
p1 = patch(
"cleveragents.cli.commands.skill.get_container",
return_value=mock_container,
)
p1.start()
context.boost_patches.append(p1)
context.boost_mock_container = mock_container
# ── Given: tools with agent_skill entries ───────────────────
@given('boost- a registered skill "{name}" with agent_skills')
def step_boost_register_skill_with_agent_skills(context: Context, name: str) -> None:
"""Register a skill that has agent_skills entries."""
skill = _make_skill(
name=name,
agent_skills=[SkillAgentSource(path="/tmp/my-agent-skill")],
)
_register_skill(context, skill)
# Patch resolve_tools to return entries with agent_skill: prefix
def patched_resolve(skill_name: str) -> tuple[Skill, list[ResolvedToolEntry]]:
sk = context.boost_service.get_skill(skill_name)
entries = [
ResolvedToolEntry(
name="agent_skill:/tmp/my-agent-skill",
source_skill=skill_name,
is_inline=False,
),
]
return sk, entries
p = patch.object(
context.boost_service, "resolve_tools", side_effect=patched_resolve
)
p.start()
context.boost_patches.append(p)
# ── When: _get_skill_service ────────────────────────────────
@when("boost- I call _get_skill_service")
def step_boost_call_get_skill_service(context: Context) -> None:
"""Call _get_skill_service and store the result."""
try:
context.boost_returned_service = _get_skill_service()
finally:
_stop_patches(context)
@when("boost- I call _get_skill_service twice")
def step_boost_call_get_skill_service_twice(context: Context) -> None:
"""Call _get_skill_service twice to test caching."""
try:
context.boost_returned_service = _get_skill_service()
_get_skill_service() # second call should use cached
finally:
_stop_patches(context)
# ── When: tools command ─────────────────────────────────────
@when('boost- I invoke tools "{name}" in format "{fmt}"')
def step_boost_invoke_tools_fmt(context: Context, name: str, fmt: str) -> None:
"""Invoke skill tools with a specified format."""
context.boost_result = context.boost_runner.invoke(
skill_app, ["tools", name, "--format", fmt]
)
_stop_patches(context)
# ── When: refresh defensive guard ───────────────────────────
@when(
"boost- I call refresh directly with name None and all_skills False bypassing first guard"
)
def step_boost_refresh_bypass_first_guard(context: Context) -> None:
"""Exercise the refresh function with name=None, all_skills=False.
The first guard (lines 971-973) catches this and aborts.
Lines 984-985 are a defensive duplicate guard that can only fire if
the first guard is somehow bypassed. We verify the first guard fires
correctly (the observable behaviour), which is the intended test.
"""
import typer
# Track what console.print receives
p_print = patch.object(skill_mod, "console")
mock_console = p_print.start()
context.boost_patches.append(p_print)
try:
from cleveragents.cli.commands.skill import refresh as refresh_fn
refresh_fn(name=None, all_skills=False, fmt="rich")
except (typer.Abort, SystemExit):
context.boost_guard_aborted = True
# Check if the error message was printed
for call_args in mock_console.print.call_args_list:
if call_args and call_args[0]:
msg = str(call_args[0][0])
if "Must specify either" in msg:
context.boost_guard_printed = True
_stop_patches(context)
# ── Then: _get_skill_service assertions ─────────────────────
@then("boost- the returned service should have a skill_repo")
def step_boost_service_has_repo(context: Context) -> None:
"""Assert the returned service was created with a skill_repo."""
svc = context.boost_returned_service
assert svc is not None, "No service was returned"
assert svc._skill_repo is not None, (
"Expected service to have a skill_repo (DB-backed), but it was None"
)
@then("boost- the container skill_service should be called once")
def step_boost_container_called_once(context: Context) -> None:
"""Assert get_container().skill_service() was called only once (caching)."""
context.boost_mock_container.skill_service.assert_called_once()
@then("boost- the container skill_service should be called twice")
def step_boost_container_called_twice(context: Context) -> None:
"""Assert get_container().skill_service() is called on every access (no cache)."""
assert context.boost_mock_container.skill_service.call_count == 2
# ── Then: CLI exit code ─────────────────────────────────────
@then("boost- the CLI exit code should be 0")
def step_boost_exit_code_0(context: Context) -> None:
"""Assert CLI exited successfully."""
assert context.boost_result is not None
assert context.boost_result.exit_code == 0, (
f"Expected exit_code=0, got {context.boost_result.exit_code}\n"
f"Output: {context.boost_result.output}"
)
# ── Then: JSON output assertions ────────────────────────────
@then("boost- the JSON output should be valid")
def step_boost_json_valid(context: Context) -> None:
"""Assert the CLI output parses as valid JSON."""
assert context.boost_result is not None
try:
context.boost_parsed_json = json.loads(context.boost_result.output)
except json.JSONDecodeError as e:
raise AssertionError(
f"Output is not valid JSON: {e}\nOutput: {context.boost_result.output}"
) from e
@then('boost- the JSON tools list should contain an entry with source "{source}"')
def step_boost_json_tools_has_source(context: Context, source: str) -> None:
"""Assert at least one tool entry has the given source type."""
data = context.boost_parsed_json
assert isinstance(data, dict), f"Expected dict, got {type(data).__name__}"
tools_list = data.get("tools", [])
assert len(tools_list) > 0, "No tools in JSON output"
sources_found = [t.get("source") for t in tools_list]
assert source in sources_found, (
f"Expected source '{source}' in tools list, found sources: {sources_found}"
)
# ── Then: refresh defensive guard assertions ────────────────
@then("boost- the second guard should have printed the error message")
def step_boost_guard_printed(context: Context) -> None:
"""Assert the guard printed 'Must specify either' error message."""
assert context.boost_guard_printed, (
"Expected the guard to have printed 'Must specify either' error message"
)
@then("boost- the function should have raised Abort")
def step_boost_guard_aborted(context: Context) -> None:
"""Assert that Abort was raised from the guard."""
assert context.boost_guard_aborted, "Expected typer.Abort to have been raised"
# ── Cleanup helper ──────────────────────────────────────────
def _stop_patches(context: Context) -> None:
"""Stop all active patches after a When step."""
import contextlib
for p in getattr(context, "boost_patches", []):
with contextlib.suppress(RuntimeError):
p.stop()
context.boost_patches = []
# Reset the module-level service to avoid leaking state
_reset_skill_service()