Compare commits

...
Author SHA1 Message Date
Vasanthdev2004 a4578dacc0 test(tools): stop the tty session before removing its root
TestExecCommandTTYSkipsInteractiveBlock runs ssh to a bogus host under
tty with a 250ms yield. When ssh is still resolving the host after the
yield, the session keeps running in the test's directory and nothing
stops it, so on Windows t.TempDir's cleanup fails with a sharing
violation. It failed Smoke (windows-latest) on a README-only PR.

Stop the session in a cleanup, and build the tool on execTestRoot,
which retries the removal while Windows still holds the directory right
after the stop, as the other exec tests that leave a session do.
2026-10-27 12:31:03 +05:30
Vasanth T 164150633a feat(skills): install a skill's bundled files and point the skill tool at them (#1167)
* 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

* 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.

* 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.
2026-10-25 07:24:59 -07:00
15 changed files with 1125 additions and 40 deletions

No files matched your search

+11
View File
@@ -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:
+13 -1
View File
@@ -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
}
+132
View File
@@ -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())
}
}
+1 -1
View File
@@ -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)
}
+25
View File
@@ -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)
}
}
+258
View File
@@ -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()
}
+351
View File
@@ -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": "<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)
}
}
+2 -1
View File
@@ -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
}
+93 -28
View File
@@ -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/<name>/, 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/<name>/: 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
}
+147 -6
View File
@@ -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
+30
View File
@@ -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
}
@@ -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
}
+9 -2
View File
@@ -936,8 +936,15 @@ func TestExecCommandInteractiveBlockSuggestsTTY(t *testing.T) {
}
func TestExecCommandTTYSkipsInteractiveBlock(t *testing.T) {
tool := NewScopedExecCommandTool(t.TempDir(), nil, newExecSessionManager())
// ssh to a bogus host fails fast on its own; the point is that the
// The ssh can still be resolving the bogus host when the 250ms yield
// returns, which leaves its session running in the test root. Stop it
// before the root goes, and use execTestRoot for the moment Windows still
// holds the directory after the stop.
root := execTestRoot(t)
manager := newExecSessionManager()
t.Cleanup(func() { manager.StopAll() })
tool := NewScopedExecCommandTool(root, nil, manager)
// ssh to a bogus host fails on its own; the point is that the
// interactive guard must not fire ahead of the tty path.
result := tool.Run(context.Background(), map[string]any{
"cmd": "ssh host.example.com", "tty": true, "yield_time_ms": 250,
+1 -1
View File
@@ -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)
}
+24
View File
@@ -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)
}
}
}