cleveragents/cleveragents-core

Fork 3

fix(cli): add missing `--skill` flag to `actor run` #887

New issue

Closed

opened 2026-03-13 23:26:48 +00:00 by freemo · 5 comments

freemo commented

2026-03-13 23:26:48 +00:00

Owner

Metadata

Commit Message: fix(cli): add --skill flag to actor run command
Branch: feature/m4-actor-run-skill

Background and Context

The specification defines agents actor run with a --skill <SKILL> repeatable flag (CLI Synopsis line 277) that allows injecting additional skills into the actor at runtime. This enables ad-hoc capability composition without modifying the actor's YAML configuration.

Neither src/cleveragents/cli/commands/actor_run.py nor src/cleveragents/cli/commands/actor.py includes a --skill flag. Both files define a run command with flags --output, -v, --unsafe, --context, --context-dir, --load-context, --temperature, and --allow-rxpy-in-run-mode — but --skill is absent from both.

Spec reference: [--skill <SKILL>]... on agents actor run

Expected Behavior

agents actor run --skill local/web-tools --skill local/db-tools myactor "Do something" should resolve and attach the named skills to the actor for the duration of that run invocation.

Acceptance Criteria

--skill flag is added to actor run command (repeatable via list[str])
Named skills are resolved from the Skill Registry at runtime
Resolved skill tools are merged into the actor's available toolset for the run
Error if a named skill does not exist in the registry
Multiple --skill flags are supported in a single invocation
Flag is optional; omitting it uses only the actor's configured skills

Subtasks

Add --skill Typer option to actor_run.py run command
Add --skill Typer option to actor.py run command (if both remain)
Pass skill names to actor runtime for resolution and injection
Integrate with SkillRegistryService for runtime resolution
Tests (Behave): Scenarios for skill injection, unknown skill error, multiple skills
Verify coverage >= 97% via nox -s coverage_report
Run nox (all default sessions), fix any errors

Definition of Done

This issue is complete when:

All subtasks above are completed and checked off.
A Git commit is created where the first line of the commit message matches the Commit Message in Metadata exactly, followed by a blank line, then additional lines providing relevant details about the implementation.
The commit is pushed to the remote on the branch matching the Branch in Metadata exactly.
The commit is submitted as a pull request to master, reviewed, and merged before this issue is marked done.

## Metadata - **Commit Message**: `fix(cli): add --skill flag to actor run command` - **Branch**: `feature/m4-actor-run-skill` ## Background and Context The specification defines `agents actor run` with a `--skill <SKILL>` repeatable flag (CLI Synopsis line 277) that allows injecting additional skills into the actor at runtime. This enables ad-hoc capability composition without modifying the actor's YAML configuration. Neither `src/cleveragents/cli/commands/actor_run.py` nor `src/cleveragents/cli/commands/actor.py` includes a `--skill` flag. Both files define a `run` command with flags `--output`, `-v`, `--unsafe`, `--context`, `--context-dir`, `--load-context`, `--temperature`, and `--allow-rxpy-in-run-mode` — but `--skill` is absent from both. **Spec reference**: `[--skill <SKILL>]...` on `agents actor run` ## Expected Behavior `agents actor run --skill local/web-tools --skill local/db-tools myactor "Do something"` should resolve and attach the named skills to the actor for the duration of that run invocation. ## Acceptance Criteria - [x] `--skill` flag is added to `actor run` command (repeatable via `list[str]`) - [x] Named skills are resolved from the Skill Registry at runtime - [x] Resolved skill tools are merged into the actor's available toolset for the run - [x] Error if a named skill does not exist in the registry - [x] Multiple `--skill` flags are supported in a single invocation - [x] Flag is optional; omitting it uses only the actor's configured skills ## Subtasks - [x] Add `--skill` Typer option to `actor_run.py` `run` command - [x] Add `--skill` Typer option to `actor.py` `run` command (if both remain) - [x] Pass skill names to actor runtime for resolution and injection - [x] Integrate with `SkillRegistryService` for runtime resolution - [x] Tests (Behave): Scenarios for skill injection, unknown skill error, multiple skills - [x] Verify coverage >= 97% via `nox -s coverage_report` - [x] Run `nox` (all default sessions), fix any errors ## Definition of Done This issue is complete when: - All subtasks above are completed and checked off. - A Git commit is created where the **first line** of the commit message matches the Commit Message in Metadata exactly, followed by a blank line, then additional lines providing relevant details about the implementation. - The commit is pushed to the remote on the branch matching the **Branch** in Metadata exactly. - The commit is submitted as a **pull request** to `master`, reviewed, and **merged** before this issue is marked done.

