diff --git a/pkg/skills/skillsvc/install.go b/pkg/skills/skillsvc/install.go index 501b2de556..4a982a8729 100644 --- a/pkg/skills/skillsvc/install.go +++ b/pkg/skills/skillsvc/install.go @@ -5,6 +5,7 @@ package skillsvc import ( "context" + "errors" "fmt" "log/slog" "net/http" @@ -267,10 +268,17 @@ func (s *service) installAndRegister( updated, err := s.recordLockState(ctx, opts, originalName, result.Skill) if err != nil { rollback() - return nil, httperr.WithCode( - fmt.Errorf("recording skill in project lock file: %w", err), - http.StatusInternalServerError, - ) + // 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 + // API boundary as itself, not masked to 500. Only a code-less + // failure (e.g. an actual lock write error) defaults to 500. + wrapped := fmt.Errorf("recording skill in project lock file: %w", err) + var coded *httperr.CodedError + if !errors.As(err, &coded) { + wrapped = httperr.WithCode(wrapped, http.StatusInternalServerError) + } + return nil, wrapped } result.Skill = updated } diff --git a/pkg/skills/skillsvc/lock_test.go b/pkg/skills/skillsvc/lock_test.go index e46da33153..3b2da5f44e 100644 --- a/pkg/skills/skillsvc/lock_test.go +++ b/pkg/skills/skillsvc/lock_test.go @@ -6,6 +6,7 @@ package skillsvc import ( "context" "fmt" + "net/http" "os" "path/filepath" "strings" @@ -16,6 +17,7 @@ import ( "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" + "github.com/stacklok/toolhive-core/httperr" "github.com/stacklok/toolhive/pkg/groups" groupmocks "github.com/stacklok/toolhive/pkg/groups/mocks" "github.com/stacklok/toolhive/pkg/skills" @@ -353,6 +355,8 @@ func TestInstallProjectScope_LockWriteFailureRollsBackInstall(t *testing.T) { Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, }) require.Error(t, err, "install must fail when the lock file cannot be written") + assert.Equal(t, http.StatusInternalServerError, httperr.Code(err), + "a code-less lock-write failure still defaults to 500") _, 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") @@ -557,3 +561,26 @@ func TestUninstall_LockWriteFailureAbortsBeforeDestruction(t *testing.T) { _, statErr := os.Stat(skillMD) require.NoError(t, statErr, "on-disk files must be untouched") } + +// TestInstallProjectScope_DependencyFailureCodePreserved: a dependency's +// specific failure code must reach the API boundary as itself. Dependency +// materialization runs inside recordLockState, whose failures used to be +// blanket-wrapped as 500 — masking a git resolver's 502 (and turning the +// typed sync failure reason into "unknown" instead of +// "registry-unreachable"). +// +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestInstallProjectScope_DependencyFailureCodePreserved(t *testing.T) { + gr, fx := newGitResolverMock(t) + missingDepRef, _ := gitRef("missing-dep") // never registered — Resolve fails with 502 + fx.register("parent-skill", gitSkill("parent-skill", missingDepRef)) + svc, projectRoot := newLockTestService(t, gr) + + ref, _ := gitRef("parent-skill") + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.Error(t, err) + assert.Equal(t, http.StatusBadGateway, httperr.Code(err), + "the dependency's 502 must not be masked to 500 by the lock-state wrap") +}