From 50387893ba4c2bea7ffbdee3b511434ee699ce2f Mon Sep 17 00:00:00 2001 From: Christopher Hlubek Date: Fri, 4 Sep 2026 14:41:31 +0200 Subject: [PATCH 1/5] sdd init: prune installed skill files the bundle no longer carries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SkillStatus walked bundle entries only, so a file left behind by a removed bundle source was invisible to the whole install pipeline (s-tac-zaz). It now also walks the install directory and reports what has no embedded counterpart, and init sweeps those for every rendered agent after the install pass. The stamp is the ownership marker: a file with no sdd-content-hash is a skill of the user's own sharing the directory, so it is neither removed nor reported. A stamped orphan still matching its stamp is removed; an edited one is preserved and named, and goes only under --force — the same rule pruneAgentSkills already applies to a dropped agent's render. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/sdd/main.go | 3 + internal/command/init.go | 13 +- internal/finders/skill.go | 52 ++++++ internal/handlers/handler_init.go | 55 +++++++ internal/handlers/handler_init_orphan_test.go | 153 ++++++++++++++++++ internal/model/skill.go | 33 ++++ internal/presenters/init.go | 26 ++- internal/query/skill.go | 14 ++ 8 files changed, 341 insertions(+), 8 deletions(-) create mode 100644 internal/handlers/handler_init_orphan_test.go diff --git a/cmd/sdd/main.go b/cmd/sdd/main.go index ad832f14..ab920b6f 100644 --- a/cmd/sdd/main.go +++ b/cmd/sdd/main.go @@ -1617,6 +1617,9 @@ func initCmd() *cli.Command { OnAgentSkillsPruned: func(result command.AgentPruneResult) { presenters.RenderInitPrune(os.Stdout, result) }, + OnSkillOrphansPruned: func(result command.AgentPruneResult) { + presenters.RenderInitOrphans(os.Stdout, result) + }, OnIndexMigrated: func(legacyDir, storeDir string, moved bool) { if moved { fmt.Printf(" index migrated: %s → %s\n", legacyDir, storeDir) diff --git a/internal/command/init.go b/internal/command/init.go index 618e1fd4..2d61e66b 100644 --- a/internal/command/init.go +++ b/internal/command/init.go @@ -159,6 +159,12 @@ type InitCmd struct { // files removed and any user-modified files preserved. OnAgentSkillsPruned func(result AgentPruneResult) + // OnSkillOrphansPruned fires once per rendered agent whose install + // directory held files the bundle no longer carries, reporting what was + // removed and which user-modified copies were preserved. Does not fire + // when the sweep found nothing. + OnSkillOrphansPruned func(result AgentPruneResult) + // OnMCPRegistered fires for each project-scope config file written to // register the SDD MCP server for an agent (a fresh file or an // add-if-missing merge). Does not fire when an existing sdd entry is @@ -166,9 +172,10 @@ type InitCmd struct { OnMCPRegistered func(target model.AgentTarget, path string) } -// AgentPruneResult reports the outcome of pruning a dropped agent's rendered -// skills: which files were removed and which user-modified files were kept -// (removable only under --force). +// AgentPruneResult reports the outcome of a prune pass over one agent's +// install directory — a dropped agent's whole render, or the orphans a +// still-rendered agent's bundle no longer carries: which files were removed +// and which user-modified files were kept (removable only under --force). type AgentPruneResult struct { Target model.AgentTarget InstallDir string diff --git a/internal/finders/skill.go b/internal/finders/skill.go index 48ff99bb..ec889e65 100644 --- a/internal/finders/skill.go +++ b/internal/finders/skill.go @@ -7,6 +7,7 @@ import ( "io/fs" "os" "path/filepath" + "strings" "github.com/networkteam/sdd/internal/bundledskills" "github.com/networkteam/sdd/internal/model" @@ -41,8 +42,10 @@ func (f *Finder) SkillStatus(ctx context.Context, q query.SkillStatusQuery) (*qu InstallDir: installDir, Entries: make([]query.SkillStatusEntry, 0, len(bundle.Entries)), } + fromBundle := make(map[string]bool, len(bundle.Entries)) for _, e := range bundle.Entries { abs := filepath.Join(installDir, e.Skill, e.RelPath) + fromBundle[abs] = true installed, err := readSkillFile(abs) if err != nil { return nil, fmt.Errorf("reading installed skill %s: %w", abs, err) @@ -57,9 +60,58 @@ func (f *Finder) SkillStatus(ctx context.Context, q query.SkillStatusQuery) (*qu Installed: installed, }) } + + orphans, err := findSkillOrphans(installDir, fromBundle) + if err != nil { + return nil, err + } + result.Orphans = orphans return result, nil } +// findSkillOrphans walks the install directory for skill files the bundle no +// longer carries. Files sdd never wrote carry no stamp and are dropped here, +// so they never reach a caller that removes things. +func findSkillOrphans(installDir string, fromBundle map[string]bool) ([]query.SkillOrphanEntry, error) { + var orphans []query.SkillOrphanEntry + err := filepath.WalkDir(installDir, func(path string, d fs.DirEntry, err error) error { + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + return fs.SkipAll + } + return err + } + if d.IsDir() || filepath.Ext(path) != ".md" || fromBundle[path] { + return nil + } + installed, err := readSkillFile(path) + if err != nil { + return fmt.Errorf("reading installed skill %s: %w", path, err) + } + class := model.ClassifySkillOrphan(installed) + if class == model.SkillOrphanForeign { + return nil + } + rel, err := filepath.Rel(installDir, path) + if err != nil { + return fmt.Errorf("resolving %s against %s: %w", path, installDir, err) + } + skill, relPath, _ := strings.Cut(filepath.ToSlash(rel), "/") + orphans = append(orphans, query.SkillOrphanEntry{ + Skill: skill, + RelPath: relPath, + AbsPath: path, + Class: class, + Installed: installed, + }) + return nil + }) + if err != nil { + return nil, fmt.Errorf("scanning %s for orphaned skill files: %w", installDir, err) + } + return orphans, nil +} + // readSkillFile returns a parsed SkillFile for path, or nil if the file does // not exist. Any other error is returned so callers can distinguish real // read failures from missing files. diff --git a/internal/handlers/handler_init.go b/internal/handlers/handler_init.go index a7c96f5e..9a69cf80 100644 --- a/internal/handlers/handler_init.go +++ b/internal/handlers/handler_init.go @@ -398,6 +398,12 @@ func (h *Handler) Init(ctx context.Context, cmd *command.InitCmd) error { } } + // Sweep out what the bundle stopped shipping, after the install pass so + // this run's own writes are on disk and stamped (s-tac-zaz). + if err := h.pruneSkillOrphans(ctx, effectiveAgents, effectiveScope, cmd, &touched); err != nil { + return err + } + // Transition messaging (d-tac-o2v): reach users who upgrade without // invoking the skill. log.Info("the /sdd skill family is deprecated — work in /sdd-engine; v0.18.0 will remove the legacy skills and rename /sdd-engine to /sdd") @@ -656,6 +662,55 @@ func (h *Handler) pruneAgentSkills(ctx context.Context, dropped []model.AgentTar return nil } +// pruneSkillOrphans removes installed files whose bundle source is gone, for +// agents that are still rendered — the upgrade path pruneAgentSkills does not +// cover, since nothing was dropped from supported_agents. Ownership and safety +// follow the same rule: only unmodified sdd-written files go, user-modified +// copies are preserved and named, and --force removes those too. Files sdd +// never wrote carry no stamp and never reach here. +func (h *Handler) pruneSkillOrphans(ctx context.Context, targets []model.AgentTarget, scope model.Scope, cmd *command.InitCmd, touched *[]string) error { + for _, target := range targets { + status, err := h.reader.SkillStatus(ctx, query.SkillStatusQuery{ + Target: target, + Scope: scope, + RepoRoot: cmd.RepoRoot, + UserHome: cmd.UserHome, + }) + if err != nil { + return fmt.Errorf("classifying %s skills for orphan prune: %w", target, err) + } + + var removed, keptModified []string + skillDirs := map[string]bool{} + for _, o := range status.Orphans { + if o.Class == model.SkillOrphanModified && !cmd.Force { + keptModified = append(keptModified, o.AbsPath) + continue + } + if err := os.Remove(o.AbsPath); err != nil && !errors.Is(err, fs.ErrNotExist) { + return fmt.Errorf("removing orphaned skill file %s: %w", o.AbsPath, err) + } + removed = append(removed, o.AbsPath) + skillDirs[filepath.Join(status.InstallDir, o.Skill)] = true + } + + for dir := range skillDirs { + pruneEmptyDirs(dir) + } + + *touched = append(*touched, removed...) + if cmd.OnSkillOrphansPruned != nil && (len(removed) > 0 || len(keptModified) > 0) { + cmd.OnSkillOrphansPruned(command.AgentPruneResult{ + Target: target, + InstallDir: status.InstallDir, + Removed: removed, + KeptModified: keptModified, + }) + } + } + return nil +} + // pruneEmptyDirs removes dir and its empty subdirectories bottom-up. A dir // that still holds files is left in place. func pruneEmptyDirs(dir string) { diff --git a/internal/handlers/handler_init_orphan_test.go b/internal/handlers/handler_init_orphan_test.go new file mode 100644 index 00000000..0de63c0c --- /dev/null +++ b/internal/handlers/handler_init_orphan_test.go @@ -0,0 +1,153 @@ +package handlers_test + +import ( + "context" + "os" + "path/filepath" + "slices" + "strings" + "testing" + + "github.com/networkteam/sdd/internal/command" + "github.com/networkteam/sdd/internal/model" +) + +// writeStampedOrphan plants a file that a previous sdd init could have written +// — carrying valid install stamps — at a path the current bundle does not +// contain. Stamp keys are stripped before hashing, so the digest computed over +// the placeholder text is the digest of the finished file. +func writeStampedOrphan(t *testing.T, path, body string) { + t.Helper() + text := "---\nname: " + filepath.Base(filepath.Dir(path)) + "\nsdd-version: v0.1.0\nsdd-content-hash: placeholder\n---\n\n" + body + stamped := strings.Replace(text, "placeholder", model.ComputeSkillHash([]byte(text)), 1) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatalf("mkdir for %s: %v", path, err) + } + if err := os.WriteFile(path, []byte(stamped), 0o644); err != nil { + t.Fatalf("write %s: %v", path, err) + } +} + +// TestInit_PrunesUnmodifiedOrphan covers the upgrade path of s-tac-zaz: a file +// a previous install wrote, whose bundle source is gone, is removed on the next +// init even though no agent was dropped — and the directory it emptied goes too. +func TestInit_PrunesUnmodifiedOrphan(t *testing.T) { + tmp := t.TempDir() + h := initExistingWithAgents(t, tmp, model.AgentClaude) + + orphan := filepath.Join(tmp, ".claude/skills/sdd-retired/SKILL.md") + writeStampedOrphan(t, orphan, "A skill the bundle no longer ships.\n") + orphanRef := filepath.Join(tmp, ".claude/skills/sdd/references/gone.md") + writeStampedOrphan(t, orphanRef, "A reference the bundle no longer ships.\n") + + var pruned []command.AgentPruneResult + if err := h.Init(context.Background(), &command.InitCmd{ + RepoRoot: tmp, + BinaryVersion: "v0.2.0", + Scope: model.ScopeProject, + OnSkillOrphansPruned: func(r command.AgentPruneResult) { pruned = append(pruned, r) }, + }); err != nil { + t.Fatal(err) + } + + for _, p := range []string{orphan, orphanRef} { + if _, err := os.Stat(p); !os.IsNotExist(err) { + t.Errorf("unmodified orphan %s should be removed, stat err = %v", p, err) + } + } + // The skill dir emptied by the removal goes with it; the one still holding + // bundle files stays. + if _, err := os.Stat(filepath.Join(tmp, ".claude/skills/sdd-retired")); !os.IsNotExist(err) { + t.Errorf("emptied skill dir should be pruned, stat err = %v", err) + } + if _, err := os.Stat(filepath.Join(tmp, ".claude/skills/sdd/SKILL.md")); err != nil { + t.Errorf("bundle files must survive the orphan sweep: %v", err) + } + if len(pruned) != 1 { + t.Fatalf("expected one orphan-prune callback, got %+v", pruned) + } + if !slices.Contains(pruned[0].Removed, orphan) || !slices.Contains(pruned[0].Removed, orphanRef) { + t.Errorf("Removed should name both orphans, got %v", pruned[0].Removed) + } +} + +// TestInit_PreservesModifiedOrphan holds the safety half of the rule: an orphan +// the user has edited is never silently discarded — it stays, and the run names +// it. Under --force it goes like any other sdd-owned file. +func TestInit_PreservesModifiedOrphan(t *testing.T) { + tmp := t.TempDir() + h := initExistingWithAgents(t, tmp, model.AgentClaude) + + orphan := filepath.Join(tmp, ".claude/skills/sdd-retired/SKILL.md") + writeStampedOrphan(t, orphan, "A skill the bundle no longer ships.\n") + appendToFile(t, orphan, "\n\n") + + var pruned []command.AgentPruneResult + if err := h.Init(context.Background(), &command.InitCmd{ + RepoRoot: tmp, + BinaryVersion: "v0.2.0", + Scope: model.ScopeProject, + OnSkillOrphansPruned: func(r command.AgentPruneResult) { pruned = append(pruned, r) }, + }); err != nil { + t.Fatal(err) + } + + if _, err := os.Stat(orphan); err != nil { + t.Errorf("modified orphan must be preserved without --force: %v", err) + } + if len(pruned) != 1 || !slices.Contains(pruned[0].KeptModified, orphan) { + t.Fatalf("KeptModified should name %s, got %+v", orphan, pruned) + } + + pruned = nil + if err := h.Init(context.Background(), &command.InitCmd{ + RepoRoot: tmp, + BinaryVersion: "v0.2.0", + Scope: model.ScopeProject, + Force: true, + OnSkillOrphansPruned: func(r command.AgentPruneResult) { pruned = append(pruned, r) }, + }); err != nil { + t.Fatal(err) + } + + if _, err := os.Stat(orphan); !os.IsNotExist(err) { + t.Errorf("--force should remove the modified orphan, stat err = %v", err) + } + if len(pruned) != 1 || !slices.Contains(pruned[0].Removed, orphan) { + t.Errorf("under --force the modified orphan should be Removed, got %+v", pruned) + } +} + +// TestInit_LeavesForeignSkillUntouched guards the ownership rule: the install +// directory also holds skills sdd never wrote. Carrying no stamp, they are not +// orphans — not removed, and not reported as anything the user must resolve. +func TestInit_LeavesForeignSkillUntouched(t *testing.T) { + tmp := t.TempDir() + h := initExistingWithAgents(t, tmp, model.AgentClaude) + + foreign := filepath.Join(tmp, ".claude/skills/my-own/SKILL.md") + if err := os.MkdirAll(filepath.Dir(foreign), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(foreign, []byte("---\nname: my-own\n---\n\nMine, not sdd's.\n"), 0o644); err != nil { + t.Fatal(err) + } + + var pruned []command.AgentPruneResult + if err := h.Init(context.Background(), &command.InitCmd{ + RepoRoot: tmp, + BinaryVersion: "v0.2.0", + Scope: model.ScopeProject, + Force: true, + OnSkillOrphansPruned: func(r command.AgentPruneResult) { pruned = append(pruned, r) }, + }); err != nil { + t.Fatal(err) + } + + if _, err := os.Stat(foreign); err != nil { + t.Errorf("a skill sdd never wrote must survive even --force: %v", err) + } + if len(pruned) != 0 { + t.Errorf("an unstamped file is not an orphan and must not be reported, got %+v", pruned) + } +} diff --git a/internal/model/skill.go b/internal/model/skill.go index b0ce3737..75699f31 100644 --- a/internal/model/skill.go +++ b/internal/model/skill.go @@ -482,3 +482,36 @@ func ParseSkillFile(absPath string, content []byte) *SkillFile { sf.StoredVersion, sf.StoredHash = readStamps(fm) return sf } + +// SkillOrphanClass classifies an installed file the embedded bundle no longer +// carries — the state left behind when a bundle source is removed by a +// rename, a split, or a retirement. +type SkillOrphanClass string + +const ( + // SkillOrphanForeign means the file carries no sdd install stamp, so sdd + // never wrote it: a skill of the user's own sharing the install + // directory. Never removed, never reported. + SkillOrphanForeign SkillOrphanClass = "foreign" + + // SkillOrphanUnmodified means sdd wrote the file and its content still + // matches the stamp from that install — safe to remove. + SkillOrphanUnmodified SkillOrphanClass = "unmodified" + + // SkillOrphanModified means sdd wrote the file and it has been edited + // since — preserved, and named so the user can resolve it. + SkillOrphanModified SkillOrphanClass = "modified" +) + +// ClassifySkillOrphan decides what may be done with an installed file that has +// no bundle counterpart. The stamp is the ownership marker: without one, the +// file is not sdd's to touch. +func ClassifySkillOrphan(installed *SkillFile) SkillOrphanClass { + if installed == nil || installed.StoredHash == "" { + return SkillOrphanForeign + } + if ComputeSkillHash(installed.Content) == installed.StoredHash { + return SkillOrphanUnmodified + } + return SkillOrphanModified +} diff --git a/internal/presenters/init.go b/internal/presenters/init.go index 2aa24715..3a5e5d63 100644 --- a/internal/presenters/init.go +++ b/internal/presenters/init.go @@ -43,10 +43,26 @@ func RenderInitSkills(w io.Writer, installDir string, result command.SkillInstal // supported_agents, and lists any user-modified files left untouched. func RenderInitPrune(w io.Writer, result command.AgentPruneResult) { fmt.Fprintf(w, "pruned %s: %d file(s) removed from %s\n", result.Target, len(result.Removed), result.InstallDir) - if len(result.KeptModified) > 0 { - fmt.Fprintf(w, " preserved: %d modified file(s) left untouched (pass --force to remove)\n", len(result.KeptModified)) - for _, p := range result.KeptModified { - fmt.Fprintf(w, " - %s\n", p) - } + renderPreserved(w, result.KeptModified) +} + +// RenderInitOrphans summarises the files removed from a still-rendered agent's +// install directory because the bundle no longer carries them, and names every +// user-modified copy left in place. +func RenderInitOrphans(w io.Writer, result command.AgentPruneResult) { + fmt.Fprintf(w, "removed %d orphaned %s file(s) from %s (no longer part of sdd)\n", len(result.Removed), result.Target, result.InstallDir) + for _, p := range result.Removed { + fmt.Fprintf(w, " - %s\n", p) + } + renderPreserved(w, result.KeptModified) +} + +func renderPreserved(w io.Writer, keptModified []string) { + if len(keptModified) == 0 { + return + } + fmt.Fprintf(w, " preserved: %d modified file(s) left untouched (pass --force to remove)\n", len(keptModified)) + for _, p := range keptModified { + fmt.Fprintf(w, " - %s\n", p) } } diff --git a/internal/query/skill.go b/internal/query/skill.go index 6e43c6f3..beb945dd 100644 --- a/internal/query/skill.go +++ b/internal/query/skill.go @@ -19,6 +19,20 @@ type SkillStatusResult struct { // Entries is one row per embedded skill file. Entries []SkillStatusEntry + + // Orphans is one row per installed file the bundle no longer carries, + // excluding files sdd never wrote. Empty on a bundle that lost nothing. + Orphans []SkillOrphanEntry +} + +// SkillOrphanEntry carries an installed file with no embedded counterpart, +// classified by whether sdd may remove it. +type SkillOrphanEntry struct { + Skill string + RelPath string + AbsPath string + Class model.SkillOrphanClass + Installed *model.SkillFile } // SkillStatusEntry carries the inputs a handler needs to decide whether (and From cbc94f52734a35f46d2722edc3c7044577aae962 Mon Sep 17 00:00:00 2001 From: Christopher Hlubek Date: Sat, 5 Sep 2026 10:54:10 +0200 Subject: [PATCH 2/5] sdd init: one prune path for both passes, and a downgrade guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The orphan sweep arrived as a near-copy of pruneAgentSkills. Both now feed removePrunable, which owns the shared rule — unmodified files go, edited ones are preserved and named, --force takes those too, emptied directories are cleaned up. Each caller keeps only what differs: which files it offers, and that a dropped agent also takes its parent skills dir. SkillStampIsAhead stops an older binary pruning what a newer one installed: absence from the running bundle is not evidence a later sdd never shipped it. Co-Authored-By: Claude Opus 5 (1M context) --- internal/command/init.go | 6 + internal/handlers/handler_init.go | 108 ++++++++++-------- internal/handlers/handler_init_orphan_test.go | 46 +++++++- internal/model/skill.go | 13 +++ 4 files changed, 127 insertions(+), 46 deletions(-) diff --git a/internal/command/init.go b/internal/command/init.go index 2d61e66b..6133ec8d 100644 --- a/internal/command/init.go +++ b/internal/command/init.go @@ -183,6 +183,12 @@ type AgentPruneResult struct { KeptModified []string } +// TouchedAnything reports whether the pass found something worth telling the +// user about — a prune that matched nothing stays silent. +func (r AgentPruneResult) TouchedAnything() bool { + return len(r.Removed) > 0 || len(r.KeptModified) > 0 +} + // Validate checks required fields. func (c *InitCmd) Validate() error { if c.RepoRoot == "" { diff --git a/internal/handlers/handler_init.go b/internal/handlers/handler_init.go index 9a69cf80..b4729035 100644 --- a/internal/handlers/handler_init.go +++ b/internal/handlers/handler_init.go @@ -621,47 +621,70 @@ func (h *Handler) pruneAgentSkills(ctx context.Context, dropped []model.AgentTar return fmt.Errorf("classifying %s skills for prune: %w", target, err) } - var removed, keptModified []string - skillDirs := map[string]bool{} + var files []prunable for _, e := range status.Entries { if e.Status == model.SkillStatusMissing { continue } - if e.Status == model.SkillStatusModified && !cmd.Force { - keptModified = append(keptModified, e.AbsPath) - continue - } - // Current, Pristine, or (Modified under --force): sdd-owned. - if err := os.Remove(e.AbsPath); err != nil && !errors.Is(err, fs.ErrNotExist) { - return fmt.Errorf("removing %s: %w", e.AbsPath, err) - } - removed = append(removed, e.AbsPath) - skillDirs[filepath.Join(status.InstallDir, e.Skill)] = true + files = append(files, prunable{ + absPath: e.AbsPath, + skill: e.Skill, + modified: e.Status == model.SkillStatusModified, + }) } - // Remove emptied skill subdirs (and their empty descendants), then the - // parent skills dir if nothing remains. A dir still holding a user's - // non-sdd skill is left in place. - for dir := range skillDirs { - pruneEmptyDirs(dir) + result, err := removePrunable(files, target, status.InstallDir, cmd.Force, touched) + if err != nil { + return err } + // A dropped agent leaves the whole tree behind, so the parent skills + // dir goes too when nothing remains in it. A dir still holding a + // user's non-sdd skill is left in place. if entries, err := os.ReadDir(status.InstallDir); err == nil && len(entries) == 0 { _ = os.Remove(status.InstallDir) } - *touched = append(*touched, removed...) - if cmd.OnAgentSkillsPruned != nil && (len(removed) > 0 || len(keptModified) > 0) { - cmd.OnAgentSkillsPruned(command.AgentPruneResult{ - Target: target, - InstallDir: status.InstallDir, - Removed: removed, - KeptModified: keptModified, - }) + if cmd.OnAgentSkillsPruned != nil && result.TouchedAnything() { + cmd.OnAgentSkillsPruned(result) } } return nil } +// prunable is one installed file a prune pass may remove, paired with whether +// the user has edited it since sdd wrote it. +type prunable struct { + absPath string + skill string + modified bool +} + +// removePrunable is the one rule both prune passes apply — a dropped agent's +// whole render, and the orphans a still-rendered agent's bundle no longer +// carries: unmodified files go, edited ones are preserved and named, --force +// takes those too, and directories a removal emptied are cleaned up. Removed +// paths join touched so the commit records the deletions. +func removePrunable(files []prunable, target model.AgentTarget, installDir string, force bool, touched *[]string) (command.AgentPruneResult, error) { + result := command.AgentPruneResult{Target: target, InstallDir: installDir} + skillDirs := map[string]bool{} + for _, f := range files { + if f.modified && !force { + result.KeptModified = append(result.KeptModified, f.absPath) + continue + } + if err := os.Remove(f.absPath); err != nil && !errors.Is(err, fs.ErrNotExist) { + return result, fmt.Errorf("removing %s: %w", f.absPath, err) + } + result.Removed = append(result.Removed, f.absPath) + skillDirs[filepath.Join(installDir, f.skill)] = true + } + for dir := range skillDirs { + pruneEmptyDirs(dir) + } + *touched = append(*touched, result.Removed...) + return result, nil +} + // pruneSkillOrphans removes installed files whose bundle source is gone, for // agents that are still rendered — the upgrade path pruneAgentSkills does not // cover, since nothing was dropped from supported_agents. Ownership and safety @@ -680,32 +703,27 @@ func (h *Handler) pruneSkillOrphans(ctx context.Context, targets []model.AgentTa return fmt.Errorf("classifying %s skills for orphan prune: %w", target, err) } - var removed, keptModified []string - skillDirs := map[string]bool{} + var files []prunable for _, o := range status.Orphans { - if o.Class == model.SkillOrphanModified && !cmd.Force { - keptModified = append(keptModified, o.AbsPath) + // A file stamped by a later sdd is not an orphan, it is the + // future: an older binary running here must not delete what a + // newer one installed just because its own bundle lacks it. + if model.SkillStampIsAhead(o.Installed.StoredVersion, cmd.BinaryVersion) { continue } - if err := os.Remove(o.AbsPath); err != nil && !errors.Is(err, fs.ErrNotExist) { - return fmt.Errorf("removing orphaned skill file %s: %w", o.AbsPath, err) - } - removed = append(removed, o.AbsPath) - skillDirs[filepath.Join(status.InstallDir, o.Skill)] = true + files = append(files, prunable{ + absPath: o.AbsPath, + skill: o.Skill, + modified: o.Class == model.SkillOrphanModified, + }) } - for dir := range skillDirs { - pruneEmptyDirs(dir) + result, err := removePrunable(files, target, status.InstallDir, cmd.Force, touched) + if err != nil { + return err } - - *touched = append(*touched, removed...) - if cmd.OnSkillOrphansPruned != nil && (len(removed) > 0 || len(keptModified) > 0) { - cmd.OnSkillOrphansPruned(command.AgentPruneResult{ - Target: target, - InstallDir: status.InstallDir, - Removed: removed, - KeptModified: keptModified, - }) + if cmd.OnSkillOrphansPruned != nil && result.TouchedAnything() { + cmd.OnSkillOrphansPruned(result) } } return nil diff --git a/internal/handlers/handler_init_orphan_test.go b/internal/handlers/handler_init_orphan_test.go index 0de63c0c..6720335a 100644 --- a/internal/handlers/handler_init_orphan_test.go +++ b/internal/handlers/handler_init_orphan_test.go @@ -9,6 +9,8 @@ import ( "testing" "github.com/networkteam/sdd/internal/command" + "github.com/networkteam/sdd/internal/finders" + "github.com/networkteam/sdd/internal/handlers" "github.com/networkteam/sdd/internal/model" ) @@ -18,7 +20,12 @@ import ( // the placeholder text is the digest of the finished file. func writeStampedOrphan(t *testing.T, path, body string) { t.Helper() - text := "---\nname: " + filepath.Base(filepath.Dir(path)) + "\nsdd-version: v0.1.0\nsdd-content-hash: placeholder\n---\n\n" + body + writeStampedOrphanAt(t, path, "v0.1.0", body) +} + +func writeStampedOrphanAt(t *testing.T, path, version, body string) { + t.Helper() + text := "---\nname: " + filepath.Base(filepath.Dir(path)) + "\nsdd-version: " + version + "\nsdd-content-hash: placeholder\n---\n\n" + body stamped := strings.Replace(text, "placeholder", model.ComputeSkillHash([]byte(text)), 1) if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { t.Fatalf("mkdir for %s: %v", path, err) @@ -118,6 +125,43 @@ func TestInit_PreservesModifiedOrphan(t *testing.T) { } } +// TestInit_LeavesAheadStampedOrphan covers the downgrade case: an older binary +// finds files a newer sdd installed missing from its own bundle, and must not +// delete the future on the strength of that. +func TestInit_LeavesAheadStampedOrphan(t *testing.T) { + tmp := t.TempDir() + h := handlers.New(handlers.Options{Reader: finders.New(finders.Options{})}) + seed := &command.InitCmd{ + RepoRoot: tmp, + BinaryVersion: "v0.17.0", + Targets: []model.AgentTarget{model.AgentClaude}, + Scope: model.ScopeProject, + } + if err := h.Init(context.Background(), seed); err != nil { + t.Fatalf("seed init: %v", err) + } + + ahead := filepath.Join(tmp, ".claude/skills/sdd-future/SKILL.md") + writeStampedOrphanAt(t, ahead, "v0.19.0", "Shipped by a later sdd than the one running.\n") + behind := filepath.Join(tmp, ".claude/skills/sdd-retired/SKILL.md") + writeStampedOrphanAt(t, behind, "v0.16.0", "Shipped by an earlier sdd.\n") + + if err := h.Init(context.Background(), &command.InitCmd{ + RepoRoot: tmp, + BinaryVersion: "v0.17.0", + Scope: model.ScopeProject, + }); err != nil { + t.Fatal(err) + } + + if _, err := os.Stat(ahead); err != nil { + t.Errorf("a file stamped by a later sdd must survive: %v", err) + } + if _, err := os.Stat(behind); !os.IsNotExist(err) { + t.Errorf("a file stamped by an earlier sdd is an ordinary orphan, stat err = %v", err) + } +} + // TestInit_LeavesForeignSkillUntouched guards the ownership rule: the install // directory also holds skills sdd never wrote. Carrying no stamp, they are not // orphans — not removed, and not reported as anything the user must resolve. diff --git a/internal/model/skill.go b/internal/model/skill.go index 75699f31..b907979c 100644 --- a/internal/model/skill.go +++ b/internal/model/skill.go @@ -9,6 +9,7 @@ import ( "path/filepath" "strings" + "golang.org/x/mod/semver" "gopkg.in/yaml.v3" ) @@ -515,3 +516,15 @@ func ClassifySkillOrphan(installed *SkillFile) SkillOrphanClass { } return SkillOrphanModified } + +// SkillStampIsAhead reports whether an installed file's version stamp names a +// release later than the running binary — the downgrade case, where an older +// sdd would otherwise prune files a newer one installed simply because its own +// bundle does not carry them. Dev builds on either side never trigger it, in +// keeping with how they bypass the other version gates. +func SkillStampIsAhead(stampVersion, binaryVersion string) bool { + if IsDevVersion(stampVersion) || IsDevVersion(binaryVersion) { + return false + } + return semver.Compare(normalizeSemver(stampVersion), normalizeSemver(binaryVersion)) > 0 +} From 7e27506b8099dd98dc3c973a0e5049795df9dfe8 Mon Sep 17 00:00:00 2001 From: Christopher Hlubek Date: Sat, 5 Sep 2026 11:26:02 +0200 Subject: [PATCH 3/5] sdd: signal tactical `sdd init` now removes installed skill files whose bundle source is ... SDD-Mutation: entry-20260905-112554-s-tac-3ja --- .sdd/graph/2026/09/05-112554-s-tac-3ja.md | 32 +++++++++++++++++++++++ 1 file changed, 32 insertions(+) create mode 100644 .sdd/graph/2026/09/05-112554-s-tac-3ja.md diff --git a/.sdd/graph/2026/09/05-112554-s-tac-3ja.md b/.sdd/graph/2026/09/05-112554-s-tac-3ja.md new file mode 100644 index 00000000..26b0f8c8 --- /dev/null +++ b/.sdd/graph/2026/09/05-112554-s-tac-3ja.md @@ -0,0 +1,32 @@ +--- +type: signal +layer: tactical +kind: done +refs: + - id: 20260821-123832-d-tac-ip1 + kind: addresses + desc: delivers its orphan-cleanup criterion; the removal, rename, surface sweep, parity verdict, kind-capture coverage and release notes stay open + - id: 20260614-182310-s-tac-wqq + kind: builds-on + desc: the dropped-agent prune whose content-hash safety rule this extends to orphans within a still-rendered agent +closes: + - 20260507-174656-s-tac-zaz +participants: + - Christopher +confidence: high +topics: + - implementation/cli +summary: '`sdd init` now detects and removes installed skill files whose bundle source is gone, extending the content-hash safety rule—preserve edited files, remove stamped matches, `--force` overrides—to orphans within a still-rendered agent, with the install stamp as ownership marker for skills sdd never wrote. It delivers the orphan-cleanup criterion of the retirement plan (20260821-123832-d-tac-ip1), leaving that plan''s other criteria open, and extends the dropped-agent prune''s rule from whole renders to individual orphans (20260614-182310-s-tac-wqq). It closes the gap signal (20260507-174656-s-tac-zaz).' +--- + +`sdd init` now removes installed skill files whose bundle source is gone, so an upgrade no longer leaves stale copies behind (commits 50387893 and cbc94f52). + +The read side was the hole: `SkillStatus` walked bundle entries only, so a file on disk with no embedded counterpart was invisible to the whole install pipeline. It now also walks the install directory and reports what has no bundle source, and init sweeps those for every rendered agent after the install pass — by the same safety rule the dropped-agent prune already applies to a whole render (20260614-182310-s-tac-wqq): a file still matching its install stamp is removed, an edited one is preserved and named, and `--force` removes that too. The sweep first landed as a near-copy of that prune; both now run through one path holding the rule, each caller supplying only the files it offers and what else its own case takes. + +One rule the gap did not state had to be settled in the building. The install directory also holds skills sdd never wrote, so the stamp became the ownership marker: a file carrying none is not an orphan at all — left untouched, and not reported as something to resolve. A test holds that under `--force`, where getting it wrong would delete a user's own skill during an upgrade. Because the stamp is written into the installed file rather than read from the bundle, recognising sdd's own work needs no record of what older versions shipped — the question raised in dialogue when the sweep was weighed against a one-time list of retired paths. One direction still needed a guard: a stamp naming a release ahead of the running binary is left alone, since absence from an older bundle is no evidence a newer sdd never shipped the file. + +Beyond the tests, the built binary was run against scratch projects: a tracked orphan is removed, its deletion committed, and the working tree is clean afterwards. + +That run also surfaced a pre-existing condition the sweep inherits: an orphan that was never committed is removed, and the auto-commit then fails on it. Ruled out of scope in dialogue, since init commits what it installs and a genuine orphan is therefore tracked. + +This is the orphan-cleanup criterion of the retirement plan (20260821-123832-d-tac-ip1) and closes the gap that plan adopted its rule from. Its remaining criteria are untouched. From 4c374dd61d9e7a3b841c78467dc74d317c9db4fc Mon Sep 17 00:00:00 2001 From: Christopher Hlubek Date: Sat, 5 Sep 2026 11:26:16 +0200 Subject: [PATCH 4/5] sdd: summarize 20260905-112554-s-tac-3ja (manual) SDD-Mutation: summary-20260905-112554-s-tac-3ja-416463235f34b340 --- .sdd/graph/2026/09/05-112554-s-tac-3ja.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.sdd/graph/2026/09/05-112554-s-tac-3ja.md b/.sdd/graph/2026/09/05-112554-s-tac-3ja.md index 26b0f8c8..ec4c27a1 100644 --- a/.sdd/graph/2026/09/05-112554-s-tac-3ja.md +++ b/.sdd/graph/2026/09/05-112554-s-tac-3ja.md @@ -16,7 +16,7 @@ participants: confidence: high topics: - implementation/cli -summary: '`sdd init` now detects and removes installed skill files whose bundle source is gone, extending the content-hash safety rule—preserve edited files, remove stamped matches, `--force` overrides—to orphans within a still-rendered agent, with the install stamp as ownership marker for skills sdd never wrote. It delivers the orphan-cleanup criterion of the retirement plan (20260821-123832-d-tac-ip1), leaving that plan''s other criteria open, and extends the dropped-agent prune''s rule from whole renders to individual orphans (20260614-182310-s-tac-wqq). It closes the gap signal (20260507-174656-s-tac-zaz).' +summary: '`sdd init` now detects and removes installed skill files whose bundle source is gone, extending the content-hash safety rule—preserve edited files, remove stamped matches, `--force` overrides—to orphans within a still-rendered agent, with the install stamp as the ownership marker that keeps skills sdd never wrote out of the sweep and a version guard that spares files stamped by a later release. It delivers the orphan-cleanup criterion of the retirement plan (20260821-123832-d-tac-ip1), leaving that plan''s other criteria open, and folds its own sweep together with the dropped-agent prune into one path holding the shared rule (20260614-182310-s-tac-wqq). It closes the gap signal (20260507-174656-s-tac-zaz).' --- `sdd init` now removes installed skill files whose bundle source is gone, so an upgrade no longer leaves stale copies behind (commits 50387893 and cbc94f52). From 3bae179b232169c5b4c8a10a0b506ba4b9283a41 Mon Sep 17 00:00:00 2001 From: Christopher Hlubek Date: Sat, 5 Sep 2026 11:34:14 +0200 Subject: [PATCH 5/5] sdd init: an unreadable path cannot make the orphan sweep fail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sweep read every Markdown file under the install directory to decide ownership, so one it could not read — a foreign file closed by permissions, a directory it may not enter — failed SkillStatus and took the whole init down, including the install pass that calls it first. Ownership is what the read establishes, and establishing it is the only thing that leads to deletion, so a path that cannot be read is passed over: never deletes more, at worst leaves an orphan for a later run. Files already known to be sdd's are read by the bundle-entry loop, where a failure still stops everything. Reported by Greptile on #7. Co-Authored-By: Claude Opus 5 (1M context) --- internal/finders/skill.go | 18 +++++++-- internal/handlers/handler_init_orphan_test.go | 39 +++++++++++++++++++ 2 files changed, 54 insertions(+), 3 deletions(-) diff --git a/internal/finders/skill.go b/internal/finders/skill.go index ec889e65..00e5a409 100644 --- a/internal/finders/skill.go +++ b/internal/finders/skill.go @@ -72,21 +72,33 @@ func (f *Finder) SkillStatus(ctx context.Context, q query.SkillStatusQuery) (*qu // findSkillOrphans walks the install directory for skill files the bundle no // longer carries. Files sdd never wrote carry no stamp and are dropped here, // so they never reach a caller that removes things. +// +// The directory holds other people's files, so a path this walk cannot read is +// a path whose ownership cannot be established — and establishing ownership is +// the only thing that leads to deletion. Such a path is passed over rather than +// failing the run, which never deletes more and at worst leaves an orphan for a +// later init. Files already known to be sdd's are read by the bundle-entry loop +// above, where a read failure still stops everything. func findSkillOrphans(installDir string, fromBundle map[string]bool) ([]query.SkillOrphanEntry, error) { var orphans []query.SkillOrphanEntry err := filepath.WalkDir(installDir, func(path string, d fs.DirEntry, err error) error { if err != nil { - if errors.Is(err, fs.ErrNotExist) { + if path == installDir { + // Nothing installed for this target yet, or the directory is + // closed to us: there are no orphans to find either way. return fs.SkipAll } - return err + if d != nil && d.IsDir() { + return fs.SkipDir + } + return nil } if d.IsDir() || filepath.Ext(path) != ".md" || fromBundle[path] { return nil } installed, err := readSkillFile(path) if err != nil { - return fmt.Errorf("reading installed skill %s: %w", path, err) + return nil } class := model.ClassifySkillOrphan(installed) if class == model.SkillOrphanForeign { diff --git a/internal/handlers/handler_init_orphan_test.go b/internal/handlers/handler_init_orphan_test.go index 6720335a..70688a6e 100644 --- a/internal/handlers/handler_init_orphan_test.go +++ b/internal/handlers/handler_init_orphan_test.go @@ -125,6 +125,45 @@ func TestInit_PreservesModifiedOrphan(t *testing.T) { } } +// TestInit_UnreadableForeignFileDoesNotAbort holds the blast radius of the +// orphan sweep: the install directory holds other people's files, and one sdd +// cannot even read must not take the whole init down with it. +func TestInit_UnreadableForeignFileDoesNotAbort(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root reads through permission bits") + } + tmp := t.TempDir() + h := initExistingWithAgents(t, tmp, model.AgentClaude) + + sealed := filepath.Join(tmp, ".claude/skills/theirs/SKILL.md") + if err := os.MkdirAll(filepath.Dir(sealed), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(sealed, []byte("---\nname: theirs\n---\n\nUnreadable.\n"), 0o000); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(sealed, 0o644) }) + + orphan := filepath.Join(tmp, ".claude/skills/sdd-retired/SKILL.md") + writeStampedOrphan(t, orphan, "A skill the bundle no longer ships.\n") + + if err := h.Init(context.Background(), &command.InitCmd{ + RepoRoot: tmp, + BinaryVersion: "v0.2.0", + Scope: model.ScopeProject, + }); err != nil { + t.Fatalf("an unreadable foreign file must not fail init: %v", err) + } + + if _, err := os.Stat(sealed); err != nil { + t.Errorf("the unreadable file must be left alone: %v", err) + } + // The sweep still does its job around it. + if _, err := os.Stat(orphan); !os.IsNotExist(err) { + t.Errorf("a readable orphan should still be pruned, stat err = %v", err) + } +} + // TestInit_LeavesAheadStampedOrphan covers the downgrade case: an older binary // finds files a newer sdd installed missing from its own bundle, and must not // delete the future on the strength of that.