freemo added the

labels

2026-03-13 23:30:22 +00:00

freemo added this to the v3.4.0 milestone

2026-03-13 23:30:30 +00:00

freemo referenced this issue

2026-03-13 23:47:43 +00:00

refactor(cli): align actor run command signature with specification positional arguments #901

freemo referenced this issue

2026-03-13 23:48:50 +00:00

refactor(cli): align actor run command signature with specification positional arguments #901

hurui200320 was assigned by freemo

2026-03-14 04:26:59 +00:00

hurui200320 added

and removed

labels

2026-03-16 07:05:01 +00:00

hurui200320 commented

2026-03-16 07:05:25 +00:00

Member

Implementation Plan

Branch: feature/m4-actor-run-skill created from master

Design Decisions

CLI layer validation: The --skill flag is added to both actor_run.py and actor.py run commands as a repeatable list[str] Typer option. Skill names are validated against the SkillService before the ReactiveCleverAgentsApp is created. This follows the pattern established in skill.py CLI commands.
Skill validation approach: The CLI reuses _get_skill_service() from cleveragents.cli.commands.skill to obtain a DB-backed SkillService singleton. Each skill name is validated via SkillService.get_skill() -- a KeyError triggers a user-facing error and typer.Exit(code=2).
Runtime integration: ReactiveCleverAgentsApp gains a skill_names: list[str] | None constructor parameter. During init, if skill names are present, the app resolves them via a lazily-obtained SkillService and stores the resolved ResolvedToolEntry items as tool config dicts. These are merged into each agent's tool list during _register_agents_from_config().
Tool merging strategy: Resolved skill tool entries are converted to tool config dicts (with "operation": "identity" baseline and "skill_source" metadata). These are appended to each agent's existing tool list, satisfying "Resolved skill tools are merged into the actor's available toolset for the run."
Test strategy: Behave scenarios added for both actor.py and actor_run.py covering: single skill, multiple skills, unknown skill error. Tests mock _get_skill_service, ReactiveCleverAgentsApp, and verify correct argument passing and error handling.

## Implementation Plan **Branch**: `feature/m4-actor-run-skill` created from `master` ### Design Decisions 1. **CLI layer validation**: The `--skill` flag is added to both `actor_run.py` and `actor.py` `run` commands as a repeatable `list[str]` Typer option. Skill names are validated against the `SkillService` before the `ReactiveCleverAgentsApp` is created. This follows the pattern established in `skill.py` CLI commands. 2. **Skill validation approach**: The CLI reuses `_get_skill_service()` from `cleveragents.cli.commands.skill` to obtain a DB-backed `SkillService` singleton. Each skill name is validated via `SkillService.get_skill()` -- a `KeyError` triggers a user-facing error and `typer.Exit(code=2)`. 3. **Runtime integration**: `ReactiveCleverAgentsApp` gains a `skill_names: list[str] | None` constructor parameter. During init, if skill names are present, the app resolves them via a lazily-obtained `SkillService` and stores the resolved `ResolvedToolEntry` items as tool config dicts. These are merged into each agent's tool list during `_register_agents_from_config()`. 4. **Tool merging strategy**: Resolved skill tool entries are converted to tool config dicts (with `"operation": "identity"` baseline and `"skill_source"` metadata). These are appended to each agent's existing tool list, satisfying "Resolved skill tools are merged into the actor's available toolset for the run." 5. **Test strategy**: Behave scenarios added for both `actor.py` and `actor_run.py` covering: single skill, multiple skills, unknown skill error. Tests mock `_get_skill_service`, `ReactiveCleverAgentsApp`, and verify correct argument passing and error handling.

hurui200320 commented

2026-03-16 07:59:12 +00:00

Member

Implementation Complete

Changes Made

CLI layer (src/cleveragents/cli/commands/actor_run.py, src/cleveragents/cli/commands/actor.py):

