fix(skills): return None on skill load failure instead of truthy stub (salvage #27147)#27290
Merged
Conversation
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.
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.
Contributor
🔎 Lint report:
|
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
3 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
Salvage of #27147 —
build_skill_invocation_message()returned a truthy stub string[Failed to load skill: NAME]when_load_skill_payload()failed, which bypassed every caller'sif msg:failure branch and injected the stub into the conversation as the user message.Root cause
All three call sites (
cli.py,gateway/run.py,gateway/platforms/webhook.py) checkif msg:to detect failure. The truthy stub passes that check, so the agent receives[Failed to load skill: foo]as the user's message instead of either (a) the CLI's red error print or (b) the gateway path falling through to normal message processing.Changes
agent/skill_commands.py— returnNoneon load failure (matches the existing "skill not in cache" branch directly above).tests/agent/test_skill_commands.py— regression test mocking_load_skill_payload→None.Validation
scripts/run_tests.sh tests/agent/test_skill_commands.py -q→ 40/40 pass.Original PR: #27147 — credit preserved via rebase-merge.