Add three more compose-file sanity checks

- depends_on cycles (not just direct self-reference): a three-color DFS
  over the dependency graph reports the actual cycle path.
- Duplicate host-port/protocol across (or within) services: two services
  both publishing the same (host_port, protocol) would only ever leave one
  reachable, even though both DNAT rules would get added later.
- container_name colliding with another service's own implicit name, not
  just two explicit container_names matching each other.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Gv3s5jckJKzh6JkMoi2Akz

src/compose_file.cpp

@@ -372,6 +372,85 @@ std::optional<ComposeVolumeMount> parse_volume_entry(std::string_view text, cons
     return mount;
 }
 
+enum class DfsColor { white, gray, black };
+
+DfsColor& color_of(std::vector<std::pair<std::string, DfsColor>>& colors, const std::string& name) {
+    for (auto& entry : colors) {
+        if (entry.first == name) {
+            return entry.second;
+        }
+    }
+    static DfsColor unreachable = DfsColor::black;  // every name is seeded into `colors` up front -- never reached
+    return unreachable;
+}
+
+const ComposeService* find_service(const std::vector<ComposeService>& services, const std::string& name) {
+    for (const auto& service : services) {
+        if (service.name == name) {
+            return &service;
+        }
+    }
+    return nullptr;
+}
+
+// Recursive step of find_dependency_cycle()'s three-color DFS: returns the
+// cycle path if the subtree rooted at `name` contains one (a depends_on
+// edge reaching back to a `gray` -- currently-on-the-stack -- node), else
+// nullopt. `stack` mirrors the recursion path so the actual cycle can be
+// read off it once a back-edge is found, rather than just reporting "a
+// cycle exists somewhere."
+std::optional<std::vector<std::string>> dfs_find_cycle(const std::vector<ComposeService>& services,
+                                                         std::vector<std::pair<std::string, DfsColor>>& colors,
+                                                         std::vector<std::string>& stack, const std::string& name) {
+    color_of(colors, name) = DfsColor::gray;
+    stack.push_back(name);
+
+    const ComposeService* service = find_service(services, name);
+    for (const auto& dep : service->depends_on) {
+        DfsColor dep_color = color_of(colors, dep);
+        if (dep_color == DfsColor::gray) {
+            auto start = std::find(stack.begin(), stack.end(), dep);
+            std::vector<std::string> cycle(start, stack.end());
+            cycle.push_back(dep);
+            return cycle;
+        }
+        if (dep_color == DfsColor::white) {
+            if (auto found = dfs_find_cycle(services, colors, stack, dep)) {
+                return found;
+            }
+        }
+    }
+
+    stack.pop_back();
+    color_of(colors, name) = DfsColor::black;
+    return std::nullopt;
+}
+
+// Finds a depends_on cycle among `services` (A -> B -> ... -> A), if any --
+// not just the direct self-reference load_compose_file() already rejects
+// per-edge before this ever runs, but any longer one, which would make a
+// future orchestrator's own startup ordering impossible. Assumes every
+// depends_on name already refers to a real, declared service (the
+// per-edge existence check that runs first guarantees this). Returns the
+// actual cycle path (e.g. {"a", "b", "c", "a"}) if one exists.
+std::optional<std::vector<std::string>> find_dependency_cycle(const std::vector<ComposeService>& services) {
+    std::vector<std::pair<std::string, DfsColor>> colors;
+    colors.reserve(services.size());
+    for (const auto& service : services) {
+        colors.emplace_back(service.name, DfsColor::white);
+    }
+
+    std::vector<std::string> stack;
+    for (const auto& service : services) {
+        if (color_of(colors, service.name) == DfsColor::white) {
+            if (auto found = dfs_find_cycle(services, colors, stack, service.name)) {
+                return found;
+            }
+        }
+    }
+    return std::nullopt;
+}
+
 }  // namespace
 
 std::optional<ComposeFile> load_compose_file(const std::filesystem::path& path) {
@@ -542,11 +621,29 @@ std::optional<ComposeFile> load_compose_file(const std::filesystem::path& path)
         if (!result.services[i].container_name) {
             continue;
         }
-        for (size_t j = i + 1; j < result.services.size(); ++j) {
+        const std::string& container_name = *result.services[i].container_name;
-            if (result.services[j].container_name &&
+        for (size_t j = 0; j < result.services.size(); ++j) {
-                *result.services[j].container_name == *result.services[i].container_name) {
+            if (j == i) {
+                continue;
+            }
+            // Two services both setting the same explicit container_name --
+            // checked with j > i so the pair is only reported once, not
+            // once as (i, j) and again as (j, i).
+            if (j > i && result.services[j].container_name && *result.services[j].container_name == container_name) {
                 spdlog::error("compose file {}: services '{}' and '{}' both use container_name '{}'", path.string(),
-                              result.services[i].name, result.services[j].name, *result.services[i].container_name);
+                              result.services[i].name, result.services[j].name, container_name);
+                return std::nullopt;
+            }
+            // A service's own explicit container_name colliding with
+            // *another* service's implicit identity (its own name, when it
+            // has no container_name of its own) -- inherently one-directional
+            // (service[i]'s container_name vs. service[j]'s name), so no
+            // j > i guard is needed here the way the symmetric check above
+            // needs one.
+            if (result.services[j].name == container_name) {
+                spdlog::error(
+                    "compose file {}: service '{}' has container_name '{}', colliding with service '{}'s own name",
+                    path.string(), result.services[i].name, container_name, result.services[j].name);
                 return std::nullopt;
             }
         }
@@ -677,5 +774,51 @@ std::optional<ComposeFile> load_compose_file(const std::filesystem::path& path)
         }
     }
 