Added --skill Typer option to the run command in both modules. The option is repeatable (list[str] | None) and defaults to None.
Added _validate_skill_names() function in each module that validates skill existence via SkillService.get_skill(). Uses _get_skill_service() from the skill.py module (DB-backed singleton) for lookup. On KeyError, emits a user-friendly error to stderr and raises typer.Exit(code=2).
Validation runs before ReactiveCleverAgentsApp creation. Validated skill names are passed via the new skill_names constructor parameter.

Runtime layer (src/cleveragents/reactive/application.py):

ReactiveCleverAgentsApp.__init__ gains skill_names: list[str] | None = None parameter.
New _resolve_skills() method resolves skill names to ResolvedToolEntry lists via SkillService.resolve_tools(). Each entry is converted to a tool config dict ({"name", "source_skill", "is_inline", "operation": "identity", "skill_source"}). On unknown skill, raises CleverAgentsException.
New _create_skill_service() static method creates a SkillService with optional DB persistence (same pattern as skill.py).
New skill_names and resolved_skill_tools read-only properties.
_register_agents_from_config()._make_agent_instance() merges _resolved_skill_tools into each agent's tool list before creating SimpleToolAgent.

Test Coverage

Feature scenarios added (10 new scenarios, 7 for CLI + 3 for reactive app + 1 for default behavior):

features/actor_cli_coverage.feature:

"Run actor with single skill flag"
"Run actor with multiple skill flags"
"Run actor with unknown skill flag"

features/actor_run_cli_coverage.feature:

"Actor run command with single skill flag"
"Actor run command with multiple skill flags"
"Actor run command with unknown skill flag"

features/reactive_application_coverage_boost.feature:

"Reactive app resolves skill names and stores tools"
"Reactive app raises error for unknown skill name"
"Reactive app merges skill tools into agent tool list"
"Reactive app works without skill names"

Quality Gate Results

nox -s lint: PASS (all checks passed)
nox -s typecheck: PASS (0 errors, 1 pre-existing warning)
nox -s unit_tests: PASS (10,825 scenarios, 0 failures)
nox -s coverage_report: 97% (meets threshold)
nox -s integration_tests: 8 pre-existing failures in Cli Plan Context Commands and Core Cli Commands (unrelated to this change, confirmed on master)

## Implementation Complete ### Changes Made **CLI layer** (`src/cleveragents/cli/commands/actor_run.py`, `src/cleveragents/cli/commands/actor.py`): - Added `--skill` Typer option to the `run` command in both modules. The option is repeatable (`list[str] | None`) and defaults to `None`. - Added `_validate_skill_names()` function in each module that validates skill existence via `SkillService.get_skill()`. Uses `_get_skill_service()` from the `skill.py` module (DB-backed singleton) for lookup. On `KeyError`, emits a user-friendly error to stderr and raises `typer.Exit(code=2)`. - Validation runs before `ReactiveCleverAgentsApp` creation. Validated skill names are passed via the new `skill_names` constructor parameter. **Runtime layer** (`src/cleveragents/reactive/application.py`): - `ReactiveCleverAgentsApp.__init__` gains `skill_names: list[str] | None = None` parameter. - New `_resolve_skills()` method resolves skill names to `ResolvedToolEntry` lists via `SkillService.resolve_tools()`. Each entry is converted to a tool config dict (`{"name", "source_skill", "is_inline", "operation": "identity", "skill_source"}`). On unknown skill, raises `CleverAgentsException`. - New `_create_skill_service()` static method creates a `SkillService` with optional DB persistence (same pattern as `skill.py`). - New `skill_names` and `resolved_skill_tools` read-only properties. - `_register_agents_from_config()._make_agent_instance()` merges `_resolved_skill_tools` into each agent's tool list before creating `SimpleToolAgent`. ### Test Coverage **Feature scenarios added** (10 new scenarios, 7 for CLI + 3 for reactive app + 1 for default behavior): `features/actor_cli_coverage.feature`: - "Run actor with single skill flag" - "Run actor with multiple skill flags" - "Run actor with unknown skill flag" `features/actor_run_cli_coverage.feature`: - "Actor run command with single skill flag" - "Actor run command with multiple skill flags" - "Actor run command with unknown skill flag" `features/reactive_application_coverage_boost.feature`: - "Reactive app resolves skill names and stores tools" - "Reactive app raises error for unknown skill name" - "Reactive app merges skill tools into agent tool list" - "Reactive app works without skill names" ### Quality Gate Results - `nox -s lint`: **PASS** (all checks passed) - `nox -s typecheck`: **PASS** (0 errors, 1 pre-existing warning) - `nox -s unit_tests`: **PASS** (10,825 scenarios, 0 failures) - `nox -s coverage_report`: **97%** (meets threshold) - `nox -s integration_tests`: 8 pre-existing failures in `Cli Plan Context Commands` and `Core Cli Commands` (unrelated to this change, confirmed on master)

