cleveragents/cleveragents-core

Fork 3

UAT: `agents skill add` does not validate that included skills are registered — spec requires registration-time validation of includes and cycle detection #2408

New issue

Open

opened 2026-04-03 17:36:19 +00:00 by freemo · 1 comment

freemo commented

2026-04-03 17:36:19 +00:00

Owner

Background and Context

The spec explicitly defines two validation rules that must be enforced at registration time (i.e., when agents skill add / SkillService.add_skill() is called), not deferred to resolve time:

Spec line 23167: "Included skills must be registered before the including skill can be added. The agents skill add command validates this."
Spec line 23165: "Circular includes are forbidden. The system validates the include graph at registration time and rejects cycles."

Both rules are currently violated: SkillService.add_skill() performs no include-graph validation whatsoever before persisting the skill, meaning broken and cyclic include chains are silently accepted at registration time and only surface as confusing errors later at resolve time.

Current Behavior

SkillService.add_skill() in src/cleveragents/application/services/skill_service.py stores a skill without checking:

Whether any skill named in includes is already registered
Whether adding the new skill would introduce a cycle in the include graph

svc = SkillService()

# Test 1: Unregistered include — should fail, but succeeds
config = SkillConfigSchema.from_yaml('''
name: local/child
description: Child skill
includes:
  - name: local/nonexistent-parent
''')
skill, is_new = svc.add_skill(config)  # Succeeds! Should raise.
print(skill.name)  # 'local/child' — registered despite invalid include

# Test 2: Circular include — should fail, but succeeds
config_a = SkillConfigSchema.from_yaml('name: local/a\ndescription: A\nincludes:\n  - name: local/b')
config_b = SkillConfigSchema.from_yaml('name: local/b\ndescription: B\nincludes:\n  - name: local/a')
svc.add_skill(config_a)
svc.add_skill(config_b)  # Succeeds! Should raise — creates a cycle.

The error only surfaces later when resolve_tools() is called (e.g., via agents skill tools), violating the spec's fail-fast principle.

Expected Behavior

Per the specification:

agents skill add (and SkillService.add_skill()) must fail immediately with a clear, actionable error if any skill named in includes is not yet registered.
agents skill add (and SkillService.add_skill()) must fail immediately with a clear, actionable error if adding the skill would create a circular include chain (cycle detection on the include graph).

Acceptance Criteria

SkillService.add_skill() raises a descriptive exception when any included skill name is not registered at call time.
SkillService.add_skill() raises a descriptive exception when the new skill would introduce a cycle in the include graph.
Both validations occur before the skill is persisted (fail-fast).
Error messages clearly identify the unregistered include name or the cycle path.
All existing tests continue to pass.
New Behave scenarios cover both failure modes (unregistered include, circular include).
New Robot Framework integration tests cover the CLI surface (agents skill add exit code and stderr).
Coverage remains ≥ 97%.

Supporting Information

