UAT: SkillRegistry class is unused by SkillService — two parallel in-memory registries exist with no integration #2892

Open
opened 2026-04-05 02:43:37 +00:00 by freemo · 1 comment
Owner

Metadata

  • Branch: fix/skill-registry/integrate-skill-registry-into-skill-service
  • Commit Message: fix(skills): integrate SkillRegistry into SkillService to eliminate parallel in-memory registries
  • Milestone: v3.7.0
  • Parent Epic: #393

Background

During UAT analysis of the Skills System, a critical architectural inconsistency was discovered between SkillRegistry and SkillService. Two completely separate, unconnected skill management implementations exist in parallel, causing the spec-required refresh(), validate_plan(), and validate_skill() operations to be dead code in the production service layer.

Problem Description

The spec defines a "Skill Registry" as a core registry component. The SkillRegistry class (src/cleveragents/skills/registry.py) provides the following critical operations:

  • refresh(name) — recompute flattened tool set for a skill
  • refresh_all() — refresh all registered skills
  • validate_plan(plan) — validate plan's skill references
  • validate_skill(skill) — validate a skill definition's tool references
  • tools(skill_name) — return flattened tool list with capability summary

However, SkillService (src/cleveragents/application/services/skill_service.py) — the actual service used by the CLI and application layer — maintains its own separate OrderedDict[str, Skill] and does not use SkillRegistry at all.

Two Parallel Implementations

  1. SkillRegistry (src/cleveragents/skills/registry.py): A full-featured in-memory registry with refresh, validation, and tool resolution. It is never instantiated or used by the main application service layer.

  2. SkillService (src/cleveragents/application/services/skill_service.py): The actual service used by the CLI and application layer. It maintains its own separate OrderedDict[str, Skill] and does NOT use SkillRegistry at all.

Consequences

  • SkillRegistry.refresh() and refresh_all() are dead code — never called in production
  • SkillRegistry.validate_plan() is dead code — never called in production
  • SkillRegistry.validate_skill() is dead code — never called in production
  • The MCP notifications/tools/list_changed event handling described in SkillRegistry docstring is never wired up
  • The SkillService has no refresh() or validate_plan() methods
  • Skills may become stale with no mechanism to detect or correct this in production

Code Locations

  • src/cleveragents/skills/registry.pySkillRegistry class (unused in production)
  • src/cleveragents/application/services/skill_service.pySkillService class (used, but lacks registry features)
  • src/cleveragents/application/services/skill_registry_service.py — potential bridge (needs investigation)

Steps to Reproduce

  1. Search the codebase for SkillRegistry() instantiation — it is never instantiated in production code
  2. Check SkillService — it has no refresh(), validate_plan(), or validate_skill() methods
  3. The SkillRegistry class is only referenced in tests

Subtasks

  • Investigate SkillRegistryService (src/cleveragents/application/services/skill_registry_service.py) to determine if it bridges SkillRegistry and SkillService, and document findings
  • Determine the correct integration strategy: either (a) have SkillService delegate to SkillRegistry internally, or (b) expose SkillRegistry directly through the service layer
  • Add refresh(name: str) and refresh_all() methods to SkillService (or wire through to SkillRegistry)
  • Add validate_plan(plan) and validate_skill(skill) methods to SkillService (or wire through to SkillRegistry)
  • Wire up MCP notifications/tools/list_changed event handling as described in SkillRegistry docstring
  • Ensure SkillRegistry is instantiated and used by the production service layer (not just tests)
  • Write BDD Behave scenarios covering: refresh(), refresh_all(), validate_plan(), validate_skill(), and tools() via the service layer
  • Update integration tests (Robot Framework) to verify end-to-end skill refresh and validation flows
  • Verify no regression in existing SkillService behaviour

Definition of Done

  • SkillRegistry is instantiated and used by SkillService (or a clearly documented architectural decision explains why they remain separate)
  • SkillService exposes refresh(), refresh_all(), validate_plan(), and validate_skill() operations as required by the spec
  • MCP notifications/tools/list_changed event handling is wired up in the production service layer
  • No dead code remains: all public methods of SkillRegistry are reachable through the production service layer
  • BDD unit tests (Behave) cover all new/modified service methods
  • All nox stages pass
  • Coverage >= 97%

Automated by CleverAgents Bot
Supervisor: UAT Testing | Agent: ca-uat-tester

