fix: allow skill_manage companion files

Squash merge PR #104 after validation.
This commit is contained in:
Duy /zuey/
2026-05-31 12:24:22 +07:00
committed by GitHub
parent 484bcc7b71
commit df8a03df48
10 changed files with 1269 additions and 62 deletions
+4
View File
@@ -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`:
+17 -14
View File
@@ -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 +<br/>optional files"] --> C2["Size and path<br/>validation"]
C2 --> C3["Security scan<br/>SKILL.md"]
C3 --> C4["Parse frontmatter"]
C4 --> C5["Slug validation"]
C5 --> C6["System skill<br/>conflict check"]
C6 --> C7["Write SKILL.md<br/>to versioned dir"]
C6 --> C7["Write SKILL.md +<br/>companions"]
C7 --> C8["DB insert<br/>(advisory lock)"]
C8 --> C9["Auto-grant +<br/>dep scan"]
end
subgraph PATCH["action = patch"]
direction TB
P1["slug + find/replace"] --> P2["Exists?<br/>System skill?"]
P1["slug + find/replace<br/>and/or files"] --> P2["Exists?<br/>System skill?"]
P2 --> P3["Ownership check"]
P3 --> P4["Read current +<br/>apply patch"]
P4 --> P5["Security scan<br/>patched content"]
P3 --> P4["Read current +<br/>overlay files"]
P4 --> P5["Security scan +<br/>path validation"]
P5 --> P6["New version<br/>(advisory lock)"]
P6 --> P7["Copy companions +<br/>DB update"]
P6 --> P7["Write companions +<br/>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 |
---
+35
View File
@@ -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())
}
}
+236 -48
View File
@@ -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
}
+609
View File
@@ -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 &copy, 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
}
@@ -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.
@@ -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.
@@ -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.
@@ -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.
@@ -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.