From 3f6d2ab6c2c7e47fe5b3c6761a491fd4cd54a5cc Mon Sep 17 00:00:00 2001
From: Xing Ji <41811005+micsama@users.noreply.github.com>
Date: Sun, 15 Mar 2026 13:50:39 +0800
Subject: [PATCH] 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.
---
 src/tools/builtin/skill_tools.rs | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/src/tools/builtin/skill_tools.rs b/src/tools/builtin/skill_tools.rs
index a7581ac4..457f1613 100644
--- a/src/tools/builtin/skill_tools.rs
+++ b/src/tools/builtin/skill_tools.rs
@@ -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"
+        );
+    }
 }