diff --git a/docs/server/docs.go b/docs/server/docs.go index 84183297ac..24270dbbea 100644 --- a/docs/server/docs.go +++ b/docs/server/docs.go @@ -1691,6 +1691,10 @@ const docTemplate = `{ "description": "InstalledAt is the timestamp when the skill was installed.", "type": "string" }, + "managed": { + "description": "Managed indicates this install is tracked in the project's\ntoolhive.lock.yaml. Only ever true for project-scoped installs. No\nomitempty: false is an observable state (unmanaged), not an absence.", + "type": "boolean" + }, "metadata": { "$ref": "#/components/schemas/github_com_stacklok_toolhive_pkg_skills.SkillMetadata" }, diff --git a/docs/server/swagger.json b/docs/server/swagger.json index 0e1ee3e319..322fbb8507 100644 --- a/docs/server/swagger.json +++ b/docs/server/swagger.json @@ -1684,6 +1684,10 @@ "description": "InstalledAt is the timestamp when the skill was installed.", "type": "string" }, + "managed": { + "description": "Managed indicates this install is tracked in the project's\ntoolhive.lock.yaml. Only ever true for project-scoped installs. No\nomitempty: false is an observable state (unmanaged), not an absence.", + "type": "boolean" + }, "metadata": { "$ref": "#/components/schemas/github_com_stacklok_toolhive_pkg_skills.SkillMetadata" }, diff --git a/docs/server/swagger.yaml b/docs/server/swagger.yaml index 303ec08a2f..39a14d1aab 100644 --- a/docs/server/swagger.yaml +++ b/docs/server/swagger.yaml @@ -1732,6 +1732,12 @@ components: installed_at: description: InstalledAt is the timestamp when the skill was installed. type: string + managed: + description: |- + Managed indicates this install is tracked in the project's + toolhive.lock.yaml. Only ever true for project-scoped installs. No + omitempty: false is an observable state (unmanaged), not an absence. + type: boolean metadata: $ref: '#/components/schemas/github_com_stacklok_toolhive_pkg_skills.SkillMetadata' project_root: diff --git a/pkg/skills/lock_service.go b/pkg/skills/lock_service.go new file mode 100644 index 0000000000..e0c37acbb6 --- /dev/null +++ b/pkg/skills/lock_service.go @@ -0,0 +1,20 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package skills + +import "context" + +//go:generate mockgen -destination=mocks/mock_lock_service.go -package=mocks -source=lock_service.go SkillLockService + +// SkillLockService defines the interface for operations driven by a +// project's toolhive.lock.yaml. It is separate from [SkillService] because +// Sync and Upgrade operate over the whole lock file rather than a single +// named skill, per RFC THV-0080. +type SkillLockService interface { + // Sync restores the project's installed skills to match its lock file. + Sync(ctx context.Context, opts SyncOptions) (*SyncResult, error) + // Upgrade re-resolves each lock entry's source and installs newer content + // when the resolved digest has changed. + Upgrade(ctx context.Context, opts UpgradeOptions) (*UpgradeResult, error) +} diff --git a/pkg/skills/mocks/mock_lock_service.go b/pkg/skills/mocks/mock_lock_service.go new file mode 100644 index 0000000000..e1c63d4a80 --- /dev/null +++ b/pkg/skills/mocks/mock_lock_service.go @@ -0,0 +1,72 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: lock_service.go +// +// Generated by this command: +// +// mockgen -destination=mocks/mock_lock_service.go -package=mocks -source=lock_service.go SkillLockService +// + +// Package mocks is a generated GoMock package. +package mocks + +import ( + context "context" + reflect "reflect" + + skills "github.com/stacklok/toolhive/pkg/skills" + gomock "go.uber.org/mock/gomock" +) + +// MockSkillLockService is a mock of SkillLockService interface. +type MockSkillLockService struct { + ctrl *gomock.Controller + recorder *MockSkillLockServiceMockRecorder + isgomock struct{} +} + +// MockSkillLockServiceMockRecorder is the mock recorder for MockSkillLockService. +type MockSkillLockServiceMockRecorder struct { + mock *MockSkillLockService +} + +// NewMockSkillLockService creates a new mock instance. +func NewMockSkillLockService(ctrl *gomock.Controller) *MockSkillLockService { + mock := &MockSkillLockService{ctrl: ctrl} + mock.recorder = &MockSkillLockServiceMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockSkillLockService) EXPECT() *MockSkillLockServiceMockRecorder { + return m.recorder +} + +// Sync mocks base method. +func (m *MockSkillLockService) Sync(ctx context.Context, opts skills.SyncOptions) (*skills.SyncResult, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Sync", ctx, opts) + ret0, _ := ret[0].(*skills.SyncResult) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// Sync indicates an expected call of Sync. +func (mr *MockSkillLockServiceMockRecorder) Sync(ctx, opts any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Sync", reflect.TypeOf((*MockSkillLockService)(nil).Sync), ctx, opts) +} + +// Upgrade mocks base method. +func (m *MockSkillLockService) Upgrade(ctx context.Context, opts skills.UpgradeOptions) (*skills.UpgradeResult, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Upgrade", ctx, opts) + ret0, _ := ret[0].(*skills.UpgradeResult) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// Upgrade indicates an expected call of Upgrade. +func (mr *MockSkillLockServiceMockRecorder) Upgrade(ctx, opts any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Upgrade", reflect.TypeOf((*MockSkillLockService)(nil).Upgrade), ctx, opts) +} diff --git a/pkg/skills/options.go b/pkg/skills/options.go index a822c8918f..6eb0525440 100644 --- a/pkg/skills/options.go +++ b/pkg/skills/options.go @@ -133,6 +133,134 @@ type PushOptions struct { Reference string `json:"reference"` } +// SyncOptions configures the behavior of the Sync operation. +type SyncOptions struct { + // ProjectRoot is the project root path whose lock file should be synced. + ProjectRoot string `json:"project_root"` + // Clients lists target clients (e.g., "claude-code"). Empty means every + // skill-supporting client detected on this host. + Clients []string `json:"clients,omitempty"` + // Prune removes project-scoped skills that are installed but not present + // in the lock file. When false, such skills are only reported. + Prune bool `json:"prune,omitempty"` + // Check verifies on-disk content against contentDigest without installing + // or writing anything. + Check bool `json:"check,omitempty"` + // Adopt writes lock entries for existing unmanaged project-scope installs. + Adopt bool `json:"adopt,omitempty"` +} + +// FailureReason is a typed failure reason for sync/upgrade operations, per +// RFC THV-0080's exit-code and automation contract. Only reasons the current +// feature set can actually produce are defined; the RFC's remaining values +// (the Sigstore verification reasons and ref-change-blocked, which surfaces +// as an UpgradeStatus rather than a failure) land together with the code +// that emits them. +type FailureReason string + +// Typed failure reasons for sync/upgrade operations. +const ( + // FailureReasonRegistryUnreachable means the skill's remote source — an + // OCI registry or a git host — could not be reached. + FailureReasonRegistryUnreachable FailureReason = "registry-unreachable" + FailureReasonDigestMissing FailureReason = "digest-missing" + FailureReasonValidationRejected FailureReason = "validation-rejected" + FailureReasonLockWriteFailed FailureReason = "lock-write-failed" + FailureReasonUnknown FailureReason = "unknown" +) + +// SyncFailure describes a single skill that failed to sync. +type SyncFailure struct { + // Name is the skill name that failed. + Name string `json:"name"` + // Reason is a typed failure reason for CI and automation. + Reason FailureReason `json:"reason,omitempty"` + // Error is a human-readable description of the failure. + Error string `json:"error"` +} + +// SyncResult contains the outcome of a Sync operation. +type SyncResult struct { + // Installed lists skills that were installed or reinstalled to match the lock file. + Installed []string `json:"installed,omitempty"` + // Drifted lists skills whose on-disk contentDigest differed from the lock + // file. Normally these are reinstalled to match it; when Check is set, + // nothing is written and this field reports the drift only. + Drifted []string `json:"drifted,omitempty"` + // AlreadyCurrent lists skills that already matched the lock file. + AlreadyCurrent []string `json:"already_current,omitempty"` + // NeverManaged lists project-scoped skills never recorded as lock-managed. + NeverManaged []string `json:"never_managed,omitempty"` + // RemovedFromLock lists previously managed skills absent from the lock file. + RemovedFromLock []string `json:"removed_from_lock,omitempty"` + // Pruned lists removed-from-lock skills that were uninstalled because Prune was set. + Pruned []string `json:"pruned,omitempty"` + // Failed lists skills that could not be synced, with the reason for each. + // Drift alone is never reported here — see Drifted. + Failed []SyncFailure `json:"failed,omitempty"` +} + +// UpgradeOptions configures the behavior of the Upgrade operation. +type UpgradeOptions struct { + // ProjectRoot is the project root path whose lock file should be upgraded. + ProjectRoot string `json:"project_root"` + // Names restricts the upgrade to specific skill names. Empty means every + // entry in the lock file. + Names []string `json:"names,omitempty"` + // Preview reports what would change without installing (still fetches + // artifacts to compare digests). + Preview bool `json:"preview,omitempty"` + // FailOnChanges exits with an error when any mutable source would upgrade. + FailOnChanges bool `json:"fail_on_changes,omitempty"` + // AllowRefChange permits resolvedReference changes during upgrade. + AllowRefChange bool `json:"allow_ref_change,omitempty"` + // Clients lists target clients (e.g., "claude-code"). Empty means every + // skill-supporting client detected on this host. + Clients []string `json:"clients,omitempty"` +} + +// UpgradeStatus represents the outcome of upgrading a single skill. +type UpgradeStatus string + +const ( + // UpgradeStatusUpgraded indicates the skill was installed at a new digest. + UpgradeStatusUpgraded UpgradeStatus = "upgraded" + // UpgradeStatusUpToDate indicates the resolved source still points at the pinned digest. + UpgradeStatusUpToDate UpgradeStatus = "up-to-date" + // UpgradeStatusNotUpgradable indicates the entry is pinned to an immutable + // reference (an OCI digest or a full git commit hash) and cannot be upgraded. + UpgradeStatusNotUpgradable UpgradeStatus = "not-upgradable" + // UpgradeStatusRefChangeBlocked indicates re-resolution changed resolvedReference. + UpgradeStatusRefChangeBlocked UpgradeStatus = "ref-change-blocked" + // UpgradeStatusFailed indicates the upgrade attempt failed. + UpgradeStatusFailed UpgradeStatus = "failed" +) + +// UpgradeOutcome describes the result of attempting to upgrade one skill. +type UpgradeOutcome struct { + // Name is the skill name. + Name string `json:"name"` + // Status is the outcome of the upgrade attempt. + Status UpgradeStatus `json:"status"` + // OldDigest is the digest pinned in the lock file before this operation. + OldDigest string `json:"old_digest,omitempty"` + // NewDigest is the digest the source currently resolves to. Equal to + // OldDigest when Status is UpgradeStatusUpToDate. + NewDigest string `json:"new_digest,omitempty"` + // NewResolvedReference is the new resolvedReference when it changed. + NewResolvedReference string `json:"new_resolved_reference,omitempty"` + // Reason is a typed failure reason when Status is UpgradeStatusFailed. + Reason FailureReason `json:"reason,omitempty"` + // Error is a human-readable description of the failure, set only when Status is UpgradeStatusFailed. + Error string `json:"error,omitempty"` +} + +// UpgradeResult contains the outcome of an Upgrade operation. +type UpgradeResult struct { + // Outcomes contains one entry per skill considered for upgrade. + Outcomes []UpgradeOutcome `json:"outcomes"` +} + // LocalBuild represents a locally-built OCI skill artifact in the local store. type LocalBuild struct { // Tag is the OCI tag or name used to reference the artifact. diff --git a/pkg/skills/types.go b/pkg/skills/types.go index 224e7b8d00..f908de075e 100644 --- a/pkg/skills/types.go +++ b/pkg/skills/types.go @@ -167,6 +167,10 @@ type InstalledSkill struct { Clients []string `json:"clients,omitempty"` // Dependencies is the list of external skill dependencies. Dependencies []Dependency `json:"dependencies,omitempty"` + // Managed indicates this install is tracked in the project's + // toolhive.lock.yaml. Only ever true for project-scoped installs. No + // omitempty: false is an observable state (unmanaged), not an absence. + Managed bool `json:"managed"` } // SkillIndexEntry represents a single skill entry in a remote skill index. diff --git a/pkg/storage/sqlite/migrations/003_add_managed_flag.sql b/pkg/storage/sqlite/migrations/003_add_managed_flag.sql new file mode 100644 index 0000000000..e9dd5dd55f --- /dev/null +++ b/pkg/storage/sqlite/migrations/003_add_managed_flag.sql @@ -0,0 +1,7 @@ +-- +goose Up + +ALTER TABLE installed_skills ADD COLUMN managed INTEGER NOT NULL DEFAULT 0; + +-- +goose Down + +ALTER TABLE installed_skills DROP COLUMN managed; diff --git a/pkg/storage/sqlite/migrations_test.go b/pkg/storage/sqlite/migrations_test.go index 52e1012905..dd48c25703 100644 --- a/pkg/storage/sqlite/migrations_test.go +++ b/pkg/storage/sqlite/migrations_test.go @@ -130,6 +130,51 @@ func newGooseProvider(t *testing.T, db *sql.DB) *goose.Provider { return provider } +// TestMigrations_ManagedFlagAppliesOverPriorState verifies migration 003 adds +// installed_skills.managed with a default of 0 (false) so rows created by an +// earlier version of the schema (001/002) are not implicitly "managed" once +// the column appears. +func TestMigrations_ManagedFlagAppliesOverPriorState(t *testing.T) { + t.Parallel() + ctx := t.Context() + dbPath := filepath.Join(t.TempDir(), "test.db") + + db, err := Open(ctx, dbPath) + require.NoError(t, err) + defer db.Close() + + provider := newGooseProvider(t, db.DB()) + + // Roll back to just after migration 002, before "managed" existed. + _, err = provider.DownTo(ctx, 2) + require.NoError(t, err) + + _, err = db.DB().Exec(`INSERT INTO entries (entry_type, name) VALUES ('skill', 'pre-migration-skill')`) + require.NoError(t, err) + _, err = db.DB().Exec(`INSERT INTO installed_skills (entry_id, scope) VALUES (1, 'user')`) + require.NoError(t, err) + + // Re-apply Up: migration 003 adds the column to the pre-existing row. + _, err = provider.Up(ctx) + require.NoError(t, err) + + var managed int + err = db.DB().QueryRow(`SELECT managed FROM installed_skills WHERE entry_id = 1`).Scan(&managed) + require.NoError(t, err) + assert.Equal(t, 0, managed, "a row created before migration 003 must default to unmanaged") + + // Down for 003 removes the column without disturbing 001/002 tables. + _, err = provider.DownTo(ctx, 2) + require.NoError(t, err) + var count int + err = db.DB().QueryRow( + `SELECT COUNT(*) FROM pragma_table_info('installed_skills') WHERE name = 'managed'`, + ).Scan(&count) + require.NoError(t, err) + assert.Equal(t, 0, count, "managed column should be dropped by 003 Down") + assert.True(t, tableExists(t, db.DB(), "installed_skills"), "installed_skills table itself must remain") +} + // TestMigrations_DownDropsPluginTables verifies that goose's Down for migration // 002 drops the plugin tables (installed_plugins, plugin_dependencies) while // leaving migration 001's tables (entries, installed_skills, skill_dependencies, diff --git a/pkg/storage/sqlite/skill_store.go b/pkg/storage/sqlite/skill_store.go index 87d197d849..0efd1065a9 100644 --- a/pkg/storage/sqlite/skill_store.go +++ b/pkg/storage/sqlite/skill_store.go @@ -39,10 +39,19 @@ var _ storage.SkillStore = (*SkillStore)(nil) // skillColumns is the SELECT column list shared by Get and List queries. const skillColumns = `is_.id, e.name, is_.scope, is_.project_root, is_.reference, is_.tag, is_.digest, is_.version, is_.description, is_.author, json(is_.tags), - json(is_.client_apps), is_.status, is_.installed_at` + json(is_.client_apps), is_.status, is_.installed_at, is_.managed` + +// errManagedRequiresProjectScope is returned by Create/Update when a +// user-scoped skill has Managed set. Managed pins a skill in a project's +// lock file, which only exists for project-scoped installs. +var errManagedRequiresProjectScope = errors.New("managed skill must be project-scoped") // Create stores a new installed skill. func (s *SkillStore) Create(ctx context.Context, skill skills.InstalledSkill) error { + if skill.Managed && skill.Scope != skills.ScopeProject { + return errManagedRequiresProjectScope + } + tx, err := s.db.BeginTx(ctx, nil) if err != nil { return fmt.Errorf("beginning transaction: %w", err) @@ -85,8 +94,8 @@ func (s *SkillStore) Create(ctx context.Context, skill skills.InstalledSkill) er res, err := tx.ExecContext(ctx, ` INSERT INTO installed_skills ( entry_id, scope, project_root, reference, tag, digest, - version, description, author, tags, client_apps, status - ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, jsonb(?), jsonb(?), ?)`, + version, description, author, tags, client_apps, status, managed + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, jsonb(?), jsonb(?), ?, ?)`, entryID, string(skill.Scope), skill.ProjectRoot, @@ -99,6 +108,7 @@ func (s *SkillStore) Create(ctx context.Context, skill skills.InstalledSkill) er tagsJSON, clientsJSON, string(skill.Status), + boolToInt(skill.Managed), ) if err != nil { if isUniqueViolation(err) { @@ -221,6 +231,10 @@ func (s *SkillStore) List(ctx context.Context, filter storage.ListFilter) ([]ski // Update modifies an existing installed skill. func (s *SkillStore) Update(ctx context.Context, skill skills.InstalledSkill) error { + if skill.Managed && skill.Scope != skills.ScopeProject { + return errManagedRequiresProjectScope + } + tx, err := s.db.BeginTx(ctx, nil) if err != nil { return fmt.Errorf("beginning transaction: %w", err) @@ -266,7 +280,7 @@ func (s *SkillStore) Update(ctx context.Context, skill skills.InstalledSkill) er if _, err := tx.ExecContext(ctx, ` UPDATE installed_skills SET reference = ?, tag = ?, digest = ?, version = ?, description = ?, - author = ?, tags = jsonb(?), client_apps = jsonb(?), status = ? + author = ?, tags = jsonb(?), client_apps = jsonb(?), status = ?, managed = ? WHERE id = ?`, skill.Reference, skill.Tag, @@ -277,6 +291,7 @@ func (s *SkillStore) Update(ctx context.Context, skill skills.InstalledSkill) er tagsJSON, clientsJSON, string(skill.Status), + boolToInt(skill.Managed), installedSkillID, ); err != nil { return fmt.Errorf("updating installed skill: %w", err) @@ -370,12 +385,13 @@ func scanSkillFields(sc scanner) (skills.InstalledSkill, int64, error) { clientsBlob []byte status string installedAtStr string + managed int ) err := sc.Scan( &installedSkillID, &name, &scope, &projectRoot, &reference, &tag, &digest, &version, &description, &author, &tagsBlob, - &clientsBlob, &status, &installedAtStr, + &clientsBlob, &status, &installedAtStr, &managed, ) if err != nil { if errors.Is(err, sql.ErrNoRows) { @@ -412,6 +428,7 @@ func scanSkillFields(sc scanner) (skills.InstalledSkill, int64, error) { Status: skills.InstallStatus(status), InstalledAt: installedAt, Clients: clients, + Managed: managed != 0, } return sk, installedSkillID, nil @@ -495,3 +512,11 @@ func isUniqueViolation(err error) bool { // rollback rolls back tx, ignoring errors (tx may already be committed). func rollback(tx *sql.Tx) { _ = tx.Rollback() } + +// boolToInt converts a bool to SQLite's 0/1 INTEGER representation. +func boolToInt(b bool) int { + if b { + return 1 + } + return 0 +} diff --git a/pkg/storage/sqlite/skill_store_test.go b/pkg/storage/sqlite/skill_store_test.go index 3d6efe578a..5c057f055c 100644 --- a/pkg/storage/sqlite/skill_store_test.go +++ b/pkg/storage/sqlite/skill_store_test.go @@ -72,6 +72,52 @@ func TestSkillStore_Create(t *testing.T) { // InstalledAt is set by the DB DEFAULT, so just assert it is not zero. assert.False(t, got.InstalledAt.IsZero(), "InstalledAt should not be zero") + assert.False(t, got.Managed, "Managed should default to false") +} + +func TestSkillStore_ManagedFlagRoundTrip(t *testing.T) { + t.Parallel() + store := newTestStore(t) + + sk := testSkill("managed-test") + sk.Scope = skills.ScopeProject + sk.ProjectRoot = "/tmp/project" + sk.Managed = true + require.NoError(t, store.Create(t.Context(), sk)) + + got, err := store.Get(t.Context(), sk.Metadata.Name, sk.Scope, sk.ProjectRoot) + require.NoError(t, err) + assert.True(t, got.Managed) + + // Update can flip managed back to false (e.g. sync --prune leaving the + // record but marking it unmanaged). + got.Managed = false + require.NoError(t, store.Update(t.Context(), got)) + + got, err = store.Get(t.Context(), sk.Metadata.Name, sk.Scope, sk.ProjectRoot) + require.NoError(t, err) + assert.False(t, got.Managed) +} + +// TestSkillStore_ManagedRequiresProjectScope guards the invariant that +// Managed only ever applies to project-scoped installs (it pins a skill in +// a project's lock file, which doesn't exist for user-scoped installs). +func TestSkillStore_ManagedRequiresProjectScope(t *testing.T) { + t.Parallel() + store := newTestStore(t) + + sk := testSkill("managed-user-scope") + sk.Managed = true // sk.Scope is ScopeUser from testSkill + err := store.Create(t.Context(), sk) + require.ErrorIs(t, err, errManagedRequiresProjectScope) + + // Also rejected on Update: create a valid user-scoped record, then try + // to flip Managed on it. + sk.Managed = false + require.NoError(t, store.Create(t.Context(), sk)) + sk.Managed = true + err = store.Update(t.Context(), sk) + require.ErrorIs(t, err, errManagedRequiresProjectScope) } func TestSkillStore_CreateDuplicate(t *testing.T) {