diff --git a/pkg/groups/skills.go b/pkg/groups/skills.go index 19fafd6dbe..c20398850f 100644 --- a/pkg/groups/skills.go +++ b/pkg/groups/skills.go @@ -11,23 +11,47 @@ import ( // AddSkillToGroup adds skillName to the Skills slice of the named group. // Groups that do not exist return an error. Duplicate skill names are skipped. -// Empty groupName is a no-op. -func AddSkillToGroup(ctx context.Context, mgr Manager, groupName string, skillName string) error { +// Empty groupName is a no-op. The bool reports whether this call inserted the +// name (false when it was already a member or groupName is empty), so a later +// rollback can remove it only when this operation added it. +func AddSkillToGroup(ctx context.Context, mgr Manager, groupName string, skillName string) (added bool, err error) { if groupName == "" { - return nil + return false, nil } group, err := mgr.Get(ctx, groupName) if err != nil { - return fmt.Errorf("getting group %q: %w", groupName, err) + return false, fmt.Errorf("getting group %q: %w", groupName, err) } if slices.Contains(group.Skills, skillName) { - return nil + return false, nil } group.Skills = append(group.Skills, skillName) if err := mgr.Update(ctx, group); err != nil { - return fmt.Errorf("updating group %q: %w", groupName, err) + return false, fmt.Errorf("updating group %q: %w", groupName, err) + } + return true, nil +} + +// RemoveSkillFromGroup removes skillName from the named group's Skills slice. +// Missing membership is a no-op. Empty groupName is a no-op. +func RemoveSkillFromGroup(ctx context.Context, mgr Manager, groupName string, skillName string) error { + if groupName == "" { + return nil + } + group, err := mgr.Get(ctx, groupName) + if err != nil { + return fmt.Errorf("getting group %q: %w", groupName, err) + } + + idx := slices.Index(group.Skills, skillName) + if idx < 0 { + return nil + } + group.Skills = slices.Delete(group.Skills, idx, idx+1) + if err := mgr.Update(ctx, group); err != nil { + return fmt.Errorf("updating group %q: %w", group.Name, err) } return nil } diff --git a/pkg/groups/skills_test.go b/pkg/groups/skills_test.go index 55012fd070..20f47b6a5d 100644 --- a/pkg/groups/skills_test.go +++ b/pkg/groups/skills_test.go @@ -85,7 +85,7 @@ func TestAddSkillToGroups(t *testing.T) { mgr := groupmocks.NewMockManager(ctrl) tt.setupMock(mgr) - err := AddSkillToGroup(context.Background(), mgr, tt.groupName, tt.skillName) + _, err := AddSkillToGroup(context.Background(), mgr, tt.groupName, tt.skillName) if tt.wantErr != "" { require.Error(t, err) @@ -97,6 +97,84 @@ func TestAddSkillToGroups(t *testing.T) { } } +func TestRemoveSkillFromGroup(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + groupName string + skillName string + setupMock func(*groupmocks.MockManager) + wantErr string + }{ + { + name: "empty group name is a no-op", + groupName: "", + skillName: "my-skill", + setupMock: func(*groupmocks.MockManager) {}, + }, + { + name: "missing membership is a no-op", + groupName: "mygroup", + skillName: "my-skill", + setupMock: func(m *groupmocks.MockManager) { + m.EXPECT().Get(gomock.Any(), "mygroup"). + Return(&Group{Name: "mygroup", Skills: []string{"other"}}, nil) + }, + }, + { + name: "removes membership", + groupName: "mygroup", + skillName: "my-skill", + setupMock: func(m *groupmocks.MockManager) { + m.EXPECT().Get(gomock.Any(), "mygroup"). + Return(&Group{Name: "mygroup", Skills: []string{"my-skill", "other"}}, nil) + m.EXPECT().Update(gomock.Any(), &Group{Name: "mygroup", Skills: []string{"other"}}). + Return(nil) + }, + }, + { + name: "get error is wrapped", + groupName: "mygroup", + skillName: "my-skill", + setupMock: func(m *groupmocks.MockManager) { + m.EXPECT().Get(gomock.Any(), "mygroup"). + Return(nil, errors.New("backend down")) + }, + wantErr: "getting group", + }, + { + name: "update error is wrapped", + groupName: "mygroup", + skillName: "my-skill", + setupMock: func(m *groupmocks.MockManager) { + m.EXPECT().Get(gomock.Any(), "mygroup"). + Return(&Group{Name: "mygroup", Skills: []string{"my-skill"}}, nil) + m.EXPECT().Update(gomock.Any(), gomock.Any()). + Return(errors.New("write failed")) + }, + wantErr: "updating group", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mgr := groupmocks.NewMockManager(ctrl) + tt.setupMock(mgr) + + err := RemoveSkillFromGroup(context.Background(), mgr, tt.groupName, tt.skillName) + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + } else { + require.NoError(t, err) + } + }) + } +} + func TestRemoveSkillFromAllGroups(t *testing.T) { t.Parallel() diff --git a/pkg/skills/lockfile/validation.go b/pkg/skills/lockfile/validation.go index f8c1f892f7..2dd8abe284 100644 --- a/pkg/skills/lockfile/validation.go +++ b/pkg/skills/lockfile/validation.go @@ -77,7 +77,7 @@ func validateEntryGraph(entries []Entry) error { // findRequiredByCycle detects a cycle in the requiredBy graph and returns // one such cycle path, or nil. Normal installs can never produce a cycle -// (the install-time Visited set breaks them), but a hand-edited or badly +// (the install-time depState active stack rejects them), but a hand-edited or badly // merge-resolved lock file can — and a ring of mutually-required, // non-explicit entries would then be impossible to ever cascade-remove, so // it is rejected at validation instead of persisting silently. diff --git a/pkg/skills/options.go b/pkg/skills/options.go index fc63c6ca62..67a979bcbd 100644 --- a/pkg/skills/options.go +++ b/pkg/skills/options.go @@ -23,7 +23,8 @@ type InstallOptions struct { Version string `json:"version,omitempty"` // Scope is the installation scope. Scope Scope `json:"scope,omitempty"` - // Clients lists target clients (e.g., "claude-code"). Empty means first skill-supporting client. + // Clients lists target clients (e.g., "claude-code"). Empty means every + // skill-supporting client detected on this host. Clients []string `json:"clients,omitempty"` // Force allows overwriting unmanaged skill directories. Force bool `json:"force,omitempty"` @@ -63,12 +64,11 @@ type InstallOptions struct { // 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:"-"` + // ExpectedCanonicalName, when set, requires the resolved skill/manifest + // name to equal this value before any install mutation. Used by + // Sync/Upgrade so a lock entry cannot be repaired under a different + // canonical identity. Internal use only — NOT exposed via HTTP API. + ExpectedCanonicalName string `json:"-"` // SyncRestore forces re-extraction to every existing client even when // Digest matches the currently-installed digest. Set by Sync when // reinstalling at a pinned reference: the whole point is repairing @@ -132,6 +132,11 @@ type InstallResult struct { // previous state instead of destructively deleting a record this call // did not create. Internal use only — NOT exposed via HTTP API. PreExisting *InstalledSkill `json:"-"` + // RestoreFiles restores on-disk skill trees overwritten by a force + // reinstall or upgrade. Set when extraction snapshotted prior content; + // rollback calls it so a failed install does not leave half-written + // files. Internal use only — NOT exposed via HTTP API. + RestoreFiles func() error `json:"-"` // Provenance is the verified signer identity this install recorded — // surfaced so callers can display what trust-on-first-use pinned. Provenance *ProvenanceInfo `json:"provenance,omitempty"` @@ -358,8 +363,9 @@ type UpgradeOptions struct { // different identity than the one recorded in the lock file; the new // identity is recorded in its place. AllowSignerChange bool `json:"allow_signer_change,omitempty"` - // Clients lists target clients (e.g., "claude-code"). Empty means every - // skill-supporting client detected on this host. + // Clients lists target clients (e.g., "claude-code"). Empty preserves the + // skill's currently installed client list; when that is also empty, + // every skill-supporting client detected on this host is used. Clients []string `json:"clients,omitempty"` } diff --git a/pkg/skills/skillsvc/file_snapshot.go b/pkg/skills/skillsvc/file_snapshot.go new file mode 100644 index 0000000000..3da3533e5c --- /dev/null +++ b/pkg/skills/skillsvc/file_snapshot.go @@ -0,0 +1,174 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package skillsvc + +import ( + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" +) + +// fileSnapshot is one regular file captured by snapshotTargets: contents plus +// a sanitized permission mode so executable bits survive restore. +type fileSnapshot struct { + data []byte + mode fs.FileMode +} + +// snapshotFileModeMask strips setuid/setgid/sticky and caps at 0755. +const snapshotFileModeMask fs.FileMode = 0o755 + +func sanitizeFileMode(mode fs.FileMode) fs.FileMode { + return mode.Perm() & snapshotFileModeMask +} + +// dirBackup captures one target directory's pre-mutation state: whether it +// existed at all, and its regular files when it did. Restore reproduces that +// exact state — directories that did not exist are removed, pre-existing +// ones are rewritten from the snapshot. +type dirBackup struct { + existed bool + files map[string]fileSnapshot +} + +// snapshotTargets captures each target directory into memory so a later +// failed install can restore prior content and remove freshly created trees. +// All filesystem access goes through an os.Root anchored at the directory's +// parent, so no traversal can escape the validated skill install location. +func snapshotTargets(dirs []string) (map[string]dirBackup, error) { + backups := make(map[string]dirBackup, len(dirs)) + for _, dir := range dirs { + dir = filepath.Clean(dir) + if _, seen := backups[dir]; seen { + continue + } + backup, err := snapshotTarget(dir) + if err != nil { + return nil, fmt.Errorf("snapshotting %q: %w", dir, err) + } + backups[dir] = backup + } + return backups, nil +} + +// snapshotTarget captures a single directory. A missing directory (or a +// missing parent) is recorded as existed=false so restore knows to remove +// whatever the failed install writes there. +func snapshotTarget(dir string) (dirBackup, error) { + parent, err := os.OpenRoot(filepath.Dir(dir)) + if err != nil { + if os.IsNotExist(err) { + return dirBackup{existed: false}, nil + } + return dirBackup{}, err + } + defer func() { _ = parent.Close() }() + + base := filepath.Base(dir) + info, err := parent.Stat(base) + if err != nil { + if os.IsNotExist(err) { + return dirBackup{existed: false}, nil + } + return dirBackup{}, err + } + if !info.IsDir() { + return dirBackup{existed: false}, nil + } + + sub, err := parent.OpenRoot(base) + if err != nil { + return dirBackup{}, err + } + defer func() { _ = sub.Close() }() + + files := make(map[string]fileSnapshot) + fsys := sub.FS() + walkErr := fs.WalkDir(fsys, ".", func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + return nil + } + info, infoErr := d.Info() + if infoErr != nil { + return infoErr + } + if !info.Mode().IsRegular() { + return nil + } + data, readErr := fs.ReadFile(fsys, path) + if readErr != nil { + return readErr + } + files[path] = fileSnapshot{data: data, mode: sanitizeFileMode(info.Mode())} + return nil + }) + if walkErr != nil { + return dirBackup{}, walkErr + } + return dirBackup{existed: true, files: files}, nil +} + +// restoreTargets restores every snapshotted directory to its exact +// pre-mutation state: targets that did not exist are removed (they were +// created by the failed install), pre-existing targets are rewritten from +// their snapshots. All errors are joined so a partial restore still surfaces. +func restoreTargets(backups map[string]dirBackup) error { + var errs []error + for dir, backup := range backups { + if err := restoreTarget(dir, backup); err != nil { + errs = append(errs, fmt.Errorf("restoring %q: %w", dir, err)) + } + } + return errors.Join(errs...) +} + +// restoreTarget restores one directory from its backup through an os.Root +// anchored at the parent directory. +func restoreTarget(dir string, backup dirBackup) error { + parent, err := os.OpenRoot(filepath.Dir(dir)) + if err != nil { + if os.IsNotExist(err) && !backup.existed { + // Neither the parent nor the target existed before, and the + // failed install did not create them either. + return nil + } + return err + } + defer func() { _ = parent.Close() }() + + base := filepath.Base(dir) + if err := parent.RemoveAll(base); err != nil { + return err + } + if !backup.existed { + return nil + } + if err := parent.Mkdir(base, 0o750); err != nil { + return err + } + sub, err := parent.OpenRoot(base) + if err != nil { + return err + } + defer func() { _ = sub.Close() }() + + var errs []error + for rel, snap := range backup.files { + if parentDir := filepath.Dir(rel); parentDir != "." { + if err := sub.MkdirAll(parentDir, 0o750); err != nil { + errs = append(errs, err) + continue + } + } + if err := sub.WriteFile(rel, snap.data, snap.mode); err != nil { + errs = append(errs, err) + } + } + return errors.Join(errs...) +} diff --git a/pkg/skills/skillsvc/file_snapshot_test.go b/pkg/skills/skillsvc/file_snapshot_test.go new file mode 100644 index 0000000000..099ecebd72 --- /dev/null +++ b/pkg/skills/skillsvc/file_snapshot_test.go @@ -0,0 +1,152 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package skillsvc + +import ( + "io/fs" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSnapshotTargets(t *testing.T) { + t.Parallel() + + t.Run("missing parent records existed=false", func(t *testing.T) { + t.Parallel() + dir := filepath.Join(t.TempDir(), "no-such-parent", "my-skill") + backups, err := snapshotTargets([]string{dir}) + require.NoError(t, err) + require.Contains(t, backups, filepath.Clean(dir)) + assert.False(t, backups[filepath.Clean(dir)].existed) + }) + + t.Run("missing target records existed=false", func(t *testing.T) { + t.Parallel() + dir := filepath.Join(t.TempDir(), "my-skill") + backups, err := snapshotTargets([]string{dir}) + require.NoError(t, err) + assert.False(t, backups[filepath.Clean(dir)].existed) + }) + + t.Run("non-directory target records existed=false", func(t *testing.T) { + t.Parallel() + dir := filepath.Join(t.TempDir(), "my-skill") + require.NoError(t, os.WriteFile(dir, []byte("a file, not a dir"), 0o644)) + backups, err := snapshotTargets([]string{dir}) + require.NoError(t, err) + assert.False(t, backups[filepath.Clean(dir)].existed) + }) + + t.Run("existing tree captures nested files and executable mode", func(t *testing.T) { + t.Parallel() + dir := filepath.Join(t.TempDir(), "my-skill") + require.NoError(t, os.MkdirAll(filepath.Join(dir, "hooks"), 0o750)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "SKILL.md"), []byte("# skill"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "hooks", "run.sh"), []byte("#!/bin/sh"), 0o755)) + + backups, err := snapshotTargets([]string{dir}) + require.NoError(t, err) + backup := backups[filepath.Clean(dir)] + require.True(t, backup.existed) + require.Len(t, backup.files, 2) + assert.Equal(t, fs.FileMode(0o755), backup.files[filepath.Join("hooks", "run.sh")].mode) + assert.Equal(t, []byte("# skill"), backup.files["SKILL.md"].data) + }) + + t.Run("duplicate targets are deduplicated", func(t *testing.T) { + t.Parallel() + dir := filepath.Join(t.TempDir(), "my-skill") + require.NoError(t, os.MkdirAll(dir, 0o750)) + backups, err := snapshotTargets([]string{dir, dir, filepath.Clean(dir)}) + require.NoError(t, err) + assert.Len(t, backups, 1) + }) +} + +func TestRestoreTargets(t *testing.T) { + t.Parallel() + + t.Run("fresh target is removed", func(t *testing.T) { + t.Parallel() + dir := filepath.Join(t.TempDir(), "my-skill") + backups, err := snapshotTargets([]string{dir}) + require.NoError(t, err) + + // The failed install created the tree after the snapshot. + require.NoError(t, os.MkdirAll(dir, 0o750)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "SKILL.md"), []byte("new"), 0o644)) + + require.NoError(t, restoreTargets(backups)) + assert.NoDirExists(t, dir, "a target that did not exist at snapshot time must be removed") + }) + + t.Run("fresh target under missing parent is a no-op", func(t *testing.T) { + t.Parallel() + dir := filepath.Join(t.TempDir(), "never-created", "my-skill") + backups, err := snapshotTargets([]string{dir}) + require.NoError(t, err) + require.NoError(t, restoreTargets(backups), + "restoring a fresh target whose parent never appeared must not fail") + }) + + t.Run("pre-existing tree is restored with content and mode", func(t *testing.T) { + t.Parallel() + dir := filepath.Join(t.TempDir(), "my-skill") + require.NoError(t, os.MkdirAll(filepath.Join(dir, "hooks"), 0o750)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "SKILL.md"), []byte("old content"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "hooks", "run.sh"), []byte("#!/bin/sh"), 0o755)) + + backups, err := snapshotTargets([]string{dir}) + require.NoError(t, err) + + // The failed install rewrote the tree and dropped a file. + require.NoError(t, os.RemoveAll(dir)) + require.NoError(t, os.MkdirAll(dir, 0o750)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "SKILL.md"), []byte("overwritten"), 0o600)) + + require.NoError(t, restoreTargets(backups)) + got, err := os.ReadFile(filepath.Join(dir, "SKILL.md")) + require.NoError(t, err) + assert.Equal(t, "old content", string(got)) + info, err := os.Stat(filepath.Join(dir, "hooks", "run.sh")) + require.NoError(t, err) + assert.Equal(t, fs.FileMode(0o755), info.Mode().Perm(), + "executable bits must survive snapshot and restore") + }) +} + +func TestDepState(t *testing.T) { + t.Parallel() + + t.Run("nil receiver is inert", func(t *testing.T) { + t.Parallel() + var d *depState + require.NoError(t, d.enter("a")) + d.leave("a") + assert.False(t, d.alreadyDone("a")) + }) + + t.Run("re-entering an active name reports a cycle", func(t *testing.T) { + t.Parallel() + d := newDepState() + require.NoError(t, d.enter("a")) + err := d.enter("a") + require.Error(t, err) + assert.Contains(t, err.Error(), "dependency cycle") + }) + + t.Run("leave marks a name completed", func(t *testing.T) { + t.Parallel() + d := newDepState() + require.NoError(t, d.enter("a")) + assert.False(t, d.alreadyDone("a")) + d.leave("a") + assert.True(t, d.alreadyDone("a")) + require.NoError(t, d.enter("a"), "a completed name can be re-entered (shared dep merge path)") + }) +} diff --git a/pkg/skills/skillsvc/install.go b/pkg/skills/skillsvc/install.go index ae13d126bb..f497e3540f 100644 --- a/pkg/skills/skillsvc/install.go +++ b/pkg/skills/skillsvc/install.go @@ -25,6 +25,11 @@ import ( // the registry and extracted. When LayerData is provided, the skill is extracted // to disk and a full installation record is created. Without LayerData, a // pending record is created. +// +// Project-scope installs acquire the project transaction for opts.ProjectRoot +// before any resolve/mutate work and hold it through bookkeeping, +// dependencies, and compensation. User-scope installs acquire a per-skill +// lock after the canonical name is known (inside git/OCI/by-name backends). 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 @@ -55,30 +60,44 @@ func (s *service) Install(ctx context.Context, opts skills.InstallOptions) (*ski opts.Name = opts.Name + ":" + opts.Version } - // Route through the shared source-dispatch skeleton (git, direct OCI - // with registry fallback, plain name) — the same sequence upgrade's - // re-resolution uses, so the two cannot drift. + if scope == skills.ScopeProject { + unlock := s.projectTx.lock(opts.ProjectRoot) + defer unlock() + return s.installLocked(ctx, opts, originalName, scope, newDepState()) + } + return s.installLocked(ctx, opts, originalName, scope, nil) +} + +// installLocked performs Install assuming the appropriate lock is already +// held: the project transaction for ScopeProject, or nothing yet for +// ScopeUser (backends acquire the per-skill lock after resolving the +// canonical name). Sync, Upgrade, and dependency materialization call this +// directly so they never reacquire. +func (s *service) installLocked( + ctx context.Context, + opts skills.InstallOptions, + originalName string, + scope skills.Scope, + deps *depState, +) (*skills.InstallResult, error) { + alreadyLocked := scope == skills.ScopeProject return dispatchSource(ctx, s, opts.Name, sourceOps[*skills.InstallResult]{ git: func(ctx context.Context, _ string) (*skills.InstallResult, error) { - result, err := s.installFromGit(ctx, &opts, scope) - if err != nil { - return nil, err - } - return s.installAndRegister(ctx, opts, originalName, result, opts.Group, result.Skill.Metadata.Name, scope) + return s.installFromGit(ctx, &opts, scope, originalName, deps, alreadyLocked) }, oci: func(ctx context.Context, ref nameref.Reference) (*skills.InstallResult, error) { - result, err := s.installFromOCI(ctx, &opts, scope, ref) + result, err := s.installFromOCI(ctx, &opts, scope, originalName, deps, alreadyLocked, ref) if err != nil { slog.Debug("OCI pull failed, registry fallback may apply", "name", opts.Name, "error", err) return nil, err } - return s.installAndRegister(ctx, opts, originalName, result, opts.Group, opts.Name, scope) + return result, nil }, registry: func(ctx context.Context, resolved *registryResolveResult) (*skills.InstallResult, error) { - return s.installFromResolvedRegistry(ctx, opts, originalName, scope, resolved) + return s.installFromResolvedRegistry(ctx, opts, originalName, scope, resolved, deps, alreadyLocked) }, plainName: func(ctx context.Context, _ string) (*skills.InstallResult, error) { - return s.installByName(ctx, opts, originalName, scope) + return s.installByName(ctx, opts, originalName, scope, deps, alreadyLocked) }, }) } @@ -90,17 +109,19 @@ func (s *service) installByName( opts skills.InstallOptions, originalName string, scope skills.Scope, + deps *depState, + alreadyLocked bool, ) (*skills.InstallResult, error) { - unlock := s.locks.lock(opts.Name, scope, opts.ProjectRoot) - locked := true - defer func() { - if locked { - unlock() - } - }() + if !alreadyLocked { + unlock := s.locks.lock(opts.Name, scope, opts.ProjectRoot) + defer unlock() + alreadyLocked = true + } // Without layer data, check the local OCI store for a matching tag, - // then the registry/index, before returning an error. + // then the registry/index, before returning an error. Registry/git/OCI + // backends enter depState after resolving the canonical name — do not + // enter here or a registry-resolved install would re-enter itself. if len(opts.LayerData) == 0 { resolved := false if s.ociStore != nil { @@ -112,18 +133,25 @@ func (s *service) installByName( } } if !resolved { - // Release lock before registry lookup -- installFromOCI - // acquires its own lock on the artifact's skill name, which - // could be the same key, causing deadlock since sync.Mutex - // is not re-entrant. - unlock() - locked = false - - return s.installFromRegistryLookup(ctx, opts, originalName, scope) + return s.installFromRegistryLookup(ctx, opts, originalName, scope, deps, alreadyLocked) } // resolved: opts hydrated, fall through to installWithExtraction } + if deps != nil { + if deps.alreadyDone(opts.Name) { + return s.mergeRequiredByOnly(ctx, opts, opts.Name, scope) + } + if err := deps.enter(opts.Name); err != nil { + return nil, err + } + defer deps.leave(opts.Name) + } + + if err := validateExpectedCanonicalName(opts); err != nil { + return nil, err + } + // Local-store artifacts and raw layer data carry no registry signature // to verify — installing them project-scoped is an unsigned trust // decision that must be explicit. @@ -139,7 +167,7 @@ func (s *service) installByName( if err != nil { return nil, err } - return s.installAndRegister(ctx, opts, originalName, result, opts.Group, opts.Name, scope) + return s.installAndRegister(ctx, opts, originalName, result, opts.Group, opts.Name, scope, deps) } // installFromRegistryLookup resolves a plain skill name via the registry and @@ -149,13 +177,15 @@ func (s *service) installFromRegistryLookup( opts skills.InstallOptions, originalName string, scope skills.Scope, + deps *depState, + alreadyLocked bool, ) (*skills.InstallResult, error) { resolved, regErr := s.resolveFromRegistry(opts.Name) if regErr != nil { return nil, regErr } if resolved != nil { - return s.installFromResolvedRegistry(ctx, opts, originalName, scope, resolved) + return s.installFromResolvedRegistry(ctx, opts, originalName, scope, resolved, deps, alreadyLocked) } return nil, httperr.WithCode( @@ -174,27 +204,18 @@ func (s *service) installFromResolvedRegistry( originalName string, scope skills.Scope, resolved *registryResolveResult, + deps *depState, + alreadyLocked bool, ) (*skills.InstallResult, error) { switch { case resolved.OCIRef != nil: slog.Info("resolved skill from registry (OCI)", "name", opts.Name, "oci_reference", resolved.OCIRef.String()) opts.Name = resolved.OCIRef.String() - result, ociErr := s.installFromOCI(ctx, &opts, scope, resolved.OCIRef) - if ociErr != nil { - return nil, ociErr - } - // 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, opts, originalName, result, opts.Group, result.Skill.Metadata.Name, scope) + return s.installFromOCI(ctx, &opts, scope, originalName, deps, alreadyLocked, resolved.OCIRef) case resolved.GitURL != "": slog.Info("resolved skill from registry (git)", "name", opts.Name, "git_url", resolved.GitURL) opts.Name = resolved.GitURL - result, gitErr := s.installFromGit(ctx, &opts, scope) - if gitErr != nil { - return nil, gitErr - } - return s.installAndRegister(ctx, opts, originalName, result, opts.Group, result.Skill.Metadata.Name, scope) + return s.installFromGit(ctx, &opts, scope, originalName, deps, alreadyLocked) } return nil, httperr.WithCode( fmt.Errorf("skill %q resolved from registry but has no installable package", opts.Name), @@ -202,12 +223,20 @@ func (s *service) installFromResolvedRegistry( ) } +func resolvedGroupName(groupName string) string { + if groupName == "" { + return groups.DefaultGroup + } + return groupName +} + // registerSkillInGroup adds the skill to the requested group when a group // manager is configured. When groupName is empty it defaults to the -// "default" group, matching workload behavior. -func (s *service) registerSkillInGroup(ctx context.Context, groupName string, skillName string) error { +// "default" group, matching workload behavior. The bool reports whether this +// call inserted the name, so rollback can remove it only then. +func (s *service) registerSkillInGroup(ctx context.Context, groupName string, skillName string) (bool, error) { if s.groupManager == nil { - return nil + return false, nil } if groupName == "" { groupName = groups.DefaultGroup @@ -215,6 +244,65 @@ func (s *service) registerSkillInGroup(ctx context.Context, groupName string, sk return groups.AddSkillToGroup(ctx, s.groupManager, groupName, skillName) } +// validateExpectedCanonicalName rejects installs whose resolved manifest +// name does not match a caller-supplied ExpectedCanonicalName (sync/upgrade +// pins). Call after opts.Name has been hydrated from the artifact/manifest. +func validateExpectedCanonicalName(opts skills.InstallOptions) error { + if opts.ExpectedCanonicalName == "" || opts.Name == opts.ExpectedCanonicalName { + return nil + } + return httperr.WithCode( + fmt.Errorf("skill name %q does not match expected canonical name %q", + opts.Name, opts.ExpectedCanonicalName), + http.StatusUnprocessableEntity, + ) +} + +// mergeRequiredByOnly updates the lock entry so RequiredBy includes the +// current parent without re-extracting an already-materialized dependency +// (diamond / shared-dep within one traversal). +func (s *service) mergeRequiredByOnly( + ctx context.Context, + opts skills.InstallOptions, + skillName string, + scope skills.Scope, +) (*skills.InstallResult, error) { + existing, err := s.store.Get(ctx, skillName, scope, opts.ProjectRoot) + if err != nil { + return nil, err + } + if opts.RequiredByParent == "" || scope != skills.ScopeProject { + return &skills.InstallResult{Skill: existing}, nil + } + root, rootErr := lockfile.OpenRoot(opts.ProjectRoot) + if rootErr != nil { + return nil, rootErr + } + lf, loadErr := lockfile.Load(root) + if loadErr != nil { + return nil, loadErr + } + e, ok := lf.Get(skillName) + if !ok { + return &skills.InstallResult{Skill: existing}, nil + } + if err := recordLockEntry(opts.ProjectRoot, lockEntryInput{ + Name: e.Name, + Version: e.Version, + Source: e.Source, + ResolvedReference: e.ResolvedReference, + Digest: e.Digest, + ContentDigest: e.ContentDigest, + Provenance: e.Provenance, + Unsigned: e.Unsigned, + RequiredByParent: opts.RequiredByParent, + PreserveExplicit: true, + }); err != nil { + return nil, fmt.Errorf("merging RequiredBy for shared dependency %q: %w", skillName, err) + } + return &skills.InstallResult{Skill: existing}, nil +} + // installAndRegister registers the just-installed skill in the target group // and, for project-scope installs with the lock file feature enabled (see // records it — and any toolhive.requires @@ -232,6 +320,7 @@ func (s *service) installAndRegister( groupName string, skillName string, scope skills.Scope, + deps *depState, ) (*skills.InstallResult, error) { lockScoped := scope == skills.ScopeProject // Surface the verification decision on the result so callers can show @@ -241,32 +330,48 @@ func (s *service) installAndRegister( // 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. + // included) rather than blindly deleting it. OpenRoot/Load failures are + // fatal: treating them as "no previous pin" would delete a pre-existing + // entry on compensation. 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 - } - } + root, rootErr := lockfile.OpenRoot(opts.ProjectRoot) + if rootErr != nil { + return nil, errors.Join( + fmt.Errorf("opening lock file root: %w", rootErr), + s.rollbackInstall(ctx, opts, result, skillName, scope, false, nil, false, ""), + ) + } + lf, loadErr := lockfile.Load(root) + if loadErr != nil { + return nil, errors.Join( + fmt.Errorf("loading lock file: %w", loadErr), + s.rollbackInstall(ctx, opts, result, skillName, scope, false, nil, false, ""), + ) + } + if e, ok := lf.Get(skillName); ok { + prevEntry = &e } } - rollback := func() { s.rollbackInstall(ctx, opts, result, skillName, scope, lockScoped, prevEntry) } + resolvedGroup := resolvedGroupName(groupName) + var addedToGroup bool + rollback := func() error { + return s.rollbackInstall(ctx, opts, result, skillName, scope, lockScoped, prevEntry, addedToGroup, resolvedGroup) + } - if err := s.registerSkillInGroup(ctx, groupName, skillName); err != nil { - // 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) + added, err := s.registerSkillInGroup(ctx, groupName, skillName) + if err != nil { + // Rollback restores files from the pre-write snapshot and removes + // freshly created trees; its errors join the trigger error so a + // partial restore is never reported as clean. + return nil, errors.Join(fmt.Errorf("registering skill in group: %w", err), rollback()) } + addedToGroup = added if lockScoped { - updated, err := s.recordLockState(ctx, opts, originalName, result.Skill) + updated, err := s.recordLockState(ctx, opts, originalName, result.Skill, deps) if err != nil { - rollback() // Preserve a specific code already attached deeper in the chain // — dependency materialization runs inside recordLockState, so a // dep's 502 (git resolve) or 404 (registry miss) must reach the @@ -277,7 +382,7 @@ func (s *service) installAndRegister( if !errors.As(err, &coded) { wrapped = httperr.WithCode(wrapped, http.StatusInternalServerError) } - return nil, wrapped + return nil, errors.Join(wrapped, rollback()) } result.Skill = updated } @@ -287,13 +392,15 @@ func (s *service) installAndRegister( // 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. +// one exists (result.PreExisting) and deleted otherwise; overwritten files +// are restored when result.RestoreFiles is set; group membership added by +// this call is removed; 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, @@ -302,27 +409,54 @@ func (s *service) rollbackInstall( scope skills.Scope, lockScoped bool, prevEntry *lockfile.Entry, -) { + addedToGroup bool, + groupName string, +) error { + var errs []error if result.PreExisting != nil { - _ = s.store.Update(ctx, *result.PreExisting) + if err := s.store.Update(ctx, *result.PreExisting); err != nil { + errs = append(errs, fmt.Errorf("restoring pre-existing DB record: %w", err)) + } } else { - _ = s.store.Delete(ctx, skillName, scope, opts.ProjectRoot) + if err := s.store.Delete(ctx, skillName, scope, opts.ProjectRoot); err != nil { + errs = append(errs, fmt.Errorf("deleting rolled-back DB record: %w", err)) + } + } + + if result.RestoreFiles != nil { + if err := result.RestoreFiles(); err != nil { + errs = append(errs, err) + } + } + + if addedToGroup && s.groupManager != nil { + if err := groups.RemoveSkillFromGroup(ctx, s.groupManager, groupName, skillName); err != nil { + errs = append(errs, fmt.Errorf("removing skill from group: %w", err)) + } } if !lockScoped { - return + return errors.Join(errs...) } if prevEntry != nil { if root, err := lockfile.OpenRoot(opts.ProjectRoot); err == nil { - _ = lockfile.UpsertEntry(root, *prevEntry) + if err := lockfile.UpsertEntry(root, *prevEntry); err != nil { + errs = append(errs, fmt.Errorf("restoring lock entry: %w", err)) + } + } else { + errs = append(errs, fmt.Errorf("reopening lock file: %w", err)) } - return + return errors.Join(errs...) } uninstallOpts := skills.UninstallOptions{Name: skillName, Scope: scope, ProjectRoot: opts.ProjectRoot} candidates, err := removeLockEntry(uninstallOpts) if err != nil { - return + errs = append(errs, err) + return errors.Join(errs...) } visited := map[string]struct{}{skillName: {}} - _ = s.cascadeUninstall(ctx, candidates, visited, opts.ProjectRoot, scope) + if err := s.cascadeUninstall(ctx, candidates, visited, opts.ProjectRoot, scope); err != nil { + errs = append(errs, err) + } + return errors.Join(errs...) } diff --git a/pkg/skills/skillsvc/install_extraction.go b/pkg/skills/skillsvc/install_extraction.go index 4824a5d43e..8ee3a669d5 100644 --- a/pkg/skills/skillsvc/install_extraction.go +++ b/pkg/skills/skillsvc/install_extraction.go @@ -8,7 +8,6 @@ import ( "errors" "fmt" "net/http" - "os" "path/filepath" "time" @@ -106,34 +105,41 @@ func (s *service) installExtractionSameDigestNewClients( } return &skills.InstallResult{Skill: sk}, nil } - var written []string + targets := cleanedDirs(dirsToWrite, clientDirs) + backups, snapErr := snapshotTargets(targets) + if snapErr != nil { + return nil, fmt.Errorf("snapshotting skill trees before install: %w", snapErr) + } + restore := func() error { return restoreTargets(backups) } for _, ct := range dirsToWrite { dir := filepath.Clean(clientDirs[ct]) - if _, statErr := os.Stat(dir); statErr == nil && !opts.Force { // lgtm[go/path-injection] - removeSkillDirs(s.installer, clientDirs, written) - return nil, httperr.WithCode( - fmt.Errorf("directory %q exists but is not managed by ToolHive; use force to overwrite", dir), - http.StatusConflict, + if backups[dir].existed && !opts.Force { + return nil, errors.Join( + httperr.WithCode( + fmt.Errorf("directory %q exists but is not managed by ToolHive; use force to overwrite", dir), + http.StatusConflict, + ), + restore(), ) } if _, exErr := s.installer.Extract(opts.LayerData, dir, opts.Force); exErr != nil { - removeSkillDirs(s.installer, clientDirs, written) - return nil, fmt.Errorf("extracting skill: %w", exErr) + return nil, errors.Join(fmt.Errorf("extracting skill: %w", exErr), restore()) } - written = append(written, ct) } sk := buildInstalledSkill(opts, scope, clientTypes, existing.Clients) if err := s.store.Update(ctx, sk); err != nil { - removeSkillDirs(s.installer, clientDirs, written) - return nil, err + return nil, errors.Join(err, restore()) } - return &skills.InstallResult{Skill: sk}, nil + return &skills.InstallResult{Skill: sk, RestoreFiles: restore}, nil } -func removeSkillDirs(inst skills.Installer, clientDirs map[string]string, clients []string) { +// cleanedDirs maps client types to their cleaned target directories. +func cleanedDirs(clients []string, clientDirs map[string]string) []string { + out := make([]string, 0, len(clients)) for _, ct := range clients { - _ = inst.Remove(filepath.Clean(clientDirs[ct])) + out = append(out, filepath.Clean(clientDirs[ct])) } + return out } func (s *service) installExtractionUpgradeDigest( @@ -151,21 +157,22 @@ func (s *service) installExtractionUpgradeDigest( } // Deduplicate so clients sharing the same directory don't conflict. dirsToWrite := uniqueDirClients(allClients, allDirs, nil) - var written []string + backups, snapErr := snapshotTargets(cleanedDirs(dirsToWrite, allDirs)) + if snapErr != nil { + return nil, fmt.Errorf("snapshotting skill trees before upgrade: %w", snapErr) + } + restore := func() error { return restoreTargets(backups) } for _, ct := range dirsToWrite { dir := filepath.Clean(allDirs[ct]) if _, exErr := s.installer.Extract(opts.LayerData, dir, true); exErr != nil { - removeSkillDirs(s.installer, allDirs, written) - return nil, fmt.Errorf("extracting skill upgrade: %w", exErr) + return nil, errors.Join(fmt.Errorf("extracting skill upgrade: %w", exErr), restore()) } - written = append(written, ct) } sk := buildInstalledSkill(opts, scope, allClients, nil) if err := s.store.Update(ctx, sk); err != nil { - removeSkillDirs(s.installer, allDirs, dirsToWrite) - return nil, err + return nil, errors.Join(err, restore()) } - return &skills.InstallResult{Skill: sk}, nil + return &skills.InstallResult{Skill: sk, RestoreFiles: restore}, nil } func (s *service) installExtractionFresh( @@ -178,30 +185,31 @@ func (s *service) installExtractionFresh( // Deduplicate so clients sharing the same directory don't conflict. dirsToWrite := uniqueDirClients(clientTypes, clientDirs, nil) + backups, snapErr := snapshotTargets(cleanedDirs(dirsToWrite, clientDirs)) + if snapErr != nil { + return nil, fmt.Errorf("snapshotting skill trees before install: %w", snapErr) + } + restore := func() error { return restoreTargets(backups) } for _, ct := range dirsToWrite { dir := filepath.Clean(clientDirs[ct]) - if _, statErr := os.Stat(dir); statErr == nil && !opts.Force { // lgtm[go/path-injection] + if backups[dir].existed && !opts.Force { return nil, httperr.WithCode( fmt.Errorf("directory %q exists but is not managed by ToolHive; use force to overwrite", dir), http.StatusConflict, ) } } - var written []string for _, ct := range dirsToWrite { dir := filepath.Clean(clientDirs[ct]) if _, exErr := s.installer.Extract(opts.LayerData, dir, opts.Force); exErr != nil { - removeSkillDirs(s.installer, clientDirs, written) - return nil, fmt.Errorf("extracting skill: %w", exErr) + return nil, errors.Join(fmt.Errorf("extracting skill: %w", exErr), restore()) } - written = append(written, ct) } sk := buildInstalledSkill(opts, scope, clientTypes, nil) if err := s.store.Create(ctx, sk); err != nil { - removeSkillDirs(s.installer, clientDirs, dirsToWrite) - return nil, err + return nil, errors.Join(err, restore()) } - return &skills.InstallResult{Skill: sk}, nil + return &skills.InstallResult{Skill: sk, RestoreFiles: restore}, nil } // buildInstalledSkill constructs an InstalledSkill from install options. diff --git a/pkg/skills/skillsvc/install_git.go b/pkg/skills/skillsvc/install_git.go index 5ea8e247a5..58e7b5c8b2 100644 --- a/pkg/skills/skillsvc/install_git.go +++ b/pkg/skills/skillsvc/install_git.go @@ -18,12 +18,23 @@ import ( ) // installFromGit clones a git repository, extracts the skill, writes files to -// disk, and creates a DB record. The digest is the git commit hash, enabling +// disk, creates a DB record, and completes group/lock bookkeeping while the +// caller-held lock remains held. The digest is the git commit hash, enabling // same-commit no-op and upgrade detection. +// +// When alreadyLocked is false (user-scope), the per-skill lock is acquired +// after the canonical name is known and held through installAndRegister. +// When alreadyLocked is true (project transaction), no per-skill lock is +// taken — the project tx is the serialization boundary. +// +//nolint:gocyclo // resolve/hydrate/lock/persist form one transactional path func (s *service) installFromGit( ctx context.Context, opts *skills.InstallOptions, scope skills.Scope, + originalName string, + deps *depState, + alreadyLocked bool, ) (*skills.InstallResult, error) { if s.gitResolver == nil { return nil, httperr.WithCode( @@ -75,12 +86,28 @@ func (s *service) installFromGit( opts.Version = resolved.SkillConfig.Version } - unlock := s.locks.lock(opts.Name, scope, opts.ProjectRoot) - defer unlock() + if err := validateExpectedCanonicalName(*opts); err != nil { + return nil, err + } + + if deps != nil { + if deps.alreadyDone(opts.Name) { + return s.mergeRequiredByOnly(ctx, *opts, opts.Name, scope) + } + if err := deps.enter(opts.Name); err != nil { + return nil, err + } + defer deps.leave(opts.Name) + } + + if !alreadyLocked { + unlock := s.locks.lock(opts.Name, scope, opts.ProjectRoot) + defer unlock() + } // Verify the commit signature before anything is written or recorded. - // This runs under the per-skill lock so concurrent first installs - // cannot both read an absent lock entry and race their TOFU anchors. + // This runs under the held lock so concurrent first installs cannot + // both read an absent lock entry and race their TOFU anchors. if shouldVerifyInstall(*opts, scope) { decision, verifyErr := s.verifyGitInstall( ctx, *opts, resolved.SkillConfig.Name, resolved.CommitPayload, resolved.CommitSignature, @@ -96,7 +123,11 @@ func (s *service) installFromGit( return nil, err } - return s.applyGitInstall(ctx, *opts, scope, clientTypes, clientDirs, resolved.Files) + result, err := s.applyGitInstall(ctx, *opts, scope, clientTypes, clientDirs, resolved.Files) + if err != nil { + return nil, err + } + return s.installAndRegister(ctx, *opts, originalName, result, opts.Group, result.Skill.Metadata.Name, scope, deps) } // applyGitInstall handles the create/upgrade/no-op logic for a git-based skill @@ -205,8 +236,10 @@ func (s *service) applyGitInstallFresh( } // gitWriteMultiAndPersist writes git files to the given client directories, -// verifies each tree, then creates or updates the store record. On failure -// after any write, previously written directories in this call are removed. +// verifies each tree, then creates or updates the store record. Every target +// is snapshotted first (recording whether it existed) so any failure — and +// any later installAndRegister rollback — restores prior content and removes +// freshly created trees, with all compensation errors joined into the result. func (s *service) gitWriteMultiAndPersist( ctx context.Context, opts skills.InstallOptions, @@ -218,7 +251,12 @@ func (s *service) gitWriteMultiAndPersist( existingClients []string, isUpgrade, writeAggressive bool, ) (*skills.InstallResult, error) { - var written []string + backups, snapErr := snapshotTargets(cleanedDirs(dirsToWrite, clientDirs)) + if snapErr != nil { + return nil, fmt.Errorf("snapshotting skill trees before write: %w", snapErr) + } + restore := func() error { return restoreTargets(backups) } + for _, ct := range dirsToWrite { dir := filepath.Clean(clientDirs[ct]) writeMode := opts.Force @@ -226,36 +264,22 @@ func (s *service) gitWriteMultiAndPersist( writeMode = true } if writeErr := gitresolver.WriteFiles(files, dir, writeMode); writeErr != nil { - for _, wct := range written { - _ = s.installer.Remove(filepath.Clean(clientDirs[wct])) - } - return nil, fmt.Errorf("writing git skill: %w", writeErr) + return nil, errors.Join(fmt.Errorf("writing git skill: %w", writeErr), restore()) } if checkErr := skills.CheckFilesystem(dir); checkErr != nil { - _ = s.installer.Remove(dir) - for _, wct := range written { - _ = s.installer.Remove(filepath.Clean(clientDirs[wct])) - } - return nil, fmt.Errorf("post-extraction verification failed: %w", checkErr) + return nil, errors.Join(fmt.Errorf("post-extraction verification failed: %w", checkErr), restore()) } - written = append(written, ct) } sk := buildInstalledSkill(opts, scope, allRequested, existingClients) + var persistErr error if isUpgrade { - if err := s.store.Update(ctx, sk); err != nil { - for _, wct := range written { - _ = s.installer.Remove(filepath.Clean(clientDirs[wct])) - } - return nil, err - } + persistErr = s.store.Update(ctx, sk) } else { - if err := s.store.Create(ctx, sk); err != nil { - for _, wct := range written { - _ = s.installer.Remove(filepath.Clean(clientDirs[wct])) - } - return nil, err - } + persistErr = s.store.Create(ctx, sk) + } + if persistErr != nil { + return nil, errors.Join(persistErr, restore()) } - return &skills.InstallResult{Skill: sk}, nil + return &skills.InstallResult{Skill: sk, RestoreFiles: restore}, nil } diff --git a/pkg/skills/skillsvc/install_oci.go b/pkg/skills/skillsvc/install_oci.go index b902b18ade..00d9540091 100644 --- a/pkg/skills/skillsvc/install_oci.go +++ b/pkg/skills/skillsvc/install_oci.go @@ -29,11 +29,22 @@ const ociPullTimeout = 5 * time.Minute const maxCompressedLayerSize int64 = 50 * 1024 * 1024 // 50 MB // installFromOCI pulls a skill artifact from a remote registry, extracts -// metadata and layer data, then delegates to the standard extraction flow. +// metadata and layer data, then completes extraction and bookkeeping while +// the caller-held lock remains held. +// +// When alreadyLocked is false (user-scope), the per-skill lock is acquired +// after the canonical name is known and held through installAndRegister. +// When alreadyLocked is true (project transaction), no per-skill lock is +// taken — the project tx is the serialization boundary. +// +//nolint:gocyclo // resolve/hydrate/lock/persist form one transactional path func (s *service) installFromOCI( ctx context.Context, opts *skills.InstallOptions, scope skills.Scope, + originalName string, + deps *depState, + alreadyLocked bool, ref nameref.Reference, ) (*skills.InstallResult, error) { if s.registry == nil || s.ociStore == nil { @@ -110,14 +121,30 @@ func (s *service) installFromOCI( } // Note: version is optional; if both are empty, install without a version. - unlock := s.locks.lock(opts.Name, scope, opts.ProjectRoot) - defer unlock() + if err := validateExpectedCanonicalName(*opts); err != nil { + return nil, err + } + + if deps != nil { + if deps.alreadyDone(opts.Name) { + return s.mergeRequiredByOnly(ctx, *opts, opts.Name, scope) + } + if err := deps.enter(opts.Name); err != nil { + return nil, err + } + defer deps.leave(opts.Name) + } + + if !alreadyLocked { + unlock := s.locks.lock(opts.Name, scope, opts.ProjectRoot) + defer unlock() + } // Verify the artifact signature before anything is extracted or // recorded; the decision (verified identity or explicit unsigned // exception) travels on opts into the DB record and lock entry. This - // runs under the per-skill lock so concurrent first installs cannot - // both read an absent lock entry and race their TOFU anchors. + // runs under the held lock so concurrent first installs cannot both + // read an absent lock entry and race their TOFU anchors. if shouldVerifyInstall(*opts, scope) { decision, verifyErr := s.verifyOCIInstall(ctx, *opts, skillConfig.Name, ociRef, opts.Digest) if verifyErr != nil { @@ -126,7 +153,11 @@ func (s *service) installFromOCI( applyDecisionToOpts(opts, decision) } - return s.installWithExtraction(ctx, *opts, scope) + result, err := s.installWithExtraction(ctx, *opts, scope) + if err != nil { + return nil, err + } + return s.installAndRegister(ctx, *opts, originalName, result, opts.Group, result.Skill.Metadata.Name, scope, deps) } // resolveFromLocalStore attempts to resolve a skill name against the local diff --git a/pkg/skills/skillsvc/install_test.go b/pkg/skills/skillsvc/install_test.go index 2d57f1ac00..45906ea0a2 100644 --- a/pkg/skills/skillsvc/install_test.go +++ b/pkg/skills/skillsvc/install_test.go @@ -73,6 +73,22 @@ func TestInstallPlainNameNotFound(t *testing.T) { } } +// extractWritingTree returns an Extract mock body that actually writes a +// skill tree to disk, so rollback assertions can verify real filesystem +// compensation instead of installer.Remove call counts. +func extractWritingTree(t *testing.T) func([]byte, string, bool) (*skills.ExtractResult, error) { + t.Helper() + return func(_ []byte, dir string, _ bool) (*skills.ExtractResult, error) { + if err := os.MkdirAll(dir, 0o750); err != nil { + return nil, err + } + if err := os.WriteFile(filepath.Join(dir, "SKILL.md"), []byte("# extracted"), 0o644); err != nil { + return nil, err + } + return &skills.ExtractResult{SkillDir: dir, Files: 1}, nil + } +} + func TestInstallWithExtraction(t *testing.T) { t.Parallel() @@ -407,9 +423,8 @@ func TestInstallWithExtraction(t *testing.T) { pr.EXPECT().ListSkillSupportingClients().Return([]string{"claude-code"}) pr.EXPECT().GetSkillPath("claude-code", "my-skill", skills.ScopeUser, "").Return(targetDir, nil) store.EXPECT().Get(gomock.Any(), "my-skill", skills.ScopeUser, "").Return(skills.InstalledSkill{}, storage.ErrNotFound) - inst.EXPECT().Extract(layerData, targetDir, false).Return(&skills.ExtractResult{SkillDir: targetDir, Files: 1}, nil) + inst.EXPECT().Extract(layerData, targetDir, false).DoAndReturn(extractWritingTree(t)) store.EXPECT().Create(gomock.Any(), gomock.Any()).Return(fmt.Errorf("db write error")) - inst.EXPECT().Remove(targetDir).Return(nil) svc := New(store, WithPathResolver(pr), WithInstaller(inst)) _, err := svc.Install(t.Context(), skills.InstallOptions{ @@ -419,6 +434,7 @@ func TestInstallWithExtraction(t *testing.T) { }) require.Error(t, err) assert.Contains(t, err.Error(), "db write error") + assert.NoDirExists(t, targetDir, "rollback must remove the freshly written skill tree") }) t.Run("upgrade rolls back extraction on store.Update failure", func(t *testing.T) { @@ -439,9 +455,11 @@ func TestInstallWithExtraction(t *testing.T) { pr.EXPECT().ListSkillSupportingClients().Return([]string{"claude-code"}) pr.EXPECT().GetSkillPath("claude-code", "my-skill", skills.ScopeUser, "").Return(targetDir, nil) store.EXPECT().Get(gomock.Any(), "my-skill", skills.ScopeUser, "").Return(existing, nil) - inst.EXPECT().Extract(layerData, targetDir, true).Return(&skills.ExtractResult{SkillDir: targetDir, Files: 1}, nil) + // The tree exists on disk before the upgrade, holding the old content. + require.NoError(t, os.MkdirAll(targetDir, 0o750)) + require.NoError(t, os.WriteFile(filepath.Join(targetDir, "SKILL.md"), []byte("old content"), 0o644)) + inst.EXPECT().Extract(layerData, targetDir, true).DoAndReturn(extractWritingTree(t)) store.EXPECT().Update(gomock.Any(), gomock.Any()).Return(fmt.Errorf("db update error")) - inst.EXPECT().Remove(targetDir).Return(nil) svc := New(store, WithPathResolver(pr), WithInstaller(inst)) _, err := svc.Install(t.Context(), skills.InstallOptions{ @@ -451,6 +469,9 @@ func TestInstallWithExtraction(t *testing.T) { }) require.Error(t, err) assert.Contains(t, err.Error(), "db update error") + got, readErr := os.ReadFile(filepath.Join(targetDir, "SKILL.md")) + require.NoError(t, readErr) + assert.Equal(t, "old content", string(got), "rollback must restore the pre-upgrade tree") }) t.Run("multi-client fresh install rolls back first client on second extract failure", func(t *testing.T) { @@ -466,11 +487,9 @@ func TestInstallWithExtraction(t *testing.T) { pr.EXPECT().GetSkillPath("opencode", "my-skill", skills.ScopeUser, "").Return(dirB, nil) store.EXPECT().Get(gomock.Any(), "my-skill", skills.ScopeUser, ""). Return(skills.InstalledSkill{}, storage.ErrNotFound) - inst.EXPECT().Extract(layerData, dirA, false). - Return(&skills.ExtractResult{SkillDir: dirA, Files: 1}, nil) + inst.EXPECT().Extract(layerData, dirA, false).DoAndReturn(extractWritingTree(t)) inst.EXPECT().Extract(layerData, dirB, false). Return(nil, fmt.Errorf("disk full")) - inst.EXPECT().Remove(dirA).Return(nil) svc := New(store, WithPathResolver(pr), WithInstaller(inst)) _, err := svc.Install(t.Context(), skills.InstallOptions{ @@ -481,6 +500,7 @@ func TestInstallWithExtraction(t *testing.T) { }) require.Error(t, err) assert.Contains(t, err.Error(), "disk full") + assert.NoDirExists(t, dirA, "rollback must remove the first client's fresh tree") }) t.Run("upgrade digest rolls back written clients on second extract failure", func(t *testing.T) { @@ -500,11 +520,9 @@ func TestInstallWithExtraction(t *testing.T) { pr.EXPECT().GetSkillPath("claude-code", "my-skill", skills.ScopeUser, "").Return(dirA, nil) pr.EXPECT().GetSkillPath("opencode", "my-skill", skills.ScopeUser, "").Return(dirB, nil) store.EXPECT().Get(gomock.Any(), "my-skill", skills.ScopeUser, "").Return(existing, nil) - inst.EXPECT().Extract(layerData, dirA, true). - Return(&skills.ExtractResult{SkillDir: dirA, Files: 1}, nil) + inst.EXPECT().Extract(layerData, dirA, true).DoAndReturn(extractWritingTree(t)) inst.EXPECT().Extract(layerData, dirB, true). Return(nil, fmt.Errorf("write error")) - inst.EXPECT().Remove(dirA).Return(nil) svc := New(store, WithPathResolver(pr), WithInstaller(inst)) _, err := svc.Install(t.Context(), skills.InstallOptions{ @@ -515,6 +533,7 @@ func TestInstallWithExtraction(t *testing.T) { }) require.Error(t, err) assert.Contains(t, err.Error(), "write error") + assert.NoDirExists(t, dirA, "rollback must remove the freshly written tree") }) t.Run("upgrade digest rolls back all clients on store.Update failure", func(t *testing.T) { @@ -534,13 +553,9 @@ func TestInstallWithExtraction(t *testing.T) { pr.EXPECT().GetSkillPath("claude-code", "my-skill", skills.ScopeUser, "").Return(dirA, nil) pr.EXPECT().GetSkillPath("opencode", "my-skill", skills.ScopeUser, "").Return(dirB, nil) store.EXPECT().Get(gomock.Any(), "my-skill", skills.ScopeUser, "").Return(existing, nil) - inst.EXPECT().Extract(layerData, dirA, true). - Return(&skills.ExtractResult{SkillDir: dirA, Files: 1}, nil) - inst.EXPECT().Extract(layerData, dirB, true). - Return(&skills.ExtractResult{SkillDir: dirB, Files: 1}, nil) + inst.EXPECT().Extract(layerData, dirA, true).DoAndReturn(extractWritingTree(t)) + inst.EXPECT().Extract(layerData, dirB, true).DoAndReturn(extractWritingTree(t)) store.EXPECT().Update(gomock.Any(), gomock.Any()).Return(fmt.Errorf("db failure")) - inst.EXPECT().Remove(dirA).Return(nil) - inst.EXPECT().Remove(dirB).Return(nil) svc := New(store, WithPathResolver(pr), WithInstaller(inst)) _, err := svc.Install(t.Context(), skills.InstallOptions{ @@ -551,6 +566,8 @@ func TestInstallWithExtraction(t *testing.T) { }) require.Error(t, err) assert.Contains(t, err.Error(), "db failure") + assert.NoDirExists(t, dirA, "rollback must remove every freshly written tree") + assert.NoDirExists(t, dirB, "rollback must remove every freshly written tree") }) t.Run("same digest new client rolls back on extract failure", func(t *testing.T) { @@ -600,10 +617,8 @@ func TestInstallWithExtraction(t *testing.T) { pr.EXPECT().GetSkillPath("claude-code", "my-skill", skills.ScopeUser, "").Return(dirA, nil) pr.EXPECT().GetSkillPath("opencode", "my-skill", skills.ScopeUser, "").Return(dirB, nil) store.EXPECT().Get(gomock.Any(), "my-skill", skills.ScopeUser, "").Return(existing, nil) - inst.EXPECT().Extract(layerData, dirB, false). - Return(&skills.ExtractResult{SkillDir: dirB, Files: 1}, nil) + inst.EXPECT().Extract(layerData, dirB, false).DoAndReturn(extractWritingTree(t)) store.EXPECT().Update(gomock.Any(), gomock.Any()).Return(fmt.Errorf("db update boom")) - inst.EXPECT().Remove(dirB).Return(nil) svc := New(store, WithPathResolver(pr), WithInstaller(inst)) _, err := svc.Install(t.Context(), skills.InstallOptions{ @@ -614,6 +629,7 @@ func TestInstallWithExtraction(t *testing.T) { }) require.Error(t, err) assert.Contains(t, err.Error(), "db update boom") + assert.NoDirExists(t, dirB, "rollback must remove the freshly added client tree") }) t.Run("same digest new client unmanaged dir conflict", func(t *testing.T) { diff --git a/pkg/skills/skillsvc/lock.go b/pkg/skills/skillsvc/lock.go index 393470d15c..b1bc19020b 100644 --- a/pkg/skills/skillsvc/lock.go +++ b/pkg/skills/skillsvc/lock.go @@ -30,6 +30,7 @@ func (s *service) recordLockState( opts skills.InstallOptions, originalName string, sk skills.InstalledSkill, + deps *depState, ) (skills.InstalledSkill, error) { contentDigest, err := computeContentDigest(s.pathResolver, sk) if err != nil { @@ -72,7 +73,7 @@ func (s *service) recordLockState( // each dependency from its mutable source string — silently upgrading // and re-pinning it, the opposite of a deterministic restore. if !opts.SyncRestore { - if err := s.materializeDependencies(ctx, opts, source, sk); err != nil { + if err := s.materializeDependencies(ctx, opts, sk, deps); err != nil { return sk, fmt.Errorf("materializing dependencies: %w", err) } } @@ -81,23 +82,19 @@ func (s *service) recordLockState( // 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. +// further dependencies). deps tracks active and completed canonical skill +// names after source resolution so alias cycles are rejected and diamond +// RequiredBy edges merge without reacquiring locks. skills.MaxDependencies +// bounds the total number of unique canonical skills materialized across +// the whole tree. // -// 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. +// Callers must already hold the project transaction. Dependencies are +// installed via installLocked — never the public Install entry. func (s *service) materializeDependencies( ctx context.Context, opts skills.InstallOptions, - source string, sk skills.InstalledSkill, + deps *depState, ) error { parsed, err := readSkillMD(s.pathResolver, sk) if err != nil { @@ -107,21 +104,15 @@ func (s *service) materializeDependencies( return nil } - visited := opts.Visited - if visited == nil { - visited = make(map[string]struct{}) + if deps == nil { + deps = newDepState() } - visited[source] = struct{}{} for _, dep := range parsed.Requires { - if _, seen := visited[dep.Reference]; seen { - continue - } - if len(visited) >= skills.MaxDependencies { + if len(deps.completed)+len(deps.active) >= 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, @@ -130,9 +121,8 @@ func (s *service) materializeDependencies( 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 { + if _, err := s.installLocked(ctx, depOpts, dep.Reference, sk.Scope, deps); err != nil { return fmt.Errorf("installing dependency %q (required by %q): %w", dep.Reference, sk.Metadata.Name, err) } } diff --git a/pkg/skills/skillsvc/lock_test.go b/pkg/skills/skillsvc/lock_test.go index 98a17f1832..c20d56cf0e 100644 --- a/pkg/skills/skillsvc/lock_test.go +++ b/pkg/skills/skillsvc/lock_test.go @@ -5,6 +5,7 @@ package skillsvc import ( "context" + "errors" "fmt" "net/http" "os" @@ -26,6 +27,7 @@ import ( "github.com/stacklok/toolhive/pkg/skills/lockfile" skillsmocks "github.com/stacklok/toolhive/pkg/skills/mocks" verifiermocks "github.com/stacklok/toolhive/pkg/skills/verifier/mocks" + "github.com/stacklok/toolhive/pkg/storage" "github.com/stacklok/toolhive/pkg/storage/sqlite" ) @@ -57,7 +59,8 @@ func newLockTestService(t *testing.T, gr *gitmocks.MockResolver, extra ...Option // skill-supporting client detected on the host. Deliberately returns // more than one client so tests can distinguish "preserved the skill's // existing (narrower) client list" from "fell through to every detected - // client" (see TestUpgrade_PreservesExistingClients). + // client" (see TestUpgrade_PreservesExistingClients and + // TestSync_DefaultExpandsToNewlyDetectedClients). pr.EXPECT().ListSkillSupportingClients().AnyTimes().Return([]string{"claude-code", "cursor"}) // Default verifier: everything verifies as signed by a fixed test @@ -288,21 +291,21 @@ func TestMaterializeDependencies_RejectsTreeExceedingMaxDependencies(t *testing. 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 + // Pre-fill completed 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) + deps := newDepState() for i := range skills.MaxDependencies { - visited[fmt.Sprintf("seen-%d", i)] = struct{}{} + deps.completed[fmt.Sprintf("seen-%d", i)] = struct{}{} } - err := svcImpl.materializeDependencies(t.Context(), skills.InstallOptions{Visited: visited}, "root-skill", sk) + err := svcImpl.materializeDependencies(t.Context(), skills.InstallOptions{}, sk, deps) 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) { +func TestInstallProjectScope_DependencyCycleRejected(t *testing.T) { gr, fx := newGitResolverMock(t) refA, _ := gitRef("skill-a") refB, _ := gitRef("skill-b") @@ -319,24 +322,11 @@ func TestInstallProjectScope_DependencyCycleTerminates(t *testing.T) { }() select { case err := <-done: - require.NoError(t, err) + require.Error(t, err, "canonical dependency cycles must be rejected, not soft-skipped") + assert.Contains(t, err.Error(), "dependency cycle") 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 @@ -358,6 +348,261 @@ 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") + + assert.NoDirExists(t, filepath.Join(projectRoot, ".claude", "skills", "my-skill"), + "a fresh install rolled back by a lock-write failure must not leave its tree on disk") +} + +// perClientPathResolver returns a PathResolver whose skill dirs differ per +// client, so add-client installs write a genuinely new directory. +func perClientPathResolver(t *testing.T) *skillsmocks.MockPathResolver { + t.Helper() + ctrl := gomock.NewController(t) + pr := skillsmocks.NewMockPathResolver(ctrl) + pr.EXPECT().GetSkillPath(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes(). + DoAndReturn(func(client, skillName string, _ skills.Scope, projectRoot string) (string, error) { + return filepath.Join(projectRoot, "."+client, "skills", skillName), nil + }) + pr.EXPECT().ListSkillSupportingClients().AnyTimes().Return([]string{"claude-code", "cursor"}) + return pr +} + +// A lock-write failure on a same-digest add-client install must remove the +// newly written client tree and leave the original install untouched. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestInstallProjectScope_LockWriteFailureRemovesAddedClientTree(t *testing.T) { + gr, fx := newGitResolverMock(t) + fx.register("my-skill", gitSkill("my-skill")) + svc, projectRoot := newLockTestService(t, gr, WithPathResolver(perClientPathResolver(t))) + + 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) + claudeTree := filepath.Join(projectRoot, ".claude-code", "skills", "my-skill") + require.DirExists(t, claudeTree) + + // Break lock writes for the add-client attempt. + lockPath := filepath.Join(projectRoot, lockfile.FileName) + require.NoError(t, os.Remove(lockPath)) + require.NoError(t, os.MkdirAll(lockPath, 0o755)) + + _, err = svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"cursor"}, + }) + require.Error(t, err, "the add-client install must fail when the lock cannot be written") + + assert.NoDirExists(t, filepath.Join(projectRoot, ".cursor", "skills", "my-skill"), + "the freshly written cursor tree must be removed on rollback") + assert.DirExists(t, claudeTree, "the original claude-code tree must survive") + + info, err := svc.Info(t.Context(), skills.InfoOptions{ + Name: "my-skill", Scope: skills.ScopeProject, ProjectRoot: projectRoot, + }) + require.NoError(t, err, "the pre-existing DB record must be restored, not deleted") + assert.Equal(t, []string{"claude-code"}, info.InstalledSkill.Clients, + "the restored record must not list the rolled-back client") +} + +// hookSkillStore lets tests inject storage failures into specific operations. +type hookSkillStore struct { + storage.SkillStore + deleteErr error + updateErr error + afterCreate func() +} + +func (s *hookSkillStore) Delete(ctx context.Context, name string, scope skills.Scope, projectRoot string) error { + if s.deleteErr != nil { + return s.deleteErr + } + return s.SkillStore.Delete(ctx, name, scope, projectRoot) +} + +func (s *hookSkillStore) Update(ctx context.Context, sk skills.InstalledSkill) error { + if s.updateErr != nil { + return s.updateErr + } + return s.SkillStore.Update(ctx, sk) +} + +func (s *hookSkillStore) Create(ctx context.Context, sk skills.InstalledSkill) error { + if err := s.SkillStore.Create(ctx, sk); err != nil { + return err + } + if s.afterCreate != nil { + s.afterCreate() + } + return nil +} + +// A rollback whose own storage compensation fails must surface that failure +// joined with the original error, not report only the trigger. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestInstallProjectScope_RollbackStorageFailureIsJoined(t *testing.T) { + gr, fx := newGitResolverMock(t) + fx.register("my-skill", gitSkill("my-skill")) + + // Group registration fails after extraction+DB create, triggering the + // rollback whose DB delete then also fails. + ctrl := gomock.NewController(t) + gm := groupmocks.NewMockManager(ctrl) + gm.EXPECT().Get(gomock.Any(), gomock.Any()).Return(nil, errors.New("group backend down")).AnyTimes() + + svc, projectRoot := newLockTestService(t, gr, WithGroupManager(gm)) + inner := svc.(*service) //nolint:forcetypeassert + inner.store = &hookSkillStore{ + SkillStore: inner.store, + deleteErr: errors.New("db delete unavailable"), + } + + 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) + assert.Contains(t, err.Error(), "registering skill in group", + "the original group registration failure must be reported") + assert.Contains(t, err.Error(), "db delete unavailable", + "the failed rollback deletion must be joined into the returned error") +} + +// A failed force reinstall over a lock-managed skill must restore the prior +// lock entry rather than delete it. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestInstallProjectScope_GroupFailureRestoresPriorLockEntry(t *testing.T) { + gr, fx := newGitResolverMock(t) + fx.register("my-skill", gitSkill("my-skill")) + + // The group manager succeeds for the first install and fails afterwards. + ctrl := gomock.NewController(t) + gm := groupmocks.NewMockManager(ctrl) + calls := 0 + gm.EXPECT().Get(gomock.Any(), gomock.Any()).AnyTimes().DoAndReturn( + func(_ context.Context, name string) (*groups.Group, error) { + calls++ + if calls > 1 { + return nil, errors.New("group backend down") + } + return &groups.Group{Name: name}, nil + }, + ) + gm.EXPECT().Update(gomock.Any(), gomock.Any()).AnyTimes().Return(nil) + + svc, projectRoot := newLockTestService(t, gr, WithGroupManager(gm)) + + 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) + prevEntry, ok := readLockfile(t, projectRoot).Get("my-skill") + require.True(t, ok) + + _, err = svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, Force: true, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "registering skill in group") + + after, ok := readLockfile(t, projectRoot).Get("my-skill") + require.True(t, ok, "the prior lock entry must be restored, not deleted") + assert.Equal(t, prevEntry.Digest, after.Digest) + + info, err := svc.Info(t.Context(), skills.InfoOptions{ + Name: "my-skill", Scope: skills.ScopeProject, ProjectRoot: projectRoot, + }) + require.NoError(t, err, "the pre-existing DB record must be restored") + require.NotNil(t, info.InstalledSkill) +} + +// A rollback that fails to restore the pre-existing DB record must join +// that failure with the trigger error. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestInstallProjectScope_RollbackUpdateFailureIsJoined(t *testing.T) { + gr, fx := newGitResolverMock(t) + fx.register("my-skill", gitSkill("my-skill")) + + ctrl := gomock.NewController(t) + gm := groupmocks.NewMockManager(ctrl) + calls := 0 + gm.EXPECT().Get(gomock.Any(), gomock.Any()).AnyTimes().DoAndReturn( + func(_ context.Context, name string) (*groups.Group, error) { + calls++ + if calls > 1 { + return nil, errors.New("group backend down") + } + return &groups.Group{Name: name}, nil + }, + ) + gm.EXPECT().Update(gomock.Any(), gomock.Any()).AnyTimes().Return(nil) + + svc, projectRoot := newLockTestService(t, gr, WithGroupManager(gm)) + inner := svc.(*service) //nolint:forcetypeassert + + 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) + + // Restoring the pre-existing DB record during rollback now fails. + inner.store = &hookSkillStore{ + SkillStore: inner.store, + updateErr: errors.New("db update unavailable"), + } + + _, err = svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, Force: true, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "registering skill in group") + assert.Contains(t, err.Error(), "db update unavailable", + "the failed pre-existing record restore must be joined into the returned error") +} + +// A lock file that becomes unreadable between extraction and bookkeeping +// must fail closed: the fresh install is rolled back and the load error is +// joined with the compensation result. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestInstallProjectScope_LockSnapshotFailureRollsBackFreshInstall(t *testing.T) { + gr, fx := newGitResolverMock(t) + fx.register("my-skill", gitSkill("my-skill")) + svc, projectRoot := newLockTestService(t, gr) + inner := svc.(*service) //nolint:forcetypeassert + + // Corrupt the lock file only after the DB row exists, so the prior-entry + // snapshot inside installAndRegister is what fails. + inner.store = &hookSkillStore{ + SkillStore: inner.store, + afterCreate: func() { + lockPath := filepath.Join(projectRoot, lockfile.FileName) + _ = os.Remove(lockPath) + _ = os.MkdirAll(lockPath, 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) + assert.Contains(t, err.Error(), "loading lock file") + + _, 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 when the lock snapshot fails") + assert.NoDirExists(t, filepath.Join(projectRoot, ".claude", "skills", "my-skill"), + "the freshly extracted tree must be removed") } // TestInstallProjectScope_DependencyFailureRollsBackParentLockEntry covers a diff --git a/pkg/skills/skillsvc/project_tx_test.go b/pkg/skills/skillsvc/project_tx_test.go new file mode 100644 index 0000000000..36b752aef7 --- /dev/null +++ b/pkg/skills/skillsvc/project_tx_test.go @@ -0,0 +1,351 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package skillsvc + +import ( + "context" + "fmt" + "os" + "path/filepath" + "sync" + "sync/atomic" + "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" +) + +// TestInstallGit_HoldsProjectTxThroughRegister proves a concurrent uninstall +// cannot observe mid-install state: the project transaction spans git +// resolve, extraction, DB, group, and lock-file bookkeeping. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestInstallGit_HoldsProjectTxThroughRegister(t *testing.T) { + ctrl := gomock.NewController(t) + gr := gitmocks.NewMockResolver(ctrl) + + var resolveStarted, allowFinish sync.WaitGroup + resolveStarted.Add(1) + allowFinish.Add(1) + + ref, url := gitRef("locked-skill") + content := gitSkill("locked-skill") + gr.EXPECT().Resolve(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, _ *gitresolver.GitReference) (*gitresolver.ResolveResult, error) { + resolveStarted.Done() + allowFinish.Wait() + return &gitresolver.ResolveResult{ + SkillConfig: &skills.ParseResult{Name: "locked-skill"}, + Files: []gitresolver.FileEntry{{Path: "SKILL.md", Content: content, Mode: 0644}}, + CommitHash: fixtureCommitHash(url, content), + }, nil + }, + ) + + svc, projectRoot := newLockTestService(t, gr) + + installDone := make(chan error, 1) + go func() { + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + installDone <- err + }() + + resolveStarted.Wait() + + uninstallStarted := make(chan struct{}) + uninstallDone := make(chan error, 1) + go func() { + close(uninstallStarted) + uninstallDone <- svc.Uninstall(t.Context(), skills.UninstallOptions{ + Name: "locked-skill", Scope: skills.ScopeProject, ProjectRoot: projectRoot, + }) + }() + + select { + case <-uninstallStarted: + case <-time.After(5 * time.Second): + t.Fatal("uninstall goroutine did not start") + } + + // Uninstall must block on the project tx until install finishes register. + select { + case err := <-uninstallDone: + t.Fatalf("uninstall completed while install still held the project tx: %v", err) + case <-time.After(100 * time.Millisecond): + // expected: still blocked + } + + allowFinish.Done() + + select { + case err := <-installDone: + require.NoError(t, err) + case <-time.After(10 * time.Second): + t.Fatal("install timed out") + } + + select { + case err := <-uninstallDone: + require.NoError(t, err) + case <-time.After(10 * time.Second): + t.Fatal("uninstall timed out after install released the project tx") + } + + _, err := svc.Info(t.Context(), skills.InfoOptions{ + Name: "locked-skill", Scope: skills.ScopeProject, ProjectRoot: projectRoot, + }) + require.Error(t, err, "uninstall that ran after install must have removed the skill") +} + +// TestSyncVsUninstall_SerializedOnProjectTx ensures sync and uninstall on the +// same project cannot interleave. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestSyncVsUninstall_SerializedOnProjectTx(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) + + var inSync atomic.Bool + var overlapped atomic.Bool + syncer := svc.(*service) //nolint:forcetypeassert + + // Hold the project tx the way Sync does, with a barrier so uninstall + // can attempt to enter while sync is "in progress". + syncDone := make(chan struct{}) + go func() { + unlock := syncer.projectTx.lock(projectRoot) + inSync.Store(true) + time.Sleep(150 * time.Millisecond) + inSync.Store(false) + unlock() + close(syncDone) + }() + + // Wait until sync holds the lock. + require.Eventually(t, func() bool { return inSync.Load() }, 2*time.Second, 5*time.Millisecond) + + uninstallDone := make(chan error, 1) + go func() { + err := svc.Uninstall(t.Context(), skills.UninstallOptions{ + Name: "my-skill", Scope: skills.ScopeProject, ProjectRoot: projectRoot, + }) + if inSync.Load() { + overlapped.Store(true) + } + uninstallDone <- err + }() + + <-syncDone + select { + case err := <-uninstallDone: + require.NoError(t, err) + case <-time.After(10 * time.Second): + t.Fatal("uninstall timed out") + } + assert.False(t, overlapped.Load(), "uninstall must not run while sync holds the project tx") +} + +// TestUpgradeVsUninstall_SerializedOnProjectTx ensures upgrade and uninstall +// on the same project cannot interleave. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestUpgradeVsUninstall_SerializedOnProjectTx(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) + + upgrader := svc.(*service) //nolint:forcetypeassert + var inUpgrade atomic.Bool + var overlapped atomic.Bool + + upgradeDone := make(chan struct{}) + go func() { + unlock := upgrader.projectTx.lock(projectRoot) + inUpgrade.Store(true) + time.Sleep(150 * time.Millisecond) + inUpgrade.Store(false) + unlock() + close(upgradeDone) + }() + + require.Eventually(t, func() bool { return inUpgrade.Load() }, 2*time.Second, 5*time.Millisecond) + + uninstallDone := make(chan error, 1) + go func() { + err := svc.Uninstall(t.Context(), skills.UninstallOptions{ + Name: "my-skill", Scope: skills.ScopeProject, ProjectRoot: projectRoot, + }) + if inUpgrade.Load() { + overlapped.Store(true) + } + uninstallDone <- err + }() + + <-upgradeDone + select { + case err := <-uninstallDone: + require.NoError(t, err) + case <-time.After(10 * time.Second): + t.Fatal("uninstall timed out") + } + assert.False(t, overlapped.Load(), "uninstall must not run while upgrade holds the project tx") +} + +// TestInstallProjectScope_AliasCycleRejected covers a cycle where requires +// edges use distinct git aliases that resolve to the same canonical names. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestInstallProjectScope_AliasCycleRejected(t *testing.T) { + ctrl := gomock.NewController(t) + gr := gitmocks.NewMockResolver(ctrl) + fx := &fakeGitFixtures{ + skills: make(map[string]gitFixture), + history: make(map[string]map[string]gitFixture), + } + + aliasA := "git://github.com/test/alias-a" + aliasB := "git://github.com/test/alias-b" + _, urlA := gitRef("skill-a") + _, urlB := gitRef("skill-b") + + // alias-a resolves to skill-a which requires alias-b; alias-b resolves + // to skill-b which requires alias-a — a canonical a↔b cycle via aliases. + fx.register("skill-a", gitSkill("skill-a", aliasB)) + fx.register("skill-b", gitSkill("skill-b", aliasA)) + // Also register under the alias URLs ParseGitReference will produce. + fx.skills[mustGitURL(t, aliasA)] = fx.skills[urlA] + fx.skills[mustGitURL(t, aliasB)] = fx.skills[urlB] + fx.history[mustGitURL(t, aliasA)] = fx.history[urlA] + fx.history[mustGitURL(t, aliasB)] = fx.history[urlB] + + 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 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: fixtureCommitHash(ref.URL, fixture.content), + }, nil + }) + + svc, projectRoot := newLockTestService(t, gr) + + done := make(chan error, 1) + go func() { + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: aliasA, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + done <- err + }() + select { + case err := <-done: + require.Error(t, err) + assert.Contains(t, err.Error(), "dependency cycle") + case <-time.After(10 * time.Second): + t.Fatal("timeout: alias cycle did not reject") + } +} + +func mustGitURL(t *testing.T, gitRefStr string) string { + t.Helper() + ref, err := gitresolver.ParseGitReference(gitRefStr) + require.NoError(t, err) + return ref.URL +} + +// TestInstallProjectScope_DiamondDependencyMergesRequiredBy installs a root +// whose two children share one dependency within a single traversal. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestInstallProjectScope_DiamondDependencyMergesRequiredBy(t *testing.T) { + gr, fx := newGitResolverMock(t) + depRef, _ := gitRef("shared-dep") + leftRef, _ := gitRef("left") + rightRef, _ := gitRef("right") + fx.register("shared-dep", gitSkill("shared-dep")) + fx.register("left", gitSkill("left", depRef)) + fx.register("right", gitSkill("right", depRef)) + fx.register("root", gitSkill("root", leftRef, rightRef)) + svc, projectRoot := newLockTestService(t, gr) + + rootRef, _ := gitRef("root") + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: rootRef, 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{"left", "right"}, dep.RequiredBy, + "diamond dependency must keep both parents from a single install traversal") +} + +// TestSync_CanonicalNameMismatchRejected fails sync when the pinned artifact +// resolves to a different skill name than the lock entry. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestSync_CanonicalNameMismatchRejected(t *testing.T) { + gr, fx := newGitResolverMock(t) + fx.register("lock-name", gitSkill("lock-name")) + svc, projectRoot := newLockTestService(t, gr) + + ref, _ := gitRef("lock-name") + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + + entry, ok := readLockfile(t, projectRoot).Get("lock-name") + require.True(t, ok) + + // Republish the same source URL with a different manifest name. + _, url := gitRef("lock-name") + fx.skills[url] = gitFixture{name: "other-name", content: gitSkill("other-name")} + fx.history[url][entry.Digest] = gitFixture{name: "other-name", content: gitSkill("other-name")} + + // Force drift so sync attempts a restore. + skillMD := filepath.Join(projectRoot, ".claude", "skills", "lock-name", "SKILL.md") + require.NoError(t, os.WriteFile(skillMD, []byte("tampered"), 0o644)) + + syncer := svc.(*service) //nolint:forcetypeassert + result, err := syncer.Sync(t.Context(), skills.SyncOptions{ProjectRoot: projectRoot}) + require.NoError(t, err) + require.NotEmpty(t, result.Failed) + assert.Contains(t, result.Failed[0].Error, "expected canonical name") + + // Lock entry and DB identity must be unchanged. + after, ok := readLockfile(t, projectRoot).Get("lock-name") + require.True(t, ok) + assert.Equal(t, entry.Digest, after.Digest) + info, err := svc.Info(t.Context(), skills.InfoOptions{ + Name: "lock-name", Scope: skills.ScopeProject, ProjectRoot: projectRoot, + }) + require.NoError(t, err) + assert.Equal(t, "lock-name", info.InstalledSkill.Metadata.Name) +} diff --git a/pkg/skills/skillsvc/service.go b/pkg/skills/skillsvc/service.go index 74cbc72a58..b1b26f8209 100644 --- a/pkg/skills/skillsvc/service.go +++ b/pkg/skills/skillsvc/service.go @@ -7,9 +7,13 @@ package skillsvc //go:generate mockgen -destination=mocks/mock_signer.go -package=mocks github.com/stacklok/toolhive-core/container/signer Signer import ( + "fmt" + "hash/fnv" + "net/http" "sync" "github.com/stacklok/toolhive-core/container/signer" + "github.com/stacklok/toolhive-core/httperr" ociskills "github.com/stacklok/toolhive-core/oci/skills" regtypes "github.com/stacklok/toolhive-core/registry/types" "github.com/stacklok/toolhive/pkg/groups" @@ -85,6 +89,9 @@ func WithGitResolver(gr gitresolver.Resolver) Option { } // skillLock provides per-skill mutual exclusion keyed by scope/name/projectRoot. +// Used for user-scope operations. Project-scope mutations serialize on +// projectTx instead, to avoid nested per-skill ABBA deadlocks. +// // Entries are never evicted. This is acceptable because the number of distinct // skills on a single machine is expected to remain small (< 1000). type skillLock struct { @@ -110,9 +117,87 @@ func (sl *skillLock) lock(name string, scope skills.Scope, projectRoot string) f return m.Unlock } +// projectTxStripes bounds the project transaction lock set. Project roots +// are request-derived in the long-running API service, so a grow-forever +// map keyed by root would leak; a fixed stripe set caps memory at a +// constant. Two projects hashing to the same stripe merely serialize +// against each other — never a correctness issue. +const projectTxStripes = 64 + +// projectTx serializes all project-scoped skill mutations for a given +// canonical ProjectRoot. Different projects remain concurrent (up to stripe +// collisions); Install, Uninstall, Sync, and Upgrade for the same project +// share one transaction that spans extraction through bookkeeping, +// dependency materialization, cascades, and compensation. +type projectTx struct { + stripes [projectTxStripes]sync.Mutex +} + +// lock acquires the project transaction mutex for projectRoot's stripe and +// returns a release function. +func (p *projectTx) lock(projectRoot string) func() { + h := fnv.New32a() + _, _ = h.Write([]byte(projectRoot)) + m := &p.stripes[h.Sum32()%projectTxStripes] + m.Lock() + return m.Unlock +} + +// depState tracks dependency traversal under a held project transaction. +// Cycle detection and diamond merging key on canonical skill names after +// source resolution — not on alias/reference strings — so alias cycles are +// rejected deterministically and shared deps still merge RequiredBy. +type depState struct { + active map[string]struct{} // currently installing (call stack) + completed map[string]struct{} // finished in this traversal +} + +func newDepState() *depState { + return &depState{ + active: make(map[string]struct{}), + completed: make(map[string]struct{}), + } +} + +// enter marks canonical as actively installing. Returns an error when the +// name is already on the active stack (an alias/reference cycle). +func (d *depState) enter(canonical string) error { + if d == nil { + return nil + } + if _, inFlight := d.active[canonical]; inFlight { + return httperr.WithCode( + fmt.Errorf("dependency cycle detected involving skill %q", canonical), + http.StatusUnprocessableEntity, + ) + } + d.active[canonical] = struct{}{} + return nil +} + +// leave removes canonical from the active stack and records it completed. +func (d *depState) leave(canonical string) { + if d == nil { + return + } + delete(d.active, canonical) + d.completed[canonical] = struct{}{} +} + +// alreadyDone reports whether canonical was fully installed earlier in this +// traversal (diamond / shared dependency). +func (d *depState) alreadyDone(canonical string) bool { + if d == nil { + return false + } + _, ok := d.completed[canonical] + return ok +} + // service is the default implementation of skills.SkillService. type service struct { locks skillLock + projectTx projectTx store storage.SkillStore groupManager groups.Manager pathResolver skills.PathResolver diff --git a/pkg/skills/skillsvc/sync.go b/pkg/skills/skillsvc/sync.go index e3f6c1c9fa..e7e1c947b9 100644 --- a/pkg/skills/skillsvc/sync.go +++ b/pkg/skills/skillsvc/sync.go @@ -32,6 +32,9 @@ func (s *service) Sync(ctx context.Context, opts skills.SyncOptions) (*skills.Sy } opts.ProjectRoot = projectRoot + unlock := s.projectTx.lock(projectRoot) + defer unlock() + root, err := lockfile.OpenRoot(projectRoot) if err != nil { return nil, err @@ -45,26 +48,70 @@ func (s *service) Sync(ctx context.Context, opts skills.SyncOptions) (*skills.Sy if err != nil { return nil, fmt.Errorf("listing installed skills: %w", err) } - installedByName := make(map[string]skills.InstalledSkill, len(installed)) - for _, sk := range installed { - installedByName[sk.Metadata.Name] = sk - } - result := &skills.SyncResult{} + names := make([]string, 0, len(lf.Skills)+len(installed)) + seen := make(map[string]struct{}, len(lf.Skills)+len(installed)) for _, entry := range lf.Skills { - sk, dbOK := installedByName[entry.Name] - s.syncLockedEntry(ctx, opts, entry, sk, dbOK, result) + names = append(names, entry.Name) + seen[entry.Name] = struct{}{} } for _, sk := range installed { - if _, ok := lf.Get(sk.Metadata.Name); ok { - continue // handled by the loop above + if _, ok := seen[sk.Metadata.Name]; ok { + continue } - s.syncUnlockedInstall(ctx, opts, sk, result) + names = append(names, sk.Metadata.Name) + } + + result := &skills.SyncResult{} + for _, name := range names { + s.syncOne(ctx, opts, name, result) } return result, nil } +// syncOne re-reads the lock entry and DB row under the held project +// transaction, then reconciles that fresh state. The initial Sync snapshot +// is only used to discover names; mutation from a stale view would +// resurrect an uninstall or prune a concurrent install. +func (s *service) syncOne( + ctx context.Context, opts skills.SyncOptions, name string, result *skills.SyncResult, +) { + root, err := lockfile.OpenRoot(opts.ProjectRoot) + if err != nil { + result.Failed = append(result.Failed, skills.SyncFailure{ + Name: name, Reason: classifySyncFailure(err), Error: err.Error(), + }) + return + } + lf, err := lockfile.Load(root) + if err != nil { + result.Failed = append(result.Failed, skills.SyncFailure{ + Name: name, Reason: classifySyncFailure(err), Error: err.Error(), + }) + return + } + entry, hasEntry := lf.Get(name) + + sk, err := s.store.Get(ctx, name, skills.ScopeProject, opts.ProjectRoot) + dbOK := err == nil + if err != nil && !errors.Is(err, storage.ErrNotFound) { + result.Failed = append(result.Failed, skills.SyncFailure{ + Name: name, Reason: classifySyncFailure(err), Error: err.Error(), + }) + return + } + + if hasEntry { + s.syncLockedEntry(ctx, opts, entry, sk, dbOK, result) + return + } + if !dbOK { + return + } + s.syncUnlockedInstall(ctx, opts, sk, result) +} + // syncLockedEntry reconciles one lock file entry against installed state, // appending its outcome to result. Missing (dbOK false) and drifted (digest // or contentDigest mismatch) entries are reinstalled at the pinned reference @@ -93,7 +140,7 @@ func (s *service) syncLockedEntry( "skill", entry.Name, "error", sigErr) } } - if dbOK && sigOK && entryMatchesInstalled(s.pathResolver, entry, sk) { + if dbOK && sigOK && entryMatchesInstalled(s.pathResolver, entry, sk, opts.Clients) { result.AlreadyCurrent = append(result.AlreadyCurrent, entry.Name) return } @@ -105,7 +152,7 @@ func (s *service) syncLockedEntry( if opts.Check { return } - if err := s.reinstallPinned(ctx, opts, entry, sk, dbOK); err != nil { + if err := s.reinstallPinned(ctx, opts, entry); err != nil { result.Failed = append(result.Failed, skills.SyncFailure{ Name: entry.Name, Reason: classifySyncFailure(err), Error: err.Error(), }) @@ -115,18 +162,27 @@ func (s *service) syncLockedEntry( } // entryMatchesInstalled reports whether the installed skill's pinned digest -// still matches the lock entry and EVERY client directory's on-disk -// contentDigest does too. Checking only one client's copy would leave -// tampering with any other client's materialized files invisible to -// --check — and which directory got checked would depend on install order. -func entryMatchesInstalled(pathResolver skills.PathResolver, entry lockfile.Entry, sk skills.InstalledSkill) bool { +// still matches the lock entry, every expected client is present, and every +// checked client directory's on-disk contentDigest matches. With no +// --clients override, expected clients are every skill-supporting client +// detected on the host so a newly installed client is not treated as current. +func entryMatchesInstalled( + pathResolver skills.PathResolver, entry lockfile.Entry, sk skills.InstalledSkill, requestedClients []string, +) bool { if sk.Digest != entry.Digest { return false } if len(sk.Clients) == 0 { return false } - for _, client := range sk.Clients { + expected := requestedClients + if len(expected) == 0 && pathResolver != nil { + expected = pathResolver.ListSkillSupportingClients() + } + if len(expected) == 0 || !clientsContainAll(sk.Clients, expected) { + return false + } + for _, client := range mergeClientLists(sk.Clients, expected) { dir, err := pathResolver.GetSkillPath(client, sk.Metadata.Name, sk.Scope, sk.ProjectRoot) if err != nil { return false @@ -140,29 +196,27 @@ func entryMatchesInstalled(pathResolver skills.PathResolver, entry lockfile.Entr } // reinstallPinned reinstalls entry at its pinned reference, preserving its -// recorded Source (never re-resolving) and the clients it was previously -// installed for unless the caller overrides them. +// recorded Source (never re-resolving). Empty opts.Clients keeps Install's +// all-detected default so a newly detected client is materialized. +// Assumes the project transaction is already held. func (s *service) reinstallPinned( - ctx context.Context, opts skills.SyncOptions, entry lockfile.Entry, existing skills.InstalledSkill, dbOK bool, + ctx context.Context, opts skills.SyncOptions, entry lockfile.Entry, ) error { pinnedRef, err := buildPinnedReference(entry) if err != nil { return fmt.Errorf("pinning %q: %w", entry.Name, err) } - clients := opts.Clients - if len(clients) == 0 && dbOK { - clients = existing.Clients - } - _, err = s.Install(ctx, skills.InstallOptions{ + _, err = s.installLocked(ctx, skills.InstallOptions{ Name: pinnedRef, Scope: skills.ScopeProject, ProjectRoot: opts.ProjectRoot, - Clients: clients, + Clients: opts.Clients, Force: true, // sync restores exactly the pinned content over any drifted files LockSource: entry.Source, LockResolvedReference: entry.ResolvedReference, // preserve — pinnedRef is a restore form SyncRestore: true, // reinstall despite unchanged Digest — drift is on disk, not the pin - }) + ExpectedCanonicalName: entry.Name, + }, pinnedRef, skills.ScopeProject, newDepState()) return err } @@ -186,9 +240,9 @@ func (s *service) syncUnlockedInstall( result.RemovedFromLock = append(result.RemovedFromLock, sk.Metadata.Name) if opts.Prune && !opts.Check { - if err := s.Uninstall(ctx, skills.UninstallOptions{ + if err := s.uninstallLocked(ctx, skills.UninstallOptions{ Name: sk.Metadata.Name, Scope: skills.ScopeProject, ProjectRoot: opts.ProjectRoot, - }); err != nil { + }, skills.ScopeProject); err != nil { result.Failed = append(result.Failed, skills.SyncFailure{ Name: sk.Metadata.Name, Reason: classifySyncFailure(err), Error: err.Error(), }) @@ -228,6 +282,10 @@ func (s *service) verifyStoredSignature(entry lockfile.Entry, sk skills.Installe // Trust state is back-filled from the stored Sigstore bundle when one // exists; otherwise adoption is the same trust decision as an unsigned // install and requires the explicit AllowUnsigned exception. +// +// Assumes the project transaction is already held. If marking the skill +// managed fails after the lock entry is written, the pre-adopt lock entry +// is restored (or the newly created entry is removed). func (s *service) adoptSkill(ctx context.Context, opts skills.SyncOptions, sk skills.InstalledSkill) error { contentDigest, err := computeContentDigest(s.pathResolver, sk) if err != nil { @@ -252,6 +310,17 @@ func (s *service) adoptSkill(ctx context.Context, opts skills.SyncOptions, sk sk } unsigned = true } + + var prevEntry *lockfile.Entry + if root, rootErr := lockfile.OpenRoot(sk.ProjectRoot); rootErr == nil { + if lf, loadErr := lockfile.Load(root); loadErr == nil { + if e, ok := lf.Get(sk.Metadata.Name); ok { + prev := e + prevEntry = &prev + } + } + } + if err := recordLockEntry(sk.ProjectRoot, lockEntryInput{ Name: sk.Metadata.Name, Version: sk.Metadata.Version, @@ -266,11 +335,33 @@ func (s *service) adoptSkill(ctx context.Context, opts skills.SyncOptions, sk sk } sk.Managed = true if err := s.store.Update(ctx, sk); err != nil { + if restoreErr := restoreAdoptedLockEntry(sk.ProjectRoot, sk.Metadata.Name, prevEntry); restoreErr != nil { + return errors.Join( + fmt.Errorf("marking skill as lock-managed: %w", err), + fmt.Errorf("restoring lock entry after failed adopt: %w", restoreErr), + ) + } return fmt.Errorf("marking skill as lock-managed: %w", err) } return nil } +// restoreAdoptedLockEntry undoes adoptSkill's lock write: reinstates the +// entry observed before adoption, or removes the name if none existed. +func restoreAdoptedLockEntry(projectRoot, name string, prevEntry *lockfile.Entry) error { + if prevEntry != nil { + root, err := lockfile.OpenRoot(projectRoot) + if err != nil { + return err + } + return lockfile.UpsertEntry(root, *prevEntry) + } + _, err := removeLockEntry(skills.UninstallOptions{ + Name: name, Scope: skills.ScopeProject, ProjectRoot: projectRoot, + }) + return err +} + // classifySyncFailure maps an error from the install/uninstall path to an // RFC THV-0080 typed failure reason using structured signals those paths // already attach — the errLockWrite sentinel and httperr status codes — diff --git a/pkg/skills/skillsvc/sync_test.go b/pkg/skills/skillsvc/sync_test.go index 0609c9cf19..cfd5bdd97b 100644 --- a/pkg/skills/skillsvc/sync_test.go +++ b/pkg/skills/skillsvc/sync_test.go @@ -4,6 +4,8 @@ package skillsvc import ( + "context" + "errors" "os" "path/filepath" "testing" @@ -16,6 +18,7 @@ import ( "github.com/stacklok/toolhive/pkg/skills/lockfile" skillsmocks "github.com/stacklok/toolhive/pkg/skills/mocks" verifiermocks "github.com/stacklok/toolhive/pkg/skills/verifier/mocks" + "github.com/stacklok/toolhive/pkg/storage" "github.com/stacklok/toolhive/pkg/storage/sqlite" ) @@ -27,7 +30,7 @@ func TestSync_ReportsUpToDateWhenNothingChanged(t *testing.T) { ref, _ := gitRef("my-skill") _, err := svc.Install(t.Context(), skills.InstallOptions{ - Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, }) require.NoError(t, err) @@ -329,6 +332,95 @@ func TestSync_AdoptsUnmanagedInstall(t *testing.T) { assert.True(t, sk.InstalledSkill.Managed, "adopt must mark the DB record as managed") } +// A failed adopt (marking the DB record managed fails after the lock write) +// must remove the entry this adoption created — the lock must not claim a +// skill is managed while the DB says otherwise. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestSync_AdoptCompensatesFailedManagedUpdate(t *testing.T) { + gr, fx := newGitResolverMock(t) + fx.register("unmanaged-skill", gitSkill("unmanaged-skill")) + svc, projectRoot := newLockTestService(t, gr) + + // Simulate a legacy unmanaged install. + ref, _ := gitRef("unmanaged-skill") + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, AllowUnsigned: true, + }) + require.NoError(t, err) + root := mustOpenRoot(t, projectRoot) + require.NoError(t, lockfile.Update(root, func(lf *lockfile.Lockfile) error { + lf.Remove("unmanaged-skill") + return nil + })) + inner := svc.(*service) //nolint:forcetypeassert + legacySk, err := inner.store.Get(t.Context(), "unmanaged-skill", skills.ScopeProject, projectRoot) + require.NoError(t, err) + legacySk.Managed = false + require.NoError(t, inner.store.Update(t.Context(), legacySk)) + + // Marking the record managed during adopt now fails. + inner.store = &hookSkillStore{ + SkillStore: inner.store, + updateErr: errors.New("db update unavailable"), + } + + result, err := inner.Sync(t.Context(), skills.SyncOptions{ + ProjectRoot: projectRoot, Adopt: true, AllowUnsigned: true, + }) + require.NoError(t, err) + require.Len(t, result.Failed, 1) + assert.Contains(t, result.Failed[0].Error, "db update unavailable") + + _, ok := readLockfile(t, projectRoot).Get("unmanaged-skill") + assert.False(t, ok, "a failed adopt must remove the lock entry it created") +} + +// Sync must fail up front on an invalid project root or an unreadable lock +// file rather than reporting per-name results from broken state. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestSync_InvalidInputsFail(t *testing.T) { + gr, _ := newGitResolverMock(t) + svc, projectRoot := newLockTestService(t, gr) + syncer := svc.(*service) //nolint:forcetypeassert + + _, err := syncer.Sync(t.Context(), skills.SyncOptions{ProjectRoot: filepath.Join(projectRoot, "does-not-exist")}) + require.Error(t, err, "a nonexistent project root must fail") + + lockPath := filepath.Join(projectRoot, lockfile.FileName) + require.NoError(t, os.MkdirAll(lockPath, 0o755)) + _, err = syncer.Sync(t.Context(), skills.SyncOptions{ProjectRoot: projectRoot}) + require.Error(t, err, "an unreadable lock file must fail") +} + +// syncOne must report a per-name failure when the lock file becomes +// unreadable under the held transaction, not panic or mutate state. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestSyncOne_UnreadableLockReportsFailure(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) + + lockPath := filepath.Join(projectRoot, lockfile.FileName) + require.NoError(t, os.Remove(lockPath)) + require.NoError(t, os.MkdirAll(lockPath, 0o755)) + + inner := svc.(*service) //nolint:forcetypeassert + result := &skills.SyncResult{} + inner.syncOne(t.Context(), skills.SyncOptions{ProjectRoot: projectRoot}, "my-skill", result) + require.Len(t, result.Failed, 1) + assert.Equal(t, "my-skill", result.Failed[0].Name) +} + //nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel func TestSync_PrunesRemovedFromLock(t *testing.T) { gr, fx := newGitResolverMock(t) @@ -413,3 +505,77 @@ func TestSync_CheckDetectsTamperInAnyClientDir(t *testing.T) { assert.Equal(t, []string{"multi-skill"}, result.Drifted, "tampering with a non-first client's copy must be reported as drift") } + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestSync_DefaultExpandsToNewlyDetectedClients(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) + + syncer := svc.(*service) //nolint:forcetypeassert + result, err := syncer.Sync(t.Context(), skills.SyncOptions{ProjectRoot: projectRoot, Check: true}) + require.NoError(t, err) + assert.Equal(t, []string{"my-skill"}, result.Drifted) + assert.Empty(t, result.AlreadyCurrent, "default sync must not treat a missing detected client as current") + + result, err = syncer.Sync(t.Context(), skills.SyncOptions{ProjectRoot: projectRoot}) + require.NoError(t, err) + assert.Equal(t, []string{"my-skill"}, result.Installed) + + info, err := svc.Info(t.Context(), skills.InfoOptions{Name: "my-skill", Scope: skills.ScopeProject, ProjectRoot: projectRoot}) + require.NoError(t, err) + assert.ElementsMatch(t, []string{"claude-code", "cursor"}, info.InstalledSkill.Clients) +} + +type staleListStore struct { + storage.SkillStore + listed []skills.InstalledSkill +} + +func (s *staleListStore) List(context.Context, storage.ListFilter) ([]skills.InstalledSkill, error) { + out := make([]skills.InstalledSkill, len(s.listed)) + copy(out, s.listed) + return out, nil +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestSync_StaleListDoesNotResurrectUninstalledSkill(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) + + inner := svc.(*service) //nolint:forcetypeassert + stale, err := inner.store.List(t.Context(), storage.ListFilter{ + Scope: skills.ScopeProject, ProjectRoot: projectRoot, + }) + require.NoError(t, err) + require.NotEmpty(t, stale) + + require.NoError(t, svc.Uninstall(t.Context(), skills.UninstallOptions{ + Name: "my-skill", Scope: skills.ScopeProject, ProjectRoot: projectRoot, + })) + + inner.store = &staleListStore{SkillStore: inner.store, listed: stale} + + result, err := inner.Sync(t.Context(), skills.SyncOptions{ProjectRoot: projectRoot}) + require.NoError(t, err) + assert.Empty(t, result.Installed) + assert.Empty(t, result.NeverManaged) + + _, err = svc.Info(t.Context(), skills.InfoOptions{ + Name: "my-skill", Scope: skills.ScopeProject, ProjectRoot: projectRoot, + }) + require.Error(t, err, "a stale List snapshot must not resurrect an uninstalled skill") +} diff --git a/pkg/skills/skillsvc/uninstall.go b/pkg/skills/skillsvc/uninstall.go index 893cfdc092..93657b18a8 100644 --- a/pkg/skills/skillsvc/uninstall.go +++ b/pkg/skills/skillsvc/uninstall.go @@ -33,6 +33,20 @@ func (s *service) Uninstall(ctx context.Context, opts skills.UninstallOptions) e scope = defaultScope(scope) opts.ProjectRoot = projectRoot + if scope == skills.ScopeProject { + unlock := s.projectTx.lock(opts.ProjectRoot) + defer unlock() + return s.uninstallLocked(ctx, opts, scope) + } + unlock := s.locks.lock(opts.Name, scope, opts.ProjectRoot) + defer unlock() + return s.uninstallLocked(ctx, opts, scope) +} + +// uninstallLocked performs Uninstall assuming the appropriate lock is already +// held (project transaction or per-skill). Sync prune and cascade removals +// call this directly so they never reacquire. +func (s *service) uninstallLocked(ctx context.Context, opts skills.UninstallOptions, scope skills.Scope) error { return s.uninstallOne(ctx, opts, scope) } @@ -47,10 +61,11 @@ func (s *service) Uninstall(ctx context.Context, opts skills.UninstallOptions) e // 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. +// +// Callers must already hold the project transaction (project scope) or the +// per-skill lock (user scope). Cascades reuse that held lock — they never +// reacquire. 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() - // Look up the existing record to find which clients have files. existing, err := s.store.Get(ctx, opts.Name, scope, opts.ProjectRoot) if err != nil { @@ -150,7 +165,8 @@ func removeLockEntry(opts skills.UninstallOptions) ([]string, error) { // 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. +// requiredBy cycle in a hand-edited lock. Assumes the project transaction +// (or user-scope skill lock) is already held. func (s *service) cascadeUninstall( ctx context.Context, candidates []string, visited map[string]struct{}, projectRoot string, scope skills.Scope, ) error { diff --git a/pkg/skills/skillsvc/uninstall_cascade_test.go b/pkg/skills/skillsvc/uninstall_cascade_test.go index 3bff0de3be..c9730ccbde 100644 --- a/pkg/skills/skillsvc/uninstall_cascade_test.go +++ b/pkg/skills/skillsvc/uninstall_cascade_test.go @@ -117,35 +117,35 @@ func TestUninstall_ExplicitDependencyIsNeverCascaded(t *testing.T) { } //nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel -func TestUninstall_CascadeCycleTerminates(t *testing.T) { +func TestUninstall_CascadeUnderProjectTxTerminates(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)) + 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: refA, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + Name: parentRef, 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, + Name: "parent-skill", 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") + t.Fatal("timeout: cascade uninstall under project tx did not terminate") } lf := readLockfile(t, projectRoot) - _, ok := lf.Get("skill-a") + _, ok := lf.Get("parent-skill") assert.False(t, ok) - _, ok = lf.Get("skill-b") - assert.False(t, ok, "skill-b lost its only (cyclic) parent and must cascade too") + _, ok = lf.Get("shared-dep") + assert.False(t, ok, "orphaned dependency must cascade under the held project tx") } diff --git a/pkg/skills/skillsvc/upgrade.go b/pkg/skills/skillsvc/upgrade.go index 8fb8ad4cd1..c1246f8527 100644 --- a/pkg/skills/skillsvc/upgrade.go +++ b/pkg/skills/skillsvc/upgrade.go @@ -36,6 +36,9 @@ func (s *service) Upgrade(ctx context.Context, opts skills.UpgradeOptions) (*ski } opts.ProjectRoot = projectRoot + unlock := s.projectTx.lock(projectRoot) + defer unlock() + root, err := lockfile.OpenRoot(projectRoot) if err != nil { return nil, err @@ -50,25 +53,9 @@ func (s *service) Upgrade(ctx context.Context, opts skills.UpgradeOptions) (*ski return nil, err } - // Resolve every target's latest state first, without installing - // anything. FailOnChanges is a CI freshness gate: it reports the full - // planned outcome set and never runs the apply pass at all — returning - // the outcomes (rather than an error that discards them) lets callers - // see exactly which skills are stale and distinguish "would change" - // from a genuine resolution failure. Exit-code mapping happens in the - // CLI from these outcomes, mirroring how sync --check works. - plans := make([]upgradePlan, len(targets)) - for i, entry := range targets { - plans[i] = s.planUpgrade(ctx, opts, entry) - } - - result := &skills.UpgradeResult{Outcomes: make([]skills.UpgradeOutcome, 0, len(plans))} - for _, p := range plans { - if opts.FailOnChanges { - result.Outcomes = append(result.Outcomes, p.outcome) - continue - } - result.Outcomes = append(result.Outcomes, s.applyUpgrade(ctx, opts, p)) + result := &skills.UpgradeResult{Outcomes: make([]skills.UpgradeOutcome, 0, len(targets))} + for _, target := range targets { + result.Outcomes = append(result.Outcomes, s.upgradeOne(ctx, opts, target.Name)) } return result, nil } @@ -96,6 +83,47 @@ func selectUpgradeTargets(lf *lockfile.Lockfile, names []string) ([]lockfile.Ent return targets, nil } +// upgradeOne reloads the named lock entry under the held project +// transaction, then plans and applies against that fresh snapshot. Planning +// every entry first and applying later would let a concurrent uninstall be +// resurrected, or a newer install be overwritten by this older plan. +// +// FailOnChanges is a CI freshness gate: it reports the planned outcome and +// never applies. Exit-code mapping happens in the CLI from these outcomes. +func (s *service) upgradeOne( + ctx context.Context, opts skills.UpgradeOptions, name string, +) skills.UpgradeOutcome { + root, err := lockfile.OpenRoot(opts.ProjectRoot) + if err != nil { + return skills.UpgradeOutcome{ + Name: name, Status: skills.UpgradeStatusFailed, + Reason: classifySyncFailure(err), Error: err.Error(), + } + } + lf, err := lockfile.Load(root) + if err != nil { + return skills.UpgradeOutcome{ + Name: name, Status: skills.UpgradeStatusFailed, + Reason: classifySyncFailure(err), Error: err.Error(), + } + } + entry, ok := lf.Get(name) + if !ok { + return skills.UpgradeOutcome{ + Name: name, + Status: skills.UpgradeStatusFailed, + Reason: skills.FailureReasonUnknown, + Error: fmt.Sprintf("skill %q is no longer in the lock file", name), + } + } + + plan := s.planUpgrade(ctx, opts, entry) + if opts.FailOnChanges { + return plan.outcome + } + return s.applyUpgrade(ctx, opts, plan) +} + // upgradePlan is entry's resolved outcome before any install happens: either // a terminal status (not-upgradable, up-to-date, ref-change-blocked, or a // resolution failure) that needs no further action, or the pinned reference @@ -263,6 +291,7 @@ func (s *service) probeCandidateSigner(ctx context.Context, newRef, newDigest st // applyUpgrade installs plan's pinned content when the plan calls for it. // Preview mode reports the plan's outcome without installing anything. +// Assumes the project transaction is already held. func (s *service) applyUpgrade(ctx context.Context, opts skills.UpgradeOptions, plan upgradePlan) skills.UpgradeOutcome { if plan.pinnedRef == "" || opts.Preview { return plan.outcome @@ -275,7 +304,7 @@ func (s *service) applyUpgrade(ctx context.Context, opts skills.UpgradeOptions, } } - if _, err := s.Install(ctx, skills.InstallOptions{ + if _, err := s.installLocked(ctx, skills.InstallOptions{ Name: plan.pinnedRef, Scope: skills.ScopeProject, ProjectRoot: opts.ProjectRoot, @@ -283,7 +312,8 @@ func (s *service) applyUpgrade(ctx context.Context, opts skills.UpgradeOptions, LockSource: plan.entry.Source, LockResolvedReference: plan.resolvedRef, AllowSignerChange: opts.AllowSignerChange, - }); err != nil { + ExpectedCanonicalName: plan.entry.Name, + }, plan.pinnedRef, skills.ScopeProject, newDepState()); err != nil { outcome := plan.outcome outcome.Status = skills.UpgradeStatusFailed outcome.Reason = classifySyncFailure(err) diff --git a/pkg/skills/skillsvc/upgrade_test.go b/pkg/skills/skillsvc/upgrade_test.go index 71f8aef13c..c1ec842465 100644 --- a/pkg/skills/skillsvc/upgrade_test.go +++ b/pkg/skills/skillsvc/upgrade_test.go @@ -5,6 +5,8 @@ package skillsvc import ( "net/http" + "os" + "path/filepath" "testing" "github.com/stretchr/testify/assert" @@ -12,8 +14,53 @@ import ( "github.com/stacklok/toolhive-core/httperr" "github.com/stacklok/toolhive/pkg/skills" + "github.com/stacklok/toolhive/pkg/skills/lockfile" ) +// Upgrade must fail up front on an invalid project root or an unreadable +// lock file. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestUpgrade_InvalidInputsFail(t *testing.T) { + gr, _ := newGitResolverMock(t) + svc, projectRoot := newLockTestService(t, gr) + inner := svc.(*service) //nolint:forcetypeassert + + _, err := inner.Upgrade(t.Context(), skills.UpgradeOptions{ProjectRoot: filepath.Join(projectRoot, "does-not-exist")}) + require.Error(t, err, "a nonexistent project root must fail") + + lockPath := filepath.Join(projectRoot, lockfile.FileName) + require.NoError(t, os.MkdirAll(lockPath, 0o755)) + _, err = inner.Upgrade(t.Context(), skills.UpgradeOptions{ProjectRoot: projectRoot}) + require.Error(t, err, "an unreadable lock file must fail") +} + +// upgradeOne must report a failed outcome when the lock file becomes +// unreadable under the held transaction. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestUpgradeOne_UnreadableLockReportsFailure(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) + + lockPath := filepath.Join(projectRoot, lockfile.FileName) + require.NoError(t, os.Remove(lockPath)) + require.NoError(t, os.MkdirAll(lockPath, 0o755)) + + inner := svc.(*service) //nolint:forcetypeassert + outcome := inner.upgradeOne(t.Context(), skills.UpgradeOptions{ProjectRoot: projectRoot}, "my-skill") + assert.Equal(t, skills.UpgradeStatusFailed, outcome.Status) + assert.Equal(t, "my-skill", outcome.Name) + assert.NotEmpty(t, outcome.Error) +} + //nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel func TestUpgrade_ReportsUpToDateWhenSourceUnchanged(t *testing.T) { gr, fx := newGitResolverMock(t) @@ -99,9 +146,9 @@ func TestUpgrade_PreviewDoesNotInstall(t *testing.T) { // expanding a skill to every detected client. Install without an explicit // InstallOptions.Clients falls back to whatever the caller passed (here, // a single client); Upgrade must default to that skill's already-installed -// Clients when opts.Clients is empty, matching Sync's reinstallPinned — not -// fall through to Install's own "no clients means every detected client" -// default. +// Clients when opts.Clients is empty — not fall through to Install's +// "no clients means every detected client" default. Sync is the opposite: +// a default sync expands to newly detected clients. // //nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel func TestUpgrade_PreservesExistingClients(t *testing.T) { @@ -244,3 +291,26 @@ func TestUpgrade_FailOnChangesWithoutPreviewDoesNotMutateAnyEntry(t *testing.T) require.True(t, ok) assert.Equal(t, stableBefore.Digest, stableAfter.Digest) } + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestUpgrade_DoesNotResurrectRemovedLockEntry(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) + + require.NoError(t, lockfile.RemoveEntry(mustOpenRoot(t, projectRoot), "my-skill")) + + inner := svc.(*service) //nolint:forcetypeassert + outcome := inner.upgradeOne(t.Context(), skills.UpgradeOptions{ProjectRoot: projectRoot}, "my-skill") + assert.Equal(t, skills.UpgradeStatusFailed, outcome.Status) + assert.Contains(t, outcome.Error, "no longer in the lock file") + + _, ok := readLockfile(t, projectRoot).Get("my-skill") + assert.False(t, ok, "upgrade must not rewrite a lock entry that was removed") +}