From c8daea4f9265d4befa9d66ca9857a0b2afbec225 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 4 Mar 2026 17:36:27 +0000 Subject: [PATCH 1/4] Initial plan From 8e881102be715c1130e8b6eeec6e1ca7378da5f3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 4 Mar 2026 17:44:50 +0000 Subject: [PATCH 2/4] Add LoadPluginWithOverride to PluginLoader and StdEngine Allow external plugins to override built-in step, module, trigger, handler, deploy target, and sidecar provider types. When a duplicate type is encountered via LoadPluginWithOverride, a warning is logged instead of returning an error. The original LoadPlugin behavior (rejecting duplicates) is preserved. Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> --- engine.go | 22 ++++++- plugin/loader.go | 56 ++++++++++++++---- plugin/loader_test.go | 134 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 199 insertions(+), 13 deletions(-) diff --git a/engine.go b/engine.go index 45fed7d5..6c3d52f3 100644 --- a/engine.go +++ b/engine.go @@ -212,8 +212,28 @@ func (e *StdEngine) SetPluginLoader(loader *plugin.PluginLoader) { // LoadPlugin loads an EnginePlugin into the engine. func (e *StdEngine) LoadPlugin(p plugin.EnginePlugin) error { + return e.loadPluginInternal(p, false) +} + +// LoadPluginWithOverride loads an EnginePlugin into the engine, allowing it to +// override existing module, step, trigger, and handler type registrations. +// When a duplicate type is encountered, the new factory silently replaces the +// previous one (with a warning log) instead of returning an error. +// This is intended for external plugins that intentionally replace built-in +// defaults (e.g., replacing a mock step with a production implementation). +func (e *StdEngine) LoadPluginWithOverride(p plugin.EnginePlugin) error { + return e.loadPluginInternal(p, true) +} + +func (e *StdEngine) loadPluginInternal(p plugin.EnginePlugin, allowOverride bool) error { loader := e.PluginLoader() - if err := loader.LoadPlugin(p); err != nil { + var err error + if allowOverride { + err = loader.LoadPluginWithOverride(p) + } else { + err = loader.LoadPlugin(p) + } + if err != nil { return fmt.Errorf("load plugin: %w", err) } for typeName, factory := range p.ModuleFactories() { diff --git a/plugin/loader.go b/plugin/loader.go index 2f3ebd0c..8a29420d 100644 --- a/plugin/loader.go +++ b/plugin/loader.go @@ -106,6 +106,20 @@ func (l *PluginLoader) ValidateTier(manifest *PluginManifest) error { // schemas, and wiring hooks. Returns an error if any factory type conflicts with // an existing registration. func (l *PluginLoader) LoadPlugin(p EnginePlugin) error { + return l.loadPlugin(p, false) +} + +// LoadPluginWithOverride is like LoadPlugin but allows the plugin to override +// existing module, step, trigger, handler, deploy target, and sidecar provider +// registrations. When a duplicate type is encountered, the new factory replaces +// the previous one and a warning is logged instead of returning an error. +// This is intended for external plugins that intentionally replace built-in +// defaults (e.g., replacing a mock authz step with a production implementation). +func (l *PluginLoader) LoadPluginWithOverride(p EnginePlugin) error { + return l.loadPlugin(p, true) +} + +func (l *PluginLoader) loadPlugin(p EnginePlugin, allowOverride bool) error { manifest := p.EngineManifest() if err := manifest.Validate(); err != nil { return fmt.Errorf("plugin %q: %w", manifest.Name, err) @@ -132,34 +146,46 @@ func (l *PluginLoader) LoadPlugin(p EnginePlugin) error { } } - // Register module factories — conflict on duplicate type. + // Register module factories — conflict on duplicate type unless override allowed. for typeName, factory := range p.ModuleFactories() { if _, exists := l.moduleFactories[typeName]; exists { - return fmt.Errorf("plugin %q: module type %q already registered", manifest.Name, typeName) + if !allowOverride { + return fmt.Errorf("plugin %q: module type %q already registered", manifest.Name, typeName) + } + slog.Warn("plugin overriding existing module type", "plugin", manifest.Name, "type", typeName) } l.moduleFactories[typeName] = factory } - // Register step factories — conflict on duplicate type. + // Register step factories — conflict on duplicate type unless override allowed. for typeName, factory := range p.StepFactories() { if _, exists := l.stepFactories[typeName]; exists { - return fmt.Errorf("plugin %q: step type %q already registered", manifest.Name, typeName) + if !allowOverride { + return fmt.Errorf("plugin %q: step type %q already registered", manifest.Name, typeName) + } + slog.Warn("plugin overriding existing step type", "plugin", manifest.Name, "type", typeName) } l.stepFactories[typeName] = factory } - // Register trigger factories — conflict on duplicate type. + // Register trigger factories — conflict on duplicate type unless override allowed. for typeName, factory := range p.TriggerFactories() { if _, exists := l.triggerFactories[typeName]; exists { - return fmt.Errorf("plugin %q: trigger type %q already registered", manifest.Name, typeName) + if !allowOverride { + return fmt.Errorf("plugin %q: trigger type %q already registered", manifest.Name, typeName) + } + slog.Warn("plugin overriding existing trigger type", "plugin", manifest.Name, "type", typeName) } l.triggerFactories[typeName] = factory } - // Register workflow handler factories — conflict on duplicate type. + // Register workflow handler factories — conflict on duplicate type unless override allowed. for typeName, factory := range p.WorkflowHandlers() { if _, exists := l.handlerFactories[typeName]; exists { - return fmt.Errorf("plugin %q: workflow handler type %q already registered", manifest.Name, typeName) + if !allowOverride { + return fmt.Errorf("plugin %q: workflow handler type %q already registered", manifest.Name, typeName) + } + slog.Warn("plugin overriding existing workflow handler type", "plugin", manifest.Name, "type", typeName) } l.handlerFactories[typeName] = factory } @@ -175,18 +201,24 @@ func (l *PluginLoader) LoadPlugin(p EnginePlugin) error { // Collect config transform hooks. l.configTransformHooks = append(l.configTransformHooks, p.ConfigTransformHooks()...) - // Register deploy targets — conflict on duplicate name. + // Register deploy targets — conflict on duplicate name unless override allowed. for name, target := range p.DeployTargets() { if _, exists := l.deployTargets[name]; exists { - return fmt.Errorf("plugin %q: deploy target %q already registered", manifest.Name, name) + if !allowOverride { + return fmt.Errorf("plugin %q: deploy target %q already registered", manifest.Name, name) + } + slog.Warn("plugin overriding existing deploy target", "plugin", manifest.Name, "target", name) } l.deployTargets[name] = target } - // Register sidecar providers — conflict on duplicate type. + // Register sidecar providers — conflict on duplicate type unless override allowed. for typeName, provider := range p.SidecarProviders() { if _, exists := l.sidecarProviders[typeName]; exists { - return fmt.Errorf("plugin %q: sidecar provider %q already registered", manifest.Name, typeName) + if !allowOverride { + return fmt.Errorf("plugin %q: sidecar provider %q already registered", manifest.Name, typeName) + } + slog.Warn("plugin overriding existing sidecar provider", "plugin", manifest.Name, "type", typeName) } l.sidecarProviders[typeName] = provider } diff --git a/plugin/loader_test.go b/plugin/loader_test.go index f467dae2..5074722a 100644 --- a/plugin/loader_test.go +++ b/plugin/loader_test.go @@ -166,6 +166,140 @@ func TestPluginLoader_DuplicateModuleTypeConflict(t *testing.T) { } } +func TestPluginLoader_LoadPluginWithOverride_ModuleType(t *testing.T) { + loader := newTestEngineLoader() + + p1 := &modulePlugin{ + BaseEnginePlugin: *makeEnginePlugin("builtin-plugin", "1.0.0", nil), + modules: map[string]ModuleFactory{ + "shared.module": func(name string, cfg map[string]any) modular.Module { + return nil + }, + }, + } + p2 := &modulePlugin{ + BaseEnginePlugin: *makeEnginePlugin("external-plugin", "1.0.0", nil), + modules: map[string]ModuleFactory{ + "shared.module": func(name string, cfg map[string]any) modular.Module { + return nil + }, + }, + } + + if err := loader.LoadPlugin(p1); err != nil { + t.Fatalf("first load should succeed: %v", err) + } + // LoadPlugin should still reject duplicates. + if err := loader.LoadPlugin(p2); err == nil { + t.Fatal("expected duplicate module type error from LoadPlugin") + } + // LoadPluginWithOverride should allow replacing the type. + if err := loader.LoadPluginWithOverride(p2); err != nil { + t.Fatalf("LoadPluginWithOverride should succeed: %v", err) + } + if got := len(loader.ModuleFactories()); got != 1 { + t.Errorf("expected 1 module factory after override, got %d", got) + } + if got := len(loader.LoadedPlugins()); got != 2 { + t.Errorf("expected 2 loaded plugins, got %d", got) + } +} + +func TestPluginLoader_LoadPluginWithOverride_StepType(t *testing.T) { + loader := newTestEngineLoader() + + p1 := &modulePlugin{ + BaseEnginePlugin: *makeEnginePlugin("builtin-steps", "1.0.0", nil), + steps: map[string]StepFactory{ + "step.authz_check": func(name string, cfg map[string]any, _ modular.Application) (any, error) { + return "builtin", nil + }, + }, + } + p2 := &modulePlugin{ + BaseEnginePlugin: *makeEnginePlugin("external-authz", "1.0.0", nil), + steps: map[string]StepFactory{ + "step.authz_check": func(name string, cfg map[string]any, _ modular.Application) (any, error) { + return "external", nil + }, + }, + } + + if err := loader.LoadPlugin(p1); err != nil { + t.Fatalf("first load should succeed: %v", err) + } + if err := loader.LoadPluginWithOverride(p2); err != nil { + t.Fatalf("LoadPluginWithOverride should succeed: %v", err) + } + + // Verify the override replaced the factory. + factories := loader.StepFactories() + if got := len(factories); got != 1 { + t.Fatalf("expected 1 step factory, got %d", got) + } + result, err := factories["step.authz_check"]("test", nil, nil) + if err != nil { + t.Fatalf("step factory returned error: %v", err) + } + if result != "external" { + t.Errorf("expected overridden factory to return %q, got %q", "external", result) + } +} + +func TestPluginLoader_LoadPluginWithOverride_AllTypes(t *testing.T) { + loader := newTestEngineLoader() + + p1 := &modulePlugin{ + BaseEnginePlugin: *makeEnginePlugin("builtin", "1.0.0", nil), + modules: map[string]ModuleFactory{ + "mod.type": func(name string, cfg map[string]any) modular.Module { return nil }, + }, + steps: map[string]StepFactory{ + "step.type": func(name string, cfg map[string]any, _ modular.Application) (any, error) { return nil, nil }, + }, + triggers: map[string]TriggerFactory{ + "trigger.type": func() any { return nil }, + }, + handlers: map[string]WorkflowHandlerFactory{ + "handler.type": func() any { return nil }, + }, + } + p2 := &modulePlugin{ + BaseEnginePlugin: *makeEnginePlugin("external", "1.0.0", nil), + modules: map[string]ModuleFactory{ + "mod.type": func(name string, cfg map[string]any) modular.Module { return nil }, + }, + steps: map[string]StepFactory{ + "step.type": func(name string, cfg map[string]any, _ modular.Application) (any, error) { return nil, nil }, + }, + triggers: map[string]TriggerFactory{ + "trigger.type": func() any { return nil }, + }, + handlers: map[string]WorkflowHandlerFactory{ + "handler.type": func() any { return nil }, + }, + } + + if err := loader.LoadPlugin(p1); err != nil { + t.Fatalf("first load should succeed: %v", err) + } + if err := loader.LoadPluginWithOverride(p2); err != nil { + t.Fatalf("LoadPluginWithOverride should succeed for all types: %v", err) + } + if got := len(loader.ModuleFactories()); got != 1 { + t.Errorf("expected 1 module factory, got %d", got) + } + if got := len(loader.StepFactories()); got != 1 { + t.Errorf("expected 1 step factory, got %d", got) + } + if got := len(loader.TriggerFactories()); got != 1 { + t.Errorf("expected 1 trigger factory, got %d", got) + } + if got := len(loader.WorkflowHandlerFactories()); got != 1 { + t.Errorf("expected 1 handler factory, got %d", got) + } +} + func TestPluginLoader_WiringHooksSortedByPriority(t *testing.T) { loader := newTestEngineLoader() From f0b7b5e77d411d2a2a737de348dde55e40b81706 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 4 Mar 2026 17:48:54 +0000 Subject: [PATCH 3/4] Fix contradictory comment in LoadPluginWithOverride doc Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> --- engine.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/engine.go b/engine.go index 6c3d52f3..406adda3 100644 --- a/engine.go +++ b/engine.go @@ -217,8 +217,8 @@ func (e *StdEngine) LoadPlugin(p plugin.EnginePlugin) error { // LoadPluginWithOverride loads an EnginePlugin into the engine, allowing it to // override existing module, step, trigger, and handler type registrations. -// When a duplicate type is encountered, the new factory silently replaces the -// previous one (with a warning log) instead of returning an error. +// When a duplicate type is encountered, the new factory replaces the previous +// one and a warning is logged, instead of returning an error. // This is intended for external plugins that intentionally replace built-in // defaults (e.g., replacing a mock step with a production implementation). func (e *StdEngine) LoadPluginWithOverride(p plugin.EnginePlugin) error { From 1a7e1181ab761eec7cc63ce3fa6210ff6c22b531 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 4 Mar 2026 20:53:04 +0000 Subject: [PATCH 4/4] Address review feedback: LoadBinaryPluginWithOverride, doc fix, extended tests - Add LoadBinaryPluginWithOverride to PluginLoader for override-capable binary plugin loading - Fix engine.go doc to mention deploy targets and sidecar providers - Add LoadPlugin duplicate rejection assertion in step override test - Extend AllTypes test to cover deploy targets and sidecar providers; add fullPlugin, mockDeployTarget, mockSidecarProvider test helpers Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> --- engine.go | 11 ++++--- plugin/loader.go | 18 +++++++++- plugin/loader_test.go | 76 +++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 97 insertions(+), 8 deletions(-) diff --git a/engine.go b/engine.go index 406adda3..eedab84c 100644 --- a/engine.go +++ b/engine.go @@ -216,11 +216,12 @@ func (e *StdEngine) LoadPlugin(p plugin.EnginePlugin) error { } // LoadPluginWithOverride loads an EnginePlugin into the engine, allowing it to -// override existing module, step, trigger, and handler type registrations. -// When a duplicate type is encountered, the new factory replaces the previous -// one and a warning is logged, instead of returning an error. -// This is intended for external plugins that intentionally replace built-in -// defaults (e.g., replacing a mock step with a production implementation). +// override existing module, step, trigger, handler, deploy target, and sidecar +// provider registrations. When a duplicate type is encountered, the new factory +// replaces the previous one and a warning is logged, instead of returning an +// error. This is intended for external plugins that intentionally replace +// built-in defaults (e.g., replacing a mock step with a production +// implementation, or swapping out deploy targets/sidecar providers). func (e *StdEngine) LoadPluginWithOverride(p plugin.EnginePlugin) error { return e.loadPluginInternal(p, true) } diff --git a/plugin/loader.go b/plugin/loader.go index 8a29420d..f2c9c9d0 100644 --- a/plugin/loader.go +++ b/plugin/loader.go @@ -76,7 +76,23 @@ func (l *PluginLoader) LoadBinaryPlugin(p EnginePlugin, binaryPath, sigPath, cer return l.LoadPlugin(p) } -// ValidateTier checks whether a plugin's tier is allowed given the current +// LoadBinaryPluginWithOverride is the override-capable counterpart to +// LoadBinaryPlugin. It verifies the plugin binary with cosign (for premium +// plugins) and then loads the plugin, allowing it to override existing module, +// step, trigger, handler, deploy target, and sidecar provider registrations. +// When a duplicate type is encountered, the new factory replaces the previous +// one and a warning is logged instead of returning an error. +func (l *PluginLoader) LoadBinaryPluginWithOverride(p EnginePlugin, binaryPath, sigPath, certPath string) error { + manifest := p.EngineManifest() + if manifest.Tier == TierPremium && l.cosignVerifier != nil { + if err := l.cosignVerifier.Verify(binaryPath, sigPath, certPath); err != nil { + return fmt.Errorf("plugin %q: binary verification failed: %w", manifest.Name, err) + } + } + return l.LoadPluginWithOverride(p) +} + + // license validator configuration: // - Core and Community plugins are always allowed. // - Premium plugins are validated against the LicenseValidator if one is set. diff --git a/plugin/loader_test.go b/plugin/loader_test.go index 5074722a..4c123d2e 100644 --- a/plugin/loader_test.go +++ b/plugin/loader_test.go @@ -1,11 +1,14 @@ package plugin import ( + "context" + "io" "testing" "github.com/CrisisTextLine/modular" "github.com/GoCodeAlone/workflow/capability" "github.com/GoCodeAlone/workflow/config" + "github.com/GoCodeAlone/workflow/deploy" "github.com/GoCodeAlone/workflow/schema" ) @@ -228,6 +231,10 @@ func TestPluginLoader_LoadPluginWithOverride_StepType(t *testing.T) { if err := loader.LoadPlugin(p1); err != nil { t.Fatalf("first load should succeed: %v", err) } + // LoadPlugin should still reject duplicate step types. + if err := loader.LoadPlugin(p2); err == nil { + t.Fatal("expected duplicate step type error from LoadPlugin") + } if err := loader.LoadPluginWithOverride(p2); err != nil { t.Fatalf("LoadPluginWithOverride should succeed: %v", err) } @@ -249,7 +256,7 @@ func TestPluginLoader_LoadPluginWithOverride_StepType(t *testing.T) { func TestPluginLoader_LoadPluginWithOverride_AllTypes(t *testing.T) { loader := newTestEngineLoader() - p1 := &modulePlugin{ + p1 := &fullPlugin{ BaseEnginePlugin: *makeEnginePlugin("builtin", "1.0.0", nil), modules: map[string]ModuleFactory{ "mod.type": func(name string, cfg map[string]any) modular.Module { return nil }, @@ -263,8 +270,10 @@ func TestPluginLoader_LoadPluginWithOverride_AllTypes(t *testing.T) { handlers: map[string]WorkflowHandlerFactory{ "handler.type": func() any { return nil }, }, + deployTargets: map[string]deploy.DeployTarget{"deploy.target": &mockDeployTarget{name: "builtin-target"}}, + sidecarProviders: map[string]deploy.SidecarProvider{"sidecar.type": &mockSidecarProvider{typeName: "builtin-sidecar"}}, } - p2 := &modulePlugin{ + p2 := &fullPlugin{ BaseEnginePlugin: *makeEnginePlugin("external", "1.0.0", nil), modules: map[string]ModuleFactory{ "mod.type": func(name string, cfg map[string]any) modular.Module { return nil }, @@ -278,11 +287,17 @@ func TestPluginLoader_LoadPluginWithOverride_AllTypes(t *testing.T) { handlers: map[string]WorkflowHandlerFactory{ "handler.type": func() any { return nil }, }, + deployTargets: map[string]deploy.DeployTarget{"deploy.target": &mockDeployTarget{name: "external-target"}}, + sidecarProviders: map[string]deploy.SidecarProvider{"sidecar.type": &mockSidecarProvider{typeName: "external-sidecar"}}, } if err := loader.LoadPlugin(p1); err != nil { t.Fatalf("first load should succeed: %v", err) } + // Verify LoadPlugin rejects all duplicate types. + if err := loader.LoadPlugin(p2); err == nil { + t.Fatal("expected duplicate type error from LoadPlugin") + } if err := loader.LoadPluginWithOverride(p2); err != nil { t.Fatalf("LoadPluginWithOverride should succeed for all types: %v", err) } @@ -298,6 +313,12 @@ func TestPluginLoader_LoadPluginWithOverride_AllTypes(t *testing.T) { if got := len(loader.WorkflowHandlerFactories()); got != 1 { t.Errorf("expected 1 handler factory, got %d", got) } + if got := len(loader.DeployTargets()); got != 1 { + t.Errorf("expected 1 deploy target, got %d", got) + } + if got := len(loader.SidecarProviders()); got != 1 { + t.Errorf("expected 1 sidecar provider, got %d", got) + } } func TestPluginLoader_WiringHooksSortedByPriority(t *testing.T) { @@ -369,3 +390,54 @@ type hookPlugin struct { } func (p *hookPlugin) WiringHooks() []WiringHook { return p.hooks } + +// fullPlugin embeds BaseEnginePlugin and overrides all factory methods including +// deploy targets and sidecar providers. +type fullPlugin struct { + BaseEnginePlugin + modules map[string]ModuleFactory + steps map[string]StepFactory + triggers map[string]TriggerFactory + handlers map[string]WorkflowHandlerFactory + deployTargets map[string]deploy.DeployTarget + sidecarProviders map[string]deploy.SidecarProvider +} + +func (p *fullPlugin) ModuleFactories() map[string]ModuleFactory { return p.modules } +func (p *fullPlugin) StepFactories() map[string]StepFactory { return p.steps } +func (p *fullPlugin) TriggerFactories() map[string]TriggerFactory { return p.triggers } +func (p *fullPlugin) WorkflowHandlers() map[string]WorkflowHandlerFactory { return p.handlers } +func (p *fullPlugin) DeployTargets() map[string]deploy.DeployTarget { return p.deployTargets } +func (p *fullPlugin) SidecarProviders() map[string]deploy.SidecarProvider { + return p.sidecarProviders +} + +// mockDeployTarget is a no-op deploy target for tests. +type mockDeployTarget struct{ name string } + +func (m *mockDeployTarget) Name() string { return m.name } +func (m *mockDeployTarget) Generate(_ context.Context, _ *deploy.DeployRequest) (*deploy.DeployArtifacts, error) { + return nil, nil +} +func (m *mockDeployTarget) Apply(_ context.Context, _ *deploy.DeployArtifacts, _ deploy.ApplyOpts) (*deploy.DeployResult, error) { + return nil, nil +} +func (m *mockDeployTarget) Destroy(_ context.Context, _, _ string) error { return nil } +func (m *mockDeployTarget) Status(_ context.Context, _, _ string) (*deploy.DeployStatus, error) { + return nil, nil +} +func (m *mockDeployTarget) Diff(_ context.Context, _ *deploy.DeployArtifacts) (string, error) { + return "", nil +} +func (m *mockDeployTarget) Logs(_ context.Context, _, _ string, _ deploy.LogOpts) (io.ReadCloser, error) { + return nil, nil +} + +// mockSidecarProvider is a no-op sidecar provider for tests. +type mockSidecarProvider struct{ typeName string } + +func (m *mockSidecarProvider) Type() string { return m.typeName } +func (m *mockSidecarProvider) Validate(_ config.SidecarConfig) error { return nil } +func (m *mockSidecarProvider) Resolve(_ config.SidecarConfig, _ string) (*deploy.SidecarSpec, error) { + return nil, nil +}