Skip to content

fix: handle None skills config in skills_config helpers - #15501

Closed
Kailigithub wants to merge 1 commit into
NousResearch:mainfrom
Kailigithub:fix/issue-15486-skills-config-none-safety
Closed

fix: handle None skills config in skills_config helpers#15501
Kailigithub wants to merge 1 commit into
NousResearch:mainfrom
Kailigithub:fix/issue-15486-skills-config-none-safety

Conversation

@Kailigithub

Copy link
Copy Markdown
Contributor

When skills is set to None in config.yaml, get_disabled_skills() crashes with AttributeError: 'NoneType' object has no attribute 'get', and save_disabled_skills() would overwrite the key with a dict without preserving the user's intent.

Changes:

  • get_disabled_skills: use or {} / or [] guards so None is treated the same as an absent key
  • save_disabled_skills: replace config.setdefault("skills", {}) with an isinstance check so a non-dict value (including None) is replaced rather than silently kept

All 28 existing tests in test_skills_config.py pass; web-server skill endpoint tests also pass.

Closes #15486

When skills is set to None in config.yaml, get_disabled_skills would
crash with AttributeError and save_disabled_skills would create a
broken state. Use `or {}` / `or []` guards and isinstance check to
handle None gracefully.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard tool/skills Skills system (list, view, manage) labels Apr 25, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of #13078 — both fix get_disabled_skills() crashing on None/null skills config. #13078 also addresses the scalar-splits-into-characters case from #13026.

@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of #13078

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the null-config hardening work. This is an automated hermes-sweeper review; the reported behavior is now implemented on current main.

The PR’s getter hunk predates the current global-plus-platform union semantics, so it should not be applied over main.

@teknium1 teknium1 closed this Jul 12, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists sweeper:implemented-on-main Sweeper: behavior already present on current main 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.

Bug: Web server returns 500 error when skills configuration returns None

4 participants