Affected file: src/cleveragents/application/services/skill_service.py, add_skill() method
Spec references: lines 23165 and 23167 of docs/specification.md
Discovered during: UAT testing of the Skills System (Issue #397 workstream)
Impact: Skills with broken include chains are silently registered, causing confusing ValueError at resolve time instead of a clear registration-time error; circular includes can be created, causing infinite recursion or ValueError at resolve time.

Metadata

Branch: fix/skill-service-registration-time-include-validation
Commit Message: fix(skills): validate include graph at registration time in SkillService.add_skill
Milestone: v3.5.0
Parent Epic: #397

Subtasks

Reproduce both failure modes with a failing Behave scenario (TDD — merge failing test first)
Implement unregistered-include validation in SkillService.add_skill() (check each include name against the skill registry before persisting)
Implement cycle-detection in SkillService.add_skill() (DFS/BFS on the include graph before persisting)
Add descriptive exception types / messages for both failure modes
Tests (Behave): Add scenarios for unregistered include rejection and circular include rejection
Tests (Robot): Add integration tests for agents skill add CLI exit code and stderr output for both failure modes
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 (fix(skills): validate include graph at registration time in SkillService.add_skill), followed by a blank line, then additional lines providing relevant details about the implementation, and a footer ISSUES CLOSED: #<this issue number>.
The commit is pushed to the remote on the branch matching the Branch in Metadata exactly (fix/skill-service-registration-time-include-validation).
The commit is submitted as a pull request to master, reviewed, and merged before this issue is marked done.
All nox stages pass.
Coverage ≥ 97%.

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

## Background and Context The spec explicitly defines two validation rules that must be enforced at **registration time** (i.e., when `agents skill add` / `SkillService.add_skill()` is called), not deferred to resolve time: 1. **Spec line 23167**: "Included skills must be registered before the including skill can be added. The `agents skill add` command validates this." 2. **Spec line 23165**: "Circular includes are forbidden. The system validates the include graph at registration time and rejects cycles." Both rules are currently violated: `SkillService.add_skill()` performs no include-graph validation whatsoever before persisting the skill, meaning broken and cyclic include chains are silently accepted at registration time and only surface as confusing errors later at resolve time. ## Current Behavior `SkillService.add_skill()` in `src/cleveragents/application/services/skill_service.py` stores a skill without checking: - Whether any skill named in `includes` is already registered - Whether adding the new skill would introduce a cycle in the include graph ```python svc = SkillService() # Test 1: Unregistered include — should fail, but succeeds config = SkillConfigSchema.from_yaml(''' name: local/child description: Child skill includes: - name: local/nonexistent-parent ''') skill, is_new = svc.add_skill(config) # Succeeds! Should raise. print(skill.name) # 'local/child' — registered despite invalid include # Test 2: Circular include — should fail, but succeeds config_a = SkillConfigSchema.from_yaml('name: local/a\ndescription: A\nincludes:\n - name: local/b') config_b = SkillConfigSchema.from_yaml('name: local/b\ndescription: B\nincludes:\n - name: local/a') svc.add_skill(config_a) svc.add_skill(config_b) # Succeeds! Should raise — creates a cycle. ``` The error only surfaces later when `resolve_tools()` is called (e.g., via `agents skill tools`), violating the spec's fail-fast principle. ## Expected Behavior Per the specification: 1. `agents skill add` (and `SkillService.add_skill()`) must **fail immediately** with a clear, actionable error if any skill named in `includes` is not yet registered. 2. `agents skill add` (and `SkillService.add_skill()`) must **fail immediately** with a clear, actionable error if adding the skill would create a circular include chain (cycle detection on the include graph). ## Acceptance Criteria - [ ] `SkillService.add_skill()` raises a descriptive exception when any included skill name is not registered at call time. - [ ] `SkillService.add_skill()` raises a descriptive exception when the new skill would introduce a cycle in the include graph. - [ ] Both validations occur **before** the skill is persisted (fail-fast). - [ ] Error messages clearly identify the unregistered include name or the cycle path. - [ ] All existing tests continue to pass. - [ ] New Behave scenarios cover both failure modes (unregistered include, circular include). - [ ] New Robot Framework integration tests cover the CLI surface (`agents skill add` exit code and stderr). - [ ] Coverage remains ≥ 97%. ## Supporting Information - **Affected file**: `src/cleveragents/application/services/skill_service.py`, `add_skill()` method - **Spec references**: lines 23165 and 23167 of `docs/specification.md` - **Discovered during**: UAT testing of the Skills System (Issue #397 workstream) - **Impact**: Skills with broken include chains are silently registered, causing confusing `ValueError` at resolve time instead of a clear registration-time error; circular includes can be created, causing infinite recursion or `ValueError` at resolve time. ## Metadata - **Branch**: `fix/skill-service-registration-time-include-validation` - **Commit Message**: `fix(skills): validate include graph at registration time in SkillService.add_skill` - **Milestone**: v3.5.0 - **Parent Epic**: #397 ## Subtasks - [ ] Reproduce both failure modes with a failing Behave scenario (TDD — merge failing test first) - [ ] Implement unregistered-include validation in `SkillService.add_skill()` (check each include name against the skill registry before persisting) - [ ] Implement cycle-detection in `SkillService.add_skill()` (DFS/BFS on the include graph before persisting) - [ ] Add descriptive exception types / messages for both failure modes - [ ] Tests (Behave): Add scenarios for unregistered include rejection and circular include rejection - [ ] Tests (Robot): Add integration tests for `agents skill add` CLI exit code and stderr output for both failure modes - [ ] 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 (`fix(skills): validate include graph at registration time in SkillService.add_skill`), followed by a blank line, then additional lines providing relevant details about the implementation, and a footer `ISSUES CLOSED: #<this issue number>`. - The commit is pushed to the remote on the branch matching the **Branch** in Metadata exactly (`fix/skill-service-registration-time-include-validation`). - The commit is submitted as a **pull request** to `master`, reviewed, and **merged** before this issue is marked done. - All nox stages pass. - Coverage ≥ 97%. --- **Automated by CleverAgents Bot** Supervisor: UAT Testing | Agent: ca-new-issue-creator

freemo added this to the v3.5.0 milestone

2026-04-03 17:36:25 +00:00

freemo added a new dependency

2026-04-03 17:36:53 +00:00

#397 Epic: Server & Autonomy Infrastructure

freemo referenced this issue

2026-04-03 17:42:38 +00:00

[UAT-SUPERVISOR] Progress Report — Cycle 1511 — v3.7.0 #2421

freemo referenced this issue

2026-04-03 17:52:45 +00:00

[UAT-SUPERVISOR] Progress Report — Cycle 1521 — v3.7.0 #2422

freemo referenced this issue

2026-04-03 18:02:52 +00:00

[UAT-SUPERVISOR] Progress Report — Cycle 1531 — v3.7.0 #2433

freemo referenced this issue

2026-04-03 18:12:59 +00:00

[UAT-SUPERVISOR] Progress Report — Cycle 1541 — v3.7.0 #2434

freemo referenced this issue

2026-04-03 18:23:14 +00:00

[UAT-SUPERVISOR] Progress Report — Cycle 1551 — v3.7.0 #2445

freemo commented

2026-04-03 18:24:40 +00:00

Author

Owner

Issue triaged by project owner:

State: Verified
Priority: Medium — Missing registration-time validation for skill includes means invalid configurations are accepted silently and fail at resolve time with confusing errors.
Milestone: v3.5.0
MoSCoW: Should Have — The spec explicitly requires registration-time validation. This is a spec compliance gap that affects user experience but skills without includes work correctly.
Parent Epic: Needs assignment — linking to Epic #397 (Server & Autonomy Infrastructure) for skill registration pipeline.

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

Issue triaged by project owner: - **State**: Verified - **Priority**: Medium — Missing registration-time validation for skill includes means invalid configurations are accepted silently and fail at resolve time with confusing errors. - **Milestone**: v3.5.0 - **MoSCoW**: Should Have — The spec explicitly requires registration-time validation. This is a spec compliance gap that affects user experience but skills without includes work correctly. - **Parent Epic**: Needs assignment — linking to Epic #397 (Server & Autonomy Infrastructure) for skill registration pipeline. --- **Automated by CleverAgents Bot** Supervisor: Project Owner | Agent: ca-project-owner