Fix Tier 1 code review findings

- Make LoadError 2-arg constructor private (was reading moved-from state)
- Remove mutable instances() cache (data race risk under shared lock)
- Add ObserverGuard RAII class to prevent dangling observer pointers
- Document add_plugin permissive dep policy and subscription ID invariant

Author: bdcbqa314159

plugin_arch/PluginError.hpp

@@ -31,16 +31,9 @@ class PluginError : public std::runtime_error {
 };
 
 // dlopen / LoadLibrary failed. Carries the platform-specific error string.
+// Always construct via ::make() to avoid reading moved-from state.
 class LoadError : public PluginError {
  public:
-  LoadError(std::string library_path, std::string platform_error)
-      : PluginError(std::move(library_path),
-                    "Failed to load library: " + this->library_path() +
-                        " (" + platform_error + ")"),
-        platform_error_(std::move(platform_error)) {}
-
-  // Two-phase construction: we need library_path stored before building what().
-  // Use a static factory to build the message cleanly.
   static LoadError make(std::string library_path, std::string platform_error) {
     std::string msg = "Failed to load library: " + library_path +
                       " (" + platform_error + ")";

plugin_arch/PluginManager.hpp

@@ -332,7 +332,7 @@ class PluginManager {
                             infos[result.info_idx].deps});
         rebuild_name_index();
         wire_instance_tracked(plugins_.back(), config_map);
-        instances_dirty_ = true;
+    
       }
     }
   }
@@ -377,26 +377,30 @@ class PluginManager {
     }
 
     // --- Dependency + version checks (D6) ---
