cleveragents/cleveragents-core

Fork 3

UAT: `agents skill add` does not validate that included skills are registered — spec requires pre-registration check at add time #2555

New issue

Open

opened 2026-04-03 18:53:22 +00:00 by freemo · 5 comments

freemo commented

2026-04-03 18:53:22 +00:00

Owner

Metadata

Branch: fix/skill-add-include-validation
Commit Message: fix(skills): validate included skills are registered before adding skill
Milestone: v3.2.0
Parent Epic: #392

Bug Report

Feature Area

Actor, Skill and Tool Abstraction - agents skill add CLI command / SkillService.add_skill()

What Was Tested

Code analysis of src/cleveragents/application/services/skill_service.py — the add_skill() method, and src/cleveragents/cli/commands/skill.py — the add command.

Expected Behavior (from spec)

The specification (docs/specification.md, section "Rules for skill composition") states:

Included skills must be registered before the including skill can be added. The agents skill add command validates this.

When a skill YAML references another skill in its includes: list, the system must verify that all referenced skills are already registered in the Skill Registry before accepting the new skill. If any included skill is not registered, the command must fail with a clear error.

Actual Behavior

SkillService.add_skill() (src/cleveragents/application/services/skill_service.py, lines 73-120) does not check whether included skills are registered before adding the new skill. The method:

Converts the SkillConfigSchema to a dict via _schema_to_skill_dict()
Creates a Skill domain object via Skill.from_config()
Stores it in self._skills[name]

At no point does it verify that skills referenced in includes: are already present in self._skills. A skill can be registered with unresolvable includes, and the error is only discovered later when resolve_tools() is called.

Code Location

src/cleveragents/application/services/skill_service.py — add_skill() method (lines 73-120)
src/cleveragents/cli/commands/skill.py — add() command (calls service.add_skill())

Steps to Reproduce

Create a skill YAML that includes local/nonexistent-skill:

name: local/my-skill
description: Test skill
includes:
  - name: local/nonexistent-skill

Run: agents skill add --config ./my-skill.yaml
Observe: The skill is registered successfully despite local/nonexistent-skill not being registered
Expected: Command should fail with "Included skill 'local/nonexistent-skill' is not registered"

Impact

Severity: High
Priority: High
Skills can be registered with dangling include references, leading to runtime failures when tools are resolved
Violates the spec's explicit guarantee that skill composition is validated at registration time
The SkillRegistry.validate_skill() method does check includes, but it is not called during add_skill()

Subtasks

Add pre-registration check in SkillService.add_skill() that verifies all includes entries exist in self._skills
Return a clear error message listing which included skills are not registered
Add Behave tests for the validation failure case
Ensure the check also runs when update=True (re-registering a skill)

Definition of Done

agents skill add fails with a clear error when any included skill is not registered
Error message names the missing skill(s)
Unit test coverage ≥ 97% maintained
PR merged and issue closed
All nox stages pass
Coverage >= 97%

Automated by CleverAgents Bot
Supervisor: UAT Testing | Agent: ca-new-issue-creator

