library: an edit made in an agent's copy of a skill goes to the library and the other agents, rather than being undone at the next start (Joren on Discord)

With skills given as copies, a sync made a copy that differed from the library's again from the library's, so an agent's edit was reverted when magpie restarted. A sync now takes edits first (takeEdits): a copy with anything newer than magpie's mark was edited since it was made, and the newest such edit is mirrored into the library (the library's old version kept with the backups), then given to every other agent. The library's own change wins when it is newer, and an edit that loses is set aside with the backups, never lost. TestSkillCopyEditReachesTheOthers fails without the change; TestSkillHow now expects an edit made before a switch to links to reach the library.

internal/library/skill_edits.go

@@ -0,0 +1,227 @@
+package library
+
+// An agent given its skills as copies (#896) may change one of them: the
+// agent itself edits the skill, or the user does in that agent's folder.
+// The copy then differs from the library's skill, as it does when the
+// library's has changed, and a sync made it again from the library's,
+// which undid the edit at the next start of magpie (Joren on Discord). So
+// a sync first tells the two apart by magpie's mark, written as the copy
+// was made: a copy with anything in it newer than its mark was edited since,
+// and the library takes the newest such edit, which the sync then gives to
+// every other agent. The library's own change wins when it is newer still
+// (an update, a folder edited by hand). An edit that loses, to the library
+// or to a newer edit in another agent, is never lost: it is kept aside
+// with the backups, as an agent's own skill is.
+
+import (
+	"io/fs"
+	"log"
+	"os"
+	"path/filepath"
+	"slices"
+	"time"
+)
+
+// takeEdits takes into the library the newest edit made in an agent's copy
+// of each of its skills, before the sync gives the library's to the agents.
+func (l *Library) takeEdits(targets []*Target, res *Result) {
+	for _, s := range l.Skills {
+		lib := realDir(skillDir(s.Name))
+		if hashDir(lib) == "" {
+			continue
+		}
+		type edit struct {
+			agent, path string
+			at          time.Time
+		}
+		var edits []edit
+		seen := map[string]bool{}
+		for _, t := range targets {
+			if t.Skills == "" || t.Desktop != nil || !slices.Contains(s.Agents, t.Agent.ID) {
+				continue
+			}
+			p := filepath.Join(t.Skills, s.Name)
+			if !ours(p, s.Name) || linked(p) || seen[realDir(p)] {
+				continue
+			}
+			seen[realDir(p)] = true
+			made, ok := copiedAt(p)
+			if !ok {
+				continue
+			}
+			at := changedAt(p)
+			if !at.After(made) || sameTree(p, lib) {
+				continue
+			}
+			edits = append(edits, edit{t.Agent.ID, p, at})
+		}
+		if len(edits) == 0 {
+			continue
+		}
+		// the newest edit, unless the library changed after it
+		slices.SortStableFunc(edits, func(a, b edit) int { return b.at.Compare(a.at) })
+		won := -1
+		if edits[0].at.After(changedAt(lib)) {
+			won = 0
+		}
+		if won == 0 {
+			e := edits[0]
+			// the library's as it was, kept with the backups
+			if _, err := keepLibrarySkill(s.Name, lib); err != nil {
+				res.fail(e.agent, "skill:"+s.Name, err)
+				continue
+			}
+			if err := mirrorDir(e.path, lib); err != nil {
+				res.fail(e.agent, "skill:"+s.Name, err)
+				continue
+			}
+			log.Printf("library: %s changed %s in its copy; the library takes it, and the other agents get it", e.agent, s.Name)
+			// made again, with a mark newer than the edit, so the copy isn't
+			// taken for edited once the library's changes after it
+			if err := copyIn(e.path, s.Name); err != nil {
+				res.fail(e.agent, "skill:"+s.Name, err)
+			}
+			res.changed(e.agent)
+		}
+		for i, e := range edits {
+			if i == won {
+				continue
+			}
+			// lost to a newer change: kept aside, and the sync makes the
+			// copy again from the library's
+			if dst, err := setAside(e.agent, e.path); err != nil {
+				res.fail(e.agent, "skill:"+s.Name, err)
+			} else {
+				log.Printf("library: %s's edit of %s lost to a newer change; kept at %s", e.agent, s.Name, dst)
+				res.changed(e.agent)
+			}
+		}
+	}
+}
+
+// copiedAt is when magpie made the copy at p: its mark's time.
+func copiedAt(p string) (time.Time, bool) {
+	fi, err := os.Stat(filepath.Join(p, marker))
+	if err != nil {
+		return time.Time{}, false
+	}
+	return fi.ModTime(), true
+}
+
+// changedAt is when anything in the folder last changed: the newest time
+// of what is in it, a file or a folder (one taken away changes its
+// folder's), leaving aside magpie's mark and what Finder or version
+// control keep in it.
+func changedAt(dir string) time.Time {
+	var at time.Time
+	filepath.WalkDir(dir, func(p string, d fs.DirEntry, err error) error {
+		if err != nil {
+			return nil
+		}
+		if p != dir && skipInSkill(d.Name()) {
+			if d.IsDir() {
+				return filepath.SkipDir
+			}
+			return nil
+		}
+		if fi, err := d.Info(); err == nil && fi.ModTime().After(at) {
+			at = fi.ModTime()
+		}
+		return nil
+	})
+	return at
+}
+
+func skipInSkill(name string) bool {
+	return name == marker || name == ".git" || name == ".DS_Store"
+}
+
+// keepLibrarySkill copies the library's skill to the backups before an
+// edit is taken over it.
+func keepLibrarySkill(name, lib string) (string, error) {
+	dst := filepath.Join(BackupDir(), time.Now().Format("2006-01-02_15-04-05.000"), "library", "skills", name)
+	if err := os.MkdirAll(filepath.Dir(dst), 0o700); err != nil {
+		return "", err
+	}
+	return dst, copyDir(lib, dst)
+}
+
+// mirrorDir makes the folder to hold what from does: its files written
+// over to's, and what to has that from doesn't taken away, leaving aside
+// magpie's mark and what Finder or version control keep. File by file, so
+// a library skill that is a folder of the user's stays that folder.
+func mirrorDir(from, to string) error {
+	err := filepath.WalkDir(from, func(p string, d fs.DirEntry, err error) error {
+		if err != nil {
+			return err
+		}
+		rel, _ := filepath.Rel(from, p)
+		if p != from && skipInSkill(d.Name()) {
+			if d.IsDir() {
+				return filepath.SkipDir
+			}
+			return nil
+		}
+		dst := filepath.Join(to, rel)
+		switch {
+		case d.IsDir():
+			if fi, err := os.Lstat(dst); err == nil && !fi.IsDir() {
+				os.Remove(dst)
+			}
+			return os.MkdirAll(dst, 0o755)
+		case d.Type()&fs.ModeSymlink != 0:
+			t, err := os.Readlink(p)
+			if err != nil {
+				return nil
+			}
+			if cur, err := os.Readlink(dst); err == nil && cur == t {
+				return nil
+			}
+			if err := os.RemoveAll(dst); err != nil {
+				return err
+			}
+			return os.Symlink(t, dst)
+		case d.Type().IsRegular():
+			b, err := os.ReadFile(p)
+			if err != nil {
+				return err
+			}
+			if fi, err := os.Lstat(dst); err == nil && !fi.Mode().IsRegular() {
+				if err := os.RemoveAll(dst); err != nil {
+					return err
+				}
+			}
+			mode := fs.FileMode(0o644)
+			if fi, err := d.Info(); err == nil {
+				mode = fi.Mode().Perm()
+			}
+			return os.WriteFile(dst, b, mode)
+		}
+		return nil
+	})
+	if err != nil {
+		return err
+	}
+	return filepath.WalkDir(to, func(p string, d fs.DirEntry, err error) error {
+		if err != nil || p == to {
+			return err
+		}
+		if skipInSkill(d.Name()) {
+			if d.IsDir() {
+				return filepath.SkipDir
+			}
+			return nil
+		}
+		rel, _ := filepath.Rel(to, p)
+		if _, err := os.Lstat(filepath.Join(from, rel)); err == nil {
+			return nil
+		}
+		if err := os.RemoveAll(p); err != nil {
+			return err
+		}
+		if d.IsDir() {
+			return filepath.SkipDir
+		}
+		return nil
+	})
+}

internal/library/skill_edits_test.go

@@ -0,0 +1,108 @@
+package library
+
+import (
+	"io/fs"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+	"time"
+)
+
+// Joren on Discord: with skills given as copies, an agent's edit of one was
+// undone at magpie's next start, the sync making the copy again from the
+// library's. The newest edit goes to the library and every other agent
+// instead; the library's own change wins when it is newer, and an edit that
+// loses is kept with the backups.
+func TestSkillCopyEditReachesTheOthers(t *testing.T) {
+	h := sandbox(t)
+	src := filepath.Join(h, "src/skills")
+	skill(t, filepath.Join(src, "pdf"), "pdf", "Read PDFs")
+	write(t, filepath.Join(src, "pdf/forms.md"), "forms")
+	ok(t)(InstallSkills(src, []string{"pdf"}, []string{"claude", "codex"}))
+	ok(t)(SetSkillHow("", HowCopy))
+	cl, cx := filepath.Join(h, ".claude/skills/pdf"), filepath.Join(h, ".codex/skills/pdf")
+	tick := func() { time.Sleep(20 * time.Millisecond) }
+
+	// Codex edits its copy, a file changed and one taken away
+	tick()
+	write(t, filepath.Join(cx, "SKILL.md"), "---\nname: pdf\ndescription: Edited in Codex\n---\n")
+	if err := os.Remove(filepath.Join(cx, "forms.md")); err != nil {
+		t.Fatal(err)
+	}
+	for range 2 { // a sync, and the one of the next start
+		ok(t)(Sync())
+		for _, p := range []string{cl, cx, filepath.Join(src, "pdf")} {
+			if s := read(t, filepath.Join(p, "SKILL.md")); !strings.Contains(s, "Edited in Codex") {
+				t.Fatalf("%s after a sync:\n%s", p, s)
+			}
+			if _, err := os.Stat(filepath.Join(p, "forms.md")); !os.IsNotExist(err) {
+				t.Fatalf("%s still has forms.md: %v", p, err)
+			}
+		}
+		if !ours(cl, "pdf") || !ours(cx, "pdf") || isLink(t, cx) {
+			t.Fatal("the copies aren't magpie's any more")
+		}
+	}
+	if !backedUp(t, "Read PDFs") {
+		t.Error("the library's skill as it was isn't with the backups")
+	}
+
+	// Claude Code edits its copy, then the library's folder is edited by
+	// hand after it: the library's wins, Claude Code's is kept aside
+	tick()
+	write(t, filepath.Join(cl, "SKILL.md"), "---\nname: pdf\ndescription: Edited in Claude\n---\n")
+	tick()
+	write(t, filepath.Join(src, "pdf/SKILL.md"), "---\nname: pdf\ndescription: Edited by hand\n---\n")
+	ok(t)(Sync())
+	for _, p := range []string{cl, cx} {
+		if s := read(t, filepath.Join(p, "SKILL.md")); !strings.Contains(s, "Edited by hand") {
+			t.Fatalf("%s after the library's change:\n%s", p, s)
+		}
+	}
+	if !backedUp(t, "Edited in Claude") {
+		t.Error("Claude Code's older edit was lost")
+	}
+
+	// both agents edit theirs: the newer one goes everywhere, the other is
+	// kept aside
+	tick()
+	write(t, filepath.Join(cx, "SKILL.md"), "---\nname: pdf\ndescription: Codex again\n---\n")
+	tick()
+	write(t, filepath.Join(cl, "SKILL.md"), "---\nname: pdf\ndescription: Claude again\n---\n")
+	ok(t)(Sync())
+	for _, p := range []string{cl, cx, filepath.Join(src, "pdf")} {
+		if s := read(t, filepath.Join(p, "SKILL.md")); !strings.Contains(s, "Claude again") {
+			t.Fatalf("%s after two edits:\n%s", p, s)
+		}
+	}
+	if !backedUp(t, "Codex again") {
+		t.Error("Codex's older edit was lost")
+	}
+
+	// a copy only touched, the same bytes, takes nothing
+	tick()
+	now := time.Now()
+	if err := os.Chtimes(filepath.Join(cx, "SKILL.md"), now, now); err != nil {
+		t.Fatal(err)
+	}
+	ok(t)(Sync())
+	if s := read(t, filepath.Join(src, "pdf/SKILL.md")); !strings.Contains(s, "Claude again") {
+		t.Fatalf("library after a touch:\n%s", s)
+	}
+}
+
+// backedUp is whether a SKILL.md with text in it is in the backups.
+func backedUp(t *testing.T, text string) bool {
+	t.Helper()
+	found := false
+	filepath.WalkDir(BackupDir(), func(p string, d fs.DirEntry, err error) error {
+		if err == nil && d.Name() == "SKILL.md" {
+			if b, _ := os.ReadFile(p); strings.Contains(string(b), text) {
+				found = true
+			}
+		}
+		return nil
+	})
+	return found
+}

internal/library/skill_how.go

@@ -9,7 +9,8 @@ package library
 // a folder of the user's by that name is never touched. A copy is made
 // again on each sync once the library's skill differs from it: the page
 // says which copies differ until then (SkillView.Behind), with a button
-// that syncs.
+// that syncs. A copy edited since it was made goes to the library first,
+// when its edit is the newest (takeEdits).
 
 import (
 	"errors"

internal/library/skill_how_test.go

@@ -8,6 +8,7 @@ import (
 	"slices"
 	"strings"
 	"testing"
+	"time"
 )
 
 // #896: skills are given as links or as copies, for every agent or one;
@@ -92,21 +93,15 @@ func TestSkillHow(t *testing.T) {
 	}
 
 	// an edit made in claude's copy isn't lost when it is a link again:
-	// it is kept with the backups
+	// the library takes it (takeEdits), and the link shows it
+	time.Sleep(20 * time.Millisecond)
 	write(t, filepath.Join(cl, "notes.md"), "my notes\n")
 	ok(t)(SetSkillHow("", HowLink))
 	if !isLink(t, cl) {
 		t.Fatal("claude isn't a link again")
 	}
-	var kept []string
-	filepath.WalkDir(BackupDir(), func(p string, d fs.DirEntry, err error) error {
-		if err == nil && d.Name() == "notes.md" {
-			kept = append(kept, p)
-		}
-		return nil
-	})
-	if len(kept) != 1 || read(t, kept[0]) != "my notes\n" {
-		t.Errorf("the edited copy wasn't kept: %v", kept)
+	if read(t, filepath.Join(src, "pdf/notes.md")) != "my notes\n" || read(t, filepath.Join(cl, "notes.md")) != "my notes\n" {
+		t.Error("the edit in the copy wasn't taken into the library")
 	}
 	// codex goes the library's way again
 	ok(t)(SetSkillHow("codex", HowCopy))

internal/library/sync.go

@@ -60,6 +60,9 @@ func (l *Library) sync() *Result {
 		l.syncInstructions(t, b, res)
 		l.syncMCP(t, b, res)
 	}
+	// an edit made in an agent's copy of a skill goes to the library first,
+	// for the others to get rather than be undone (takeEdits)
+	l.takeEdits(all, res)
 	// ~/.agents/skills first: an agent that reads it too gets no second
 	// link to what is there already
 	shared := realDir(sharedSkillsDir())