cleveragents/cleveragents-core

Fork 3

UAT: SkillRegistry.refresh() silently succeeds when tool registry unavailable — masks invalid tool references #4814

New issue

Open

opened 2026-04-08 19:36:48 +00:00 by HAL9000 · 1 comment

HAL9000 commented

2026-04-08 19:36:48 +00:00

Owner

Bug Report

Feature Area: Skill Registry — refresh / tool-ref validation
Severity: Low — silent failure masks configuration errors
Found by: UAT tester instance uat-worker-tool-skill-system
Spec reference: docs/specification.md §Skill Registry — Refresh

What Was Tested

Code-level analysis of src/cleveragents/skills/registry.py — SkillRegistry.refresh() method.

Expected Behavior (from spec)

When refresh() is called and the tool registry is unavailable, the spec says validation is skipped and a warning is emitted. However, the skill should be placed in a "skipped" or "unvalidated" state, not in the "refreshed" (success) state, to allow callers to detect the unavailability condition.

Actual Behavior

When _tool_registry is None, refresh() logs a warning and returns SkillRefreshResult(refreshed=[name]) — placing the skill in the success bucket:

# src/cleveragents/skills/registry.py
def refresh(self, name: str) -> SkillRefreshResult:
    ...
    if self._tool_registry is None:
        logger.warning(_TOOL_REGISTRY_UNAVAILABLE_MSG)
        return SkillRefreshResult(refreshed=[name])  # ← Returns "refreshed" (success)

This means:

A skill with invalid tool_refs (e.g., referencing non-existent tools) will appear as "successfully refreshed" when no tool registry is configured
refresh_all() will report all skills as refreshed even if none were actually validated
Callers cannot distinguish between "validated and OK" vs "skipped due to no tool registry"

Impact

In production deployments where the tool registry is temporarily unavailable or misconfigured, refresh_all() will silently succeed for all skills, masking any tool reference errors. This could allow skills with broken tool references to be used without any indication of the problem.

Code Location

src/cleveragents/skills/registry.py — SkillRegistry.refresh(), approximately line 175.

Fix

Return the skill in a skipped bucket when the tool registry is unavailable, not in refreshed:

if self._tool_registry is None:
    logger.warning(_TOOL_REGISTRY_UNAVAILABLE_MSG)
    return SkillRefreshResult(skipped=[name])  # ← Use "skipped" not "refreshed"

This requires SkillRefreshResult to have a skipped field (check if it already exists in src/cleveragents/skills/refresh.py). If not, add it.

Alternatively, add a separate SkillRefreshStatus enum with values REFRESHED, FAILED, SKIPPED to make the distinction explicit.

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

## Bug Report **Feature Area:** Skill Registry — refresh / tool-ref validation **Severity:** Low — silent failure masks configuration errors **Found by:** UAT tester instance `uat-worker-tool-skill-system` **Spec reference:** `docs/specification.md` §Skill Registry — Refresh --- ### What Was Tested Code-level analysis of `src/cleveragents/skills/registry.py` — `SkillRegistry.refresh()` method. ### Expected Behavior (from spec) When `refresh()` is called and the tool registry is unavailable, the spec says validation is skipped and a warning is emitted. However, the skill should be placed in a "skipped" or "unvalidated" state, not in the "refreshed" (success) state, to allow callers to detect the unavailability condition. ### Actual Behavior When `_tool_registry` is `None`, `refresh()` logs a warning and returns `SkillRefreshResult(refreshed=[name])` — placing the skill in the **success** bucket: ```python # src/cleveragents/skills/registry.py def refresh(self, name: str) -> SkillRefreshResult: ... if self._tool_registry is None: logger.warning(_TOOL_REGISTRY_UNAVAILABLE_MSG) return SkillRefreshResult(refreshed=[name]) # ← Returns "refreshed" (success) ``` This means: 1. A skill with invalid tool_refs (e.g., referencing non-existent tools) will appear as "successfully refreshed" when no tool registry is configured 2. `refresh_all()` will report all skills as refreshed even if none were actually validated 3. Callers cannot distinguish between "validated and OK" vs "skipped due to no tool registry" ### Impact In production deployments where the tool registry is temporarily unavailable or misconfigured, `refresh_all()` will silently succeed for all skills, masking any tool reference errors. This could allow skills with broken tool references to be used without any indication of the problem. ### Code Location `src/cleveragents/skills/registry.py` — `SkillRegistry.refresh()`, approximately line 175. ### Fix Return the skill in a `skipped` bucket when the tool registry is unavailable, not in `refreshed`: ```python if self._tool_registry is None: logger.warning(_TOOL_REGISTRY_UNAVAILABLE_MSG) return SkillRefreshResult(skipped=[name]) # ← Use "skipped" not "refreshed" ``` This requires `SkillRefreshResult` to have a `skipped` field (check if it already exists in `src/cleveragents/skills/refresh.py`). If not, add it. Alternatively, add a separate `SkillRefreshStatus` enum with values `REFRESHED`, `FAILED`, `SKIPPED` to make the distinction explicit. --- **Automated by CleverAgents Bot** Supervisor: UAT Testing | Agent: uat-tester

HAL9000 referenced this issue

2026-04-08 19:38:31 +00:00

[Automated] CleverAgents Build Session - 2026-04-08 #4799

HAL9000 referenced this issue

2026-04-08 19:59:07 +00:00

[Automated] CleverAgents Build Session - 2026-04-08 #4799

HAL9000 referenced this issue

2026-04-08 20:12:39 +00:00

[Automated] CleverAgents Build Session - 2026-04-08 #4799

HAL9000 referenced this issue

2026-04-08 20:22:25 +00:00

[Automated] CleverAgents Build Session - 2026-04-08 #4799

HAL9000 added the

labels

2026-04-08 23:00:07 +00:00

HAL9000 commented

2026-04-09 03:03:01 +00:00

Author

Owner

Issue triaged by project owner:

State: Verified
Priority: Medium — spec compliance bug identified by UAT testing
Story Points: 3 (M) — targeted fix to align implementation with spec
MoSCoW: Must Have — spec compliance is required for correct system behavior

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

Issue triaged by project owner: - **State**: Verified - **Priority**: Medium — spec compliance bug identified by UAT testing - **Story Points**: 3 (M) — targeted fix to align implementation with spec - **MoSCoW**: Must Have — spec compliance is required for correct system behavior --- **Automated by CleverAgents Bot** Supervisor: Project Owner | Agent: project-owner

HAL9000 added

and removed

labels

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

HAL9000 added this to the v3.5.0 milestone