diff --git a/docs/16-skill-publishing.md b/docs/16-skill-publishing.md index 7244c1e5..bae4e67a 100644 --- a/docs/16-skill-publishing.md +++ b/docs/16-skill-publishing.md @@ -15,6 +15,8 @@ The skill publishing system bridges the gap between **skill creation** (filesyst Without `publish_skill`, skills created by agents exist only on the filesystem and are invisible to the database-backed skill management system (no search, no grants, no UI visibility). +For small text companion files created directly during conversation, `skill_manage` also accepts a `files` map on create/patch. Keep using `publish_skill` for full directories, binary assets, existing filesystem projects, or bulk skill packaging. + --- ## 2. End-to-End Flow @@ -145,6 +147,8 @@ skills-store/ `GetNextVersion(slug)` queries `MAX(version)` from the skills table (includes archived skills). +`skill_manage(files=...)` writes the same versioned directory shape when an agent creates or patches text companion files without staging a directory first. + ### 4.4 Database Upsert Uses `CreateSkillManaged()` with `ON CONFLICT(slug) DO UPDATE`: diff --git a/docs/21-agent-evolution-and-skill-management.md b/docs/21-agent-evolution-and-skill-management.md index 06ddf443..b535df5c 100644 --- a/docs/21-agent-evolution-and-skill-management.md +++ b/docs/21-agent-evolution-and-skill-management.md @@ -186,12 +186,13 @@ SHOULD NOT create skill when: - Simple tasks (< 5 tool calls) - User explicitly said "skip" or declined -Creating: skill_manage(action="create", content="---\nname: ...\n...") -Improving: skill_manage(action="patch", slug="...", find="...", replace="...") +Creating: skill_manage(action="create", content="---\nname: ...\n...", files={"references/guide.md":"..."}) +Improving: skill_manage(action="patch", slug="...", find="...", replace="...", files={"references/guide.md":"..."}) Removing: skill_manage(action="delete", slug="...") Constraints: - You can only manage skills you created (not system or other users' skills) +- Use files for small text companion files. Use publish_skill or ZIP upload for full directories and binary assets. - Quality over quantity — one excellent skill beats five mediocre ones - Ask user before creating if unsure ``` @@ -262,7 +263,7 @@ Two paths for creating skills programmatically: | Path | Interface | Use Case | |------|-----------|----------| -| `skill_manage` | Content string (SKILL.md body) | Agent creates during conversation (learning loop) | +| `skill_manage` | Content string plus optional text companion files | Agent creates during conversation (learning loop) | | `publish_skill` | Directory path | Agent creates via filesystem (see [doc 16](./16-skill-publishing.md)) | Admin management via HTTP API + WebSocket RPC. Grants system controls per-agent and per-user access. @@ -278,6 +279,8 @@ Admin management via HTTP API + WebSocket RPC. Grants system controls per-agent | `content` | string | create | Full SKILL.md including YAML frontmatter | | `find` | string | patch | Exact text to find in current SKILL.md | | `replace` | string | patch | Replacement text | +| `files` | object | no | Optional text companion files keyed by relative path, e.g. `references/guide.md` | +| `visibility` | string | patch | Optional metadata-only visibility change when no content/files change | **Operations flow:** @@ -285,24 +288,24 @@ Admin management via HTTP API + WebSocket RPC. Grants system controls per-agent flowchart LR subgraph CREATE["action = create"] direction TB - C1["Content string"] --> C2["Size ≤ 100KB?"] - C2 --> C3["Security scan"] + C1["Content +
optional files"] --> C2["Size and path
validation"] + C2 --> C3["Security scan
SKILL.md"] C3 --> C4["Parse frontmatter"] C4 --> C5["Slug validation"] C5 --> C6["System skill
conflict check"] - C6 --> C7["Write SKILL.md
to versioned dir"] + C6 --> C7["Write SKILL.md +
companions"] C7 --> C8["DB insert
(advisory lock)"] C8 --> C9["Auto-grant +
dep scan"] end subgraph PATCH["action = patch"] direction TB - P1["slug + find/replace"] --> P2["Exists?
System skill?"] + P1["slug + find/replace
and/or files"] --> P2["Exists?
System skill?"] P2 --> P3["Ownership check"] - P3 --> P4["Read current +
apply patch"] - P4 --> P5["Security scan
patched content"] + P3 --> P4["Read current +
overlay files"] + P4 --> P5["Security scan +
path validation"] P5 --> P6["New version
(advisory lock)"] - P6 --> P7["Copy companions +
DB update"] + P6 --> P7["Write companions +
DB update"] end subgraph DELETE["action = delete"] @@ -324,8 +327,8 @@ Directory-based alternative. See [16 - Skill Publishing System](./16-skill-publi | Dimension | `skill_manage` | `publish_skill` | |-----------|---------------|-----------------| -| Input | Content string | Directory path | -| Files | SKILL.md only (patch copies companions) | Entire directory (scripts, assets, etc.) | +| Input | SKILL.md content plus optional files map | Directory path | +| Files | SKILL.md plus direct text companion files; patch copies existing companions forward | Entire directory (scripts, assets, etc.) | | Dependency scan | Yes (warn only) | Yes (warn only) | | Auto-grant | Yes | Yes | | Skill creation guidance | Yes (skill_evolve prompt) | No (uses skill-creator core skill) | @@ -448,9 +451,9 @@ System skills (`is_system=true`) cannot be modified through any path. | Protection | Implementation | |------------|----------------| | Symlink detection | `filepath.WalkDir` + `d.Type()&os.ModeSymlink` check | -| Path traversal | `strings.Contains(rel, "..")` rejection | +| Path traversal | Direct `skill_manage(files=...)` payload rejects absolute paths, Windows drive paths, null bytes, `..`, `SKILL.md`, dotfiles/dotdirs, and system artifacts | | Content size limit | 100KB max for SKILL.md content | -| Companion size limit | Configurable per ZIP upload; default 20MB, clamped to 1-500MB | +| Companion size limit | Direct `skill_manage(files=...)` text files are capped at 2MB each. Existing companions copy forward with the 20MB total copy limit. ZIP upload remains configurable, default 20MB and clamped to 1-500MB | | Soft-delete | Files moved to `.trash/`, never hard-deleted | --- diff --git a/internal/http/skills_versions_test.go b/internal/http/skills_versions_test.go index f311e32b..5cf32978 100644 --- a/internal/http/skills_versions_test.go +++ b/internal/http/skills_versions_test.go @@ -3,6 +3,7 @@ package http import ( "os" "path/filepath" + "slices" "testing" ) @@ -32,3 +33,37 @@ func TestReadableSkillRootsDoesNotFallbackForCustomSkill(t *testing.T) { t.Fatalf("roots = %#v, want no custom fallback", roots) } } + +func TestSkillVersionReadbackListsAndReadsCompanionReferenceFiles(t *testing.T) { + t.Parallel() + tmp := t.TempDir() + versionDir := filepath.Join(tmp, "managed", "demo", "2") + referencePath := filepath.Join(versionDir, "references", "ship-workflow.md") + if err := os.MkdirAll(filepath.Dir(referencePath), 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(versionDir, "SKILL.md"), []byte("---\nname: Demo\n---\n"), 0644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(referencePath, []byte("# Ship\n"), 0644); err != nil { + t.Fatal(err) + } + + files := walkSkillFiles(versionDir) + if !slices.ContainsFunc(files, func(entry fileEntry) bool { + return entry.Path == filepath.Join("references", "ship-workflow.md") && !entry.IsDir && entry.Size == int64(len("# Ship\n")) + }) { + t.Fatalf("files = %#v, want references/ship-workflow.md", files) + } + + data, info, err := readSkillFile(referencePath) + if err != nil { + t.Fatalf("readSkillFile: %v", err) + } + if string(data) != "# Ship\n" { + t.Fatalf("content = %q", data) + } + if info.Size() != int64(len("# Ship\n")) { + t.Fatalf("size = %d", info.Size()) + } +} diff --git a/internal/tools/skill_manage.go b/internal/tools/skill_manage.go index aabcd6b1..405669c9 100644 --- a/internal/tools/skill_manage.go +++ b/internal/tools/skill_manage.go @@ -4,9 +4,9 @@ import ( "context" "crypto/sha256" "fmt" - "io" "log/slog" "os" + "path" "path/filepath" "strings" "time" @@ -85,8 +85,8 @@ func (t *SkillManageTool) Name() string { return "skill_manage" } func (t *SkillManageTool) Description() string { return "Create, patch, or delete your own skills from content strings. " + - "action=create: write a new skill from SKILL.md content (content string, no directory needed). " + - "action=patch: update an existing skill via find/replace (creates new immutable version). " + + "action=create: write a new skill from SKILL.md content and optional companion files. " + + "action=patch: update an existing skill via find/replace and/or companion files (creates new immutable version). " + "action=delete: archive a skill so it is no longer discoverable. " + "Security scanner rejects dangerous patterns. You can only manage skills you own." } @@ -110,7 +110,7 @@ func (t *SkillManageTool) Parameters() map[string]any { }, "find": map[string]any{ "type": "string", - "description": "Exact text to find in the current SKILL.md. Required for patch unless only 'visibility' is being updated.", + "description": "Exact text to find in the current SKILL.md. Required for content patch unless only 'files' or 'visibility' is being updated.", }, "replace": map[string]any{ "type": "string", @@ -121,6 +121,11 @@ func (t *SkillManageTool) Parameters() map[string]any { "enum": []string{skills.VisibilityPrivate, skills.VisibilityPublic}, "description": "Skill visibility. For create: defaults to 'private'. For patch: updates who can discover the skill without creating a new version.", }, + "files": map[string]any{ + "type": "object", + "additionalProperties": map[string]any{"type": "string"}, + "description": "Optional companion files keyed by relative path under the skill root. SKILL.md must use 'content' or find/replace. Unsafe paths and system artifacts are rejected.", + }, }, "required": []string{"action"}, } @@ -143,6 +148,13 @@ func (t *SkillManageTool) Execute(ctx context.Context, args map[string]any) *Res // maxSkillContentSize limits SKILL.md content to 100KB to prevent abuse. const maxSkillContentSize = 100 * 1024 +const maxManagedSkillFileSize = 2 << 20 + +type managedSkillFile struct { + Path string + Content []byte +} + // executeCreate writes a new skill from a SKILL.md content string. func (t *SkillManageTool) executeCreate(ctx context.Context, args map[string]any) *Result { content, _ := args["content"].(string) @@ -152,6 +164,13 @@ func (t *SkillManageTool) executeCreate(ctx context.Context, args map[string]any if len(content) > maxSkillContentSize { return ErrorResult(fmt.Sprintf("content too large (%d bytes, max %d)", len(content), maxSkillContentSize)) } + companionFiles, err := parseManagedSkillFiles(args["files"]) + if err != nil { + return ErrorResult(err.Error()) + } + if err := validateManagedSkillTotalSize(companionFiles); err != nil { + return ErrorResult(err.Error()) + } rawVisibility, _ := args["visibility"].(string) if err := skills.ValidateVisibility(rawVisibility); err != nil { @@ -191,6 +210,12 @@ func (t *SkillManageTool) executeCreate(ctx context.Context, args map[string]any if err := os.MkdirAll(destDir, 0755); err != nil { return ErrorResult(fmt.Sprintf("failed to create skill directory: %v", err)) } + cleanupDest := true + defer func() { + if cleanupDest { + _ = os.RemoveAll(destDir) + } + }() // Write SKILL.md contentBytes := []byte(content) @@ -198,12 +223,18 @@ func (t *SkillManageTool) executeCreate(ctx context.Context, args map[string]any if err := os.WriteFile(skillPath, contentBytes, 0644); err != nil { return ErrorResult(fmt.Sprintf("failed to write SKILL.md: %v", err)) } + if err := writeManagedSkillFiles(destDir, companionFiles); err != nil { + return ErrorResult(fmt.Sprintf("failed to write companion files: %v", err)) + } // Hash + size hasher := sha256.New() hasher.Write(contentBytes) fileHash := fmt.Sprintf("%x", hasher.Sum(nil)) - fileSize := int64(len(contentBytes)) + fileSize, err := dirSize(destDir) + if err != nil { + return ErrorResult(fmt.Sprintf("failed to calculate skill size: %v", err)) + } // DB insert — owner = actor (real sender) so skill belongs to the individual // user rather than the group principal in group chats (#915). @@ -227,6 +258,7 @@ func (t *SkillManageTool) executeCreate(ctx context.Context, args map[string]any if err != nil { return ErrorResult(fmt.Sprintf("failed to register skill: %v", err)) } + cleanupDest = false slog.Info("skill_manage: created", "id", id, "slug", slug, "version", version, "owner", ownerID) @@ -257,6 +289,9 @@ func (t *SkillManageTool) executeCreate(ctx context.Context, args map[string]any } result := fmt.Sprintf("Skill %q created.\n- Slug: %s\n- Version: %d", name, slug, version) + if len(companionFiles) > 0 { + result += fmt.Sprintf("\n- Companion files: %d", len(companionFiles)) + } if granted { result += "\n- Granted to current agent" } @@ -273,15 +308,19 @@ func (t *SkillManageTool) executePatch(ctx context.Context, args map[string]any) find, _ := args["find"].(string) replace, _ := args["replace"].(string) rawVisibility, _ := args["visibility"].(string) + companionFiles, filesErr := parseManagedSkillFiles(args["files"]) + if filesErr != nil { + return ErrorResult(filesErr.Error()) + } if slug == "" { return ErrorResult("slug is required for action=patch") } if err := skills.ValidateVisibility(rawVisibility); err != nil { return ErrorResult(err.Error()) } - // Patch requires at least one of: content edit (find) or visibility change. - if find == "" && rawVisibility == "" { - return ErrorResult("patch requires either 'find' (content edit) or 'visibility' (metadata update)") + // Patch requires at least one of: content edit (find), file payload, or visibility change. + if find == "" && len(companionFiles) == 0 && rawVisibility == "" { + return ErrorResult("patch requires either 'find' (content edit), 'files' (companion files), or 'visibility' (metadata update)") } info, ok := t.skills.GetSkill(ctx, slug) @@ -305,8 +344,8 @@ func (t *SkillManageTool) executePatch(ctx context.Context, args map[string]any) return ErrorResult(fmt.Sprintf("cannot manage skill %q: you are not the owner", slug)) } - // Visibility-only patch path: no content change, no new version. - if find == "" && rawVisibility != "" { + // Visibility-only patch path: no content/files change, no new version. + if find == "" && len(companionFiles) == 0 && rawVisibility != "" { skillID, err := uuid.Parse(info.ID) if err != nil { return ErrorResult(fmt.Sprintf("invalid skill ID in database: %v", err)) @@ -325,14 +364,43 @@ func (t *SkillManageTool) executePatch(ctx context.Context, args map[string]any) return NewResult(fmt.Sprintf("Skill %q visibility set to %s.", slug, newVisibility)) } - // Read current SKILL.md from latest version - current, err := os.ReadFile(info.Path) + newVer, commitLock, lockErr := t.skills.GetNextVersionLocked(ctx, slug) + if lockErr != nil { + return ErrorResult(fmt.Sprintf("failed to lock version: %v", lockErr)) + } + defer commitLock() //nolint:errcheck + + latestInfo, ok := t.skills.GetSkill(ctx, slug) + if !ok { + return ErrorResult(fmt.Sprintf("skill %q not found or archived", slug)) + } + if t.skills.IsSystemSkill(slug) { + return ErrorResult(fmt.Sprintf("cannot manage system skill %q", slug)) + } + if !canManageSkill(ctx, t.skills, latestInfo) { + return ErrorResult(fmt.Sprintf("cannot manage skill %q: you are not the owner", slug)) + } + + existingFiles, err := collectExistingManagedSkillCompanionFiles(latestInfo.BaseDir) + if err != nil { + return ErrorResult(fmt.Sprintf("failed to inspect companion files: %v", err)) + } + finalCompanionFiles := overlayManagedSkillFiles(existingFiles, companionFiles) + if err := validateManagedSkillTotalSize(finalCompanionFiles); err != nil { + return ErrorResult(err.Error()) + } + + // Read current SKILL.md from the latest version while the slug lock is held. + current, err := os.ReadFile(latestInfo.Path) if err != nil { return ErrorResult(fmt.Sprintf("failed to read current SKILL.md: %v", err)) } - patched := strings.Replace(string(current), find, replace, 1) - if patched == string(current) { + patched := string(current) + if find != "" { + patched = strings.Replace(patched, find, replace, 1) + } + if find != "" && patched == string(current) { return NewResult("no change: find text not found in current SKILL.md") } @@ -342,16 +410,17 @@ func (t *SkillManageTool) executePatch(ctx context.Context, args map[string]any) return ErrorResult(skills.FormatGuardViolations(violations)) } - oldVer := info.Version - newVer, commitLock, lockErr := t.skills.GetNextVersionLocked(ctx, slug) - if lockErr != nil { - return ErrorResult(fmt.Sprintf("failed to lock version: %v", lockErr)) - } - defer commitLock() //nolint:errcheck + oldVer := latestInfo.Version destDir := filepath.Join(t.tenantSkillsDir(ctx), slug, fmt.Sprintf("%d", newVer)) if err := os.MkdirAll(destDir, 0755); err != nil { return ErrorResult(fmt.Sprintf("failed to create new version directory: %v", err)) } + cleanupDest := true + defer func() { + if cleanupDest { + _ = os.RemoveAll(destDir) + } + }() // Write patched SKILL.md patchedBytes := []byte(patched) @@ -359,16 +428,18 @@ func (t *SkillManageTool) executePatch(ctx context.Context, args map[string]any) return ErrorResult(fmt.Sprintf("failed to write patched SKILL.md: %v", err)) } - // Copy any companion files from old version (scripts, assets, etc.) - if err := copyOtherFiles(info.BaseDir, destDir); err != nil { - slog.Warn("skill_manage: failed to copy companion files", "error", err) + if err := writeManagedSkillFiles(destDir, finalCompanionFiles); err != nil { + return ErrorResult(fmt.Sprintf("failed to write companion files: %v", err)) } // Hash + size hasher := sha256.New() hasher.Write(patchedBytes) fileHash := fmt.Sprintf("%x", hasher.Sum(nil)) - fileSize := int64(len(patchedBytes)) + fileSize, err := dirSize(destDir) + if err != nil { + return ErrorResult(fmt.Sprintf("failed to calculate skill size: %v", err)) + } // DB update skillID, err := uuid.Parse(info.ID) @@ -388,14 +459,19 @@ func (t *SkillManageTool) executePatch(ctx context.Context, args map[string]any) if err := t.skills.UpdateSkill(ctx, skillID, updates); err != nil { return ErrorResult(fmt.Sprintf("failed to update skill in database: %v", err)) } + cleanupDest = false - slog.Info("skill_manage: patched", "slug", slug, "old_version", oldVer, "new_version", newVer) + slog.Info("skill_manage: patched", "slug", slug, "old_version", oldVer, "new_version", newVer, "companion_files", len(companionFiles)) if t.loader != nil { t.loader.BumpVersion() } - return NewResult(fmt.Sprintf("Skill %q patched. v%d → v%d. Changes active next turn.", slug, oldVer, newVer)) + result := fmt.Sprintf("Skill %q patched. v%d → v%d. Changes active next turn.", slug, oldVer, newVer) + if len(companionFiles) > 0 { + result += fmt.Sprintf("\n- Companion files written: %d", len(companionFiles)) + } + return NewResult(result) } // executeDelete archives a skill in the DB and moves its directory to .trash/. @@ -455,53 +531,165 @@ func (t *SkillManageTool) executeDelete(ctx context.Context, args map[string]any // maxCopySize limits total companion file copy to 20MB (matching publish_skill). const maxCopySize = 20 << 20 -// copyOtherFiles copies all files from srcDir to dstDir except SKILL.md. -// Used by patch to carry companion files (scripts, assets) into the new version directory. -// Uses WalkDir (not Walk) so symlinks are detected via DirEntry.Type() before Stat follows them. -// Enforces a 20MB total size limit. -func copyOtherFiles(srcDir, dstDir string) error { +func parseManagedSkillFiles(raw any) ([]managedSkillFile, error) { + if raw == nil { + return nil, nil + } + files, ok := raw.(map[string]any) + if !ok { + return nil, fmt.Errorf("files must be an object mapping relative paths to string content") + } + out := make([]managedSkillFile, 0, len(files)) + for rawPath, rawContent := range files { + content, ok := rawContent.(string) + if !ok { + return nil, fmt.Errorf("files[%q] must be a string", rawPath) + } + cleanPath, err := validateManagedSkillFilePath(rawPath) + if err != nil { + return nil, err + } + if len(content) > maxManagedSkillFileSize { + return nil, fmt.Errorf("file %q too large (%d bytes, max %d)", cleanPath, len(content), maxManagedSkillFileSize) + } + out = append(out, managedSkillFile{Path: cleanPath, Content: []byte(content)}) + } + return out, nil +} + +func validateManagedSkillFilePath(rawPath string) (string, error) { + if rawPath == "" { + return "", fmt.Errorf("invalid file path %q: empty path", rawPath) + } + if strings.ContainsRune(rawPath, 0x00) { + return "", fmt.Errorf("invalid file path %q: null byte", rawPath) + } + if len(rawPath) >= 2 && rawPath[1] == ':' { + return "", fmt.Errorf("invalid file path %q: windows drive paths are not allowed", rawPath) + } + normalized := strings.ReplaceAll(rawPath, "\\", "/") + if strings.HasPrefix(normalized, "/") { + return "", fmt.Errorf("invalid file path %q: absolute paths are not allowed", rawPath) + } + for part := range strings.SplitSeq(normalized, "/") { + switch part { + case "..": + return "", fmt.Errorf("invalid file path %q: parent traversal is not allowed", rawPath) + case ".git": + return "", fmt.Errorf("invalid file path %q: system artifact paths are not allowed", rawPath) + } + if strings.HasPrefix(part, ".") { + return "", fmt.Errorf("invalid file path %q: hidden files are not allowed", rawPath) + } + } + cleanPath := path.Clean(normalized) + if cleanPath == "." || cleanPath == "SKILL.md" || strings.EqualFold(cleanPath, "SKILL.md") { + return "", fmt.Errorf("invalid file path %q: SKILL.md must be provided via content or find/replace", rawPath) + } + if strings.HasPrefix(cleanPath, "../") || cleanPath == ".." || strings.HasPrefix(cleanPath, "/") { + return "", fmt.Errorf("invalid file path %q: path escapes skill root", rawPath) + } + if skills.IsSystemArtifact(cleanPath) { + return "", fmt.Errorf("invalid file path %q: system artifact paths are not allowed", rawPath) + } + return cleanPath, nil +} + +func collectExistingManagedSkillCompanionFiles(srcDir string) ([]managedSkillFile, error) { + var out []managedSkillFile var totalSize int64 - return filepath.WalkDir(srcDir, func(path string, d os.DirEntry, err error) error { + err := filepath.WalkDir(srcDir, func(filePath string, d os.DirEntry, err error) error { if err != nil { return err } - // Skip symlinks — WalkDir exposes the raw type before following if d.Type()&os.ModeSymlink != 0 { return nil } - rel, err := filepath.Rel(srcDir, path) + rel, err := filepath.Rel(srcDir, filePath) if err != nil { return err } + rel = filepath.ToSlash(rel) if rel == "." || rel == "SKILL.md" { return nil } - // Skip path traversal attempts - if strings.Contains(rel, "..") { + cleanPath := path.Clean(rel) + if cleanPath == "." || strings.HasPrefix(cleanPath, "../") || cleanPath == ".." || strings.HasPrefix(cleanPath, "/") { + return fmt.Errorf("existing companion file %q escapes skill root", rel) + } + if skills.IsSystemArtifact(cleanPath) { + if d.IsDir() { + return filepath.SkipDir + } return nil } if d.IsDir() { - return os.MkdirAll(filepath.Join(dstDir, rel), 0755) + return nil } - fi, err := d.Info() + info, err := d.Info() if err != nil { return err } - totalSize += fi.Size() + totalSize += info.Size() if totalSize > maxCopySize { return fmt.Errorf("companion files exceed %d bytes limit", maxCopySize) } - src, err := os.Open(path) + data, err := os.ReadFile(filePath) if err != nil { return err } - defer src.Close() - dst, err := os.Create(filepath.Join(dstDir, rel)) - if err != nil { - return err - } - defer dst.Close() - _, err = io.Copy(dst, src) - return err + out = append(out, managedSkillFile{Path: cleanPath, Content: data}) + return nil }) + return out, err +} + +func overlayManagedSkillFiles(existing, payload []managedSkillFile) []managedSkillFile { + byPath := make(map[string]managedSkillFile, len(existing)+len(payload)) + order := make([]string, 0, len(existing)+len(payload)) + for _, file := range existing { + if _, exists := byPath[file.Path]; !exists { + order = append(order, file.Path) + } + byPath[file.Path] = file + } + for _, file := range payload { + if _, exists := byPath[file.Path]; !exists { + order = append(order, file.Path) + } + byPath[file.Path] = file + } + out := make([]managedSkillFile, 0, len(order)) + for _, filePath := range order { + out = append(out, byPath[filePath]) + } + return out +} + +func validateManagedSkillTotalSize(files []managedSkillFile) error { + var total int64 + for _, file := range files { + total += int64(len(file.Content)) + if total > maxCopySize { + return fmt.Errorf("companion files exceed %d bytes limit", maxCopySize) + } + } + return nil +} + +func writeManagedSkillFiles(destDir string, files []managedSkillFile) error { + for _, file := range files { + destPath := filepath.Join(destDir, filepath.FromSlash(file.Path)) + cleanDest := filepath.Clean(destPath) + if !strings.HasPrefix(cleanDest, destDir+string(filepath.Separator)) { + return fmt.Errorf("file %q escapes skill root", file.Path) + } + if err := os.MkdirAll(filepath.Dir(cleanDest), 0755); err != nil { + return err + } + if err := os.WriteFile(cleanDest, file.Content, 0644); err != nil { + return err + } + } + return nil } diff --git a/internal/tools/skill_manage_files_test.go b/internal/tools/skill_manage_files_test.go new file mode 100644 index 00000000..298df992 --- /dev/null +++ b/internal/tools/skill_manage_files_test.go @@ -0,0 +1,609 @@ +package tools + +import ( + "context" + "maps" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/skills" + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +type skillManageFilesStore struct { + nextBySlug map[string]int + skills map[uuid.UUID]store.SkillInfo + owners map[string]string + lastUpdates map[uuid.UUID]map[string]any + beforeVersionLockHook func(slug string) +} + +func newSkillManageFilesStore() *skillManageFilesStore { + return &skillManageFilesStore{ + nextBySlug: map[string]int{}, + skills: map[uuid.UUID]store.SkillInfo{}, + owners: map[string]string{}, + lastUpdates: map[uuid.UUID]map[string]any{}, + } +} + +func skillManageFilesContext() context.Context { + ctx := store.WithTenantID(context.Background(), store.MasterTenantID) + ctx = store.WithUserID(ctx, "owner") + ctx = store.WithSenderID(ctx, "owner") + ctx = store.WithAgentID(ctx, uuid.New()) + return ctx +} + +func writeManagedSkillVersion(t *testing.T, root, slug string, version int, content string) string { + t.Helper() + dir := filepath.Join(root, "skills-store", slug, "1") + if version != 1 { + dir = filepath.Join(root, "skills-store", slug, "2") + } + if err := os.MkdirAll(dir, 0755); err != nil { + t.Fatalf("mkdir skill dir: %v", err) + } + if err := os.WriteFile(filepath.Join(dir, "SKILL.md"), []byte(content), 0644); err != nil { + t.Fatalf("write SKILL.md: %v", err) + } + return dir +} + +func seedManagedSkill(t *testing.T, st *skillManageFilesStore, root, slug string, content string) (uuid.UUID, string) { + t.Helper() + dir := writeManagedSkillVersion(t, root, slug, 1, content) + id := uuid.New() + st.nextBySlug[slug] = 1 + st.owners[slug] = "owner" + st.skills[id] = store.SkillInfo{ + ID: id.String(), + TenantID: store.MasterTenantID.String(), + Name: "Managed Skill", + Slug: slug, + Path: filepath.Join(dir, "SKILL.md"), + BaseDir: dir, + Version: 1, + Status: "active", + Enabled: true, + Visibility: skills.VisibilityPrivate, + OwnerID: "owner", + } + return id, dir +} + +func validManagedSkillMarkdown(slug string) string { + return "---\nname: Managed Skill\nslug: " + slug + "\n---\nOriginal body\n" +} + +func newSkillManageFilesTool(root string, st *skillManageFilesStore) *SkillManageTool { + return NewSkillManageTool(st, filepath.Join(root, "skills-store"), root, nil) +} + +func TestSkillManagePatchFilesOnlyCreatesNewVersionWithReferenceFile(t *testing.T) { + t.Parallel() + root := t.TempDir() + st := newSkillManageFilesStore() + ctx := skillManageFilesContext() + seedManagedSkill(t, st, root, "managed-skill", validManagedSkillMarkdown("managed-skill")) + + res := newSkillManageFilesTool(root, st).Execute(ctx, map[string]any{ + "action": "patch", + "slug": "managed-skill", + "files": map[string]any{ + "references/ship-workflow.md": "# Ship\n", + }, + }) + if res.IsError { + t.Fatalf("patch returned error: %s", res.ForLLM) + } + if got := readTestFile(t, root, "skills-store/managed-skill/2/references/ship-workflow.md"); got != "# Ship\n" { + t.Fatalf("reference content = %q", got) + } + if got := st.latestBySlug("managed-skill").Version; got != 2 { + t.Fatalf("version = %d, want 2", got) + } +} + +func TestSkillManagePatchFindReplaceAndFilesCopiesExistingCompanions(t *testing.T) { + t.Parallel() + root := t.TempDir() + st := newSkillManageFilesStore() + ctx := skillManageFilesContext() + _, v1Dir := seedManagedSkill(t, st, root, "managed-skill", validManagedSkillMarkdown("managed-skill")) + if err := os.MkdirAll(filepath.Join(v1Dir, "assets"), 0755); err != nil { + t.Fatalf("mkdir assets: %v", err) + } + if err := os.WriteFile(filepath.Join(v1Dir, "assets/logo.txt"), []byte("logo"), 0644); err != nil { + t.Fatalf("write asset: %v", err) + } + + res := newSkillManageFilesTool(root, st).Execute(ctx, map[string]any{ + "action": "patch", + "slug": "managed-skill", + "find": "Original body", + "replace": "Updated body", + "files": map[string]any{ + "references/ship-workflow.md": "# Ship\n", + }, + }) + if res.IsError { + t.Fatalf("patch returned error: %s", res.ForLLM) + } + if got := readTestFile(t, root, "skills-store/managed-skill/2/SKILL.md"); !strings.Contains(got, "Updated body") { + t.Fatalf("patched SKILL.md missing update: %q", got) + } + if got := readTestFile(t, root, "skills-store/managed-skill/2/assets/logo.txt"); got != "logo" { + t.Fatalf("copied asset = %q", got) + } + if got := readTestFile(t, root, "skills-store/managed-skill/2/references/ship-workflow.md"); got != "# Ship\n" { + t.Fatalf("reference content = %q", got) + } +} + +func TestSkillManagePatchCopiesExistingHiddenCompanions(t *testing.T) { + t.Parallel() + root := t.TempDir() + st := newSkillManageFilesStore() + ctx := skillManageFilesContext() + _, v1Dir := seedManagedSkill(t, st, root, "managed-skill", validManagedSkillMarkdown("managed-skill")) + paths := map[string]string{ + ".env.example": "TOKEN=\n", + ".github/workflows/check.yml": "name: check\n", + "references/ship-workflow.md": "# Ship\n", + "references/nested/.keep-example.md": "keep\n", + } + for relPath, content := range paths { + fullPath := filepath.Join(v1Dir, filepath.FromSlash(relPath)) + if err := os.MkdirAll(filepath.Dir(fullPath), 0755); err != nil { + t.Fatalf("mkdir %s: %v", relPath, err) + } + if err := os.WriteFile(fullPath, []byte(content), 0644); err != nil { + t.Fatalf("write %s: %v", relPath, err) + } + } + + res := newSkillManageFilesTool(root, st).Execute(ctx, map[string]any{ + "action": "patch", + "slug": "managed-skill", + "files": map[string]any{ + "references/new.md": "# New\n", + }, + }) + if res.IsError { + t.Fatalf("patch returned error: %s", res.ForLLM) + } + for relPath, want := range paths { + got := readTestFile(t, root, filepath.Join("skills-store/managed-skill/2", filepath.ToSlash(relPath))) + if got != want { + t.Fatalf("copied %s = %q, want %q", relPath, got, want) + } + } +} + +func TestSkillManagePatchCopiesExistingLargeCompanionUnderTotalLimit(t *testing.T) { + t.Parallel() + root := t.TempDir() + st := newSkillManageFilesStore() + ctx := skillManageFilesContext() + _, v1Dir := seedManagedSkill(t, st, root, "managed-skill", validManagedSkillMarkdown("managed-skill")) + largeContent := strings.Repeat("x", maxManagedSkillFileSize+1) + largePath := filepath.Join(v1Dir, "assets", "large.txt") + if err := os.MkdirAll(filepath.Dir(largePath), 0755); err != nil { + t.Fatalf("mkdir large asset: %v", err) + } + if err := os.WriteFile(largePath, []byte(largeContent), 0644); err != nil { + t.Fatalf("write large asset: %v", err) + } + + res := newSkillManageFilesTool(root, st).Execute(ctx, map[string]any{ + "action": "patch", + "slug": "managed-skill", + "files": map[string]any{ + "references/new.md": "# New\n", + }, + }) + if res.IsError { + t.Fatalf("patch returned error: %s", res.ForLLM) + } + if got := readTestFile(t, root, "skills-store/managed-skill/2/assets/large.txt"); got != largeContent { + t.Fatalf("large asset length = %d, want %d", len(got), len(largeContent)) + } +} + +func TestSkillManagePatchReloadsLatestVersionAfterLock(t *testing.T) { + t.Parallel() + root := t.TempDir() + st := newSkillManageFilesStore() + ctx := skillManageFilesContext() + id, _ := seedManagedSkill(t, st, root, "managed-skill", validManagedSkillMarkdown("managed-skill")) + st.beforeVersionLockHook = func(slug string) { + if slug != "managed-skill" { + return + } + v2Dir := writeManagedSkillVersion(t, root, slug, 2, validManagedSkillMarkdown(slug)) + firstPath := filepath.Join(v2Dir, "references", "first.md") + if err := os.MkdirAll(filepath.Dir(firstPath), 0755); err != nil { + t.Fatalf("mkdir concurrent reference: %v", err) + } + if err := os.WriteFile(firstPath, []byte("# First\n"), 0644); err != nil { + t.Fatalf("write concurrent reference: %v", err) + } + skill := st.skills[id] + skill.Version = 2 + skill.BaseDir = v2Dir + skill.Path = filepath.Join(v2Dir, "SKILL.md") + st.skills[id] = skill + st.nextBySlug[slug] = 2 + st.beforeVersionLockHook = nil + } + + res := newSkillManageFilesTool(root, st).Execute(ctx, map[string]any{ + "action": "patch", + "slug": "managed-skill", + "files": map[string]any{ + "references/second.md": "# Second\n", + }, + }) + if res.IsError { + t.Fatalf("patch returned error: %s", res.ForLLM) + } + if got := readTestFile(t, root, "skills-store/managed-skill/3/references/first.md"); got != "# First\n" { + t.Fatalf("first concurrent reference = %q", got) + } + if got := readTestFile(t, root, "skills-store/managed-skill/3/references/second.md"); got != "# Second\n" { + t.Fatalf("second reference = %q", got) + } +} + +func TestSkillManagePatchFilesOverlayExistingCompanion(t *testing.T) { + t.Parallel() + root := t.TempDir() + st := newSkillManageFilesStore() + ctx := skillManageFilesContext() + _, v1Dir := seedManagedSkill(t, st, root, "managed-skill", validManagedSkillMarkdown("managed-skill")) + if err := os.MkdirAll(filepath.Join(v1Dir, "references"), 0755); err != nil { + t.Fatalf("mkdir references: %v", err) + } + if err := os.WriteFile(filepath.Join(v1Dir, "references/guide.md"), []byte("old"), 0644); err != nil { + t.Fatalf("write reference: %v", err) + } + + res := newSkillManageFilesTool(root, st).Execute(ctx, map[string]any{ + "action": "patch", + "slug": "managed-skill", + "files": map[string]any{ + "references/guide.md": "new", + }, + }) + if res.IsError { + t.Fatalf("patch returned error: %s", res.ForLLM) + } + if got := readTestFile(t, root, "skills-store/managed-skill/2/references/guide.md"); got != "new" { + t.Fatalf("overlaid reference content = %q", got) + } +} + +func TestSkillManageCreateWritesCompanionFiles(t *testing.T) { + t.Parallel() + root := t.TempDir() + st := newSkillManageFilesStore() + ctx := skillManageFilesContext() + + res := newSkillManageFilesTool(root, st).Execute(ctx, map[string]any{ + "action": "create", + "content": validManagedSkillMarkdown("new-skill"), + "files": map[string]any{ + "references/guide.md": "# Guide\n", + }, + }) + if res.IsError { + t.Fatalf("create returned error: %s", res.ForLLM) + } + if got := readTestFile(t, root, "skills-store/new-skill/1/references/guide.md"); got != "# Guide\n" { + t.Fatalf("reference content = %q", got) + } +} + +func TestSkillManageVisibilityOnlyPatchDoesNotCreateNewVersion(t *testing.T) { + t.Parallel() + root := t.TempDir() + st := newSkillManageFilesStore() + ctx := skillManageFilesContext() + id, _ := seedManagedSkill(t, st, root, "managed-skill", validManagedSkillMarkdown("managed-skill")) + + res := newSkillManageFilesTool(root, st).Execute(ctx, map[string]any{ + "action": "patch", + "slug": "managed-skill", + "visibility": skills.VisibilityPublic, + }) + if res.IsError { + t.Fatalf("patch returned error: %s", res.ForLLM) + } + if got := st.latestBySlug("managed-skill").Version; got != 1 { + t.Fatalf("version = %d, want unchanged v1", got) + } + if _, err := os.Stat(filepath.Join(root, "skills-store/managed-skill/2")); !os.IsNotExist(err) { + t.Fatalf("version 2 dir exists after visibility-only patch: err=%v", err) + } + if got := st.lastUpdates[id]["visibility"]; got != skills.VisibilityPublic { + t.Fatalf("visibility update = %v, want public", got) + } +} + +func TestSkillManageFilesRejectUnsafePathsBeforeCreatingVersion(t *testing.T) { + t.Parallel() + cases := []string{ + "/abs.md", + "../escape.md", + `C:/escape.md`, + "references/ok\x00.md", + ".git/config", + ".env", + "references/.secret", + "__MACOSX/x", + ".DS_Store", + "SKILL.md", + } + for _, relPath := range cases { + t.Run(relPath, func(t *testing.T) { + t.Parallel() + root := t.TempDir() + st := newSkillManageFilesStore() + ctx := skillManageFilesContext() + seedManagedSkill(t, st, root, "managed-skill", validManagedSkillMarkdown("managed-skill")) + + res := newSkillManageFilesTool(root, st).Execute(ctx, map[string]any{ + "action": "patch", + "slug": "managed-skill", + "files": map[string]any{ + relPath: "bad", + }, + }) + if !res.IsError { + t.Fatalf("patch succeeded for unsafe path %q: %s", relPath, res.ForLLM) + } + if !strings.Contains(res.ForLLM, "invalid file path") { + t.Fatalf("error = %q, want invalid file path", res.ForLLM) + } + if _, err := os.Stat(filepath.Join(root, "skills-store/managed-skill/2")); !os.IsNotExist(err) { + t.Fatalf("version 2 dir exists after rejected path: err=%v", err) + } + }) + } +} + +func TestSkillManageFilesRejectNonStringPayload(t *testing.T) { + t.Parallel() + root := t.TempDir() + st := newSkillManageFilesStore() + ctx := skillManageFilesContext() + seedManagedSkill(t, st, root, "managed-skill", validManagedSkillMarkdown("managed-skill")) + + res := newSkillManageFilesTool(root, st).Execute(ctx, map[string]any{ + "action": "patch", + "slug": "managed-skill", + "files": map[string]any{ + "references/guide.md": map[string]any{"nested": "no"}, + }, + }) + if !res.IsError { + t.Fatalf("patch succeeded with non-string payload: %s", res.ForLLM) + } + if !strings.Contains(res.ForLLM, "must be a string") { + t.Fatalf("error = %q, want string validation", res.ForLLM) + } + if _, err := os.Stat(filepath.Join(root, "skills-store/managed-skill/2")); !os.IsNotExist(err) { + t.Fatalf("version 2 dir exists after rejected payload: err=%v", err) + } +} + +func TestSkillManageFilesRejectOversizePayloadBeforeCreatingVersion(t *testing.T) { + t.Parallel() + root := t.TempDir() + st := newSkillManageFilesStore() + ctx := skillManageFilesContext() + seedManagedSkill(t, st, root, "managed-skill", validManagedSkillMarkdown("managed-skill")) + + res := newSkillManageFilesTool(root, st).Execute(ctx, map[string]any{ + "action": "patch", + "slug": "managed-skill", + "files": map[string]any{ + "references/too-large.md": strings.Repeat("x", maxManagedSkillFileSize+1), + }, + }) + if !res.IsError { + t.Fatalf("patch succeeded with oversize payload: %s", res.ForLLM) + } + if !strings.Contains(res.ForLLM, "too large") { + t.Fatalf("error = %q, want size validation", res.ForLLM) + } + if _, err := os.Stat(filepath.Join(root, "skills-store/managed-skill/2")); !os.IsNotExist(err) { + t.Fatalf("version 2 dir exists after rejected payload: err=%v", err) + } +} + +func TestSkillManagePatchFindMissWithFilesDoesNotCreateVersion(t *testing.T) { + t.Parallel() + root := t.TempDir() + st := newSkillManageFilesStore() + ctx := skillManageFilesContext() + seedManagedSkill(t, st, root, "managed-skill", validManagedSkillMarkdown("managed-skill")) + + res := newSkillManageFilesTool(root, st).Execute(ctx, map[string]any{ + "action": "patch", + "slug": "managed-skill", + "find": "missing text", + "replace": "replacement", + "files": map[string]any{ + "references/guide.md": "# Guide\n", + }, + }) + if res.IsError { + t.Fatalf("patch returned error: %s", res.ForLLM) + } + if !strings.Contains(res.ForLLM, "no change") { + t.Fatalf("result = %q, want no-change message", res.ForLLM) + } + if _, err := os.Stat(filepath.Join(root, "skills-store/managed-skill/2")); !os.IsNotExist(err) { + t.Fatalf("version 2 dir exists after missing find: err=%v", err) + } +} + +func readTestFile(t *testing.T, root, rel string) string { + t.Helper() + data, err := os.ReadFile(filepath.Join(root, filepath.FromSlash(rel))) + if err != nil { + t.Fatalf("read %s: %v", rel, err) + } + return string(data) +} + +func (s *skillManageFilesStore) latestBySlug(slug string) store.SkillInfo { + var latest store.SkillInfo + for _, skill := range s.skills { + if skill.Slug == slug && skill.Version > latest.Version { + latest = skill + } + } + return latest +} + +func (s *skillManageFilesStore) ListSkills(context.Context) []store.SkillInfo { return nil } +func (s *skillManageFilesStore) LoadSkill(context.Context, string) (string, bool) { + return "", false +} +func (s *skillManageFilesStore) LoadForContext(context.Context, []string) string { return "" } +func (s *skillManageFilesStore) BuildSummary(context.Context, []string) string { return "" } +func (s *skillManageFilesStore) GetSkill(_ context.Context, slug string) (*store.SkillInfo, bool) { + for _, skill := range s.skills { + if skill.Slug == slug && skill.Status != "deleted" { + copy := skill + return ©, true + } + } + return nil, false +} +func (s *skillManageFilesStore) FilterSkills(context.Context, []string) []store.SkillInfo { + return nil +} +func (s *skillManageFilesStore) Version() int64 { return 0 } +func (s *skillManageFilesStore) BumpVersion() {} +func (s *skillManageFilesStore) Dirs() []string { return nil } +func (s *skillManageFilesStore) CreateSkillManaged(ctx context.Context, p store.SkillCreateParams) (uuid.UUID, error) { + id := uuid.New() + version := p.Version + if version == 0 { + version = s.nextBySlug[p.Slug] + 1 + } + if version > s.nextBySlug[p.Slug] { + s.nextBySlug[p.Slug] = version + } + s.owners[p.Slug] = p.OwnerID + s.skills[id] = store.SkillInfo{ + ID: id.String(), + TenantID: store.MasterTenantID.String(), + Name: p.Name, + Slug: p.Slug, + Description: derefString(p.Description), + Path: filepath.Join(p.FilePath, "SKILL.md"), + BaseDir: p.FilePath, + Version: version, + Status: "active", + Enabled: true, + Visibility: p.Visibility, + OwnerID: p.OwnerID, + } + return id, nil +} +func (s *skillManageFilesStore) UpdateSkill(_ context.Context, id uuid.UUID, updates map[string]any) error { + skill, ok := s.skills[id] + if !ok { + return nil + } + if version, ok := updates["version"].(int); ok { + skill.Version = version + if version > s.nextBySlug[skill.Slug] { + s.nextBySlug[skill.Slug] = version + } + } + if filePath, ok := updates["file_path"].(string); ok { + skill.BaseDir = filePath + skill.Path = filepath.Join(filePath, "SKILL.md") + } + if visibility, ok := updates["visibility"].(string); ok { + skill.Visibility = visibility + } + s.lastUpdates[id] = maps.Clone(updates) + s.skills[id] = skill + return nil +} +func (s *skillManageFilesStore) DeleteSkill(context.Context, uuid.UUID) error { return nil } +func (s *skillManageFilesStore) ToggleSkill(context.Context, uuid.UUID, bool) error { return nil } +func (s *skillManageFilesStore) GetSkillByID(_ context.Context, id uuid.UUID) (store.SkillInfo, bool) { + info, ok := s.skills[id] + return info, ok +} +func (s *skillManageFilesStore) GetSkillOwnerID(context.Context, uuid.UUID) (string, bool) { + return "", false +} +func (s *skillManageFilesStore) GetSkillOwnerIDBySlug(_ context.Context, slug string) (string, bool) { + owner, ok := s.owners[slug] + return owner, ok +} +func (s *skillManageFilesStore) GetNextVersion(_ context.Context, slug string) int { + return s.nextBySlug[slug] + 1 +} +func (s *skillManageFilesStore) GetNextVersionLocked(_ context.Context, slug string) (int, func() error, error) { + if s.beforeVersionLockHook != nil { + s.beforeVersionLockHook(slug) + } + return s.GetNextVersion(context.Background(), slug), func() error { return nil }, nil +} +func (s *skillManageFilesStore) GetSkillHashBySlug(context.Context, string) (string, int, bool) { + return "", 0, false +} +func (s *skillManageFilesStore) IsSystemSkill(string) bool { return false } +func (s *skillManageFilesStore) ListAllSkills(context.Context) []store.SkillInfo { return nil } +func (s *skillManageFilesStore) ListAllSystemSkills(context.Context) []store.SkillInfo { + return nil +} +func (s *skillManageFilesStore) ListSystemSkillDirs(context.Context) map[string]string { + return nil +} +func (s *skillManageFilesStore) StoreMissingDeps(context.Context, uuid.UUID, []string) error { + return nil +} +func (s *skillManageFilesStore) GrantToAgent(context.Context, uuid.UUID, uuid.UUID, int, string, ...bool) error { + return nil +} +func (s *skillManageFilesStore) RevokeFromAgent(context.Context, uuid.UUID, uuid.UUID) error { + return nil +} +func (s *skillManageFilesStore) GrantToUser(context.Context, uuid.UUID, string, string) error { + return nil +} +func (s *skillManageFilesStore) RevokeFromUser(context.Context, uuid.UUID, string) error { return nil } +func (s *skillManageFilesStore) ListWithGrantStatus(context.Context, uuid.UUID) ([]store.SkillWithGrantStatus, error) { + return nil, nil +} +func (s *skillManageFilesStore) ListAgentGrantsForSkill(context.Context, uuid.UUID) ([]store.SkillAgentGrantInfo, error) { + return nil, nil +} +func (s *skillManageFilesStore) AgentCanManageSkill(context.Context, uuid.UUID, uuid.UUID) (bool, error) { + return false, nil +} +func (s *skillManageFilesStore) GetSkillFilePath(context.Context, uuid.UUID) (string, string, int, bool, bool) { + return "", "", 0, false, false +} + +func derefString(v *string) string { + if v == nil { + return "" + } + return *v +} diff --git a/plans/260528-1805-skill-manage-companion-files/phase-01-tdd-contract-and-threat-model.md b/plans/260528-1805-skill-manage-companion-files/phase-01-tdd-contract-and-threat-model.md new file mode 100644 index 00000000..437aa24f --- /dev/null +++ b/plans/260528-1805-skill-manage-companion-files/phase-01-tdd-contract-and-threat-model.md @@ -0,0 +1,80 @@ +--- +phase: 1 +title: "TDD Contract and Threat Model" +status: complete +priority: P1 +effort: "2h" +dependencies: [] +--- + +# Phase 1: TDD Contract and Threat Model + +## Overview + +Lock the tool contract and threat model with failing tests before implementation. The core risk is not versioning; it is allowing agents to write arbitrary relative paths into a managed skill directory without path traversal, system-artifact, or size-limit gaps. + +## Requirements + +- Functional: define `files` payload for `skill_manage` create and patch. +- Functional: patch can be file-only or `find`/`replace` plus files. +- Functional: existing companion files copy forward and payload entries overlay them. +- Non-functional: no filesystem staging required before `skill_manage`. +- Non-functional: no UI file editor and no SQLite work in this round. +- Security: reject unsafe paths before any disk write. + +## Architecture + +`skill_manage` remains the only changed public tool. It should treat `files` as a version payload: + +1. Resolve current skill and manage permission. +2. Build final `SKILL.md` content. +3. Validate final `SKILL.md` with `skills.GuardSkillContent`. +4. Validate companion file payload paths, names, sizes, and total size. +5. Create new version dir. +6. Copy existing companions from previous version. +7. Overlay payload files. +8. Update DB metadata and bump loader version. + +## Related Code Files + +- Modify: `internal/tools/skill_manage.go` +- Read: `internal/tools/publish_skill.go` +- Read: `internal/skills/guard.go` +- Read: `internal/skills/archive_extract.go` +- Read: `internal/http/skills_versions.go` +- Modify/create tests near existing `internal/tools` tests. + +## Implementation Steps + +1. Add tests first for successful file-only patch: + - create managed skill v1 with `SKILL.md` + - call `skill_manage patch` with `files: {"references/ship-workflow.md": "# Ship"}` + - assert v2 directory has `SKILL.md` and `references/ship-workflow.md` + - assert DB version moved to v2 +2. Add tests for patch with `find`/`replace` plus `files` in the same call. +3. Add tests for create with `content` plus `files`. +4. Add tests that existing companion files copy forward: + - v1 has `assets/logo.txt` + - patch adds `references/a.md` + - v2 has both files +5. Add rejection tests before implementation: + - absolute path + - `../escape.md` + - Windows drive path such as `C:/x` + - null byte path + - system artifacts such as `.git/config`, `.DS_Store`, `__MACOSX/x` + - total payload/copy size above limit +6. Keep test fixtures small; do not add stress or benchmark tests. + +## Success Criteria + +- [x] Tests fail for missing `files` support before implementation. +- [x] Tests cover manage permission path by using existing owner/manage grant helpers where practical. +- [x] Path rejection tests prove no unsafe file lands on disk. +- [x] Test names describe behavior, not plan/finding labels. + +## Risk Assessment + +- Risk: validating only raw strings but not cleaned paths. Mitigation: test both raw and cleaned escape forms. +- Risk: partial version directory left after rejected write. Mitigation: validation must happen before creating destination; add cleanup expectation if destination is created. +- Risk: test setup over-couples to PG. Mitigation: keep Phase 1 tests at tool/filesystem level with a fake or existing test store where possible; add PG-specific coverage only if needed for version metadata. diff --git a/plans/260528-1805-skill-manage-companion-files/phase-02-skill-manage-file-payload-implementation.md b/plans/260528-1805-skill-manage-companion-files/phase-02-skill-manage-file-payload-implementation.md new file mode 100644 index 00000000..a8beca97 --- /dev/null +++ b/plans/260528-1805-skill-manage-companion-files/phase-02-skill-manage-file-payload-implementation.md @@ -0,0 +1,103 @@ +--- +phase: 2 +title: "Skill Manage File Payload Implementation" +status: complete +priority: P1 +effort: "4h" +dependencies: [1] +--- + +# Phase 2: Skill Manage File Payload Implementation + +## Overview + +Implement the smallest safe `files` extension inside `skill_manage`. Reuse existing copy behavior where it is correct, but add explicit payload validation because `publish_skill` copies trusted workspace directories while this path accepts direct model-provided content. + +## Requirements + +- Functional: `files` is optional object/map on `create` and `patch`. +- Functional: create requires `content`; patch requires at least one of `find`, `visibility`, or non-empty `files`. +- Functional: file-only patch creates a new immutable version, not a metadata-only update. +- Functional: visibility-only patch remains metadata-only and should not create a new version. +- Non-functional: no new database tables or migrations. +- Security: reject unsafe paths and oversize content before writing. + +## Architecture + +Add a small internal representation: + +```go +type skillManagedFile struct { + Path string + Content string +} +``` + +Parsing should accept a JSON-object-shaped value from tool args: + +```json +{ + "files": { + "references/ship-workflow.md": "# Ship workflow", + "scripts/check.sh": "#!/usr/bin/env bash\n..." + } +} +``` + +Validation rules: +- path is relative after separator normalization +- no `..` component +- no absolute path +- no Windows drive prefix +- no null byte +- not `SKILL.md`; main content stays controlled by `content` or `find`/`replace` +- not `skills.IsSystemArtifact(path)` nor any system-artifact path component +- file content fits per-file limit +- final companion copy + payload total fits `maxCopySize` + +## Related Code Files + +- Modify: `internal/tools/skill_manage.go` +- Optional helper extraction: `internal/tools/publish_skill.go` +- Do not modify: `internal/store/pg/skills_crud.go` unless metadata size calculation requires no alternative. + +## Implementation Steps + +1. Extend `SkillManageTool.Parameters()` with `files`. +2. Add parser helper for `files` from `map[string]any`, validating all values are strings. +3. Add path validation helper in `internal/tools/skill_manage.go` or a small shared helper if `publish_skill` can reuse it without churn. +4. Update create flow: + - validate `SKILL.md` content + - parse and validate files + - create version dir + - write `SKILL.md` + - write files + - compute directory size and hash + - register skill +5. Update patch flow: + - permit `files` without `find` + - preserve visibility-only fast path when no content/files changes + - read current `SKILL.md` from the latest version while the slug lock is held + - validate final content and file payload + - create new version dir + - write final `SKILL.md` + - copy existing companions + - overlay payload files + - compute directory size and hash + - update DB +6. If copy/overlay fails after destination creation, remove the new version directory before returning error. +7. Keep response concise but mention count of companion files written. + +## Success Criteria + +- [x] Phase 1 tests pass. +- [x] `skill_manage patch` can add `references/*.md` without filesystem staging. +- [x] `skill_manage patch` with only `visibility` still does not create a new version. +- [x] Invalid file payloads fail without partial durable writes. +- [x] Code stays in existing tool boundary; no broad refactor. + +## Risk Assessment + +- Risk: model passes nested non-string values. Mitigation: fail clearly; no implicit JSON serialization. +- Risk: `file_size` remains `SKILL.md`-only. Mitigation: compute version directory size after writes or intentionally document if existing metadata semantics stay unchanged; preferred is directory size. +- Risk: helper reuse from `publish_skill` causes unnecessary churn. Mitigation: duplicate tiny validation if extraction would make unrelated code noisier. diff --git a/plans/260528-1805-skill-manage-companion-files/phase-03-runtime-readback-and-documentation.md b/plans/260528-1805-skill-manage-companion-files/phase-03-runtime-readback-and-documentation.md new file mode 100644 index 00000000..80720ae9 --- /dev/null +++ b/plans/260528-1805-skill-manage-companion-files/phase-03-runtime-readback-and-documentation.md @@ -0,0 +1,62 @@ +--- +phase: 3 +title: "Runtime Readback and Documentation" +status: complete +priority: P2 +effort: "2h" +dependencies: [2] +--- + +# Phase 3: Runtime Readback and Documentation + +## Overview + +Verify that companion files written by `skill_manage` are visible through existing runtime and HTTP readback paths, then update docs so agents know when to use `skill_manage` versus `publish_skill`. + +## Requirements + +- Functional: existing `/v1/skills/{id}/files` lists new companion files. +- Functional: existing `/v1/skills/{id}/files/{path}` reads new companion files. +- Documentation: update tool contract and examples. +- Out of scope: new UI editor, new REST update endpoint, SQLite Desktop support. + +## Architecture + +No new runtime API should be needed. Existing file APIs derive the version directory from `skills.file_path`, and `skill_manage` updates that path during patch. The validation task is to prove the new files are located under that directory. + +## Related Code Files + +- Read/test: `internal/http/skills_versions.go` +- Modify: `docs/21-agent-evolution-and-skill-management.md` +- Modify: `docs/16-skill-publishing.md` if cross-reference wording becomes stale +- Optional modify: `docs/15-core-skills-system.md` endpoint table only if needed +- Optional modify: `internal/agent/systemprompt.go` if agent guidance should mention companion files + +## Implementation Steps + +1. Add readback test if existing coverage does not already prove arbitrary companion files: + - create or patch managed skill with `references/ship-workflow.md` + - call list files helper/handler + - call read file helper/handler +2. Confirm no frontend change is required: + - `useSkills.getSkillFiles` already calls `/v1/skills/{id}/files` + - `useSkills.getSkillFileContent` already reads a path + - `SkillUploadDialog` already supports ZIP upload for UI acceptance +3. Update docs: + - `skill_manage` now supports `files` + - accepted paths and limits + - examples for adding `references/*.md` + - contrast with `publish_skill` for bulk directory publish +4. If `internal/agent/systemprompt.go` is changed, keep guidance to one concise line to avoid prompt bloat. + +## Success Criteria + +- [x] Existing file viewer API can list/read added reference files. +- [x] Docs describe `files` payload and security constraints. +- [x] Docs explicitly say UI editor remains out of scope; use ZIP upload in UI. +- [x] No stale statement remains that `skill_manage` is strictly `SKILL.md`-only. + +## Risk Assessment + +- Risk: docs overpromise arbitrary binary assets while payload is string-only. Mitigation: say text/file content payload; binary assets should continue through ZIP upload unless implementation adds encoding. +- Risk: prompt guidance causes agents to prefer `skill_manage` for bulk imports. Mitigation: docs recommend `publish_skill` for pre-existing directories and ZIP upload for UI. diff --git a/plans/260528-1805-skill-manage-companion-files/phase-04-validation-and-issue-handoff.md b/plans/260528-1805-skill-manage-companion-files/phase-04-validation-and-issue-handoff.md new file mode 100644 index 00000000..bdd390c3 --- /dev/null +++ b/plans/260528-1805-skill-manage-companion-files/phase-04-validation-and-issue-handoff.md @@ -0,0 +1,60 @@ +--- +phase: 4 +title: "Validation and Issue Handoff" +status: pending +priority: P1 +effort: "2h" +dependencies: [3] +--- + +# Phase 4: Validation and Issue Handoff + +## Overview + +Run focused validation for the planning scope, then update GitHub issue #72 with the implementation summary and plan path. Do not claim SQLite/Desktop support unless separately implemented later. + +## Requirements + +- Validation: focused tests for `skill_manage` and skill file readback pass. +- Validation: compile check for Go package surface touched by the change. +- Handoff: issue comment links this plan and states scope boundaries. +- Git: commit and push implementation or plan changes according to requested workflow. + +## Architecture + +Validation should stay proportional. This issue changes a tool contract and filesystem writes; it does not require load tests or full integration stress. + +## Related Code Files + +- Test command surface: `go test ./internal/tools ./internal/http ./internal/skills` +- Compile command surface: `go test ./internal/tools` +- GitHub issue: `digitopvn/goclaw#72` +- Plan path: `plans/260528-1805-skill-manage-companion-files/plan.md` + +## Implementation Steps + +1. Run focused Go tests for modified packages. +2. Run broader compile-safe test only if helper extraction touches shared packages. +3. Inspect `git diff --stat` and `git diff --check`. +4. Commit with conventional message: + - plan-only: `feat(skills): plan skill_manage companion files` + - implementation later: use `fix(skills): allow skill_manage companion files` +5. Push branch. +6. Comment on issue #72: + - plan path + - accepted scope + - planned phases + - explicit exclusions +7. If implemented later, include validation commands and results in the issue comment or PR body. + +## Success Criteria + +- [ ] Tests relevant to touched packages pass. +- [ ] Branch is pushed. +- [ ] GitHub issue #72 has a concise comment with plan summary and filepath. +- [ ] Handoff does not imply unsupported SQLite/Desktop scope. + +## Risk Assessment + +- Risk: current branch name could imply unrelated issue. Mitigation: use `codex/issue-72-skill-manage-files-plan` for plan commit. +- Risk: issue comment becomes too verbose. Mitigation: concise summary with plan path and phase list. diff --git a/plans/260528-1805-skill-manage-companion-files/plan.md b/plans/260528-1805-skill-manage-companion-files/plan.md new file mode 100644 index 00000000..9bb6fbe4 --- /dev/null +++ b/plans/260528-1805-skill-manage-companion-files/plan.md @@ -0,0 +1,63 @@ +--- +title: "Skill Manage Companion Files" +description: "TDD plan for issue #72: let skill_manage create new immutable skill versions with SKILL.md plus companion files." +status: in_progress +priority: P2 +branch: "codex/issue-72-skill-manage-files-plan" +tags: [skills, tools, tdd, issue-72] +blockedBy: [] +blocks: [] +created: "2026-05-28T11:05:51.716Z" +createdBy: "ck:plan" +source: skill +--- + +# Skill Manage Companion Files + +## Overview + +Fix `digitopvn/goclaw#72` by extending the agent-facing `skill_manage` tool so agents with manage access can add or overwrite companion files while creating a new immutable managed-skill version. + +Scope is intentionally narrow: +- PostgreSQL Standard only for this round. +- No web UI file editor; existing ZIP upload and file viewer satisfy UI acceptance. +- No change to `publish_skill` except optional helper reuse. +- No execution/install behavior for added scripts; only store and expose files safely. + +Approved contract: +- `skill_manage(action="create"|"patch", files={...})` accepts relative file paths under the skill root. +- Allowed files include `references/**/*.md`, `scripts/**`, `assets/**`, and arbitrary non-system files. +- Patch with only `files` is valid and creates one new immutable version. +- Patch with `find`/`replace` plus `files` creates one new immutable version. +- Existing companion files copy forward; new payload overlays additions/updates. +- Security scanner still validates final `SKILL.md`; companion paths and sizes are separately validated. + +## Phases + +| Phase | Name | Status | +|-------|------|--------| +| 1 | [TDD Contract and Threat Model](./phase-01-tdd-contract-and-threat-model.md) | Complete | +| 2 | [Skill Manage File Payload Implementation](./phase-02-skill-manage-file-payload-implementation.md) | Complete | +| 3 | [Runtime Readback and Documentation](./phase-03-runtime-readback-and-documentation.md) | Complete | +| 4 | [Validation and Issue Handoff](./phase-04-validation-and-issue-handoff.md) | Pending | + +## Dependencies + +- Related issue: https://github.com/digitopvn/goclaw/issues/72 +- Existing tool surface: `internal/tools/skill_manage.go` +- Existing directory publish behavior: `internal/tools/publish_skill.go` +- Existing runtime readback: `internal/http/skills_versions.go` +- Existing docs: `docs/21-agent-evolution-and-skill-management.md`, `docs/16-skill-publishing.md` + +## Success Criteria + +- Agent with manage access can patch an existing skill and add `references/ship-workflow.md`. +- New version contains updated `SKILL.md` plus newly added files. +- Existing companion files survive patch unless overwritten. +- Runtime/API file reader can read newly added files. +- Invalid paths and system artifacts are rejected before disk write. +- Focused Go tests cover success and rejection paths. + +## Unresolved Questions + +None. User approved scope decisions on 2026-05-28.