fix(feature): prevent false positive circular dependency detection for installsAfter

pkg/devcontainer/feature/extend.go

@@ -754,6 +754,7 @@ func addHardDependencies(
 			if err := g.AddEdge(depKey, featureKey); err != nil {
 				return err
 			}
+			g.MarkEdgeHard(depKey, featureKey)
 		}
 	}
 	return nil
@@ -772,10 +773,26 @@ func addSoftDependencies(
 			continue
 		}
 
+		if depKey == featureKey {
+			continue
+		}
+
 		if hasHardDependency(feature, id, normalizedID) {
 			continue
 		}
 
+		if g.IsReachableViaHardEdges(featureKey, depKey) {
+			continue
+		}
+
+		if g.IsReachable(featureKey, depKey) {
+			return fmt.Errorf(
+				"circular dependency detected: feature %q has installsAfter dependency on %q which creates a cycle",
+				feature.ConfigID,
+				normalizedID,
+			)
+		}
+
 		if err := g.AddEdge(depKey, featureKey); err != nil {
 			return err
 		}

pkg/devcontainer/feature/extend_test.go

@@ -269,6 +269,172 @@ func (suite *ExtendTestSuite) TestFeatureOrderWithDependencies_SameDependsOnAndI
 	suite.Equal("dev-code", installationOrder[1].ConfigID)
 }
 
