@phase1 @domain @skill_service_coverage
Feature: SkillService uncovered code paths
  As a developer maintaining the SkillService
  I want comprehensive test coverage for defensive guards, exception
  handling in database persistence, and the update-persist path
  So that edge cases and failure modes are verified

  # ────────────────────────────────────────────────────────
  # add_skill: None config guard (line 95)
  # ────────────────────────────────────────────────────────

  @ssc_none_config
  Scenario: add_skill raises ValueError when config is None
    Given a skill service without persistence
    When I call add_skill with a None config
    Then a ssc ValueError should be raised with message "config cannot be None"

  # ────────────────────────────────────────────────────────
  # _load_from_db: early return when repo is None (line 278)
  # ────────────────────────────────────────────────────────

  @ssc_load_no_repo
  Scenario: _load_from_db returns immediately when no repo is set
    Given a skill service without persistence
    When I explicitly call _load_from_db
    Then the skills cache should remain empty
    And no ssc error should be raised

  # ────────────────────────────────────────────────────────
  # _load_from_db: exception handling (lines 287-288)
  # ────────────────────────────────────────────────────────

  @ssc_load_db_error
  Scenario: _load_from_db logs warning when list_all raises an exception
    Given a mock skill repo that raises on list_all
    When I create a skill service with the failing repo
    Then the skills cache should remain empty
    And a warning about failed database load should be logged

  # ────────────────────────────────────────────────────────
  # _persist_skill: update path (line 298)
  # ────────────────────────────────────────────────────────

  @ssc_persist_update
  Scenario: Updating an existing skill calls repo.update instead of repo.create
    Given a skill service with a mock repo and session factory
    And I add a skill "local/update-target" via config
    When I update the skill "local/update-target" via add_skill with update flag
    Then the mock repo update method should have been called
    And the mock repo create method should have been called once

  # ────────────────────────────────────────────────────────
  # _persist_skill: exception handling on create (lines 300-304)
  # ────────────────────────────────────────────────────────

  @ssc_persist_create_error
  Scenario: _persist_skill logs warning when repo.create raises
    Given a mock skill repo that raises on create
    And a skill service with the failing-create repo
    When I add a skill "local/fail-persist" via config ignoring persist errors
    Then the skill should still be in the in-memory cache
    And a warning about failed persist should be logged

  # ────────────────────────────────────────────────────────
  # _persist_skill: exception handling on update (lines 300-304)
  # ────────────────────────────────────────────────────────

  @ssc_persist_update_error
  Scenario: _persist_skill logs warning when repo.update raises
    Given a mock skill repo that raises on update
    And a skill service with the failing-update repo
    And I add a new skill "local/upd-fail" to the service
    When I update skill "local/upd-fail" with update flag ignoring persist errors
    Then the updated skill should be in the in-memory cache
    And a warning about failed persist on update should be logged

  # ────────────────────────────────────────────────────────
  # _delete_skill_from_db: exception handling (lines 314-318)
  # ────────────────────────────────────────────────────────

  @ssc_delete_db_error
  Scenario: _delete_skill_from_db logs warning when repo.delete raises
    Given a mock skill repo that raises on delete
    And a skill service with the failing-delete repo
    And I add a skill "local/del-fail" to the failing-delete service
    When I remove skill "local/del-fail" from the service
    Then the skill should be removed from the in-memory cache
    And a warning about failed database delete should be logged

  # ────────────────────────────────────────────────────────
  # _commit: early return when no session_factory (line 324)
  # ────────────────────────────────────────────────────────

  @ssc_commit_no_session
  Scenario: _commit returns early when session_factory is None
    Given a skill service with a mock repo but no session factory
    When I add a skill "local/no-session" via config on the no-session service
    Then the skill should be in the no-session service cache
    And no ssc error should be raised

  # ────────────────────────────────────────────────────────
  # _commit: exception handling (lines 328-329)
  # ────────────────────────────────────────────────────────

  @ssc_commit_error
  Scenario: _commit logs warning when session.commit raises
    Given sscb a mock session factory that raises on commit
    And a skill service with a mock repo and the failing session factory
    When I add a skill "local/commit-fail" via config on the failing-commit service
    Then the skill should be in the failing-commit service cache
    And a warning about failed session commit should be logged