hurui200320 referenced this issue from a commit

2026-03-16 07:59:55 +00:00

fix(cli): add --skill flag to actor run command

hurui200320 referenced this issue from a pull request that will close it,

2026-03-16 08:00:06 +00:00

fix(cli): add --skill flag to actor run command #971

hurui200320 added

and removed

labels

2026-03-16 08:02:02 +00:00

hurui200320 added a new dependency

2026-03-16 08:11:49 +00:00

#971 fix(cli): add --skill flag to actor run command

hurui200320 referenced this issue

2026-03-16 08:39:01 +00:00

fix(cli): add --skill flag to actor run command #971

hurui200320 referenced this issue from a commit

2026-03-16 09:05:31 +00:00

fix(cli): add --skill flag to actor run command

hurui200320 referenced this issue

2026-03-16 09:25:17 +00:00

fix(cli): add --skill flag to actor run command #971

hurui200320 referenced this issue

2026-03-16 10:54:35 +00:00

fix(reactive): SimpleToolAgent only executes first tool — skill tools at later indices are ignored #974

hurui200320 referenced this issue from a commit

2026-03-16 10:56:05 +00:00

fix(cli): add --skill flag to actor run command

hurui200320 commented

2026-03-16 10:57:30 +00:00

Member

Implementation Notes — Second Review Response (commit aa18d0f7)

Design Decisions

DI Container for SkillService (C3): Added _build_skill_service() and skill_service Singleton to container.py following the established _build_* pattern. This eliminates the architectural layer violation where the reactive layer imported from the CLI layer. The Singleton ensures consistent state across CLI and reactive layers within the same process.
Single Validation Point (M5): Removed the separate validate_skill_names() call from the CLI. The ReactiveCleverAgentsApp._resolve_skills() method is now the single point of both validation and resolution. The constructor is wrapped in the existing try/except block so CleverAgentsException is properly caught and displayed.
Graph Executor Extraction (M4): Moved graph execution logic to ReactiveCleverAgentsApp → GraphExecutor in graph_executor.py. This brings application.py from 625 to 411 lines. Backward-compatible proxy methods remain on ReactiveCleverAgentsApp for existing test compatibility.
LLM Agent Guard (C1): Skill tools are only injected into agents that already have configured tools: if self._resolved_skill_tools and tools:. This prevents LLM agents from being inadvertently converted to SimpleToolAgent instances.
C2 Follow-up: Created #974 for the SimpleToolAgent single-tool execution limitation. This is a pre-existing architectural constraint not introduced by this PR.

Key Code Locations

cleveragents.application.container._build_skill_service — DI factory (commit aa18d0f7)
cleveragents.reactive.application.ReactiveCleverAgentsApp._resolve_skills — skill resolution via DI (commit aa18d0f7)
cleveragents.reactive.graph_executor.GraphExecutor — extracted graph execution (commit aa18d0f7)

Quality Gates

All passing: lint, typecheck (0 errors), unit_tests (10,832 scenarios, 0 failures), coverage (97%), integration_tests (all pass).

