From c088ad408fed87c2c3e616d2351b12a82c6fc248 Mon Sep 17 00:00:00 2001 From: harshbansal7 Date: Wed, 18 Feb 2026 02:50:16 +0530 Subject: [PATCH] Comments addressed --- cmd/picoclaw/main.go | 11 +-- pkg/agent/loop.go | 13 +-- pkg/config/config.go | 5 ++ pkg/skills/clawhub_registry.go | 123 +++++--------------------- pkg/skills/clawhub_registry_test.go | 13 ++- pkg/skills/registry.go | 6 +- pkg/skills/registry_test.go | 13 +-- pkg/skills/search_cache.go | 63 ++++++++----- pkg/skills/search_cache_repro_test.go | 37 ++++++++ pkg/skills/search_cache_test.go | 36 +++++++- pkg/tools/skills_install.go | 42 +++++---- pkg/tools/skills_search.go | 3 +- pkg/utils/download.go | 93 +++++++++++++++++++ pkg/utils/skills.go | 18 ++++ pkg/utils/string.go | 9 ++ pkg/utils/zip.go | 101 +++++++++++++++++++++ 16 files changed, 411 insertions(+), 175 deletions(-) create mode 100644 pkg/skills/search_cache_repro_test.go create mode 100644 pkg/utils/download.go create mode 100644 pkg/utils/skills.go create mode 100644 pkg/utils/zip.go diff --git a/cmd/picoclaw/main.go b/cmd/picoclaw/main.go index 03a4cb874..3bba58ba0 100644 --- a/cmd/picoclaw/main.go +++ b/cmd/picoclaw/main.go @@ -1304,16 +1304,7 @@ func skillsInstallFromRegistry(cfg *config.Config, registryName, slug string) { registryMgr := skills.NewRegistryManagerFromConfig(skills.RegistryConfig{ MaxConcurrentSearches: cfg.Tools.Skills.MaxConcurrentSearches, - ClawHub: skills.ClawHubConfig{ - Enabled: cfg.Tools.Skills.Registries.ClawHub.Enabled, - BaseURL: cfg.Tools.Skills.Registries.ClawHub.BaseURL, - AuthToken: cfg.Tools.Skills.Registries.ClawHub.AuthToken, - SearchPath: cfg.Tools.Skills.Registries.ClawHub.SearchPath, - SkillsPath: cfg.Tools.Skills.Registries.ClawHub.SkillsPath, - DownloadPath: cfg.Tools.Skills.Registries.ClawHub.DownloadPath, - Timeout: cfg.Tools.Skills.Registries.ClawHub.Timeout, - MaxZipSize: cfg.Tools.Skills.Registries.ClawHub.MaxZipSize, - }, + ClawHub: skills.ClawHubConfig(cfg.Tools.Skills.Registries.ClawHub), }) registry := registryMgr.GetRegistry(registryName) diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index 70b6f21dd..3dd5c4d23 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -108,18 +108,9 @@ func createToolRegistry(workspace string, restrict bool, cfg *config.Config, msg // Skill discovery and installation tools registryMgr := skills.NewRegistryManagerFromConfig(skills.RegistryConfig{ MaxConcurrentSearches: cfg.Tools.Skills.MaxConcurrentSearches, - ClawHub: skills.ClawHubConfig{ - Enabled: cfg.Tools.Skills.Registries.ClawHub.Enabled, - BaseURL: cfg.Tools.Skills.Registries.ClawHub.BaseURL, - AuthToken: cfg.Tools.Skills.Registries.ClawHub.AuthToken, - SearchPath: cfg.Tools.Skills.Registries.ClawHub.SearchPath, - SkillsPath: cfg.Tools.Skills.Registries.ClawHub.SkillsPath, - DownloadPath: cfg.Tools.Skills.Registries.ClawHub.DownloadPath, - Timeout: cfg.Tools.Skills.Registries.ClawHub.Timeout, - MaxZipSize: cfg.Tools.Skills.Registries.ClawHub.MaxZipSize, - }, + ClawHub: skills.ClawHubConfig(cfg.Tools.Skills.Registries.ClawHub), }) - searchCache := skills.NewSearchCache(cfg.Tools.Skills.SearchCache.MaxSize, time.Duration(cfg.Tools.Skills.SearchCache.TTLSeconds) * time.Second) + searchCache := skills.NewSearchCache(cfg.Tools.Skills.SearchCache.MaxSize, time.Duration(cfg.Tools.Skills.SearchCache.TTLSeconds)*time.Second) registry.Register(tools.NewFindSkillsTool(registryMgr, searchCache)) registry.Register(tools.NewInstallSkillTool(registryMgr, workspace)) diff --git a/pkg/config/config.go b/pkg/config/config.go index 148efd71f..0f7d45137 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -376,6 +376,11 @@ func DefaultConfig() *Config { BaseURL: "https://clawhub.ai", }, }, + MaxConcurrentSearches: 2, + SearchCache: SearchCacheConfig{ + MaxSize: 50, + TTLSeconds: 300, + }, }, }, Heartbeat: HeartbeatConfig{ diff --git a/pkg/skills/clawhub_registry.go b/pkg/skills/clawhub_registry.go index be9008e82..07867e709 100644 --- a/pkg/skills/clawhub_registry.go +++ b/pkg/skills/clawhub_registry.go @@ -1,8 +1,6 @@ package skills import ( - "archive/zip" - "bytes" "context" "encoding/json" "fmt" @@ -10,9 +8,9 @@ import ( "net/http" "net/url" "os" - "path/filepath" - "strings" "time" + + "github.com/sipeed/picoclaw/pkg/utils" ) const ( @@ -121,17 +119,17 @@ func (c *ClawHubRegistry) Search(ctx context.Context, query string, limit int) ( results := make([]SearchResult, 0, len(resp.Results)) for _, r := range resp.Results { - slug := derefStr(r.Slug, "") + slug := utils.DerefStr(r.Slug, "") if slug == "" { continue } - summary := derefStr(r.Summary, "") + summary := utils.DerefStr(r.Summary, "") if summary == "" { continue } - displayName := derefStr(r.DisplayName, "") + displayName := utils.DerefStr(r.DisplayName, "") if displayName == "" { displayName = slug } @@ -141,7 +139,7 @@ func (c *ClawHubRegistry) Search(ctx context.Context, query string, limit int) ( Slug: slug, DisplayName: displayName, Summary: summary, - Version: derefStr(r.Version, ""), + Version: utils.DerefStr(r.Version, ""), RegistryName: c.Name(), }) } @@ -169,8 +167,8 @@ type clawhubModerationInfo struct { } func (c *ClawHubRegistry) GetSkillMeta(ctx context.Context, slug string) (*SkillMeta, error) { - if !isSafeSlug(slug) { - return nil, fmt.Errorf("invalid slug: %q", slug) + if err := utils.ValidateSkillIdentifier(slug); err != nil { + return nil, fmt.Errorf("invalid slug %q: error: %s", slug, err.Error()) } u := c.baseURL + c.skillsPath + "/" + url.PathEscape(slug) @@ -209,8 +207,8 @@ func (c *ClawHubRegistry) GetSkillMeta(ctx context.Context, slug string) (*Skill // downloads the skill ZIP, and extracts it to targetDir. // Returns an InstallResult for the caller to use for moderation decisions. func (c *ClawHubRegistry) DownloadAndInstall(ctx context.Context, slug, version, targetDir string) (*InstallResult, error) { - if !isSafeSlug(slug) { - return nil, fmt.Errorf("invalid slug: %q", slug) + if err := utils.ValidateSkillIdentifier(slug); err != nil { + return nil, fmt.Errorf("invalid slug %q: error: %s", slug, err.Error()) } // Step 1: Fetch metadata (with fallback). @@ -237,7 +235,7 @@ func (c *ClawHubRegistry) DownloadAndInstall(ctx context.Context, slug, version, } result.Version = installVersion - // Step 3: Download ZIP. + // Step 3: Download ZIP to temp file (streams in ~32KB chunks). u, err := url.Parse(c.baseURL + c.downloadPath) if err != nil { return nil, fmt.Errorf("invalid base URL: %w", err) @@ -250,17 +248,22 @@ func (c *ClawHubRegistry) DownloadAndInstall(ctx context.Context, slug, version, } u.RawQuery = q.Encode() - zipData, err := c.doGet(ctx, u.String()) + req, err := http.NewRequestWithContext(ctx, "GET", u.String(), nil) + if err != nil { + return nil, fmt.Errorf("failed to create request: %w", err) + } + if c.authToken != "" { + req.Header.Set("Authorization", "Bearer "+c.authToken) + } + + tmpPath, err := utils.DownloadToFile(ctx, c.client, req, int64(c.maxZipSize)) if err != nil { return nil, fmt.Errorf("download failed: %w", err) } + defer os.Remove(tmpPath) - if len(zipData) > c.maxZipSize { - return nil, fmt.Errorf("ZIP too large: %d bytes (max %d)", len(zipData), c.maxZipSize) - } - - // Step 4: Extract. - if err := extractZip(zipData, targetDir); err != nil { + // Step 4: Extract from file on disk. + if err := utils.ExtractZipFile(tmpPath, targetDir); err != nil { return nil, err } @@ -298,83 +301,3 @@ func (c *ClawHubRegistry) doGet(ctx context.Context, urlStr string) ([]byte, err return body, nil } - -// --- ZIP extraction --- - -func extractZip(data []byte, targetDir string) error { - reader, err := zip.NewReader(bytes.NewReader(data), int64(len(data))) - if err != nil { - return fmt.Errorf("invalid ZIP: %w", err) - } - - if err := os.MkdirAll(targetDir, 0755); err != nil { - return fmt.Errorf("failed to create target dir: %w", err) - } - - for _, f := range reader.File { - // Path traversal protection. - cleanName := filepath.Clean(f.Name) - if strings.HasPrefix(cleanName, "..") || filepath.IsAbs(cleanName) { - return fmt.Errorf("zip entry has unsafe path: %q", f.Name) - } - - destPath := filepath.Join(targetDir, cleanName) - - // Double-check the resolved path is within target. - if !strings.HasPrefix(filepath.Clean(destPath), filepath.Clean(targetDir)) { - return fmt.Errorf("zip entry escapes target dir: %q", f.Name) - } - - if f.FileInfo().IsDir() { - if err := os.MkdirAll(destPath, 0755); err != nil { - return err - } - continue - } - - // Ensure parent directory exists. - if err := os.MkdirAll(filepath.Dir(destPath), 0755); err != nil { - return err - } - - rc, err := f.Open() - if err != nil { - return fmt.Errorf("failed to open zip entry %q: %w", f.Name, err) - } - - outFile, err := os.Create(destPath) - if err != nil { - rc.Close() - return fmt.Errorf("failed to create file %q: %w", destPath, err) - } - - _, err = io.Copy(outFile, rc) - rc.Close() - outFile.Close() - if err != nil { - return fmt.Errorf("failed to extract %q: %w", f.Name, err) - } - } - - return nil -} - -// --- Utilities --- - -func isSafeSlug(slug string) bool { - slug = strings.TrimSpace(slug) - if slug == "" { - return false - } - if strings.ContainsAny(slug, "/\\") || strings.Contains(slug, "..") { - return false - } - return true -} - -func derefStr(s *string, fallback string) string { - if s == nil { - return fallback - } - return *s -} diff --git a/pkg/skills/clawhub_registry_test.go b/pkg/skills/clawhub_registry_test.go index 32a5cf70c..d12e19504 100644 --- a/pkg/skills/clawhub_registry_test.go +++ b/pkg/skills/clawhub_registry_test.go @@ -11,6 +11,7 @@ import ( "path/filepath" "testing" + "github.com/sipeed/picoclaw/pkg/utils" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -159,8 +160,12 @@ func TestExtractZipPathTraversal(t *testing.T) { zw.Close() + // Write to temp file for extractZipFile. + tmpZip := filepath.Join(t.TempDir(), "bad.zip") + require.NoError(t, os.WriteFile(tmpZip, buf.Bytes(), 0644)) + tmpDir := t.TempDir() - err = extractZip(buf.Bytes(), tmpDir) + err = utils.ExtractZipFile(tmpZip, tmpDir) assert.Error(t, err) assert.Contains(t, err.Error(), "unsafe path") } @@ -172,10 +177,14 @@ func TestExtractZipWithSubdirectories(t *testing.T) { "examples/demo.yaml": "key: value", }) + // Write to temp file for extractZipFile. + tmpZip := filepath.Join(t.TempDir(), "test.zip") + require.NoError(t, os.WriteFile(tmpZip, zipBuf, 0644)) + tmpDir := t.TempDir() targetDir := filepath.Join(tmpDir, "my-skill") - err := extractZip(zipBuf, targetDir) + err := utils.ExtractZipFile(tmpZip, targetDir) require.NoError(t, err) // Verify nested file. diff --git a/pkg/skills/registry.go b/pkg/skills/registry.go index 9a04e87cf..cb4e6f5a1 100644 --- a/pkg/skills/registry.go +++ b/pkg/skills/registry.go @@ -160,7 +160,7 @@ func (rm *RegistryManager) SearchAll(ctx context.Context, query string, limit in return } - searchCtx, cancel := context.WithTimeout(ctx, 15*time.Second) + searchCtx, cancel := context.WithTimeout(ctx, 1*time.Minute) defer cancel() results, err := r.Search(searchCtx, query, limit) @@ -182,16 +182,18 @@ func (rm *RegistryManager) SearchAll(ctx context.Context, query string, limit in var merged []SearchResult var lastErr error + var anyRegistrySucceeded bool for rr := range resultsCh { if rr.err != nil { lastErr = rr.err continue } + anyRegistrySucceeded = true merged = append(merged, rr.results...) } // If all registries failed, return the last error. - if len(merged) == 0 && lastErr != nil { + if !anyRegistrySucceeded && lastErr != nil { return nil, fmt.Errorf("all registries failed: %w", lastErr) } diff --git a/pkg/skills/registry_test.go b/pkg/skills/registry_test.go index 21b585fdc..daecd5a59 100644 --- a/pkg/skills/registry_test.go +++ b/pkg/skills/registry_test.go @@ -6,6 +6,7 @@ import ( "testing" "time" + "github.com/sipeed/picoclaw/pkg/utils" "github.com/stretchr/testify/assert" ) @@ -169,10 +170,10 @@ func TestSortByScoreDesc(t *testing.T) { } func TestIsSafeSlug(t *testing.T) { - assert.True(t, isSafeSlug("github")) - assert.True(t, isSafeSlug("docker-compose")) - assert.False(t, isSafeSlug("")) - assert.False(t, isSafeSlug("../etc/passwd")) - assert.False(t, isSafeSlug("path/traversal")) - assert.False(t, isSafeSlug("path\\traversal")) + assert.NoError(t, utils.ValidateSkillIdentifier("github")) + assert.NoError(t, utils.ValidateSkillIdentifier("docker-compose")) + assert.Error(t, utils.ValidateSkillIdentifier("")) + assert.Error(t, utils.ValidateSkillIdentifier("../etc/passwd")) + assert.Error(t, utils.ValidateSkillIdentifier("path/traversal")) + assert.Error(t, utils.ValidateSkillIdentifier("path\\traversal")) } diff --git a/pkg/skills/search_cache.go b/pkg/skills/search_cache.go index b6495cadc..a1584e747 100644 --- a/pkg/skills/search_cache.go +++ b/pkg/skills/search_cache.go @@ -1,6 +1,7 @@ package skills import ( + "sort" "strings" "sync" "time" @@ -19,7 +20,7 @@ type SearchCache struct { type cacheEntry struct { query string - trigrams map[string]struct{} + trigrams []uint32 results []SearchResult createdAt time.Time } @@ -53,12 +54,13 @@ func (sc *SearchCache) Get(query string) ([]SearchResult, bool) { return nil, false } - sc.mu.RLock() - defer sc.mu.RUnlock() + sc.mu.Lock() + defer sc.mu.Unlock() // Exact match first. if entry, ok := sc.entries[normalized]; ok { if time.Since(entry.createdAt) < sc.ttl { + sc.moveToEndLocked(normalized) return copyResults(entry.results), true } } @@ -80,6 +82,7 @@ func (sc *SearchCache) Get(query string) ([]SearchResult, bool) { } if bestSim >= similarityThreshold && bestEntry != nil { + sc.moveToEndLocked(bestEntry.query) return copyResults(bestEntry.results), true } @@ -166,39 +169,53 @@ func normalizeQuery(q string) string { return strings.ToLower(strings.TrimSpace(q)) } -// buildTrigrams generates character trigrams from a string. +// buildTrigrams generates hash of trigrams from a string. // Example: "hello" → {"hel", "ell", "llo"} -func buildTrigrams(s string) map[string]struct{} { - trigrams := make(map[string]struct{}) - runes := []rune(s) - for i := 0; i <= len(runes)-3; i++ { - tri := string(runes[i : i+3]) - trigrams[tri] = struct{}{} +// "hel" -> 0x0068656c -> 4 bytes; compared to 16 byptes of a string +func buildTrigrams(s string) []uint32 { + if len(s) < 3 { + return nil } - return trigrams + + trigrams := make([]uint32, 0, len(s)-2) + for i := 0; i <= len(s)-3; i++ { + trigrams = append(trigrams, uint32(s[i])<<16|uint32(s[i+1])<<8|uint32(s[i+2])) + } + + // Sort and Deduplication + sort.Slice(trigrams, func(i, j int) bool { return trigrams[i] < trigrams[j] }) + n := 1 + for i := 1; i < len(trigrams); i++ { + if trigrams[i] != trigrams[i-1] { + trigrams[n] = trigrams[i] + n++ + } + } + + return trigrams[:n] } // jaccardSimilarity computes |A ∩ B| / |A ∪ B|. -func jaccardSimilarity(a, b map[string]struct{}) float64 { +func jaccardSimilarity(a, b []uint32) float64 { if len(a) == 0 && len(b) == 0 { - return 1.0 + return 1 } - if len(a) == 0 || len(b) == 0 { - return 0.0 - } - + i, j := 0, 0 intersection := 0 - for k := range a { - if _, ok := b[k]; ok { + + for i < len(a) && j < len(b) { + if a[i] == b[j] { intersection++ + i++ + j++ + } else if a[i] < b[j] { + i++ + } else { + j++ } } union := len(a) + len(b) - intersection - if union == 0 { - return 0.0 - } - return float64(intersection) / float64(union) } diff --git a/pkg/skills/search_cache_repro_test.go b/pkg/skills/search_cache_repro_test.go new file mode 100644 index 000000000..c076c07e1 --- /dev/null +++ b/pkg/skills/search_cache_repro_test.go @@ -0,0 +1,37 @@ +package skills + +import ( + "testing" + "time" +) + +func TestSearchCache_LRU_Behavior(t *testing.T) { + // Capacity 3 + // Capacity 3 + cache := NewSearchCache(3, time.Hour) + + // Fill cache: query-A, query-B, query-C + // Use longer strings to ensure trigrams are generated and avoid false positive similarity + cache.Put("query-A", []SearchResult{{Slug: "A"}}) + cache.Put("query-B", []SearchResult{{Slug: "B"}}) + cache.Put("query-C", []SearchResult{{Slug: "C"}}) + + // Access query-A (should make it most recently used) + // Current behavior: query-A remains at front (oldest) if Get doesn't update order + if _, found := cache.Get("query-A"); !found { + t.Fatal("query-A should be in cache") + } + + // Add query-D. Should evict query-A if FIFO (current bug), or query-B if LRU (desired). + cache.Put("query-D", []SearchResult{{Slug: "D"}}) + + // Check if query-A is still there + if _, found := cache.Get("query-A"); !found { + t.Fatalf("query-A was evicted! valid LRU should have kept query-A and evicted query-B.") + } + + // Check if query-B is evicted (if A was kept, B should be gone) + if _, found := cache.Get("query-B"); found { + t.Fatal("query-B should have been evicted") + } +} diff --git a/pkg/skills/search_cache_test.go b/pkg/skills/search_cache_test.go index df61d3fd9..816bdfb93 100644 --- a/pkg/skills/search_cache_test.go +++ b/pkg/skills/search_cache_test.go @@ -120,9 +120,9 @@ func TestSearchCacheResultsCopied(t *testing.T) { func TestBuildTrigrams(t *testing.T) { trigrams := buildTrigrams("hello") - assert.Contains(t, trigrams, "hel") - assert.Contains(t, trigrams, "ell") - assert.Contains(t, trigrams, "llo") + assert.Contains(t, trigrams, uint32('h')<<16|uint32('e')<<8|uint32('l')) + assert.Contains(t, trigrams, uint32('e')<<16|uint32('l')<<8|uint32('l')) + assert.Contains(t, trigrams, uint32('l')<<16|uint32('l')<<8|uint32('o')) assert.Len(t, trigrams, 3) } @@ -168,5 +168,33 @@ func TestSearchCacheConcurrency(t *testing.T) { }() <-done - <-done +} + +func TestSearchCacheLRUUpdateOnGet(t *testing.T) { + // Capacity 3 + cache := NewSearchCache(3, time.Hour) + + // Fill cache: query-A, query-B, query-C + // Use longer strings to ensure trigrams are generated and avoid false positive similarity + cache.Put("query-A", []SearchResult{{Slug: "A"}}) + cache.Put("query-B", []SearchResult{{Slug: "B"}}) + cache.Put("query-C", []SearchResult{{Slug: "C"}}) + + // Access query-A (should make it most recently used) + if _, found := cache.Get("query-A"); !found { + t.Fatal("query-A should be in cache") + } + + // Add query-D. Should evict query-B (LRU) instead of query-A (which was refreshed) + cache.Put("query-D", []SearchResult{{Slug: "D"}}) + + // Check if query-A is still there + if _, found := cache.Get("query-A"); !found { + t.Fatalf("query-A was evicted! valid LRU should have kept query-A and evicted query-B.") + } + + // Check if query-B is evicted + if _, found := cache.Get("query-B"); found { + t.Fatal("query-B should have been evicted") + } } diff --git a/pkg/tools/skills_install.go b/pkg/tools/skills_install.go index 78c6579f3..be52f4ca5 100644 --- a/pkg/tools/skills_install.go +++ b/pkg/tools/skills_install.go @@ -6,10 +6,12 @@ import ( "fmt" "os" "path/filepath" - "strings" + "sync" "time" + "github.com/sipeed/picoclaw/pkg/logger" "github.com/sipeed/picoclaw/pkg/skills" + "github.com/sipeed/picoclaw/pkg/utils" ) // InstallSkillTool allows the LLM agent to install skills from registries. @@ -64,25 +66,25 @@ func (t *InstallSkillTool) Parameters() map[string]interface{} { } func (t *InstallSkillTool) Execute(ctx context.Context, args map[string]interface{}) *ToolResult { - slug, ok := args["slug"].(string) - if !ok || strings.TrimSpace(slug) == "" { - return ErrorResult("slug is required and must be a non-empty string") + // Install lock to prevent concurrent directory operations. + // Ideally this should be done at a `slug` level, currently, its at a `workspace` level. + slugLock := sync.Mutex{} + slugLock.Lock() + defer slugLock.Unlock() + + // Validate slug + slug, _ := args["slug"].(string) + if err := utils.ValidateSkillIdentifier(slug); err != nil { + return ErrorResult(fmt.Sprintf("invalid slug %q: error: %s", slug, err.Error())) } - slug = strings.TrimSpace(slug) - - // Validate slug safety. - if strings.ContainsAny(slug, "/\\") || strings.Contains(slug, "..") { - return ErrorResult(fmt.Sprintf("invalid slug: %q (must not contain path separators or '..')", slug)) + // Validate registry + registryName, _ := args["registry"].(string) + if err := utils.ValidateSkillIdentifier(registryName); err != nil { + return ErrorResult(fmt.Sprintf("invalid registry %q: error: %s", registryName, err.Error())) } version, _ := args["version"].(string) - registryName, ok := args["registry"].(string) - if !ok || strings.TrimSpace(registryName) == "" { - return ErrorResult("registry is required") - } - registryName = strings.TrimSpace(registryName) - force, _ := args["force"].(bool) // Check if already installed. @@ -125,7 +127,15 @@ func (t *InstallSkillTool) Execute(ctx context.Context, args map[string]interfac // Write origin metadata. if err := writeOriginMeta(targetDir, registry.Name(), slug, result.Version); err != nil { - // Non-fatal: skill is installed, just origin tracking failed. + logger.ErrorCF("tool", "Failed to write origin metadata", + map[string]interface{}{ + "tool": "install_skill", + "error": err.Error(), + "target": targetDir, + "registry": registry.Name(), + "slug": slug, + "version": result.Version, + }) _ = err } diff --git a/pkg/tools/skills_search.go b/pkg/tools/skills_search.go index 986372f48..b12949ec2 100644 --- a/pkg/tools/skills_search.go +++ b/pkg/tools/skills_search.go @@ -53,7 +53,8 @@ func (t *FindSkillsTool) Parameters() map[string]interface{} { func (t *FindSkillsTool) Execute(ctx context.Context, args map[string]interface{}) *ToolResult { query, ok := args["query"].(string) - if !ok || strings.TrimSpace(query) == "" { + query = strings.ToLower(strings.TrimSpace(query)) + if !ok || query == "" { return ErrorResult("query is required and must be a non-empty string") } diff --git a/pkg/utils/download.go b/pkg/utils/download.go new file mode 100644 index 000000000..9fa7fbfa7 --- /dev/null +++ b/pkg/utils/download.go @@ -0,0 +1,93 @@ +package utils + +import ( + "context" + "fmt" + "io" + "net/http" + "os" + + "github.com/sipeed/picoclaw/pkg/logger" +) + +// DownloadToFile streams an HTTP response body to a temporary file in small +// chunks (~32KB), keeping peak memory usage constant regardless of file size. +// +// Parameters: +// - ctx: context for cancellation/timeout +// - client: HTTP client to use (caller controls timeouts, transport, etc.) +// - req: fully prepared *http.Request (method, URL, headers, etc.) +// - maxBytes: maximum bytes to download; 0 means no limit +// +// Returns the path to the temporary file. The caller is responsible for +// removing it when done (defer os.Remove(path)). +// +// On any error the temp file is cleaned up automatically. +func DownloadToFile(ctx context.Context, client *http.Client, req *http.Request, maxBytes int64) (string, error) { + // Attach context. + req = req.WithContext(ctx) + + logger.DebugCF("download", "Starting download", map[string]interface{}{ + "url": req.URL.String(), + "max_bytes": maxBytes, + }) + + resp, err := client.Do(req) + if err != nil { + return "", fmt.Errorf("request failed: %w", err) + } + defer resp.Body.Close() + + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + // Read a small amount for the error message. + errBody := make([]byte, 512) + n, _ := io.ReadFull(resp.Body, errBody) + return "", fmt.Errorf("HTTP %d: %s", resp.StatusCode, string(errBody[:n])) + } + + // Create temp file. + tmpFile, err := os.CreateTemp("", "picoclaw-dl-*") + if err != nil { + return "", fmt.Errorf("failed to create temp file: %w", err) + } + tmpPath := tmpFile.Name() + + logger.DebugCF("download", "Streaming to temp file", map[string]interface{}{ + "path": tmpPath, + }) + + // Cleanup helper — removes the temp file on any error. + cleanup := func() { + _ = tmpFile.Close() + _ = os.Remove(tmpPath) + } + + // Optionally limit the download size. + var src io.Reader = resp.Body + if maxBytes > 0 { + src = io.LimitReader(resp.Body, maxBytes+1) // +1 to detect overflow + } + + written, err := io.Copy(tmpFile, src) + if err != nil { + cleanup() + return "", fmt.Errorf("download write failed: %w", err) + } + + if maxBytes > 0 && written > maxBytes { + cleanup() + return "", fmt.Errorf("download too large: %d bytes (max %d)", written, maxBytes) + } + + if err := tmpFile.Close(); err != nil { + _ = os.Remove(tmpPath) + return "", fmt.Errorf("failed to close temp file: %w", err) + } + + logger.DebugCF("download", "Download complete", map[string]interface{}{ + "path": tmpPath, + "bytes_written": written, + }) + + return tmpPath, nil +} diff --git a/pkg/utils/skills.go b/pkg/utils/skills.go new file mode 100644 index 000000000..f66fa4915 --- /dev/null +++ b/pkg/utils/skills.go @@ -0,0 +1,18 @@ +package utils + +import ( + "fmt" + "strings" +) + +// ValidateSkillIdentifier validates that the given skill identifier (slug or registry name) is non-empty +// and does not contain path separators ("/", "\\") or ".." for security. +func ValidateSkillIdentifier(identifier string) error { + if identifier == "" { + return fmt.Errorf("identifier is required and must be a non-empty string") + } + if strings.ContainsAny(identifier, "/\\") || strings.Contains(identifier, "..") { + return fmt.Errorf("identifier must not contain path separators or '..' to prevent directory traversal") + } + return nil +} diff --git a/pkg/utils/string.go b/pkg/utils/string.go index 0d9837cb9..7a6aa37cc 100644 --- a/pkg/utils/string.go +++ b/pkg/utils/string.go @@ -14,3 +14,12 @@ func Truncate(s string, maxLen int) string { } return string(runes[:maxLen-3]) + "..." } + +// DerefStr dereferences a pointer to a string and +// returns the value or a fallback if the pointer is nil. +func DerefStr(s *string, fallback string) string { + if s == nil { + return fallback + } + return *s +} diff --git a/pkg/utils/zip.go b/pkg/utils/zip.go new file mode 100644 index 000000000..501c8cdbc --- /dev/null +++ b/pkg/utils/zip.go @@ -0,0 +1,101 @@ +package utils + +import ( + "archive/zip" + "fmt" + "io" + "os" + "path/filepath" + "strings" + + "github.com/sipeed/picoclaw/pkg/logger" +) + +// ExtractZipFile extracts a ZIP archive from disk to targetDir. +// It reads entries one at a time from disk, keeping memory usage minimal. +// +// Security: rejects path traversal attempts and symlinks. +func ExtractZipFile(zipPath string, targetDir string) error { + reader, err := zip.OpenReader(zipPath) + if err != nil { + return fmt.Errorf("invalid ZIP: %w", err) + } + defer reader.Close() + + logger.DebugCF("zip", "Extracting ZIP", map[string]interface{}{ + "zip_path": zipPath, + "target_dir": targetDir, + "entries": len(reader.File), + }) + + if err := os.MkdirAll(targetDir, 0755); err != nil { + return fmt.Errorf("failed to create target dir: %w", err) + } + + for _, f := range reader.File { + // Path traversal protection. + cleanName := filepath.Clean(f.Name) + if strings.HasPrefix(cleanName, "..") || filepath.IsAbs(cleanName) { + return fmt.Errorf("zip entry has unsafe path: %q", f.Name) + } + + destPath := filepath.Join(targetDir, cleanName) + + // Double-check the resolved path is within target. + if !strings.HasPrefix(filepath.Clean(destPath), filepath.Clean(targetDir)) { + return fmt.Errorf("zip entry escapes target dir: %q", f.Name) + } + + mode := f.FileInfo().Mode() + + // Reject any symlink. + if mode&os.ModeSymlink != 0 { + return fmt.Errorf("zip contains symlink %q; symlinks are not allowed", f.Name) + } + + if f.FileInfo().IsDir() { + if err := os.MkdirAll(destPath, 0755); err != nil { + return err + } + continue + } + + // Ensure parent directory exists. + if err := os.MkdirAll(filepath.Dir(destPath), 0755); err != nil { + return err + } + + if err := extractSingleFile(f, destPath); err != nil { + return err + } + } + + return nil +} + +// extractSingleFile extracts one zip.File entry to destPath. +func extractSingleFile(f *zip.File, destPath string) error { + rc, err := f.Open() + if err != nil { + return fmt.Errorf("failed to open zip entry %q: %w", f.Name, err) + } + defer rc.Close() + + outFile, err := os.Create(destPath) + if err != nil { + return fmt.Errorf("failed to create file %q: %w", destPath, err) + } + + if _, err := io.Copy(outFile, rc); err != nil { + _ = outFile.Close() + _ = os.Remove(destPath) + return fmt.Errorf("failed to extract %q: %w", f.Name, err) + } + + if err := outFile.Close(); err != nil { + _ = os.Remove(destPath) + return fmt.Errorf("failed to close file %q: %w", destPath, err) + } + + return nil +}