## Metadata - **Branch**: `fix/skill-registry/integrate-skill-registry-into-skill-service` - **Commit Message**: `fix(skills): integrate SkillRegistry into SkillService to eliminate parallel in-memory registries` - **Milestone**: v3.7.0 - **Parent Epic**: #393 ## Background During UAT analysis of the Skills System, a critical architectural inconsistency was discovered between `SkillRegistry` and `SkillService`. Two completely separate, unconnected skill management implementations exist in parallel, causing the spec-required `refresh()`, `validate_plan()`, and `validate_skill()` operations to be dead code in the production service layer. ## Problem Description The spec defines a "Skill Registry" as a core registry component. The `SkillRegistry` class (`src/cleveragents/skills/registry.py`) provides the following critical operations: - `refresh(name)` — recompute flattened tool set for a skill - `refresh_all()` — refresh all registered skills - `validate_plan(plan)` — validate plan's skill references - `validate_skill(skill)` — validate a skill definition's tool references - `tools(skill_name)` — return flattened tool list with capability summary **However**, `SkillService` (`src/cleveragents/application/services/skill_service.py`) — the actual service used by the CLI and application layer — maintains its own separate `OrderedDict[str, Skill]` and does **not** use `SkillRegistry` at all. ### Two Parallel Implementations 1. **`SkillRegistry`** (`src/cleveragents/skills/registry.py`): A full-featured in-memory registry with refresh, validation, and tool resolution. It is **never instantiated or used** by the main application service layer. 2. **`SkillService`** (`src/cleveragents/application/services/skill_service.py`): The actual service used by the CLI and application layer. It maintains its own separate `OrderedDict[str, Skill]` and does NOT use `SkillRegistry` at all. ### Consequences - `SkillRegistry.refresh()` and `refresh_all()` are dead code — never called in production - `SkillRegistry.validate_plan()` is dead code — never called in production - `SkillRegistry.validate_skill()` is dead code — never called in production - The MCP `notifications/tools/list_changed` event handling described in `SkillRegistry` docstring is never wired up - The `SkillService` has no `refresh()` or `validate_plan()` methods - Skills may become stale with no mechanism to detect or correct this in production ### Code Locations - `src/cleveragents/skills/registry.py` — `SkillRegistry` class (unused in production) - `src/cleveragents/application/services/skill_service.py` — `SkillService` class (used, but lacks registry features) - `src/cleveragents/application/services/skill_registry_service.py` — potential bridge (needs investigation) ### Steps to Reproduce 1. Search the codebase for `SkillRegistry()` instantiation — it is never instantiated in production code 2. Check `SkillService` — it has no `refresh()`, `validate_plan()`, or `validate_skill()` methods 3. The `SkillRegistry` class is only referenced in tests ## Subtasks - [ ] Investigate `SkillRegistryService` (`src/cleveragents/application/services/skill_registry_service.py`) to determine if it bridges `SkillRegistry` and `SkillService`, and document findings - [ ] Determine the correct integration strategy: either (a) have `SkillService` delegate to `SkillRegistry` internally, or (b) expose `SkillRegistry` directly through the service layer - [ ] Add `refresh(name: str)` and `refresh_all()` methods to `SkillService` (or wire through to `SkillRegistry`) - [ ] Add `validate_plan(plan)` and `validate_skill(skill)` methods to `SkillService` (or wire through to `SkillRegistry`) - [ ] Wire up MCP `notifications/tools/list_changed` event handling as described in `SkillRegistry` docstring - [ ] Ensure `SkillRegistry` is instantiated and used by the production service layer (not just tests) - [ ] Write BDD Behave scenarios covering: `refresh()`, `refresh_all()`, `validate_plan()`, `validate_skill()`, and `tools()` via the service layer - [ ] Update integration tests (Robot Framework) to verify end-to-end skill refresh and validation flows - [ ] Verify no regression in existing `SkillService` behaviour ## Definition of Done - [ ] `SkillRegistry` is instantiated and used by `SkillService` (or a clearly documented architectural decision explains why they remain separate) - [ ] `SkillService` exposes `refresh()`, `refresh_all()`, `validate_plan()`, and `validate_skill()` operations as required by the spec - [ ] MCP `notifications/tools/list_changed` event handling is wired up in the production service layer - [ ] No dead code remains: all public methods of `SkillRegistry` are reachable through the production service layer - [ ] BDD unit tests (Behave) cover all new/modified service methods - [ ] All nox stages pass - [ ] Coverage >= 97% --- **Automated by CleverAgents Bot** Supervisor: UAT Testing | Agent: ca-uat-tester
freemo added this to the v3.7.0 milestone 2026-04-05 02:43:48 +00:00
Author
Owner

Issue triaged by project owner:

  • State: Verified
  • Priority: Medium (confirmed) — Two parallel in-memory registries with no integration is an architectural issue but doesn't cause runtime failures.
  • MoSCoW: Should Have — The dual registry creates confusion and potential inconsistency, but the system functions.

Valid UAT finding.


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

Issue triaged by project owner: - **State**: Verified - **Priority**: Medium (confirmed) — Two parallel in-memory registries with no integration is an architectural issue but doesn't cause runtime failures. - **MoSCoW**: Should Have — The dual registry creates confusion and potential inconsistency, but the system functions. Valid UAT finding. --- **Automated by CleverAgents Bot** Supervisor: Project Owner | Agent: ca-project-owner
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Reference
cleveragents/cleveragents-core#2892
No description provided.