From dc32f36f195ae6aa99bcf8f68a717396616ad46d Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 5 May 2026 21:59:35 -0500 Subject: [PATCH 1/4] fix(feature): prevent false positive circular dependency detection for installsAfter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit installsAfter is a soft ordering hint — cycles involving only soft edges should not cause hard failures. Previously, self-references and mutual installsAfter entries incorrectly triggered circular dependency errors in the topological sort. Add Graph.IsReachable to detect when a soft edge would create a cycle, and skip it instead of failing. This fixes three E2E test scenarios: - Feature in both dependsOn and installsAfter - Forward references in installsAfter - Self-references in installsAfter --- pkg/devcontainer/feature/extend.go | 8 ++ pkg/devcontainer/feature/extend_test.go | 133 ++++++++++++++++++++++++ pkg/devcontainer/graph/graph.go | 22 ++++ pkg/devcontainer/graph/graph_test.go | 19 ++++ 4 files changed, 182 insertions(+) diff --git a/pkg/devcontainer/feature/extend.go b/pkg/devcontainer/feature/extend.go index c347d01c9..5c89ad874 100644 --- a/pkg/devcontainer/feature/extend.go +++ b/pkg/devcontainer/feature/extend.go @@ -772,10 +772,18 @@ func addSoftDependencies( continue } + if depKey == featureKey { + continue + } + if hasHardDependency(feature, id, normalizedID) { continue } + if g.IsReachable(featureKey, depKey) { + continue + } + if err := g.AddEdge(depKey, featureKey); err != nil { return err } diff --git a/pkg/devcontainer/feature/extend_test.go b/pkg/devcontainer/feature/extend_test.go index 0e8cddc3f..6aea15709 100644 --- a/pkg/devcontainer/feature/extend_test.go +++ b/pkg/devcontainer/feature/extend_test.go @@ -269,6 +269,139 @@ 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("feature-a") + featureBID := normalizeFeatureID("feature-b") + features := []*config.FeatureSet{ + { + ConfigID: featureAID, + Config: &config.FeatureConfig{ + DependsOn: config.DependsOnField{}, + InstallsAfter: []string{"feature-b"}, + }, + }, + { + 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_MutualInstallsAfterDoesNotCycle() { + featureAID := normalizeFeatureID("feature-a") + featureBID := normalizeFeatureID("feature-b") + features := []*config.FeatureSet{ + { + ConfigID: featureAID, + Config: &config.FeatureConfig{ + DependsOn: config.DependsOnField{}, + InstallsAfter: []string{"feature-b"}, + }, + }, + { + ConfigID: featureBID, + Config: &config.FeatureConfig{ + DependsOn: config.DependsOnField{}, + InstallsAfter: []string{"feature-a"}, + }, + }, + } + + installationOrder, err := getOrderedFeatureSets(features) + suite.Require().NoError(err) + suite.Len(installationOrder, 2) +} + +func (suite *ExtendTestSuite) TestFeatureOrder_InstallsAfterWithHardDepDoesNotCycle() { + featureAID := normalizeFeatureID("feature-a") + featureBID := normalizeFeatureID("feature-b") + features := []*config.FeatureSet{ + { + ConfigID: featureAID, + Config: &config.FeatureConfig{ + DependsOn: config.DependsOnField{ + "feature-b": map[string]any{}, + }, + InstallsAfter: []string{"feature-b"}, + }, + }, + { + ConfigID: featureBID, + Config: &config.FeatureConfig{ + DependsOn: config.DependsOnField{}, + InstallsAfter: []string{"feature-a"}, + }, + }, + } + + 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("feature-a") + featureBID := normalizeFeatureID("feature-b") + features := []*config.FeatureSet{ + { + ConfigID: featureAID, + Config: &config.FeatureConfig{ + DependsOn: config.DependsOnField{ + "feature-b": map[string]any{}, + }, + }, + }, + { + ConfigID: featureBID, + Config: &config.FeatureConfig{ + DependsOn: config.DependsOnField{ + "feature-a": 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{ diff --git a/pkg/devcontainer/graph/graph.go b/pkg/devcontainer/graph/graph.go index f19a2618e..a2cc8f00c 100644 --- a/pkg/devcontainer/graph/graph.go +++ b/pkg/devcontainer/graph/graph.go @@ -183,6 +183,28 @@ 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]) HasNode(id string) bool { _, exists := g.nodes[id] return exists diff --git a/pkg/devcontainer/graph/graph_test.go b/pkg/devcontainer/graph/graph_test.go index 55c0f108a..523384c88 100644 --- a/pkg/devcontainer/graph/graph_test.go +++ b/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 From 643f5faf77f8e5dcbe2c81971815926cf7399787 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 5 May 2026 22:08:05 -0500 Subject: [PATCH 2/4] fix(test): use testFeatureA/B constants in new InstallsAfter tests Replaces inline string literals with existing constants to satisfy goconst linter. --- pkg/devcontainer/feature/extend_test.go | 32 ++++++++++++------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/pkg/devcontainer/feature/extend_test.go b/pkg/devcontainer/feature/extend_test.go index 6aea15709..d3488901e 100644 --- a/pkg/devcontainer/feature/extend_test.go +++ b/pkg/devcontainer/feature/extend_test.go @@ -295,14 +295,14 @@ func (suite *ExtendTestSuite) TestFeatureOrder_SelfReferenceInInstallsAfter() { } func (suite *ExtendTestSuite) TestFeatureOrder_ForwardReferenceInInstallsAfter() { - featureAID := normalizeFeatureID("feature-a") - featureBID := normalizeFeatureID("feature-b") + featureAID := normalizeFeatureID(testFeatureA) + featureBID := normalizeFeatureID(testFeatureB) features := []*config.FeatureSet{ { ConfigID: featureAID, Config: &config.FeatureConfig{ DependsOn: config.DependsOnField{}, - InstallsAfter: []string{"feature-b"}, + InstallsAfter: []string{testFeatureB}, }, }, { @@ -322,21 +322,21 @@ func (suite *ExtendTestSuite) TestFeatureOrder_ForwardReferenceInInstallsAfter() } func (suite *ExtendTestSuite) TestFeatureOrder_MutualInstallsAfterDoesNotCycle() { - featureAID := normalizeFeatureID("feature-a") - featureBID := normalizeFeatureID("feature-b") + featureAID := normalizeFeatureID(testFeatureA) + featureBID := normalizeFeatureID(testFeatureB) features := []*config.FeatureSet{ { ConfigID: featureAID, Config: &config.FeatureConfig{ DependsOn: config.DependsOnField{}, - InstallsAfter: []string{"feature-b"}, + InstallsAfter: []string{testFeatureB}, }, }, { ConfigID: featureBID, Config: &config.FeatureConfig{ DependsOn: config.DependsOnField{}, - InstallsAfter: []string{"feature-a"}, + InstallsAfter: []string{testFeatureA}, }, }, } @@ -347,23 +347,23 @@ func (suite *ExtendTestSuite) TestFeatureOrder_MutualInstallsAfterDoesNotCycle() } func (suite *ExtendTestSuite) TestFeatureOrder_InstallsAfterWithHardDepDoesNotCycle() { - featureAID := normalizeFeatureID("feature-a") - featureBID := normalizeFeatureID("feature-b") + featureAID := normalizeFeatureID(testFeatureA) + featureBID := normalizeFeatureID(testFeatureB) features := []*config.FeatureSet{ { ConfigID: featureAID, Config: &config.FeatureConfig{ DependsOn: config.DependsOnField{ - "feature-b": map[string]any{}, + testFeatureB: map[string]any{}, }, - InstallsAfter: []string{"feature-b"}, + InstallsAfter: []string{testFeatureB}, }, }, { ConfigID: featureBID, Config: &config.FeatureConfig{ DependsOn: config.DependsOnField{}, - InstallsAfter: []string{"feature-a"}, + InstallsAfter: []string{testFeatureA}, }, }, } @@ -376,14 +376,14 @@ func (suite *ExtendTestSuite) TestFeatureOrder_InstallsAfterWithHardDepDoesNotCy } func (suite *ExtendTestSuite) TestFeatureOrder_TrueCircularDependencyStillDetected() { - featureAID := normalizeFeatureID("feature-a") - featureBID := normalizeFeatureID("feature-b") + featureAID := normalizeFeatureID(testFeatureA) + featureBID := normalizeFeatureID(testFeatureB) features := []*config.FeatureSet{ { ConfigID: featureAID, Config: &config.FeatureConfig{ DependsOn: config.DependsOnField{ - "feature-b": map[string]any{}, + testFeatureB: map[string]any{}, }, }, }, @@ -391,7 +391,7 @@ func (suite *ExtendTestSuite) TestFeatureOrder_TrueCircularDependencyStillDetect ConfigID: featureBID, Config: &config.FeatureConfig{ DependsOn: config.DependsOnField{ - "feature-a": map[string]any{}, + testFeatureA: map[string]any{}, }, }, }, From 21c78dab341b12e4310d771a7f4d9087272b28f4 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 5 May 2026 22:43:01 -0500 Subject: [PATCH 3/4] fix(feature): error on pure installsAfter cycles instead of silently skipping The previous fix silently skipped ALL installsAfter edges that would create cycles, including pure soft cycles. Per the devcontainer spec, pure installsAfter cycles (not backed by any hard dependsOn path) must produce a fatal error. Add edge type tracking to the graph (hard vs soft). When adding a soft edge would create a cycle, check whether the path is reachable via hard edges. If yes, skip silently (hard path already orders them). If no, return an error for the genuine circular dependency. --- pkg/devcontainer/feature/extend.go | 11 +++++- pkg/devcontainer/feature/extend_test.go | 41 +++++++++++++++++++--- pkg/devcontainer/graph/graph.go | 46 +++++++++++++++++++++---- 3 files changed, 87 insertions(+), 11 deletions(-) diff --git a/pkg/devcontainer/feature/extend.go b/pkg/devcontainer/feature/extend.go index 5c89ad874..21dc605a0 100644 --- a/pkg/devcontainer/feature/extend.go +++ b/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 @@ -780,10 +781,18 @@ func addSoftDependencies( continue } - if g.IsReachable(featureKey, depKey) { + 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 } diff --git a/pkg/devcontainer/feature/extend_test.go b/pkg/devcontainer/feature/extend_test.go index d3488901e..5cd0ca321 100644 --- a/pkg/devcontainer/feature/extend_test.go +++ b/pkg/devcontainer/feature/extend_test.go @@ -321,7 +321,7 @@ func (suite *ExtendTestSuite) TestFeatureOrder_ForwardReferenceInInstallsAfter() suite.Equal(featureAID, installationOrder[1].ConfigID) } -func (suite *ExtendTestSuite) TestFeatureOrder_MutualInstallsAfterDoesNotCycle() { +func (suite *ExtendTestSuite) TestFeatureOrder_MutualInstallsAfterProducesError() { featureAID := normalizeFeatureID(testFeatureA) featureBID := normalizeFeatureID(testFeatureB) features := []*config.FeatureSet{ @@ -341,9 +341,42 @@ func (suite *ExtendTestSuite) TestFeatureOrder_MutualInstallsAfterDoesNotCycle() }, } - installationOrder, err := getOrderedFeatureSets(features) - suite.Require().NoError(err) - suite.Len(installationOrder, 2) + _, 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() { diff --git a/pkg/devcontainer/graph/graph.go b/pkg/devcontainer/graph/graph.go index a2cc8f00c..5c4fd496f 100644 --- a/pkg/devcontainer/graph/graph.go +++ b/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), } } @@ -205,6 +207,38 @@ func (g *Graph[T]) IsReachable(from, to string) bool { 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 From 3de8e70008a2dc271a19035271e29cc6cfe388f4 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 6 May 2026 00:12:12 -0500 Subject: [PATCH 4/4] fix(metadata): exclude feature containerEnv from image metadata to prevent PATH corruption --- pkg/devcontainer/metadata/metadata.go | 11 +++++------ pkg/devcontainer/metadata/metadata_test.go | 12 ++++++------ 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/pkg/devcontainer/metadata/metadata.go b/pkg/devcontainer/metadata/metadata.go index 85346e115..d329c60be 100644 --- a/pkg/devcontainer/metadata/metadata.go +++ b/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, }, } } diff --git a/pkg/devcontainer/metadata/metadata_test.go b/pkg/devcontainer/metadata/metadata_test.go index 7c9436796..37693a9ad 100644 --- a/pkg/devcontainer/metadata/metadata_test.go +++ b/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, + ) } }