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..d228fec6dc 100644 --- a/pkg/skills/options.go +++ b/pkg/skills/options.go @@ -37,12 +37,34 @@ 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. 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. @@ -53,6 +75,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..501b2de556 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 @@ -22,6 +23,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 +46,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 +70,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 +85,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 +95,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 +103,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 +134,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 +143,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 +151,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 +159,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 +175,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 +190,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 +198,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 +219,103 @@ 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 -// starts fresh rather than leaving the system in an inconsistent state (skill -// installed but not in the expected group). +// 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 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, + originalName string, result *skills.InstallResult, groupName string, skillName string, scope skills.Scope, - projectRoot string, ) (*skills.InstallResult, error) { + 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. - _ = s.store.Delete(ctx, skillName, scope, projectRoot) + // 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 lockScoped { + 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 } + +// 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 new file mode 100644 index 0000000000..ec9aa4d568 --- /dev/null +++ b/pkg/skills/skillsvc/lock.go @@ -0,0 +1,173 @@ +// 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, source, 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. +// +// 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) + if err != nil { + return err + } + if len(parsed.Requires) == 0 { + return nil + } + + visited := opts.Visited + if visited == nil { + visited = make(map[string]struct{}) + } + visited[source] = 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, + Group: opts.Group, // deps join the parent's group, not the default one + 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..16b633f19c --- /dev/null +++ b/pkg/skills/skillsvc/lock_test.go @@ -0,0 +1,516 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package skillsvc + +import ( + "context" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "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" + "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 +// 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) { + 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 +// 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}, "root-skill", 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) + // 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) +} + +//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") +} + +// 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") +} + +// 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 4c74d8ec98..9c52241957 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,21 @@ 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 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() @@ -38,36 +57,25 @@ func (s *service) Uninstall(ctx context.Context, opts skills.UninstallOptions) e return err } - // Determine the boundary directory for empty-parent cleanup. - stopDir := opts.ProjectRoot - if scope == skills.ScopeUser { - if homeDir, err := os.UserHomeDir(); err == nil { - stopDir = homeDir - } + visited := opts.Visited + if visited == nil { + visited = make(map[string]struct{}) } + visited[opts.Name] = struct{}{} - // 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 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. @@ -77,5 +85,95 @@ func (s *service) Uninstall(ctx context.Context, opts skills.UninstallOptions) e } } + if cascadeErr := s.cascadeUninstall(ctx, cascadeCandidates, visited, opts.ProjectRoot, scope); cascadeErr != nil { + cleanupErrs = append(cleanupErrs, 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 { + if homeDir, err := os.UserHomeDir(); err == nil { + stopDir = homeDir + } + } + + 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 +} + +// 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 nil, 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 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 candidates { + 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, projectRoot); getErr != nil { + continue + } + if uninstallErr := s.uninstallOne(ctx, skills.UninstallOptions{ + Name: dep, + Scope: scope, + ProjectRoot: projectRoot, + Visited: visited, + }, scope); uninstallErr != nil { + errs = append(errs, fmt.Errorf("cascade-removing dependency %q: %w", dep, uninstallErr)) + } + } + 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") + }) +})