fix(skills): return None instead of truthy stub when skill load fails (#17283)#17330
Closed
Bartok9 wants to merge 1 commit into
Closed
fix(skills): return None instead of truthy stub when skill load fails (#17283)#17330Bartok9 wants to merge 1 commit into
Bartok9 wants to merge 1 commit into
Conversation
…NousResearch#17283) build_skill_invocation_message() returned a formatted string '[Failed to load skill: <name>]' when _load_skill_payload() failed. This truthy string caused webhook callers to overwrite the user's prompt with the stub, silently dropping the real payload. Root cause: the webhook handler checks 'if skill_content:' and substitutes the prompt — a truthy stub satisfies that check. Fix: - Return None from build_skill_invocation_message() on load failure (consistent with the unknown-command path) - Add logger.warning() calls in _load_skill_payload() so failures are visible in gateway logs instead of silently swallowed - Add regression test Fixes NousResearch#17283
Collaborator
Contributor
Author
|
Good catch @alt-glitch — this is indeed a dup of my earlier #14752 which also covers the CLI path (#14713). Closing this one in favor of that PR which has broader scope. |
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #17283 — webhook gateway skill auto-loader returns truthy stub string instead of
Noneon load failure, silently overwriting the user's prompt.Root Cause
build_skill_invocation_message()returned"[Failed to load skill: <name>]"when_load_skill_payload()failed. The webhook handler atgateway/platforms/webhook.py:401-405checksif skill_content:and substitutes the prompt — a truthy stub passes that check, replacing the real user payload with a 42-character stub.Additionally,
_load_skill_payload()caught all exceptions with a bareexcept Exception: return Noneand logged nothing, making the failure invisible in gateway logs.Fix
Nonefrombuild_skill_invocation_message()when payload loading fails (consistent with the unknown-command path that already returnsNone)logger.warning()calls in_load_skill_payload()for both exception andsuccess=falsepaths so failures are visible in logsNoneis returned (not a truthy string) when payload load failsWhy this works
The webhook caller already handles
Nonecorrectly —if skill_content:isFalseforNone, so the original user prompt is preserved. The fix aligns the failure path with what callers expect.Testing
All 36 existing tests pass + 1 new regression test.