refactor(cli): centralize module graph validation

Author: GaspardKirira

include/vix/cli/modules/ModuleGraph.hpp

@@ -0,0 +1,55 @@
+#ifndef VIX_CLI_MODULES_MODULE_GRAPH_HPP
+#define VIX_CLI_MODULES_MODULE_GRAPH_HPP
+
+#include <filesystem>
+#include <string>
+#include <vector>
+
+#include <vix/cli/app/AppManifest.hpp>
+
+namespace vix::cli::modules
+{
+  struct ModuleNode
+  {
+    std::string name;
+    std::string identity;
+    std::filesystem::path path;
+    std::string kind;
+    bool enabled{true};
+    std::vector<std::string> dependencies;
+  };
+
+  class ModuleGraph
+  {
+  public:
+    static std::string canonical_identity(const std::string &name);
+    static std::string case_folded_identity(const std::string &name);
+    static ModuleGraph from_app_modules(
+        const std::vector<vix::cli::app::AppModule> &modules,
+        std::string &error);
+
+    bool valid() const { return error_.empty(); }
+    const std::string &error() const { return error_; }
+    bool contains(const std::string &name) const;
+    const ModuleNode *find(const std::string &name) const;
+    const std::vector<ModuleNode> &nodes() const { return nodes_; }
+
+    // Validates paths against a project root without modifying project files.
+    bool validate_paths(const std::filesystem::path &projectRoot,
+                        bool requireDirectories,
+                        std::string &error) const;
+    bool topological_order(std::vector<std::string> &order,
+                           bool activeOnly = false,
+                           std::string *error = nullptr) const;
+    bool dependency_closure(const std::string &name,
+                            std::vector<std::string> &closure,
+                            std::string *error = nullptr) const;
+    bool active_closure(std::vector<std::string> &closure,
+                        std::string *error = nullptr) const;
+
+  private:
+    std::vector<ModuleNode> nodes_;
+    std::string error_;
+  };
+}
+#endif

src/app/AppCMakeGenerator.cpp

@@ -16,6 +16,7 @@
 
 #include <vix/cli/app/AppCMakeGenerator.hpp>
 #include <vix/cli/modules/ModuleManifest.hpp>
+#include <vix/cli/modules/ModuleGraph.hpp>
 
 #include <algorithm>
 #include <cctype>
@@ -1477,6 +1478,16 @@ namespace vix::cli::app
     const fs::path normalizedProjectDir =
         fs::absolute(projectDir).lexically_normal();
 
+    std::string graphError;
+    const auto graph = vix::cli::modules::ModuleGraph::from_app_modules(
+        manifest.appModules, graphError);
+    if (!graph.valid() ||
+        !graph.validate_paths(normalizedProjectDir, true, graphError))
+    {
+      result.error = "Invalid module graph: " + graphError;
+      return result;
+    }
+
     const std::vector<AppModule> runtimeModules =
         enabled_runtime_app_modules(manifest, normalizedProjectDir);
 

src/app/AppManifest.cpp

@@ -192,7 +192,10 @@ namespace vix::cli::app
               std::string("[module.").size(),
               value.size() - std::string("[module.").size() - 1);
 
-      moduleName = normalize_module_id(rawName);
+      // Preserve the spelling supplied by the user.  ModuleGraph owns the
+      // CMake identity normalization and must be able to diagnose collisions
+      // such as foo-bar versus foo_bar.
+      moduleName = trim_copy(rawName);
 
       return is_valid_module_name(moduleName);
     }

src/app/AppProjectResolver.cpp

@@ -19,6 +19,7 @@
 #include <vix/cli/app/AppCMakeGenerator.hpp>
 #include <vix/cli/app/AppManifest.hpp>
 #include <vix/cli/modules/ModuleManifest.hpp>
+#include <vix/cli/modules/ModuleGraph.hpp>
 
 #include <vix/cli/util/Lockfile.hpp>
 #include <vix/cli/util/Manifest.hpp>
@@ -274,6 +275,15 @@ namespace vix::cli::app
 
       AppManifest manifest = loadResult.manifest;
 
+      std::string graphError;
+      const auto graph = vix::cli::modules::ModuleGraph::from_app_modules(
+          manifest.appModules, graphError);
+      if (!graph.valid() || !graph.validate_paths(projectDir, true, graphError))
+      {
+        result.error = "Invalid module graph: " + graphError;
+        return result;
+      }
+
       merge_enabled_module_registry_deps(
           manifest,
           projectDir);

src/commands/modules/ModulesCommands.cpp

@@ -13,6 +13,7 @@
 #include <vix/cli/commands/modules/ModulesUtils.hpp>
 #include <vix/cli/app/AppManifest.hpp>
 #include <vix/cli/modules/ModuleManifest.hpp>
+#include <vix/cli/modules/ModuleGraph.hpp>
 #include <vix/cli/Style.hpp>
 #include <vix/cli/util/Ui.hpp>
 
@@ -1357,109 +1358,6 @@ namespace vix::commands::modules_cmd::commands
     return set_module_enabled_in_vix_app(root, module, false);
   }
 