+    // Not just the direct self-reference already rejected above -- a longer
+    // cycle (A -> B -> C -> A) would make a future orchestrator's own
+    // startup ordering impossible.
+    if (auto cycle = find_dependency_cycle(result.services)) {
+        std::string cycle_text;
+        for (size_t i = 0; i < cycle->size(); ++i) {
+            if (i > 0) {
+                cycle_text += " -> ";
+            }
+            cycle_text += (*cycle)[i];
+        }
+        spdlog::error("compose file {}: depends_on cycle: {}", path.string(), cycle_text);
+        return std::nullopt;
+    }
+
+    // A (host_port, protocol) pair names one real, host-wide socket -- two
+    // services (or two entries within the same one) both publishing it
+    // would only ever leave one of them actually reachable, even though
+    // both DNAT rules would get added by a future orchestrator.
+    struct PublishedPort {
+        int host_port;
+        PortForwardProtocol protocol;
+        std::string service_name;
+    };
+    std::vector<PublishedPort> published_ports;
+    for (const auto& service : result.services) {
+        for (const auto& port : service.ports) {
+            auto colliding =
+                std::find_if(published_ports.begin(), published_ports.end(), [&](const PublishedPort& p) {
+                    return p.host_port == port.host_port && p.protocol == port.protocol;
+                });
+            if (colliding != published_ports.end()) {
+                const char* proto = port.protocol == PortForwardProtocol::udp ? "udp" : "tcp";
+                if (colliding->service_name == service.name) {
+                    spdlog::error("compose file {}: service '{}' publishes host port {}/{} twice", path.string(),
+                                  service.name, port.host_port, proto);
+                } else {
+                    spdlog::error("compose file {}: services '{}' and '{}' both publish host port {}/{}",
+                                  path.string(), colliding->service_name, service.name, port.host_port, proto);
+                }
+                return std::nullopt;
+            }
+            published_ports.push_back({port.host_port, port.protocol, service.name});
+        }
+    }
+
     return result;
 }

tests/unit/test_compose_file.cpp

@@ -110,6 +110,19 @@ TEST_CASE("compose file: container_name -- optional, and duplicates across servi
                                     "    image: busybox:latest\n"
                                     "    container_name: shared\n");
     CHECK_FALSE(load_compose_file(colliding).has_value());
