From 79bb8aed6cca3895b7c888c89754ba01f5b8694b Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Tue, 21 Jul 2026 15:57:04 +0200 Subject: [PATCH 1/3] Add SkillLockService interface and managed install flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RFC THV-0080's sync and upgrade commands operate over a project's whole lock file rather than a single named skill, so they need their own service boundary (SkillLockService) instead of extending SkillService. Installed skills also need a durable "is this tracked in the lock file" marker so sync --prune can distinguish lock-managed installs from ones a user manages by hand. - SkillLockService{Sync,Upgrade} interface with generated mock - Sync/Upgrade option and result types, RFC's typed failure reasons - installed_skills.managed column (migration 003) Part of the production stack for #5715 (RFC THV-0080). Stack: 2/6. No consumers yet — skillsvc wiring lands in PR3. --- docs/server/docs.go | 4 + docs/server/swagger.json | 4 + docs/server/swagger.yaml | 5 + pkg/skills/lock_service.go | 20 +++ pkg/skills/mocks/mock_lock_service.go | 72 ++++++++++ pkg/skills/options.go | 126 ++++++++++++++++++ pkg/skills/types.go | 3 + .../migrations/003_add_managed_flag.sql | 7 + pkg/storage/sqlite/migrations_test.go | 45 +++++++ pkg/storage/sqlite/skill_store.go | 22 ++- pkg/storage/sqlite/skill_store_test.go | 23 ++++ 11 files changed, 326 insertions(+), 5 deletions(-) create mode 100644 pkg/skills/lock_service.go create mode 100644 pkg/skills/mocks/mock_lock_service.go create mode 100644 pkg/storage/sqlite/migrations/003_add_managed_flag.sql diff --git a/docs/server/docs.go b/docs/server/docs.go index 84183297ac..9cabf05b3c 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.", + "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..30dcd7bf04 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.", + "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..761edf1d7a 100644 --- a/docs/server/swagger.yaml +++ b/docs/server/swagger.yaml @@ -1732,6 +1732,11 @@ 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. + 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..c156b20edb 100644 --- a/pkg/skills/options.go +++ b/pkg/skills/options.go @@ -133,6 +133,132 @@ 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. +type FailureReason string + +// Typed failure reasons for sync/upgrade operations. +const ( + FailureReasonRegistryUnreachable FailureReason = "registry-unreachable" + FailureReasonDigestMissing FailureReason = "digest-missing" + FailureReasonValidationRejected FailureReason = "validation-rejected" + FailureReasonLockWriteFailed FailureReason = "lock-write-failed" + FailureReasonRefChangeBlocked FailureReason = "ref-change-blocked" + FailureReasonSignatureInvalid FailureReason = "signature-invalid" + FailureReasonSignerMismatch FailureReason = "signer-mismatch" + FailureReasonUnsignedRejected FailureReason = "unsigned-rejected" + 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 and were reinstalled to match it (reinstalled also when Check is set, + // in which case nothing is written and this reports the drift only). + Drifted []string `json:"drifted,omitempty"` + // AlreadyCurrent lists skills that already matched the lock file. + AlreadyCurrent []string `json:"up_to_date,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..93cc846a6f 100644 --- a/pkg/skills/types.go +++ b/pkg/skills/types.go @@ -167,6 +167,9 @@ 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. + Managed bool `json:"managed,omitempty"` } // 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..475da4b8cc 100644 --- a/pkg/storage/sqlite/skill_store.go +++ b/pkg/storage/sqlite/skill_store.go @@ -39,7 +39,7 @@ 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` // Create stores a new installed skill. func (s *SkillStore) Create(ctx context.Context, skill skills.InstalledSkill) error { @@ -85,8 +85,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 +99,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) { @@ -266,7 +267,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 +278,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 +372,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 +415,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 +499,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..0088f677d0 100644 --- a/pkg/storage/sqlite/skill_store_test.go +++ b/pkg/storage/sqlite/skill_store_test.go @@ -72,6 +72,29 @@ 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.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) } func TestSkillStore_CreateDuplicate(t *testing.T) { From 28ac86e49aae3a744e418e3b62024f02db56e951 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Tue, 21 Jul 2026 18:59:49 +0200 Subject: [PATCH 2/3] Enforce Managed only for project-scoped skills Managed is documented as pinning a skill in a project's lock file, but Create/Update wrote it unconditionally regardless of scope, and the existing round-trip test created a user-scoped skill with Managed: true. Reject the invalid combination in the store instead of only in a doc comment. Also fix a self-contradictory SyncResult doc comment and an inconsistent JSON tag on AlreadyCurrent. --- pkg/skills/options.go | 6 +++--- pkg/storage/sqlite/skill_store.go | 13 +++++++++++++ pkg/storage/sqlite/skill_store_test.go | 23 +++++++++++++++++++++++ 3 files changed, 39 insertions(+), 3 deletions(-) diff --git a/pkg/skills/options.go b/pkg/skills/options.go index c156b20edb..7db2a0d7dc 100644 --- a/pkg/skills/options.go +++ b/pkg/skills/options.go @@ -182,11 +182,11 @@ 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 and were reinstalled to match it (reinstalled also when Check is set, - // in which case nothing is written and this reports the drift only). + // 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:"up_to_date,omitempty"` + 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. diff --git a/pkg/storage/sqlite/skill_store.go b/pkg/storage/sqlite/skill_store.go index 475da4b8cc..0efd1065a9 100644 --- a/pkg/storage/sqlite/skill_store.go +++ b/pkg/storage/sqlite/skill_store.go @@ -41,8 +41,17 @@ const skillColumns = `is_.id, e.name, is_.scope, is_.project_root, is_.reference is_.digest, is_.version, is_.description, is_.author, json(is_.tags), 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) @@ -222,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) diff --git a/pkg/storage/sqlite/skill_store_test.go b/pkg/storage/sqlite/skill_store_test.go index 0088f677d0..5c057f055c 100644 --- a/pkg/storage/sqlite/skill_store_test.go +++ b/pkg/storage/sqlite/skill_store_test.go @@ -80,6 +80,8 @@ func TestSkillStore_ManagedFlagRoundTrip(t *testing.T) { 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)) @@ -97,6 +99,27 @@ func TestSkillStore_ManagedFlagRoundTrip(t *testing.T) { 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) { t.Parallel() store := newTestStore(t) From bae69170e7e7778ddd5d52996ec4877fa27f689a Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Thu, 23 Jul 2026 10:45:17 +0200 Subject: [PATCH 3/3] Trim unproduced FailureReason values and expose Managed=false MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four typed failure reasons shipped in the JSON API with no code able to produce them: the three Sigstore verification reasons (deferred to the signing stack) and ref-change-blocked (which surfaces as an UpgradeStatus, not a failure). Pinning unproducible API constants now would make refining them later a breaking change — they land with their producers instead. Also drop omitempty from InstalledSkill's Managed flag so list/get consumers can observe the false state, and reword registry-unreachable's doc to cover git hosts, which map to the same reason. --- docs/server/docs.go | 2 +- docs/server/swagger.json | 2 +- docs/server/swagger.yaml | 3 ++- pkg/skills/options.go | 12 +++++++----- pkg/skills/types.go | 5 +++-- 5 files changed, 14 insertions(+), 10 deletions(-) diff --git a/docs/server/docs.go b/docs/server/docs.go index 9cabf05b3c..24270dbbea 100644 --- a/docs/server/docs.go +++ b/docs/server/docs.go @@ -1692,7 +1692,7 @@ const docTemplate = `{ "type": "string" }, "managed": { - "description": "Managed indicates this install is tracked in the project's\ntoolhive.lock.yaml. Only ever true for project-scoped installs.", + "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": { diff --git a/docs/server/swagger.json b/docs/server/swagger.json index 30dcd7bf04..322fbb8507 100644 --- a/docs/server/swagger.json +++ b/docs/server/swagger.json @@ -1685,7 +1685,7 @@ "type": "string" }, "managed": { - "description": "Managed indicates this install is tracked in the project's\ntoolhive.lock.yaml. Only ever true for project-scoped installs.", + "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": { diff --git a/docs/server/swagger.yaml b/docs/server/swagger.yaml index 761edf1d7a..39a14d1aab 100644 --- a/docs/server/swagger.yaml +++ b/docs/server/swagger.yaml @@ -1735,7 +1735,8 @@ components: managed: description: |- Managed indicates this install is tracked in the project's - toolhive.lock.yaml. Only ever true for project-scoped installs. + 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' diff --git a/pkg/skills/options.go b/pkg/skills/options.go index 7db2a0d7dc..6eb0525440 100644 --- a/pkg/skills/options.go +++ b/pkg/skills/options.go @@ -151,19 +151,21 @@ type SyncOptions struct { } // FailureReason is a typed failure reason for sync/upgrade operations, per -// RFC THV-0080's exit-code and automation contract. +// 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" - FailureReasonRefChangeBlocked FailureReason = "ref-change-blocked" - FailureReasonSignatureInvalid FailureReason = "signature-invalid" - FailureReasonSignerMismatch FailureReason = "signer-mismatch" - FailureReasonUnsignedRejected FailureReason = "unsigned-rejected" FailureReasonUnknown FailureReason = "unknown" ) diff --git a/pkg/skills/types.go b/pkg/skills/types.go index 93cc846a6f..f908de075e 100644 --- a/pkg/skills/types.go +++ b/pkg/skills/types.go @@ -168,8 +168,9 @@ type InstalledSkill struct { // 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. - Managed bool `json:"managed,omitempty"` + // 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.