Skip to content

fix(skills_config): handle null/scalar values in get_disabled_skills - #13078

Closed
zhanggttry wants to merge 2 commits into
NousResearch:mainfrom
zhanggttry:fix/get-disabled-skills-crash-13026
Closed

zhanggttry wants to merge 2 commits into
NousResearch:mainfrom
zhanggttry:fix/get-disabled-skills-crash-13026

Conversation

@zhanggttry

Copy link
Copy Markdown
Contributor

Fix #13026get_disabled_skills crash on null/scalar values

Problem

get_disabled_skills() in hermes_cli/skills_config.py crashes or produces incorrect results when the YAML config contains unexpected but plausible values:

  1. skills: nullAttributeError: 'NoneType' object has no attribute 'get'
  2. skills.disabled: my-skill (scalar string) → set("my-skill") splits into individual characters: {'m', 'y', '-', 's', 'k', 'i', 'l', 'l'}
  3. skills.platform_disabled.telegram: my-skill → same character-splitting bug

These edge cases arise from hand-edited YAML configs where a user might write:

skills:
  disabled: my-skill     # scalar instead of list

or where skills: is set to null by another tool.

Solution

  • Add _normalize_string_set() helper that safely coerces None, scalar strings, and lists into proper Set[str]. This mirrors the existing _normalize_string_set() in agent/skill_utils.py (L144-168) which already handles these cases for the runtime skill loader.
  • Guard skills_cfg against None and non-dict values (config.get("skills") or {})
  • Guard platform_disabled against None values (skills_cfg.get("platform_disabled") or {})

Changes

File Change
hermes_cli/skills_config.py Add _normalize_string_set(), harden get_disabled_skills()
tests/test_skills_config.py New: 18 test cases covering all edge cases

Testing

19 passed, 0 failed ✅

All edge cases covered: null config, scalar strings, empty values, normal lists, platform fallbacks, and overrides.

Fixes NousResearch#13026

- Add _normalize_string_set() helper to safely coerce scalar strings
  and None into proper string sets, mirroring agent.skill_utils
- Guard skills_cfg against null and non-dict values
- Guard platform_disabled against null values
- Add comprehensive test suite for edge cases
Covers null skills config, scalar string disabled values,
and other malformed-but-plausible YAML inputs.
@Bartok9

Bartok9 commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Salvaged your fix onto current main in #50648 (credited to you + @YangManBOBO). This branch predates main's global | platform_disabled union rewrite (#46201) and now conflicts; #50648 re-applies the null/scalar normalization on top of current main while preserving that union semantics, and mirrors the existing agent.skill_utils._normalize_string_set helper. Full credit to your original work. 🙏

@teknium1

Copy link
Copy Markdown
Collaborator

Closing — this fix landed on main via PR #61797. You were the earliest submitter of this fix (April 20, nearly three months before the others), and your implementation was essentially the shape that ended up merged: null → empty set, scalar → single skill name, mirroring agent.skill_utils._normalize_string_set. Apologies that this sat unreviewed long enough for the fix to be rebuilt independently — the credit for first identifying and fixing #13026 is yours. Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists 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]: get_disabled_skills crashes on skills: null and splits scalar names into characters

4 participants