Skip to content

fix: tighten engine_module_bridge and mockApplication based on review feedback #148

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
intel352 merged 2 commits into from
Feb 23, 2026
Merged

fix: tighten engine_module_bridge and mockApplication based on review feedback #148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f28a0cd
Initial plan
Copilot Feb 23, 2026
6b5b647
refactor: address code review feedback on engine_module_bridge and en…
Copilot Feb 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions engine_module_bridge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,13 @@ func (e *StdEngine) registerPluginSteps(typeName string, stepFactory func(name s
// uses the canonical interface type.
func (e *StdEngine) registerPluginTrigger(triggerType string, factory func() any) {
result := factory()
if trigger, ok := result.(interfaces.Trigger); ok {
e.triggerTypeMap[triggerType] = trigger.Name()
e.RegisterTrigger(trigger)
trigger, ok := result.(interfaces.Trigger)
if !ok {
// Fail fast with a clear warning when a plugin misconfigures its trigger factory.
// This avoids silent failures that later surface as "no handler found" errors.
e.logger.Error(fmt.Sprintf("workflow: plugin trigger factory for %q returned non-Trigger type %T; trigger not registered", triggerType, result))
return
}
e.triggerTypeMap[triggerType] = trigger.Name()
e.RegisterTrigger(trigger)
}
23 changes: 9 additions & 14 deletions engine_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,10 +281,15 @@ func (a *mockApplication) GetService(name string, out any) error {

// If out is provided, try to assign the service to it using reflection
if out != nil {
// Guard: nil service value would make reflect.ValueOf(svc).Type() panic.
if svc == nil {
return fmt.Errorf("service %s has a nil value", name)
}

// Get reflect values
outVal := reflect.ValueOf(out)
if outVal.Kind() != reflect.Pointer {
return fmt.Errorf("out parameter must be a pointer")
if outVal.Kind() != reflect.Pointer || outVal.IsNil() {
return fmt.Errorf("out parameter must be a non-nil pointer")
}

// Dereference the pointer
Expand Down Expand Up @@ -1544,14 +1549,6 @@ func TestEngine_BuildFromConfig_RequiresPlugins_EmptyPluginsList(t *testing.T) {
// TestEngine_BuildFromConfig_RequiresPlugins_ExactVersionMatch verifies
// exact version matching (=1.2.3 constraint).
func TestEngine_BuildFromConfig_RequiresPlugins_ExactVersionMatch(t *testing.T) {
app := newMockApplication()
engine := NewStdEngine(app, app.Logger())

p := newMinimalPlugin("exact-plugin", "1.2.3")
if err := engine.LoadPlugin(p); err != nil {
t.Fatalf("LoadPlugin failed: %v", err)
}

tests := []struct {
constraint string
wantOK bool
Expand All @@ -1577,10 +1574,8 @@ func TestEngine_BuildFromConfig_RequiresPlugins_ExactVersionMatch(t *testing.T)
},
}

// Use a fresh engine for each sub-test to avoid state contamination.
// We re-use the same app since plugins are already registered in the
// PluginLoader (which we recreate by calling LoadPlugin again on a
// fresh engine but referencing the same plugin instance).
// Use a fresh app and engine (with a freshly loaded plugin) for each
// sub-test to avoid state contamination.
subApp := newMockApplication()
subEngine := NewStdEngine(subApp, subApp.Logger())
subP := newMinimalPlugin("exact-plugin", "1.2.3")
Expand Down