fix(skill): treat empty url param as absent when installing skills (#1128)

LLMs sometimes pass "" for optional parameters instead of omitting them.
Previously, passing url: "" to skill_install would match the explicit-URL
branch and attempt to fetch from an empty string, producing an invalid URL
error instead of falling back to the catalog lookup.

Fix by adding .filter(|s| !s.is_empty()) so an empty url is treated the
same as a missing field.

A unit test verifies the parameter filtering behaviour directly; the full
execute path (catalog lookup + install) requires a real catalog and database
and cannot be covered at the unit level.
This commit is contained in:
Xing Ji
2026-03-15 05:50:39 +00:00
committed by GitHub
parent f059d50331
commit 3f6d2ab6c2
+24 -1
View File
@@ -301,7 +301,11 @@ impl Tool for SkillInstallTool {
let content = if let Some(raw) = params.get("content").and_then(|v| v.as_str()) {
// Direct content provided
raw.to_string()
} else if let Some(url) = params.get("url").and_then(|v| v.as_str()) {
} else if let Some(url) = params
.get("url")
.and_then(|v| v.as_str())
.filter(|s| !s.is_empty())
{
// Fetch from explicit URL
fetch_skill_content(url).await?
} else {
@@ -1297,4 +1301,23 @@ mod tests {
);
}
}
#[test]
fn test_empty_url_param_is_treated_as_absent() {
// LLMs sometimes pass "" for optional parameters instead of omitting them.
// Before the fix, url: "" would match Some("") and attempt to fetch from an
// empty URL (failing with an invalid URL error) instead of falling through to
// the catalog lookup. The full execute path cannot be tested here without a
// real catalog and database, so this test verifies the parameter filtering
// behaviour directly.
let params = serde_json::json!({"name": "my-skill", "url": ""});
let url = params
.get("url")
.and_then(|v| v.as_str())
.filter(|s| !s.is_empty());
assert!(
url.is_none(),
"empty url string should be treated as absent"
);
}
}