diff --git a/docs/PLUGIN_ARCHITECTURE.md b/docs/PLUGIN_ARCHITECTURE.md index 4b0b624ea..35ec65c06 100644 --- a/docs/PLUGIN_ARCHITECTURE.md +++ b/docs/PLUGIN_ARCHITECTURE.md @@ -81,7 +81,7 @@ All complex data (configurations, pipeline context, step outputs, service invoca Conversion functions in `plugin/external/convert.go` handle the mapping between Go `map[string]any` and `protobuf.Struct`: ```go -func mapToStruct(m map[string]any) *structpb.Struct +func mapToStruct(m map[string]any) (*structpb.Struct, error) func structToMap(s *structpb.Struct) map[string]any ``` diff --git a/plugin/external/sdk/grpc_server.go b/plugin/external/sdk/grpc_server.go index 1c0bd8235..f6f5fa92f 100644 --- a/plugin/external/sdk/grpc_server.go +++ b/plugin/external/sdk/grpc_server.go @@ -436,8 +436,12 @@ func (s *grpcServer) ExecuteStep(ctx context.Context, req *pb.ExecuteStepRequest return &pb.ExecuteStepResponse{Error: err.Error()}, nil //nolint:nilerr // app error in response field } + output, encErr := mapToStruct(result.Output) + if encErr != nil { + return &pb.ExecuteStepResponse{Error: fmt.Sprintf("encode step output: %v", encErr)}, nil //nolint:nilerr // app error in response field + } return &pb.ExecuteStepResponse{ - Output: mapToStruct(result.Output), + Output: output, StopPipeline: result.StopPipeline, }, nil } @@ -510,7 +514,11 @@ func (s *grpcServer) InvokeService(ctx context.Context, req *pb.InvokeServiceReq } return &pb.InvokeServiceResponse{Error: err.Error()}, nil //nolint:nilerr // app error in response field } - return &pb.InvokeServiceResponse{Result: mapToStruct(result)}, nil + resultStruct, encErr := mapToStruct(result) + if encErr != nil { + return &pb.InvokeServiceResponse{Error: fmt.Sprintf("encode service result: %v", encErr)}, nil //nolint:nilerr // app error in response field + } + return &pb.InvokeServiceResponse{Result: resultStruct}, nil } invoker, ok := inst.(ServiceInvoker) @@ -527,7 +535,11 @@ func (s *grpcServer) InvokeService(ctx context.Context, req *pb.InvokeServiceReq } return &pb.InvokeServiceResponse{Error: err.Error()}, nil //nolint:nilerr // app error in response field } - return &pb.InvokeServiceResponse{Result: mapToStruct(result)}, nil + resultStruct, encErr := mapToStruct(result) + if encErr != nil { + return &pb.InvokeServiceResponse{Error: fmt.Sprintf("encode service result: %v", encErr)}, nil //nolint:nilerr // app error in response field + } + return &pb.InvokeServiceResponse{Result: resultStruct}, nil } type grpcStatusError interface { @@ -572,10 +584,13 @@ func structToMap(s *structpb.Struct) map[string]any { return s.AsMap() } -func mapToStruct(m map[string]any) *structpb.Struct { +func mapToStruct(m map[string]any) (*structpb.Struct, error) { if m == nil { - return nil + return nil, nil + } + s, err := structpb.NewStruct(m) + if err != nil { + return nil, fmt.Errorf("structpb.NewStruct: %w", err) } - s, _ := structpb.NewStruct(m) - return s + return s, nil } diff --git a/plugin/external/sdk/grpc_server_test.go b/plugin/external/sdk/grpc_server_test.go index 3647ddc18..9513e0ab5 100644 --- a/plugin/external/sdk/grpc_server_test.go +++ b/plugin/external/sdk/grpc_server_test.go @@ -15,11 +15,23 @@ import ( "google.golang.org/protobuf/proto" "google.golang.org/protobuf/types/known/anypb" "google.golang.org/protobuf/types/known/emptypb" + "google.golang.org/protobuf/types/known/structpb" "google.golang.org/protobuf/types/known/wrapperspb" ) // --- minimal test providers --- +// mustMapToStruct is a test helper wrapping mapToStruct; it fails the test if +// the map contains a structpb-incompatible value (e.g. chan, func). +func mustMapToStruct(t *testing.T, m map[string]any) *structpb.Struct { + t.Helper() + s, err := mapToStruct(m) + if err != nil { + t.Fatalf("mustMapToStruct: %v", err) + } + return s +} + type minimalProvider struct{} func (p *minimalProvider) Manifest() PluginManifest { @@ -408,7 +420,7 @@ func TestInvokeService_WithTypedModuleFactoryForwardsLegacyInvoker(t *testing.T) resp, err := srv.InvokeService(context.Background(), &pb.InvokeServiceRequest{ HandleId: createResp.HandleId, Method: "Echo", - Args: mapToStruct(map[string]any{ + Args: mustMapToStruct(t, map[string]any{ "value": "legacy-input", }), }) @@ -443,7 +455,7 @@ func TestInvokeService_ForwardsContextToLegacyInvoker(t *testing.T) { resp, err := srv.InvokeService(ctx, &pb.InvokeServiceRequest{ HandleId: createResp.HandleId, Method: "Echo", - Args: mapToStruct(map[string]any{ + Args: mustMapToStruct(t, map[string]any{ "value": "legacy-input", }), }) @@ -476,7 +488,7 @@ func TestInvokeService_PreservesStatusErrors(t *testing.T) { resp, err := srv.InvokeService(context.Background(), &pb.InvokeServiceRequest{ HandleId: createResp.HandleId, Method: "IaCProvider.RepairDirtyMigration", - Args: mapToStruct(map[string]any{}), + Args: mustMapToStruct(t, map[string]any{}), }) if status.Code(err) != codes.Unimplemented { t.Fatalf("InvokeService error code = %v, want Unimplemented (resp=%+v, err=%v)", status.Code(err), resp, err) @@ -511,6 +523,67 @@ func TestInvokeService_WithTypedInputRequiresTypedInvoker(t *testing.T) { } } +// TestMapToStruct_SDK_PropagatesError verifies that the SDK's local mapToStruct +// surfaces structpb.NewStruct errors instead of silently dropping data +// (workflow#537 — mirrors the same test in plugin/external/convert_test.go). +func TestMapToStruct_SDK_PropagatesError(t *testing.T) { + m := map[string]any{ + "ok": "value", + "bad": make(chan int), // chan is not structpb-representable + } + s, err := mapToStruct(m) + if err == nil { + t.Fatal("expected error from structpb.NewStruct on chan, got nil") + } + if s != nil { + t.Errorf("expected nil struct on error, got %v", s) + } +} + +// TestInvokeService_PropagatesOutputEncodingError verifies that InvokeService +// surfaces a structpb-encoding failure in the Response.Error field instead of +// silently returning an empty result (workflow#537). +func TestInvokeService_PropagatesOutputEncodingError(t *testing.T) { + // Module whose InvokeMethod returns a value that structpb cannot encode. + badModule := &badOutputModule{} + srv := newGRPCServer(&typedServiceProvider{module: badModule}) + + createResp, err := srv.CreateModule(context.Background(), &pb.CreateModuleRequest{ + Type: "typed.service", + Name: "svc", + }) + if err != nil { + t.Fatalf("CreateModule rpc error: %v", err) + } + if createResp.Error != "" { + t.Fatalf("unexpected CreateModule application error: %s", createResp.Error) + } + if createResp.HandleId == "" { + t.Fatal("CreateModule returned empty HandleId") + } + + resp, err := srv.InvokeService(context.Background(), &pb.InvokeServiceRequest{ + HandleId: createResp.HandleId, + Method: "BadOutput", + }) + if err != nil { + t.Fatalf("InvokeService returned unexpected rpc error: %v", err) + } + if resp.Error == "" { + t.Fatal("expected encoding error in Response.Error, got empty string") + } +} + +// badOutputModule returns a map with a chan value which structpb cannot encode. +type badOutputModule struct{} + +func (badOutputModule) Init() error { return nil } +func (badOutputModule) Start(context.Context) error { return nil } +func (badOutputModule) Stop(context.Context) error { return nil } +func (badOutputModule) InvokeMethod(_ string, _ map[string]any) (map[string]any, error) { + return map[string]any{"bad": make(chan int)}, nil +} + func mustPackGRPCTestMessage(t *testing.T, msg proto.Message) *anypb.Any { t.Helper() typed, err := anypb.New(msg)