Skip to content

fix(cli): normalize disabled skills config parsing - #13047

Closed
YangManBOBO wants to merge 1 commit into
NousResearch:mainfrom
YangManBOBO:fix/issue-13026-skills-config-normalization
Closed

fix(cli): normalize disabled skills config parsing#13047
YangManBOBO wants to merge 1 commit into
NousResearch:mainfrom
YangManBOBO:fix/issue-13026-skills-config-normalization

Conversation

@YangManBOBO

Copy link
Copy Markdown

Summary

  • 修复 get_disabled_skills() 对异常配置结构的健壮性:skills: null 不再崩溃。
  • 修复 skills.disabled 为字符串时被拆成字符集合的问题,统一按单个技能名处理。
  • 增加回归测试覆盖上述两种场景,避免后续回归。

Test plan

  • python -m pytest tests/hermes_cli/test_skills_config.py -q -n 4

Closes #13026

兼容 skills:null 与字符串 disabled 配置,避免字符拆分与空对象崩溃,并补充回归测试覆盖。
@trevorgordon981

Copy link
Copy Markdown
Contributor

Great work on this fix! 🎉

This PR properly normalizes the disabled skills config parsing with platform-specific fallback logic. The changes look solid:

Key improvements:

  • Platform-specific disabled skills now fall back to global config correctly
  • Explicit platform params properly override env vars
  • Empty configs and missing keys are handled gracefully
  • All 30 tests pass with comprehensive coverage

Test coverage highlights:

  • Platform fallback logic ()
  • Env var overrides ()
  • Edge cases (empty configs, missing keys, scalar vs list handling)
  • Session-specific platform env vars

The normalization logic ensures consistent behavior across CLI, gateway, and platform-specific contexts. This unblocks proper skill management per-platform while maintaining backward compatibility.

Ready to merge! 🚀

@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) area/config Config system, migrations, profiles labels Apr 22, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Competing fix with #13078 — both target #13026 (get_disabled_skills crash on null/scalar). Maintainer should pick one.

@Bartok9

Bartok9 commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Salvaged onto current main in #50648 (credited to you + @zhanggttry). This branch is now CONFLICTING against main's union rewrite (#46201); #50648 re-targets the same fix and preserves the union semantics. Full credit to your original work. 🙏

@alt-glitch alt-glitch added the duplicate This issue or pull request already exists label Jun 26, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #13078 — same get_disabled_skills() null/scalar normalization for #13026 (both add the same _normalize_string_set handling); #13078 is the earliest open canonical fix. Related (different facet / salvages, not duplicates): #48333 (multi-line block-scalar split), #50648 and #52895 (rebased salvages of the same family).

@alt-glitch alt-glitch removed the duplicate This issue or pull request already exists label Jun 26, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the report and regression tests. This is already implemented on current main via #61797; this is an automated hermes-sweeper review.

@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

area/config Config system, migrations, profiles 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]: get_disabled_skills crashes on skills: null and splits scalar names into characters

5 participants