From ca94d02aeef49421314a00b575fd3bd392bf895b Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Tue, 21 Jul 2026 16:23:05 +0200 Subject: [PATCH 1/4] Record project-scope skill installs in the lock file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RFC THV-0080 requires every project-scoped skill install to be pinned in toolhive.lock.yaml, including transitively materialized toolhive.requires dependencies, with a fail-hard guarantee so a lock-write failure never leaves an install silently unpinned. The whole feature stays inert on main until the full RFC v1 (this lock-file stack plus the later Sigstore stack) has landed — gated behind TOOLHIVE_SKILLS_LOCK_ENABLED, following the existing TOOLHIVE_DEV precedent for staged rollouts. Each PR in the stack is independently mergeable without exposing partial behavior. - Install hooks into the single choke point (installAndRegister): compute contentDigest, upsert the lock entry, roll back the DB record if the lock write fails - toolhive.requires dependencies materialize recursively (visited set guards cycles, skills.MaxDependencies bounds the whole tree), with RequiredBy merged across shared dependencies - Uninstall cascades to orphaned, non-explicit dependencies via Lockfile.RemoveParentFromRequiredBy, itself cycle-safe - E2E coverage for the real HTTP API + thv serve subprocess path Part of the production stack for #5715 (RFC THV-0080). Stack: 3/6. --- pkg/skills/feature_gate.go | 27 ++ pkg/skills/feature_gate_test.go | 33 ++ pkg/skills/options.go | 23 ++ pkg/skills/skillsvc/content_digest.go | 73 +++++ pkg/skills/skillsvc/install.go | 57 +++- pkg/skills/skillsvc/lock.go | 163 ++++++++++ pkg/skills/skillsvc/lock_test.go | 301 ++++++++++++++++++ pkg/skills/skillsvc/uninstall.go | 126 ++++++-- pkg/skills/skillsvc/uninstall_cascade_test.go | 151 +++++++++ test/e2e/api_helpers.go | 6 +- test/e2e/api_skills_test.go | 126 ++++++++ 11 files changed, 1047 insertions(+), 39 deletions(-) create mode 100644 pkg/skills/feature_gate.go create mode 100644 pkg/skills/feature_gate_test.go create mode 100644 pkg/skills/skillsvc/content_digest.go create mode 100644 pkg/skills/skillsvc/lock.go create mode 100644 pkg/skills/skillsvc/lock_test.go create mode 100644 pkg/skills/skillsvc/uninstall_cascade_test.go diff --git a/pkg/skills/feature_gate.go b/pkg/skills/feature_gate.go new file mode 100644 index 0000000000..8efff9d00a --- /dev/null +++ b/pkg/skills/feature_gate.go @@ -0,0 +1,27 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package skills + +import ( + "os" + "strings" +) + +// LockFileEnvVar gates the project-level skills lock file feature (RFC +// THV-0080) while it lands across multiple PRs. The feature is inert on +// main until every PR in the stack — lock file, sync, upgrade, and Sigstore +// signing/verification — has merged; this keeps each PR mergeable on its own +// without exposing partial, unsigned-by-default behavior in between. +// +// This is intentionally a plain env var, not persisted config or a CLI flag: +// it exists only for the duration of the rollout and is expected to be +// removed once the feature ships, matching the existing TOOLHIVE_DEV / +// TOOLHIVE_REMOTE_HEALTHCHECKS precedent for staged/dev-only behavior. +const LockFileEnvVar = "TOOLHIVE_SKILLS_LOCK_ENABLED" + +// LockFileFeatureEnabled reports whether the project-level skills lock file +// feature is enabled for this process. +func LockFileFeatureEnabled() bool { + return strings.EqualFold(os.Getenv(LockFileEnvVar), "true") +} diff --git a/pkg/skills/feature_gate_test.go b/pkg/skills/feature_gate_test.go new file mode 100644 index 0000000000..cac0d8e4d9 --- /dev/null +++ b/pkg/skills/feature_gate_test.go @@ -0,0 +1,33 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package skills + +import "testing" + +func TestLockFileFeatureEnabled(t *testing.T) { + tests := []struct { + name string + value string + want bool + }{ + {name: "unset defaults to disabled", value: "", want: false}, + {name: "true enables", value: "true", want: true}, + {name: "mixed case true enables", value: "True", want: true}, + {name: "false stays disabled", value: "false", want: false}, + {name: "arbitrary value stays disabled", value: "1", want: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.value == "" { + t.Setenv(LockFileEnvVar, "") + } else { + t.Setenv(LockFileEnvVar, tt.value) + } + if got := LockFileFeatureEnabled(); got != tt.want { + t.Errorf("LockFileFeatureEnabled() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/skills/options.go b/pkg/skills/options.go index 6eb0525440..7291a18307 100644 --- a/pkg/skills/options.go +++ b/pkg/skills/options.go @@ -37,6 +37,23 @@ type InstallOptions struct { Reference string `json:"-"` // Digest is the OCI digest for upgrade detection. Digest string `json:"-"` + // LockSource overrides the value recorded as the lock entry's Source. When + // empty, the entry's Source is Name as given by the caller before any + // internal resolution. Set by Sync/Upgrade, which pass an already-resolved + // Name that must not overwrite the entry's original Source. Internal use + // only — NOT exposed via HTTP API. + LockSource string `json:"-"` + // RequiredByParent is set when this install is a transitively materialized + // dependency (toolhive.requires) of another skill, naming that parent. + // Empty means the user explicitly requested this install. Internal use + // only — NOT exposed via HTTP API. + RequiredByParent string `json:"-"` + // Visited tracks skill names already materialized in this dependency + // tree, preventing infinite recursion on a requires cycle. Left nil by + // external callers; Install initializes it on first entry and threads it + // through recursive dependency installs. Internal use only — NOT exposed + // via HTTP API. + Visited map[string]struct{} `json:"-"` } // InstallResult contains the outcome of an Install operation. @@ -53,6 +70,12 @@ type UninstallOptions struct { Scope Scope `json:"scope,omitempty"` // ProjectRoot is the project root path for project-scoped skills. ProjectRoot string `json:"project_root,omitempty"` + // Visited tracks skill names already removed in this cascade-uninstall + // tree, preventing infinite recursion on a requiredBy cycle. Left nil by + // external callers; Uninstall initializes it on first entry and threads + // it through recursive cascade removals. Internal use only — NOT exposed + // via HTTP API. + Visited map[string]struct{} `json:"-"` } // InfoOptions configures the behavior of the Info operation. diff --git a/pkg/skills/skillsvc/content_digest.go b/pkg/skills/skillsvc/content_digest.go new file mode 100644 index 0000000000..6f9364cf49 --- /dev/null +++ b/pkg/skills/skillsvc/content_digest.go @@ -0,0 +1,73 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package skillsvc + +import ( + "fmt" + "os" + + "github.com/stacklok/toolhive/pkg/skills" + "github.com/stacklok/toolhive/pkg/skills/lockfile" +) + +// skillMDFileName is the well-known skill definition file every installed +// skill directory contains. +const skillMDFileName = "SKILL.md" + +// computeContentDigest hashes the on-disk file set for an installed skill, +// for lock file integrity verification. Every client directory a skill is +// installed into is written from the same source, so any one of them is +// representative; the first client in the skill's Clients list is used. +func computeContentDigest(pathResolver skills.PathResolver, sk skills.InstalledSkill) (string, error) { + dir, err := installedSkillDir(pathResolver, sk) + if err != nil { + return "", err + } + digest, err := lockfile.ContentDigestFromDir(dir) + if err != nil { + return "", fmt.Errorf("computing content digest: %w", err) + } + return digest, nil +} + +// readSkillMD reads and parses SKILL.md from an installed skill's directory, +// for discovering toolhive.requires dependencies at install time. The +// artifact source (OCI labels or a git checkout) is not normalized to expose +// requires before extraction, but every installed skill has SKILL.md on disk +// once extraction succeeds, so reading it back is the uniform way to recover +// dependency declarations regardless of source type. +func readSkillMD(pathResolver skills.PathResolver, sk skills.InstalledSkill) (*skills.ParseResult, error) { + dir, err := installedSkillDir(pathResolver, sk) + if err != nil { + return nil, err + } + root, err := os.OpenRoot(dir) + if err != nil { + return nil, fmt.Errorf("opening skill directory: %w", err) + } + defer func() { _ = root.Close() }() + + content, err := root.ReadFile(skillMDFileName) + if err != nil { + return nil, fmt.Errorf("reading %s: %w", skillMDFileName, err) + } + parsed, err := skills.ParseSkillMD(content) + if err != nil { + return nil, fmt.Errorf("parsing %s: %w", skillMDFileName, err) + } + return parsed, nil +} + +// installedSkillDir resolves the filesystem path for one of sk's installed +// clients, used as a representative directory for reading back on-disk state. +func installedSkillDir(pathResolver skills.PathResolver, sk skills.InstalledSkill) (string, error) { + if len(sk.Clients) == 0 { + return "", fmt.Errorf("skill %q has no installed clients", sk.Metadata.Name) + } + dir, err := pathResolver.GetSkillPath(sk.Clients[0], sk.Metadata.Name, sk.Scope, sk.ProjectRoot) + if err != nil { + return "", fmt.Errorf("resolving skill path: %w", err) + } + return dir, nil +} diff --git a/pkg/skills/skillsvc/install.go b/pkg/skills/skillsvc/install.go index 028f78530f..dd59e34092 100644 --- a/pkg/skills/skillsvc/install.go +++ b/pkg/skills/skillsvc/install.go @@ -22,6 +22,13 @@ import ( // to disk and a full installation record is created. Without LayerData, a // pending record is created. func (s *service) Install(ctx context.Context, opts skills.InstallOptions) (*skills.InstallResult, error) { + // Captured before any internal resolution (version splicing, registry + // lookup, git/OCI dispatch) mutates a *local* copy of opts.Name in the + // functions below. This is the RFC THV-0080 lock entry "source": exactly + // what the caller asked for, preserved verbatim so upgrade can re-resolve + // the same input later. + originalName := opts.Name + scope, projectRoot, err := normalizeProjectRoot(opts.Scope, opts.ProjectRoot) if err != nil { return nil, err @@ -38,7 +45,7 @@ func (s *service) Install(ctx context.Context, opts skills.InstallOptions) (*ski if err != nil { return nil, err } - return s.installAndRegister(ctx, result, opts.Group, result.Skill.Metadata.Name, scope, opts.ProjectRoot) + return s.installAndRegister(ctx, opts, originalName, result, opts.Group, result.Skill.Metadata.Name, scope) } // When the caller supplies `version` separately and the name is a tag-less @@ -62,7 +69,7 @@ func (s *service) Install(ctx context.Context, opts skills.InstallOptions) (*ski if isOCI { result, ociErr := s.installFromOCI(ctx, opts, scope, ref) if ociErr == nil { - return s.installAndRegister(ctx, result, opts.Group, opts.Name, scope, opts.ProjectRoot) + return s.installAndRegister(ctx, opts, originalName, result, opts.Group, opts.Name, scope) } // OCI pull failed — fall back to registry lookup for names that look // like a qualified "namespace/name". Names that are unambiguously OCI @@ -77,7 +84,7 @@ func (s *service) Install(ctx context.Context, opts skills.InstallOptions) (*ski return nil, regErr } if resolved != nil { - return s.installFromResolvedRegistry(ctx, opts, scope, resolved) + return s.installFromResolvedRegistry(ctx, opts, originalName, scope, resolved) } return nil, ociErr } @@ -87,7 +94,7 @@ func (s *service) Install(ctx context.Context, opts skills.InstallOptions) (*ski return nil, httperr.WithCode(err, http.StatusBadRequest) } - return s.installByName(ctx, opts, scope) + return s.installByName(ctx, opts, originalName, scope) } // installByName handles installation for a validated plain skill name. It @@ -95,6 +102,7 @@ func (s *service) Install(ctx context.Context, opts skills.InstallOptions) (*ski func (s *service) installByName( ctx context.Context, opts skills.InstallOptions, + originalName string, scope skills.Scope, ) (*skills.InstallResult, error) { unlock := s.locks.lock(opts.Name, scope, opts.ProjectRoot) @@ -125,7 +133,7 @@ func (s *service) installByName( unlock() locked = false - return s.installFromRegistryLookup(ctx, opts, scope) + return s.installFromRegistryLookup(ctx, opts, originalName, scope) } // resolved: opts hydrated, fall through to installWithExtraction } @@ -134,7 +142,7 @@ func (s *service) installByName( if err != nil { return nil, err } - return s.installAndRegister(ctx, result, opts.Group, opts.Name, scope, opts.ProjectRoot) + return s.installAndRegister(ctx, opts, originalName, result, opts.Group, opts.Name, scope) } // installFromRegistryLookup resolves a plain skill name via the registry and @@ -142,6 +150,7 @@ func (s *service) installByName( func (s *service) installFromRegistryLookup( ctx context.Context, opts skills.InstallOptions, + originalName string, scope skills.Scope, ) (*skills.InstallResult, error) { resolved, regErr := s.resolveFromRegistry(opts.Name) @@ -149,7 +158,7 @@ func (s *service) installFromRegistryLookup( return nil, regErr } if resolved != nil { - return s.installFromResolvedRegistry(ctx, opts, scope, resolved) + return s.installFromResolvedRegistry(ctx, opts, originalName, scope, resolved) } return nil, httperr.WithCode( @@ -165,6 +174,7 @@ func (s *service) installFromRegistryLookup( func (s *service) installFromResolvedRegistry( ctx context.Context, opts skills.InstallOptions, + originalName string, scope skills.Scope, resolved *registryResolveResult, ) (*skills.InstallResult, error) { @@ -179,7 +189,7 @@ func (s *service) installFromResolvedRegistry( // Use the skill name extracted from the artifact, not opts.Name which // holds the OCI ref string. installFromOCI mutates its own copy of opts // (Go pass-by-value), so the caller never sees the updated name. - return s.installAndRegister(ctx, result, opts.Group, result.Skill.Metadata.Name, scope, opts.ProjectRoot) + return s.installAndRegister(ctx, opts, originalName, result, opts.Group, result.Skill.Metadata.Name, scope) case resolved.GitURL != "": slog.Info("resolved skill from registry (git)", "name", opts.Name, "git_url", resolved.GitURL) opts.Name = resolved.GitURL @@ -187,7 +197,7 @@ func (s *service) installFromResolvedRegistry( if gitErr != nil { return nil, gitErr } - return s.installAndRegister(ctx, result, opts.Group, result.Skill.Metadata.Name, scope, opts.ProjectRoot) + return s.installAndRegister(ctx, opts, originalName, result, opts.Group, result.Skill.Metadata.Name, scope) } return nil, httperr.WithCode( fmt.Errorf("skill %q resolved from registry but has no installable package", opts.Name), @@ -208,24 +218,43 @@ func (s *service) registerSkillInGroup(ctx context.Context, groupName string, sk return groups.AddSkillToGroup(ctx, s.groupManager, groupName, skillName) } -// installAndRegister registers the just-installed skill in the target group. -// If group registration fails, the DB record is rolled back so that a retry +// installAndRegister registers the just-installed skill in the target group +// and, for project-scope installs with the lock file feature enabled (see +// skills.LockFileFeatureEnabled), records it — and any toolhive.requires +// dependencies — in the project's toolhive.lock.yaml. If group registration +// or the lock write fails, the DB record is rolled back so that a retry // starts fresh rather than leaving the system in an inconsistent state (skill -// installed but not in the expected group). +// installed but not in the expected group, or installed without a pin). func (s *service) installAndRegister( ctx context.Context, + opts skills.InstallOptions, + originalName string, result *skills.InstallResult, groupName string, skillName string, scope skills.Scope, - projectRoot string, ) (*skills.InstallResult, error) { + rollback := func() { _ = s.store.Delete(ctx, skillName, scope, opts.ProjectRoot) } + if err := s.registerSkillInGroup(ctx, groupName, skillName); err != nil { // Best-effort rollback: remove the DB record so retries start fresh. // Files on disk are left in place; a fresh install will detect them // and either overwrite (force) or return a conflict. - _ = s.store.Delete(ctx, skillName, scope, projectRoot) + rollback() return nil, fmt.Errorf("registering skill in group: %w", err) } + + if scope == skills.ScopeProject && skills.LockFileFeatureEnabled() { + updated, err := s.recordLockState(ctx, opts, originalName, result.Skill) + if err != nil { + rollback() + return nil, httperr.WithCode( + fmt.Errorf("recording skill in project lock file: %w", err), + http.StatusInternalServerError, + ) + } + result.Skill = updated + } + return result, nil } diff --git a/pkg/skills/skillsvc/lock.go b/pkg/skills/skillsvc/lock.go new file mode 100644 index 0000000000..dd90fad6d1 --- /dev/null +++ b/pkg/skills/skillsvc/lock.go @@ -0,0 +1,163 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package skillsvc + +import ( + "context" + "fmt" + "slices" + + "github.com/stacklok/toolhive/pkg/skills" + "github.com/stacklok/toolhive/pkg/skills/lockfile" +) + +// recordLockState updates opts.ProjectRoot's lock file to reflect a +// just-completed project-scope install: an entry for sk, plus recursively +// materialized entries for any toolhive.requires dependencies declared in +// its SKILL.md. It also marks sk as lock-managed in the store. Callers must +// only invoke this for project-scope installs with the lock file feature +// enabled (see skills.LockFileFeatureEnabled) — sk is returned updated so the +// caller can reflect the Managed flag back to its own result. +func (s *service) recordLockState( + ctx context.Context, + opts skills.InstallOptions, + originalName string, + sk skills.InstalledSkill, +) (skills.InstalledSkill, error) { + contentDigest, err := computeContentDigest(s.pathResolver, sk) + if err != nil { + return sk, fmt.Errorf("computing content digest: %w", err) + } + + source := opts.LockSource + if source == "" { + source = originalName + } + if err := recordLockEntry(sk.ProjectRoot, lockEntryInput{ + Name: sk.Metadata.Name, + Version: sk.Metadata.Version, + Source: source, + ResolvedReference: sk.Reference, + Digest: sk.Digest, + ContentDigest: contentDigest, + RequiredByParent: opts.RequiredByParent, + }); err != nil { + return sk, fmt.Errorf("writing lock entry: %w", err) + } + + if !sk.Managed { + sk.Managed = true + if err := s.store.Update(ctx, sk); err != nil { + return sk, fmt.Errorf("marking skill as lock-managed: %w", err) + } + } + + if err := s.materializeDependencies(ctx, opts, sk); err != nil { + return sk, fmt.Errorf("materializing dependencies: %w", err) + } + return sk, nil +} + +// materializeDependencies installs every toolhive.requires dependency +// declared by sk's SKILL.md, recursively (a dependency may itself declare +// further dependencies). A Visited set threaded through opts prevents +// infinite recursion on a requires cycle; skills.MaxDependencies bounds the +// total number of skills materialized across the whole tree, not just the +// direct dependency list of a single skill. +func (s *service) materializeDependencies( + ctx context.Context, + opts skills.InstallOptions, + sk skills.InstalledSkill, +) error { + parsed, err := readSkillMD(s.pathResolver, sk) + if err != nil { + return err + } + if len(parsed.Requires) == 0 { + return nil + } + + visited := opts.Visited + if visited == nil { + visited = make(map[string]struct{}) + } + visited[sk.Metadata.Name] = struct{}{} + + for _, dep := range parsed.Requires { + if _, seen := visited[dep.Reference]; seen { + continue + } + if len(visited) >= skills.MaxDependencies { + return fmt.Errorf("dependency tree for %q exceeds maximum of %d skills", + sk.Metadata.Name, skills.MaxDependencies) + } + visited[dep.Reference] = struct{}{} + + depOpts := skills.InstallOptions{ + Name: dep.Reference, + Scope: sk.Scope, + ProjectRoot: sk.ProjectRoot, + Clients: sk.Clients, + RequiredByParent: sk.Metadata.Name, + Visited: visited, + } + if _, err := s.Install(ctx, depOpts); err != nil { + return fmt.Errorf("installing dependency %q (required by %q): %w", dep.Reference, sk.Metadata.Name, err) + } + } + return nil +} + +// lockEntryInput carries the fields recordLockEntry needs to upsert a lock +// entry, decoupled from skillsvc's own InstallOptions/InstalledSkill shapes. +type lockEntryInput struct { + Name string + Version string + Source string + ResolvedReference string + Digest string + ContentDigest string + // RequiredByParent names the parent skill when this entry is a + // transitively materialized dependency. Empty means the entry is + // explicit (a direct, user-requested install). + RequiredByParent string +} + +// recordLockEntry upserts a single entry into projectRoot's lock file. When +// an entry for the same name already exists, its RequiredBy list is merged +// (not overwritten) so a dependency shared by multiple parents keeps every +// parent, and Explicit is sticky once true. +func recordLockEntry(projectRoot string, in lockEntryInput) error { + root, err := lockfile.OpenRoot(projectRoot) + if err != nil { + return err + } + return lockfile.Update(root, func(lf *lockfile.Lockfile) error { + entry := lockfile.Entry{ + Name: in.Name, + Version: in.Version, + Source: in.Source, + ResolvedReference: in.ResolvedReference, + Digest: in.Digest, + ContentDigest: in.ContentDigest, + Explicit: in.RequiredByParent == "", + } + if existing, ok := lf.Get(in.Name); ok { + entry.RequiredBy = existing.RequiredBy + entry.Explicit = entry.Explicit || existing.Explicit + } + if in.RequiredByParent != "" { + entry.RequiredBy = appendUnique(entry.RequiredBy, in.RequiredByParent) + } + lf.Upsert(entry) + return nil + }) +} + +func appendUnique(list []string, value string) []string { + if slices.Contains(list, value) { + return list + } + return append(list, value) +} diff --git a/pkg/skills/skillsvc/lock_test.go b/pkg/skills/skillsvc/lock_test.go new file mode 100644 index 0000000000..7eee8e6525 --- /dev/null +++ b/pkg/skills/skillsvc/lock_test.go @@ -0,0 +1,301 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package skillsvc + +import ( + "context" + "fmt" + "os" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + + "github.com/stacklok/toolhive/pkg/skills" + "github.com/stacklok/toolhive/pkg/skills/gitresolver" + gitmocks "github.com/stacklok/toolhive/pkg/skills/gitresolver/mocks" + "github.com/stacklok/toolhive/pkg/skills/lockfile" + skillsmocks "github.com/stacklok/toolhive/pkg/skills/mocks" + "github.com/stacklok/toolhive/pkg/storage/sqlite" +) + +// newLockTestService builds a service backed by a real (temp-file) SQLite +// store and the real default Installer, so extracted files and lock file +// state can be inspected on disk exactly as they would be in production. +// Only the git resolver and path resolver are test doubles. +func newLockTestService(t *testing.T, gr *gitmocks.MockResolver) (skills.SkillService, string) { + t.Helper() + t.Setenv(skills.LockFileEnvVar, "true") + + dbPath := filepath.Join(t.TempDir(), "test.db") + db, err := sqlite.Open(t.Context(), dbPath) + require.NoError(t, err) + t.Cleanup(func() { _ = db.Close() }) + store := sqlite.NewSkillStore(db) + + projectRoot := makeProjectRoot(t) + installBase := filepath.Join(projectRoot, ".claude", "skills") + + ctrl := gomock.NewController(t) + pr := skillsmocks.NewMockPathResolver(ctrl) + pr.EXPECT().GetSkillPath(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes(). + DoAndReturn(func(_, skillName string, _ skills.Scope, _ string) (string, error) { + return filepath.Join(installBase, skillName), nil + }) + + svc := New(store, WithPathResolver(pr), WithGitResolver(gr)) + return svc, projectRoot +} + +// gitSkill builds SKILL.md content for a git-resolved test fixture, +// optionally declaring toolhive.requires dependencies. +func gitSkill(name string, requires ...string) []byte { + fm := fmt.Sprintf("---\nname: %s\ndescription: test skill\n", name) + if len(requires) > 0 { + fm += "toolhive.requires:\n" + for _, r := range requires { + fm += " - " + r + "\n" + } + } + fm += "---\n# " + name + "\n" + return []byte(fm) +} + +// gitRef builds the git:// reference and matching *gitresolver.GitReference +// for a fake repo named after the skill it resolves to. ParseGitReference +// normalizes to an "http://" clone URL (a pre-existing quirk unrelated to +// this package — see TestParseGitReference_RefAndSubdir in pkg/plugins). +func gitRef(name string) (string, string) { + return "git://github.com/test/" + name, "http://github.com/test/" + name +} + +// gitFixture is a single registered skill: its SKILL.md content and the +// parsed name/version installFromGit reads from SkillConfig (it trusts the +// resolver's parse rather than re-parsing Files itself). +type gitFixture struct { + name string + content []byte +} + +// fakeGitFixtures dispatches a MockResolver's Resolve calls by matching the +// reference URL against a fixed set of registered skills, each getting its +// own commit hash so digests differ. +type fakeGitFixtures struct { + skills map[string]gitFixture // keyed by URL +} + +func newGitResolverMock(t *testing.T) (*gitmocks.MockResolver, *fakeGitFixtures) { + t.Helper() + fx := &fakeGitFixtures{skills: make(map[string]gitFixture)} + gr := gitmocks.NewMockResolver(gomock.NewController(t)) + gr.EXPECT().Resolve(gomock.Any(), gomock.Any()).AnyTimes(). + DoAndReturn(func(_ context.Context, ref *gitresolver.GitReference) (*gitresolver.ResolveResult, error) { + fixture, ok := fx.skills[ref.URL] + if !ok { + return nil, fmt.Errorf("no fixture registered for %q", ref.URL) + } + return &gitresolver.ResolveResult{ + SkillConfig: &skills.ParseResult{Name: fixture.name}, + Files: []gitresolver.FileEntry{{Path: "SKILL.md", Content: fixture.content, Mode: 0644}}, + CommitHash: fmt.Sprintf("%040x", len(fixture.content)+len(ref.URL)), // deterministic, distinct per fixture + }, nil + }) + return gr, fx +} + +func (f *fakeGitFixtures) register(name string, content []byte) { + _, url := gitRef(name) + f.skills[url] = gitFixture{name: name, content: content} +} + +func readLockfile(t *testing.T, projectRoot string) *lockfile.Lockfile { + t.Helper() + root, err := lockfile.OpenRoot(projectRoot) + require.NoError(t, err) + lf, err := lockfile.Load(root) + require.NoError(t, err) + return lf +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestInstallProjectScope_LockFileDisabled_NoLockFileWritten(t *testing.T) { + gr, fx := newGitResolverMock(t) + fx.register("my-skill", gitSkill("my-skill")) + svc, projectRoot := newLockTestService(t, gr) + t.Setenv(skills.LockFileEnvVar, "false") // override newLockTestService's default + + ref, _ := gitRef("my-skill") + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + + _, err = os.Stat(filepath.Join(projectRoot, lockfile.FileName)) + assert.True(t, os.IsNotExist(err), "lock file must not be written when the feature is disabled") +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestInstallProjectScope_RecordsExplicitEntry(t *testing.T) { + gr, fx := newGitResolverMock(t) + fx.register("my-skill", gitSkill("my-skill")) + svc, projectRoot := newLockTestService(t, gr) + + ref, _ := gitRef("my-skill") + result, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + assert.True(t, result.Skill.Managed, "project-scope install must be marked lock-managed") + + lf := readLockfile(t, projectRoot) + entry, ok := lf.Get("my-skill") + require.True(t, ok, "expected a lock entry for my-skill") + assert.Equal(t, ref, entry.Source, "source must be exactly what the caller requested") + // For a direct git install, ResolvedReference is the same git:// URL: the + // package preserves it verbatim as the artifact's provenance reference, + // distinct from a registry-resolved install where Source (registry name) + // and ResolvedReference (the concrete git:// URL it resolved to) differ. + assert.Equal(t, ref, entry.ResolvedReference) + assert.NotEmpty(t, entry.Digest) + assert.NotEmpty(t, entry.ContentDigest, "contentDigest must be computed for a project-scope install") + assert.True(t, entry.Explicit) + assert.Empty(t, entry.RequiredBy) +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestInstallProjectScope_MaterializesDependency(t *testing.T) { + gr, fx := newGitResolverMock(t) + depRef, _ := gitRef("shared-dep") + fx.register("parent-skill", gitSkill("parent-skill", depRef)) + fx.register("shared-dep", gitSkill("shared-dep")) + svc, projectRoot := newLockTestService(t, gr) + + parentRef, _ := gitRef("parent-skill") + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: parentRef, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + + lf := readLockfile(t, projectRoot) + parent, ok := lf.Get("parent-skill") + require.True(t, ok) + assert.True(t, parent.Explicit) + + dep, ok := lf.Get("shared-dep") + require.True(t, ok, "dependency must be materialized and recorded") + assert.False(t, dep.Explicit) + assert.Equal(t, []string{"parent-skill"}, dep.RequiredBy) + assert.NotEmpty(t, dep.ContentDigest) +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestInstallProjectScope_SharedDependencyMergesRequiredBy(t *testing.T) { + gr, fx := newGitResolverMock(t) + depRef, _ := gitRef("shared-dep") + fx.register("parent-a", gitSkill("parent-a", depRef)) + fx.register("parent-b", gitSkill("parent-b", depRef)) + fx.register("shared-dep", gitSkill("shared-dep")) + svc, projectRoot := newLockTestService(t, gr) + + refA, _ := gitRef("parent-a") + refB, _ := gitRef("parent-b") + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: refA, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + _, err = svc.Install(t.Context(), skills.InstallOptions{ + Name: refB, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + + lf := readLockfile(t, projectRoot) + dep, ok := lf.Get("shared-dep") + require.True(t, ok) + assert.ElementsMatch(t, []string{"parent-a", "parent-b"}, dep.RequiredBy, + "a dependency shared by two parents must keep both, not overwrite on the second install") +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestMaterializeDependencies_RejectsTreeExceedingMaxDependencies(t *testing.T) { + gr, _ := newGitResolverMock(t) + svc, projectRoot := newLockTestService(t, gr) + svcImpl := svc.(*service) //nolint:forcetypeassert // white-box test in the same package + + depRef, _ := gitRef("one-more-dep") + sk := skills.InstalledSkill{ + Metadata: skills.SkillMetadata{Name: "root-skill"}, + Scope: skills.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, + } + skillDir := filepath.Join(projectRoot, ".claude", "skills", sk.Metadata.Name) + require.NoError(t, os.MkdirAll(skillDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(skillDir, "SKILL.md"), gitSkill("root-skill", depRef), 0o644)) + + // Pre-fill Visited to the cap, as if this were deep in an already-large + // dependency tree; the next never-seen dependency must trip the limit + // rather than silently keep expanding. + visited := make(map[string]struct{}, skills.MaxDependencies) + for i := range skills.MaxDependencies { + visited[fmt.Sprintf("seen-%d", i)] = struct{}{} + } + + err := svcImpl.materializeDependencies(t.Context(), skills.InstallOptions{Visited: visited}, sk) + require.Error(t, err) + assert.Contains(t, err.Error(), "exceeds maximum") +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestInstallProjectScope_DependencyCycleTerminates(t *testing.T) { + gr, fx := newGitResolverMock(t) + refA, _ := gitRef("skill-a") + refB, _ := gitRef("skill-b") + fx.register("skill-a", gitSkill("skill-a", refB)) + fx.register("skill-b", gitSkill("skill-b", refA)) // cycle: a -> b -> a + svc, projectRoot := newLockTestService(t, gr) + + done := make(chan error, 1) + go func() { + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: refA, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + done <- err + }() + select { + case err := <-done: + require.NoError(t, err) + case <-time.After(10 * time.Second): + t.Fatal("timeout: dependency cycle did not terminate") + } + + lf := readLockfile(t, projectRoot) + a, ok := lf.Get("skill-a") + require.True(t, ok) + assert.True(t, a.Explicit) + b, ok := lf.Get("skill-b") + require.True(t, ok) + assert.Equal(t, []string{"skill-a"}, b.RequiredBy) +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestInstallProjectScope_LockWriteFailureRollsBackInstall(t *testing.T) { + gr, fx := newGitResolverMock(t) + fx.register("my-skill", gitSkill("my-skill")) + svc, projectRoot := newLockTestService(t, gr) + + // Replace the lock file location with a directory so writes to it fail. + require.NoError(t, os.MkdirAll(filepath.Join(projectRoot, lockfile.FileName), 0o755)) + + ref, _ := gitRef("my-skill") + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.Error(t, err, "install must fail when the lock file cannot be written") + + _, err = svc.Info(t.Context(), skills.InfoOptions{Name: "my-skill", Scope: skills.ScopeProject, ProjectRoot: projectRoot}) + require.Error(t, err, "the DB record must be rolled back so a retry starts fresh") +} diff --git a/pkg/skills/skillsvc/uninstall.go b/pkg/skills/skillsvc/uninstall.go index 4c74d8ec98..2d1da2a584 100644 --- a/pkg/skills/skillsvc/uninstall.go +++ b/pkg/skills/skillsvc/uninstall.go @@ -14,9 +14,13 @@ import ( "github.com/stacklok/toolhive-core/httperr" "github.com/stacklok/toolhive/pkg/groups" "github.com/stacklok/toolhive/pkg/skills" + "github.com/stacklok/toolhive/pkg/skills/lockfile" ) // Uninstall removes an installed skill and cleans up files for all clients. +// For a project-scope, lock-managed skill (see skills.LockFileFeatureEnabled), +// it also removes the skill's lock entry and cascades to any dependency that +// loses its last requiring parent as a result. func (s *service) Uninstall(ctx context.Context, opts skills.UninstallOptions) error { if err := skills.ValidateSkillName(opts.Name); err != nil { return httperr.WithCode(err, http.StatusBadRequest) @@ -29,6 +33,13 @@ func (s *service) Uninstall(ctx context.Context, opts skills.UninstallOptions) e scope = defaultScope(scope) opts.ProjectRoot = projectRoot + return s.uninstallOne(ctx, opts, scope) +} + +// uninstallOne performs a single skill's file/DB/group cleanup and, when +// applicable, its lock entry removal and cascade. It is called both for the +// top-level Uninstall request and recursively for cascade candidates. +func (s *service) uninstallOne(ctx context.Context, opts skills.UninstallOptions, scope skills.Scope) error { unlock := s.locks.lock(opts.Name, scope, opts.ProjectRoot) defer unlock() @@ -38,6 +49,41 @@ func (s *service) Uninstall(ctx context.Context, opts skills.UninstallOptions) e return err } + cleanupErrs := s.removeClientFiles(existing, opts, scope) + + if err := s.store.Delete(ctx, opts.Name, scope, opts.ProjectRoot); err != nil { + return err + } + + // Remove the skill from all groups — best-effort, same pattern as file cleanup. + if s.groupManager != nil { + if groupErr := groups.RemoveSkillFromAllGroups(ctx, s.groupManager, opts.Name); groupErr != nil { + cleanupErrs = append(cleanupErrs, fmt.Errorf("removing skill from groups: %w", groupErr)) + } + } + + // Lock file cleanup is best-effort, matching every other cleanup step + // here: the DB record and files are already gone by this point, so there + // is nothing left to roll back to on failure. + if scope == skills.ScopeProject && existing.Managed && skills.LockFileFeatureEnabled() { + if cascadeErr := s.removeLockEntryAndCascade(ctx, opts, scope); cascadeErr != nil { + cleanupErrs = append(cleanupErrs, fmt.Errorf("updating project lock file: %w", cascadeErr)) + } + } + + return errors.Join(cleanupErrs...) +} + +// removeClientFiles removes on-disk files for every client existing is +// installed for. It is best-effort: errors are collected rather than +// aborting, so cleanup proceeds as far as possible. +func (s *service) removeClientFiles( + existing skills.InstalledSkill, opts skills.UninstallOptions, scope skills.Scope, +) []error { + if s.pathResolver == nil { + return nil + } + // Determine the boundary directory for empty-parent cleanup. stopDir := opts.ProjectRoot if scope == skills.ScopeUser { @@ -46,36 +92,68 @@ func (s *service) Uninstall(ctx context.Context, opts skills.UninstallOptions) e } } - // Remove files for each client — best-effort: collect errors but don't - // abort on the first failure so we clean up as much as possible. - var cleanupErrs []error - if s.pathResolver != nil { - for _, clientType := range existing.Clients { - skillPath, pathErr := s.pathResolver.GetSkillPath(clientType, opts.Name, scope, opts.ProjectRoot) - if pathErr != nil { - cleanupErrs = append(cleanupErrs, fmt.Errorf("resolving path for client %q: %w", clientType, pathErr)) - continue - } - if rmErr := s.installer.Remove(skillPath); rmErr != nil { - cleanupErrs = append(cleanupErrs, fmt.Errorf("removing files for client %q: %w", clientType, rmErr)) - continue - } - if stopDir != "" { - skills.RemoveEmptyParents(filepath.Dir(skillPath), stopDir) - } + var errs []error + for _, clientType := range existing.Clients { + skillPath, pathErr := s.pathResolver.GetSkillPath(clientType, opts.Name, scope, opts.ProjectRoot) + if pathErr != nil { + errs = append(errs, fmt.Errorf("resolving path for client %q: %w", clientType, pathErr)) + continue + } + if rmErr := s.installer.Remove(skillPath); rmErr != nil { + errs = append(errs, fmt.Errorf("removing files for client %q: %w", clientType, rmErr)) + continue } + if stopDir != "" { + skills.RemoveEmptyParents(filepath.Dir(skillPath), stopDir) + } + } + return errs +} + +// removeLockEntryAndCascade removes opts.Name's lock entry and, for any +// dependency that consequently loses its last requiring parent (and is not +// itself explicit), uninstalls it too. A Visited set threaded through opts +// prevents infinite recursion on a requiredBy cycle in a hand-edited lock. +func (s *service) removeLockEntryAndCascade(ctx context.Context, opts skills.UninstallOptions, scope skills.Scope) error { + visited := opts.Visited + if visited == nil { + visited = make(map[string]struct{}) } + visited[opts.Name] = struct{}{} - if err := s.store.Delete(ctx, opts.Name, scope, opts.ProjectRoot); err != nil { + root, err := lockfile.OpenRoot(opts.ProjectRoot) + if err != nil { + return err + } + var cascadeCandidates []string + if err := lockfile.Update(root, func(lf *lockfile.Lockfile) error { + lf.Remove(opts.Name) + cascadeCandidates = lf.RemoveParentFromRequiredBy(opts.Name) + return nil + }); err != nil { return err } - // Remove the skill from all groups — best-effort, same pattern as file cleanup. - if s.groupManager != nil { - if groupErr := groups.RemoveSkillFromAllGroups(ctx, s.groupManager, opts.Name); groupErr != nil { - cleanupErrs = append(cleanupErrs, fmt.Errorf("removing skill from groups: %w", groupErr)) + var errs []error + for _, dep := range cascadeCandidates { + if _, seen := visited[dep]; seen { + continue + } + visited[dep] = struct{}{} + // A lock entry can reference a dependency whose install failed + // partway (or was already removed by hand); skip it rather than + // erroring on a missing DB record. + if _, getErr := s.store.Get(ctx, dep, scope, opts.ProjectRoot); getErr != nil { + continue + } + if uninstallErr := s.uninstallOne(ctx, skills.UninstallOptions{ + Name: dep, + Scope: scope, + ProjectRoot: opts.ProjectRoot, + Visited: visited, + }, scope); uninstallErr != nil { + errs = append(errs, fmt.Errorf("cascade-removing dependency %q: %w", dep, uninstallErr)) } } - - return errors.Join(cleanupErrs...) + return errors.Join(errs...) } diff --git a/pkg/skills/skillsvc/uninstall_cascade_test.go b/pkg/skills/skillsvc/uninstall_cascade_test.go new file mode 100644 index 0000000000..3bff0de3be --- /dev/null +++ b/pkg/skills/skillsvc/uninstall_cascade_test.go @@ -0,0 +1,151 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package skillsvc + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive/pkg/skills" +) + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestUninstall_CascadesToOrphanedDependency(t *testing.T) { + gr, fx := newGitResolverMock(t) + depRef, _ := gitRef("shared-dep") + fx.register("parent-skill", gitSkill("parent-skill", depRef)) + fx.register("shared-dep", gitSkill("shared-dep")) + svc, projectRoot := newLockTestService(t, gr) + + parentRef, _ := gitRef("parent-skill") + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: parentRef, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + _, ok := readLockfile(t, projectRoot).Get("shared-dep") + require.True(t, ok, "expected shared-dep to be materialized before uninstalling parent-skill") + + require.NoError(t, svc.Uninstall(t.Context(), skills.UninstallOptions{ + Name: "parent-skill", Scope: skills.ScopeProject, ProjectRoot: projectRoot, + })) + + lf := readLockfile(t, projectRoot) + _, ok = lf.Get("parent-skill") + assert.False(t, ok, "parent's own lock entry must be removed") + _, ok = lf.Get("shared-dep") + assert.False(t, ok, "an orphaned, non-explicit dependency must cascade-uninstall") + + _, err = svc.Info(t.Context(), skills.InfoOptions{Name: "shared-dep", Scope: skills.ScopeProject, ProjectRoot: projectRoot}) + require.Error(t, err, "cascade must remove the dependency's DB record and files too") +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestUninstall_SharedDependencySurvivesOneParentRemoval(t *testing.T) { + gr, fx := newGitResolverMock(t) + depRef, _ := gitRef("shared-dep") + fx.register("parent-a", gitSkill("parent-a", depRef)) + fx.register("parent-b", gitSkill("parent-b", depRef)) + fx.register("shared-dep", gitSkill("shared-dep")) + svc, projectRoot := newLockTestService(t, gr) + + refA, _ := gitRef("parent-a") + refB, _ := gitRef("parent-b") + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: refA, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + _, err = svc.Install(t.Context(), skills.InstallOptions{ + Name: refB, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + + require.NoError(t, svc.Uninstall(t.Context(), skills.UninstallOptions{ + Name: "parent-a", Scope: skills.ScopeProject, ProjectRoot: projectRoot, + })) + + lf := readLockfile(t, projectRoot) + dep, ok := lf.Get("shared-dep") + require.True(t, ok, "dependency must survive while parent-b still requires it") + assert.Equal(t, []string{"parent-b"}, dep.RequiredBy) + + _, err = svc.Info(t.Context(), skills.InfoOptions{Name: "shared-dep", Scope: skills.ScopeProject, ProjectRoot: projectRoot}) + require.NoError(t, err, "the dependency's DB record and files must remain") +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestUninstall_ExplicitDependencyIsNeverCascaded(t *testing.T) { + gr, fx := newGitResolverMock(t) + depRef, _ := gitRef("also-explicit") + fx.register("parent-skill", gitSkill("parent-skill", depRef)) + fx.register("also-explicit", gitSkill("also-explicit")) + svc, projectRoot := newLockTestService(t, gr) + + // Install the dependency explicitly first, then install a parent that + // also requires it. + explicitRef, _ := gitRef("also-explicit") + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: explicitRef, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + parentRef, _ := gitRef("parent-skill") + _, err = svc.Install(t.Context(), skills.InstallOptions{ + Name: parentRef, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + + lf := readLockfile(t, projectRoot) + dep, ok := lf.Get("also-explicit") + require.True(t, ok) + assert.True(t, dep.Explicit, "an entry installed explicitly stays explicit even once also required by a parent") + assert.Equal(t, []string{"parent-skill"}, dep.RequiredBy) + + require.NoError(t, svc.Uninstall(t.Context(), skills.UninstallOptions{ + Name: "parent-skill", Scope: skills.ScopeProject, ProjectRoot: projectRoot, + })) + + lf = readLockfile(t, projectRoot) + dep, ok = lf.Get("also-explicit") + require.True(t, ok, "an explicit entry must never be cascade-removed") + assert.Empty(t, dep.RequiredBy) + + _, err = svc.Info(t.Context(), skills.InfoOptions{Name: "also-explicit", Scope: skills.ScopeProject, ProjectRoot: projectRoot}) + require.NoError(t, err) +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestUninstall_CascadeCycleTerminates(t *testing.T) { + gr, fx := newGitResolverMock(t) + refA, _ := gitRef("skill-a") + refB, _ := gitRef("skill-b") + fx.register("skill-a", gitSkill("skill-a", refB)) + fx.register("skill-b", gitSkill("skill-b", refA)) + svc, projectRoot := newLockTestService(t, gr) + + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: refA, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + + done := make(chan error, 1) + go func() { + done <- svc.Uninstall(t.Context(), skills.UninstallOptions{ + Name: "skill-a", Scope: skills.ScopeProject, ProjectRoot: projectRoot, + }) + }() + select { + case err := <-done: + require.NoError(t, err) + case <-time.After(10 * time.Second): + t.Fatal("timeout: cascade uninstall did not terminate on a requiredBy cycle") + } + + lf := readLockfile(t, projectRoot) + _, ok := lf.Get("skill-a") + assert.False(t, ok) + _, ok = lf.Get("skill-b") + assert.False(t, ok, "skill-b lost its only (cyclic) parent and must cascade too") +} diff --git a/test/e2e/api_helpers.go b/test/e2e/api_helpers.go index 7b509a2032..7612114f50 100644 --- a/test/e2e/api_helpers.go +++ b/test/e2e/api_helpers.go @@ -27,6 +27,10 @@ type ServerConfig struct { StartTimeout time.Duration RequestTimeout time.Duration DebugMode bool + // ExtraEnv adds "KEY=VALUE" environment variables to the `thv serve` + // subprocess, for tests that need to opt into experimental or + // feature-gated behavior without changing it for every other E2E test. + ExtraEnv []string } // NewServerConfig creates a new API server configuration with defaults @@ -104,7 +108,7 @@ func NewServer(config *ServerConfig) (*Server, error) { "TOOLHIVE_DEV=true", fmt.Sprintf("XDG_CONFIG_HOME=%s", tempXdgConfigHome), fmt.Sprintf("HOME=%s", tempHome), - }, cmd.Env...) + }, config.ExtraEnv...) cmd.Stdout = &stdout cmd.Stderr = &stderr diff --git a/test/e2e/api_skills_test.go b/test/e2e/api_skills_test.go index 9ca6d33c1a..7aa25c7ac8 100644 --- a/test/e2e/api_skills_test.go +++ b/test/e2e/api_skills_test.go @@ -9,6 +9,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "net/url" "os" "path/filepath" "time" @@ -17,6 +18,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/stacklok/toolhive/pkg/skills/lockfile" "github.com/stacklok/toolhive/test/e2e" ) @@ -1048,3 +1050,127 @@ var _ = Describe("Skills API", Label("api", "api-registry", "skills", "e2e"), fu }) }) }) + +// makeE2EProjectRoot creates a resolved temp dir with a .git directory, +// satisfying the project-root validation the lock file requires. +func makeE2EProjectRoot() string { + dir := GinkgoT().TempDir() + resolved, err := filepath.EvalSymlinks(dir) + ExpectWithOffset(1, err).ToNot(HaveOccurred()) + ExpectWithOffset(1, os.MkdirAll(filepath.Join(resolved, ".git"), 0o755)).To(Succeed()) + return resolved +} + +func uninstallScopedSkill(server *e2e.Server, name, scope, projectRoot string) *http.Response { + u := fmt.Sprintf("%s/api/v1beta/skills/%s?scope=%s&project_root=%s", + server.BaseURL(), name, scope, url.QueryEscape(projectRoot)) + req, err := http.NewRequest(http.MethodDelete, u, nil) + ExpectWithOffset(1, err).ToNot(HaveOccurred()) + resp, err := http.DefaultClient.Do(req) + ExpectWithOffset(1, err).ToNot(HaveOccurred()) + return resp +} + +// buildAndPushSkill creates a skill directory, builds it, and pushes it to +// ociRegistry, returning the OCI reference. +func buildAndPushSkill(server *e2e.Server, ociRegistry *httptest.Server, skillName, description string) string { + ociRef := fmt.Sprintf("%s/e2e-test/%s:v0.1.0", ociRegistry.Listener.Addr().String(), skillName) + + skillDir := createTestSkillDir(skillName, description) + buildResp := buildSkill(server, skillDir, ociRef) + defer buildResp.Body.Close() + ExpectWithOffset(1, buildResp.StatusCode).To(Equal(http.StatusOK)) + + pushResp := pushSkill(server, ociRef) + defer pushResp.Body.Close() + ExpectWithOffset(1, pushResp.StatusCode).To(Equal(http.StatusNoContent)) + + return ociRef +} + +// This RFC THV-0080 feature is gated behind TOOLHIVE_SKILLS_LOCK_ENABLED +// while it lands across a stack of PRs (see skills.LockFileFeatureEnabled), +// so this Describe block runs its own server with the gate turned on rather +// than sharing the "Skills API" block's default-off server above. +var _ = Describe("Project-scope skills lock file (RFC THV-0080)", Label("api", "api-registry", "skills", "skills-lock", "e2e"), func() { + var ( + config *e2e.ServerConfig + apiServer *e2e.Server + ) + + BeforeEach(func() { + config = e2e.NewServerConfig() + config.ExtraEnv = []string{"TOOLHIVE_SKILLS_LOCK_ENABLED=true"} + apiServer = e2e.StartServer(config) + }) + + It("records an installed skill in the project's toolhive.lock.yaml", func() { + projectRoot := makeE2EProjectRoot() + skillName := "lock-e2e-skill" + + By("Starting an in-process OCI registry and pushing a test skill") + ociRegistry := httptest.NewServer(registry.New()) + DeferCleanup(ociRegistry.Close) + ociRef := buildAndPushSkill(apiServer, ociRegistry, skillName, "A skill for lock file E2E testing") + + By("Installing the skill into the project") + installResp := installSkill(apiServer, installSkillRequest{ + Name: ociRef, Scope: "project", ProjectRoot: projectRoot, + }) + defer installResp.Body.Close() + Expect(installResp.StatusCode).To(Equal(http.StatusCreated)) + var installResult installSkillResponse + Expect(json.NewDecoder(installResp.Body).Decode(&installResult)).To(Succeed()) + + By("Verifying the lock file was written with the expected entry") + root, err := lockfile.OpenRoot(projectRoot) + Expect(err).ToNot(HaveOccurred()) + lf, err := lockfile.Load(root) + Expect(err).ToNot(HaveOccurred()) + entry, ok := lf.Get(skillName) + Expect(ok).To(BeTrue(), "expected a lock entry for %q", skillName) + Expect(entry.Source).To(Equal(ociRef)) + Expect(entry.Digest).ToNot(BeEmpty()) + Expect(entry.ContentDigest).ToNot(BeEmpty()) + Expect(entry.Explicit).To(BeTrue()) + + By("Cleaning up") + cleanupResp := uninstallScopedSkill(apiServer, skillName, "project", projectRoot) + defer cleanupResp.Body.Close() + }) + + It("removes the lock entry when the skill is uninstalled", func() { + projectRoot := makeE2EProjectRoot() + skillName := "lock-e2e-uninstall-skill" + + By("Starting an in-process OCI registry and pushing a test skill") + ociRegistry := httptest.NewServer(registry.New()) + DeferCleanup(ociRegistry.Close) + ociRef := buildAndPushSkill(apiServer, ociRegistry, skillName, "A skill for lock file uninstall E2E testing") + + By("Installing the skill into the project") + installResp := installSkill(apiServer, installSkillRequest{ + Name: ociRef, Scope: "project", ProjectRoot: projectRoot, + }) + defer installResp.Body.Close() + Expect(installResp.StatusCode).To(Equal(http.StatusCreated)) + + root, err := lockfile.OpenRoot(projectRoot) + Expect(err).ToNot(HaveOccurred()) + lf, err := lockfile.Load(root) + Expect(err).ToNot(HaveOccurred()) + _, ok := lf.Get(skillName) + Expect(ok).To(BeTrue(), "expected a lock entry before uninstall") + + By("Uninstalling the skill") + uninstallResp := uninstallScopedSkill(apiServer, skillName, "project", projectRoot) + defer uninstallResp.Body.Close() + Expect(uninstallResp.StatusCode).To(Equal(http.StatusNoContent)) + + By("Verifying the lock entry was removed") + lf, err = lockfile.Load(root) + Expect(err).ToNot(HaveOccurred()) + _, ok = lf.Get(skillName) + Expect(ok).To(BeFalse(), "expected the lock entry to be removed after uninstall") + }) +}) From 71b314b58cea997b2cb8637dab8d1a0bad34d7a3 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Tue, 21 Jul 2026 17:28:40 +0200 Subject: [PATCH 2/4] Fix CI failure: gitRef test fixture hardcoded http scheme gitresolver.ParseGitReference only uses "http://" when TOOLHIVE_DEV=true; the correct default (and what GitHub Actions CI runners use, since they don't set that var) is "https://". The gitRef test helper hardcoded "http://" for the fixture registration key, so every test using it failed in CI with "no fixture registered for https://...". Compute the scheme the same way the resolver does instead of hardcoding either one. --- pkg/skills/skillsvc/lock_test.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/pkg/skills/skillsvc/lock_test.go b/pkg/skills/skillsvc/lock_test.go index 7eee8e6525..cc57b6edc5 100644 --- a/pkg/skills/skillsvc/lock_test.go +++ b/pkg/skills/skillsvc/lock_test.go @@ -8,6 +8,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "testing" "time" @@ -67,10 +68,16 @@ func gitSkill(name string, requires ...string) []byte { // gitRef builds the git:// reference and matching *gitresolver.GitReference // for a fake repo named after the skill it resolves to. ParseGitReference -// normalizes to an "http://" clone URL (a pre-existing quirk unrelated to -// this package — see TestParseGitReference_RefAndSubdir in pkg/plugins). +// picks "http://" instead of "https://" only when TOOLHIVE_DEV=true (see +// gitresolver.isDevMode); this mirrors that so the fixture URL matches +// whatever ParseGitReference actually produces in the environment the test +// runs in, rather than hardcoding one scheme. func gitRef(name string) (string, string) { - return "git://github.com/test/" + name, "http://github.com/test/" + name + scheme := "https" + if strings.EqualFold(os.Getenv("TOOLHIVE_DEV"), "true") { + scheme = "http" + } + return "git://github.com/test/" + name, scheme + "://github.com/test/" + name } // gitFixture is a single registered skill: its SKILL.md content and the From f723ac4a919e4d36a90d8136b7b1d8492c3bb032 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Tue, 21 Jul 2026 19:13:56 +0200 Subject: [PATCH 3/4] Fix install rollback and dependency cycle detection gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit installAndRegister's rollback only deleted the DB record, so a dependency-materialization failure after the parent's own lock entry had already been written left toolhive.lock.yaml claiming a pin that Info() couldn't find. Also key materializeDependencies' cycle-check visited set by the reference a skill was installed with instead of its resolved name, matching how dependency edges reference it — the mismatch let a cycle through unresolved names re-materialize a skill as its own dependency and corrupt its RequiredBy list. --- pkg/skills/skillsvc/install.go | 20 +++++++++++--- pkg/skills/skillsvc/lock.go | 13 +++++++-- pkg/skills/skillsvc/lock_test.go | 46 +++++++++++++++++++++++++++++++- 3 files changed, 72 insertions(+), 7 deletions(-) diff --git a/pkg/skills/skillsvc/install.go b/pkg/skills/skillsvc/install.go index dd59e34092..e95781c5c7 100644 --- a/pkg/skills/skillsvc/install.go +++ b/pkg/skills/skillsvc/install.go @@ -14,6 +14,7 @@ import ( "github.com/stacklok/toolhive/pkg/groups" "github.com/stacklok/toolhive/pkg/skills" "github.com/stacklok/toolhive/pkg/skills/gitresolver" + "github.com/stacklok/toolhive/pkg/skills/lockfile" ) // Install installs a skill. When the Name field contains an OCI reference @@ -222,9 +223,13 @@ func (s *service) registerSkillInGroup(ctx context.Context, groupName string, sk // and, for project-scope installs with the lock file feature enabled (see // skills.LockFileFeatureEnabled), records it — and any toolhive.requires // dependencies — in the project's toolhive.lock.yaml. If group registration -// or the lock write fails, the DB record is rolled back so that a retry -// starts fresh rather than leaving the system in an inconsistent state (skill -// installed but not in the expected group, or installed without a pin). +// or the lock write fails, the DB record and any lock entry already written +// for this skill are rolled back so that a retry starts fresh rather than +// leaving the system in an inconsistent state (skill installed but not in +// the expected group, or the lock file claiming a pin that Info() can't +// find). Dependency materialization can write the top-level skill's lock +// entry and then fail on a later dependency, which is why the lock entry is +// always removed here rather than only when the write itself failed. func (s *service) installAndRegister( ctx context.Context, opts skills.InstallOptions, @@ -234,7 +239,14 @@ func (s *service) installAndRegister( skillName string, scope skills.Scope, ) (*skills.InstallResult, error) { - rollback := func() { _ = s.store.Delete(ctx, skillName, scope, opts.ProjectRoot) } + rollback := func() { + _ = s.store.Delete(ctx, skillName, scope, opts.ProjectRoot) + if scope == skills.ScopeProject && skills.LockFileFeatureEnabled() { + if root, err := lockfile.OpenRoot(opts.ProjectRoot); err == nil { + _ = lockfile.RemoveEntry(root, skillName) + } + } + } if err := s.registerSkillInGroup(ctx, groupName, skillName); err != nil { // Best-effort rollback: remove the DB record so retries start fresh. diff --git a/pkg/skills/skillsvc/lock.go b/pkg/skills/skillsvc/lock.go index dd90fad6d1..b3d47a88ea 100644 --- a/pkg/skills/skillsvc/lock.go +++ b/pkg/skills/skillsvc/lock.go @@ -53,7 +53,7 @@ func (s *service) recordLockState( } } - if err := s.materializeDependencies(ctx, opts, sk); err != nil { + if err := s.materializeDependencies(ctx, opts, source, sk); err != nil { return sk, fmt.Errorf("materializing dependencies: %w", err) } return sk, nil @@ -65,9 +65,18 @@ func (s *service) recordLockState( // infinite recursion on a requires cycle; skills.MaxDependencies bounds the // total number of skills materialized across the whole tree, not just the // direct dependency list of a single skill. +// +// source is the reference sk was installed from (recordLockState's Source, +// after any LockSource override) — the same kind of string a dependency +// edge names in another skill's toolhive.requires. Visited must be keyed +// consistently by that reference form, not by sk's resolved Metadata.Name: +// a requires edge naming sk by a reference string other than its resolved +// name would otherwise bypass the cycle check and re-materialize sk as its +// own dependency, corrupting its RequiredBy list. func (s *service) materializeDependencies( ctx context.Context, opts skills.InstallOptions, + source string, sk skills.InstalledSkill, ) error { parsed, err := readSkillMD(s.pathResolver, sk) @@ -82,7 +91,7 @@ func (s *service) materializeDependencies( if visited == nil { visited = make(map[string]struct{}) } - visited[sk.Metadata.Name] = struct{}{} + visited[source] = struct{}{} for _, dep := range parsed.Requires { if _, seen := visited[dep.Reference]; seen { diff --git a/pkg/skills/skillsvc/lock_test.go b/pkg/skills/skillsvc/lock_test.go index cc57b6edc5..ab54d2ae6f 100644 --- a/pkg/skills/skillsvc/lock_test.go +++ b/pkg/skills/skillsvc/lock_test.go @@ -251,7 +251,7 @@ func TestMaterializeDependencies_RejectsTreeExceedingMaxDependencies(t *testing. visited[fmt.Sprintf("seen-%d", i)] = struct{}{} } - err := svcImpl.materializeDependencies(t.Context(), skills.InstallOptions{Visited: visited}, sk) + err := svcImpl.materializeDependencies(t.Context(), skills.InstallOptions{Visited: visited}, "root-skill", sk) require.Error(t, err) assert.Contains(t, err.Error(), "exceeds maximum") } @@ -283,6 +283,12 @@ func TestInstallProjectScope_DependencyCycleTerminates(t *testing.T) { a, ok := lf.Get("skill-a") require.True(t, ok) assert.True(t, a.Explicit) + // The cycle check must trip on skill-b's requires entry for refA — the + // same git:// reference skill-a was installed with, not skill-a's + // resolved bare name — so skill-a is never re-materialized as its own + // dependency. Without that, skill-a would gain itself a spurious + // RequiredBy via skill-b. + assert.Empty(t, a.RequiredBy, "skill-a must not become required-by skill-b through the a<->b cycle") b, ok := lf.Get("skill-b") require.True(t, ok) assert.Equal(t, []string{"skill-a"}, b.RequiredBy) @@ -306,3 +312,41 @@ func TestInstallProjectScope_LockWriteFailureRollsBackInstall(t *testing.T) { _, err = svc.Info(t.Context(), skills.InfoOptions{Name: "my-skill", Scope: skills.ScopeProject, ProjectRoot: projectRoot}) require.Error(t, err, "the DB record must be rolled back so a retry starts fresh") } + +// TestInstallProjectScope_DependencyFailureRollsBackParentLockEntry covers a +// failure that happens after the parent's own lock entry was already +// written: recordLockEntry succeeds for the parent, then +// materializeDependencies fails on an unresolvable toolhive.requires +// dependency. The parent's lock entry (and DB record) must be rolled back +// too, not just the DB record — otherwise toolhive.lock.yaml claims the +// parent is pinned while Info() reports it doesn't exist. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestInstallProjectScope_DependencyFailureRollsBackParentLockEntry(t *testing.T) { + gr, fx := newGitResolverMock(t) + missingDepRef, _ := gitRef("missing-dep") // never registered — Resolve fails for it + fx.register("parent-skill", gitSkill("parent-skill", missingDepRef)) + svc, projectRoot := newLockTestService(t, gr) + + ref, _ := gitRef("parent-skill") + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.Error(t, err, "install must fail when a declared dependency cannot be resolved") + + lf := readLockfile(t, projectRoot) + _, ok := lf.Get("parent-skill") + assert.False(t, ok, "parent's lock entry must be rolled back when a dependency fails to materialize") + + _, err = svc.Info(t.Context(), skills.InfoOptions{Name: "parent-skill", Scope: skills.ScopeProject, ProjectRoot: projectRoot}) + require.Error(t, err, "the DB record must be rolled back so a retry starts fresh") + + // Retrying (with Force, since the first attempt's extracted files are + // left on disk per this rollback's documented contract) must succeed + // once the dependency is fixed, not hit a stale, permanently-broken state. + fx.register("missing-dep", gitSkill("missing-dep")) + _, err = svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, Force: true, + }) + require.NoError(t, err, "retry after fixing the dependency must succeed") +} From 187a35d91ed04d7f26fff1bde228cef00b181a55 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Thu, 23 Jul 2026 10:53:27 +0200 Subject: [PATCH 4/4] Make install rollback restorative and uninstall resurrection-proof Three panel-review findings on the install/uninstall lifecycle: rollback deleted only the top-level skill, leaking earlier-succeeded sibling dependencies as managed orphans whose RequiredBy pointed at a rolled-back parent; rollback was also unconditionally destructive, so a --force reinstall of a valid, lock-managed skill that failed on a new dependency destroyed its pre-existing DB record and lock entry; and uninstall removed the lock entry last and best-effort, so a lock-write failure after files and DB were gone left a stale entry the next sync silently reinstalled. Rollback now snapshots pre-install state (DB record via InstallResult.PreExisting, lock entry before any write) and restores it when the record pre-existed; when this call created it, removal runs the same dependency cascade as uninstall so fresh orphans are cleaned while shared or explicit deps survive. Uninstall removes the lock entry first and aborts intact on failure. Materialized dependencies also now join the parent's --group instead of falling back to the default group. --- pkg/skills/options.go | 5 + pkg/skills/skillsvc/install.go | 81 ++++++++--- pkg/skills/skillsvc/install_extraction.go | 21 +++ pkg/skills/skillsvc/install_git.go | 10 +- pkg/skills/skillsvc/lock.go | 1 + pkg/skills/skillsvc/lock_test.go | 164 ++++++++++++++++++++++ pkg/skills/skillsvc/uninstall.go | 74 ++++++---- 7 files changed, 312 insertions(+), 44 deletions(-) diff --git a/pkg/skills/options.go b/pkg/skills/options.go index 7291a18307..d228fec6dc 100644 --- a/pkg/skills/options.go +++ b/pkg/skills/options.go @@ -60,6 +60,11 @@ type InstallOptions struct { type InstallResult struct { // Skill is the installed skill. Skill InstalledSkill `json:"skill"` + // PreExisting is the store record as it was before this install, or nil + // when this install created the record. Rollback uses it to restore the + // previous state instead of destructively deleting a record this call + // did not create. Internal use only — NOT exposed via HTTP API. + PreExisting *InstalledSkill `json:"-"` } // UninstallOptions configures the behavior of the Uninstall operation. diff --git a/pkg/skills/skillsvc/install.go b/pkg/skills/skillsvc/install.go index e95781c5c7..501b2de556 100644 --- a/pkg/skills/skillsvc/install.go +++ b/pkg/skills/skillsvc/install.go @@ -223,13 +223,11 @@ func (s *service) registerSkillInGroup(ctx context.Context, groupName string, sk // and, for project-scope installs with the lock file feature enabled (see // skills.LockFileFeatureEnabled), records it — and any toolhive.requires // dependencies — in the project's toolhive.lock.yaml. If group registration -// or the lock write fails, the DB record and any lock entry already written -// for this skill are rolled back so that a retry starts fresh rather than -// leaving the system in an inconsistent state (skill installed but not in -// the expected group, or the lock file claiming a pin that Info() can't -// find). Dependency materialization can write the top-level skill's lock -// entry and then fail on a later dependency, which is why the lock entry is -// always removed here rather than only when the write itself failed. +// or the lock write fails, the DB record and lock entry are rolled back to +// their pre-install state: restored when this call updated a pre-existing +// record (a --force reinstall must not be destroyed by a transient failure), +// deleted — with a dependency cascade cleaning up any freshly materialized +// orphans — when this call created them. func (s *service) installAndRegister( ctx context.Context, opts skills.InstallOptions, @@ -239,24 +237,33 @@ func (s *service) installAndRegister( skillName string, scope skills.Scope, ) (*skills.InstallResult, error) { - rollback := func() { - _ = s.store.Delete(ctx, skillName, scope, opts.ProjectRoot) - if scope == skills.ScopeProject && skills.LockFileFeatureEnabled() { - if root, err := lockfile.OpenRoot(opts.ProjectRoot); err == nil { - _ = lockfile.RemoveEntry(root, skillName) + lockScoped := scope == skills.ScopeProject && skills.LockFileFeatureEnabled() + + // Snapshot the prior lock entry before anything below can write one, so + // rollback can reinstate it (RequiredBy links from other parents + // included) rather than blindly deleting it. + var prevEntry *lockfile.Entry + if lockScoped { + if root, rootErr := lockfile.OpenRoot(opts.ProjectRoot); rootErr == nil { + if lf, loadErr := lockfile.Load(root); loadErr == nil { + if e, ok := lf.Get(skillName); ok { + prevEntry = &e + } } } } + rollback := func() { s.rollbackInstall(ctx, opts, result, skillName, scope, lockScoped, prevEntry) } + if err := s.registerSkillInGroup(ctx, groupName, skillName); err != nil { - // Best-effort rollback: remove the DB record so retries start fresh. - // Files on disk are left in place; a fresh install will detect them - // and either overwrite (force) or return a conflict. + // Best-effort rollback. Files on disk are left in place; a fresh + // install will detect them and either overwrite (force) or return a + // conflict. rollback() return nil, fmt.Errorf("registering skill in group: %w", err) } - if scope == skills.ScopeProject && skills.LockFileFeatureEnabled() { + if lockScoped { updated, err := s.recordLockState(ctx, opts, originalName, result.Skill) if err != nil { rollback() @@ -270,3 +277,45 @@ func (s *service) installAndRegister( return result, nil } + +// rollbackInstall undoes installAndRegister's side effects after a failure, +// best-effort. The DB record is restored to its pre-install snapshot when +// one exists (result.PreExisting) and deleted otherwise; the lock entry is +// likewise reinstated from prevEntry or removed. When this call created the +// entry, removal runs the same dependency cascade as uninstall so that +// freshly materialized dependencies — installed, marked managed, and +// required only by the now-rolled-back skill — do not leak as orphans, +// while pre-existing dependencies with other parents (or explicit installs) +// survive with this skill stripped from their RequiredBy. +func (s *service) rollbackInstall( + ctx context.Context, + opts skills.InstallOptions, + result *skills.InstallResult, + skillName string, + scope skills.Scope, + lockScoped bool, + prevEntry *lockfile.Entry, +) { + if result.PreExisting != nil { + _ = s.store.Update(ctx, *result.PreExisting) + } else { + _ = s.store.Delete(ctx, skillName, scope, opts.ProjectRoot) + } + + if !lockScoped { + return + } + if prevEntry != nil { + if root, err := lockfile.OpenRoot(opts.ProjectRoot); err == nil { + _ = lockfile.UpsertEntry(root, *prevEntry) + } + return + } + uninstallOpts := skills.UninstallOptions{Name: skillName, Scope: scope, ProjectRoot: opts.ProjectRoot} + candidates, err := removeLockEntry(uninstallOpts) + if err != nil { + return + } + visited := map[string]struct{}{skillName: {}} + _ = s.cascadeUninstall(ctx, candidates, visited, opts.ProjectRoot, scope) +} diff --git a/pkg/skills/skillsvc/install_extraction.go b/pkg/skills/skillsvc/install_extraction.go index 60a4da681b..4a9f680c10 100644 --- a/pkg/skills/skillsvc/install_extraction.go +++ b/pkg/skills/skillsvc/install_extraction.go @@ -33,6 +33,27 @@ func (s *service) installWithExtraction( return nil, fmt.Errorf("checking existing skill: %w", storeErr) } + result, err := s.dispatchExtraction(ctx, opts, scope, existing, storeErr, clientTypes, clientDirs) + if err == nil && storeErr == nil { + // Preserve the pre-install record so a later rollback (e.g. a failed + // dependency materialization) can restore it rather than delete it. + pre := existing + result.PreExisting = &pre + } + return result, err +} + +// dispatchExtraction routes an extraction-based install to the no-op, +// same-digest, upgrade, or fresh path based on the pre-install store state. +func (s *service) dispatchExtraction( + ctx context.Context, + opts skills.InstallOptions, + scope skills.Scope, + existing skills.InstalledSkill, + storeErr error, + clientTypes []string, + clientDirs map[string]string, +) (*skills.InstallResult, error) { if isExtractionNoOp(existing, storeErr, opts, clientTypes) { return &skills.InstallResult{Skill: existing}, nil } diff --git a/pkg/skills/skillsvc/install_git.go b/pkg/skills/skillsvc/install_git.go index 9a9bb672bc..36b8c84c52 100644 --- a/pkg/skills/skillsvc/install_git.go +++ b/pkg/skills/skillsvc/install_git.go @@ -102,7 +102,15 @@ func (s *service) applyGitInstall( return nil, fmt.Errorf("checking existing skill: %w", storeErr) } if !isNotFound { - return s.applyGitInstallExisting(ctx, opts, scope, existing, clientTypes, clientDirs, files) + result, err := s.applyGitInstallExisting(ctx, opts, scope, existing, clientTypes, clientDirs, files) + if err == nil { + // Preserve the pre-install record so a later rollback (e.g. a + // failed dependency materialization) can restore it rather than + // delete it. + pre := existing + result.PreExisting = &pre + } + return result, err } return s.applyGitInstallFresh(ctx, opts, scope, clientTypes, clientDirs, files) } diff --git a/pkg/skills/skillsvc/lock.go b/pkg/skills/skillsvc/lock.go index b3d47a88ea..ec9aa4d568 100644 --- a/pkg/skills/skillsvc/lock.go +++ b/pkg/skills/skillsvc/lock.go @@ -108,6 +108,7 @@ func (s *service) materializeDependencies( Scope: sk.Scope, ProjectRoot: sk.ProjectRoot, Clients: sk.Clients, + Group: opts.Group, // deps join the parent's group, not the default one RequiredByParent: sk.Metadata.Name, Visited: visited, } diff --git a/pkg/skills/skillsvc/lock_test.go b/pkg/skills/skillsvc/lock_test.go index ab54d2ae6f..16b633f19c 100644 --- a/pkg/skills/skillsvc/lock_test.go +++ b/pkg/skills/skillsvc/lock_test.go @@ -16,6 +16,8 @@ import ( "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" + "github.com/stacklok/toolhive/pkg/groups" + groupmocks "github.com/stacklok/toolhive/pkg/groups/mocks" "github.com/stacklok/toolhive/pkg/skills" "github.com/stacklok/toolhive/pkg/skills/gitresolver" gitmocks "github.com/stacklok/toolhive/pkg/skills/gitresolver/mocks" @@ -350,3 +352,165 @@ func TestInstallProjectScope_DependencyFailureRollsBackParentLockEntry(t *testin }) require.NoError(t, err, "retry after fixing the dependency must succeed") } + +// TestInstallProjectScope_SiblingDependencyRolledBackWithParent covers the +// partial-success case: the first declared dependency installs cleanly, then +// a later sibling fails. Rolling back only the parent would leak the +// succeeded sibling as an orphan — installed, managed, and pinned with a +// RequiredBy pointing at a parent that no longer exists. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestInstallProjectScope_SiblingDependencyRolledBackWithParent(t *testing.T) { + gr, fx := newGitResolverMock(t) + okDepRef, _ := gitRef("ok-dep") + missingDepRef, _ := gitRef("missing-dep") // never registered — Resolve fails + fx.register("ok-dep", gitSkill("ok-dep")) + fx.register("parent-skill", gitSkill("parent-skill", okDepRef, missingDepRef)) + svc, projectRoot := newLockTestService(t, gr) + + ref, _ := gitRef("parent-skill") + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.Error(t, err, "install must fail when a later dependency cannot be resolved") + + lf := readLockfile(t, projectRoot) + _, ok := lf.Get("parent-skill") + assert.False(t, ok, "parent's lock entry must be rolled back") + _, ok = lf.Get("ok-dep") + assert.False(t, ok, "the succeeded sibling's lock entry must be rolled back too, not leaked as an orphan") + + _, err = svc.Info(t.Context(), skills.InfoOptions{Name: "ok-dep", Scope: skills.ScopeProject, ProjectRoot: projectRoot}) + require.Error(t, err, "the succeeded sibling's DB record must be rolled back too") +} + +// TestInstallProjectScope_RollbackRestoresPreExistingState covers the +// destructive-rollback hazard: a --force reinstall of an already-installed, +// lock-managed skill that fails partway (here: on a newly-declared +// dependency) must restore the previously-valid DB record and lock entry — +// including RequiredBy links other parents merged onto it — not delete them. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestInstallProjectScope_RollbackRestoresPreExistingState(t *testing.T) { + gr, fx := newGitResolverMock(t) + sharedRef, _ := gitRef("shared-skill") + fx.register("shared-skill", gitSkill("shared-skill")) + fx.register("other-parent", gitSkill("other-parent", sharedRef)) + svc, projectRoot := newLockTestService(t, gr) + + // shared-skill is installed both explicitly and as other-parent's + // dependency, so its entry carries Explicit=true and RequiredBy. + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: sharedRef, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + otherRef, _ := gitRef("other-parent") + _, err = svc.Install(t.Context(), skills.InstallOptions{ + Name: otherRef, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + + before, ok := readLockfile(t, projectRoot).Get("shared-skill") + require.True(t, ok) + require.Equal(t, []string{"other-parent"}, before.RequiredBy) + + // Republish shared-skill with a new, unresolvable dependency and force- + // reinstall it: the install fails after the DB record and lock entry + // were already rewritten. + missingDepRef, _ := gitRef("missing-dep") + fx.register("shared-skill", gitSkill("shared-skill", missingDepRef)) + _, err = svc.Install(t.Context(), skills.InstallOptions{ + Name: sharedRef, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, Force: true, + }) + require.Error(t, err, "reinstall must fail on the unresolvable dependency") + + after, ok := readLockfile(t, projectRoot).Get("shared-skill") + require.True(t, ok, "a transient failure must not destroy the pre-existing lock entry") + assert.Equal(t, before.Digest, after.Digest, "the previous pin must be restored") + assert.Equal(t, before.RequiredBy, after.RequiredBy, "RequiredBy links from other parents must survive") + assert.True(t, after.Explicit) + + info, err := svc.Info(t.Context(), skills.InfoOptions{Name: "shared-skill", Scope: skills.ScopeProject, ProjectRoot: projectRoot}) + require.NoError(t, err, "the pre-existing DB record must survive") + assert.Equal(t, before.Digest, info.InstalledSkill.Digest, "the DB record must be restored to its previous state") +} + +// TestInstallProjectScope_DependencyJoinsParentGroup asserts a transitively +// materialized dependency is registered in the same group the parent was +// installed into, rather than silently falling back to the default group. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestInstallProjectScope_DependencyJoinsParentGroup(t *testing.T) { + gr, fx := newGitResolverMock(t) + depRef, _ := gitRef("grouped-dep") + fx.register("grouped-dep", gitSkill("grouped-dep")) + fx.register("grouped-parent", gitSkill("grouped-parent", depRef)) + svc, projectRoot := newLockTestService(t, gr) + + // Track which group each skill lands in via a stub manager. + skillGroups := make(map[string]string) + gm := groupmocks.NewMockManager(gomock.NewController(t)) + gm.EXPECT().Get(gomock.Any(), gomock.Any()).AnyTimes(). + DoAndReturn(func(_ context.Context, name string) (*groups.Group, error) { + return &groups.Group{Name: name}, nil + }) + gm.EXPECT().Update(gomock.Any(), gomock.Any()).AnyTimes(). + DoAndReturn(func(_ context.Context, g *groups.Group) error { + for _, sk := range g.Skills { + skillGroups[sk] = g.Name + } + return nil + }) + svcImpl := svc.(*service) //nolint:forcetypeassert // white-box test in the same package + svcImpl.groupManager = gm + + ref, _ := gitRef("grouped-parent") + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, Group: "custom-group", + }) + require.NoError(t, err) + + assert.Equal(t, "custom-group", skillGroups["grouped-parent"]) + assert.Equal(t, "custom-group", skillGroups["grouped-dep"], + "a materialized dependency must join the parent's group, not the default one") +} + +// TestUninstall_LockWriteFailureAbortsBeforeDestruction guards the uninstall +// ordering: the lock entry is removed first, and if that fails the +// uninstall aborts with everything intact. The reverse order left a stale +// lock entry for a gone skill, which the next sync silently reinstalled. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestUninstall_LockWriteFailureAbortsBeforeDestruction(t *testing.T) { + gr, fx := newGitResolverMock(t) + fx.register("my-skill", gitSkill("my-skill")) + svc, projectRoot := newLockTestService(t, gr) + + ref, _ := gitRef("my-skill") + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + + // Make the lock file unwritable: read-only project root blocks the + // temp-file write inside lockfile.Update. + require.NoError(t, os.Chmod(projectRoot, 0o555)) + t.Cleanup(func() { _ = os.Chmod(projectRoot, 0o755) }) + + err = svc.Uninstall(t.Context(), skills.UninstallOptions{ + Name: "my-skill", Scope: skills.ScopeProject, ProjectRoot: projectRoot, + }) + require.Error(t, err, "uninstall must fail when the lock entry cannot be removed") + + require.NoError(t, os.Chmod(projectRoot, 0o755)) + info, err := svc.Info(t.Context(), skills.InfoOptions{Name: "my-skill", Scope: skills.ScopeProject, ProjectRoot: projectRoot}) + require.NoError(t, err, "the skill must remain fully installed — nothing may be destroyed before the lock update") + assert.NotNil(t, info.InstalledSkill) + _, ok := readLockfile(t, projectRoot).Get("my-skill") + assert.True(t, ok, "the lock entry must be untouched") + + skillMD := filepath.Join(projectRoot, ".claude", "skills", "my-skill", "SKILL.md") + _, statErr := os.Stat(skillMD) + require.NoError(t, statErr, "on-disk files must be untouched") +} diff --git a/pkg/skills/skillsvc/uninstall.go b/pkg/skills/skillsvc/uninstall.go index 2d1da2a584..9c52241957 100644 --- a/pkg/skills/skillsvc/uninstall.go +++ b/pkg/skills/skillsvc/uninstall.go @@ -36,9 +36,17 @@ func (s *service) Uninstall(ctx context.Context, opts skills.UninstallOptions) e return s.uninstallOne(ctx, opts, scope) } -// uninstallOne performs a single skill's file/DB/group cleanup and, when -// applicable, its lock entry removal and cascade. It is called both for the -// top-level Uninstall request and recursively for cascade candidates. +// uninstallOne performs a single skill's lock-entry removal, file/DB/group +// cleanup and, when applicable, its dependency cascade. It is called both +// for the top-level Uninstall request and recursively for cascade candidates. +// +// The lock entry is removed FIRST, and its failure aborts the uninstall +// while everything is still intact. The reverse order (files/DB first, lock +// entry best-effort) had a resurrection hazard: a lock-write failure after +// the record and files were gone left a stale entry that the next sync +// silently reinstalled. Failing after the entry is removed leaves the +// opposite, safe inconsistency — an installed-but-unlocked skill that sync +// reports as removed-from-lock and prune can clean up. func (s *service) uninstallOne(ctx context.Context, opts skills.UninstallOptions, scope skills.Scope) error { unlock := s.locks.lock(opts.Name, scope, opts.ProjectRoot) defer unlock() @@ -49,10 +57,25 @@ func (s *service) uninstallOne(ctx context.Context, opts skills.UninstallOptions return err } + visited := opts.Visited + if visited == nil { + visited = make(map[string]struct{}) + } + visited[opts.Name] = struct{}{} + + var cascadeCandidates []string + if scope == skills.ScopeProject && existing.Managed && skills.LockFileFeatureEnabled() { + cascadeCandidates, err = removeLockEntry(opts) + if err != nil { + return fmt.Errorf("updating project lock file: %w", err) + } + } + cleanupErrs := s.removeClientFiles(existing, opts, scope) if err := s.store.Delete(ctx, opts.Name, scope, opts.ProjectRoot); err != nil { - return err + cleanupErrs = append(cleanupErrs, err) + return errors.Join(cleanupErrs...) } // Remove the skill from all groups — best-effort, same pattern as file cleanup. @@ -62,13 +85,8 @@ func (s *service) uninstallOne(ctx context.Context, opts skills.UninstallOptions } } - // Lock file cleanup is best-effort, matching every other cleanup step - // here: the DB record and files are already gone by this point, so there - // is nothing left to roll back to on failure. - if scope == skills.ScopeProject && existing.Managed && skills.LockFileFeatureEnabled() { - if cascadeErr := s.removeLockEntryAndCascade(ctx, opts, scope); cascadeErr != nil { - cleanupErrs = append(cleanupErrs, fmt.Errorf("updating project lock file: %w", cascadeErr)) - } + if cascadeErr := s.cascadeUninstall(ctx, cascadeCandidates, visited, opts.ProjectRoot, scope); cascadeErr != nil { + cleanupErrs = append(cleanupErrs, cascadeErr) } return errors.Join(cleanupErrs...) @@ -110,20 +128,14 @@ func (s *service) removeClientFiles( return errs } -// removeLockEntryAndCascade removes opts.Name's lock entry and, for any -// dependency that consequently loses its last requiring parent (and is not -// itself explicit), uninstalls it too. A Visited set threaded through opts -// prevents infinite recursion on a requiredBy cycle in a hand-edited lock. -func (s *service) removeLockEntryAndCascade(ctx context.Context, opts skills.UninstallOptions, scope skills.Scope) error { - visited := opts.Visited - if visited == nil { - visited = make(map[string]struct{}) - } - visited[opts.Name] = struct{}{} - +// removeLockEntry removes opts.Name's lock entry and strips it from every +// other entry's RequiredBy list, returning the names of dependencies that +// consequently lost their last requiring parent and are not explicit — the +// cascade-removal candidates. +func removeLockEntry(opts skills.UninstallOptions) ([]string, error) { root, err := lockfile.OpenRoot(opts.ProjectRoot) if err != nil { - return err + return nil, err } var cascadeCandidates []string if err := lockfile.Update(root, func(lf *lockfile.Lockfile) error { @@ -131,11 +143,19 @@ func (s *service) removeLockEntryAndCascade(ctx context.Context, opts skills.Uni cascadeCandidates = lf.RemoveParentFromRequiredBy(opts.Name) return nil }); err != nil { - return err + return nil, err } + return cascadeCandidates, nil +} +// cascadeUninstall uninstalls each candidate dependency that has not +// already been visited. The visited set prevents infinite recursion on a +// requiredBy cycle in a hand-edited lock. +func (s *service) cascadeUninstall( + ctx context.Context, candidates []string, visited map[string]struct{}, projectRoot string, scope skills.Scope, +) error { var errs []error - for _, dep := range cascadeCandidates { + for _, dep := range candidates { if _, seen := visited[dep]; seen { continue } @@ -143,13 +163,13 @@ func (s *service) removeLockEntryAndCascade(ctx context.Context, opts skills.Uni // A lock entry can reference a dependency whose install failed // partway (or was already removed by hand); skip it rather than // erroring on a missing DB record. - if _, getErr := s.store.Get(ctx, dep, scope, opts.ProjectRoot); getErr != nil { + if _, getErr := s.store.Get(ctx, dep, scope, projectRoot); getErr != nil { continue } if uninstallErr := s.uninstallOne(ctx, skills.UninstallOptions{ Name: dep, Scope: scope, - ProjectRoot: opts.ProjectRoot, + ProjectRoot: projectRoot, Visited: visited, }, scope); uninstallErr != nil { errs = append(errs, fmt.Errorf("cascade-removing dependency %q: %w", dep, uninstallErr))