-  static bool check_dependency_cycle_visit(
-      const std::string &module,
-      const std::unordered_map<std::string, std::vector<std::string>> &graph,
-      std::unordered_set<std::string> &visiting,
-      std::unordered_set<std::string> &visited,
-      std::vector<std::string> &stack,
-      std::vector<std::string> &cycle)
-  {
-    if (visited.find(module) != visited.end())
-      return false;
-
-    if (visiting.find(module) != visiting.end())
-    {
-      cycle.clear();
-
-      bool capture = false;
-
-      for (const std::string &item : stack)
-      {
-        if (item == module)
-          capture = true;
-
-        if (capture)
-          cycle.push_back(item);
-      }
-
-      cycle.push_back(module);
-      return true;
-    }
-
-    visiting.insert(module);
-    stack.push_back(module);
-
-    const auto it = graph.find(module);
-
-    if (it != graph.end())
-    {
-      for (const std::string &dep : it->second)
-      {
-        if (check_dependency_cycle_visit(
-                dep,
-                graph,
-                visiting,
-                visited,
-                stack,
-                cycle))
-        {
-          return true;
-        }
-      }
-    }
-
-    stack.pop_back();
-    visiting.erase(module);
-    visited.insert(module);
-
-    return false;
-  }
-
-  static bool check_dependency_cycles(
-      const std::unordered_map<std::string, std::vector<std::string>> &graph,
-      std::vector<std::string> &cycle)
-  {
-    std::unordered_set<std::string> visiting;
-    std::unordered_set<std::string> visited;
-    std::vector<std::string> stack;
-
-    for (const auto &entry : graph)
-    {
-      if (check_dependency_cycle_visit(
-              entry.first,
-              graph,
-              visiting,
-              visited,
-              stack,
-              cycle))
-      {
-        return true;
-      }
-    }
-
-    return false;
-  }
-
-  static std::string join_cycle(
-      const std::vector<std::string> &cycle)
-  {
-    if (cycle.empty())
-      return "-";
-
-    std::ostringstream out;
-
-    for (std::size_t i = 0; i < cycle.size(); ++i)
-    {
-      if (i > 0)
-        out << " -> ";
-
-      out << cycle[i];
-    }
-
-    return out.str();
-  }
-
   bool cmd_check(const fs::path &root, const std::string &project)
   {
     const fs::path modulesDir = root / "modules";
@@ -1481,7 +1379,7 @@ namespace vix::commands::modules_cmd::commands
     std::unordered_map<std::string, bool> enabledInApp;
     std::unordered_map<std::string, std::string> kindInApp;
     std::unordered_map<std::string, std::string> pathInApp;
-    std::unordered_map<std::string, std::vector<std::string>> appDeps;
+    vix::cli::modules::ModuleGraph graph;
 
     if (hasVixApp)
     {
@@ -1495,6 +1393,15 @@ namespace vix::commands::modules_cmd::commands
         return false;
       }
 
+      std::string graphError;
+      graph = vix::cli::modules::ModuleGraph::from_app_modules(
+          loadResult.manifest.appModules, graphError);
+      if (!graph.valid() || !graph.validate_paths(root, false, graphError))
+      {
+        ui::err_line(std::cout, "Invalid module graph: " + graphError);
+        return false;
+      }
+
       for (const auto &module : loadResult.manifest.appModules)
       {
         const std::string name =
@@ -1504,11 +1411,6 @@ namespace vix::commands::modules_cmd::commands
         enabledInApp[name] = module.enabled;
         kindInApp[name] = module.kind.empty() ? "module" : module.kind;
         pathInApp[name] = module.path.empty() ? ("modules/" + name) : module.path;
-
-        for (const std::string &depRaw : module.depends)
-        {
-          appDeps[name].push_back(cnt::normalize_module_id(depRaw));
-        }
       }
     }
 
@@ -1587,43 +1489,6 @@ namespace vix::commands::modules_cmd::commands
         }
       }
 
-      for (const auto &entry : appDeps)
-      {
-        const std::string &module = entry.first;
-
-        for (const std::string &dep : entry.second)
-        {
-          if (declaredInApp.find(dep) == declaredInApp.end())
-          {
-            ok = false;
-            ++violations;
-
-            ui::err_line(std::cout, "Module depends on an undeclared module");
-            ui::kv(std::cout, "module", module, 12);
-            ui::kv(std::cout, "depends", dep, 12);
-          }
-          else if (enabledInApp[module] && !enabledInApp[dep])
-          {
-            ok = false;
-            ++violations;
-
-            ui::err_line(std::cout, "Enabled module depends on a disabled module");
-            ui::kv(std::cout, "module", module, 12);
-            ui::kv(std::cout, "depends", dep, 12);
-          }
-        }
-      }
-
-      std::vector<std::string> cycle;
-
-      if (check_dependency_cycles(appDeps, cycle))
-      {
-        ok = false;
-        ++violations;
-
-        ui::err_line(std::cout, "Circular module dependency detected");
-        ui::kv(std::cout, "cycle", join_cycle(cycle), 12);
-      }
     }
 
     std::unordered_map<std::string, std::string> routePrefixes;