+
+    // A service's own explicit container_name colliding with a *different*
+    // service's implicit identity (its own name, since it has no
+    // container_name of its own) -- also an error, not just two explicit
+    // container_names matching each other.
+    auto colliding_with_name = write_compose(scratch.path(),
+                                              "services:\n"
+                                              "  web:\n"
+                                              "    image: busybox:latest\n"
+                                              "  worker:\n"
+                                              "    image: busybox:latest\n"
+                                              "    container_name: web\n");
+    CHECK_FALSE(load_compose_file(colliding_with_name).has_value());
 }
 
 TEST_CASE("compose file: command -- list form used as-is, scalar form wrapped in sh -c", "[unit]") {
@@ -243,6 +256,53 @@ TEST_CASE("compose file: depends_on -- long and short forms, condition handling,
     CHECK_FALSE(load_compose_file(undeclared).has_value());
 }
 
+TEST_CASE("compose file: depends_on cycles are rejected, acyclic graphs are not", "[unit]") {
+    ScratchXdgDirs scratch;
+
+    // A 2-service direct cycle (a -> b -> a) -- distinct from the
+    // already-tested direct self-reference (a -> a).
+    auto two_cycle = write_compose(scratch.path(),
+                                    "services:\n"
+                                    "  a:\n"
+                                    "    image: busybox:latest\n"
+                                    "    depends_on: [\"b\"]\n"
+                                    "  b:\n"
+                                    "    image: busybox:latest\n"
+                                    "    depends_on: [\"a\"]\n");
+    CHECK_FALSE(load_compose_file(two_cycle).has_value());
+
+    // A longer, 3-service cycle (a -> b -> c -> a).
+    auto three_cycle = write_compose(scratch.path(),
+                                      "services:\n"
+                                      "  a:\n"
+                                      "    image: busybox:latest\n"
+                                      "    depends_on: [\"b\"]\n"
+                                      "  b:\n"
+                                      "    image: busybox:latest\n"
+                                      "    depends_on: [\"c\"]\n"
+                                      "  c:\n"
+                                      "    image: busybox:latest\n"
+                                      "    depends_on: [\"a\"]\n");
+    CHECK_FALSE(load_compose_file(three_cycle).has_value());
+
+    // A valid, acyclic multi-service graph (a diamond: d depends on both b
+    // and c, both of which depend on a) must still load successfully.
+    auto acyclic = write_compose(scratch.path(),
+                                  "services:\n"
+                                  "  a:\n"
+                                  "    image: busybox:latest\n"
+                                  "  b:\n"
+                                  "    image: busybox:latest\n"
+                                  "    depends_on: [\"a\"]\n"
+                                  "  c:\n"
+                                  "    image: busybox:latest\n"
+                                  "    depends_on: [\"a\"]\n"
+                                  "  d:\n"
+                                  "    image: busybox:latest\n"
+                                  "    depends_on: [\"b\", \"c\"]\n");
+    CHECK(load_compose_file(acyclic).has_value());
+}
+
 TEST_CASE("compose file: stop_grace_period parses durations, rejects malformed ones", "[unit]") {
     ScratchXdgDirs scratch;
 
@@ -386,6 +446,43 @@ TEST_CASE("compose file: ports -- list and scalar forms reuse parse_port_forward
     CHECK_FALSE(load_compose_file(bad).has_value());
 }
 
+TEST_CASE("compose file: duplicate host-port/protocol across (or within) services is an error", "[unit]") {
+    ScratchXdgDirs scratch;
+
+    auto across_services = write_compose(scratch.path(),
+                                          "services:\n"
+                                          "  web:\n"
+                                          "    image: busybox:latest\n"
+                                          "    ports:\n"
+                                          "      - \"8080:80\"\n"
+                                          "  web2:\n"
+                                          "    image: busybox:latest\n"
+                                          "    ports:\n"
+                                          "      - \"8080:81\"\n");
+    CHECK_FALSE(load_compose_file(across_services).has_value());
+
+    auto within_one_service = write_compose(scratch.path(),
+                                             "services:\n"
+                                             "  web:\n"
+                                             "    image: busybox:latest\n"
+                                             "    ports:\n"
+                                             "      - \"8080:80\"\n"
+                                             "      - \"8080:81\"\n");
+    CHECK_FALSE(load_compose_file(within_one_service).has_value());
+
+    // Same host port, different protocols -- must still succeed, matching
+    // port_forward.h's own existing "same port pair once per protocol"
+    // precedent (e.g. a DNS-like service forwarding both tcp and udp).
+    auto different_protocols = write_compose(scratch.path(),
+                                              "services:\n"
+                                              "  web:\n"
+                                              "    image: busybox:latest\n"
+                                              "    ports:\n"
+                                              "      - \"53:53/tcp\"\n"
+                                              "      - \"53:53/udp\"\n");
+    CHECK(load_compose_file(different_protocols).has_value());
+}
+
 TEST_CASE("compose file: service volumes -- bind mounts (absolute-resolved, ro), named volume references",
           "[unit]") {
     ScratchXdgDirs scratch;