Add LoadPluginWithOverride to allow external plugins to replace built-in types

[Copilot]

LoadPlugin() rejects duplicate step/module/trigger/handler type names, preventing external plugins from overriding built-in defaults (e.g., replacing the mock step.authz_check with a production Casbin-backed implementation).

Changes

• plugin/loader.go: Extract LoadPlugin logic into loadPlugin(p, allowOverride). Add LoadPluginWithOverride(p) — on duplicate, logs a warning via slog.Warn and replaces the factory instead of returning an error. Covers all factory types: modules, steps, triggers, handlers, deploy targets, sidecar providers. Also adds LoadBinaryPluginWithOverride(p, binaryPath, sigPath, certPath) as the override-capable counterpart to LoadBinaryPlugin, so binary-loaded external plugins can also intentionally replace built-ins.
• engine.go: Mirror with StdEngine.LoadPluginWithOverride(p) delegating to the loader's override path. Doc updated to accurately reflect that deploy targets and sidecar providers are also overridable.
• plugin/loader_test.go: Tests for module override, step override (the step.authz_check scenario, including assertion that LoadPlugin still rejects the duplicate), and an all-types override scenario covering modules, steps, triggers, handlers, deploy targets, and sidecar providers (with LoadPlugin duplicate rejection verified for each).

Usage

// Built-in plugins loaded normally — duplicates rejected
engine.LoadPlugin(pipelinesteps.New())

// External plugin overrides built-in step.authz_check
engine.LoadPluginWithOverride(externalAuthzPlugin)

// Binary plugin with cosign verification + override
loader.LoadBinaryPluginWithOverride(externalPlugin, binaryPath, sigPath, certPath)

Original LoadPlugin and LoadBinaryPlugin behavior is unchanged — callers must opt in to override semantics.

Original prompt

---

This section details on the original issue you should resolve

<issue_title>Plugin loader: allow external plugins to override built-in step types</issue_title>
<issue_description>## Problem

In v0.3.11, the pipelinesteps plugin now registers a built-in step.authz_check step type. External plugins (e.g., workflow-plugin-authz) that also register step.authz_check fail to load via engine.LoadPlugin() because the plugin loader (plugin/loader.go line 146) rejects duplicate step type names.

Current Behavior

step type "step.authz_check" already registered

The plugin loader checks for duplicates and returns an error, preventing the external plugin from loading entirely.

Expected Behavior

External plugins should be able to override built-in step types. This is important because:

1. The built-in step.authz_check only works with PolicyEngineModule (which only supports "mock" backend via resolvePolicyEngine() concrete type assertion)
2. The external workflow-plugin-authz provides a Casbin-backed implementation that is production-ready
3. External plugins should be treated as intentional overrides of built-in defaults

Workaround

We bypass the plugin loader entirely by registering external plugin factories directly via engine.AddModuleType() / engine.AddStepType() (which silently overwrite):

func registerExternalPlugin(engine *wf.StdEngine, p plugin.EnginePlugin) error {
    for typeName, factory := range p.ModuleFactories() {
        f := factory
        engine.AddModuleType(typeName, func(name string, cfg map[string]any) modular.Module {
            return f(name, cfg)
        })
    }
    for typeName, factory := range p.StepFactories() {
        capturedType := typeName
        capturedFactory := factory
        engine.AddStepType(typeName, func(name string, cfg map[string]any, app modular.Application) (module.PipelineStep, error) {
            result, err := capturedFactory(name, cfg, app)
            if err != nil {
                return nil, err
            }
            step, ok := result.(module.PipelineStep)
            if !ok {
                return nil, fmt.Errorf("external plugin step factory for %q returned non-PipelineStep type %T", capturedType, result)
            }
            return step, nil
        })
    }
    return nil
}

Suggested Fix

Option A: Allow LoadPlugin() to accept an override flag or option that permits replacing existing types.

Option B: Change the plugin loader to log a warning instead of returning an error when an external plugin registers a type that already exists, treating external plugins as intentional overrides.

Option C: Add an engine.LoadPluginWithOverride() method that uses the existing loader but skips the duplicate check.</issue_description>

Comments on the Issue (you are @copilot in this section)

• Fixes Plugin loader: allow external plugins to override built-in step types #262

---

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

[Copilot]

Pull request overview

Adds an opt-in “override” plugin loading path so external plugins can intentionally replace built-in module/step/trigger/handler (and related) registrations without failing on duplicate type names—supporting scenarios like overriding the built-in step.authz_check.

Changes:

• Refactors PluginLoader.LoadPlugin internals into loadPlugin(p, allowOverride) and adds LoadPluginWithOverride that replaces duplicates while logging warnings.
• Adds StdEngine.LoadPluginWithOverride, delegating to the loader’s override behavior.
• Adds unit tests validating override behavior for modules, steps, and a multi-type case.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
plugin/loader.go	Introduces LoadPluginWithOverride and shared loadPlugin(..., allowOverride) to replace factories on duplicates with warnings.
engine.go	Adds StdEngine.LoadPluginWithOverride and internal helper to select override vs strict loader path.
plugin/loader_test.go	Adds tests for override behavior (module override, step.authz_check override, and a combined-types override scenario).

[Copilot]

Engine.LoadPluginWithOverride doc says it only overrides module/step/trigger/handler registrations, but it delegates to PluginLoader.LoadPluginWithOverride which also overrides deploy targets and sidecar providers. Please either update this doc to mention deploy targets/sidecar providers too, or constrain the override behavior to match the doc so callers aren’t surprised.

Suggested change

	// 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).

[Copilot]

PluginLoader has LoadBinaryPlugin() which currently always calls LoadPlugin() (duplicate types rejected). With the new override capability, consider adding a LoadBinaryPluginWithOverride() (or an override flag) so binary-loaded external plugins can intentionally replace built-ins as well.

Suggested change

	// LoadBinaryPluginWithOverride is the override-capable counterpart to loading
	// binary or externally provided plugins. It behaves like LoadPluginWithOverride,
	// allowing a binary-loaded plugin to intentionally replace built-in module,
	// step, trigger, handler, deploy target, and sidecar provider registrations.
	func (l *PluginLoader) LoadBinaryPluginWithOverride(p EnginePlugin) error {
	return l.loadPlugin(p, true)
	}

[Copilot]

This test validates override behavior, but it doesn’t assert that the non-override path still rejects duplicate step types (only the module override test covers the non-override behavior for modules). Adding a LoadPlugin(p2) failure assertion here would better protect against regressions in the step duplicate check.

[Copilot]

Test name/intent says "AllTypes", but it only covers modules/steps/triggers/handlers. The new override path also includes deploy targets and sidecar providers; either extend this test to include those types (and/or their duplicate rejection in LoadPlugin) or rename/split the test so coverage matches what’s being claimed.

[intel352]

@copilot apply changes based on the comments in this thread

[Copilot]

@copilot apply changes based on the comments in this thread

Applied all changes from the review thread in commit 1a7e118:

1. engine.go doc — Updated LoadPluginWithOverride comment to mention deploy targets and sidecar providers.
2. LoadBinaryPluginWithOverride — Added to PluginLoader so binary-loaded plugins can also override built-ins.
3. Step type test — Added LoadPlugin(p2) failure assertion before the override call to guard against regressions.
4. AllTypes test — Extended to cover deploy targets and sidecar providers; added fullPlugin, mockDeployTarget, and mockSidecarProvider test helpers.