Skip to content

fix(skills): return None instead of truthy stub when skill load fails#27147

Closed
0xchainer wants to merge 2 commits into
NousResearch:mainfrom
0xchainer:fix/skill-load-failure-returns-none
Closed

fix(skills): return None instead of truthy stub when skill load fails#27147
0xchainer wants to merge 2 commits into
NousResearch:mainfrom
0xchainer:fix/skill-load-failure-returns-none

Conversation

@0xchainer

@0xchainer 0xchainer commented May 16, 2026

Copy link
Copy Markdown
Contributor

Summary

build_skill_invocation_message() returns a non-empty placeholder string ([Failed to load skill: ...]) when the skill exists in the command cache but loading the actual SKILL.md payload fails. CLI/gateway callers treat any truthy return value as success (using if msg: checks), so the failure text gets injected into the model as a user prompt.

What does this PR do?

Changes the failure return value from a truthy error string to None, matching the existing behavior for unknown commands (/nonexistent → None). This way callers using if msg: properly detect the failure instead of routing bogus prompt text to the LLM.

Changes Made

  • agent/skill_commands.py:428 — return f"[Failed to load skill: ...]" → return None

How to Test

  1. Scan a skill, then delete its SKILL.md
  2. Call build_skill_invocation_message("/skill-name", "hello")
  3. Previously: returns truthy string → injected as user prompt
  4. Now: returns None → callers detect failure properly

Checklist

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I've tested on my platform: macOS

fix(skills): return None instead of truthy stub when skill load fails
877d01b 
build_skill_invocation_message() returns a non-empty placeholder string
('[Failed to load skill: ...]') when the skill exists in the command cache
but loading the actual SKILL.md payload fails. CLI/gateway callers treat
any truthy return value as success, so the failure is silently routed into
the model as if it were a valid skill prompt.

Return None instead, matching the existing behavior for unknown commands,
so callers using 'if msg:' can properly detect the failure.
@cardtest15-coder

This comment was marked as spam.

Sign in to view
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists tool/skills Skills system (list, view, manage) comp/agent Core agent loop, run_agent.py, prompt builder labels May 16, 2026
@alt-glitch

alt-glitch commented May 16, 2026

Copy link
Copy Markdown
Collaborator

Prior fix attempts for this bug (#11408, #14752, #17330) were all closed without merging. Root issue #11200 remains open. This is a valid new attempt at the same fix.

test(skills): add regression test for skill load failure returning None
90c9c85 
Add test_returns_none_when_skill_load_fails to verify that
build_skill_invocation_message() returns None when a registered
skill exists in the command cache but _load_skill_payload() fails.
This guards against regression of the fix in 877d01b.
@0xchainer

0xchainer commented May 16, 2026

Copy link
Copy Markdown
Contributor Author

  1. Логика и потенциальные баги. Изменение сигнатуры функции: ранее всегда возвращалась строка (включая заглушку при ошибке), теперь — None. Это ломает контракт для вызывающего кода. Если хотя бы один коллер ожидает строку и вызывает на результате строковые методы, конкатенирует или форматирует её — получим AttributeError/TypeError. В диффе отсутствуют изменения вызывающих сторон, что создаёт высокий риск регрессии.

  2. Безопасность. Прямых уязвимостей нет. С другой стороны, удаление f-строки с skill_info['name'] убирает потенциальное раскрытие внутренних имён скиллов в пользовательском/LLM-контексте, что можно считать незначительным улучшением с точки зрения инфодисклошера.

  3. Качество кода. Изменение слишком «тихое» для такого сдвига в контракте. Не обновлены (или не показаны) type hints, docstring и логика обработки на стороне коллеров. Если функция именуется build_*_message, возврат None должен быть явно задокументирован как штатное поведение при ошибке.

  4. Тесты и краевые случаи. В диффе нет тестов. Обязательно нужны:

    • проверка, что при неудаче возвращается None;
    • аудит всех вызывающих функций на корректную обработку None;
    • тесты на поведение коллеров при получении None (пропуск сообщения, fallback и т.д.).

Вердикт: NEEDS_WORK — требуется убедиться, что все коллеры обрабатывают None, и добавить/обновить тесты.

Tests Added

  • test_returns_none_when_skill_load_fails in tests/agent/test_skill_commands.py — verifies that build_skill_invocation_message() returns None when a registered skill exists in the command cache but _load_skill_payload() fails.

@teknium1 teknium1 mentioned this pull request May 17, 2026
Merged
@teknium1

teknium1 commented May 17, 2026

Copy link
Copy Markdown
Contributor

Merged via #27290 with your authorship preserved via rebase-merge. Commit 782d74373 is on main. Thanks!

@teknium1 teknium1 closed this May 17, 2026
@teknium1 teknium1 mentioned this pull request May 17, 2026
Merged
teknium1 added a commit that referenced this pull request May 17, 2026
@teknium1
chore(release): AUTHOR_MAP entries for batch salvage group 4 contribu…
7322816 
…tors

Adds release-note attribution mappings for 9 contributors from group 4:
- @EloquentBrush0x (PR #26657)
- @subtract0 (PR #25658)
- @zwolniony (PR #26961)
- @that-ambuj (PR #26582)
- @zccyman (PR #25294)
- @lidge-jun (PR #26814)
- @phoenixshen (PR #26768)
- @AhmetArif0 (PR #26635)
- (francip already mapped from prior PR #26134 attribution)

#27147 dropped from this batch — already landed on main as 4b17c24.
@teknium1 teknium1 mentioned this pull request May 17, 2026
Merged
gweeteve pushed a commit to gweeteve/hermes-agent that referenced this pull request Jun 2, 2026
@teknium1
chore(release): AUTHOR_MAP entries for batch salvage group 4 contribu…
4051448 
…tors

Adds release-note attribution mappings for 9 contributors from group 4:
- @EloquentBrush0x (PR NousResearch#26657)
- @subtract0 (PR NousResearch#25658)
- @zwolniony (PR NousResearch#26961)
- @that-ambuj (PR NousResearch#26582)
- @zccyman (PR NousResearch#25294)
- @lidge-jun (PR NousResearch#26814)
- @phoenixshen (PR NousResearch#26768)
- @AhmetArif0 (PR NousResearch#26635)
- (francip already mapped from prior PR NousResearch#26134 attribution)

NousResearch#27147 dropped from this batch — already landed on main as 4b17c24.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

comp/agent Core agent loop, run_agent.py, prompt builder P2 Medium — degraded but workaround exists tool/skills Skills system (list, view, manage) type/bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@0xchainer @cardtest15-coder @alt-glitch @teknium1