## Implementation Notes — Second Review Response (commit aa18d0f7) ### Design Decisions 1. **DI Container for SkillService (C3)**: Added `_build_skill_service()` and `skill_service` Singleton to `container.py` following the established `_build_*` pattern. This eliminates the architectural layer violation where the reactive layer imported from the CLI layer. The Singleton ensures consistent state across CLI and reactive layers within the same process. 2. **Single Validation Point (M5)**: Removed the separate `validate_skill_names()` call from the CLI. The `ReactiveCleverAgentsApp._resolve_skills()` method is now the single point of both validation and resolution. The constructor is wrapped in the existing `try/except` block so `CleverAgentsException` is properly caught and displayed. 3. **Graph Executor Extraction (M4)**: Moved graph execution logic to `ReactiveCleverAgentsApp` → `GraphExecutor` in `graph_executor.py`. This brings `application.py` from 625 to 411 lines. Backward-compatible proxy methods remain on `ReactiveCleverAgentsApp` for existing test compatibility. 4. **LLM Agent Guard (C1)**: Skill tools are only injected into agents that already have configured tools: `if self._resolved_skill_tools and tools:`. This prevents LLM agents from being inadvertently converted to `SimpleToolAgent` instances. 5. **C2 Follow-up**: Created #974 for the `SimpleToolAgent` single-tool execution limitation. This is a pre-existing architectural constraint not introduced by this PR. ### Key Code Locations - `cleveragents.application.container._build_skill_service` — DI factory (commit aa18d0f7) - `cleveragents.reactive.application.ReactiveCleverAgentsApp._resolve_skills` — skill resolution via DI (commit aa18d0f7) - `cleveragents.reactive.graph_executor.GraphExecutor` — extracted graph execution (commit aa18d0f7) ### Quality Gates All passing: lint, typecheck (0 errors), unit_tests (10,832 scenarios, 0 failures), coverage (97%), integration_tests (all pass).

hurui200320 referenced this issue from a commit

2026-03-17 07:45:51 +00:00

fix(cli): add --skill flag to actor run command

hurui200320 referenced this issue from a commit

2026-03-17 08:39:00 +00:00

fix(cli): add --skill flag to actor run command

hurui200320 commented

2026-03-17 08:54:34 +00:00

Member

Self-QA Implementation Notes (Cycles 1–3)

PR !971 underwent 3 automated self-QA review/fix cycles. Final verdict: Approved.

Cycle 1

Review findings (5 major, 8 minor, 7 nits):

M1: Dead code — validate_skill_names() defined but never called (leftover from earlier review iteration)
M2: Bare except Exception: in _build_skill_service() too broad, missing exc_info=True
M3: Two independent SkillService instances (DI container vs module singleton) with divergent state
M4: Zero test coverage on _build_skill_service() (both happy and fallback paths)
M5: CLI "unknown skill" tests inject fake errors rather than testing real error chain
m1–m8: Combined test missing ContextManager assertion, actor.py over 500 lines (pre-existing), zero-tool warning invisible at default verbosity, undocumented exception class change, GraphExecutor using Any types, code duplication between actor.py/actor_run.py, no input validation on --skill, no ANSI sanitization
N1–N7: Metavar, test naming, structlog exc_info, missing @coverage tags, import style, stale comment, duplicate step definitions

Fixes applied:

Removed validate_skill_names() entirely
Restructured exception handling into two try blocks: ImportError for missing SQLAlchemy, (OperationalError, DatabaseError, OSError) for DB failures; added exc_info=True
Unified SkillService — _get_skill_service() now delegates to get_container().skill_service()
Added 2 Behave scenarios testing _build_skill_service() happy path and fallback
Rewrote actor_run.py unknown-skill test to mock only get_container()
Added _sanitize_skill_name() with regex validation and ANSI control char stripping
Added print() to stderr for zero-tool warning, replaced GraphExecutor Any types with specific types
Fixed metavar to NAME, added @coverage tags, consolidated duplicate steps, updated comments
Deferred: actor.py line count (pre-existing), actor.py/actor_run.py duplication (coupled refactor)

Cycle 2

Review findings (3 major, 8 minor, 5 nits):

M1: ValueError path in _resolve_skills() completely untested
M2: Zero-tool skill resolution warning path untested
M3: actor.py unknown-skill test still uses full mock (not updated in cycle 1)
m4–m11: Regex doesn't enforce namespace/name structure, \w matches Unicode but docs say ASCII, module-level _service cache stale-reference risk, duplicate warning emission, dedup scenario name mismatch, ContextManager assertions in When not Then, container.py line count, no _sanitize_skill_name edge case tests

Fixes applied:

Added scenario for ValueError path ("Cycle detected in skill includes")
Added scenario for zero-tool resolution (empty entry list, verified resolved_skill_tools empty)
Rewrote actor.py unknown-skill test to match actor_run.py pattern (mock only get_container())
Tightened regex to ^[\w.-]{1,127}/[\w.-]{1,127}$ with re.ASCII flag
Removed module-level _service cache — always delegates to container
Removed duplicate logger.warning(), kept only print() to stderr
Renamed dedup step to "pass duplicate skill names to the runtime"
Moved ContextManager assertions to dedicated Then steps
Added 3 edge case scenarios (max length, ANSI codes, disallowed chars)
Added @coverage tags to all 25 new scenarios
Deferred: container.py line count (acknowledged in PR), graph_executor.py dict[str, Any] (informational), proxy method instantiation (informational)

Cycle 3

Review findings (0 major, 5 minor, 6 nits):

All cycle 1 and cycle 2 fixes verified as properly resolved
Remaining minor items are all non-blocking: DB fallback warning invisibility (pre-existing pattern), stale docstring number (256→255), two small test coverage gaps (zero-tool stderr assertion, ImportError path), regex restriction vs spec naming convention
No critical or major issues remain

No fixes needed — PR approved.

Remaining Issues (Non-blocking)

#	Severity	Description	Reason
1	Minor	DB fallback warning invisible at default verbosity	Pre-existing pattern in codebase; `structlog` used consistently
2	Minor	Docstring says "256 chars" but actual limit is 255	Cosmetic; error message is accurate
3	Minor	Zero-tool `print()` output not captured in test	Branch coverage met; functional test verifies empty result
4	Minor	`ImportError` fallback branch untested	Edge case for missing SQLAlchemy; `OperationalError` path tested
5	Minor	Regex stricter than spec's general naming convention	Intentional; all spec `--skill` examples use `namespace/name`
6	Nit	`actor.py` at 678 lines (pre-existing)	Requires dedicated refactor ticket
7	Nit	`actor.py`/`actor_run.py` code duplication (pre-existing)	Coupled with actor.py line count refactor
8	Nit	`container.py` at 739 lines	Acknowledged in PR description

Quality Gates (Final)

Gate	Result
`nox -e lint`	✅ PASS
`nox -e typecheck`	✅ PASS (0 errors)
`nox -e unit_tests`	✅ PASS (10,838 scenarios)
`nox -e integration_tests`	✅ PASS
`nox -e coverage_report`	✅ 97%

