Pull request overview

This PR adds a skill discovery + installation subsystem backed by pluggable “skill registries” (initially Clawhub) and a query SearchCache, exposing it via new agent tools (find_skills, install_skill) and an extended CLI flow (picoclaw skills install --registry ...).

Changes:

• Introduces a registry abstraction (SkillRegistry) and manager (RegistryManager) with concurrent multi-registry search.
• Adds Clawhub registry integration (search/meta/download+extract) and an LRU+TTL search cache using trigram/Jaccard similarity.
• Adds agent tools and CLI plumbing/config to discover/install skills from registries.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 10 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 9 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

PR Review: Feature: Implement Skill Discovery — Clawhub Integration & Caching

Verdict: REQUEST_CHANGES ❌

@harshbansal7 — This is a well-architected feature addition with clean abstractions (SkillRegistry interface, RegistryManager, SearchCache with trigram/Jaccard similarity). However, I found 1 critical bug, 3 high-severity issues, and several medium-severity items that must be addressed before merge. Tested locally with go vet, go build, and go test -race.

Findings

[CRITICAL] InstallSkillTool mutex is local to Execute — provides ZERO concurrency protection

File: pkg/tools/skills_install.go:68-73

func (t *InstallSkillTool) Execute(ctx context.Context, args map[string]interface{}) *ToolResult {
    slugLock := sync.Mutex{}
    slugLock.Lock()
    defer slugLock.Unlock()

The mutex is created inside Execute — a brand new mutex on every call. This means every concurrent invocation gets its own independent mutex, providing absolutely no mutual exclusion. The TOCTOU race between os.Stat(targetDir) (line 95) and DownloadAndInstall (line 115) remains completely unprotected.

Fix: Move the mutex to the struct:

type InstallSkillTool struct {
    registryMgr *skills.RegistryManager
    workspace   string
    mu          sync.Mutex  // protects concurrent installations
}

func (t *InstallSkillTool) Execute(ctx context.Context, args map[string]interface{}) *ToolResult {
    t.mu.Lock()
    defer t.mu.Unlock()

[HIGH] Two tests are failing — error message mismatch after refactor

Verified locally: go test -race ./pkg/tools/... → FAIL

• TestInstallSkillToolMissingSlug expects "slug is required" but gets "identifier is required"
• TestInstallSkillToolMissingRegistry expects "registry is required" but gets "identifier is required"

This happened because slug validation was extracted to ValidateSkillIdentifier() but the tests weren't updated. The tests must match the actual error messages.

[HIGH] doGet allows 50MB+ response body for JSON API responses

File: pkg/skills/clawhub_registry.go:293

body, err := io.ReadAll(io.LimitReader(resp.Body, int64(c.maxZipSize)+1024))

maxZipSize defaults to 50MB. This limit is applied to search and metadata API responses, which should return a few KB of JSON at most. On a device with 10-20MB of RAM (PicoClaw's target hardware), a malicious or misconfigured server returning 50MB of JSON would OOM the process.

Fix: Use a separate, reasonable limit for JSON responses:

const maxJSONResponseSize = 1 * 1024 * 1024 // 1 MB

body, err := io.ReadAll(io.LimitReader(resp.Body, maxJSONResponseSize))

[HIGH] CLI path skillsInstallFromRegistry does NOT validate slug

File: cmd/picoclaw/main.go:1317

targetDir := filepath.Join(workspace, "skills", slug)

The slug comes from os.Args[5] (line 1281) and is used to construct targetDir without any validation. If a user passes ../../etc as slug, filepath.Join resolves it, and on install failure, os.RemoveAll(targetDir) at line 1334 could delete directories outside the skills folder.

The tool-side (skills_install.go) validates via ValidateSkillIdentifier, but the CLI path bypasses this entirely.

Fix: Add validation before using the slug:

func skillsInstallFromRegistry(cfg *config.Config, registryName, slug string) {
    if err := utils.ValidateSkillIdentifier(slug); err != nil {
        fmt.Printf("\u2717 Invalid slug: %v\n", err)
        os.Exit(1)
    }
    if err := utils.ValidateSkillIdentifier(registryName); err != nil {
        fmt.Printf("\u2717 Invalid registry name: %v\n", err)
        os.Exit(1)
    }
    // ... rest of function

[MEDIUM] Malware check happens AFTER ZIP is downloaded and extracted

File: pkg/tools/skills_install.go:115-125

The flow is:

1. DownloadAndInstall → fetches metadata, downloads ZIP, extracts to disk
2. Then checks result.IsMalwareBlocked
3. If blocked, os.RemoveAll(targetDir)

By the time the malware check runs, the ZIP is already fully extracted to disk. If the ZIP itself is weaponized (e.g., a zip bomb or files with exploit payloads), the damage is done before cleanup.

Fix: Check IsMalwareBlocked inside DownloadAndInstall between steps 1 (metadata) and 3 (download):

if meta != nil && meta.IsMalwareBlocked {
    return &InstallResult{IsMalwareBlocked: true}, nil
}

[MEDIUM] No decompression bomb (zip bomb) protection

File: pkg/utils/zip.go

The ZIP extraction has no limit on total extracted size or individual file sizes. A malicious ZIP with a small compressed size could expand to gigabytes, exhausting disk space on PicoClaw's target hardware (which may have very limited storage).

Fix: Add a maxTotalSize parameter and track bytes written:

const maxExtractedSize = 100 * 1024 * 1024 // 100 MB total
var totalWritten int64

// Inside extractSingleFile:
written, err := io.Copy(outFile, io.LimitReader(rc, maxExtractedSize-totalWritten))
totalWritten += written
if totalWritten >= maxExtractedSize {
    return fmt.Errorf("extracted size exceeds limit")
}

[MEDIUM] ZIP path traversal check vulnerable to directory prefix collision

File: pkg/utils/zip.go:45

if !strings.HasPrefix(filepath.Clean(destPath), filepath.Clean(targetDir)) {

If targetDir is /tmp/skill and a zip entry resolves to /tmp/skill_evil/file, the HasPrefix check passes because /tmp/skill_evil/file starts with /tmp/skill. The fix is to add a separator boundary:

cleanTarget := filepath.Clean(targetDir) + string(filepath.Separator)
cleanDest := filepath.Clean(destPath)
if cleanDest != filepath.Clean(targetDir) && !strings.HasPrefix(cleanDest, cleanTarget) {

Note: In the current codebase, slug validation prevents crafting such a targetDir, so this is defense-in-depth for the utility function.

[MEDIUM] Struct type conversion ClawHubConfig(cfg.Tools.Skills.Registries.ClawHub) is fragile

Files: pkg/agent/loop.go:111, cmd/picoclaw/main.go:1307

ClawHub: skills.ClawHubConfig(cfg.Tools.Skills.Registries.ClawHub),

This relies on config.ClawHubRegistryConfig and skills.ClawHubConfig having identical field types and names in the same order. If any field is added, reordered, or renamed in one struct but not the other, this will either fail to compile (best case) or silently map wrong values (worst case). This is done in two separate places.

Fix: Use explicit field mapping or create a conversion function in one place:

func ConfigToClawHubConfig(c config.ClawHubRegistryConfig) skills.ClawHubConfig {
    return skills.ClawHubConfig{
        Enabled:      c.Enabled,
        BaseURL:      c.BaseURL,
        // ... explicit mapping
    }
}

[MEDIUM] RegistryManager creation logic duplicated between agent loop and CLI

Files: pkg/agent/loop.go:109-112 and cmd/picoclaw/main.go:1305-1308

Identical NewRegistryManagerFromConfig calls with the same struct conversion. If config shape changes, both places must be updated.

Fix: Extract to a helper in the skills package.

[LOW] writeOriginMeta error is logged but then _ = err is a dead assignment

File: pkg/tools/skills_install.go:139

_ = err

After logging the error, _ = err is a no-op. Remove this dead line.

[POSITIVE] Architecture & abstractions

The SkillRegistry interface design is clean and extensible. The RegistryManager with concurrent fan-out search using semaphores is well-implemented. The SearchCache with trigram/Jaccard similarity is a creative solution for fuzzy query matching. The DownloadToFile utility with streaming to temp file addresses the memory constraint well.

[POSITIVE] Security measures in place

• Slug validation via ValidateSkillIdentifier
• ZIP symlink rejection
• ZIP path traversal checks (with the prefix boundary issue noted above)
• maxZipSize download limit
• Malware/suspicious flags from registry metadata

Test Results (Local)

Summary

The CRITICAL mutex bug and the 3 HIGH issues must be fixed before merge. The MEDIUM items are strongly recommended, especially the malware check ordering and zip bomb protection — these are security-relevant for a tool that downloads and executes code from the internet.

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 8 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 8 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 9 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.