Repository navigation
fix(skills): prune uninstalled skill from disabled config - #51422
briandevans wants to merge 2 commits into
Conversation
`hermes skills uninstall <name>` removed the skill from the hub lock file but left its entry in skills.disabled / skills.platform_disabled in config.yaml. That config disable-state is stored separately from the lock file, which uninstall_skill already reconciles on its own. The orphaned entry meant a later re-install of the same name came back silently disabled (the agent never loaded it, with no hint why) and the enabled/disabled count in the skills summary was wrong. Prune the uninstalled name from the global disabled list and every platform_disabled[*] in do_uninstall after a successful uninstall. The prune is best-effort and never aborts a successful uninstall.
There was a problem hiding this comment.
Pull request overview
Fixes a CLI bug where hermes skills uninstall <name> removed a hub-installed skill from the hub lock, but left its disable-state behind in config.yaml (skills.disabled / skills.platform_disabled), which could cause a later reinstall to be silently disabled and skew hermes skills counts.
Changes:
- After a successful uninstall, prunes the uninstalled skill name from
skills.disabledand allskills.platform_disabled[*]lists. - Adds regression tests verifying the prune behavior and the no-op behavior when the skill was never disabled.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
hermes_cli/skills_hub.py |
Adds best-effort pruning of disable-state from config after uninstall. |
tests/hermes_cli/test_skills_skip_confirm.py |
Adds regression tests covering the new prune behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| from hermes_cli.config import load_config, save_config | ||
| cfg = load_config() | ||
| skills_cfg = cfg.get("skills") | ||
| if isinstance(skills_cfg, dict): | ||
| changed = False | ||
| global_disabled = skills_cfg.get("disabled") | ||
| if isinstance(global_disabled, list) and name in global_disabled: | ||
| skills_cfg["disabled"] = [s for s in global_disabled if s != name] | ||
| changed = True | ||
| platform_disabled = skills_cfg.get("platform_disabled") | ||
| if isinstance(platform_disabled, dict): | ||
| for platform, names in list(platform_disabled.items()): | ||
| if isinstance(names, list) and name in names: | ||
| platform_disabled[platform] = [s for s in names if s != name] | ||
| changed = True | ||
| if changed: | ||
| save_config(cfg) | ||
| except Exception: | ||
| # Best-effort: a config prune must never abort a successful uninstall. | ||
| pass |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Prune uninstalled skill from disabled config.
Looks Good
- Good fix for a real issue: reinstalling a skill that was previously disabled comes back silently disabled
- Properly handles both global and per-platform disabled lists
- Best-effort pattern (catches exceptions, never aborts uninstall)
- Good test coverage: tests both the prune and the no-op cases
- Clean implementation that integrates with existing config system
Reviewed by Hermes Agent
The best-effort disable-state prune in do_uninstall read the config via load_config(), which deep-merges DEFAULT_CONFIG and expands env refs. Passing that result to save_config() rewrote config.yaml with the fully expanded/merged tree, churning large amounts of unrelated config from a narrow skill-uninstall cleanup. Read the raw on-disk config with read_raw_config() so only the user's own keys are touched (the documented read -> mutate -> save_config pattern), and surface a warning when the prune fails instead of silently swallowing the exception, so the "silently disabled on reinstall" symptom can't be reintroduced without any trace.
|
@copilot Both findings addressed in commit 35563c4:
Full |
|
Draining our contribution queue to keep it lean and reviewable \u2014 closing this as stale (no reviewer traction in 2+ weeks). If the underlying issue is still live on current main, we'll re-file a fresh, focused fix. |
What does this PR do?
hermes skills uninstall <name>removed the skill from the hub lock file but left the skill's entry inskills.disabled/skills.platform_disabledinconfig.yaml. That config disable-state is stored and reconciled separately from the hub lock file —uninstall_skill()only pops the lock entry (tools/skills_hub.py), and nothing pruned the config side.The orphaned entry has two user-visible consequences:
hermes skillsenabled/disabled summary count is wrong, because it still counts the uninstalled name as a disabled skill.This prunes the uninstalled name from the global disabled list and from every
platform_disabled[*]entry indo_uninstall, right after a successful uninstall. The prune is best-effort: an unreadable/locked config must never abort an otherwise-successful uninstall.The fix lives in the CLI layer (
hermes_cli/skills_hub.py:do_uninstall), which already owns config and already depends ontools—do_uninstallis the only production caller ofuninstall_skill. The hub lock-file layer (tools/) is left untouched and gains no new dependency onhermes_cli.Type of Change
Changes Made
hermes_cli/skills_hub.py— indo_uninstall, after a successfuluninstall_skill, prune the uninstalled name fromskills.disabledand everyskills.platform_disabled[*]list inconfig.yaml, thensave_configonly if something changed. Wrapped best-effort so a config failure cannot break the uninstall.tests/hermes_cli/test_skills_skip_confirm.py— regression tests covering (a) prune of both global and per-platform disabled entries while leaving unrelated entries intact, and (b) no-op when the uninstalled skill was never disabled.How to Test
skills.disabledandskills.platform_disabled[<platform>]in~/.hermes/config.yaml.hermes skills uninstall <name>.config.yaml— the name is gone from bothskills.disabledandplatform_disabled; unrelated disabled skills are untouched.Regression test (fails before the fix, passes after):
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/A (no new config keys)CONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A