## Self-QA Implementation Notes (Cycles 1–3) PR !971 underwent 3 automated self-QA review/fix cycles. Final verdict: **Approved**. --- ### Cycle 1 **Review findings (5 major, 8 minor, 7 nits):** - **M1:** Dead code — `validate_skill_names()` defined but never called (leftover from earlier review iteration) - **M2:** Bare `except Exception:` in `_build_skill_service()` too broad, missing `exc_info=True` - **M3:** Two independent `SkillService` instances (DI container vs module singleton) with divergent state - **M4:** Zero test coverage on `_build_skill_service()` (both happy and fallback paths) - **M5:** CLI "unknown skill" tests inject fake errors rather than testing real error chain - **m1–m8:** Combined test missing ContextManager assertion, `actor.py` over 500 lines (pre-existing), zero-tool warning invisible at default verbosity, undocumented exception class change, `GraphExecutor` using `Any` types, code duplication between `actor.py`/`actor_run.py`, no input validation on `--skill`, no ANSI sanitization - **N1–N7:** Metavar, test naming, structlog `exc_info`, missing `@coverage` tags, import style, stale comment, duplicate step definitions **Fixes applied:** - Removed `validate_skill_names()` entirely - Restructured exception handling into two try blocks: `ImportError` for missing SQLAlchemy, `(OperationalError, DatabaseError, OSError)` for DB failures; added `exc_info=True` - Unified `SkillService` — `_get_skill_service()` now delegates to `get_container().skill_service()` - Added 2 Behave scenarios testing `_build_skill_service()` happy path and fallback - Rewrote `actor_run.py` unknown-skill test to mock only `get_container()` - Added `_sanitize_skill_name()` with regex validation and ANSI control char stripping - Added `print()` to stderr for zero-tool warning, replaced `GraphExecutor` `Any` types with specific types - Fixed metavar to `NAME`, added `@coverage` tags, consolidated duplicate steps, updated comments - **Deferred:** `actor.py` line count (pre-existing), `actor.py`/`actor_run.py` duplication (coupled refactor) --- ### Cycle 2 **Review findings (3 major, 8 minor, 5 nits):** - **M1:** `ValueError` path in `_resolve_skills()` completely untested - **M2:** Zero-tool skill resolution warning path untested - **M3:** `actor.py` unknown-skill test still uses full mock (not updated in cycle 1) - **m4–m11:** Regex doesn't enforce namespace/name structure, `\w` matches Unicode but docs say ASCII, module-level `_service` cache stale-reference risk, duplicate warning emission, dedup scenario name mismatch, ContextManager assertions in When not Then, `container.py` line count, no `_sanitize_skill_name` edge case tests **Fixes applied:** - Added scenario for `ValueError` path (`"Cycle detected in skill includes"`) - Added scenario for zero-tool resolution (empty entry list, verified `resolved_skill_tools` empty) - Rewrote `actor.py` unknown-skill test to match `actor_run.py` pattern (mock only `get_container()`) - Tightened regex to `^[\w.-]{1,127}/[\w.-]{1,127}$` with `re.ASCII` flag - Removed module-level `_service` cache — always delegates to container - Removed duplicate `logger.warning()`, kept only `print()` to stderr - Renamed dedup step to `"pass duplicate skill names to the runtime"` - Moved ContextManager assertions to dedicated Then steps - Added 3 edge case scenarios (max length, ANSI codes, disallowed chars) - Added `@coverage` tags to all 25 new scenarios - **Deferred:** `container.py` line count (acknowledged in PR), `graph_executor.py` `dict[str, Any]` (informational), proxy method instantiation (informational) --- ### Cycle 3 **Review findings (0 major, 5 minor, 6 nits):** - All cycle 1 and cycle 2 fixes verified as properly resolved - Remaining minor items are all non-blocking: DB fallback warning invisibility (pre-existing pattern), stale docstring number (256→255), two small test coverage gaps (zero-tool stderr assertion, ImportError path), regex restriction vs spec naming convention - No critical or major issues remain **No fixes needed — PR approved.** --- ### Remaining Issues (Non-blocking) | # | Severity | Description | Reason | |---|----------|-------------|--------| | 1 | Minor | DB fallback warning invisible at default verbosity | Pre-existing pattern in codebase; `structlog` used consistently | | 2 | Minor | Docstring says "256 chars" but actual limit is 255 | Cosmetic; error message is accurate | | 3 | Minor | Zero-tool `print()` output not captured in test | Branch coverage met; functional test verifies empty result | | 4 | Minor | `ImportError` fallback branch untested | Edge case for missing SQLAlchemy; `OperationalError` path tested | | 5 | Minor | Regex stricter than spec's general naming convention | Intentional; all spec `--skill` examples use `namespace/name` | | 6 | Nit | `actor.py` at 678 lines (pre-existing) | Requires dedicated refactor ticket | | 7 | Nit | `actor.py`/`actor_run.py` code duplication (pre-existing) | Coupled with actor.py line count refactor | | 8 | Nit | `container.py` at 739 lines | Acknowledged in PR description | ### Quality Gates (Final) | Gate | Result | |------|--------| | `nox -e lint` | ✅ PASS | | `nox -e typecheck` | ✅ PASS (0 errors) | | `nox -e unit_tests` | ✅ PASS (10,838 scenarios) | | `nox -e integration_tests` | ✅ PASS | | `nox -e coverage_report` | ✅ 97% |

hurui200320 referenced this issue

2026-03-17 09:06:14 +00:00

fix(cli): add --skill flag to actor run command #971

hurui200320 referenced this issue from a commit

2026-03-17 09:09:36 +00:00

fix(cli): add --skill flag to actor run command

hurui200320 referenced this issue from a commit

2026-03-18 07:38:20 +00:00

fix(cli): add --skill flag to actor run command

hurui200320 commented

2026-03-18 07:39:54 +00:00

Member

Implementation Notes — Review Round 3 Fixes