## Metadata - **Branch**: `fix/skill-add-include-validation` - **Commit Message**: `fix(skills): validate included skills are registered before adding skill` - **Milestone**: v3.2.0 - **Parent Epic**: #392 ## Bug Report ### Feature Area Actor, Skill and Tool Abstraction - `agents skill add` CLI command / `SkillService.add_skill()` ### What Was Tested Code analysis of `src/cleveragents/application/services/skill_service.py` — the `add_skill()` method, and `src/cleveragents/cli/commands/skill.py` — the `add` command. ### Expected Behavior (from spec) The specification (`docs/specification.md`, section "Rules for skill composition") states: > 3. Included skills must be registered before the including skill can be added. The `agents skill add` command validates this. When a skill YAML references another skill in its `includes:` list, the system must verify that all referenced skills are already registered in the Skill Registry before accepting the new skill. If any included skill is not registered, the command must fail with a clear error. ### Actual Behavior `SkillService.add_skill()` (`src/cleveragents/application/services/skill_service.py`, lines 73-120) does not check whether included skills are registered before adding the new skill. The method: 1. Converts the `SkillConfigSchema` to a dict via `_schema_to_skill_dict()` 2. Creates a `Skill` domain object via `Skill.from_config()` 3. Stores it in `self._skills[name]` At no point does it verify that skills referenced in `includes:` are already present in `self._skills`. A skill can be registered with unresolvable includes, and the error is only discovered later when `resolve_tools()` is called. ### Code Location - `src/cleveragents/application/services/skill_service.py` — `add_skill()` method (lines 73-120) - `src/cleveragents/cli/commands/skill.py` — `add()` command (calls `service.add_skill()`) ### Steps to Reproduce 1. Create a skill YAML that includes `local/nonexistent-skill`: ``` name: local/my-skill description: Test skill includes: - name: local/nonexistent-skill ``` 2. Run: `agents skill add --config ./my-skill.yaml` 3. Observe: The skill is registered successfully despite `local/nonexistent-skill` not being registered 4. Expected: Command should fail with "Included skill 'local/nonexistent-skill' is not registered" ### Impact - Severity: High - Priority: High - Skills can be registered with dangling include references, leading to runtime failures when tools are resolved - Violates the spec's explicit guarantee that skill composition is validated at registration time - The `SkillRegistry.validate_skill()` method does check includes, but it is not called during `add_skill()` ## Subtasks - [x] Add pre-registration check in `SkillService.add_skill()` that verifies all `includes` entries exist in `self._skills` - [x] Return a clear error message listing which included skills are not registered - [x] Add Behave tests for the validation failure case - [x] Ensure the check also runs when `update=True` (re-registering a skill) ## Definition of Done - [ ] `agents skill add` fails with a clear error when any included skill is not registered - [ ] Error message names the missing skill(s) - [ ] Unit test coverage ≥ 97% maintained - [ ] PR merged and issue closed - All nox stages pass - Coverage >= 97% --- Automated by CleverAgents Bot Supervisor: UAT Testing | Agent: ca-new-issue-creator

freemo added this to the v3.2.0 milestone

2026-04-03 18:53:33 +00:00

freemo referenced this issue

2026-04-03 18:53:37 +00:00

[UAT-SUPERVISOR] Progress Report — Cycle 1581 — v3.7.0 #2558

freemo added a new dependency

2026-04-03 18:54:43 +00:00

#392 Epic: Actor YAML & Compiler

freemo commented

2026-04-03 19:02:09 +00:00

Author

Owner

Issue triaged by project owner:

State: Verified
MoSCoW: Should Have

Automated by CleverAgents Bot
Supervisor: Project Owner | Agent: ca-project-owner

Issue triaged by project owner: - **State**: Verified - **MoSCoW**: Should Have --- **Automated by CleverAgents Bot** Supervisor: Project Owner | Agent: ca-project-owner

freemo added a new dependency

2026-04-03 19:08:45 +00:00

#2597 fix(ci): restore all CI quality gates to passing on master — no suppression, no bypasses

freemo commented

2026-04-05 02:54:35 +00:00

Author

Owner

Transition to State/In Progress: Attempting to move issue #2555 from State/Verified to State/In Progress.

Pre-checks:

Current labels include State/Verified; No Blocked label present.

Actions attempted:

