UAT: SkillService.get_skill and remove_skill raise KeyError instead of domain NotFoundError #3678

New issue

Open

opened 2026-04-05 21:30:46 +00:00 by freemo · 1 comment

freemo commented

2026-04-05 21:30:46 +00:00

Owner

Background and Context

The service layer is designed to use consistent domain exceptions across all services. All other services (ActorService, PlanLifecycleService, ResourceRegistryService, SessionService) raise NotFoundError (from cleveragents.core.exceptions) when a resource is not found. The SkillService deviates from this pattern, raising a raw Python KeyError instead.

Per the specification, the service layer must present a uniform exception contract to callers (CLI handlers, integration tests, and other services). Inconsistent exception types break generic error handlers and cause raw Python tracebacks to surface to end users.

Current Behavior

SkillService.get_skill() (line ~220) raises KeyError when the skill is not found:

def get_skill(self, name: str) -> Skill:
    if name not in self._skills:
        raise KeyError(f"Skill '{name}' is not registered")
    return self._skills[name]

SkillService.remove_skill() (line ~190) also raises KeyError:

def remove_skill(self, name: str) -> Skill:
    if name not in self._skills:
        raise KeyError(f"Skill '{name}' is not registered")

Code location: src/cleveragents/application/services/skill_service.py

get_skill() method (~line 220)
remove_skill() method (~line 190)

Steps to reproduce:

Create a SkillService instance
Call get_skill("nonexistent/skill")
Observe KeyError is raised instead of NotFoundError

Expected Behavior

Both get_skill() and remove_skill() should raise NotFoundError from cleveragents.core.exceptions, consistent with all other services in the application layer:

from cleveragents.core.exceptions import NotFoundError

def get_skill(self, name: str) -> Skill:
    if name not in self._skills:
        raise NotFoundError(resource_type="skill", resource_id=name)
    return self._skills[name]

Impact:

CLI error handling code that catches NotFoundError will not catch KeyError from SkillService, leading to unhandled exceptions bubbling up to the user as raw Python tracebacks instead of user-friendly error messages.
Inconsistent error handling across the service layer makes it harder to write generic error handlers.

Metadata

Branch: fix/skill-service-not-found-error
Commit Message: fix(skill-service): replace KeyError with NotFoundError in get_skill and remove_skill
Milestone: Backlog (no milestone assigned)
Parent Epic: Needs manual linking — no dedicated Service Layer Epic exists; candidate: #392 (Epic: Actor YAML & Compiler)

Subtasks

Write TDD issue-capture Behave scenario tagged @tdd_expected_fail demonstrating KeyError is raised (before fix)
Replace KeyError with NotFoundError in SkillService.get_skill() (~line 220)
Replace KeyError with NotFoundError in SkillService.remove_skill() (~line 190)
Ensure NotFoundError is imported from cleveragents.core.exceptions in skill_service.py
Update/add Behave unit test scenarios covering get_skill and remove_skill raising NotFoundError
Update any existing tests that currently assert KeyError is raised (if any)
Run nox -e typecheck — fix any type errors
Run nox -e unit_tests — all scenarios pass
Run nox -e coverage_report — coverage >= 97%
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(skill-service): replace KeyError with NotFoundError in get_skill and remove_skill
The commit is pushed to the remote on the branch matching the Branch in Metadata exactly: fix/skill-service-not-found-error
The commit is submitted as a pull request to master, reviewed, and merged.
All nox stages pass
Coverage >= 97%

Backlog note: This issue was discovered during autonomous operation
on milestone v3.5.0. It does not block milestone completion and has been
placed in the backlog for human review and future milestone assignment.

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

