fix: address review comments on dynamic config system

- HasNonModuleChanges now also compares Requires (plugin/capability changes)
- DatabaseSource.Hash() fallback bypasses cache to ensure accurate change detection
- DatabasePoller.Stop() and ConfigWatcher.Stop() are idempotent via sync.Once
- ConfigWatcher handles fsnotify.Rename events for atomic-save editor compatibility
- ConfigReloader falls back to full reload when reconfigurer is nil but modules changed
- Add SetReconfigurer() for updating the reconfigurer after a full engine reload
- serverApp implements ModuleReconfigurer as stable adapter (survives engine reloads)
- Module reconfigure endpoint uses proper JSON encoding, 1MiB body limit, and RequireAuth
- Add tests for nil reconfigurer fallback and SetReconfigurer

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: intel352

cmd/server/main.go

@@ -347,6 +347,12 @@ type serverApp struct {
 	currentConfig  *config.WorkflowConfig // last loaded config, used by dynamic config watcher
 }
 
+// ReconfigureModules delegates to the current engine, ensuring the reloader
+// always targets the active engine even after a full reload replaces it.
+func (app *serverApp) ReconfigureModules(ctx context.Context, changes []config.ModuleConfigChange) ([]string, error) {
+	return app.engine.ReconfigureModules(ctx, changes)
+}
+
 // setup initializes all server components: engine, AI services, and HTTP mux.
 func setup(logger *slog.Logger, cfg *config.WorkflowConfig) (*serverApp, error) {
 	app := &serverApp{
@@ -1232,9 +1238,9 @@ func run(ctx context.Context, app *serverApp, listenAddr string) error {
 
 		var reloaderErr error
 		reloader, reloaderErr = config.NewConfigReloader(
-			app.currentConfig,   // the loaded WorkflowConfig
-			app.reloadEngine,    // existing full reload function
-			app.engine,          // implements ModuleReconfigurer
+			app.currentConfig, // the loaded WorkflowConfig
+			app.reloadEngine,  // existing full reload function
+			app,               // stable adapter — delegates to current app.engine
 			app.logger,
 		)
 		if reloaderErr != nil {
@@ -1269,7 +1275,7 @@ func run(ctx context.Context, app *serverApp, listenAddr string) error {
 		// Reuse or create a reloader for the DB poller.
 		if reloader == nil {
 			var reloaderErr error
-			reloader, reloaderErr = config.NewConfigReloader(app.currentConfig, app.reloadEngine, app.engine, app.logger)
+			reloader, reloaderErr = config.NewConfigReloader(app.currentConfig, app.reloadEngine, app, app.logger)
 			if reloaderErr != nil {
 				app.logger.Error("Failed to create config reloader for DB poller", "error", reloaderErr)
 			}
@@ -1590,40 +1596,56 @@ func runMultiWorkflow(logger *slog.Logger) error {
 
 	// Module reconfiguration endpoint — allows runtime hot-reload of individual
 	// modules that implement interfaces.Reconfigurable without a full engine restart.
-	mux.HandleFunc("PUT /api/v1/modules/{name}/config", func(w http.ResponseWriter, r *http.Request) {
+	// Wrapped with the same RequireAuth middleware used by the API router.
+	reconfigPerms := apihandler.NewPermissionService(stores.Memberships, stores.Workflows, stores.Projects)
+	reconfigMw := apihandler.NewMiddleware([]byte(secret), stores.Users, reconfigPerms)
+	mux.Handle("PUT /api/v1/modules/{name}/config", reconfigMw.RequireAuth(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		moduleName := r.PathValue("name")
 		if moduleName == "" {
-			http.Error(w, `{"error":"module name required"}`, http.StatusBadRequest)
+			w.Header().Set("Content-Type", "application/json")
+			w.WriteHeader(http.StatusBadRequest)
+			_ = json.NewEncoder(w).Encode(map[string]string{"error": "module name required"})
 			return
 		}
 
+		const maxConfigBytes = 1 << 20 // 1 MiB
+		limitedBody := http.MaxBytesReader(w, r.Body, maxConfigBytes)
+		defer limitedBody.Close() //nolint:errcheck
+
 		var newConfig map[string]any
-		if err := json.NewDecoder(r.Body).Decode(&newConfig); err != nil {
-			http.Error(w, fmt.Sprintf(`{"error":"invalid JSON: %v"}`, err), http.StatusBadRequest)
+		if err := json.NewDecoder(limitedBody).Decode(&newConfig); err != nil {
+			w.Header().Set("Content-Type", "application/json")
+			w.WriteHeader(http.StatusBadRequest)
+			_ = json.NewEncoder(w).Encode(map[string]string{"error": fmt.Sprintf("invalid JSON: %v", err)})
 			return
 		}
 
 		mod := app.engine.GetApp().GetModule(moduleName)
 		if mod == nil {
-			http.Error(w, fmt.Sprintf(`{"error":"module %q not found"}`, moduleName), http.StatusNotFound)
+			w.Header().Set("Content-Type", "application/json")
+			w.WriteHeader(http.StatusNotFound)
+			_ = json.NewEncoder(w).Encode(map[string]string{"error": fmt.Sprintf("module %q not found", moduleName)})
 			return
 		}
 
 		reconf, ok := mod.(interfaces.Reconfigurable)
 		if !ok {
-			http.Error(w, fmt.Sprintf(`{"error":"module %q does not support runtime reconfiguration"}`, moduleName), http.StatusNotImplemented)
+			w.Header().Set("Content-Type", "application/json")
+			w.WriteHeader(http.StatusNotImplemented)
+			_ = json.NewEncoder(w).Encode(map[string]string{"error": fmt.Sprintf("module %q does not support runtime reconfiguration", moduleName)})
 			return
 		}
 
 		if err := reconf.Reconfigure(r.Context(), newConfig); err != nil {
-			http.Error(w, fmt.Sprintf(`{"error":"reconfiguration failed: %v"}`, err), http.StatusInternalServerError)
+			w.Header().Set("Content-Type", "application/json")
+			w.WriteHeader(http.StatusInternalServerError)
+			_ = json.NewEncoder(w).Encode(map[string]string{"error": fmt.Sprintf("reconfiguration failed: %v", err)})
 			return
 		}
 
 		w.Header().Set("Content-Type", "application/json")
-		resp, _ := json.Marshal(map[string]string{"status": "ok", "module": moduleName})
-		w.Write(resp) //nolint:errcheck
-	})
+		_ = json.NewEncoder(w).Encode(map[string]string{"status": "ok", "module": moduleName})
+	})))
 
 	mux.Handle("/", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.Header().Set("Content-Type", "application/json")

config/diff.go

@@ -62,13 +62,15 @@ func DiffModuleConfigs(old, new *WorkflowConfig) *ModuleConfigDiff {
 	return diff
 }
 
-// HasNonModuleChanges returns true if workflows, triggers, pipelines, or
-// platform config changed between old and new (requiring full reload).
+// HasNonModuleChanges returns true if workflows, triggers, pipelines,
+// platform config, or requirements changed between old and new
+// (requiring full reload).
 func HasNonModuleChanges(old, new *WorkflowConfig) bool {
 	return hashAny(old.Workflows) != hashAny(new.Workflows) ||
 		hashAny(old.Triggers) != hashAny(new.Triggers) ||
 		hashAny(old.Pipelines) != hashAny(new.Pipelines) ||
-		hashAny(old.Platform) != hashAny(new.Platform)
+		hashAny(old.Platform) != hashAny(new.Platform) ||
+		hashAny(old.Requires) != hashAny(new.Requires)
 }
 
 func hashModuleConfig(m ModuleConfig) string {

config/reloader.go

@@ -52,6 +52,15 @@ func NewConfigReloader(
 	}, nil
 }
 
+// SetReconfigurer updates the ModuleReconfigurer used for partial (per-module)
+// reloads. This should be called after a successful full engine reload if the
+// underlying engine (and its reconfigurer) has changed.
+func (r *ConfigReloader) SetReconfigurer(reconfigurer ModuleReconfigurer) {
+	r.mu.Lock()
+	defer r.mu.Unlock()
+	r.reconfigurer = reconfigurer
+}
+
 // HandleChange processes a config change event. It diffs the old and new configs,
 // attempts per-module reconfiguration for module-only changes, and falls back
 // to a full reload when necessary.
@@ -75,7 +84,19 @@ func (r *ConfigReloader) HandleChange(evt ConfigChangeEvent) error {
 	}
 
 	// Only module config changes — try partial reconfiguration.
-	if len(diff.Modified) > 0 && r.reconfigurer != nil {
+	if len(diff.Modified) > 0 {
+		if r.reconfigurer == nil {
+			// No reconfigurer available — fall back to full reload.
+			r.logger.Info("module changes detected but no reconfigurer, performing full reload",
+				"modified", len(diff.Modified))
+			if err := r.fullReloadFn(evt.Config); err != nil {
+				return err
+			}
+			r.current = evt.Config
+			r.currentHash = evt.NewHash
+			return nil
+		}
+
 		failed, err := r.reconfigurer.ReconfigureModules(context.Background(), diff.Modified)
 		if err != nil {
 			return err

config/reloader_test.go

@@ -237,6 +237,80 @@ func TestConfigReloader_FallbackToFull(t *testing.T) {
 	}
 }
 
+func TestConfigReloader_NilReconfigurer_FallsBackToFullReload(t *testing.T) {
+	initial := makeWorkflowConfig(
+		[]ModuleConfig{{Name: "alpha", Type: "cache", Config: map[string]any{"ttl": 60}}},
+		nil,
+	)
+
+	var fullReloadCalled int
+	fullFn := func(cfg *WorkflowConfig) error {
+		fullReloadCalled++
+		return nil
+	}
+
+	// nil reconfigurer — module changes should still trigger full reload.
+	r := newTestReloader(t, initial, fullFn, nil)
+
+	newCfg := makeWorkflowConfig(
+		[]ModuleConfig{{Name: "alpha", Type: "cache", Config: map[string]any{"ttl": 120}}},
+		nil,
+	)
+	evt, err := makeChangeEvent(newCfg)
+	if err != nil {
+		t.Fatalf("makeChangeEvent: %v", err)
+	}
+
+	if err := r.HandleChange(evt); err != nil {
+		t.Fatalf("HandleChange: %v", err)
+	}
+
+	if fullReloadCalled != 1 {
+		t.Errorf("expected 1 full reload when reconfigurer is nil, got %d", fullReloadCalled)
+	}
+}
+
+func TestConfigReloader_SetReconfigurer(t *testing.T) {
+	initial := makeWorkflowConfig(
+		[]ModuleConfig{{Name: "alpha", Type: "cache", Config: map[string]any{"ttl": 60}}},
+		nil,
+	)
+
+	var fullReloadCalled int
+	fullFn := func(cfg *WorkflowConfig) error {
+		fullReloadCalled++
+		return nil
+	}
+
+	// Start with nil reconfigurer.
+	r := newTestReloader(t, initial, fullFn, nil)
+
+	// Set a reconfigurer dynamically.
+	rec := &mockReconfigurer{}
+	r.SetReconfigurer(rec)
+
+	newCfg := makeWorkflowConfig(
+		[]ModuleConfig{{Name: "alpha", Type: "cache", Config: map[string]any{"ttl": 120}}},
+		nil,
+	)
+	evt, err := makeChangeEvent(newCfg)
+	if err != nil {
+		t.Fatalf("makeChangeEvent: %v", err)
+	}
+
+	if err := r.HandleChange(evt); err != nil {
+		t.Fatalf("HandleChange: %v", err)
+	}
+
+	// Should use partial reconfigure, not full reload.
+	if fullReloadCalled != 0 {
+		t.Errorf("expected 0 full reloads after SetReconfigurer, got %d", fullReloadCalled)
+	}
+	if len(rec.called) != 1 {
+		t.Errorf("expected ReconfigureModules called once, got %d", len(rec.called))
+	}
+}
+
 func TestConfigReloader_NoEffectiveChanges(t *testing.T) {
 	initial := makeWorkflowConfig(
 		[]ModuleConfig{{Name: "alpha", Type: "http.server", Config: map[string]any{"port": 8080}}},

config/source_db.go

@@ -98,15 +98,16 @@ func (s *DatabaseSource) refresh(ctx context.Context) (*WorkflowConfig, error) {
 
 // Hash returns the SHA256 hex digest of the stored config bytes. It first
 // tries the fast path of fetching the pre-computed hash from the database,
-// and falls back to loading the full document if that fails.
+// and falls back to loading the full document if that fails. The fallback
+// always fetches fresh data to ensure change detection is accurate.
 func (s *DatabaseSource) Hash(ctx context.Context) (string, error) {
 	hash, err := s.store.GetConfigDocumentHash(ctx, s.key)
 	if err == nil {
 		return hash, nil
 	}
-	// Fallback: load and hash.
-	if _, loadErr := s.Load(ctx); loadErr != nil {
-		return "", loadErr
+	// Fallback: force a fresh load (bypass cache) to get an accurate hash.
+	if _, refreshErr := s.refresh(ctx); refreshErr != nil {
+		return "", refreshErr
 	}
 	s.mu.RLock()
 	defer s.mu.RUnlock()

config/source_db_poller.go

@@ -16,8 +16,9 @@ type DatabasePoller struct {
 	logger   *slog.Logger
 	lastHash string
 
-	done chan struct{}
-	wg   sync.WaitGroup
+	done     chan struct{}
+	stopOnce sync.Once
+	wg       sync.WaitGroup
 }
 
 // NewDatabasePoller creates a DatabasePoller that calls onChange whenever the
@@ -46,8 +47,9 @@ func (p *DatabasePoller) Start(ctx context.Context) error {
 }
 
 // Stop signals the polling goroutine to exit and waits for it to finish.
+// It is safe to call Stop multiple times.
 func (p *DatabasePoller) Stop() {
-	close(p.done)
+	p.stopOnce.Do(func() { close(p.done) })
 	p.wg.Wait()
 }
 

config/watcher.go

@@ -35,6 +35,7 @@ type ConfigWatcher struct {
 
 	fsWatcher *fsnotify.Watcher
 	done      chan struct{}
+	stopOnce  sync.Once
 	wg        sync.WaitGroup
 	lastHash  string
 
@@ -87,8 +88,9 @@ func (w *ConfigWatcher) Start() error {
 }
 
 // Stop terminates the watcher and waits for the background goroutine to exit.
+// It is safe to call Stop multiple times.
 func (w *ConfigWatcher) Stop() error {
-	close(w.done)
+	w.stopOnce.Do(func() { close(w.done) })
 	w.wg.Wait()
 	if w.fsWatcher != nil {
 		return w.fsWatcher.Close()
@@ -114,9 +116,16 @@ func (w *ConfigWatcher) loop() {
 			if !isYAMLFile(event.Name) {
 				continue
 			}
-			if event.Op&(fsnotify.Write|fsnotify.Create) != 0 {
+			if event.Op&(fsnotify.Write|fsnotify.Create|fsnotify.Rename) != 0 {
+				// Also handle Rename for atomic-save editors that rename-over
+				// the config file. On a Rename we enqueue the target config path
+				// rather than the renamed-away path so processChange still matches.
+				name := event.Name
+				if event.Op&fsnotify.Rename != 0 {
+					name = w.source.Path()
+				}
 				w.mu.Lock()
-				w.pending[event.Name] = time.Now()
+				w.pending[name] = time.Now()
 				w.mu.Unlock()
 			}