From 2aeb796fae70ad9c70a6de3f2c1108f404d3afce Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Tue, 20 Oct 2026 19:38:05 +0530 Subject: [PATCH 1/3] feat(skills): install a skill's bundled files and point the skill tool at them A skill was only its SKILL.md: install copied that one file, and the skill tool returned only the markdown body, so a skill whose instructions run its own scripts was broken by installing it, and a hand-placed skill directory gave the model no way to find the files beside it. Install now copies the whole skill directory, leaving out hidden entries (.git, .env) and symlinks, which it neither follows nor copies but reports. A script stays executable; everything else is installed 0644. A tree over 1,000 files or 32 MiB is refused. skills.lock records a treeHash beside the SKILL.md hash; skill info reports drift in bundled files, and an entry from before this is still checked on SKILL.md alone. A reinstall that changed only a bundled file is reported as an update. Both skill tools end their output with the skill's directory and a list of up to 50 bundled files when there are any; a SKILL.md-only skill is returned unchanged. Closes #584 --- docs/EXTENDING.md | 11 ++ internal/cli/distribution.go | 9 +- internal/cli/distribution_test.go | 55 ++++++ internal/plugins/activate.go | 2 +- internal/plugins/activate_test.go | 25 +++ internal/skills/bundle.go | 254 +++++++++++++++++++++++++ internal/skills/bundle_test.go | 295 ++++++++++++++++++++++++++++++ internal/skills/export_test.go | 3 +- internal/skills/install.go | 71 +++++-- internal/skills/install_test.go | 13 +- internal/tools/skill.go | 2 +- internal/tools/skill_test.go | 24 +++ 12 files changed, 735 insertions(+), 29 deletions(-) create mode 100644 internal/skills/bundle.go create mode 100644 internal/skills/bundle_test.go 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..88dc16a7b 100644 --- a/internal/cli/distribution.go +++ b/internal/cli/distribution.go @@ -92,13 +92,20 @@ 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))) + if result.PreviousHash != result.Hash { + lines = append(lines, fmt.Sprintf(" hash: %s -> %s", shortHash(result.PreviousHash), shortHash(result.Hash))) + } else { + 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..95e2524e9 100644 --- a/internal/cli/distribution_test.go +++ b/internal/cli/distribution_test.go @@ -391,3 +391,58 @@ 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()) + } + } +} + +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..2e3eb9509 --- /dev/null +++ b/internal/skills/bundle.go @@ -0,0 +1,254 @@ +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. +func copySkillTree(src, dst string) (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) + } + mode := os.FileMode(0o644) + if info.Mode().Perm()&0o111 != 0 { + mode = 0o755 + } + return copySkillFile(path, target, mode) + }) + if err != nil { + return nil, err + } + return 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..fc368a491 --- /dev/null +++ b/internal/skills/bundle_test.go @@ -0,0 +1,295 @@ +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) + } + 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) + } + lock, err := ReadLock(destDir) + 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(destDir, LockFileName), data, 0o644); err != nil { + t.Fatal(err) + } + + 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") + } +} + +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..1e752e54d 100644 --- a/internal/skills/install.go +++ b/internal/skills/install.go @@ -61,16 +61,26 @@ 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"` + // 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 +89,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 +145,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) + 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 +182,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,13 +196,18 @@ 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 + // A changed script with the same SKILL.md is still an update. An entry + // from before tree hashes counts as one too: the reinstall is the first + // that puts the bundled files in place. + result.Updated = previous.Hash != hash || previous.TreeHash != treeHash result.PreviousHash = previous.Hash } return result, nil @@ -346,9 +369,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 +580,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 +594,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..142bb764b 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") } } 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) + } + } +} From 9a2c8fb90e9e6e4411a2691d46a7e90b33375176 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Tue, 20 Oct 2026 20:10:21 +0530 Subject: [PATCH 2/3] fix(skills): compare a reinstall over an old lock entry with SKILL.md alone A lock entry written before tree hashes has no treeHash, so the reinstall compared the new tree hash with an empty string and always counted as an update. Re-adding an unchanged skill that is only a SKILL.md printed "Updated" and "the files bundled with it changed" when there were none. That entry comes from an install that copied SKILL.md alone, so compare with that: it is an update when SKILL.md changed or the reinstall now brings bundled files. InstallResult gains previousTreeHash, and the CLI uses it to say the bundled files are now installed rather than changed on the first reinstall over such an entry. --- internal/cli/distribution.go | 9 +++- internal/cli/distribution_test.go | 77 ++++++++++++++++++++++++++++ internal/skills/bundle.go | 12 +++-- internal/skills/bundle_test.go | 84 +++++++++++++++++++++++++------ internal/skills/install.go | 21 ++++++-- 5 files changed, 178 insertions(+), 25 deletions(-) diff --git a/internal/cli/distribution.go b/internal/cli/distribution.go index 88dc16a7b..f31e94867 100644 --- a/internal/cli/distribution.go +++ b/internal/cli/distribution.go @@ -92,9 +92,14 @@ 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)) - if result.PreviousHash != result.Hash { + switch { + case result.PreviousHash != result.Hash: lines = append(lines, fmt.Sprintf(" hash: %s -> %s", shortHash(result.PreviousHash), shortHash(result.Hash))) - } else { + 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 { diff --git a/internal/cli/distribution_test.go b/internal/cli/distribution_test.go index 95e2524e9..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 { @@ -426,6 +428,81 @@ func TestRunSkillAddReportsAnUpdateToBundledFilesOnly(t *testing.T) { } } +// 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"), diff --git a/internal/skills/bundle.go b/internal/skills/bundle.go index 2e3eb9509..a894895ed 100644 --- a/internal/skills/bundle.go +++ b/internal/skills/bundle.go @@ -42,8 +42,9 @@ func skipSkillEntry(name string) bool { // 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. -func copySkillTree(src, dst string) (skipped []string, err error) { +// 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 { @@ -86,6 +87,9 @@ func copySkillTree(src, dst string) (skipped []string, err error) { 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 @@ -93,9 +97,9 @@ func copySkillTree(src, dst string) (skipped []string, err error) { return copySkillFile(path, target, mode) }) if err != nil { - return nil, err + return 0, nil, err } - return skipped, nil + return bundled, skipped, nil } func copySkillFile(src, dst string, mode os.FileMode) error { diff --git a/internal/skills/bundle_test.go b/internal/skills/bundle_test.go index fc368a491..c59496f75 100644 --- a/internal/skills/bundle_test.go +++ b/internal/skills/bundle_test.go @@ -168,6 +168,9 @@ func TestReinstallWithOnlyABundledFileChangedIsAnUpdate(t *testing.T) { 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) @@ -201,20 +204,7 @@ func TestLockEntryWithoutATreeHashChecksOnlySKILLmd(t *testing.T) { if _, err := Install(context.Background(), InstallOptions{Source: src, Dir: destDir}); err != nil { t.Fatalf("Install: %v", err) } - lock, err := ReadLock(destDir) - 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(destDir, LockFileName), data, 0o644); err != nil { - t.Fatal(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) @@ -230,6 +220,72 @@ func TestLockEntryWithoutATreeHashChecksOnlySKILLmd(t *testing.T) { } } +// 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")) diff --git a/internal/skills/install.go b/internal/skills/install.go index 1e752e54d..e07916465 100644 --- a/internal/skills/install.go +++ b/internal/skills/install.go @@ -68,6 +68,10 @@ type InstallResult struct { // 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"` @@ -145,7 +149,7 @@ 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) } - skippedLinks, err := copySkillTree(skillDir, staged) + bundled, skippedLinks, err := copySkillTree(skillDir, staged) if err != nil { return InstallResult{}, fmt.Errorf("stage skill directory: %w", err) } @@ -204,11 +208,18 @@ func Install(ctx context.Context, options InstallOptions) (InstallResult, error) SkippedLinks: skippedLinks, } if existed { - // A changed script with the same SKILL.md is still an update. An entry - // from before tree hashes counts as one too: the reinstall is the first - // that puts the bundled files in place. - result.Updated = previous.Hash != hash || previous.TreeHash != treeHash 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 } From e6ebe1da33d8bbbfe401cb62f418dc196ad19dbf Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Wed, 21 Oct 2026 12:14:08 +0530 Subject: [PATCH 3/3] fix(skills): match the tree hash when recovering an interrupted install lockReconciler matched both trees of an interrupted workspace on the SKILL.md hash alone. A reinstall that changes only a bundled file keeps SKILL.md, so a kill between the tree swap and the lockfile publish left two trees carrying the recorded hash, and recovery kept the new one and retired the backup the lockfile still described. A tree now matches an entry with a tree hash only when both hashes match. An entry from before tree hashes is still matched on SKILL.md alone. A tree hash that cannot be computed is returned as an error, the way an unreadable SKILL.md already is, so recovery decides nothing about a tree it could not read. Also bring the package comment up to date: install copies the skill directory, not SKILL.md alone. --- internal/skills/install.go | 39 ++++-- internal/skills/install_test.go | 140 +++++++++++++++++++++ internal/skills/unreadable_posix_test.go | 30 +++++ internal/skills/unreadable_windows_test.go | 28 +++++ 4 files changed, 228 insertions(+), 9 deletions(-) create mode 100644 internal/skills/unreadable_posix_test.go create mode 100644 internal/skills/unreadable_windows_test.go diff --git a/internal/skills/install.go b/internal/skills/install.go index e07916465..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" @@ -227,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) @@ -249,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 diff --git a/internal/skills/install_test.go b/internal/skills/install_test.go index 142bb764b..604ae9bc2 100644 --- a/internal/skills/install_test.go +++ b/internal/skills/install_test.go @@ -862,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 +}