Removed old state label: State/Verified (not removed due to API limitations in this environment; Forgejo adds labels rather than removing existing ones when using the Add Labels API).
Added new state label: State/In Progress (requires the label to exist in the repository; the label does not appear to exist in this repo's label catalog, so I could not add it automatically).

Notes:

The transition rules specify ALLOWED transitions and require removal of all State/* labels before adding the new one. I can complete this once the State/In Progress label exists and I can remove the old State/* labels.

Next steps:

Create the label 'State/In Progress' in the repository labels (and any other State/* labels needed for future transitions).
Then re-run the transition to remove all existing State/* labels and add State/In Progress.

If you prefer, I can also try a best-effort removal of State/Verified by a maintainer action to standardize the labels, but I cannot remove without a corresponding label replacement path in this environment.

Automated by CleverAgents Bot
Supervisor: Implementation | Agent: ca-issue-state-updater

Transition to State/In Progress: Attempting to move issue #2555 from State/Verified to State/In Progress. Pre-checks: - Current labels include State/Verified; No Blocked label present. Actions attempted: - Removed old state label: State/Verified (not removed due to API limitations in this environment; Forgejo adds labels rather than removing existing ones when using the Add Labels API). - Added new state label: State/In Progress (requires the label to exist in the repository; the label does not appear to exist in this repo's label catalog, so I could not add it automatically). Notes: - The transition rules specify ALLOWED transitions and require removal of all State/* labels before adding the new one. I can complete this once the State/In Progress label exists and I can remove the old State/* labels. Next steps: - Create the label 'State/In Progress' in the repository labels (and any other State/* labels needed for future transitions). - Then re-run the transition to remove all existing State/* labels and add State/In Progress. If you prefer, I can also try a best-effort removal of State/Verified by a maintainer action to standardize the labels, but I cannot remove without a corresponding label replacement path in this environment. --- **Automated by CleverAgents Bot** Supervisor: Implementation | Agent: ca-issue-state-updater

freemo commented

2026-04-05 02:56:02 +00:00

Author

Owner

Starting implementation on branch fix/skill-add-include-validation.

Difficulty assessment: Medium → starting at sonnet tier.

Plan:

Wave 1 (parallel):
- Subtask 1: Add pre-registration check in SkillService.add_skill()
- Subtask 2: Return clear error message listing missing skills
- Subtask 4: Ensure check runs when update=True
Wave 2 (depends on Wave 1):
- Subtask 3: Add Behave tests for the validation failure case

Automated by CleverAgents Bot
Supervisor: Implementation | Agent: ca-issue-worker

Starting implementation on branch `fix/skill-add-include-validation`. Difficulty assessment: Medium → starting at sonnet tier. **Plan:** - Wave 1 (parallel): - Subtask 1: Add pre-registration check in `SkillService.add_skill()` - Subtask 2: Return clear error message listing missing skills - Subtask 4: Ensure check runs when `update=True` - Wave 2 (depends on Wave 1): - Subtask 3: Add Behave tests for the validation failure case --- **Automated by CleverAgents Bot** Supervisor: Implementation | Agent: ca-issue-worker

freemo referenced this issue

2026-04-05 03:10:48 +00:00

Task: Autonomous build progress report — 2026-04-05 #2874

freemo referenced this issue from a commit

2026-04-05 04:37:00 +00:00

fix(skills): validate included skills are registered before adding skill

freemo commented

2026-04-05 04:37:38 +00:00

Author

Owner

All subtasks complete. Quality gates passed (lint ✓, typecheck ✓, Behave tests ✓). Creating PR.

Implementation summary:

Added pre-registration check in SkillService.add_skill() (service layer) — validates all includes entries exist in self._skills before accepting the skill
Check applies on both initial add and update=True re-registration
Error message lists all missing skills at once for complete user guidance
8 new Behave scenarios in features/skill_add_include_validation.feature
Updated 2 existing test scenarios that used the full skill YAML (which includes local/file-reader) to pre-register the dependency

Automated by CleverAgents Bot
Supervisor: Implementation | Agent: ca-issue-worker

All subtasks complete. Quality gates passed (lint ✓, typecheck ✓, Behave tests ✓). Creating PR. **Implementation summary:** - Added pre-registration check in `SkillService.add_skill()` (service layer) — validates all `includes` entries exist in `self._skills` before accepting the skill - Check applies on both initial add and `update=True` re-registration - Error message lists all missing skills at once for complete user guidance - 8 new Behave scenarios in `features/skill_add_include_validation.feature` - Updated 2 existing test scenarios that used the full skill YAML (which includes `local/file-reader`) to pre-register the dependency --- **Automated by CleverAgents Bot** Supervisor: Implementation | Agent: ca-issue-worker

freemo referenced this issue

2026-04-05 06:35:39 +00:00

[CA-AUTO] Backlog Groomer — groomer-1 — Session Tracker #2771

freemo removed this from the v3.2.0 milestone

2026-04-06 22:29:50 +00:00

freemo commented

2026-04-06 22:30:05 +00:00

Author

Owner

This issue has been moved to the backlog as part of an aggressive grooming of the v3.2.0 milestone. It has been deemed non-critical for the minimal viability of the milestone and will be addressed in a future release.

freemo removed a dependency

2026-04-14 22:18:57 +00:00

#2597 fix(ci): restore all CI quality gates to passing on master — no suppression, no bypasses