From 39b88b203ea2d13c844b45e1be6f5824078fc99d Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Wed, 13 May 2026 18:49:00 -0400 Subject: [PATCH 01/10] fix(engine,#663): plumb yaml dependsOn into RemoteModule.Dependencies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to PR #664 (v0.51.8). The v0.51.8 fix reordered cfg.Modules by yaml dependsOn before app.RegisterModule, but modular's app.Init() then runs its OWN DependencyAware-driven sort over the registered modules — and RemoteModule (the wrapper used for every external-plugin module) returned nil from Dependencies(). So modular still saw every external-plugin module as a root and sorted alphabetically. BMW PR #280 image-launch surfaced this on 53283b2: with the aaa-/aab- prefix workaround removed and v0.51.8 in place, the engine still initialised the 6 bmw-consumer-* modules before bmw-eventbus and bmw-stream, hit the broker-not-registered 10s race, and never served /healthz. The fix has two parts: 1. plugin/external/remote_module.go: add a dependencies field on RemoteModule + a SetDependencies setter. Dependencies() now returns the stored slice instead of always returning nil. 2. engine.go: after the factory returns the module, call SetDependencies(modCfg.DependsOn) via a structural type assertion: if dt, ok := mod.(interface{ SetDependencies([]string) }); ok { dt.SetDependencies(deps) } Built-in modules opt in by implementing the same setter; modules that don't implement it see no behaviour change. The yaml slice is defensively copied before storing. 6 unit tests in remote_module_dependencies_test.go cover the contract: default-nil, plumb-from-yaml, empty-slice tolerance, overwrite-replaces, and two structural type-assertion pins so a future refactor that drops SetDependencies fails loudly instead of silently re-introducing the workflow#663 race. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 4 + engine.go | 17 ++++ plugin/external/remote_module.go | 20 ++++- .../remote_module_dependencies_test.go | 85 +++++++++++++++++++ 4 files changed, 125 insertions(+), 1 deletion(-) create mode 100644 plugin/external/remote_module_dependencies_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index dce7702d..ae64bec6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,10 @@ Configs that still reference the legacy types now fail to load with an actionabl ## [Unreleased] +### Fixed (issue #663 — follow-up) + +- **`*external.RemoteModule.Dependencies()` now returns the yaml-level `dependsOn:` keys** instead of always returning `nil`. The v0.51.8 fix (PR #664) only reordered the `cfg.Modules` slice — but modular's `app.Init()` then runs its own `DependencyAware`-driven sort over the registered modules, and `RemoteModule` (the wrapper used for every external-plugin module) returned `nil` from `Dependencies()`, so modular saw every external-plugin module as a root and sorted alphabetically. BMW PR #280 image-launch surfaced this as the same `bmw-eventbus`/`bmw-stream` ordering race that v0.51.8 was supposed to close. Engine `BuildFromConfig` now calls `SetDependencies(modCfg.DependsOn)` on each module that implements `interface{ SetDependencies([]string) }` immediately after the factory returns, before `app.RegisterModule`. `RemoteModule` implements that setter and stores the slice on the struct; modular's Init() walker then reads it via the existing `Dependencies()` contract. 6 unit tests cover the contract (default-nil, plumb, empty-slice, overwrite, two structural type-assertion pins). Built-in modules can opt in by implementing the same setter; existing behaviour is unchanged for modules that don't. + ### Fixed (issue #663) - **`StdEngine.BuildFromConfig` now topologically sorts `cfg.Modules` by yaml `dependsOn:` before module registration.** Previously the engine walked `cfg.Modules` in slice order and the modular framework's `app.Init()` then walked in registration order. For external-plugin modules (which do not implement `DependencyAware`), yaml-level `dependsOn:` was validated by the schema but ignored at init time — a child module's `Init()` could fire before its parent's `Init()` had registered runtime state in a plugin-local registry (broker, factory table, etc.), causing startup races like the "broker not registered within 10s" failure that bit BMW PR #279. The new `topoSortModules` (in `engine_module_order.go`) uses Kahn's algorithm with a stable tie-break on declared index, tolerates missing dependency targets (schema validation catches those separately), and returns an error listing every module that could not be ordered (cycle members and their downstream dependents — Kahn cannot distinguish them by inDegree alone) if dependencies form a loop. 12 unit tests cover the BMW shape, parallel chains, cycles + their downstream dependents, and edge cases. Operators who used the alphabetical-prefix workaround (renaming a dependency to `aaa-…`) can now revert to canonical names and rely on `dependsOn:` to drive the order. diff --git a/engine.go b/engine.go index 7328e707..3aacdb7d 100644 --- a/engine.go +++ b/engine.go @@ -555,6 +555,23 @@ func (e *StdEngine) BuildFromConfig(cfg *config.WorkflowConfig) error { return fmt.Errorf("factory for module type %q returned nil for module %q", modCfg.Type, modCfg.Name) } + // Plumb yaml-level `dependsOn:` into the module so modular's Init() + // walker honours it (workflow#663). Without this, external-plugin + // modules — which all return nil from Dependencies() by default — + // looked like roots to modular and got initialised in alphabetical + // order, which broke any plugin where module A's Init() registered + // runtime state that module B's Init() needed. The topoSortModules + // pass above already reordered cfg.Modules; this hands the same + // information to modular so its own initialisation graph agrees + // with engine-level ordering. + if len(modCfg.DependsOn) > 0 { + if depTarget, ok := mod.(interface{ SetDependencies([]string) }); ok { + deps := make([]string, len(modCfg.DependsOn)) + copy(deps, modCfg.DependsOn) + depTarget.SetDependencies(deps) + } + } + e.app.RegisterModule(mod) } diff --git a/plugin/external/remote_module.go b/plugin/external/remote_module.go index dc57d92b..5bcd1f48 100644 --- a/plugin/external/remote_module.go +++ b/plugin/external/remote_module.go @@ -18,6 +18,16 @@ type RemoteModule struct { contract *pb.ContractDescriptor serviceContracts map[string]*pb.ContractDescriptor types protoregistry.MessageTypeResolver + + // dependencies holds the yaml-level `dependsOn:` keys for this module. + // Populated by the engine after the factory returns via SetDependencies + // — see workflow#663. Returned from Dependencies() so modular's Init() + // walker honours them when computing module init order. Without this, + // every external-plugin module appeared as a root and modular sorted + // them alphabetically, which broke any plugin where module A's Init() + // registered runtime state in a plugin-local registry that module B's + // Init() looked up. + dependencies []string } type remoteModuleContracts struct { @@ -53,7 +63,15 @@ func (m *RemoteModule) Name() string { } func (m *RemoteModule) Dependencies() []string { - return nil + return m.dependencies +} + +// SetDependencies records the yaml-level `dependsOn:` keys declared for this +// module so modular's Init() walker can honour them. Called by the engine +// from BuildFromConfig once per module, immediately after the factory +// returns and before app.RegisterModule. See workflow#663. +func (m *RemoteModule) SetDependencies(deps []string) { + m.dependencies = deps } func (m *RemoteModule) ProvidesServices() []string { diff --git a/plugin/external/remote_module_dependencies_test.go b/plugin/external/remote_module_dependencies_test.go new file mode 100644 index 00000000..933c9a26 --- /dev/null +++ b/plugin/external/remote_module_dependencies_test.go @@ -0,0 +1,85 @@ +package external + +import ( + "testing" +) + +// TestRemoteModule_Dependencies_DefaultsToNil pins that a freshly-constructed +// RemoteModule reports no dependencies — the same behaviour as before +// workflow#663. The DependencyAware contract still applies; modular's +// Init() walker just sees an empty list and treats the module as a root. +func TestRemoteModule_Dependencies_DefaultsToNil(t *testing.T) { + m := &RemoteModule{name: "x"} + if got := m.Dependencies(); got != nil { + t.Errorf("Dependencies() default = %v, want nil", got) + } +} + +// TestRemoteModule_SetDependencies_PlumbsYamlDependsOn verifies the engine's +// post-factory plumb path: after the factory returns the module the engine +// calls SetDependencies with the yaml-level `dependsOn:` slice. Dependencies() +// must then return exactly that slice so modular's DependencyAware-driven +// Init() walker honours it. Closes the workflow#663 follow-up that surfaced +// on BMW PR #280's image-launch. +func TestRemoteModule_SetDependencies_PlumbsYamlDependsOn(t *testing.T) { + m := &RemoteModule{name: "bmw-consumer-audit-appender"} + m.SetDependencies([]string{"bmw-eventbus", "bmw-stream"}) + got := m.Dependencies() + if len(got) != 2 || got[0] != "bmw-eventbus" || got[1] != "bmw-stream" { + t.Errorf("Dependencies() = %v, want [bmw-eventbus bmw-stream]", got) + } +} + +// TestRemoteModule_SetDependencies_EmptySliceIsEmpty pins that passing an +// empty slice records an empty (non-nil) slice. modular treats a non-nil +// empty slice as "no dependencies declared" the same as nil, so the +// distinction is internal — but pinning it prevents accidental drift if a +// future refactor adds a nil-check that changes semantics. +func TestRemoteModule_SetDependencies_EmptySliceIsEmpty(t *testing.T) { + m := &RemoteModule{name: "x"} + m.SetDependencies([]string{}) + got := m.Dependencies() + if got == nil || len(got) != 0 { + t.Errorf("Dependencies() after SetDependencies([]) = %v, want empty non-nil slice", got) + } +} + +// TestRemoteModule_SetDependencies_Overwrites pins that a second call +// replaces the previous slice rather than appending. The engine calls +// SetDependencies exactly once per module per BuildFromConfig, but +// pinning the contract guards against future engine-side double-calls +// (e.g., a config-transform hook that mutates dependsOn post-registration). +func TestRemoteModule_SetDependencies_Overwrites(t *testing.T) { + m := &RemoteModule{name: "x"} + m.SetDependencies([]string{"a", "b"}) + m.SetDependencies([]string{"c"}) + got := m.Dependencies() + if len(got) != 1 || got[0] != "c" { + t.Errorf("Dependencies() after second SetDependencies = %v, want [c]", got) + } +} + +// TestRemoteModule_ImplementsDependencyAware pins that *RemoteModule +// satisfies a structurally-typed interface matching modular.DependencyAware. +// We can't import modular's interface here without a circular dep, but +// the structural check is sufficient — modular itself does the same +// type assertion at init time. +func TestRemoteModule_ImplementsDependencyAware(t *testing.T) { + var _ interface { + Dependencies() []string + } = (*RemoteModule)(nil) +} + +// TestRemoteModule_ImplementsDependencyTargetInterface pins the structural +// type that engine.go uses to find modules whose dependsOn it should plumb: +// +// if dt, ok := mod.(interface{ SetDependencies([]string) }); ok { ... } +// +// If a future refactor drops SetDependencies the engine silently stops +// plumbing, which would re-introduce the workflow#663 race. This test +// fails loudly instead. +func TestRemoteModule_ImplementsDependencyTargetInterface(t *testing.T) { + var _ interface { + SetDependencies([]string) + } = (*RemoteModule)(nil) +} From f0a6599b9090238bb0a232695c62bf63ea671b0d Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Wed, 13 May 2026 18:58:55 -0400 Subject: [PATCH 02/10] test(engine,#665): cover BuildFromConfig dependsOn plumbing path Copilot R1 on PR #665 flagged: the new engine.go plumbing path that calls SetDependencies via structural type assertion is not covered by an end-to-end test. The added RemoteModule unit tests prove the storage works in isolation, but a future refactor that moves or removes the assertion in engine.go would silently regress the workflow#663 fix and only image-launch CI would catch it. This commit adds 3 BuildFromConfig-level tests using a custom ModuleFactory that registers a fake module implementing the SetDependencies setter: - TestEngine_BuildFromConfig_PlumbsDependsOnIntoModule: a 2-module config (broker + consumer where consumer DependsOn:[broker]) must drive exactly one SetDependencies call on the consumer with the value [broker], and zero calls on the broker. - TestEngine_BuildFromConfig_PlumbsDefensiveCopy: mutates the yaml-derived dependsOn slice after BuildFromConfig returns and verifies the recorded value didn't change. Proves the engine copies the slice before passing it through. - TestEngine_BuildFromConfig_SkipsModulesWithoutSetter: registers a module whose type does NOT implement SetDependencies and confirms BuildFromConfig succeeds (doesn't panic, doesn't error). Co-Authored-By: Claude Opus 4.7 (1M context) --- engine_dependson_plumbing_test.go | 176 ++++++++++++++++++++++++++++++ 1 file changed, 176 insertions(+) create mode 100644 engine_dependson_plumbing_test.go diff --git a/engine_dependson_plumbing_test.go b/engine_dependson_plumbing_test.go new file mode 100644 index 00000000..a1513bf2 --- /dev/null +++ b/engine_dependson_plumbing_test.go @@ -0,0 +1,176 @@ +package workflow + +import ( + "testing" + + "github.com/GoCodeAlone/modular" + "github.com/GoCodeAlone/workflow/config" +) + +// fakeDepAwareModule implements modular.Module + the +// `interface{ SetDependencies([]string) }` contract that engine.go uses to +// plumb yaml-level dependsOn into modules. Recording every SetDependencies +// call lets the test assert the engine actually invoked it. +type fakeDepAwareModule struct { + name string + setCalls [][]string // ordered record of every SetDependencies invocation + initialised bool +} + +func (m *fakeDepAwareModule) Name() string { return m.name } +func (m *fakeDepAwareModule) Init(_ modular.Application) error { m.initialised = true; return nil } +func (m *fakeDepAwareModule) RegisterConfig(_ modular.Application) error { return nil } +func (m *fakeDepAwareModule) Dependencies() []string { return nil } +func (m *fakeDepAwareModule) ProvidesServices() []modular.ServiceProvider { return nil } +func (m *fakeDepAwareModule) RequiresServices() []modular.ServiceDependency { return nil } +func (m *fakeDepAwareModule) SetDependencies(deps []string) { + // Defensive copy so we record what the engine passed, not what later + // code may have mutated on the slice. + cp := make([]string, len(deps)) + copy(cp, deps) + m.setCalls = append(m.setCalls, cp) +} + +// fakePlainModule deliberately does NOT implement SetDependencies. The +// engine plumbing must skip it without panicking, regardless of whether +// modCfg.DependsOn is set. +type fakePlainModule struct { + name string +} + +func (m *fakePlainModule) Name() string { return m.name } +func (m *fakePlainModule) Init(_ modular.Application) error { return nil } +func (m *fakePlainModule) RegisterConfig(_ modular.Application) error { return nil } +func (m *fakePlainModule) Dependencies() []string { return nil } +func (m *fakePlainModule) ProvidesServices() []modular.ServiceProvider { return nil } +func (m *fakePlainModule) RequiresServices() []modular.ServiceDependency { return nil } + +// TestEngine_BuildFromConfig_PlumbsDependsOnIntoModule pins the production +// path that closes workflow#663: BuildFromConfig must call SetDependencies +// on each module that implements it, with a defensive copy of +// modCfg.DependsOn, before app.RegisterModule. Without this end-to-end +// test, a future refactor that moves the structural type assertion out +// of the registration loop (or changes the contract) would silently +// regress the BMW PR #279/280 fix and only image-launch CI would catch it. +func TestEngine_BuildFromConfig_PlumbsDependsOnIntoModule(t *testing.T) { + app := newMockApplication() + engine := NewStdEngine(app, app.Logger()) + + var capturedConsumer *fakeDepAwareModule + var capturedBroker *fakeDepAwareModule + engine.AddModuleType("test.broker", func(name string, _ map[string]any) modular.Module { + capturedBroker = &fakeDepAwareModule{name: name} + return capturedBroker + }) + engine.AddModuleType("test.consumer", func(name string, _ map[string]any) modular.Module { + capturedConsumer = &fakeDepAwareModule{name: name} + return capturedConsumer + }) + + cfg := &config.WorkflowConfig{ + Modules: []config.ModuleConfig{ + {Name: "broker", Type: "test.broker"}, + {Name: "consumer", Type: "test.consumer", DependsOn: []string{"broker"}}, + }, + Workflows: map[string]any{}, + Triggers: map[string]any{}, + } + + if err := engine.BuildFromConfig(cfg); err != nil { + t.Fatalf("BuildFromConfig: %v", err) + } + + if capturedBroker == nil || capturedConsumer == nil { + t.Fatal("factories were not invoked") + } + + // Broker has no dependsOn → no SetDependencies call. + if len(capturedBroker.setCalls) != 0 { + t.Errorf("broker SetDependencies calls = %d, want 0", len(capturedBroker.setCalls)) + } + + // Consumer dependsOn:[broker] → exactly one SetDependencies call with that value. + if len(capturedConsumer.setCalls) != 1 { + t.Fatalf("consumer SetDependencies calls = %d, want 1", len(capturedConsumer.setCalls)) + } + got := capturedConsumer.setCalls[0] + if len(got) != 1 || got[0] != "broker" { + t.Errorf("consumer SetDependencies received %v, want [broker]", got) + } +} + +// TestEngine_BuildFromConfig_PlumbsDefensiveCopy pins the defensive copy: +// after BuildFromConfig returns, mutating modCfg.DependsOn must not +// affect the slice the module recorded. Without the copy, downstream +// code that re-uses ModuleConfig structs across builds (or mutates them +// for any reason) could silently corrupt the engine-side dependency graph. +func TestEngine_BuildFromConfig_PlumbsDefensiveCopy(t *testing.T) { + app := newMockApplication() + engine := NewStdEngine(app, app.Logger()) + + var captured *fakeDepAwareModule + engine.AddModuleType("test.depmod", func(name string, _ map[string]any) modular.Module { + captured = &fakeDepAwareModule{name: name} + return captured + }) + + deps := []string{"root-a", "root-b"} + cfg := &config.WorkflowConfig{ + Modules: []config.ModuleConfig{ + {Name: "root-a", Type: "test.depmod"}, + {Name: "root-b", Type: "test.depmod"}, + {Name: "child", Type: "test.depmod", DependsOn: deps}, + }, + Workflows: map[string]any{}, + Triggers: map[string]any{}, + } + + if err := engine.BuildFromConfig(cfg); err != nil { + t.Fatalf("BuildFromConfig: %v", err) + } + + // Mutate the original yaml-derived slice. Because we registered three + // modules using the same factory, `captured` is the LAST one — the + // "child" module with the dependsOn slice. Its recorded SetDependencies + // arg should NOT change after we mutate `deps`. + if len(captured.setCalls) != 1 { + t.Fatalf("child SetDependencies calls = %d, want 1", len(captured.setCalls)) + } + deps[0] = "root-b" // valid mutation — duplicate of root-b, doesn't matter for the assertion + got := captured.setCalls[0] + if got[0] != "root-a" { + t.Errorf("engine did NOT defensively copy DependsOn — mutating the yaml slice changed the recorded value from root-a to %q", got[0]) + } +} + +// TestEngine_BuildFromConfig_SkipsModulesWithoutSetter pins that modules +// not implementing the SetDependencies contract are silently skipped +// (not panicked, not errored) even when modCfg.DependsOn is non-empty. +// The yaml-level dependsOn is then expressed only through cfg.Modules +// slice ordering (via topoSortModules) — modular's own DependencyAware +// sort sees nil from the module's Dependencies() and treats it as a root. +// This is the back-compat surface for built-in modules that haven't +// opted in to the new setter. +func TestEngine_BuildFromConfig_SkipsModulesWithoutSetter(t *testing.T) { + app := newMockApplication() + engine := NewStdEngine(app, app.Logger()) + + engine.AddModuleType("test.plain", func(name string, _ map[string]any) modular.Module { + return &fakePlainModule{name: name} + }) + + cfg := &config.WorkflowConfig{ + Modules: []config.ModuleConfig{ + {Name: "root", Type: "test.plain"}, + {Name: "child", Type: "test.plain", DependsOn: []string{"root"}}, + }, + Workflows: map[string]any{}, + Triggers: map[string]any{}, + } + + // The structural-type-assertion path must not panic on a module that + // lacks the setter. A successful BuildFromConfig is the assertion. + if err := engine.BuildFromConfig(cfg); err != nil { + t.Fatalf("BuildFromConfig must tolerate modules without SetDependencies: %v", err) + } +} From 925fafe8d6f3e82718e06e8563fa6b58a6714644 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Wed, 13 May 2026 19:06:17 -0400 Subject: [PATCH 03/10] test(engine,#665): use raw-recorder fake for defensive-copy assertion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot R2 caught it: the original fakeDepAwareModule.SetDependencies defensively copied the slice on the recording side, which masked whether the engine itself copies — even if BuildFromConfig passed modCfg.DependsOn through verbatim, mutating the yaml slice afterward would not change the recorded value (the fake had its own copy). This commit splits the recorder into a second fake type (rawSliceRecorderModule) that retains the EXACT slice reference passed to SetDependencies. TestEngine_BuildFromConfig_PlumbsDefensiveCopy now uses that fake and proves the engine itself copies: mutating the yaml-derived `deps` slice after BuildFromConfig must not affect the child's recorded reference. Sanity-checked locally by temporarily replacing the engine's `depTarget.SetDependencies(deps)` with `depTarget.SetDependencies(modCfg.DependsOn)` — the test correctly FAILS with "engine did NOT defensively copy". After restoring the engine the test passes again. Co-Authored-By: Claude Opus 4.7 (1M context) --- engine_dependson_plumbing_test.go | 71 +++++++++++++++++++++++-------- 1 file changed, 54 insertions(+), 17 deletions(-) diff --git a/engine_dependson_plumbing_test.go b/engine_dependson_plumbing_test.go index a1513bf2..12698c09 100644 --- a/engine_dependson_plumbing_test.go +++ b/engine_dependson_plumbing_test.go @@ -31,6 +31,29 @@ func (m *fakeDepAwareModule) SetDependencies(deps []string) { m.setCalls = append(m.setCalls, cp) } +// rawSliceRecorderModule retains the EXACT slice reference passed to +// SetDependencies — no defensive copy on the recording side. Used by +// TestEngine_BuildFromConfig_PlumbsDefensiveCopy to prove the engine +// itself copies; if the engine just passed modCfg.DependsOn through, +// mutating the original yaml-derived slice would mutate this recorded +// reference too, and the test would fail. +type rawSliceRecorderModule struct { + name string + rawRecorded []string // nil until SetDependencies fires; then the exact slice ref + callCount int +} + +func (m *rawSliceRecorderModule) Name() string { return m.name } +func (m *rawSliceRecorderModule) Init(_ modular.Application) error { return nil } +func (m *rawSliceRecorderModule) RegisterConfig(_ modular.Application) error { return nil } +func (m *rawSliceRecorderModule) Dependencies() []string { return nil } +func (m *rawSliceRecorderModule) ProvidesServices() []modular.ServiceProvider { return nil } +func (m *rawSliceRecorderModule) RequiresServices() []modular.ServiceDependency { return nil } +func (m *rawSliceRecorderModule) SetDependencies(deps []string) { + m.rawRecorded = deps // INTENTIONAL: no copy. Asserts engine-side copy. + m.callCount++ +} + // fakePlainModule deliberately does NOT implement SetDependencies. The // engine plumbing must skip it without panicking, regardless of whether // modCfg.DependsOn is set. @@ -104,22 +127,31 @@ func TestEngine_BuildFromConfig_PlumbsDependsOnIntoModule(t *testing.T) { // affect the slice the module recorded. Without the copy, downstream // code that re-uses ModuleConfig structs across builds (or mutates them // for any reason) could silently corrupt the engine-side dependency graph. +// +// The fake here records the EXACT slice reference passed to +// SetDependencies (no defensive copy on the recording side) so a +// regression where the engine passes modCfg.DependsOn through verbatim +// would let a post-build mutation appear in the recorded value and fail +// the assertion. func TestEngine_BuildFromConfig_PlumbsDefensiveCopy(t *testing.T) { app := newMockApplication() engine := NewStdEngine(app, app.Logger()) - var captured *fakeDepAwareModule - engine.AddModuleType("test.depmod", func(name string, _ map[string]any) modular.Module { - captured = &fakeDepAwareModule{name: name} - return captured + var capturedChild *rawSliceRecorderModule + engine.AddModuleType("test.rawmod", func(name string, _ map[string]any) modular.Module { + m := &rawSliceRecorderModule{name: name} + if name == "child" { + capturedChild = m + } + return m }) deps := []string{"root-a", "root-b"} cfg := &config.WorkflowConfig{ Modules: []config.ModuleConfig{ - {Name: "root-a", Type: "test.depmod"}, - {Name: "root-b", Type: "test.depmod"}, - {Name: "child", Type: "test.depmod", DependsOn: deps}, + {Name: "root-a", Type: "test.rawmod"}, + {Name: "root-b", Type: "test.rawmod"}, + {Name: "child", Type: "test.rawmod", DependsOn: deps}, }, Workflows: map[string]any{}, Triggers: map[string]any{}, @@ -129,17 +161,22 @@ func TestEngine_BuildFromConfig_PlumbsDefensiveCopy(t *testing.T) { t.Fatalf("BuildFromConfig: %v", err) } - // Mutate the original yaml-derived slice. Because we registered three - // modules using the same factory, `captured` is the LAST one — the - // "child" module with the dependsOn slice. Its recorded SetDependencies - // arg should NOT change after we mutate `deps`. - if len(captured.setCalls) != 1 { - t.Fatalf("child SetDependencies calls = %d, want 1", len(captured.setCalls)) + if capturedChild == nil { + t.Fatal("child factory was not invoked") + } + if capturedChild.callCount != 1 { + t.Fatalf("child SetDependencies call count = %d, want 1", capturedChild.callCount) } - deps[0] = "root-b" // valid mutation — duplicate of root-b, doesn't matter for the assertion - got := captured.setCalls[0] - if got[0] != "root-a" { - t.Errorf("engine did NOT defensively copy DependsOn — mutating the yaml slice changed the recorded value from root-a to %q", got[0]) + + // Mutate the original yaml-derived slice. The child's recorded + // reference must NOT see this change, because the engine should have + // passed a defensive copy. If the engine passed `deps` directly, this + // mutation would change capturedChild.rawRecorded[0] too and the + // assertion below would fail. + deps[0] = "MUTATED" + if capturedChild.rawRecorded[0] != "root-a" { + t.Errorf("engine did NOT defensively copy DependsOn — mutating the yaml slice from %q to %q changed the recorded value (now %q)", + "root-a", deps[0], capturedChild.rawRecorded[0]) } } From 04ac269fa0571e930c951338e38f61ccc4a854d4 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Wed, 13 May 2026 19:16:07 -0400 Subject: [PATCH 04/10] review fixes R3: defensive setter copy + real-modular Init test + CHANGELOG MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Copilot R3 on PR #665: 1. **plugin/external/remote_module.go**: SetDependencies now defensively copies the slice. The engine path already copies before calling, but the setter is exported and Dependencies() exposes the same backing array to modular — copying inside the setter too means any external caller (tests, future integration paths) can mutate its source slice without silently corrupting this module's init graph. 2. **CHANGELOG.md**: clarified that BuildFromConfig calls SetDependencies only when modCfg.DependsOn is non-empty (the production code's `len(modCfg.DependsOn) > 0` guard exists so calling SetDependencies(nil) doesn't clobber a custom module's constructor-time defaults). Also bumped the test count from 6 to 10 to reflect the engine-level coverage added in #f0a6599b + #925fafe8 + this commit. 3. **engine_dependson_plumbing_test.go**: added TestEngine_BuildFromConfig_RealModularHonoursDependsOn which uses modular.NewStdApplication (NOT the test-package mockApplication whose Init() is a no-op). The test names the broker module "z-broker" — alphabetically AFTER its dependent "consumer" — so the only way the assertion passes is if SetDependencies actually plumbs into modular's DependencyAware-driven Init walker. A regression where SetDependencies is called but modular still misorders would FAIL with order=[consumer z-broker] instead of the asserted [z-broker consumer]. Adds 2 helpers (initOrderRecorderModule + stubConfigProvider) so this remains self-contained inside the workflow package. Sanity checked locally that the new test passes against the real modular framework + against the canonical engine code. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 2 +- engine_dependson_plumbing_test.go | 101 ++++++++++++++++++++++++++++++ plugin/external/remote_module.go | 10 ++- 3 files changed, 111 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ae64bec6..50f1badc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,7 +25,7 @@ Configs that still reference the legacy types now fail to load with an actionabl ### Fixed (issue #663 — follow-up) -- **`*external.RemoteModule.Dependencies()` now returns the yaml-level `dependsOn:` keys** instead of always returning `nil`. The v0.51.8 fix (PR #664) only reordered the `cfg.Modules` slice — but modular's `app.Init()` then runs its own `DependencyAware`-driven sort over the registered modules, and `RemoteModule` (the wrapper used for every external-plugin module) returned `nil` from `Dependencies()`, so modular saw every external-plugin module as a root and sorted alphabetically. BMW PR #280 image-launch surfaced this as the same `bmw-eventbus`/`bmw-stream` ordering race that v0.51.8 was supposed to close. Engine `BuildFromConfig` now calls `SetDependencies(modCfg.DependsOn)` on each module that implements `interface{ SetDependencies([]string) }` immediately after the factory returns, before `app.RegisterModule`. `RemoteModule` implements that setter and stores the slice on the struct; modular's Init() walker then reads it via the existing `Dependencies()` contract. 6 unit tests cover the contract (default-nil, plumb, empty-slice, overwrite, two structural type-assertion pins). Built-in modules can opt in by implementing the same setter; existing behaviour is unchanged for modules that don't. +- **`*external.RemoteModule.Dependencies()` now returns the yaml-level `dependsOn:` keys** instead of always returning `nil`. The v0.51.8 fix (PR #664) only reordered the `cfg.Modules` slice — but modular's `app.Init()` then runs its own `DependencyAware`-driven sort over the registered modules, and `RemoteModule` (the wrapper used for every external-plugin module) returned `nil` from `Dependencies()`, so modular saw every external-plugin module as a root and sorted alphabetically. BMW PR #280 image-launch surfaced this as the same `bmw-eventbus`/`bmw-stream` ordering race that v0.51.8 was supposed to close. Engine `BuildFromConfig` now calls `SetDependencies(modCfg.DependsOn)` on each module that **declares `dependsOn:` in yaml AND implements** `interface{ SetDependencies([]string) }` immediately after the factory returns, before `app.RegisterModule`. (Modules with empty `dependsOn:` are skipped — calling `SetDependencies(nil)` would clear any defaults a custom module set up in its constructor.) `RemoteModule` implements that setter, defensively copies the slice, and modular's Init() walker then reads it via the existing `Dependencies()` contract. 6 unit tests cover the `RemoteModule` contract (default-nil, plumb, empty-slice, overwrite, two structural type-assertion pins) plus 4 engine-level `BuildFromConfig` tests covering the production path (basic plumb + defensive copy via raw-slice recorder + back-compat skip + real-modular Init order). Built-in modules can opt in by implementing the same setter; existing behaviour is unchanged for modules that don't. ### Fixed (issue #663) diff --git a/engine_dependson_plumbing_test.go b/engine_dependson_plumbing_test.go index 12698c09..a8edcbb0 100644 --- a/engine_dependson_plumbing_test.go +++ b/engine_dependson_plumbing_test.go @@ -1,6 +1,8 @@ package workflow import ( + "io" + "log/slog" "testing" "github.com/GoCodeAlone/modular" @@ -180,6 +182,105 @@ func TestEngine_BuildFromConfig_PlumbsDefensiveCopy(t *testing.T) { } } +// initOrderRecorderModule appends its name to a shared slice during Init(). +// Used by TestEngine_BuildFromConfig_RealModularHonoursDependsOn to assert +// the order modular's real Init walker fires modules in — proving that +// SetDependencies plumbed by BuildFromConfig actually drives modular's +// dependency-aware sort, not just that the recorded values look right. +type initOrderRecorderModule struct { + name string + deps []string + initOrder *[]string // shared pointer; appended to on Init +} + +func (m *initOrderRecorderModule) Name() string { return m.name } +func (m *initOrderRecorderModule) Init(_ modular.Application) error { + *m.initOrder = append(*m.initOrder, m.name) + return nil +} +func (m *initOrderRecorderModule) RegisterConfig(_ modular.Application) error { return nil } +func (m *initOrderRecorderModule) Dependencies() []string { return m.deps } +func (m *initOrderRecorderModule) ProvidesServices() []modular.ServiceProvider { return nil } +func (m *initOrderRecorderModule) RequiresServices() []modular.ServiceDependency { return nil } +func (m *initOrderRecorderModule) SetDependencies(deps []string) { + cp := make([]string, len(deps)) + copy(cp, deps) + m.deps = cp +} + +// stubConfigProvider is a minimal modular.ConfigProvider for NewStdApplication. +type stubConfigProvider struct{} + +func (stubConfigProvider) GetConfig() any { return nil } + +// TestEngine_BuildFromConfig_RealModularHonoursDependsOn proves the +// fix end-to-end against the *real* modular framework — not the test +// mockApplication whose Init() is a no-op. A consumer module declared +// FIRST in cfg.Modules with dependsOn:[broker] must initialise AFTER +// the broker, even though the broker is declared SECOND. This pins +// the actual production behaviour the BMW PR #279/#280 race depends on. +func TestEngine_BuildFromConfig_RealModularHonoursDependsOn(t *testing.T) { + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + app := modular.NewStdApplication(stubConfigProvider{}, logger) + engine := NewStdEngine(app, logger) + + var initOrder []string + engine.AddModuleType("test.depinit", func(name string, _ map[string]any) modular.Module { + return &initOrderRecorderModule{name: name, initOrder: &initOrder} + }) + + cfg := &config.WorkflowConfig{ + Modules: []config.ModuleConfig{ + // Consumer declared FIRST, depends on broker. + {Name: "consumer", Type: "test.depinit", DependsOn: []string{"broker"}}, + // Broker declared SECOND. + {Name: "broker", Type: "test.depinit"}, + }, + Workflows: map[string]any{}, + Triggers: map[string]any{}, + } + + if err := engine.BuildFromConfig(cfg); err != nil { + t.Fatalf("BuildFromConfig: %v", err) + } + + // Without the SetDependencies plumbing, modular's Init walker would + // see Dependencies()=nil for both modules and fall back to alphabetical + // (broker before consumer) — which would actually pass this assertion + // by coincidence. To prove the plumbing matters, declare the broker + // with a name that sorts AFTER the consumer alphabetically. With + // SetDependencies wired, modular still inits broker first (because + // consumer.Dependencies() now returns ["broker"]). Without it, modular + // would init "consumer" then "z-broker" (alphabetical) and the + // assertion would fail. + // + // Reset + re-run with that shape. + initOrder = initOrder[:0] + app2 := modular.NewStdApplication(stubConfigProvider{}, logger) + engine2 := NewStdEngine(app2, logger) + engine2.AddModuleType("test.depinit", func(name string, _ map[string]any) modular.Module { + return &initOrderRecorderModule{name: name, initOrder: &initOrder} + }) + cfg2 := &config.WorkflowConfig{ + Modules: []config.ModuleConfig{ + {Name: "consumer", Type: "test.depinit", DependsOn: []string{"z-broker"}}, + {Name: "z-broker", Type: "test.depinit"}, + }, + Workflows: map[string]any{}, + Triggers: map[string]any{}, + } + if err := engine2.BuildFromConfig(cfg2); err != nil { + t.Fatalf("BuildFromConfig (z-broker shape): %v", err) + } + if len(initOrder) != 2 { + t.Fatalf("initOrder = %v, want 2 entries", initOrder) + } + if initOrder[0] != "z-broker" || initOrder[1] != "consumer" { + t.Errorf("modular Init order = %v, want [z-broker consumer] (broker must precede dependent even when alphabetically later)", + initOrder) + } +} + // TestEngine_BuildFromConfig_SkipsModulesWithoutSetter pins that modules // not implementing the SetDependencies contract are silently skipped // (not panicked, not errored) even when modCfg.DependsOn is non-empty. diff --git a/plugin/external/remote_module.go b/plugin/external/remote_module.go index 5bcd1f48..af8c6466 100644 --- a/plugin/external/remote_module.go +++ b/plugin/external/remote_module.go @@ -70,8 +70,16 @@ func (m *RemoteModule) Dependencies() []string { // module so modular's Init() walker can honour them. Called by the engine // from BuildFromConfig once per module, immediately after the factory // returns and before app.RegisterModule. See workflow#663. +// +// Defensive copy: although the engine already copies before calling, the +// setter is exported and Dependencies() exposes the same backing array to +// modular. Copying here too means any caller (engine, tests, future +// integration paths) can mutate its source slice after calling without +// silently corrupting this module's init graph. func (m *RemoteModule) SetDependencies(deps []string) { - m.dependencies = deps + cp := make([]string, len(deps)) + copy(cp, deps) + m.dependencies = cp } func (m *RemoteModule) ProvidesServices() []string { From fb9d4ddbb5e7ded02d807b1faf0e1cfe99948520 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Wed, 13 May 2026 19:23:12 -0400 Subject: [PATCH 05/10] review fixes R4: aliasing test + direct DependencyAware assertion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Copilot R4 on PR #665: 1. Added TestRemoteModule_SetDependencies_DefensivelyCopies — mutates the caller's source slice after SetDependencies and asserts Dependencies() still returns the original values. Sanity-checked locally: temporarily removed the cp := make()/copy block in remote_module.go and the test correctly FAILS with "Dependencies() = [MUTATED b]". Restoring the copy makes it pass. This closes the regression window where the engine's pre-copy masks a missing setter-side copy from R3's review. 2. TestRemoteModule_ImplementsDependencyAware now asserts *RemoteModule satisfies modular.DependencyAware directly, not a structural-typed interface. plugin/external already imports modular (remote_module.go imports it for modular.Module + modular.Application), so the original "circular dep" rationale was misleading — Copilot caught it. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../remote_module_dependencies_test.go | 36 +++++++++++++++---- 1 file changed, 29 insertions(+), 7 deletions(-) diff --git a/plugin/external/remote_module_dependencies_test.go b/plugin/external/remote_module_dependencies_test.go index 933c9a26..4b821669 100644 --- a/plugin/external/remote_module_dependencies_test.go +++ b/plugin/external/remote_module_dependencies_test.go @@ -2,6 +2,8 @@ package external import ( "testing" + + "github.com/GoCodeAlone/modular" ) // TestRemoteModule_Dependencies_DefaultsToNil pins that a freshly-constructed @@ -59,15 +61,35 @@ func TestRemoteModule_SetDependencies_Overwrites(t *testing.T) { } } +// TestRemoteModule_SetDependencies_DefensivelyCopies pins the setter-side +// defensive copy: mutating the caller's slice after SetDependencies must +// not change what Dependencies() returns. Without the copy, the engine +// stores its own pre-copied slice safely, but other callers (tests, +// future integration paths) hold a live reference to the module's +// dependency graph. Aliasing through that reference would silently +// corrupt modular's init order. +func TestRemoteModule_SetDependencies_DefensivelyCopies(t *testing.T) { + m := &RemoteModule{name: "x"} + src := []string{"a", "b"} + m.SetDependencies(src) + + // Mutate the source slice. The module must not see the mutation. + src[0] = "MUTATED" + + got := m.Dependencies() + if len(got) != 2 || got[0] != "a" || got[1] != "b" { + t.Errorf("Dependencies() = %v after caller-side mutation; want [a b] — SetDependencies did NOT defensively copy", got) + } +} + // TestRemoteModule_ImplementsDependencyAware pins that *RemoteModule -// satisfies a structurally-typed interface matching modular.DependencyAware. -// We can't import modular's interface here without a circular dep, but -// the structural check is sufficient — modular itself does the same -// type assertion at init time. +// satisfies modular.DependencyAware (the actual interface modular's +// Init() walker type-asserts against). A regression that broke this +// satisfaction would silently drop external-plugin modules out of +// modular's dependency-aware sort and re-introduce the workflow#663 +// alphabetical-init race. func TestRemoteModule_ImplementsDependencyAware(t *testing.T) { - var _ interface { - Dependencies() []string - } = (*RemoteModule)(nil) + var _ modular.DependencyAware = (*RemoteModule)(nil) } // TestRemoteModule_ImplementsDependencyTargetInterface pins the structural From 448463cab7305c9e6cf3bf8f8e8e6a2d8b79e743 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Wed, 13 May 2026 19:29:17 -0400 Subject: [PATCH 06/10] =?UTF-8?q?review=20fixes=20R5:=20CHANGELOG=20test?= =?UTF-8?q?=20count=206=20=E2=86=92=207=20(defensive-copy=20aliasing)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot R5 caught: CHANGELOG said 6 RemoteModule unit tests but the file now contains 7 after R4 added TestRemoteModule_SetDependencies_DefensivelyCopies. Updated the count + the descriptor to mention the new aliasing test and reflect that one of the type-assertion pins is now against the real modular.DependencyAware interface rather than a structural one. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 50f1badc..236ba84b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,7 +25,7 @@ Configs that still reference the legacy types now fail to load with an actionabl ### Fixed (issue #663 — follow-up) -- **`*external.RemoteModule.Dependencies()` now returns the yaml-level `dependsOn:` keys** instead of always returning `nil`. The v0.51.8 fix (PR #664) only reordered the `cfg.Modules` slice — but modular's `app.Init()` then runs its own `DependencyAware`-driven sort over the registered modules, and `RemoteModule` (the wrapper used for every external-plugin module) returned `nil` from `Dependencies()`, so modular saw every external-plugin module as a root and sorted alphabetically. BMW PR #280 image-launch surfaced this as the same `bmw-eventbus`/`bmw-stream` ordering race that v0.51.8 was supposed to close. Engine `BuildFromConfig` now calls `SetDependencies(modCfg.DependsOn)` on each module that **declares `dependsOn:` in yaml AND implements** `interface{ SetDependencies([]string) }` immediately after the factory returns, before `app.RegisterModule`. (Modules with empty `dependsOn:` are skipped — calling `SetDependencies(nil)` would clear any defaults a custom module set up in its constructor.) `RemoteModule` implements that setter, defensively copies the slice, and modular's Init() walker then reads it via the existing `Dependencies()` contract. 6 unit tests cover the `RemoteModule` contract (default-nil, plumb, empty-slice, overwrite, two structural type-assertion pins) plus 4 engine-level `BuildFromConfig` tests covering the production path (basic plumb + defensive copy via raw-slice recorder + back-compat skip + real-modular Init order). Built-in modules can opt in by implementing the same setter; existing behaviour is unchanged for modules that don't. +- **`*external.RemoteModule.Dependencies()` now returns the yaml-level `dependsOn:` keys** instead of always returning `nil`. The v0.51.8 fix (PR #664) only reordered the `cfg.Modules` slice — but modular's `app.Init()` then runs its own `DependencyAware`-driven sort over the registered modules, and `RemoteModule` (the wrapper used for every external-plugin module) returned `nil` from `Dependencies()`, so modular saw every external-plugin module as a root and sorted alphabetically. BMW PR #280 image-launch surfaced this as the same `bmw-eventbus`/`bmw-stream` ordering race that v0.51.8 was supposed to close. Engine `BuildFromConfig` now calls `SetDependencies(modCfg.DependsOn)` on each module that **declares `dependsOn:` in yaml AND implements** `interface{ SetDependencies([]string) }` immediately after the factory returns, before `app.RegisterModule`. (Modules with empty `dependsOn:` are skipped — calling `SetDependencies(nil)` would clear any defaults a custom module set up in its constructor.) `RemoteModule` implements that setter, defensively copies the slice, and modular's Init() walker then reads it via the existing `Dependencies()` contract. 7 unit tests cover the `RemoteModule` contract (default-nil, plumb, empty-slice, overwrite, defensive-copy aliasing, plus two type-assertion pins for `modular.DependencyAware` and the engine's `SetDependencies` interface) plus 4 engine-level `BuildFromConfig` tests covering the production path (basic plumb + defensive copy via raw-slice recorder + back-compat skip + real-modular Init order). Built-in modules can opt in by implementing the same setter; existing behaviour is unchanged for modules that don't. ### Fixed (issue #663) From 212ffa0b103602582d0d3a5e0d13219359adb5a4 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Wed, 13 May 2026 19:35:33 -0400 Subject: [PATCH 07/10] review fix R6: SetDependencies docstring matches actual call path Copilot R6 caught: the SetDependencies docstring said the engine calls it once per module, but BuildFromConfig only calls it when modCfg.DependsOn is non-empty AND the module satisfies the setter interface. Updated the docstring to spell out both conditions and the rationale (constructor-time defaults must not be clobbered by SetDependencies(nil)). Co-Authored-By: Claude Opus 4.7 (1M context) --- plugin/external/remote_module.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/plugin/external/remote_module.go b/plugin/external/remote_module.go index af8c6466..8128dc72 100644 --- a/plugin/external/remote_module.go +++ b/plugin/external/remote_module.go @@ -68,8 +68,11 @@ func (m *RemoteModule) Dependencies() []string { // SetDependencies records the yaml-level `dependsOn:` keys declared for this // module so modular's Init() walker can honour them. Called by the engine -// from BuildFromConfig once per module, immediately after the factory -// returns and before app.RegisterModule. See workflow#663. +// from BuildFromConfig immediately after the factory returns and before +// app.RegisterModule, but only when the module's `modCfg.DependsOn` is +// non-empty AND the module satisfies `interface{ SetDependencies([]string) }` +// — modules with no declared dependsOn are skipped so a constructor-time +// default isn't clobbered with a SetDependencies(nil) call. See workflow#663. // // Defensive copy: although the engine already copies before calling, the // setter is exported and Dependencies() exposes the same backing array to From a096a677a513dccc51328f90afe3b5c41e4a72e8 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Wed, 13 May 2026 19:47:34 -0400 Subject: [PATCH 08/10] review fix R7: filter empty + unknown deps before SetDependencies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot R7 caught: the engine forwarded the raw modCfg.DependsOn slice into SetDependencies even though topoSortModules intentionally ignored empty strings and dependency names not present in cfg.Modules. modular's DependencyAware sort would then see unresolvable entries — either mis-ordering modules or panicking depending on modular version. Schema validation rejects empty + unknown deps for declared modules, but ConfigTransformHooks can inject modules whose dependsOn was never schema-checked — this is the engine's defensive boundary against that. Two-part fix: 1. engine_module_order.go: new filterResolvableDeps helper that drops empty strings + names not in moduleNames, preserving input order. Same filter as topoSortModules' internal edge construction. 2. engine.go: BuildFromConfig builds a moduleNameSet once after topoSortModules + uses filterResolvableDeps before each SetDependencies call. If the filtered slice is empty (everything was unresolvable or empty), the setter is skipped entirely — same back-compat surface as a module declared with no dependsOn. 4 unit tests covering filterResolvableDeps directly (drops-empty, all-unknown, empty-input, order-preserved). Going through BuildFromConfig isn't possible without registering a config transform hook (schema rejects the bad inputs upstream); the helper-level tests are the practical surface. Co-Authored-By: Claude Opus 4.7 (1M context) --- engine.go | 27 +++++++++++++++++++++------ engine_module_order.go | 24 ++++++++++++++++++++++++ engine_module_order_test.go | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 79 insertions(+), 6 deletions(-) diff --git a/engine.go b/engine.go index 3aacdb7d..c649ac3c 100644 --- a/engine.go +++ b/engine.go @@ -509,6 +509,20 @@ func (e *StdEngine) BuildFromConfig(cfg *config.WorkflowConfig) error { } cfg.Modules = orderedModules + // Build the resolvable-dependency name set so the SetDependencies plumbing + // below can filter raw modCfg.DependsOn the same way topoSortModules + // filters its internal edges. Without this filter, the engine would forward + // empty-string entries and unknown names — both intentionally ignored by + // topoSortModules — straight into modular's DependencyAware sort, which + // would then either misorder (unknown names treated as future deps that + // never resolve) or panic (depending on the modular framework version). + moduleNameSet := make(map[string]struct{}, len(cfg.Modules)) + for _, m := range cfg.Modules { + if m.Name != "" { + moduleNameSet[m.Name] = struct{}{} + } + } + // Compute config hash after transform hooks + dependency ordering so the // hash reflects the effective runtime config (hooks may mutate cfg, and // topoSortModules may reorder cfg.Modules, before modules are registered). @@ -563,12 +577,13 @@ func (e *StdEngine) BuildFromConfig(cfg *config.WorkflowConfig) error { // runtime state that module B's Init() needed. The topoSortModules // pass above already reordered cfg.Modules; this hands the same // information to modular so its own initialisation graph agrees - // with engine-level ordering. - if len(modCfg.DependsOn) > 0 { - if depTarget, ok := mod.(interface{ SetDependencies([]string) }); ok { - deps := make([]string, len(modCfg.DependsOn)) - copy(deps, modCfg.DependsOn) - depTarget.SetDependencies(deps) + // with engine-level ordering. filterResolvableDeps drops empty + // entries + unknown names so the slice modular sees matches the + // edge set topoSortModules used. + if depTarget, ok := mod.(interface{ SetDependencies([]string) }); ok { + filtered := filterResolvableDeps(modCfg.DependsOn, moduleNameSet) + if len(filtered) > 0 { + depTarget.SetDependencies(filtered) } } diff --git a/engine_module_order.go b/engine_module_order.go index b88da19c..8320f9a3 100644 --- a/engine_module_order.go +++ b/engine_module_order.go @@ -127,3 +127,27 @@ func (h *intHeap) Pop() any { *h = old[:n-1] return x } + +// filterResolvableDeps returns a new slice containing only entries from deps +// that are present in the moduleNames set, in the same order. Empty strings +// and names not in moduleNames are dropped. Used by engine BuildFromConfig +// before calling SetDependencies on a module, so modular's DependencyAware +// sort sees the same edge set that topoSortModules used when ordering +// cfg.Modules (both ignore unresolvable + empty entries). +// +// Schema validation rejects empty + unknown dependsOn entries for declared +// modules, but ConfigTransformHooks can inject modules whose dependsOn was +// never validated. This is the engine's defensive boundary against that. +func filterResolvableDeps(deps []string, moduleNames map[string]struct{}) []string { + out := make([]string, 0, len(deps)) + for _, dep := range deps { + if dep == "" { + continue + } + if _, exists := moduleNames[dep]; !exists { + continue + } + out = append(out, dep) + } + return out +} diff --git a/engine_module_order_test.go b/engine_module_order_test.go index 5acefaf7..930be5f9 100644 --- a/engine_module_order_test.go +++ b/engine_module_order_test.go @@ -254,6 +254,40 @@ func TestTopoSortModules_DuplicateNames_FirstWins(t *testing.T) { } } +func TestFilterResolvableDeps_DropsEmptyAndUnknown(t *testing.T) { + names := map[string]struct{}{"a": {}, "b": {}, "c": {}} + got := filterResolvableDeps([]string{"a", "", "ghost", "b", "phantom", "c"}, names) + want := []string{"a", "b", "c"} + if !equalSlice(got, want) { + t.Errorf("filterResolvableDeps = %v, want %v", got, want) + } +} + +func TestFilterResolvableDeps_AllUnknownReturnsEmpty(t *testing.T) { + names := map[string]struct{}{"only-this": {}} + got := filterResolvableDeps([]string{"x", "y", "z", ""}, names) + if len(got) != 0 { + t.Errorf("filterResolvableDeps all-unknown = %v, want empty", got) + } +} + +func TestFilterResolvableDeps_EmptyInputEmptyOutput(t *testing.T) { + got := filterResolvableDeps(nil, map[string]struct{}{"a": {}}) + if len(got) != 0 { + t.Errorf("filterResolvableDeps(nil) = %v, want empty", got) + } +} + +func TestFilterResolvableDeps_PreservesOrder(t *testing.T) { + // Order in input must be preserved in output. + names := map[string]struct{}{"a": {}, "b": {}, "c": {}} + got := filterResolvableDeps([]string{"c", "a", "b"}, names) + want := []string{"c", "a", "b"} + if !equalSlice(got, want) { + t.Errorf("filterResolvableDeps = %v, want %v (order preserved)", got, want) + } +} + func TestTopoSortModules_EmptyDependencyStringIgnored(t *testing.T) { // Schema validation rejects "" entries, but defensively the sort should // not blow up if one slips through (e.g., from a hand-built ModuleConfig From 78d77189456edf1c2d42117a7aa81d5cc031610b Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Wed, 13 May 2026 19:55:12 -0400 Subject: [PATCH 09/10] review fix R8: comment drift after R7 filter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot R8 caught 4 wording-drift sites where comments still described the pre-R7 plumb behaviour ("calls SetDependencies on each module that implements it") instead of the post-R7 reality ("calls only when filterResolvableDeps leaves at least one resolvable name"): - CHANGELOG.md: spell out filterResolvableDeps role + the non-empty-after-filter precondition. - plugin/external/remote_module.go: SetDependencies docstring now mentions filterResolvableDeps + the no-resolvable-dep skip path. - plugin/external/remote_module_dependencies_test.go: TestRemoteModule_SetDependencies_Overwrites comment says "at most once per BuildFromConfig" + spells out the filter precondition. - engine_dependson_plumbing_test.go: TestEngine_BuildFromConfig_PlumbsDependsOnIntoModule comment narrows to the setter-implementing + has-resolvable-dep case. No code changes — only doc accuracy. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 2 +- engine_dependson_plumbing_test.go | 14 ++++++++------ plugin/external/remote_module.go | 8 +++++--- plugin/external/remote_module_dependencies_test.go | 9 ++++++--- 4 files changed, 20 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 236ba84b..e9f8cd0e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,7 +25,7 @@ Configs that still reference the legacy types now fail to load with an actionabl ### Fixed (issue #663 — follow-up) -- **`*external.RemoteModule.Dependencies()` now returns the yaml-level `dependsOn:` keys** instead of always returning `nil`. The v0.51.8 fix (PR #664) only reordered the `cfg.Modules` slice — but modular's `app.Init()` then runs its own `DependencyAware`-driven sort over the registered modules, and `RemoteModule` (the wrapper used for every external-plugin module) returned `nil` from `Dependencies()`, so modular saw every external-plugin module as a root and sorted alphabetically. BMW PR #280 image-launch surfaced this as the same `bmw-eventbus`/`bmw-stream` ordering race that v0.51.8 was supposed to close. Engine `BuildFromConfig` now calls `SetDependencies(modCfg.DependsOn)` on each module that **declares `dependsOn:` in yaml AND implements** `interface{ SetDependencies([]string) }` immediately after the factory returns, before `app.RegisterModule`. (Modules with empty `dependsOn:` are skipped — calling `SetDependencies(nil)` would clear any defaults a custom module set up in its constructor.) `RemoteModule` implements that setter, defensively copies the slice, and modular's Init() walker then reads it via the existing `Dependencies()` contract. 7 unit tests cover the `RemoteModule` contract (default-nil, plumb, empty-slice, overwrite, defensive-copy aliasing, plus two type-assertion pins for `modular.DependencyAware` and the engine's `SetDependencies` interface) plus 4 engine-level `BuildFromConfig` tests covering the production path (basic plumb + defensive copy via raw-slice recorder + back-compat skip + real-modular Init order). Built-in modules can opt in by implementing the same setter; existing behaviour is unchanged for modules that don't. +- **`*external.RemoteModule.Dependencies()` now returns the yaml-level `dependsOn:` keys** instead of always returning `nil`. The v0.51.8 fix (PR #664) only reordered the `cfg.Modules` slice — but modular's `app.Init()` then runs its own `DependencyAware`-driven sort over the registered modules, and `RemoteModule` (the wrapper used for every external-plugin module) returned `nil` from `Dependencies()`, so modular saw every external-plugin module as a root and sorted alphabetically. BMW PR #280 image-launch surfaced this as the same `bmw-eventbus`/`bmw-stream` ordering race that v0.51.8 was supposed to close. Engine `BuildFromConfig` now filters `modCfg.DependsOn` through `filterResolvableDeps` (drops empty strings + names not present in `cfg.Modules` — the same edge-set topoSortModules used for ordering) and calls `SetDependencies(filtered)` on each module that **implements** `interface{ SetDependencies([]string) }`, but **only when the filtered slice is non-empty**, immediately after the factory returns and before `app.RegisterModule`. (Modules with no resolvable dependsOn — empty yaml + transform-injected modules whose dependsOn is all empty/ghost — are skipped, so a constructor-time default isn't clobbered with `SetDependencies(nil)`.) `RemoteModule` implements that setter, defensively copies the slice, and modular's Init() walker then reads it via the existing `Dependencies()` contract. 7 unit tests cover the `RemoteModule` contract (default-nil, plumb, empty-slice, overwrite, defensive-copy aliasing, plus two type-assertion pins for `modular.DependencyAware` and the engine's `SetDependencies` interface) plus 4 engine-level `BuildFromConfig` tests covering the production path (basic plumb + defensive copy via raw-slice recorder + back-compat skip + real-modular Init order). Built-in modules can opt in by implementing the same setter; existing behaviour is unchanged for modules that don't. ### Fixed (issue #663) diff --git a/engine_dependson_plumbing_test.go b/engine_dependson_plumbing_test.go index a8edcbb0..9ea1457e 100644 --- a/engine_dependson_plumbing_test.go +++ b/engine_dependson_plumbing_test.go @@ -71,12 +71,14 @@ func (m *fakePlainModule) ProvidesServices() []modular.ServiceProvider { retur func (m *fakePlainModule) RequiresServices() []modular.ServiceDependency { return nil } // TestEngine_BuildFromConfig_PlumbsDependsOnIntoModule pins the production -// path that closes workflow#663: BuildFromConfig must call SetDependencies -// on each module that implements it, with a defensive copy of -// modCfg.DependsOn, before app.RegisterModule. Without this end-to-end -// test, a future refactor that moves the structural type assertion out -// of the registration loop (or changes the contract) would silently -// regress the BMW PR #279/280 fix and only image-launch CI would catch it. +// path that closes workflow#663: for a module that implements the +// SetDependencies setter AND has at least one resolvable dependsOn entry +// (after filterResolvableDeps drops empties + unknown names), +// BuildFromConfig must call SetDependencies with a defensive copy of the +// filtered slice before app.RegisterModule. Without this end-to-end test, +// a future refactor that moves the structural type assertion out of the +// registration loop (or changes the contract) would silently regress the +// BMW PR #279/280 fix and only image-launch CI would catch it. func TestEngine_BuildFromConfig_PlumbsDependsOnIntoModule(t *testing.T) { app := newMockApplication() engine := NewStdEngine(app, app.Logger()) diff --git a/plugin/external/remote_module.go b/plugin/external/remote_module.go index 8128dc72..cca5c304 100644 --- a/plugin/external/remote_module.go +++ b/plugin/external/remote_module.go @@ -70,9 +70,11 @@ func (m *RemoteModule) Dependencies() []string { // module so modular's Init() walker can honour them. Called by the engine // from BuildFromConfig immediately after the factory returns and before // app.RegisterModule, but only when the module's `modCfg.DependsOn` is -// non-empty AND the module satisfies `interface{ SetDependencies([]string) }` -// — modules with no declared dependsOn are skipped so a constructor-time -// default isn't clobbered with a SetDependencies(nil) call. See workflow#663. +// **non-empty after filtering** (filterResolvableDeps drops empty strings +// + names not present in cfg.Modules) AND the module satisfies +// `interface{ SetDependencies([]string) }` — modules with no resolvable +// dependsOn are skipped so a constructor-time default isn't clobbered with +// a SetDependencies(nil) call. See workflow#663. // // Defensive copy: although the engine already copies before calling, the // setter is exported and Dependencies() exposes the same backing array to diff --git a/plugin/external/remote_module_dependencies_test.go b/plugin/external/remote_module_dependencies_test.go index 4b821669..99280fd0 100644 --- a/plugin/external/remote_module_dependencies_test.go +++ b/plugin/external/remote_module_dependencies_test.go @@ -48,9 +48,12 @@ func TestRemoteModule_SetDependencies_EmptySliceIsEmpty(t *testing.T) { // TestRemoteModule_SetDependencies_Overwrites pins that a second call // replaces the previous slice rather than appending. The engine calls -// SetDependencies exactly once per module per BuildFromConfig, but -// pinning the contract guards against future engine-side double-calls -// (e.g., a config-transform hook that mutates dependsOn post-registration). +// SetDependencies at most once per BuildFromConfig — only on modules +// that implement the setter AND have at least one resolvable dependsOn +// entry after filterResolvableDeps drops empties + unknown names. But +// pinning the overwrite contract guards against future engine-side +// double-calls (e.g., a config-transform hook that mutates dependsOn +// post-registration and re-fires the plumb). func TestRemoteModule_SetDependencies_Overwrites(t *testing.T) { m := &RemoteModule{name: "x"} m.SetDependencies([]string{"a", "b"}) From a55b13b9ea1c5df02f2e3860c8e456b478506759 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Wed, 13 May 2026 20:04:14 -0400 Subject: [PATCH 10/10] review fix R9: filter self-dependencies + detect single-module self-cycle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot R9 caught the self-dependency edge case: 1. Schema validation does NOT reject a module declaring itself in dependsOn (only checks the dep name exists, which "self" trivially does). 2. topoSortModules' n<=1 early return short-circuited before cycle detection, so a single-module config with a self-dep slipped past too. 3. filterResolvableDeps preserved the self-name in the filtered slice (the name resolves to itself), and the engine handed it to modular's DependencyAware sort which would either treat it as a 1-cycle or stall. Three-part fix: - engine_module_order.go: filterResolvableDeps now takes a `selfName` parameter and drops any dep matching it. Comment explains why the schema gap matters. - engine_module_order.go: topoSortModules splits the n<=1 path — n==0 still short-circuits, n==1 explicitly checks for a self-dep and returns the same cycle error format Kahn produces for n>=2. - engine.go: passes modCfg.Name as selfName to filterResolvableDeps. 3 new unit tests: - TestFilterResolvableDeps_DropsSelfReference — module "me" depending on ["me", "you"] filters to ["you"]. - TestFilterResolvableDeps_OnlySelfReferenceReturnsEmpty — module "me" depending only on itself returns empty (engine then skips SetDependencies, preserving constructor defaults). - TestTopoSortModules_SingleModuleSelfDepIsCycle — single module declaring itself errors with the cycle message. - TestTopoSortModules_SingleModuleNoSelfDepIsClean — sanity that n==1 without self-dep still passes through. Existing 4 filterResolvableDeps tests updated to pass the new selfName arg ("_test_self_" — non-matching sentinel). Co-Authored-By: Claude Opus 4.7 (1M context) --- engine.go | 2 +- engine_module_order.go | 37 +++++++++++++++++------ engine_module_order_test.go | 59 ++++++++++++++++++++++++++++++++++--- 3 files changed, 84 insertions(+), 14 deletions(-) diff --git a/engine.go b/engine.go index c649ac3c..47f28e44 100644 --- a/engine.go +++ b/engine.go @@ -581,7 +581,7 @@ func (e *StdEngine) BuildFromConfig(cfg *config.WorkflowConfig) error { // entries + unknown names so the slice modular sees matches the // edge set topoSortModules used. if depTarget, ok := mod.(interface{ SetDependencies([]string) }); ok { - filtered := filterResolvableDeps(modCfg.DependsOn, moduleNameSet) + filtered := filterResolvableDeps(modCfg.DependsOn, moduleNameSet, modCfg.Name) if len(filtered) > 0 { depTarget.SetDependencies(filtered) } diff --git a/engine_module_order.go b/engine_module_order.go index 8320f9a3..cbdd8a69 100644 --- a/engine_module_order.go +++ b/engine_module_order.go @@ -28,7 +28,20 @@ import ( // the dependent looks up its parent in a plugin-local registry. func topoSortModules(modules []config.ModuleConfig) ([]config.ModuleConfig, error) { n := len(modules) - if n <= 1 { + if n == 0 { + return modules, nil + } + if n == 1 { + // Single module can still self-reference, which the n>=2 path + // catches via Kahn's cycle detection. Detect it explicitly here. + for _, dep := range modules[0].DependsOn { + if dep == modules[0].Name { + return nil, fmt.Errorf( + "module dependsOn cycle (or dependents of one) among: %s", + modules[0].Name, + ) + } + } return modules, nil } @@ -129,21 +142,27 @@ func (h *intHeap) Pop() any { } // filterResolvableDeps returns a new slice containing only entries from deps -// that are present in the moduleNames set, in the same order. Empty strings -// and names not in moduleNames are dropped. Used by engine BuildFromConfig -// before calling SetDependencies on a module, so modular's DependencyAware -// sort sees the same edge set that topoSortModules used when ordering -// cfg.Modules (both ignore unresolvable + empty entries). +// that are present in the moduleNames set AND are not equal to selfName, in +// the same order. Empty strings, names not in moduleNames, and self-references +// are dropped. Used by engine BuildFromConfig before calling SetDependencies +// on a module, so modular's DependencyAware sort sees the same edge set that +// topoSortModules used when ordering cfg.Modules. // // Schema validation rejects empty + unknown dependsOn entries for declared -// modules, but ConfigTransformHooks can inject modules whose dependsOn was -// never validated. This is the engine's defensive boundary against that. -func filterResolvableDeps(deps []string, moduleNames map[string]struct{}) []string { +// modules, but does not reject a module declaring itself in dependsOn — and +// ConfigTransformHooks can inject modules whose dependsOn was never validated. +// This is the engine's defensive boundary against both: stripping the +// self-reference here means modular can't either treat it as a 1-cycle or +// stall waiting for the module to initialise itself before itself. +func filterResolvableDeps(deps []string, moduleNames map[string]struct{}, selfName string) []string { out := make([]string, 0, len(deps)) for _, dep := range deps { if dep == "" { continue } + if dep == selfName { + continue + } if _, exists := moduleNames[dep]; !exists { continue } diff --git a/engine_module_order_test.go b/engine_module_order_test.go index 930be5f9..765829e5 100644 --- a/engine_module_order_test.go +++ b/engine_module_order_test.go @@ -256,7 +256,7 @@ func TestTopoSortModules_DuplicateNames_FirstWins(t *testing.T) { func TestFilterResolvableDeps_DropsEmptyAndUnknown(t *testing.T) { names := map[string]struct{}{"a": {}, "b": {}, "c": {}} - got := filterResolvableDeps([]string{"a", "", "ghost", "b", "phantom", "c"}, names) + got := filterResolvableDeps([]string{"a", "", "ghost", "b", "phantom", "c"}, names, "_test_self_") want := []string{"a", "b", "c"} if !equalSlice(got, want) { t.Errorf("filterResolvableDeps = %v, want %v", got, want) @@ -265,14 +265,14 @@ func TestFilterResolvableDeps_DropsEmptyAndUnknown(t *testing.T) { func TestFilterResolvableDeps_AllUnknownReturnsEmpty(t *testing.T) { names := map[string]struct{}{"only-this": {}} - got := filterResolvableDeps([]string{"x", "y", "z", ""}, names) + got := filterResolvableDeps([]string{"x", "y", "z", ""}, names, "_test_self_") if len(got) != 0 { t.Errorf("filterResolvableDeps all-unknown = %v, want empty", got) } } func TestFilterResolvableDeps_EmptyInputEmptyOutput(t *testing.T) { - got := filterResolvableDeps(nil, map[string]struct{}{"a": {}}) + got := filterResolvableDeps(nil, map[string]struct{}{"a": {}}, "_test_self_") if len(got) != 0 { t.Errorf("filterResolvableDeps(nil) = %v, want empty", got) } @@ -281,13 +281,64 @@ func TestFilterResolvableDeps_EmptyInputEmptyOutput(t *testing.T) { func TestFilterResolvableDeps_PreservesOrder(t *testing.T) { // Order in input must be preserved in output. names := map[string]struct{}{"a": {}, "b": {}, "c": {}} - got := filterResolvableDeps([]string{"c", "a", "b"}, names) + got := filterResolvableDeps([]string{"c", "a", "b"}, names, "_test_self_") want := []string{"c", "a", "b"} if !equalSlice(got, want) { t.Errorf("filterResolvableDeps = %v, want %v (order preserved)", got, want) } } +func TestFilterResolvableDeps_DropsSelfReference(t *testing.T) { + // Schema validation does NOT reject a module declaring itself in + // dependsOn — and modular's DependencyAware sort would treat it as + // a 1-cycle (or stall, depending on framework version). Filter it. + names := map[string]struct{}{"me": {}, "you": {}} + got := filterResolvableDeps([]string{"me", "you"}, names, "me") + want := []string{"you"} + if !equalSlice(got, want) { + t.Errorf("filterResolvableDeps with self-reference = %v, want %v", got, want) + } +} + +func TestFilterResolvableDeps_OnlySelfReferenceReturnsEmpty(t *testing.T) { + // Module declaring only itself → engine should skip SetDependencies + // entirely (filtered slice is empty). + names := map[string]struct{}{"me": {}} + got := filterResolvableDeps([]string{"me", "me", "me"}, names, "me") + if len(got) != 0 { + t.Errorf("filterResolvableDeps only-self = %v, want empty", got) + } +} + +func TestTopoSortModules_SingleModuleSelfDepIsCycle(t *testing.T) { + // n==1 path used to short-circuit before cycle detection. A single + // module declaring itself in dependsOn IS a 1-cycle and must error. + in := []config.ModuleConfig{ + withDeps(config.ModuleConfig{Name: "self", Type: "x"}, "self"), + } + _, err := topoSortModules(in) + if err == nil { + t.Fatalf("expected cycle error for single-module self-dep, got nil") + } + if !strings.Contains(err.Error(), "cycle") || !strings.Contains(err.Error(), "self") { + t.Errorf("error %q should mention cycle + module name 'self'", err.Error()) + } +} + +func TestTopoSortModules_SingleModuleNoSelfDepIsClean(t *testing.T) { + // Sanity: n==1 with no self-dep still passes through cleanly. + in := []config.ModuleConfig{ + {Name: "lonely", Type: "x"}, + } + out, err := topoSortModules(in) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(out) != 1 || out[0].Name != "lonely" { + t.Errorf("singleton-no-self order = %v, want [lonely]", order(out)) + } +} + func TestTopoSortModules_EmptyDependencyStringIgnored(t *testing.T) { // Schema validation rejects "" entries, but defensively the sort should // not blow up if one slips through (e.g., from a hand-built ModuleConfig