fix(plugin/external): propagate plugin errors instead of swallowing as nil

cmd/wfctl/deploy_providers.go

@@ -19,6 +19,7 @@ import (
 
 	"github.com/GoCodeAlone/workflow/config"
 	"github.com/GoCodeAlone/workflow/interfaces"
+	"github.com/GoCodeAlone/workflow/plugin"
 	"github.com/GoCodeAlone/workflow/plugin/external"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
@@ -120,6 +121,31 @@ func findIaCPluginDir(pluginDir, providerName string) (name string, hasBinary bo
 	return "", false, nil
 }
 
+// loadIaCPlugin finds and loads the IaC plugin for the given provider, returning
+// its name, module factories, and a closer that shuts down the plugin subprocess.
+// Tests replace this var to inject fakes without touching the filesystem.
+var loadIaCPlugin = defaultLoadIaCPlugin
+
+func defaultLoadIaCPlugin(pluginDir, providerName string) (pluginName string, factories map[string]plugin.ModuleFactory, closer io.Closer, err error) {
+	pName, hasBinary, findErr := findIaCPluginDir(pluginDir, providerName)
+	if findErr != nil {
+		return "", nil, nil, fmt.Errorf("resolve IaC provider %q: %w", providerName, findErr)
+	}
+	if pName == "" {
+		return "", nil, nil, fmt.Errorf("no plugin found for IaC provider %q in %s — run: wfctl plugin install <plugin-name>", providerName, pluginDir)
+	}
+	if !hasBinary {
+		return "", nil, nil, fmt.Errorf("plugin %q declares provider %q but binary is missing — run: wfctl plugin install %s", pName, providerName, pName)
+	}
+	mgr := external.NewExternalPluginManager(pluginDir, nil)
+	adapter, loadErr := mgr.LoadPlugin(pName)
+	if loadErr != nil {
+		mgr.Shutdown()
+		return "", nil, nil, fmt.Errorf("load plugin %q for provider %q: %w", pName, providerName, loadErr)
+	}
+	return pName, adapter.ModuleFactories(), closerFunc(func() error { mgr.Shutdown(); return nil }), nil
+}
+
 // discoverAndLoadIaCProvider implements the default resolveIaCProvider: it scans
 // the plugin directory for a plugin that declares iacProvider.name == providerName,
 // loads it via ExternalPluginManager, and returns the IaCProvider plus a Closer
@@ -130,53 +156,41 @@ func discoverAndLoadIaCProvider(ctx context.Context, providerName string, cfg ma
 		pluginDir = "./data/plugins"
 	}
 
-	pluginName, hasBinary, err := findIaCPluginDir(pluginDir, providerName)
+	pluginName, factories, closer, err := loadIaCPlugin(pluginDir, providerName)
 	if err != nil {
-		return nil, nil, fmt.Errorf("resolve IaC provider %q: %w", providerName, err)
-	}
-	if pluginName == "" {
-		return nil, nil, fmt.Errorf("no plugin found for IaC provider %q in %s — run: wfctl plugin install <plugin-name>", providerName, pluginDir)
-	}
-	if !hasBinary {
-		return nil, nil, fmt.Errorf("plugin %q declares provider %q but binary is missing — run: wfctl plugin install %s", pluginName, providerName, pluginName)
-	}
-
-	mgr := external.NewExternalPluginManager(pluginDir, nil)
-	closer := closerFunc(func() error { mgr.Shutdown(); return nil })
-
-	adapter, loadErr := mgr.LoadPlugin(pluginName)
-	if loadErr != nil {
-		mgr.Shutdown()
-		return nil, nil, fmt.Errorf("load plugin %q for provider %q: %w", pluginName, providerName, loadErr)
+		return nil, nil, err
 	}
 
-	factories := adapter.ModuleFactories()
 	factory, ok := factories["iac.provider"]
 	if !ok {
-		mgr.Shutdown()
+		_ = closer.Close()
 		return nil, nil, fmt.Errorf("plugin %q does not expose an iac.provider module type — upgrade with: wfctl plugin update %s", pluginName, pluginName)
 	}
 
 	mod := factory("iac-provider", cfg)
+	if pluginErr := external.AsModuleError(mod); pluginErr != nil {
+		_ = closer.Close()
+		return nil, nil, fmt.Errorf("plugin %q iac.provider factory failed: %w", pluginName, pluginErr)
+	}
 	if mod == nil {
-		mgr.Shutdown()
-		return nil, nil, fmt.Errorf("plugin %q iac.provider factory returned nil", pluginName)
+		_ = closer.Close()
+		return nil, nil, fmt.Errorf("plugin %q iac.provider factory returned nil (unexpected — file an issue)", pluginName)
 	}
 
 	// RemoteModule does not directly implement interfaces.IaCProvider; instead it
 	// exposes InvokeService for cross-process method dispatch. Wrap it in a
 	// remoteIaCProvider that routes each IaCProvider call through InvokeService.
 	invoker, ok := mod.(remoteServiceInvoker)
 	if !ok {
-		mgr.Shutdown()
+		_ = closer.Close()
 		return nil, nil, fmt.Errorf("plugin %q iac.provider module (%T) does not support service invocation — upgrade with: wfctl plugin update %s", pluginName, mod, pluginName)
 	}
 
 	iacProvider := &remoteIaCProvider{invoker: invoker}
 	// Notify the plugin that Initialize has been called (the plugin may treat
 	// this as a no-op if it already ran Initialize inside CreateModule).
 	if initErr := iacProvider.Initialize(ctx, cfg); initErr != nil {
-		mgr.Shutdown()
+		_ = closer.Close()
 		return nil, nil, fmt.Errorf("initialize provider %q: %w", providerName, initErr)
 	}
 	return iacProvider, closer, nil

cmd/wfctl/deploy_providers_test.go

@@ -2,13 +2,52 @@ package main
 
 import (
 	"context"
+	"errors"
+	"io"
 	"os"
 	"strings"
 	"testing"
 
+	"github.com/GoCodeAlone/modular"
 	"github.com/GoCodeAlone/workflow/config"
+	"github.com/GoCodeAlone/workflow/plugin"
+	"github.com/GoCodeAlone/workflow/plugin/external"
 )
 
+// noopCloser is an io.Closer that does nothing — used in test stubs.
+type noopCloser struct{}
+
+func (noopCloser) Close() error { return nil }
+
+// ── discoverAndLoadIaCProvider — error propagation ────────────────────────────
+
+// TestResolveIaCProviderSurfacesPluginError is the regression gate for the
+// caller-side fix: discoverAndLoadIaCProvider must surface the error message
+// from an *errorModule returned by the factory, not fall through to a generic
+// "does not support service invocation" message.
+func TestResolveIaCProviderSurfacesPluginError(t *testing.T) {
+	const pluginErrMsg = "digitalocean: missing required config key 'token'"
+
+	oldLoader := loadIaCPlugin
+	loadIaCPlugin = func(_, _ string) (string, map[string]plugin.ModuleFactory, io.Closer, error) {
+		factory := func(name string, _ map[string]any) modular.Module {
+			return external.NewErrorModule(name, errors.New(pluginErrMsg))
+		}
+		return "workflow-plugin-digitalocean", map[string]plugin.ModuleFactory{
+			"iac.provider": factory,
+		}, noopCloser{}, nil
+	}
+	defer func() { loadIaCPlugin = oldLoader }()
+
+	_, _, err := discoverAndLoadIaCProvider(context.Background(), "digitalocean", map[string]any{})
+	if err == nil {
+		t.Fatal("expected error, got nil")
+	}
+	if !strings.Contains(err.Error(), pluginErrMsg) {
+		t.Errorf("expected plugin error message %q in returned error, got: %v", pluginErrMsg, err)
+	}
+}
+
 // ── newDeployProvider ─────────────────────────────────────────────────────────
 
 func TestNewDeployProvider_Kubernetes(t *testing.T) {

plugin/external/adapter.go

@@ -61,6 +61,24 @@ func (m *errorModule) Init(modular.Application) error { return m.err }
 func (m *errorModule) Start(context.Context) error    { return m.err }
 func (m *errorModule) Stop(context.Context) error     { return nil }
 
+// AsModuleError returns the wrapped error if m was produced by a failed
+// CreateModule call (i.e. the factory returned an *errorModule), or nil if m
+// is a successfully-created module. Callers outside this package use this to
+// surface plugin-reported errors without depending on the unexported type.
+func AsModuleError(m modular.Module) error {
+	if em, ok := m.(*errorModule); ok {
+		return em.err
+	}
+	return nil
+}
+
+// NewErrorModule returns a Module that surfaces err from Init and Start.
+// Exported for use in tests that need to simulate a plugin factory failure
+// without importing the unexported errorModule type directly.
+func NewErrorModule(name string, err error) modular.Module {
+	return &errorModule{name: name, err: err}
+}
+
 // NewExternalPluginAdapter creates an adapter from a connected plugin client.
 func NewExternalPluginAdapter(name string, client *PluginClient) (*ExternalPluginAdapter, error) {
 	ctx := context.Background()
@@ -326,8 +344,11 @@ func (a *ExternalPluginAdapter) ModuleFactories() map[string]plugin.ModuleFactor
 				Config:      config,
 				TypedConfig: typedConfig,
 			})
-			if createErr != nil || createResp.Error != "" {
-				return nil
+			if createErr != nil {
+				return &errorModule{name: name, err: fmt.Errorf("create remote module %s: %w", tn, createErr)}
+			}
+			if createResp.Error != "" {
+				return &errorModule{name: name, err: fmt.Errorf("create remote module %s: plugin reported: %s", tn, createResp.Error)}
 			}
 			remote := NewRemoteModule(name, createResp.HandleId, a.client.client, remoteModuleContracts{
 				module:   a.contracts.module(tn),

plugin/external/adapter_test.go

@@ -212,6 +212,59 @@ func TestContractRegistry_UnimplementedUsesEmptyRegistry(t *testing.T) {
 	}
 }
 
+// errorOnCreateModuleClient overrides CreateModule to return a plugin-reported
+// error in the response Error field (not as a gRPC error).
+type errorOnCreateModuleClient struct {
+	adapterTestPluginServiceClient
+	createModuleError string
+}
+
+func (c *errorOnCreateModuleClient) CreateModule(_ context.Context, req *pb.CreateModuleRequest, _ ...grpc.CallOption) (*pb.HandleResponse, error) {
+	c.lastCreateModReq = req
+	return &pb.HandleResponse{Error: c.createModuleError}, nil
+}
+
+// TestModuleFactoriesPropagatesPluginError is a regression gate for the class
+// invariant: when CreateModule returns a non-empty Error field, ModuleFactories
+// must return an *errorModule wrapping the plugin's message — not bare nil.
+// Previously the condition `if createErr != nil || createResp.Error != ""` fell
+// through to `return nil`, silently discarding the plugin diagnostic.
+func TestModuleFactoriesPropagatesPluginError(t *testing.T) {
+	const pluginErrMsg = "digitalocean: missing required config key 'token'"
+	client := &errorOnCreateModuleClient{
+		adapterTestPluginServiceClient: adapterTestPluginServiceClient{
+			manifest:    &pb.Manifest{Name: "test-plugin"},
+			moduleTypes: []string{"iac.provider"},
+		},
+		createModuleError: pluginErrMsg,
+	}
+	a, err := NewExternalPluginAdapter("test-plugin", &PluginClient{client: client})
+	if err != nil {
+		t.Fatalf("NewExternalPluginAdapter: %v", err)
+	}
+
+	factories := a.ModuleFactories()
+	factory, ok := factories["iac.provider"]
+	if !ok {
+		t.Fatal("expected iac.provider factory to be registered")
+	}
+
+	mod := factory("test-provider", map[string]any{})
+	if mod == nil {
+		t.Fatal("expected *errorModule, got nil — plugin error was swallowed")
+	}
+	errMod, ok := mod.(*errorModule)
+	if !ok {
+		t.Fatalf("expected *errorModule, got %T", mod)
+	}
+	if errMod.err == nil {
+		t.Fatal("errorModule has nil err")
+	}
+	if !strings.Contains(errMod.err.Error(), pluginErrMsg) {
+		t.Errorf("expected plugin error message %q in propagated error, got: %v", pluginErrMsg, errMod.err)
+	}
+}
+
 func TestExternalPluginAdapter_ServiceContractsAttachByModuleType(t *testing.T) {
 	registry := &pb.ContractRegistry{
 		Contracts: []*pb.ContractDescriptor{