Skip to content

fix(skills): emit lifecycle events for removals - #83316

Open
Vocllum wants to merge 2 commits into
NousResearch:mainfrom
Vocllum:fix/skill-lifecycle-removal-events
Open

fix(skills): emit lifecycle events for removals#83316
Vocllum wants to merge 2 commits into
NousResearch:mainfrom
Vocllum:fix/skill-lifecycle-removal-events

Conversation

@Vocllum

@Vocllum Vocllum commented Aug 10, 2026

Copy link
Copy Markdown

Summary

  • emit deleted after a successful hard delete through skill_manage
  • emit uninstalled after a successful Skills Hub uninstall
  • preserve provenance before removal and keep curator archives on their existing archived action
  • document the complete observer scope and its filesystem boundary

Why

The merged on_skill_lifecycle observer from #68883 reports skill creation, mutation, loading, and curator transitions, but successful removal paths were silent. Plugins that maintain an external index therefore cannot invalidate a removed skill when Hermes performs the operation.

This extends the existing observer rather than adding another hook. Emission happens only after the authoritative filesystem operation succeeds. Usage-sidecar cleanup remains best-effort because its failure cannot undo a deletion that already occurred.

Scope

Validation

  • 32 passed across skill usage, hard-delete, archive, and Hub uninstall tests
  • Ruff checks passed for all changed Python files
  • git diff --check passed

@Vocllum
Vocllum marked this pull request as ready for review August 10, 2026 16:11
@Vocllum

Vocllum commented Aug 13, 2026

Copy link
Copy Markdown
Author

Hi @teknium1 — could you take a look when convenient? This closes the lifecycle-observer gap for successful hard deletes and Hub uninstalls, while keeping curator archival on its existing archived path. It preserves provenance before removal and emits only after the authoritative operation succeeds. I replayed it on current main and reran the focused suite (147 passed). This would be especially useful for downstream skill-management integrations that need to reconcile removals. Thanks!

@alt-glitch alt-glitch added type/bug Something isn't working tool/skills Skills system (list, view, manage) P3 Low — cosmetic, nice to have labels Aug 13, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix(skills): emit lifecycle events for removals

  1. Hardcoded provenance in hub uninstalltools/skills_hub.py (uninstall_skill): forget(skill_name, lifecycle_action="uninstalled", provenance="installed") always reports "installed" regardless of how the skill actually got there (hub with trust level, external dir, agent-created). The hub lock entry records source/trust_level; deriving provenance from that (or accepting it as a parameter) would make the event truthful for mixed-provenance skill directories.

  2. Event emitted for untracked skillstools/skill_usage.py (forget): when the skill has no usage record (skill_name not in data), the lifecycle event is still emitted with prior_record=None, so consumers receive a deleted/uninstalled event with all stats None for a skill that was never tracked. If that is intended (observer completeness), fine; otherwise consider only emitting when a record existed, or add an explicit flag so hooks can distinguish.

  3. Silent provenance-capture failure — in skill_manage delete, telemetry_provenance(name) is wrapped in a bare try/except Exception: pass; the fallback to record-based provenance inside _emit_skill_lifecycle is good, but a debug-level log of the capture failure would make the fallback diagnosable.

  4. Minor: the forget-with-lifecycle pattern (try/except + debug log) is duplicated between skill_manager_tool.py and skills_hub.py — a small shared helper would keep the two removal paths consistent.

- skills_hub uninstall: derive provenance from hub lock entry instead of
  hardcoding "installed" so mixed-provenance dirs report truthfully
- skill_usage.forget: document untracked-skill emission semantics
  (record=None is the explicit marker for observer completeness)
- skill_manage delete: log provenance-capture failure instead of silent pass
- extract shared forget_with_lifecycle helper for the two removal paths
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low — cosmetic, nice to have tool/skills Skills system (list, view, manage) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants