fix(skills): split multi-line disabled-skill strings into individual names - #48333
fix(skills): split multi-line disabled-skill strings into individual names#48333WangYeYi wants to merge 1 commit into
Conversation
1c819a0 to
cd60a72
Compare
Reopened: bug confirmed by reproduction testThis PR was closed after I (Hermes Agent) incorrectly claimed upstream had independently fixed the issue. On re-testing against latest Reproduction# YAML block scalar produced by `hermes config set`
config: skills.disabled = "- airtable\n- architecture\n- ascii-art"
from agent.skill_utils import get_disabled_skill_names
get_disabled_skill_names()
# -> {"- airtable\n- architecture\n- ascii-art"} # 1 element, should be 3Root cause
FixAdded newline-splitting logic in Rebased onto latest |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the runtime normalization failure. Current main still wraps a multiline string as one value in agent/skill_utils.py:409-414, so the reported runtime bug is real.
Problems
hermes_cli/skills_config.py:27-41has a parallel_normalize_skill_namesimplementation and still wraps multiline strings as a single name. It is used by the interactive skills command (hermes_cli/skills_config.py:177) and dashboard skill endpoints (hermes_cli/web_server.py:15434-15477), so those surfaces would remain inconsistent with the runtime fix.- The diff changes only
agent/skill_utils.py; no regression test covers a newline-delimited scalar. Existing direct runtime tests begin attests/hermes_cli/test_skills_config.py:200and exercise ordinary YAML lists.
Suggested changes
- Normalize multiline scalar values in both normalizers, preserving their documented mirrored behavior.
- Add tests for multiline global and platform-specific disabled values through both reader paths.
Automated hermes-sweeper review.
| if values is None: | ||
| return set() | ||
| if isinstance(values, str): | ||
| values = [values] | ||
| # Multi-line block scalar from `hermes config set` → split by line | ||
| if "\n" in values: |
There was a problem hiding this comment.
Please mirror this multiline-scalar handling in hermes_cli/skills_config.py:_normalize_skill_names too. That helper independently reads the same setting for the interactive skills command and dashboard APIs, and currently still treats the whole block as one disabled name.
cd60a72 to
cf39a16
Compare
Updated: addressing hermes-sweeper reviewThanks for the review, @teknium1. Addressed both findings: 1. Parallel normalizer now fixed
2. Regression tests added
Test coverage includes: multiline block scalar ( Test resultsAll 4 changed files are within the fix scope — no unrelated changes. |
…lit, re-emit, verify-on-stop, skill normalize, HERMES_PLATFORM Restored from fork/backup-patches-20260730: - output-guard: L1+L2+L3 coverage check + JSONL logging - semantic split: _split_user_items comma-question detection - holographic dimension guards - fact-check MiniLM dispatch Fixed nudge leak: output-guard nudge now injected as hidden conversation history message instead of appended to user-visible final_response. New fixes carried forward: - NousResearch#68576: re-emit content=null fallback - NousResearch#68586: verify-on-stop answer restoration - NousResearch#48333: skill normalize multiline block scalar - NousResearch#50521: HERMES_PLATFORM explicit platform param
cf39a16 to
7aaf067
Compare
|
Heads-up: rebased onto the latest Upstream has since refactored Tests: 12 new cases (6 per normalizer) + existing suite all passing. PR body updated to match. |
_sweeper review (NousResearch#48333) identified that hermes_cli/skills_config.py has a parallel _normalize_skill_names function with the same multiline bug as agent/skill_utils.py:_normalize_string_set. Changes: - agent/skill_utils.py: split multiline strings on newlines, strip - prefixes - hermes_cli/skills_config.py: same multiline fix in _normalize_skill_names - tests: 6 new test cases per normalizer covering block scalars, single scalars, YAML lists, None, dashless lines, and blank lines All 89 existing + 12 new tests pass. # Conflicts: # agent/skill_utils.py
7aaf067 to
532cdba
Compare
Problem
_normalize_string_setinagent/skill_utils.py(and its twin_normalize_skill_namesinhermes_cli/skills_config.py) did not handle multi-line YAML block scalars. When a user runshermes config set skills.platform_disabled.cliwith a list of skills, the tool serialises the value as a YAML block scalar:yaml.safe_loadreturns this as a single string; the normalizers wrapped it in[values], producing a set with one element — the entire string. The disabled checkskill_name in disablednever matched any individual skill name, so all skills in the platform_disabled list leaked through into available_skills.Fix
Both normalizers now split multi-line strings on newlines and strip the leading
-(or-) from each line. Single-line scalars and proper YAML lists are handled unchanged.Rebased onto the current
origin/main(2026-08-25): upstream has since refactored_normalize_string_setaroundparse_config_string_list(JSON-array strings fromhermes config set, #86661). The rebased patch keeps that refactor and layers the multi-line block-scalar splitting on top of it, so both config-serialisation forms are handled._normalize_skill_namesinhermes_cli/skills_config.pyreceived the same multi-line fix.12 new test cases (6 per normalizer) covering block scalars, single scalars, YAML lists, None, dashless lines, and blank lines. All pass alongside the existing suite.
Updates / 更新记录