+func (suite *ExtendTestSuite) TestFeatureOrder_SelfReferenceInInstallsAfter() {
+	selfRefID := normalizeFeatureID("self-ref-feature")
+	otherID := normalizeFeatureID("other-feature")
+	features := []*config.FeatureSet{
+		{
+			ConfigID: selfRefID,
+			Config: &config.FeatureConfig{
+				DependsOn:     config.DependsOnField{},
+				InstallsAfter: []string{"self-ref-feature"},
+			},
+		},
+		{
+			ConfigID: otherID,
+			Config: &config.FeatureConfig{
+				DependsOn:     config.DependsOnField{},
+				InstallsAfter: []string{},
+			},
+		},
+	}
+
+	installationOrder, err := getOrderedFeatureSets(features)
+	suite.Require().NoError(err)
+	suite.Len(installationOrder, 2)
+}
+
+func (suite *ExtendTestSuite) TestFeatureOrder_ForwardReferenceInInstallsAfter() {
+	featureAID := normalizeFeatureID(testFeatureA)
+	featureBID := normalizeFeatureID(testFeatureB)
+	features := []*config.FeatureSet{
+		{
+			ConfigID: featureAID,
+			Config: &config.FeatureConfig{
+				DependsOn:     config.DependsOnField{},
+				InstallsAfter: []string{testFeatureB},
+			},
+		},
+		{
+			ConfigID: featureBID,
+			Config: &config.FeatureConfig{
+				DependsOn:     config.DependsOnField{},
+				InstallsAfter: []string{},
+			},
+		},
+	}
+
+	installationOrder, err := getOrderedFeatureSets(features)
+	suite.Require().NoError(err)
+	suite.Len(installationOrder, 2)
+	suite.Equal(featureBID, installationOrder[0].ConfigID)
+	suite.Equal(featureAID, installationOrder[1].ConfigID)
+}
+
+func (suite *ExtendTestSuite) TestFeatureOrder_MutualInstallsAfterProducesError() {
+	featureAID := normalizeFeatureID(testFeatureA)
+	featureBID := normalizeFeatureID(testFeatureB)
+	features := []*config.FeatureSet{
+		{
+			ConfigID: featureAID,
+			Config: &config.FeatureConfig{
+				DependsOn:     config.DependsOnField{},
+				InstallsAfter: []string{testFeatureB},
+			},
+		},
+		{
+			ConfigID: featureBID,
+			Config: &config.FeatureConfig{
+				DependsOn:     config.DependsOnField{},
+				InstallsAfter: []string{testFeatureA},
+			},
+		},
+	}
+
+	_, err := getOrderedFeatureSets(features)
+	suite.Error(err)
+	suite.Contains(err.Error(), "circular dependency detected")
+}
+
+func (suite *ExtendTestSuite) TestFeatureOrder_ThreeNodePureSoftCycleProducesError() {
+	featureAID := normalizeFeatureID(testFeatureA)
+	featureBID := normalizeFeatureID(testFeatureB)
+	featureCID := normalizeFeatureID(testFeatureC)
+	features := []*config.FeatureSet{
+		{
+			ConfigID: featureAID,
+			Config: &config.FeatureConfig{
+				DependsOn:     config.DependsOnField{},
+				InstallsAfter: []string{testFeatureB},
+			},
+		},
+		{
+			ConfigID: featureBID,
+			Config: &config.FeatureConfig{
+				DependsOn:     config.DependsOnField{},
+				InstallsAfter: []string{testFeatureC},
+			},
+		},
+		{
+			ConfigID: featureCID,
+			Config: &config.FeatureConfig{
+				DependsOn:     config.DependsOnField{},
+				InstallsAfter: []string{testFeatureA},
+			},
+		},
+	}
+
+	_, err := getOrderedFeatureSets(features)
+	suite.Error(err)
+	suite.Contains(err.Error(), "circular dependency detected")
+}
+
+func (suite *ExtendTestSuite) TestFeatureOrder_InstallsAfterWithHardDepDoesNotCycle() {
+	featureAID := normalizeFeatureID(testFeatureA)
+	featureBID := normalizeFeatureID(testFeatureB)
+	features := []*config.FeatureSet{
+		{
+			ConfigID: featureAID,
+			Config: &config.FeatureConfig{
+				DependsOn: config.DependsOnField{
+					testFeatureB: map[string]any{},
+				},
+				InstallsAfter: []string{testFeatureB},
+			},
+		},
+		{
+			ConfigID: featureBID,
+			Config: &config.FeatureConfig{
+				DependsOn:     config.DependsOnField{},
+				InstallsAfter: []string{testFeatureA},
+			},
+		},
+	}
+
+	installationOrder, err := getOrderedFeatureSets(features)
+	suite.Require().NoError(err)
+	suite.Len(installationOrder, 2)
+	suite.Equal(featureBID, installationOrder[0].ConfigID)
+	suite.Equal(featureAID, installationOrder[1].ConfigID)
+}
+
+func (suite *ExtendTestSuite) TestFeatureOrder_TrueCircularDependencyStillDetected() {
+	featureAID := normalizeFeatureID(testFeatureA)
+	featureBID := normalizeFeatureID(testFeatureB)
+	features := []*config.FeatureSet{
+		{
+			ConfigID: featureAID,
+			Config: &config.FeatureConfig{
+				DependsOn: config.DependsOnField{
+					testFeatureB: map[string]any{},
+				},
+			},
+		},
+		{
+			ConfigID: featureBID,
+			Config: &config.FeatureConfig{
+				DependsOn: config.DependsOnField{
+					testFeatureA: map[string]any{},
+				},
+			},
+		},
+	}
+
+	_, err := getOrderedFeatureSets(features)
+	suite.Error(err)
+	suite.Contains(err.Error(), "circular")
+}
+
 func (suite *ExtendTestSuite) TestComputeFeatureOrder_NoOverride() {
 	devContainer := &config.DevContainerConfig{
 		DevContainerConfigBase: config.DevContainerConfigBase{

pkg/devcontainer/graph/graph.go

@@ -9,16 +9,18 @@ import (
 )
 
 type Graph[T comparable] struct {
-	nodes    map[string]T
-	edges    map[string][]string
-	inDegree map[string]int
+	nodes     map[string]T
+	edges     map[string][]string
+	inDegree  map[string]int
+	hardEdges map[string]map[string]bool
 }
 
 func NewGraph[T comparable]() *Graph[T] {
 	return &Graph[T]{
-		nodes:    make(map[string]T),
-		edges:    make(map[string][]string),
-		inDegree: make(map[string]int),
+		nodes:     make(map[string]T),
+		edges:     make(map[string][]string),
+		inDegree:  make(map[string]int),
+		hardEdges: make(map[string]map[string]bool),
 	}
 }
 
@@ -183,6 +185,60 @@ func (g *Graph[T]) RemoveChildren(id string) error {
 	return nil
 }
 
+func (g *Graph[T]) IsReachable(from, to string) bool {
+	if from == to {
+		return true
+	}
+	visited := make(map[string]bool)
+	queue := []string{from}
+	for len(queue) > 0 {
+		current := queue[0]
+		queue = queue[1:]
+		for _, neighbor := range g.edges[current] {
+			if neighbor == to {
+				return true
+			}
+			if !visited[neighbor] {
+				visited[neighbor] = true
+				queue = append(queue, neighbor)
+			}
+		}
+	}
+	return false
+}
+
+func (g *Graph[T]) MarkEdgeHard(from, to string) {
+	if g.hardEdges[from] == nil {
+		g.hardEdges[from] = make(map[string]bool)
+	}
+	g.hardEdges[from][to] = true
+}
+
+func (g *Graph[T]) IsReachableViaHardEdges(from, to string) bool {
+	if from == to {
+		return true
+	}
+	visited := make(map[string]bool)
+	queue := []string{from}
+	for len(queue) > 0 {
+		current := queue[0]
+		queue = queue[1:]
+		for _, neighbor := range g.edges[current] {
+			if !g.hardEdges[current][neighbor] {
+				continue
+			}
+			if neighbor == to {
+				return true
+			}
+			if !visited[neighbor] {
+				visited[neighbor] = true
+				queue = append(queue, neighbor)
+			}
+		}
+	}
+	return false
+}
+
 func (g *Graph[T]) HasNode(id string) bool {
 	_, exists := g.nodes[id]
 	return exists

pkg/devcontainer/graph/graph_test.go

@@ -501,6 +501,25 @@ func (suite *GraphTestSuite) TestEdgeCases() {
 	}
 }
 
+func (suite *GraphTestSuite) TestIsReachable() {
+	suite.Require().NoError(suite.graph.AddNode("A", "dataA"))
+	suite.Require().NoError(suite.graph.AddNode("B", "dataB"))
+	suite.Require().NoError(suite.graph.AddNode("C", "dataC"))
+	suite.Require().NoError(suite.graph.AddNode("D", "dataD"))
+
+	suite.Require().NoError(suite.graph.AddEdge("A", "B"))
+	suite.Require().NoError(suite.graph.AddEdge("B", "C"))
+
+	suite.True(suite.graph.IsReachable("A", "B"))
+	suite.True(suite.graph.IsReachable("A", "C"))
+	suite.True(suite.graph.IsReachable("B", "C"))
+	suite.False(suite.graph.IsReachable("C", "A"))
+	suite.False(suite.graph.IsReachable("C", "B"))
+	suite.False(suite.graph.IsReachable("A", "D"))
+	suite.False(suite.graph.IsReachable("D", "A"))
+	suite.True(suite.graph.IsReachable("A", "A"))
+}
+
 func (suite *GraphTestSuite) TestLargeGraph() {
 	nodeCount := 100
 

pkg/devcontainer/metadata/metadata.go

@@ -73,12 +73,11 @@ func FeatureConfigToImageMetadata(feature *config.FeatureConfig) *config.ImageMe
 			Customizations:       feature.Customizations,
 		},
 		NonComposeBase: config.NonComposeBase{
-			ContainerEnv: feature.ContainerEnv,
-			Mounts:       feature.Mounts,
-			Init:         feature.Init,
-			Privileged:   feature.Privileged,
-			CapAdd:       feature.CapAdd,
-			SecurityOpt:  feature.SecurityOpt,
+			Mounts:      feature.Mounts,
+			Init:        feature.Init,
+			Privileged:  feature.Privileged,
+			CapAdd:      feature.CapAdd,
+			SecurityOpt: feature.SecurityOpt,
 		},
 	}
 }

pkg/devcontainer/metadata/metadata_test.go

@@ -8,7 +8,7 @@ import (
 	"github.com/devsy-org/devsy/pkg/devcontainer/config"
 )
 
-func TestFeatureConfigToImageMetadata_IncludesContainerEnv(t *testing.T) {
+func TestFeatureConfigToImageMetadata_ExcludesContainerEnv(t *testing.T) {
 	feature := &config.FeatureConfig{
 		ContainerEnv: map[string]string{
 			"FOO": "bar",
@@ -18,11 +18,11 @@ func TestFeatureConfigToImageMetadata_IncludesContainerEnv(t *testing.T) {
 
 	got := FeatureConfigToImageMetadata(feature)
 
-	if got.ContainerEnv == nil {
-		t.Fatal("expected ContainerEnv to be included in per-feature metadata, got nil")
-	}
-	if got.ContainerEnv["FOO"] != "bar" || got.ContainerEnv["BAZ"] != "qux" {
-		t.Fatalf("expected ContainerEnv to contain FOO=bar and BAZ=qux, got %v", got.ContainerEnv)
+	if got.ContainerEnv != nil {
+		t.Fatalf(
+			"expected ContainerEnv to NOT be in per-feature metadata (applied via Dockerfile ENV only), got %v",
+			got.ContainerEnv,
+		)
 	}
 }