+    // add_plugin is permissive about missing deps (provider may be added
+    // later — the reverse version check on line ~407 catches mismatches
+    // when the provider arrives). Version constraints are still validated
+    // when the provider is already loaded.
     std::vector<std::string> deps;
     auto* dep_aware = dynamic_cast<IDependencyAware*>(instance.get());
     if (dep_aware) {
       for (const auto& raw_dep : dep_aware->dependencies()) {
         auto parsed = Dependency::parse(raw_dep);
         deps.push_back(parsed.type);
 
-        if (parsed.op != Dependency::Op::any) {
-          // Find the provider and check version constraint.
-          for (const auto& lp : plugins_) {
-            if (lp.entry.type == parsed.type) {
+        // Check version constraint only if the provider is already loaded.
+        for (const auto& lp : plugins_) {
+          if (lp.entry.type == parsed.type) {
+            if (parsed.op != Dependency::Op::any) {
               auto provider_version = SemVer::parse(lp.entry.version);
               if (!parsed.satisfied_by(provider_version)) {
                 throw std::runtime_error(
                     "Plugin '" + entry.name + "' requires " + raw_dep +
                     " but '" + lp.entry.name + "' provides version " +
                     lp.entry.version);
               }
-              break;
             }
+            break;
           }
         }
       }
@@ -448,7 +452,7 @@ class PluginManager {
     rebuild_name_index();
     wire_instance_tracked(plugins_.back(), config_map, /*skip_validation=*/true);
     notify_loaded(entry.name, entry.type);
-    instances_dirty_ = true;
+
   }
 
   // --- Per-plugin unload (A1) ---
@@ -495,7 +499,7 @@ class PluginManager {
     plugins_ = std::move(remaining);
     rebuild_name_index();
     rebuild_locator();
-    instances_dirty_ = true;
+
   }
 
   // --- Per-plugin reload (A1) ---
@@ -568,7 +572,7 @@ class PluginManager {
     }
 
     notify_reloaded(name, target_type);
-    instances_dirty_ = true;
+
   }
 
   // --- Health checks (B1) ---
@@ -793,7 +797,7 @@ class PluginManager {
     locator_.clear();
     event_bus_.clear();
     load_errors_.clear();
-    instances_dirty_ = true;
+
   }
 
   [[nodiscard]] ServiceLocator& locator() { return locator_; }
@@ -802,17 +806,13 @@ class PluginManager {
   [[nodiscard]] EventBus& event_bus() { return event_bus_; }
   [[nodiscard]] const EventBus& event_bus() const { return event_bus_; }
 
-  [[nodiscard]] const std::vector<std::shared_ptr<IPlugin>>& instances()
-      const {
-    if (instances_dirty_) {
-      instances_cache_.clear();
-      instances_cache_.reserve(plugins_.size());
-      for (const auto& lp : plugins_) {
-        instances_cache_.push_back(lp.instance);
-      }
-      instances_dirty_ = false;
+  [[nodiscard]] std::vector<std::shared_ptr<IPlugin>> instances() const {
+    std::vector<std::shared_ptr<IPlugin>> result;
+    result.reserve(plugins_.size());
+    for (const auto& lp : plugins_) {
+      result.push_back(lp.instance);
     }
-    return instances_cache_;
+    return result;
   }
 
   // Get a loaded plugin by name. Returns nullptr if not found.
@@ -859,8 +859,6 @@ class PluginManager {
   EventBus event_bus_;
   std::vector<LoadedPlugin> plugins_;
   std::unordered_map<std::string, std::size_t> name_index_;
-  mutable std::vector<std::shared_ptr<IPlugin>> instances_cache_;
-  mutable bool instances_dirty_ = true;
   std::vector<ErrorRecord> load_errors_;
   std::vector<PluginObserver*> observers_;
 
@@ -1148,7 +1146,7 @@ class PluginManager {
     rebuild_name_index();
     wire_instance_tracked(plugins_.back(), config_map);
     notify_loaded(entry.name, entry.type);
-    instances_dirty_ = true;
+
   }
 
   // Wire all opt-in mixins on an instance.
@@ -1218,18 +1216,43 @@ class PluginManager {
   }
 
   // Wire + track EventBus subscription IDs (for enable/disable).
+  //
+  // Relies on EventBus::next_id_ being monotonically increasing with no
+  // reuse: all IDs allocated between [before, after) belong to this plugin.
+  // This holds because PluginManager is single-threaded (or exclusively
+  // locked via ThreadSafe), and wire_instance may call on_init() /
+  // set_event_bus() which subscribe — those subscriptions correctly belong
+  // to this plugin. Unsubscribes during wiring don't break the invariant
+  // because IDs are never recycled.
   void wire_instance_tracked(LoadedPlugin& lp, const ConfigMap& config_map,
                               bool skip_validation = false) {
     auto before = event_bus_.next_subscription_id();
     wire_instance(lp.instance, lp.entry, config_map, skip_validation);
     auto after = event_bus_.next_subscription_id();
 
-    // Record all subscription IDs created during wiring
     lp.subscription_ids.clear();
     for (auto id = before; id < after; ++id) {
       lp.subscription_ids.push_back(id);
     }
   }
 };
 
+// --- ObserverGuard inline implementation ---
+// Defined here because it needs PluginManager to be complete.
+
+inline ObserverGuard::ObserverGuard(PluginManager& manager,
+                                    PluginObserver* observer)
+    : manager_(&manager), observer_(observer) {
+  if (observer_) manager_->add_observer(observer_);
+}
+
+inline ObserverGuard::~ObserverGuard() { release(); }
+
+inline void ObserverGuard::release() {
+  if (observer_) {
+    manager_->remove_observer(observer_);
+    observer_ = nullptr;
+  }
+}
+
 }  // namespace plugin_arch

plugin_arch/PluginObserver.hpp

@@ -3,13 +3,17 @@
 // Hosts register observers with PluginManager to receive callbacks
 // when plugins are loaded, unloaded, reloaded, enabled, or disabled.
 // Useful for metrics, logging, and debugging.
+//
+// Use ObserverGuard for RAII auto-deregistration to prevent dangling pointers.
 
 #pragma once
 
 #include <string>
 
 namespace plugin_arch {
 
+class PluginManager;
+
 class PluginObserver {
  public:
   virtual ~PluginObserver() = default;
@@ -26,4 +30,37 @@ class PluginObserver {
                                   const std::string& type) {}
 };
 
+// RAII guard that auto-deregisters the observer on destruction.
+// Prevents dangling pointer callbacks when the observer is destroyed
+// before the PluginManager.
+class ObserverGuard {
+ public:
+  ObserverGuard(PluginManager& manager, PluginObserver* observer);
+  ~ObserverGuard();
+
+  ObserverGuard(const ObserverGuard&) = delete;
+  ObserverGuard& operator=(const ObserverGuard&) = delete;
+
+  ObserverGuard(ObserverGuard&& other) noexcept
+      : manager_(other.manager_), observer_(other.observer_) {
+    other.observer_ = nullptr;
+  }
+
+  ObserverGuard& operator=(ObserverGuard&& other) noexcept {
+    if (this != &other) {
+      release();
+      manager_ = other.manager_;
+      observer_ = other.observer_;
+      other.observer_ = nullptr;
+    }
+    return *this;
+  }
+
+  void release();
+
+ private:
+  PluginManager* manager_;
+  PluginObserver* observer_;
+};
+
 }  // namespace plugin_arch