Address PR review feedback for RegisterEntity

- Use errors.As pattern for UserVisibleError passthrough in handlers
- Convert validator errors to util.UserVisibleError for proper gRPC responses
- Reduce cyclomatic complexity in CreateEntity by extracting helpers:
  - runValidators for validator loop
  - cleanupProviderRegistration for cleanup logic
- Remove unused functions:
  - upsertLegacyEntity in add_originating_entity.go
  - pushReconcilerEvent and persistRepository in service.go
  - Unused test helpers in handler_test.go and service_test.go
- Fix unused parameters in test files
- Update tests to mock EntityCreator instead of internal services
- Update expected error messages to match new UserVisibleError format

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>

Author: JAORMX

internal/controlplane/handlers_entity_instances.go

@@ -14,7 +14,6 @@ import (
 
 	"github.com/mindersec/minder/internal/engine/engcontext"
 	"github.com/mindersec/minder/internal/entities/models"
-	"github.com/mindersec/minder/internal/entities/service/validators"
 	"github.com/mindersec/minder/internal/logger"
 	"github.com/mindersec/minder/internal/util"
 	pb "github.com/mindersec/minder/pkg/api/protobuf/go/minder/v1"
@@ -224,9 +223,12 @@ func (s *Server) RegisterEntity(
 	ewp, err := s.entityCreator.CreateEntity(ctx, provider, projectID,
 		in.GetEntityType(), identifyingProps, nil) // Use default options
 	if err != nil {
-		if errors.Is(err, validators.ErrPrivateRepoForbidden) ||
-			errors.Is(err, validators.ErrArchivedRepoForbidden) {
-			return nil, util.UserVisibleError(codes.InvalidArgument, "%s", err.Error())
+		// If the error is already a UserVisibleError, pass it through directly.
+		// This allows providers and EntityCreator to add user-visible errors
+		// without needing to update this allow-list.
+		var userErr *util.NiceStatus
+		if errors.As(err, &userErr) {
+			return nil, err
 		}
 		return nil, util.UserVisibleError(codes.Internal,
 			"unable to register entity: %v", err)

internal/controlplane/handlers_entity_instances_test.go

@@ -36,14 +36,14 @@ func TestServer_RegisterEntity(t *testing.T) {
 	providerName := "github"
 
 	tests := []struct {
-		name          string
-		request       *pb.RegisterEntityRequest
-		setupContext  func(context.Context) context.Context
-		setupMocks    func(*mockproviders.MockProviderStore, *mockentitysvc.MockEntityCreator)
-		wantErr       bool
-		wantCode      codes.Code
-		errContains   string
-		validateResp  func(*testing.T, *pb.RegisterEntityResponse)
+		name         string
+		request      *pb.RegisterEntityRequest
+		setupContext func(context.Context) context.Context
+		setupMocks   func(*mockproviders.MockProviderStore, *mockentitysvc.MockEntityCreator)
+		wantErr      bool
+		wantCode     codes.Code
+		errContains  string
+		validateResp func(*testing.T, *pb.RegisterEntityResponse)
 	}{
 		{
 			name: "successfully registers repository",
@@ -95,6 +95,7 @@ func TestServer_RegisterEntity(t *testing.T) {
 			},
 			wantErr: false,
 			validateResp: func(t *testing.T, resp *pb.RegisterEntityResponse) {
+				t.Helper()
 				assert.NotNil(t, resp)
 				assert.NotNil(t, resp.GetEntity())
 				assert.Equal(t, entityID.String(), resp.GetEntity().GetId())
@@ -118,9 +119,7 @@ func TestServer_RegisterEntity(t *testing.T) {
 					Provider: engcontext.Provider{Name: providerName},
 				})
 			},
-			setupMocks: func(provStore *mockproviders.MockProviderStore, creator *mockentitysvc.MockEntityCreator) {
-				// No mocks needed - should fail early
-			},
+			// No mocks needed - should fail early
 			wantErr:     true,
 			wantCode:    codes.InvalidArgument,
 			errContains: "entity_type must be specified",
@@ -138,7 +137,7 @@ func TestServer_RegisterEntity(t *testing.T) {
 					Provider: engcontext.Provider{Name: providerName},
 				})
 			},
-			setupMocks: func(provStore *mockproviders.MockProviderStore, creator *mockentitysvc.MockEntityCreator) {},
+			// No mocks needed - should fail early
 			wantErr:     true,
 			wantCode:    codes.InvalidArgument,
 			errContains: "identifying_properties is required",
@@ -159,7 +158,7 @@ func TestServer_RegisterEntity(t *testing.T) {
 					Provider: engcontext.Provider{Name: providerName},
 				})
 			},
-			setupMocks: func(provStore *mockproviders.MockProviderStore, creator *mockentitysvc.MockEntityCreator) {
+			setupMocks: func(provStore *mockproviders.MockProviderStore, _ *mockentitysvc.MockEntityCreator) {
 				provStore.EXPECT().
 					GetByName(gomock.Any(), projectID, providerName).
 					Return(nil, sql.ErrNoRows)
@@ -286,7 +285,9 @@ func TestServer_RegisterEntity(t *testing.T) {
 			mockProvStore := mockproviders.NewMockProviderStore(ctrl)
 			mockEntityCreator := mockentitysvc.NewMockEntityCreator(ctrl)
 
-			tt.setupMocks(mockProvStore, mockEntityCreator)
+			if tt.setupMocks != nil {
+				tt.setupMocks(mockProvStore, mockEntityCreator)
+			}
 
 			server := &Server{
 				providerStore: mockProvStore,
@@ -342,6 +343,7 @@ func TestParseIdentifyingProperties(t *testing.T) {
 			},
 			wantErr: false,
 			validate: func(t *testing.T, props *properties.Properties) {
+				t.Helper()
 				owner := props.GetProperty("github/repo_owner").GetString()
 				assert.Equal(t, "stacklok", owner)
 
@@ -402,6 +404,7 @@ func TestParseIdentifyingProperties(t *testing.T) {
 			},
 			wantErr: false,
 			validate: func(t *testing.T, props *properties.Properties) {
+				t.Helper()
 				// Empty properties is valid (provider will validate)
 				assert.NotNil(t, props)
 			},

internal/controlplane/handlers_repositories_test.go

@@ -80,7 +80,7 @@ func TestServer_RegisterRepository(t *testing.T) {
 				reposvc.ErrPrivateRepoForbidden,
 				projectID,
 			)),
-			ExpectedError: "private repos cannot be registered in this project",
+			ExpectedError: "private repositories are not allowed in this project",
 		},
 		{
 			Name:      "Repo creation fails repo is archived, and archived repos are not allowed",
@@ -90,7 +90,7 @@ func TestServer_RegisterRepository(t *testing.T) {
 				reposvc.ErrArchivedRepoForbidden,
 				projectID,
 			)),
-			ExpectedError: "archived repos cannot be registered in this project",
+			ExpectedError: "archived repositories cannot be registered",
 		},
 		{
 			Name:      "Repo creation on unexpected error",

internal/entities/handlers/handler_test.go

@@ -24,7 +24,6 @@ import (
 	"github.com/mindersec/minder/internal/entities/properties/service"
 	"github.com/mindersec/minder/internal/entities/properties/service/mock/fixtures"
 	stubeventer "github.com/mindersec/minder/internal/events/stubs"
-	pbinternal "github.com/mindersec/minder/internal/proto"
 	mockgithub "github.com/mindersec/minder/internal/providers/github/mock"
 	ghprops "github.com/mindersec/minder/internal/providers/github/properties"
 	"github.com/mindersec/minder/internal/providers/manager"
@@ -96,10 +95,6 @@ type (
 	providerMockBuilder = func(controller *gomock.Controller) providerMock
 )
 
-func getPullRequestProperties() *properties.Properties {
-	return properties.NewProperties(pullRequestPropMap)
-}
-
 func newProviderMock(opts ...func(providerMock)) providerMockBuilder {
 	return func(ctrl *gomock.Controller) providerMock {
 		mock := mockgithub.NewMockGitHub(ctrl)
@@ -110,22 +105,6 @@ func newProviderMock(opts ...func(providerMock)) providerMockBuilder {
 	}
 }
 
-func withSuccessfulGetEntityName(name string) func(providerMock) {
-	return func(mock providerMock) {
-		mock.EXPECT().
-			GetEntityName(gomock.Any(), gomock.Any()).
-			Return(name, nil)
-	}
-}
-
-func withSuccessfulFetchAllProperties(props *properties.Properties) func(mock providerMock) {
-	return func(mock providerMock) {
-		mock.EXPECT().
-			FetchAllProperties(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
-			Return(props, nil)
-	}
-}
-
 func WithSuccessfulPropertiesToProtoMessage(proto protoreflect.ProtoMessage) func(mock providerMock) {
 	return func(mock providerMock) {
 		mock.EXPECT().
@@ -168,18 +147,6 @@ func checkRepoMessage(t *testing.T, msg *watermill.Message) {
 	assert.Equal(t, repoPropMap[properties.RepoPropertyIsFork].(bool), pbrepo.IsFork)
 }
 
-func checkPullRequestMessage(t *testing.T, msg *watermill.Message) {
-	t.Helper()
-
-	eiw, err := entities.ParseEntityEvent(msg)
-	require.NoError(t, err)
-	require.NotNil(t, eiw)
-
-	pbpr, ok := eiw.Entity.(*pbinternal.PullRequest)
-	require.True(t, ok)
-	assert.Equal(t, pullRequestPropMap[ghprops.PullPropertyNumber].(int64), pbpr.Number)
-}
-
 type handlerBuilder func(
 	evt interfaces.Publisher,
 	store db.Store,
@@ -205,17 +172,6 @@ func refreshByIDHandlerBuilder(
 	return NewRefreshByIDAndEvaluateHandler(evt, store, propSvc, provMgr)
 }
 
-func addOriginatingEntityHandlerBuilder(
-	evt interfaces.Publisher,
-	store db.Store,
-	propSvc service.PropertiesService,
-	provMgr manager.ProviderManager,
-) interfaces.Consumer {
-	// Create a nil entityCreator for testing - the tests focus on other handlers
-	// and addOriginatingEntityHandler tests would need separate setup
-	return NewAddOriginatingEntityHandler(evt, store, propSvc, provMgr, nil)
-}
-
 func removeOriginatingEntityHandlerBuilder(
 	evt interfaces.Publisher,
 	store db.Store,
@@ -548,80 +504,80 @@ func TestRefreshEntityAndDoHandler_HandleRefreshEntityAndEval(t *testing.T) {
 		// The test was testing internal implementation details that have been refactored
 		// New tests for addOriginatingEntityHandler should be written that properly mock EntityCreator
 		/*
-		{
-			name:             "NewAddOriginatingEntityHandler: Adding a pull request originating entity publishes",
-			handlerBuilderFn: addOriginatingEntityHandlerBuilder,
-			messageBuilder: func() *message.HandleEntityAndDoMessage {
-				prProps := properties.NewProperties(map[string]any{
-					properties.PropertyUpstreamID: "789",
-					ghprops.PullPropertyNumber:    int64(789),
-				})
-				originatorProps := properties.NewProperties(map[string]any{
-					properties.PropertyUpstreamID: "123",
-				})
-
-				return message.NewEntityRefreshAndDoMessage().
-					WithEntity(minderv1.Entity_ENTITY_PULL_REQUESTS, prProps).
-					WithOriginator(minderv1.Entity_ENTITY_REPOSITORIES, originatorProps).
-					WithProviderImplementsHint("github")
-			},
-			setupPropSvcMocks: func() fixtures.MockPropertyServiceBuilder {
-				pullEwp := buildEwp(t, pullRequestEwp, pullRequestPropMap)
-				pullProtoEnt, err := ghprops.PullRequestV1FromProperties(pullEwp.Properties)
-				require.NoError(t, err)
-
-				repoPropsEwp := buildEwp(t, repoEwp, pullRequestPropMap)
-
-				return fixtures.NewMockPropertiesService(
-					fixtures.WithSuccessfulEntityByUpstreamHint(repoPropsEwp, githubHint),
-					fixtures.WithSuccessfulEntityWithPropertiesAsProto(pullProtoEnt),
-					fixtures.WithSuccessfulSaveAllProperties(),
-				)
-			},
-			mockStoreFunc: df.NewMockStore(
-				df.WithTransaction(),
-				df.WithSuccessfulUpsertPullRequestWithParams(
-
-					db.EntityInstance{
-						ID:         pullRequestID,
-						EntityType: db.EntitiesPullRequest,
-						Name:       "",
-						ProjectID:  projectID,
-						ProviderID: providerID,
-						OriginatedFrom: uuid.NullUUID{
-							UUID:  repoID,
-							Valid: true,
+			{
+				name:             "NewAddOriginatingEntityHandler: Adding a pull request originating entity publishes",
+				handlerBuilderFn: addOriginatingEntityHandlerBuilder,
+				messageBuilder: func() *message.HandleEntityAndDoMessage {
+					prProps := properties.NewProperties(map[string]any{
+						properties.PropertyUpstreamID: "789",
+						ghprops.PullPropertyNumber:    int64(789),
+					})
+					originatorProps := properties.NewProperties(map[string]any{
+						properties.PropertyUpstreamID: "123",
+					})
+
+					return message.NewEntityRefreshAndDoMessage().
+						WithEntity(minderv1.Entity_ENTITY_PULL_REQUESTS, prProps).
+						WithOriginator(minderv1.Entity_ENTITY_REPOSITORIES, originatorProps).
+						WithProviderImplementsHint("github")
+				},
+				setupPropSvcMocks: func() fixtures.MockPropertyServiceBuilder {
+					pullEwp := buildEwp(t, pullRequestEwp, pullRequestPropMap)
+					pullProtoEnt, err := ghprops.PullRequestV1FromProperties(pullEwp.Properties)
+					require.NoError(t, err)
+
+					repoPropsEwp := buildEwp(t, repoEwp, pullRequestPropMap)
+
+					return fixtures.NewMockPropertiesService(
+						fixtures.WithSuccessfulEntityByUpstreamHint(repoPropsEwp, githubHint),
+						fixtures.WithSuccessfulEntityWithPropertiesAsProto(pullProtoEnt),
+						fixtures.WithSuccessfulSaveAllProperties(),
+					)
+				},
+				mockStoreFunc: df.NewMockStore(
+					df.WithTransaction(),
+					df.WithSuccessfulUpsertPullRequestWithParams(
+
+						db.EntityInstance{
+							ID:         pullRequestID,
+							EntityType: db.EntitiesPullRequest,
+							Name:       "",
+							ProjectID:  projectID,
+							ProviderID: providerID,
+							OriginatedFrom: uuid.NullUUID{
+								UUID:  repoID,
+								Valid: true,
+							},
 						},
-					},
-					db.CreateOrEnsureEntityByIDParams{
-						ID:         uuid.New(),
-						EntityType: db.EntitiesPullRequest,
-						Name:       pullName,
-						ProjectID:  projectID,
-						ProviderID: providerID,
-						OriginatedFrom: uuid.NullUUID{
-							UUID:  repoID,
-							Valid: true,
+						db.CreateOrEnsureEntityByIDParams{
+							ID:         uuid.New(),
+							EntityType: db.EntitiesPullRequest,
+							Name:       pullName,
+							ProjectID:  projectID,
+							ProviderID: providerID,
+							OriginatedFrom: uuid.NullUUID{
+								UUID:  repoID,
+								Valid: true,
+							},
 						},
-					},
+					),
 				),
-			),
-			providerSetup: newProviderMock(
-				withSuccessfulGetEntityName(pullName),
-				withSuccessfulFetchAllProperties(getPullRequestProperties()),
-				WithSuccessfulPropertiesToProtoMessage(&pbinternal.PullRequest{
-					Number: 789,
-				}),
-			),
-			providerManagerSetup: func(prov provifv1.Provider) provManFixtures.ProviderManagerMockBuilder {
-				return provManFixtures.NewProviderManagerMock(
-					provManFixtures.WithSuccessfulInstantiateFromID(prov),
-				)
+				providerSetup: newProviderMock(
+					withSuccessfulGetEntityName(pullName),
+					withSuccessfulFetchAllProperties(getPullRequestProperties()),
+					WithSuccessfulPropertiesToProtoMessage(&pbinternal.PullRequest{
+						Number: 789,
+					}),
+				),
+				providerManagerSetup: func(prov provifv1.Provider) provManFixtures.ProviderManagerMockBuilder {
+					return provManFixtures.NewProviderManagerMock(
+						provManFixtures.WithSuccessfulInstantiateFromID(prov),
+					)
+				},
+				expectedPublish: true,
+				topic:           constants.TopicQueueEntityEvaluate,
+				checkWmMsg:      checkPullRequestMessage,
 			},
-			expectedPublish: true,
-			topic:           constants.TopicQueueEntityEvaluate,
-			checkWmMsg:      checkPullRequestMessage,
-		},
 		*/
 		{
 			name:             "NewRemoveOriginatingEntityHandler: Happy path does not publish",

internal/entities/handlers/strategies/entity/add_originating_entity.go

@@ -7,17 +7,13 @@ import (
 	"context"
 	"fmt"
 
-	"github.com/google/uuid"
-	"google.golang.org/protobuf/reflect/protoreflect"
-
 	"github.com/mindersec/minder/internal/db"
 	"github.com/mindersec/minder/internal/entities/handlers/message"
 	"github.com/mindersec/minder/internal/entities/handlers/strategies"
 	"github.com/mindersec/minder/internal/entities/models"
 	propertyService "github.com/mindersec/minder/internal/entities/properties/service"
 	entityService "github.com/mindersec/minder/internal/entities/service"
 	"github.com/mindersec/minder/internal/providers/manager"
-	minderv1 "github.com/mindersec/minder/pkg/api/protobuf/go/minder/v1"
 	"github.com/mindersec/minder/pkg/entities/properties"
 )
 
@@ -84,14 +80,3 @@ func (a *addOriginatingEntityStrategy) GetEntity(
 func (*addOriginatingEntityStrategy) GetName() string {
 	return "addOriginatingEntityStrategy"
 }
-
-func (*addOriginatingEntityStrategy) upsertLegacyEntity(
-	_ context.Context,
-	_ minderv1.Entity,
-	_ *models.EntityWithProperties, _ protoreflect.ProtoMessage,
-	_ db.ExtendQuerier,
-) (uuid.UUID, error) {
-	// Legacy entity writes have been removed as part of Phase 1 of the legacy table removal plan.
-	// All entities are now written only to entity_instances and properties tables.
-	return uuid.Nil, nil
-}