Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions pkg/skills/skillsvc/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package skillsvc

import (
"context"
"errors"
"fmt"
"log/slog"
"net/http"
Expand Down Expand Up @@ -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
}
Expand Down
27 changes: 27 additions & 0 deletions pkg/skills/skillsvc/lock_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package skillsvc
import (
"context"
"fmt"
"net/http"
"os"
"path/filepath"
"strings"
Expand All @@ -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"
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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")
}
Loading