## Background and Context The service layer is designed to use consistent domain exceptions across all services. All other services (`ActorService`, `PlanLifecycleService`, `ResourceRegistryService`, `SessionService`) raise `NotFoundError` (from `cleveragents.core.exceptions`) when a resource is not found. The `SkillService` deviates from this pattern, raising a raw Python `KeyError` instead. Per the specification, the service layer must present a uniform exception contract to callers (CLI handlers, integration tests, and other services). Inconsistent exception types break generic error handlers and cause raw Python tracebacks to surface to end users. ## Current Behavior `SkillService.get_skill()` (line ~220) raises `KeyError` when the skill is not found: ```python def get_skill(self, name: str) -> Skill: if name not in self._skills: raise KeyError(f"Skill '{name}' is not registered") return self._skills[name] ``` `SkillService.remove_skill()` (line ~190) also raises `KeyError`: ```python def remove_skill(self, name: str) -> Skill: if name not in self._skills: raise KeyError(f"Skill '{name}' is not registered") ``` **Code location:** `src/cleveragents/application/services/skill_service.py` - `get_skill()` method (~line 220) - `remove_skill()` method (~line 190) **Steps to reproduce:** 1. Create a `SkillService` instance 2. Call `get_skill("nonexistent/skill")` 3. Observe `KeyError` is raised instead of `NotFoundError` ## Expected Behavior Both `get_skill()` and `remove_skill()` should raise `NotFoundError` from `cleveragents.core.exceptions`, consistent with all other services in the application layer: ```python from cleveragents.core.exceptions import NotFoundError def get_skill(self, name: str) -> Skill: if name not in self._skills: raise NotFoundError(resource_type="skill", resource_id=name) return self._skills[name] ``` **Impact:** - CLI error handling code that catches `NotFoundError` will not catch `KeyError` from `SkillService`, leading to unhandled exceptions bubbling up to the user as raw Python tracebacks instead of user-friendly error messages. - Inconsistent error handling across the service layer makes it harder to write generic error handlers. ## Metadata - **Branch**: `fix/skill-service-not-found-error` - **Commit Message**: `fix(skill-service): replace KeyError with NotFoundError in get_skill and remove_skill` - **Milestone**: Backlog (no milestone assigned) - **Parent Epic**: Needs manual linking — no dedicated Service Layer Epic exists; candidate: #392 (Epic: Actor YAML & Compiler) ## Subtasks - [ ] Write TDD issue-capture Behave scenario tagged `@tdd_expected_fail` demonstrating `KeyError` is raised (before fix) - [ ] Replace `KeyError` with `NotFoundError` in `SkillService.get_skill()` (~line 220) - [ ] Replace `KeyError` with `NotFoundError` in `SkillService.remove_skill()` (~line 190) - [ ] Ensure `NotFoundError` is imported from `cleveragents.core.exceptions` in `skill_service.py` - [ ] Update/add Behave unit test scenarios covering `get_skill` and `remove_skill` raising `NotFoundError` - [ ] Update any existing tests that currently assert `KeyError` is raised (if any) - [ ] Run `nox -e typecheck` — fix any type errors - [ ] Run `nox -e unit_tests` — all scenarios pass - [ ] Run `nox -e coverage_report` — coverage >= 97% - [ ] 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(skill-service): replace KeyError with NotFoundError in get_skill and remove_skill` - The commit is pushed to the remote on the branch matching the **Branch** in Metadata exactly: `fix/skill-service-not-found-error` - The commit is submitted as a **pull request** to `master`, reviewed, and **merged**. - All nox stages pass - Coverage >= 97% > **Backlog note:** This issue was discovered during autonomous operation > on milestone v3.5.0. It does not block milestone completion and has been > placed in the backlog for human review and future milestone assignment. --- **Automated by CleverAgents Bot** Supervisor: UAT Testing | Agent: ca-uat-tester

freemo added a new dependency

2026-04-05 21:31:06 +00:00

#392 Epic: Actor YAML & Compiler

freemo commented

2026-04-05 22:14:03 +00:00

Author

Owner

Issue triaged by project owner:

State: Verified
Priority: Backlog — Service layer consistency issue. The SkillService works correctly but uses the wrong exception type, causing inconsistent error handling.
Milestone: None (backlog)
Story Points: 1 — XS — Replace KeyError with NotFoundError in 2 methods. Trivial change.
MoSCoW: Should Have — The service layer should present a uniform exception contract. This inconsistency causes raw Python tracebacks to surface to end users when skills are not found.
Parent Epic: #392

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

Issue triaged by project owner: - **State**: Verified - **Priority**: Backlog — Service layer consistency issue. The SkillService works correctly but uses the wrong exception type, causing inconsistent error handling. - **Milestone**: None (backlog) - **Story Points**: 1 — XS — Replace `KeyError` with `NotFoundError` in 2 methods. Trivial change. - **MoSCoW**: Should Have — The service layer should present a uniform exception contract. This inconsistency causes raw Python tracebacks to surface to end users when skills are not found. - **Parent Epic**: #392 --- **Automated by CleverAgents Bot** Supervisor: Project Owner | Agent: ca-project-owner

freemo referenced this issue

2026-04-05 22:16:05 +00:00

[Automated] CleverAgents Build Session - 2026-04-05 (Resumed) #3654

freemo referenced this issue

2026-04-05 22:20:24 +00:00

[Automated] CleverAgents Build Session - 2026-04-05 (Resumed) #3654