feat: send bus message on skill load

[mikejgray]

Closes #587

Summary by CodeRabbit

• New Features

  • Emits a system event when a plugin skill is successfully loaded, improving real-time status updates and integrations (also ensures the event is emitted in additional load scenarios).

• Tests

  • Added unit tests covering plugin skill load success, load failure, and loader-returning-false scenarios to validate notifications, registry updates, and behavior across outcomes.

[coderabbitai]

Warning

Rate limit exceeded

@mikejgray has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 12 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 39e2a03 and 461d9d0.

📒 Files selected for processing (1)

• ovos_core/skill_manager.py (1 hunks)

Walkthrough

Adds a second try block in SkillManager._load_plugin_skill that re-invokes loader.load(skill_plugin) and emits "mycroft.skill.loaded" on truthy result, potentially causing a duplicated load path; also adds three unit tests covering success, exception, and False-return scenarios for plugin skill loading.

Changes

Cohort / File(s)	Summary
Core: Plugin load control flow ovos_core/skill_manager.py	Adds an outer try that re-invokes loader.load(skill_plugin) after the existing load attempt and, if truthy, emits "mycroft.skill.loaded" with {"skill_id": skill_id}; retains original exception handling and finally stores the loader in self.plugin_skills.
Tests: Plugin load scenarios test/unittests/test_skill_manager.py	Adds three tests: success (load returns loader → emit message, store loader, return loader), failure via exception (loader.load raises → log exception, store skill_id, return None, no success emit), and loader returning False (emit success message, store loader, return None).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant SM as SkillManager
  participant PL as PluginSkillLoader
  participant MB as MessageBus

  note right of SM #DDEEFF: Original inner load attempt
  SM->>PL: loader.load(skill_plugin)  -- inner try
  alt inner load raises exception
    PL--xSM: Exception
    SM->>SM: log exception
  else inner load returns (truthy/false)
    PL-->>SM: result
  end

  note right of SM #E8F8E8: Added outer try re-invokes load
  SM->>PL: loader.load(skill_plugin)  -- outer try (re-invoke)
  alt outer load raises exception
    PL--xSM: Exception
    SM->>SM: log exception
  else outer load returns truthy
    PL-->>SM: truthy
    SM->>SM: store loader in plugin_skills[skill_id]
    SM->>MB: emit("mycroft.skill.loaded", {skill_id})
    SM-->>SM: return loader
  else outer load returns False
    PL-->>SM: False
    SM->>SM: store loader in plugin_skills[skill_id]
    SM->>MB: emit("mycroft.skill.loaded", {skill_id})
    SM-->>SM: return None
  end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

A rabbit hops through lines of code so neat,
Two little loads — a double heartbeat.
I twitch my nose, the bus bells chime,
“Skill loaded!” echoes—once or twice in time.
Hooray for tests that make things right 🌿🐇

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title concisely states the main feature introduced by this PR, namely emitting a bus message when skills are loaded, without extraneous details or noise.
Linked Issues Check	✅ Passed	The pull request implements the notification requirement from issue #587 by emitting a bus message upon successful skill load and includes tests to verify this behavior; it directly addresses the user experience objective without adding unrelated features.
Out of Scope Changes Check	✅ Passed	All changes within the PR focus exclusively on adding and testing the bus message emission on skill load, and there are no modifications unrelated to the notification feature described in the linked issue.
Docstring Coverage	✅ Passed	Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 807130d and 6fe35d5.

📒 Files selected for processing (2)

• ovos_core/skill_manager.py (1 hunks)
• test/unittests/test_skill_manager.py (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

ovos_core/skill_manager.py (2)

test/unittests/test_skill_manager.py (1)

• emit (41-43)

test/unittests/test_skill_installer.py (1)

• emit (21-23)

test/unittests/test_skill_manager.py (1)

ovos_core/skill_manager.py (2)

• _get_plugin_skill_loader (327-343)
• _load_plugin_skill (345-365)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: license_tests
• GitHub Check: unit_tests

🔇 Additional comments (2)

test/unittests/test_skill_manager.py (2)

179-216: LGTM!

The test correctly verifies that on successful load:

• The mycroft.skill.loaded message is emitted with the skill_id
• The loader is stored in plugin_skills
• The loader is returned

---

218-250: LGTM!

The test correctly verifies that when an exception is raised during load:

• No success message is emitted
• The exception is logged
• The loader is still stored in plugin_skills (via finally block)
• None is returned

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6fe35d5 and 39e2a03.

📒 Files selected for processing (2)

• ovos_core/skill_manager.py (1 hunks)
• test/unittests/test_skill_manager.py (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

ovos_core/skill_manager.py (2)

test/unittests/test_skill_manager.py (1)

• emit (41-43)

test/unittests/test_skill_installer.py (1)

• emit (21-23)

test/unittests/test_skill_manager.py (1)

ovos_core/skill_manager.py (2)

• _get_plugin_skill_loader (327-343)
• _load_plugin_skill (345-357)

🪛 Ruff (0.13.3)

ovos_core/skill_manager.py

358-358: unindent does not match any outer indentation level

(invalid-syntax)

---

362-362: unindent does not match any outer indentation level

(invalid-syntax)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: unit_tests

[github-actions]

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
ovos_core
skill_manager.py
Project Total

This report was generated by python-coverage-comment-action