Addressed all findings from @brent.edwards code review (Rounds 1 & 2, Reviews #2383 and #2390) and rebased onto latest master per PM Day 37 request.

Changes Made

P1-1 — Zero-tool skill warning logging (ReactiveCleverAgentsApp._resolve_skills):
Replaced print(f"Warning: ...", file=sys.stderr) with logger.warning("Skill '%s' resolved to zero tools", name). This integrates the warning into the standard logging pipeline, respecting log-level filtering and structured log output. The import sys was removed as it had no remaining usages in application.py.

P2-4↑ — Exception catch scope in actor_run.py (actor_run.run):
Reverted the exception handler from except CleverAgentsError to except CleverAgentsException, matching master. CleverAgentsException is a subclass of CleverAgentsError; catching the base class would silently intercept DomainError, ValidationError, NotFoundError, etc. This behavioral broadening is unrelated to the --skill flag addition and should be a separate PR if intentional.

P2-2 — Observability for skipped skill injection (ReactiveCleverAgentsApp._make_agent_instance):
Added logger.debug("Skipping skill tool injection for agent '%s' (agent has no base tools)", name) when _resolved_skill_tools is non-empty but the agent has zero base tools. This gives developers visibility into which agents are unaffected by --skill. Updated the --skill help text in both actor.py and actor_run.py to explicitly state "only augments tool-bearing agents".

P3-5 — Robot Framework smoke test (robot/skill_actor_run.robot, robot/helper_skill_actor_run.py):
Added 2 integration tests:

Unknown skill name → exit code 2 with "not found in registry" error message.
Valid skill name → CLI accepts the flag without "not found" error.

Design Decisions

P2-3 (container.py size) and P3-6 (static method pattern) are acknowledged as valid observations but deferred — both are pre-existing patterns worsened minimally by this PR and warrant separate refactoring tickets.
The Robot helper uses CliRunner from Typer to test the actual CLI entry point without spawning subprocesses, consistent with helper_skill_cli.py.

Quality Gates

All gates pass: lint ✅, typecheck ✅, 11,130 unit tests ✅, 1,559 integration tests ✅, 97% coverage ✅.

## Implementation Notes — Review Round 3 Fixes Addressed all findings from @brent.edwards code review (Rounds 1 & 2, Reviews #2383 and #2390) and rebased onto latest master per PM Day 37 request. ### Changes Made **P1-1 — Zero-tool skill warning logging** (`ReactiveCleverAgentsApp._resolve_skills`): Replaced `print(f"Warning: ...", file=sys.stderr)` with `logger.warning("Skill '%s' resolved to zero tools", name)`. This integrates the warning into the standard logging pipeline, respecting log-level filtering and structured log output. The `import sys` was removed as it had no remaining usages in `application.py`. **P2-4↑ — Exception catch scope in actor_run.py** (`actor_run.run`): Reverted the exception handler from `except CleverAgentsError` to `except CleverAgentsException`, matching master. `CleverAgentsException` is a subclass of `CleverAgentsError`; catching the base class would silently intercept `DomainError`, `ValidationError`, `NotFoundError`, etc. This behavioral broadening is unrelated to the `--skill` flag addition and should be a separate PR if intentional. **P2-2 — Observability for skipped skill injection** (`ReactiveCleverAgentsApp._make_agent_instance`): Added `logger.debug("Skipping skill tool injection for agent '%s' (agent has no base tools)", name)` when `_resolved_skill_tools` is non-empty but the agent has zero base tools. This gives developers visibility into which agents are unaffected by `--skill`. Updated the `--skill` help text in both `actor.py` and `actor_run.py` to explicitly state "only augments tool-bearing agents". **P3-5 — Robot Framework smoke test** (`robot/skill_actor_run.robot`, `robot/helper_skill_actor_run.py`): Added 2 integration tests: 1. Unknown skill name → exit code 2 with "not found in registry" error message. 2. Valid skill name → CLI accepts the flag without "not found" error. ### Design Decisions - P2-3 (container.py size) and P3-6 (static method pattern) are acknowledged as valid observations but deferred — both are pre-existing patterns worsened minimally by this PR and warrant separate refactoring tickets. - The Robot helper uses `CliRunner` from Typer to test the actual CLI entry point without spawning subprocesses, consistent with `helper_skill_cli.py`. ### Quality Gates All gates pass: lint ✅, typecheck ✅, 11,130 unit tests ✅, 1,559 integration tests ✅, 97% coverage ✅.

hurui200320 referenced this issue from a commit

2026-03-18 07:46:26 +00:00

fix(cli): add --skill flag to actor run command

hurui200320 referenced this issue from a commit

2026-03-18 08:27:46 +00:00

fix(cli): add --skill flag to actor run command

hurui200320 referenced this issue from a commit

2026-03-19 06:17:03 +00:00

fix(cli): add --skill flag to actor run command

hurui200320 referenced this issue from a commit

2026-03-19 09:21:31 +00:00

fix(cli): add --skill flag to actor run command