diff --git a/docs/EXTENDING.md b/docs/EXTENDING.md index 96d18b464..6afb49ec0 100644 --- a/docs/EXTENDING.md +++ b/docs/EXTENDING.md @@ -110,6 +110,8 @@ Discovery roots (earlier wins on name collisions): A missing directory is fine — Zero just omits it. Management commands (`zero skill add` / `zero skill remove`) always write to the primary Zero dir only; `list` / `info` search primary + `~/.agents/skills`. `skill info` reports lock source/hash when present and whether the on-disk `SKILL.md` has drifted from the recorded hash. +For a skill installed since Zero started installing bundled files, `skill info` also reports drift in any file installed with it. + ```text ~/.local/share/zero/skills/ # primary (install/remove target) run-benchmarks/ @@ -140,6 +142,15 @@ Only `name` and `description` are recognized in the frontmatter today. The `name The `skill` tool lets the agent load any discovered skill by name (primary, agents, or plugin). +A skill can ship files beside its `SKILL.md`, in the [agentskills.io](https://agentskills.io/specification) layout: `scripts/`, `references/`, `assets/` or anything else the instructions refer to. `zero skill add` installs the whole skill directory, leaving out hidden entries such as `.git` and not following or copying symlinks (it lists any it skipped). A skill larger than 1,000 files or 32 MiB is refused. When the `skill` tool loads a skill that has such files, its output ends with the skill's directory and a list of the files, so instructions like "run `scripts/fill.py`" can be followed. Running a bundled script goes through the normal tool permissions; installing or loading a skill never runs anything. + +```text +pdf/ + SKILL.md + scripts/fill.py + references/form-fields.md +``` + ## 4. Hooks Hooks fire shell commands on lifecycle events. Configure them in JSON: diff --git a/internal/cli/distribution.go b/internal/cli/distribution.go index d710964f5..f31e94867 100644 --- a/internal/cli/distribution.go +++ b/internal/cli/distribution.go @@ -92,13 +92,25 @@ func runSkillAdd(args []string, dir string, stdout io.Writer, stderr io.Writer) lines := []string{} if result.Updated { lines = append(lines, fmt.Sprintf("Updated skill %q.", result.Name)) - lines = append(lines, fmt.Sprintf(" hash: %s -> %s", shortHash(result.PreviousHash), shortHash(result.Hash))) + switch { + case result.PreviousHash != result.Hash: + lines = append(lines, fmt.Sprintf(" hash: %s -> %s", shortHash(result.PreviousHash), shortHash(result.Hash))) + case result.PreviousTreeHash == "": + // The install this replaced predates tree hashes and copied SKILL.md + // alone, so the bundled files are new rather than changed. + lines = append(lines, " SKILL.md is unchanged; the files bundled with it are now installed too") + default: + lines = append(lines, " SKILL.md is unchanged; the files bundled with it changed") + } } else { lines = append(lines, fmt.Sprintf("Installed skill %q.", result.Name)) lines = append(lines, " hash: "+result.Hash) } lines = append(lines, " source: "+result.Source) lines = append(lines, " path: "+result.Path) + if len(result.SkippedLinks) > 0 { + lines = append(lines, " not copied (symlinks): "+strings.Join(result.SkippedLinks, ", ")) + } if _, err := fmt.Fprintln(stdout, redaction.RedactString(strings.Join(lines, "\n"), redaction.Options{})); err != nil { return exitCrash } diff --git a/internal/cli/distribution_test.go b/internal/cli/distribution_test.go index b636f2fa3..20eafd229 100644 --- a/internal/cli/distribution_test.go +++ b/internal/cli/distribution_test.go @@ -7,6 +7,8 @@ import ( "path/filepath" "strings" "testing" + + "github.com/Twigpine/zero/internal/skills" ) func writeSourceSkillDir(t *testing.T, dir string, content string) string { @@ -391,3 +393,133 @@ func TestRunDistributionCommandsDoNotLaunchTUI(t *testing.T) { } } } + +// A reinstall that only changed a bundled script must still read as an update +// (#584): the SKILL.md hash is the same, so "hash: X -> X" would say nothing. +func TestRunSkillAddReportsAnUpdateToBundledFilesOnly(t *testing.T) { + skillsDir := t.TempDir() + src := writeSourceSkillDir(t, filepath.Join(t.TempDir(), "src"), + "---\nname: pdf\ndescription: Fill forms.\n---\nRun scripts/fill.py.\n") + script := filepath.Join(src, "scripts", "fill.py") + if err := os.MkdirAll(filepath.Dir(script), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(script, []byte("print('v1')\n"), 0o755); err != nil { + t.Fatal(err) + } + deps := appDeps{skillsDir: func() string { return skillsDir }} + + var stdout, stderr bytes.Buffer + if exit := runWithDeps([]string{"skill", "add", src}, &stdout, &stderr, deps); exit != 0 { + t.Fatalf("first skill add exit = %d, stderr = %s", exit, stderr.String()) + } + if err := os.WriteFile(script, []byte("print('v2')\n"), 0o755); err != nil { + t.Fatal(err) + } + stdout.Reset() + stderr.Reset() + if exit := runWithDeps([]string{"skill", "add", src}, &stdout, &stderr, deps); exit != 0 { + t.Fatalf("second skill add exit = %d, stderr = %s", exit, stderr.String()) + } + for _, want := range []string{`Updated skill "pdf".`, "SKILL.md is unchanged; the files bundled with it changed"} { + if !strings.Contains(stdout.String(), want) { + t.Fatalf("second add output does not say %q:\n%s", want, stdout.String()) + } + } +} + +// An install from before tree hashes copied SKILL.md alone and recorded no tree +// hash. Adding the skill again over that entry must not claim bundled files +// changed: there are none, or they are installed for the first time. +func TestRunSkillAddOverAnEntryWithoutATreeHash(t *testing.T) { + for _, tc := range []struct { + name string + script bool + want []string + notWant []string + }{ + { + name: "SKILL.md only", + want: []string{`Installed skill "pdf".`}, + notWant: []string{"Updated skill", "files bundled with it"}, + }, + { + name: "with a bundled script", + script: true, + want: []string{`Updated skill "pdf".`, "SKILL.md is unchanged; the files bundled with it are now installed too"}, + notWant: []string{"files bundled with it changed"}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + skillsDir := t.TempDir() + src := writeSourceSkillDir(t, filepath.Join(t.TempDir(), "src"), + "---\nname: pdf\ndescription: Fill forms.\n---\nRun scripts/fill.py.\n") + if tc.script { + script := filepath.Join(src, "scripts", "fill.py") + if err := os.MkdirAll(filepath.Dir(script), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(script, []byte("print('fill')\n"), 0o755); err != nil { + t.Fatal(err) + } + } + deps := appDeps{skillsDir: func() string { return skillsDir }} + + var stdout, stderr bytes.Buffer + if exit := runWithDeps([]string{"skill", "add", src}, &stdout, &stderr, deps); exit != 0 { + t.Fatalf("first skill add exit = %d, stderr = %s", exit, stderr.String()) + } + lock, err := skills.ReadLock(skillsDir) + if err != nil { + t.Fatal(err) + } + entry := lock["pdf"] + entry.TreeHash = "" + lock["pdf"] = entry + data, err := json.Marshal(lock) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(skillsDir, skills.LockFileName), data, 0o644); err != nil { + t.Fatal(err) + } + + stdout.Reset() + stderr.Reset() + if exit := runWithDeps([]string{"skill", "add", src}, &stdout, &stderr, deps); exit != 0 { + t.Fatalf("second skill add exit = %d, stderr = %s", exit, stderr.String()) + } + for _, want := range tc.want { + if !strings.Contains(stdout.String(), want) { + t.Fatalf("second add output does not say %q:\n%s", want, stdout.String()) + } + } + for _, notWant := range tc.notWant { + if strings.Contains(stdout.String(), notWant) { + t.Fatalf("second add output says %q:\n%s", notWant, stdout.String()) + } + } + }) + } +} + +func TestRunSkillAddListsTheSymlinksItDidNotCopy(t *testing.T) { + skillsDir := t.TempDir() + src := writeSourceSkillDir(t, filepath.Join(t.TempDir(), "src"), + "---\nname: pdf\ndescription: Fill forms.\n---\nbody\n") + outside := filepath.Join(t.TempDir(), "notes.md") + if err := os.WriteFile(outside, []byte("outside\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Symlink(outside, filepath.Join(src, "notes.md")); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + var stdout, stderr bytes.Buffer + deps := appDeps{skillsDir: func() string { return skillsDir }} + if exit := runWithDeps([]string{"skill", "add", src}, &stdout, &stderr, deps); exit != 0 { + t.Fatalf("skill add exit = %d, stderr = %s", exit, stderr.String()) + } + if !strings.Contains(stdout.String(), "not copied (symlinks): notes.md") { + t.Fatalf("add output does not list the skipped symlink:\n%s", stdout.String()) + } +} diff --git a/internal/plugins/activate.go b/internal/plugins/activate.go index 43dc66f65..2ad055159 100644 --- a/internal/plugins/activate.go +++ b/internal/plugins/activate.go @@ -374,7 +374,7 @@ func (tool skillTool) Run(_ context.Context, args map[string]any) tools.Result { available := make([]string, 0, len(merged)) for _, skill := range merged { if skill.Name == name { - return tools.Result{Status: tools.StatusOK, Output: skill.Content} + return tools.Result{Status: tools.StatusOK, Output: skills.ToolOutput(skill)} } available = append(available, skill.Name) } diff --git a/internal/plugins/activate_test.go b/internal/plugins/activate_test.go index cd788ecc3..b842cd5a6 100644 --- a/internal/plugins/activate_test.go +++ b/internal/plugins/activate_test.go @@ -673,3 +673,28 @@ func TestExpandPluginRootPathRelative(t *testing.T) { } } } + +// The multi-root skill tool is the one the CLI registers, so it has to point at +// a skill's bundled files the same way the core tool does (#584). +func TestNewSkillToolPointsAtTheSkillsBundledFiles(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + dir := t.TempDir() + writeTestSkill(t, dir, "pdf", "Run scripts/fill.py.") + script := filepath.Join(dir, "pdf", "scripts", "fill.py") + if err := os.MkdirAll(filepath.Dir(script), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(script, []byte("print('fill')\n"), 0o755); err != nil { + t.Fatal(err) + } + + got := NewSkillTool(dir, nil).Run(context.Background(), map[string]any{"name": "pdf"}) + if got.Status != tools.StatusOK { + t.Fatalf("Status = %s (output: %s)", got.Status, got.Output) + } + if !strings.Contains(got.Output, "- scripts/fill.py") || !strings.Contains(got.Output, filepath.Join(dir, "pdf")) { + t.Fatalf("output does not point at the bundled script:\n%s", got.Output) + } +} diff --git a/internal/skills/bundle.go b/internal/skills/bundle.go new file mode 100644 index 000000000..a894895ed --- /dev/null +++ b/internal/skills/bundle.go @@ -0,0 +1,258 @@ +package skills + +import ( + "crypto/sha256" + "encoding/hex" + "errors" + "fmt" + "io" + "io/fs" + "os" + "path/filepath" + "sort" + "strings" +) + +// A skill is a directory, not just its SKILL.md: the instructions often point at +// scripts, references or assets that sit beside them (the agentskills.io +// layout). Install copies that directory, and the skill tool says where it is. + +// Install refuses a skill tree larger than this. A skill is a handful of +// scripts and documents; a source that is really a whole repository or a pile +// of binaries should fail loudly instead of filling the skills directory. +var ( + maxSkillTreeFiles = 1000 + maxSkillTreeBytes = int64(32 << 20) +) + +// bundledFileListLimit caps how many bundled files the skill tool lists, so a +// large skill does not crowd the instructions out of the tool output. +const bundledFileListLimit = 50 + +// skipSkillEntry reports whether a skill tree entry is left out of an install, +// a tree hash and a listing alike. Hidden entries cover VCS metadata (.git, +// which a cloned source always has) and local clutter such as .DS_Store or +// .env; they are not part of what a skill ships. +func skipSkillEntry(name string) bool { + return strings.HasPrefix(name, ".") +} + +// copySkillTree copies the skill directory src into dst, which must exist. +// Regular files are copied with normalized permissions (0o755 when the source +// is executable, 0o644 otherwise), so a script stays runnable without carrying +// over group- or world-writable bits. Symlinks are not followed or copied: +// their relative paths come back in skipped, so the caller can say so. Other +// non-regular entries (devices, sockets, pipes) are left out. bundled counts +// the files copied besides SKILL.md. +func copySkillTree(src, dst string) (bundled int, skipped []string, err error) { + files := 0 + var total int64 + err = filepath.WalkDir(src, func(path string, entry fs.DirEntry, walkErr error) error { + if walkErr != nil { + return walkErr + } + if path == src { + return nil + } + rel, err := filepath.Rel(src, path) + if err != nil { + return err + } + if skipSkillEntry(entry.Name()) { + if entry.IsDir() { + return filepath.SkipDir + } + return nil + } + if entry.Type()&fs.ModeSymlink != 0 { + skipped = append(skipped, filepath.ToSlash(rel)) + return nil + } + target := filepath.Join(dst, rel) + if entry.IsDir() { + return os.MkdirAll(target, 0o755) + } + if !entry.Type().IsRegular() { + return nil + } + info, err := entry.Info() + if err != nil { + return err + } + files++ + total += info.Size() + if files > maxSkillTreeFiles { + return fmt.Errorf("skill has more than %d files; a skill directory should hold only what the skill uses", maxSkillTreeFiles) + } + if total > maxSkillTreeBytes { + return fmt.Errorf("skill is larger than %d MiB; a skill directory should hold only what the skill uses", maxSkillTreeBytes>>20) + } + if filepath.ToSlash(rel) != skillFileName { + bundled++ + } + mode := os.FileMode(0o644) + if info.Mode().Perm()&0o111 != 0 { + mode = 0o755 + } + return copySkillFile(path, target, mode) + }) + if err != nil { + return 0, nil, err + } + return bundled, skipped, nil +} + +func copySkillFile(src, dst string, mode os.FileMode) error { + in, err := os.Open(src) + if err != nil { + return err + } + defer in.Close() + out, err := os.OpenFile(dst, os.O_WRONLY|os.O_CREATE|os.O_EXCL, mode) + if err != nil { + return err + } + if _, err := io.Copy(out, in); err != nil { + _ = out.Close() + return err + } + if err := out.Close(); err != nil { + return err + } + // The create mode is filtered by the umask; set it outright so an + // executable script is still executable wherever the install ran. + return os.Chmod(dst, mode) +} + +// skillTreeHash hashes every file a skill ships, with the same entries +// copySkillTree copies: its relative path, whether it is executable, and its +// content. Sorted by path, so the hash depends only on the tree. +func skillTreeHash(dir string) (string, error) { + type entry struct { + rel string + exec bool + sum string + } + var entries []entry + err := filepath.WalkDir(dir, func(path string, item fs.DirEntry, walkErr error) error { + if walkErr != nil { + return walkErr + } + if path == dir { + return nil + } + if skipSkillEntry(item.Name()) { + if item.IsDir() { + return filepath.SkipDir + } + return nil + } + if !item.Type().IsRegular() { + return nil + } + info, err := item.Info() + if err != nil { + return err + } + data, err := os.ReadFile(path) + if err != nil { + return err + } + rel, err := filepath.Rel(dir, path) + if err != nil { + return err + } + sum := sha256.Sum256(data) + entries = append(entries, entry{rel: filepath.ToSlash(rel), exec: info.Mode().Perm()&0o111 != 0, sum: hex.EncodeToString(sum[:])}) + return nil + }) + if err != nil { + return "", err + } + sort.Slice(entries, func(i, j int) bool { return entries[i].rel < entries[j].rel }) + hash := sha256.New() + for _, item := range entries { + flag := "-" + if item.exec { + flag = "x" + } + // Length-prefixed, so no path can be read as the start of another entry. + fmt.Fprintf(hash, "%d:%s|%s|%s;", len(item.rel), item.rel, flag, item.sum) + } + return "sha256:" + hex.EncodeToString(hash.Sum(nil)), nil +} + +// BundledFiles lists the files a skill ships besides its SKILL.md, as +// slash-separated paths relative to the skill's directory, sorted. Hidden +// entries and symlinks are left out: the listing tells the model what the +// skill brings, and is not a way to reach anything outside it. total is the +// number of files found; at most limit come back. +func BundledFiles(skill Skill, limit int) (files []string, total int, err error) { + dir := filepath.Dir(skill.Path) + if strings.TrimSpace(skill.Path) == "" { + return nil, 0, errors.New("skill has no path") + } + err = filepath.WalkDir(dir, func(path string, entry fs.DirEntry, walkErr error) error { + if walkErr != nil { + return walkErr + } + if path == dir { + return nil + } + if skipSkillEntry(entry.Name()) { + if entry.IsDir() { + return filepath.SkipDir + } + return nil + } + if !entry.Type().IsRegular() { + return nil + } + rel, err := filepath.Rel(dir, path) + if err != nil { + return err + } + rel = filepath.ToSlash(rel) + if rel == skillFileName { + return nil + } + total++ + if total > maxSkillTreeFiles { + // A directory this large is not something the listing should walk + // to the end of; the count stops being exact past the cap. + return filepath.SkipAll + } + files = append(files, rel) + return nil + }) + if err != nil { + return nil, 0, err + } + sort.Strings(files) + if limit >= 0 && len(files) > limit { + files = files[:limit] + } + return files, total, nil +} + +// ToolOutput is what the skill tool returns for a loaded skill: its SKILL.md +// body and, when the skill ships other files, the directory they are in and +// what they are, so instructions that say "run scripts/extract.py" can be +// followed. A skill that is only a SKILL.md gets its body back unchanged. +func ToolOutput(skill Skill) string { + files, total, err := BundledFiles(skill, bundledFileListLimit) + if err != nil || total == 0 { + return skill.Content + } + var builder strings.Builder + builder.WriteString(strings.TrimRight(skill.Content, "\n")) + builder.WriteString("\n\n---\n") + fmt.Fprintf(&builder, "This skill's files are in %s. Paths in the instructions above are relative to that directory.\n", filepath.Dir(skill.Path)) + for _, file := range files { + builder.WriteString("- " + file + "\n") + } + if more := total - len(files); more > 0 { + fmt.Fprintf(&builder, "- (and %d more)\n", more) + } + return builder.String() +} diff --git a/internal/skills/bundle_test.go b/internal/skills/bundle_test.go new file mode 100644 index 000000000..c59496f75 --- /dev/null +++ b/internal/skills/bundle_test.go @@ -0,0 +1,351 @@ +package skills + +import ( + "context" + "encoding/json" + "fmt" + "os" + "path/filepath" + "runtime" + "strings" + "testing" +) + +const bundledSkillManifest = "---\nname: pdf\ndescription: fill PDF forms\n---\nRun scripts/fill.py with the form path.\n" + +// writeBundledSourceSkill lays down a skill in the agentskills.io shape: a +// SKILL.md plus the scripts, references and assets it points at, and the hidden +// clutter a cloned source carries. +func writeBundledSourceSkill(t *testing.T, dir string) string { + t.Helper() + writeSourceSkill(t, dir, bundledSkillManifest) + files := map[string]string{ + "scripts/fill.py": "print('fill')\n", + "references/form.md": "# Form fields\n", + "assets/logo.svg": "\n", + "LICENSE.txt": "MIT\n", + ".git/config": "[core]\n", + ".env": "TOKEN=local-only\n", + "scripts/.cache/tmp": "scratch\n", + } + for rel, content := range files { + path := filepath.Join(dir, filepath.FromSlash(rel)) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + mode := os.FileMode(0o644) + if strings.HasPrefix(rel, "scripts/") { + mode = 0o755 + } + if err := os.WriteFile(path, []byte(content), mode); err != nil { + t.Fatal(err) + } + } + return dir +} + +func TestInstallCopiesTheSkillDirectory(t *testing.T) { + destDir := t.TempDir() + src := writeBundledSourceSkill(t, filepath.Join(t.TempDir(), "pdf")) + + result, err := Install(context.Background(), InstallOptions{Source: src, Dir: destDir}) + if err != nil { + t.Fatalf("Install: %v", err) + } + installed := filepath.Join(destDir, "pdf") + for _, rel := range []string{"SKILL.md", "scripts/fill.py", "references/form.md", "assets/logo.svg", "LICENSE.txt"} { + if _, err := os.Stat(filepath.Join(installed, filepath.FromSlash(rel))); err != nil { + t.Errorf("%s was not installed: %v", rel, err) + } + } + for _, rel := range []string{".git", ".env", "scripts/.cache"} { + if _, err := os.Stat(filepath.Join(installed, filepath.FromSlash(rel))); err == nil { + t.Errorf("hidden %s was installed; it is not part of the skill", rel) + } + } + if !strings.HasPrefix(result.TreeHash, "sha256:") { + t.Fatalf("TreeHash = %q, want a sha256 tree hash", result.TreeHash) + } + lock, err := ReadLock(destDir) + if err != nil { + t.Fatal(err) + } + if lock["pdf"].TreeHash != result.TreeHash || lock["pdf"].Hash != result.Hash { + t.Fatalf("lock entry = %+v, want the install's hashes %q and %q", lock["pdf"], result.Hash, result.TreeHash) + } + if runtime.GOOS == "windows" { + return + } + script, err := os.Stat(filepath.Join(installed, "scripts", "fill.py")) + if err != nil { + t.Fatal(err) + } + if script.Mode().Perm() != 0o755 { + t.Errorf("installed script mode = %v, want 0755 so the skill can still run it", script.Mode().Perm()) + } + reference, err := os.Stat(filepath.Join(installed, "references", "form.md")) + if err != nil { + t.Fatal(err) + } + if reference.Mode().Perm() != 0o644 { + t.Errorf("installed reference mode = %v, want 0644", reference.Mode().Perm()) + } +} + +func TestInstallSkipsSymlinksAndSaysSo(t *testing.T) { + destDir := t.TempDir() + src := writeBundledSourceSkill(t, filepath.Join(t.TempDir(), "pdf")) + outside := filepath.Join(t.TempDir(), "secret.txt") + if err := os.WriteFile(outside, []byte("not part of the skill\n"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Symlink(outside, filepath.Join(src, "references", "linked.md")); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + result, err := Install(context.Background(), InstallOptions{Source: src, Dir: destDir}) + if err != nil { + t.Fatalf("Install: %v", err) + } + if len(result.SkippedLinks) != 1 || result.SkippedLinks[0] != "references/linked.md" { + t.Fatalf("SkippedLinks = %v, want [references/linked.md]", result.SkippedLinks) + } + if _, err := os.Lstat(filepath.Join(destDir, "pdf", "references", "linked.md")); err == nil { + t.Fatal("a symlink from the source was installed") + } +} + +func TestInstallRefusesAnOversizedSkill(t *testing.T) { + for _, tc := range []struct { + name string + files int + bytes int64 + want string + }{ + {name: "too many files", files: 3, bytes: maxSkillTreeBytes, want: "more than 3 files"}, + {name: "too many bytes", files: maxSkillTreeFiles, bytes: 1 << 20, want: "larger than 1 MiB"}, + } { + t.Run(tc.name, func(t *testing.T) { + files, bytes := maxSkillTreeFiles, maxSkillTreeBytes + t.Cleanup(func() { maxSkillTreeFiles, maxSkillTreeBytes = files, bytes }) + maxSkillTreeFiles, maxSkillTreeBytes = tc.files, tc.bytes + + destDir := t.TempDir() + src := writeBundledSourceSkill(t, filepath.Join(t.TempDir(), "pdf")) + big := make([]byte, 2<<20) + if err := os.WriteFile(filepath.Join(src, "assets", "big.bin"), big, 0o644); err != nil { + t.Fatal(err) + } + _, err := Install(context.Background(), InstallOptions{Source: src, Dir: destDir}) + if err == nil || !strings.Contains(err.Error(), tc.want) { + t.Fatalf("Install error = %v, want %q", err, tc.want) + } + if _, statErr := os.Stat(filepath.Join(destDir, "pdf")); statErr == nil { + t.Fatal("a refused install left a skill directory behind") + } + }) + } +} + +func TestReinstallWithOnlyABundledFileChangedIsAnUpdate(t *testing.T) { + destDir := t.TempDir() + src := writeBundledSourceSkill(t, filepath.Join(t.TempDir(), "pdf")) + first, err := Install(context.Background(), InstallOptions{Source: src, Dir: destDir}) + if err != nil { + t.Fatalf("first Install: %v", err) + } + if err := os.WriteFile(filepath.Join(src, "scripts", "fill.py"), []byte("print('fill v2')\n"), 0o755); err != nil { + t.Fatal(err) + } + + second, err := Install(context.Background(), InstallOptions{Source: src, Dir: destDir}) + if err != nil { + t.Fatalf("second Install: %v", err) + } + if !second.Updated { + t.Fatal("a reinstall that changed a bundled script was not reported as an update") + } + if second.Hash != first.Hash || second.TreeHash == first.TreeHash { + t.Fatalf("hashes = %q/%q after %q/%q, want the same SKILL.md hash and a new tree hash", second.Hash, second.TreeHash, first.Hash, first.TreeHash) + } + if second.PreviousHash != first.Hash || second.PreviousTreeHash != first.TreeHash { + t.Fatalf("previous hashes = %q/%q, want the first install's %q/%q", second.PreviousHash, second.PreviousTreeHash, first.Hash, first.TreeHash) + } + got, err := os.ReadFile(filepath.Join(destDir, "pdf", "scripts", "fill.py")) + if err != nil || string(got) != "print('fill v2')\n" { + t.Fatalf("installed script = %q, %v; want the new version", got, err) + } +} + +func TestInfoReportsDriftInABundledFile(t *testing.T) { + destDir := t.TempDir() + src := writeBundledSourceSkill(t, filepath.Join(t.TempDir(), "pdf")) + if _, err := Install(context.Background(), InstallOptions{Source: src, Dir: destDir}); err != nil { + t.Fatalf("Install: %v", err) + } + if info, ok := Info(destDir, "pdf"); !ok || info.HashDrift { + t.Fatalf("a fresh install reports drift: %+v, %v", info, ok) + } + script := filepath.Join(destDir, "pdf", "scripts", "fill.py") + if err := os.WriteFile(script, []byte("print('edited by hand')\n"), 0o755); err != nil { + t.Fatal(err) + } + if info, _ := Info(destDir, "pdf"); !info.HashDrift { + t.Fatal("an edited bundled script is not reported as drift") + } +} + +// A lock entry written before installs carried their bundled files has no tree +// hash. Checking its tree would report every such skill as drifted, so only +// its SKILL.md is checked, as before. +func TestLockEntryWithoutATreeHashChecksOnlySKILLmd(t *testing.T) { + destDir := t.TempDir() + src := writeBundledSourceSkill(t, filepath.Join(t.TempDir(), "pdf")) + if _, err := Install(context.Background(), InstallOptions{Source: src, Dir: destDir}); err != nil { + t.Fatalf("Install: %v", err) + } + dropTreeHash(t, destDir, "pdf") + + if err := os.WriteFile(filepath.Join(destDir, "pdf", "scripts", "fill.py"), []byte("print('edited')\n"), 0o755); err != nil { + t.Fatal(err) + } + if info, _ := Info(destDir, "pdf"); info.HashDrift { + t.Fatal("an entry without a tree hash reported drift for a bundled file") + } + if err := os.WriteFile(filepath.Join(destDir, "pdf", skillFileName), []byte(bundledSkillManifest+"edited\n"), 0o644); err != nil { + t.Fatal(err) + } + if info, _ := Info(destDir, "pdf"); !info.HashDrift { + t.Fatal("an edited SKILL.md under an old lock entry is not reported as drift") + } +} + +// Reinstalling over an entry from before tree hashes compares with what that +// install left, SKILL.md alone: an unchanged skill that is only a SKILL.md is +// not an update, and one whose bundled files this install adds is. +func TestReinstallOverAnEntryWithoutATreeHash(t *testing.T) { + t.Run("SKILL.md only", func(t *testing.T) { + destDir := t.TempDir() + src := writeSourceSkill(t, filepath.Join(t.TempDir(), "pdf"), bundledSkillManifest) + if _, err := Install(context.Background(), InstallOptions{Source: src, Dir: destDir}); err != nil { + t.Fatalf("first Install: %v", err) + } + dropTreeHash(t, destDir, "pdf") + + result, err := Install(context.Background(), InstallOptions{Source: src, Dir: destDir}) + if err != nil { + t.Fatalf("second Install: %v", err) + } + if result.Updated { + t.Fatalf("reinstalling an unchanged SKILL.md-only skill over an old entry was reported as an update: %+v", result) + } + lock, err := ReadLock(destDir) + if err != nil { + t.Fatal(err) + } + if lock["pdf"].TreeHash != result.TreeHash { + t.Fatalf("lock tree hash = %q after the reinstall, want %q", lock["pdf"].TreeHash, result.TreeHash) + } + }) + t.Run("with bundled files", func(t *testing.T) { + destDir := t.TempDir() + src := writeBundledSourceSkill(t, filepath.Join(t.TempDir(), "pdf")) + first, err := Install(context.Background(), InstallOptions{Source: src, Dir: destDir}) + if err != nil { + t.Fatalf("first Install: %v", err) + } + dropTreeHash(t, destDir, "pdf") + + result, err := Install(context.Background(), InstallOptions{Source: src, Dir: destDir}) + if err != nil { + t.Fatalf("second Install: %v", err) + } + if !result.Updated || result.Hash != first.Hash || result.PreviousHash != first.Hash || result.PreviousTreeHash != "" { + t.Fatalf("result = %+v, want an update with SKILL.md hash %q unchanged and no previous tree hash", result, first.Hash) + } + }) +} + +// dropTreeHash rewrites the lock entry for name the way an install from before +// tree hashes left it: a source and a SKILL.md hash, nothing for the tree. +func dropTreeHash(t *testing.T, destDir, name string) { + t.Helper() + lock, err := ReadLock(destDir) + if err != nil { + t.Fatal(err) + } + entry := lock[name] + entry.TreeHash = "" + lock[name] = entry + data, err := json.Marshal(lock) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(destDir, LockFileName), data, 0o644); err != nil { + t.Fatal(err) + } +} + +func TestToolOutputSaysWhereTheBundledFilesAre(t *testing.T) { + root := t.TempDir() + writeBundledSourceSkill(t, filepath.Join(root, "pdf")) + skill, ok := Get(root, "pdf") + if !ok { + t.Fatal("skill not loaded") + } + + output := ToolOutput(skill) + if !strings.HasPrefix(output, strings.TrimRight(skill.Content, "\n")) { + t.Fatalf("the instructions do not come first:\n%s", output) + } + if !strings.Contains(output, filepath.Dir(skill.Path)) { + t.Fatalf("the output does not say where the skill's files are:\n%s", output) + } + for _, want := range []string{"- LICENSE.txt", "- assets/logo.svg", "- references/form.md", "- scripts/fill.py"} { + if !strings.Contains(output, want+"\n") { + t.Errorf("output does not list %q:\n%s", want, output) + } + } + for _, hidden := range []string{".env", ".git", ".cache", "SKILL.md\n"} { + if strings.Contains(output, "- "+hidden) { + t.Errorf("output lists %q:\n%s", hidden, output) + } + } +} + +func TestToolOutputCapsALongListing(t *testing.T) { + root := t.TempDir() + dir := writeSourceSkill(t, filepath.Join(root, "many"), "---\nname: many\n---\nbody\n") + for i := 0; i < bundledFileListLimit+7; i++ { + path := filepath.Join(dir, "references", fmt.Sprintf("part-%03d.md", i)) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte("x\n"), 0o644); err != nil { + t.Fatal(err) + } + } + skill, ok := Get(root, "many") + if !ok { + t.Fatal("skill not loaded") + } + output := ToolOutput(skill) + if got := strings.Count(output, "\n- references/"); got != bundledFileListLimit { + t.Fatalf("listed %d files, want the first %d:\n%s", got, bundledFileListLimit, output) + } + if !strings.Contains(output, "- (and 7 more)\n") { + t.Fatalf("output does not say how many more there are:\n%s", output) + } +} + +func TestToolOutputOfAManifestOnlySkillIsItsContent(t *testing.T) { + root := t.TempDir() + writeSourceSkill(t, filepath.Join(root, "plain"), "---\nname: plain\n---\nJust instructions.\n") + skill, ok := Get(root, "plain") + if !ok { + t.Fatal("skill not loaded") + } + if got := ToolOutput(skill); got != skill.Content { + t.Fatalf("ToolOutput = %q, want the unchanged content %q", got, skill.Content) + } +} diff --git a/internal/skills/export_test.go b/internal/skills/export_test.go index 7a07b8a5c..468019a80 100644 --- a/internal/skills/export_test.go +++ b/internal/skills/export_test.go @@ -15,9 +15,10 @@ func Info(dir string, name string) (SkillInfo, bool) { if entry, found := lock[skill.Name]; found { info.Source = entry.Source info.Hash = entry.Hash + info.TreeHash = entry.TreeHash } } - info.HashDrift = skillHashDrift(skill, info.Hash) + info.HashDrift = skillHashDrift(skill, info.Hash, info.TreeHash) return info, true } diff --git a/internal/skills/install.go b/internal/skills/install.go index 31eae9e97..4417775d9 100644 --- a/internal/skills/install.go +++ b/internal/skills/install.go @@ -1,9 +1,9 @@ package skills // Distribution: install a skill from a git URL or a local path into the skills -// directory, with a content hash recorded in a lockfile (skills.lock) so every -// install/update is verifiable. Skills are markdown, so install NEVER executes -// fetched content — it copies and validates the SKILL.md and nothing else. +// directory, with content hashes recorded in a lockfile (skills.lock) so every +// install/update is verifiable. Install copies the skill's directory, its +// SKILL.md and the files beside it, and NEVER executes fetched content. import ( "context" @@ -61,16 +61,30 @@ type InstallResult struct { Hash string `json:"hash"` // Source echoes the source the skill was installed from. Source string `json:"source"` + // TreeHash covers every file the skill ships, SKILL.md included; Hash is + // still the SKILL.md hash alone. + TreeHash string `json:"treeHash"` // Updated is true when an existing install was replaced; PreviousHash then // carries the prior recorded hash so a caller can show the change. Updated bool `json:"updated"` PreviousHash string `json:"previousHash,omitempty"` + // PreviousTreeHash is the tree hash the replaced install recorded. It is + // empty when that install predates tree hashes, and so copied SKILL.md + // alone. + PreviousTreeHash string `json:"previousTreeHash,omitempty"` + // SkippedLinks lists symlinks in the source, relative to the skill + // directory. Install neither follows nor copies them. + SkippedLinks []string `json:"skippedLinks,omitempty"` } // LockEntry records the source and content hash for one installed skill. type LockEntry struct { Source string `json:"source"` Hash string `json:"hash"` + // TreeHash covers the whole installed skill directory. Entries written + // before skills installed their bundled files have none, and are checked on + // their SKILL.md alone. + TreeHash string `json:"treeHash,omitempty"` } // SkillInfo bundles a discovered skill with its recorded source and hash, for @@ -79,12 +93,14 @@ type SkillInfo struct { Skill Skill `json:"skill"` Source string `json:"source,omitempty"` Hash string `json:"hash,omitempty"` + TreeHash string `json:"treeHash,omitempty"` HashDrift bool `json:"hashDrift"` } -// Install fetches the skill at options.Source and copies its SKILL.md into -// options.Dir//, validating the frontmatter and recording a content hash -// in the lockfile. A git URL is fetched via the (injectable) GitRunner into a +// Install fetches the skill at options.Source and copies its directory into +// options.Dir//: SKILL.md and the scripts, references and assets beside +// it. It validates the frontmatter and records the SKILL.md hash and a hash of +// the whole tree in the lockfile. A git URL is fetched via the (injectable) GitRunner into a // temp dir; a local path is read in place. Fetched content is never executed. func Install(ctx context.Context, options InstallOptions) (InstallResult, error) { source := strings.TrimSpace(options.Source) @@ -133,10 +149,11 @@ func Install(ctx context.Context, options InstallOptions) (InstallResult, error) if err := os.MkdirAll(staged, 0o755); err != nil { return InstallResult{}, fmt.Errorf("create staged skill dir: %w", err) } - stagedManifest := filepath.Join(staged, skillFileName) - if err := os.WriteFile(stagedManifest, data, 0o644); err != nil { - return InstallResult{}, fmt.Errorf("stage SKILL.md: %w", err) + bundled, skippedLinks, err := copySkillTree(skillDir, staged) + if err != nil { + return InstallResult{}, fmt.Errorf("stage skill directory: %w", err) } + stagedManifest := filepath.Join(staged, skillFileName) stagedData, err := os.ReadFile(stagedManifest) if err != nil || hashContent(stagedData) != hash { if err != nil { @@ -169,8 +186,13 @@ func Install(ctx context.Context, options InstallOptions) (InstallResult, error) return InstallResult{}, fmt.Errorf("%w: %q is installed from %s (use --force to overwrite)", ErrNameClash, name, previous.Source) } + treeHash, err := skillTreeHash(staged) + if err != nil { + return InstallResult{}, fmt.Errorf("hash staged skill directory: %w", err) + } + target := filepath.Join(dir, name) - lock[name] = LockEntry{Source: source, Hash: hash} + lock[name] = LockEntry{Source: source, Hash: hash, TreeHash: treeHash} if err := installtxn.CommitDir(target, staged, func() error { return writeLock(dir, lock) }); err != nil { @@ -178,14 +200,26 @@ func Install(ctx context.Context, options InstallOptions) (InstallResult, error) } result := InstallResult{ - Name: name, - Path: filepath.Join(target, skillFileName), - Hash: hash, - Source: source, + Name: name, + Path: filepath.Join(target, skillFileName), + Hash: hash, + TreeHash: treeHash, + Source: source, + SkippedLinks: skippedLinks, } if existed { - result.Updated = previous.Hash != hash result.PreviousHash = previous.Hash + result.PreviousTreeHash = previous.TreeHash + if previous.TreeHash == "" { + // An entry from before tree hashes is from an install that copied + // SKILL.md alone. Against that, this install is an update when + // SKILL.md changed or it now brings bundled files; a skill that is + // still only its unchanged SKILL.md is not. + result.Updated = previous.Hash != hash || bundled > 0 + } else { + // A changed script with the same SKILL.md is still an update. + result.Updated = previous.Hash != hash || previous.TreeHash != treeHash + } } return result, nil } @@ -193,8 +227,8 @@ func Install(ctx context.Context, options InstallOptions) (InstallResult, error) // lockReconciler answers, for one interrupted workspace, which of the two trees // beside each other the lockfile records. A kill between the tree swap and the // lockfile publish leaves exactly what a kill after both leaves, so filesystem -// shape cannot tell them apart and only the recorded hash can: the tree it -// describes is the one the user was told they had. +// shape cannot tell them apart and only the recorded hashes can: the tree they +// describe is the one the user was told they had. func lockReconciler(dir string) installtxn.Reconciler { return func(name string, target string, backup string) (installtxn.Phase, error) { lock, err := ReadLock(dir) @@ -215,24 +249,45 @@ func lockReconciler(dir string) installtxn.Reconciler { // recorded one and recovery must not pick between them. return installtxn.PhaseUnknown, nil } - targetHash, err := manifestHash(target) + committed, err := treeMatchesEntry(target, entry) if err != nil { return installtxn.PhaseUnknown, err } - if targetHash == entry.Hash { + if committed { return installtxn.PhaseCommitted, nil } - backupHash, err := manifestHash(backup) + prePublish, err := treeMatchesEntry(backup, entry) if err != nil { return installtxn.PhaseUnknown, err } - if backupHash == entry.Hash { + if prePublish { return installtxn.PhasePrePublish, nil } return installtxn.PhaseUnknown, nil } } +// treeMatchesEntry reports whether the skill tree at path is the one entry +// records. A reinstall that changes only a bundled file leaves SKILL.md the same +// in both trees of an interrupted workspace, so an entry with a tree hash has to +// match on that as well. An entry from before tree hashes records SKILL.md +// alone, and that is all it can be matched on. As in manifestHash, a tree with +// no SKILL.md is a non-match and any other read error is returned. +func treeMatchesEntry(path string, entry LockEntry) (bool, error) { + sum, err := manifestHash(path) + if err != nil || sum != entry.Hash { + return false, err + } + if entry.TreeHash == "" { + return true, nil + } + tree, err := skillTreeHash(path) + if err != nil { + return false, err + } + return tree == entry.TreeHash, nil +} + // manifestHash hashes the SKILL.md under path the same way an install records // it. A tree with no SKILL.md is a non-match rather than a failure: an install // killed mid-copy can leave one, and it simply is not the tree the lockfile @@ -346,9 +401,10 @@ func InfoFromRoots(primaryDir string, roots []string, name string) (SkillInfo, b if entry, found := lock[skill.Name]; found { info.Source = entry.Source info.Hash = entry.Hash + info.TreeHash = entry.TreeHash } } - info.HashDrift = skillHashDrift(skill, info.Hash) + info.HashDrift = skillHashDrift(skill, info.Hash, info.TreeHash) return info, true } @@ -556,11 +612,12 @@ func hashContent(data []byte) string { return "sha256:" + hex.EncodeToString(sum[:]) } -// skillHashDrift reports whether the on-disk SKILL.md no longer matches the -// lockfile hash recorded at install time. Missing lock hashes never count as -// drift (agents-only / unlocked skills). A locked skill whose SKILL.md cannot -// be read is treated as drifted — not as a clean match. -func skillHashDrift(skill Skill, lockHash string) bool { +// skillHashDrift reports whether the installed skill no longer matches the +// lockfile hashes recorded at install time: the SKILL.md hash, and the tree +// hash when the entry has one, so an edited script counts as drift too. Missing +// lock hashes never count as drift (skills Zero did not install). A locked +// skill that cannot be read is treated as drifted, not as a clean match. +func skillHashDrift(skill Skill, lockHash string, lockTreeHash string) bool { lockHash = strings.TrimSpace(lockHash) if lockHash == "" { return false @@ -569,5 +626,13 @@ func skillHashDrift(skill Skill, lockHash string) bool { if err != nil { return true } - return hashContent(data) != lockHash + if hashContent(data) != lockHash { + return true + } + lockTreeHash = strings.TrimSpace(lockTreeHash) + if lockTreeHash == "" { + return false + } + treeHash, err := skillTreeHash(filepath.Dir(skill.Path)) + return err != nil || treeHash != lockTreeHash } diff --git a/internal/skills/install_test.go b/internal/skills/install_test.go index 4799b1703..604ae9bc2 100644 --- a/internal/skills/install_test.go +++ b/internal/skills/install_test.go @@ -364,10 +364,11 @@ func TestInstallGitSourceUsesRunnerAndDoesNotExecuteContent(t *testing.T) { if _, err := os.Stat(filepath.Join(cloneRoot, "PWNED")); err == nil { t.Fatalf("install must never execute fetched content") } - // The fetched executable must not be copied into the skills dir either (skills - // are markdown — only SKILL.md is installed, not arbitrary fetched files). - if _, err := os.Stat(filepath.Join(destDir, "remote", "install.sh")); err == nil { - t.Fatalf("install must not copy fetched executable into the skills dir") + // A script beside SKILL.md is part of the skill (#584), so it is installed, + // and still only copied: running it is up to whoever uses the skill later, + // through the normal tool permissions. + if _, err := os.Stat(filepath.Join(destDir, "remote", "install.sh")); err != nil { + t.Fatalf("the skill's bundled script was not installed: %v", err) } } @@ -440,10 +441,10 @@ func TestInfoReturnsFrontmatterSourceAndHash(t *testing.T) { func TestSkillHashDriftUnreadableLockedPath(t *testing.T) { missing := filepath.Join(t.TempDir(), "missing", "SKILL.md") - if !skillHashDrift(Skill{Path: missing}, "sha256:deadbeef") { + if !skillHashDrift(Skill{Path: missing}, "sha256:deadbeef", "") { t.Fatal("expected drift when locked SKILL.md cannot be read") } - if skillHashDrift(Skill{Path: missing}, "") { + if skillHashDrift(Skill{Path: missing}, "", "") { t.Fatal("missing lock hash must not count as drift") } } @@ -861,6 +862,146 @@ func TestRecoveryResolvesEveryInterruptedUpdateStep(t *testing.T) { } } +const ( + bundledScriptV1 = "print('v1')\n" + bundledScriptV2 = "print('v2')\n" +) + +func writeBundledScript(t *testing.T, skillDir string, content string) string { + t.Helper() + path := filepath.Join(skillDir, "scripts", "run.py") + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(content), 0o755); err != nil { + t.Fatal(err) + } + return path +} + +// plantInterruptedBundledUpdate is plantInterruptedAlphaUpdate for a reinstall +// that changes only a bundled script, killed after the tree swap: alpha's +// SKILL.md is the same in the live tree and the backup, and only the tree hash +// in the lockfile tells them apart. published says whether the lockfile was +// rewritten for the new tree before the kill (S4) or not (S3). +func plantInterruptedBundledUpdate(t *testing.T, published bool) interruptedUpdate { + t.Helper() + dir := t.TempDir() + source := writeSourceSkill(t, filepath.Join(t.TempDir(), "alpha"), oldAlphaSkill) + writeBundledScript(t, source, bundledScriptV1) + if _, err := Install(context.Background(), InstallOptions{Source: source, Dir: dir}); err != nil { + t.Fatalf("seed alpha: %v", err) + } + gammaSource := writeSourceSkill(t, filepath.Join(t.TempDir(), "gamma"), gammaSkill) + if _, err := Install(context.Background(), InstallOptions{Source: gammaSource, Dir: dir}); err != nil { + t.Fatalf("seed gamma: %v", err) + } + + staged, _, err := installtxn.StageDir(dir) + if err != nil { + t.Fatal(err) + } + workspace := filepath.Dir(staged) + writeWorkspaceMarker(t, workspace, "alpha") + writeSourceSkill(t, staged, oldAlphaSkill) + writeBundledScript(t, staged, bundledScriptV2) + + target := filepath.Join(dir, "alpha") + if err := os.Rename(target, filepath.Join(workspace, "previous")); err != nil { + t.Fatal(err) + } + if err := os.Rename(staged, target); err != nil { + t.Fatal(err) + } + if published { + treeHash, err := skillTreeHash(target) + if err != nil { + t.Fatal(err) + } + publishLockEntry(t, dir, "alpha", LockEntry{Source: canonicalSource(source), Hash: hashContent([]byte(oldAlphaSkill)), TreeHash: treeHash}) + } + return interruptedUpdate{dir: dir, workspace: workspace, oldSource: canonicalSource(source), newSource: canonicalSource(source)} +} + +// SKILL.md cannot say which tree of an interrupted bundled-only update the +// lockfile records, since both carry the same one. The tree hash can: before the +// publish it still describes the backup, after it the live tree. +func TestRecoveryTellsABundledOnlyUpdateApartByTreeHash(t *testing.T) { + steps := []struct { + step string + published bool + want string + }{ + {step: "S3", published: false, want: bundledScriptV1}, + {step: "S4", published: true, want: bundledScriptV2}, + } + for _, step := range steps { + for _, driver := range recoveryDrivers { + t.Run(step.step+"/"+driver.name, func(t *testing.T) { + update := plantInterruptedBundledUpdate(t, step.published) + + if err := driver.run(t, update.dir); err != nil { + t.Fatalf("%s over a recoverable state: %v", driver.name, err) + } + + target := filepath.Join(update.dir, "alpha") + got, err := os.ReadFile(filepath.Join(target, "scripts", "run.py")) + if err != nil || string(got) != step.want { + t.Fatalf("recovered script = %q, %v; want %q", got, err, step.want) + } + lock, err := ReadLock(update.dir) + if err != nil { + t.Fatal(err) + } + tree, err := skillTreeHash(target) + if err != nil { + t.Fatal(err) + } + if lock["alpha"].TreeHash != tree { + t.Errorf("lock tree hash %q does not describe the recovered tree %q", lock["alpha"].TreeHash, tree) + } + if _, err := os.Stat(update.workspace); !os.IsNotExist(err) { + t.Errorf("a resolved workspace must be cleared, got %v", err) + } + }) + } + } +} + +// A live tree whose files cannot be read cannot be shown to be the one the +// lockfile records, or not to be. Recovery reports it, as it does for a SKILL.md +// it cannot read, and decides nothing until a later pass can read it. +func TestRecoveryReportsALiveTreeItCannotHash(t *testing.T) { + for _, driver := range recoveryDrivers { + t.Run(driver.name, func(t *testing.T) { + update := plantInterruptedBundledUpdate(t, false) + lockBefore := readLockBytes(t, update.dir) + dirBefore := treeSnapshot(t, update.dir) + release := holdUnreadable(t, filepath.Join(update.dir, "alpha", "scripts", "run.py")) + + err := driver.run(t, update.dir) + release() + if err == nil || !strings.Contains(err.Error(), "classify interrupted install alpha") { + t.Fatalf("%s over a live tree it cannot hash: %v, want the classification error", driver.name, err) + } + assertSnapshotUnchanged(t, dirBefore, treeSnapshot(t, update.dir)) + if got := readLockBytes(t, update.dir); got != lockBefore { + t.Errorf("the lockfile was rewritten by a caller that must have aborted:\n%s", got) + } + + // Once the tree can be read, the next pass resolves it: the lockfile + // still describes the backup, so the backup comes back. + if err := driver.run(t, update.dir); err != nil { + t.Fatalf("%s once the tree is readable: %v", driver.name, err) + } + got, err := os.ReadFile(filepath.Join(update.dir, "alpha", "scripts", "run.py")) + if err != nil || string(got) != bundledScriptV1 { + t.Fatalf("recovered script = %q, %v; want %q", got, err, bundledScriptV1) + } + }) + } +} + // A recovery that could not be made is not the same as nothing to recover. // Installing over or reporting the removal of a tree that is still owed a // restore destroys the only copy of it, so the caller has to stop before it diff --git a/internal/skills/unreadable_posix_test.go b/internal/skills/unreadable_posix_test.go new file mode 100644 index 000000000..ad50b8aa1 --- /dev/null +++ b/internal/skills/unreadable_posix_test.go @@ -0,0 +1,30 @@ +//go:build !windows + +package skills + +import ( + "os" + "sync" + "testing" +) + +// holdUnreadable takes every permission off path, so reading it fails until +// release puts them back. Cleanup releases it too. Root reads a file whatever +// its mode, so a test that needs the read to fail is skipped there. +func holdUnreadable(t *testing.T, path string) (release func()) { + t.Helper() + if os.Geteuid() == 0 { + t.Skip("root reads a file whatever its mode") + } + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if err := os.Chmod(path, 0); err != nil { + t.Fatal(err) + } + var once sync.Once + release = func() { once.Do(func() { _ = os.Chmod(path, info.Mode().Perm()) }) } + t.Cleanup(release) + return release +} diff --git a/internal/skills/unreadable_windows_test.go b/internal/skills/unreadable_windows_test.go new file mode 100644 index 000000000..a7d7426a5 --- /dev/null +++ b/internal/skills/unreadable_windows_test.go @@ -0,0 +1,28 @@ +//go:build windows + +package skills + +import ( + "sync" + "testing" + + "golang.org/x/sys/windows" +) + +// holdUnreadable keeps path open with no sharing, so every other open of it +// fails until release runs. Cleanup releases it too. +func holdUnreadable(t *testing.T, path string) (release func()) { + t.Helper() + name, err := windows.UTF16PtrFromString(path) + if err != nil { + t.Fatal(err) + } + handle, err := windows.CreateFile(name, windows.GENERIC_READ, 0, nil, windows.OPEN_EXISTING, windows.FILE_ATTRIBUTE_NORMAL, 0) + if err != nil { + t.Fatalf("hold %s open: %v", path, err) + } + var once sync.Once + release = func() { once.Do(func() { _ = windows.CloseHandle(handle) }) } + t.Cleanup(release) + return release +} diff --git a/internal/tools/skill.go b/internal/tools/skill.go index 366c4d84c..040d9e764 100644 --- a/internal/tools/skill.go +++ b/internal/tools/skill.go @@ -73,7 +73,7 @@ func (tool *skillTool) Run(_ context.Context, args map[string]any) Result { names := make([]string, 0, len(loaded)) for _, skill := range loaded { if skill.Name == name { - return okResult(skill.Content) + return okResult(skills.ToolOutput(skill)) } names = append(names, skill.Name) } diff --git a/internal/tools/skill_test.go b/internal/tools/skill_test.go index 3279e4203..3d2b79b50 100644 --- a/internal/tools/skill_test.go +++ b/internal/tools/skill_test.go @@ -96,3 +96,27 @@ func TestSkillToolNoSkillsAvailable(t *testing.T) { t.Fatalf("expected a no-skills message, got: %q", result.Output) } } + +// A skill that ships files beside its SKILL.md (#584) is no use if the model +// cannot find them, so the output names their directory and lists them. +func TestSkillToolPointsAtTheSkillsBundledFiles(t *testing.T) { + dir := t.TempDir() + writeSkillFile(t, dir, "pdf", "---\nname: pdf\n---\nRun scripts/fill.py.") + script := filepath.Join(dir, "pdf", "scripts", "fill.py") + if err := os.MkdirAll(filepath.Dir(script), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(script, []byte("print('fill')\n"), 0o755); err != nil { + t.Fatal(err) + } + + result := NewSkillTool(dir).Run(context.Background(), map[string]any{"name": "pdf"}) + if result.Status != StatusOK { + t.Fatalf("Status = %s (output: %s)", result.Status, result.Output) + } + for _, want := range []string{"Run scripts/fill.py.", filepath.Join(dir, "pdf"), "- scripts/fill.py"} { + if !strings.Contains(result.Output, want) { + t.Fatalf("output does not contain %q:\n%s", want, result.Output) + } + } +}