Skip to content

fix(skills-config): normalize scalar/None disabled-skill entries (salvage of #14094 by @LeonSGP43) - #52895

Closed
Bartok9 wants to merge 1 commit into
NousResearch:mainfrom
Bartok9:salvage/14094-skills-config-normalize
Closed

fix(skills-config): normalize scalar/None disabled-skill entries (salvage of #14094 by @LeonSGP43)#52895
Bartok9 wants to merge 1 commit into
NousResearch:mainfrom
Bartok9:salvage/14094-skills-config-normalize

Conversation

@Bartok9

@Bartok9 Bartok9 commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Summary

Salvage of #14094 by @LeonSGP43 — rebased onto current origin/main (preserving main's global|platform union semantics) with regression tests.

get_disabled_skills does set(skills_cfg.get("disabled", [])). A scalar disabled: my-skill becomes a set of its characters, and skills: null raises AttributeError on .get().

Root Cause

Symptom — Two config-shape footguns: (1) disabled: my-skill (a common single-skill mistake) silently disables skills named m, y, -, s, … instead of my-skill; (2) skills: null (valid YAML) crashes skill-disable resolution with AttributeError.

Root cause — In get_disabled_skills (hermes_cli/skills_config.py), skills_cfg = config.get("skills", {}) can be None (→ .get AttributeError), and set(scalar_string) iterates the string into characters. Same for platform_disabled values.

Evidence — The added tests cover scalar disabled, skills: null, and scalar platform_disabled; all three fail without the fix (char-set / AttributeError).

Fix + why this level — Add _normalize_string_set() (None→empty, scalar→single-element, list→trimmed set) and guard the skills section with isinstance(..., dict). Applied at get_disabled_skills, the single resolver every caller uses. Main's union semantics (global | platform) are preserved exactly — only the coercion is hardened.

Scope / risk — One helper + guards in one function. List-shaped configs behave identically (verified by the existing 30 tests); only scalar/None shapes change (now tolerant instead of wrong/crashing).

Changes from original

  • Rebased onto current main. The original used a global-fallback shape; main unions global+platform — I kept main's union semantics and applied only the normalization/guard hardening.
  • Carried the original's 3 regression tests — all fail without the fix.

Verification

python3 -m pytest tests/hermes_cli/test_skills_config.py -q
33 passed

# without the fix (stashed): scalar char-set + None AttributeError
3 failed

Real behavior proof

# With fix:    {"disabled":"my-skill"} -> {"my-skill"}; {"skills":None} -> set(); platform scalar -> {"tg-skill"}
# Without fix: "my-skill" -> {'m','y','-','s','k','i','l'}; {"skills":None} -> AttributeError

Credit: salvage of #14094 by @LeonSGP43 — rebased onto current main with guardrail tests.

Salvage of NousResearch#14094 by @LeonSGP43 — rebased onto current main (preserving its
global|platform union semantics) with regression tests that fail without it.
@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard tool/skills Skills system (list, view, manage) area/config Config system, migrations, profiles P2 Medium — degraded but workaround exists labels Jun 26, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Closing — the normalization landed on main via PR #61797 with the same shape as this salvage (and #52895's tests were mirrored there). Credit chain for this fix: @zhanggttry's #13078 (April) was the earliest, then @LeonSGP43's #14094, then this salvage. Thanks for pushing it forward!

@teknium1 teknium1 closed this Jul 10, 2026
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.

3 participants