From 24d01b1518d5d59b1f618ce81a42e0fc1a745f5b Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 27 Jul 2026 21:58:19 -0500 Subject: [PATCH 1/4] fix(feature): match devcontainer CLI lockfile dependsOn format The lockfile generator emitted `dependsOn` as a JSON object mapping feature IDs to empty option objects. The reference devcontainer CLI emits it as an array of feature IDs in declaration order (`Object.keys(dependsOn)`). Retype LockedFeature.DependsOn to []string and preserve the JSON declaration order of dependsOn keys via a custom FeatureConfig unmarshaler, so generated lockfiles are byte-identical to the CLI. Signed-off-by: Samuel K --- pkg/devcontainer/config/feature.go | 80 +++++++++++++++++++++++ pkg/devcontainer/config/feature_test.go | 51 +++++++++++++++ pkg/devcontainer/feature/extend.go | 2 +- pkg/devcontainer/feature/lockfile.go | 8 +-- pkg/devcontainer/feature/lockfile_test.go | 27 ++++++++ 5 files changed, 163 insertions(+), 5 deletions(-) create mode 100644 pkg/devcontainer/config/feature_test.go diff --git a/pkg/devcontainer/config/feature.go b/pkg/devcontainer/config/feature.go index b06c948e8..bddb66e98 100644 --- a/pkg/devcontainer/config/feature.go +++ b/pkg/devcontainer/config/feature.go @@ -1,12 +1,55 @@ package config import ( + "bytes" "encoding/json" "fmt" + "sort" "github.com/devsy-org/devsy/pkg/types" ) +// objectKeyOrder returns the keys of the named top-level object field in the +// order they appear in the JSON document. It returns nil when the field is +// absent or is not a JSON object. +func objectKeyOrder(data []byte, field string) ([]string, error) { + var raw map[string]json.RawMessage + if err := json.Unmarshal(data, &raw); err != nil { + return nil, err + } + value, ok := raw[field] + if !ok { + return nil, nil + } + + dec := json.NewDecoder(bytes.NewReader(value)) + tok, err := dec.Token() + if err != nil { + return nil, err + } + if delim, ok := tok.(json.Delim); !ok || delim != '{' { + return nil, nil + } + + var keys []string + for dec.More() { + keyTok, err := dec.Token() + if err != nil { + return nil, err + } + key, ok := keyTok.(string) + if !ok { + return nil, fmt.Errorf("unexpected %s object key token: %v", field, keyTok) + } + keys = append(keys, key) + var skip json.RawMessage + if err := dec.Decode(&skip); err != nil { + return nil, err + } + } + return keys, nil +} + type FeatureSet struct { ConfigID string Version string @@ -78,6 +121,43 @@ type FeatureConfig struct { // Origin is the path where the feature was loaded from Origin string `json:"-"` + + // dependsOnOrder preserves the declaration order of DependsOn keys as they + // appear in devcontainer-feature.json. The reference devcontainer CLI emits + // lockfile dependsOn arrays in this order (Object.keys), so we retain it to + // produce byte-identical lockfiles. + dependsOnOrder []string `json:"-"` +} + +// UnmarshalJSON decodes a FeatureConfig while capturing the declaration order +// of the dependsOn object's keys, which a plain map cannot preserve. +func (c *FeatureConfig) UnmarshalJSON(data []byte) error { + type alias FeatureConfig + if err := json.Unmarshal(data, (*alias)(c)); err != nil { + return err + } + order, err := objectKeyOrder(data, "dependsOn") + if err != nil { + return err + } + c.dependsOnOrder = order + return nil +} + +// DependsOnKeys returns the dependency feature identifiers in their original +// declaration order, matching the reference devcontainer CLI's lockfile output. +// It falls back to sorted keys when order was not captured (e.g. a +// programmatically constructed config). +func (c *FeatureConfig) DependsOnKeys() []string { + if len(c.dependsOnOrder) == len(c.DependsOn) { + return c.dependsOnOrder + } + keys := make([]string, 0, len(c.DependsOn)) + for k := range c.DependsOn { + keys = append(keys, k) + } + sort.Strings(keys) + return keys } type DependsOnField map[string]any diff --git a/pkg/devcontainer/config/feature_test.go b/pkg/devcontainer/config/feature_test.go new file mode 100644 index 000000000..041a13b51 --- /dev/null +++ b/pkg/devcontainer/config/feature_test.go @@ -0,0 +1,51 @@ +package config + +import ( + "encoding/json" + "reflect" + "testing" +) + +func TestFeatureConfig_DependsOnKeysPreservesDeclarationOrder(t *testing.T) { + // Keys are intentionally not alphabetical to prove declaration order is + // preserved rather than sorted, matching the reference devcontainer CLI. + data := []byte(`{ + "id": "example", + "dependsOn": { + "ghcr.io/x/zeta:1": {}, + "ghcr.io/x/alpha:1": {}, + "ghcr.io/x/mid:1": {} + } + }`) + + var cfg FeatureConfig + if err := json.Unmarshal(data, &cfg); err != nil { + t.Fatalf("unmarshal: %v", err) + } + + want := []string{"ghcr.io/x/zeta:1", "ghcr.io/x/alpha:1", "ghcr.io/x/mid:1"} + if got := cfg.DependsOnKeys(); !reflect.DeepEqual(got, want) { + t.Errorf("DependsOnKeys() = %v, want %v", got, want) + } +} + +func TestFeatureConfig_DependsOnKeysFallsBackToSorted(t *testing.T) { + // A programmatically constructed config has no captured order; keys must be + // returned deterministically (sorted). + cfg := FeatureConfig{DependsOn: DependsOnField{ + "ghcr.io/x/zeta:1": map[string]any{}, + "ghcr.io/x/alpha:1": map[string]any{}, + }} + + want := []string{"ghcr.io/x/alpha:1", "ghcr.io/x/zeta:1"} + if got := cfg.DependsOnKeys(); !reflect.DeepEqual(got, want) { + t.Errorf("DependsOnKeys() = %v, want %v", got, want) + } +} + +func TestFeatureConfig_DependsOnKeysEmpty(t *testing.T) { + var cfg FeatureConfig + if got := cfg.DependsOnKeys(); len(got) != 0 { + t.Errorf("DependsOnKeys() = %v, want empty", got) + } +} diff --git a/pkg/devcontainer/feature/extend.go b/pkg/devcontainer/feature/extend.go index db306e351..3b6aa08d2 100644 --- a/pkg/devcontainer/feature/extend.go +++ b/pkg/devcontainer/feature/extend.go @@ -529,7 +529,7 @@ func (p *featureProcessor) recordLockEntry( Integrity: res.integrity, } if len(cfg.DependsOn) > 0 { - entry.DependsOn = map[string]any(cfg.DependsOn) + entry.DependsOn = cfg.DependsOnKeys() } p.lock.record(featureID, entry) } diff --git a/pkg/devcontainer/feature/lockfile.go b/pkg/devcontainer/feature/lockfile.go index f4cc59a0b..61483daf6 100644 --- a/pkg/devcontainer/feature/lockfile.go +++ b/pkg/devcontainer/feature/lockfile.go @@ -16,10 +16,10 @@ import ( // LockedFeature is a single pinned entry in a devcontainer-lock.json file. It // mirrors the structure produced by the reference devcontainer CLI. type LockedFeature struct { - Version string `json:"version,omitempty"` - Resolved string `json:"resolved,omitempty"` - Integrity string `json:"integrity,omitempty"` - DependsOn map[string]any `json:"dependsOn,omitempty"` + Version string `json:"version,omitempty"` + Resolved string `json:"resolved,omitempty"` + Integrity string `json:"integrity,omitempty"` + DependsOn []string `json:"dependsOn,omitempty"` } // Lockfile mirrors the devcontainer-lock.json structure: a map of feature diff --git a/pkg/devcontainer/feature/lockfile_test.go b/pkg/devcontainer/feature/lockfile_test.go index dbfab6d80..8ee943812 100644 --- a/pkg/devcontainer/feature/lockfile_test.go +++ b/pkg/devcontainer/feature/lockfile_test.go @@ -125,6 +125,33 @@ func TestWriteLockfile_CreatesSortedStable(t *testing.T) { } } +func TestWriteLockfile_DependsOnIsArray(t *testing.T) { + path := filepath.Join(t.TempDir(), "devcontainer-lock.json") + lf := &Lockfile{Features: map[string]LockedFeature{ + lockTestFeatureA: { + Version: lockTestVersion, + Resolved: lockTestResolvedA, + Integrity: lockTestShaA, + DependsOn: []string{"ghcr.io/b/feature:1"}, + }, + }} + + if err := WriteLockfile(path, lf, false); err != nil { + t.Fatalf("WriteLockfile: %v", err) + } + + data, err := os.ReadFile(filepath.Clean(path)) + if err != nil { + t.Fatal(err) + } + content := string(data) + // The reference devcontainer CLI serializes dependsOn as an array of + // feature identifiers, not an object. + if !strings.Contains(content, "\"dependsOn\": [") { + t.Errorf("expected dependsOn as JSON array, got:\n%s", content) + } +} + func TestWriteLockfile_SkipsUnchanged(t *testing.T) { path := filepath.Join(t.TempDir(), "devcontainer-lock.json") lf := &Lockfile{Features: map[string]LockedFeature{ From 01c3a5830befdf9539b6d2bf5ed21080713d9dde Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 27 Jul 2026 22:17:43 -0500 Subject: [PATCH 2/4] fix(feature): resolve cyclop and goconst lint findings Signed-off-by: Samuel K --- pkg/devcontainer/config/feature.go | 21 +++++++++++---------- pkg/devcontainer/config/feature_test.go | 14 ++++++++++---- 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/pkg/devcontainer/config/feature.go b/pkg/devcontainer/config/feature.go index bddb66e98..6d4f1eae5 100644 --- a/pkg/devcontainer/config/feature.go +++ b/pkg/devcontainer/config/feature.go @@ -22,14 +22,19 @@ func objectKeyOrder(data []byte, field string) ([]string, error) { return nil, nil } + var obj map[string]json.RawMessage + if err := json.Unmarshal(value, &obj); err != nil { + return nil, nil //nolint:nilerr // non-object values carry no key order + } + return decodeObjectKeys(value) +} + +// decodeObjectKeys reads the keys of a JSON object in document order. +func decodeObjectKeys(value json.RawMessage) ([]string, error) { dec := json.NewDecoder(bytes.NewReader(value)) - tok, err := dec.Token() - if err != nil { + if _, err := dec.Token(); err != nil { // consume opening '{' return nil, err } - if delim, ok := tok.(json.Delim); !ok || delim != '{' { - return nil, nil - } var keys []string for dec.More() { @@ -37,11 +42,7 @@ func objectKeyOrder(data []byte, field string) ([]string, error) { if err != nil { return nil, err } - key, ok := keyTok.(string) - if !ok { - return nil, fmt.Errorf("unexpected %s object key token: %v", field, keyTok) - } - keys = append(keys, key) + keys = append(keys, keyTok.(string)) var skip json.RawMessage if err := dec.Decode(&skip); err != nil { return nil, err diff --git a/pkg/devcontainer/config/feature_test.go b/pkg/devcontainer/config/feature_test.go index 041a13b51..63f640904 100644 --- a/pkg/devcontainer/config/feature_test.go +++ b/pkg/devcontainer/config/feature_test.go @@ -6,6 +6,12 @@ import ( "testing" ) +const ( + depZeta = "ghcr.io/x/zeta:1" + depAlpha = "ghcr.io/x/alpha:1" + depMid = "ghcr.io/x/mid:1" +) + func TestFeatureConfig_DependsOnKeysPreservesDeclarationOrder(t *testing.T) { // Keys are intentionally not alphabetical to prove declaration order is // preserved rather than sorted, matching the reference devcontainer CLI. @@ -23,7 +29,7 @@ func TestFeatureConfig_DependsOnKeysPreservesDeclarationOrder(t *testing.T) { t.Fatalf("unmarshal: %v", err) } - want := []string{"ghcr.io/x/zeta:1", "ghcr.io/x/alpha:1", "ghcr.io/x/mid:1"} + want := []string{depZeta, depAlpha, depMid} if got := cfg.DependsOnKeys(); !reflect.DeepEqual(got, want) { t.Errorf("DependsOnKeys() = %v, want %v", got, want) } @@ -33,11 +39,11 @@ func TestFeatureConfig_DependsOnKeysFallsBackToSorted(t *testing.T) { // A programmatically constructed config has no captured order; keys must be // returned deterministically (sorted). cfg := FeatureConfig{DependsOn: DependsOnField{ - "ghcr.io/x/zeta:1": map[string]any{}, - "ghcr.io/x/alpha:1": map[string]any{}, + depZeta: map[string]any{}, + depAlpha: map[string]any{}, }} - want := []string{"ghcr.io/x/alpha:1", "ghcr.io/x/zeta:1"} + want := []string{depAlpha, depZeta} if got := cfg.DependsOnKeys(); !reflect.DeepEqual(got, want) { t.Errorf("DependsOnKeys() = %v, want %v", got, want) } From 675cd27c332a833e5750799f64c036652d112c62 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 27 Jul 2026 22:30:46 -0500 Subject: [PATCH 3/4] fix(feature): harden dependsOn ordering and lockfile back-compat Address review feedback: - DependsOnKeys now retains captured declaration order only when every captured key still exists in DependsOn, guarding against post-unmarshal mutation that keeps the map size unchanged; otherwise it uses the sorted fallback. - LockedFeature.UnmarshalJSON normalizes legacy object-shaped dependsOn into the []string form, so pre-fix lockfiles still load and pin correctly. Signed-off-by: Samuel K --- pkg/devcontainer/config/feature.go | 18 ++++++- pkg/devcontainer/config/feature_test.go | 21 ++++++++ pkg/devcontainer/feature/lockfile.go | 46 +++++++++++++++++ pkg/devcontainer/feature/lockfile_test.go | 60 +++++++++++++++++++++++ 4 files changed, 144 insertions(+), 1 deletion(-) diff --git a/pkg/devcontainer/config/feature.go b/pkg/devcontainer/config/feature.go index 6d4f1eae5..79f8b3281 100644 --- a/pkg/devcontainer/config/feature.go +++ b/pkg/devcontainer/config/feature.go @@ -150,7 +150,7 @@ func (c *FeatureConfig) UnmarshalJSON(data []byte) error { // It falls back to sorted keys when order was not captured (e.g. a // programmatically constructed config). func (c *FeatureConfig) DependsOnKeys() []string { - if len(c.dependsOnOrder) == len(c.DependsOn) { + if c.capturedOrderMatchesDeps() { return c.dependsOnOrder } keys := make([]string, 0, len(c.DependsOn)) @@ -161,6 +161,22 @@ func (c *FeatureConfig) DependsOnKeys() []string { return keys } +// capturedOrderMatchesDeps reports whether the captured declaration order still +// describes exactly the current DependsOn keys. It guards against DependsOn +// being mutated after unmarshal (e.g. a key swapped without changing the count), +// in which case the sorted fallback is used instead of a stale order. +func (c *FeatureConfig) capturedOrderMatchesDeps() bool { + if len(c.dependsOnOrder) != len(c.DependsOn) { + return false + } + for _, k := range c.dependsOnOrder { + if _, ok := c.DependsOn[k]; !ok { + return false + } + } + return true +} + type DependsOnField map[string]any func (d *DependsOnField) UnmarshalJSON(data []byte) error { diff --git a/pkg/devcontainer/config/feature_test.go b/pkg/devcontainer/config/feature_test.go index 63f640904..4c199dcbb 100644 --- a/pkg/devcontainer/config/feature_test.go +++ b/pkg/devcontainer/config/feature_test.go @@ -49,6 +49,27 @@ func TestFeatureConfig_DependsOnKeysFallsBackToSorted(t *testing.T) { } } +func TestFeatureConfig_DependsOnKeysFallsBackWhenKeyMutated(t *testing.T) { + // Declaration order is captured for zeta+alpha, then a key is swapped for a + // different one without changing the map size. The stale captured order must + // be rejected in favor of the sorted fallback. + data := []byte( + `{"id": "example", "dependsOn": {"ghcr.io/x/zeta:1": {}, "ghcr.io/x/alpha:1": {}}}`, + ) + + var cfg FeatureConfig + if err := json.Unmarshal(data, &cfg); err != nil { + t.Fatalf("unmarshal: %v", err) + } + delete(cfg.DependsOn, depAlpha) + cfg.DependsOn[depMid] = map[string]any{} + + want := []string{depMid, depZeta} + if got := cfg.DependsOnKeys(); !reflect.DeepEqual(got, want) { + t.Errorf("DependsOnKeys() = %v, want %v", got, want) + } +} + func TestFeatureConfig_DependsOnKeysEmpty(t *testing.T) { var cfg FeatureConfig if got := cfg.DependsOnKeys(); len(got) != 0 { diff --git a/pkg/devcontainer/feature/lockfile.go b/pkg/devcontainer/feature/lockfile.go index 61483daf6..fb9e5e4a3 100644 --- a/pkg/devcontainer/feature/lockfile.go +++ b/pkg/devcontainer/feature/lockfile.go @@ -7,6 +7,7 @@ import ( "fmt" "os" "path/filepath" + "sort" "strings" "github.com/devsy-org/devsy/pkg/devcontainer/config" @@ -22,6 +23,51 @@ type LockedFeature struct { DependsOn []string `json:"dependsOn,omitempty"` } +// UnmarshalJSON decodes a LockedFeature, normalizing dependsOn into the array +// form regardless of how it was persisted. Older lockfiles (and those written +// by pre-fix devsy builds) stored dependsOn as an object mapping feature IDs to +// option objects; accepting both keeps pinning working across the transition. +func (f *LockedFeature) UnmarshalJSON(data []byte) error { + type alias LockedFeature + aux := struct { + DependsOn json.RawMessage `json:"dependsOn,omitempty"` + *alias + }{alias: (*alias)(f)} + if err := json.Unmarshal(data, &aux); err != nil { + return err + } + deps, err := normalizeLockDependsOn(aux.DependsOn) + if err != nil { + return err + } + f.DependsOn = deps + return nil +} + +// normalizeLockDependsOn converts a persisted dependsOn value into the array +// form. Array values are returned as-is; legacy object values contribute their +// keys in sorted order (declaration order is unavailable and irrelevant, as the +// loaded value is only used for pinning, never rewritten). +func normalizeLockDependsOn(raw json.RawMessage) ([]string, error) { + if len(bytes.TrimSpace(raw)) == 0 { + return nil, nil + } + var arr []string + if err := json.Unmarshal(raw, &arr); err == nil { + return arr, nil + } + var obj map[string]json.RawMessage + if err := json.Unmarshal(raw, &obj); err != nil { + return nil, fmt.Errorf("lockfile dependsOn must be an array or object: %w", err) + } + keys := make([]string, 0, len(obj)) + for k := range obj { + keys = append(keys, k) + } + sort.Strings(keys) + return keys, nil +} + // Lockfile mirrors the devcontainer-lock.json structure: a map of feature // identifier to its pinned resolution. type Lockfile struct { diff --git a/pkg/devcontainer/feature/lockfile_test.go b/pkg/devcontainer/feature/lockfile_test.go index 8ee943812..8ed3d6498 100644 --- a/pkg/devcontainer/feature/lockfile_test.go +++ b/pkg/devcontainer/feature/lockfile_test.go @@ -3,6 +3,7 @@ package feature import ( "os" "path/filepath" + "reflect" "strings" "testing" @@ -92,6 +93,65 @@ func TestReadLockfile_ParsesEntries(t *testing.T) { } } +func TestReadLockfile_NormalizesLegacyObjectDependsOn(t *testing.T) { + path := filepath.Join(t.TempDir(), "devcontainer-lock.json") + // Legacy/pre-fix lockfiles stored dependsOn as an object mapping IDs to + // option objects. ReadLockfile must accept it and keep pinning working. + content := `{ + "features": { + "ghcr.io/x/go-task:1": { + "version": "1.0.0", + "resolved": "ghcr.io/x/go-task@sha256:t", + "integrity": "sha256:t", + "dependsOn": { + "ghcr.io/x/picolayer:1": {} + } + } + } +}` + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + + lf, err := ReadLockfile(path) + if err != nil { + t.Fatalf("ReadLockfile: %v", err) + } + entry := lf.Features["ghcr.io/x/go-task:1"] + if entry.Integrity != "sha256:t" { + t.Errorf("integrity lost: %+v", entry) + } + want := []string{"ghcr.io/x/picolayer:1"} + if !reflect.DeepEqual(entry.DependsOn, want) { + t.Errorf("dependsOn = %v, want %v", entry.DependsOn, want) + } +} + +func TestReadLockfile_KeepsArrayDependsOn(t *testing.T) { + path := filepath.Join(t.TempDir(), "devcontainer-lock.json") + content := `{ + "features": { + "ghcr.io/x/go-task:1": { + "version": "1.0.0", + "integrity": "sha256:t", + "dependsOn": ["ghcr.io/x/picolayer:1"] + } + } +}` + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + + lf, err := ReadLockfile(path) + if err != nil { + t.Fatalf("ReadLockfile: %v", err) + } + want := []string{"ghcr.io/x/picolayer:1"} + if got := lf.Features["ghcr.io/x/go-task:1"].DependsOn; !reflect.DeepEqual(got, want) { + t.Errorf("dependsOn = %v, want %v", got, want) + } +} + func TestWriteLockfile_CreatesSortedStable(t *testing.T) { path := filepath.Join(t.TempDir(), "devcontainer-lock.json") lf := &Lockfile{Features: map[string]LockedFeature{ From 52eb63474c5b3572a6625fdc645bc6cefb5930e9 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 27 Jul 2026 22:39:48 -0500 Subject: [PATCH 4/4] refactor(feature): drop lockfile back-compat and explanatory comments Signed-off-by: Samuel K --- pkg/devcontainer/config/feature.go | 20 +------- pkg/devcontainer/config/feature_test.go | 7 --- pkg/devcontainer/feature/lockfile.go | 46 ----------------- pkg/devcontainer/feature/lockfile_test.go | 62 ----------------------- 4 files changed, 1 insertion(+), 134 deletions(-) diff --git a/pkg/devcontainer/config/feature.go b/pkg/devcontainer/config/feature.go index 79f8b3281..12d60abb4 100644 --- a/pkg/devcontainer/config/feature.go +++ b/pkg/devcontainer/config/feature.go @@ -9,9 +9,6 @@ import ( "github.com/devsy-org/devsy/pkg/types" ) -// objectKeyOrder returns the keys of the named top-level object field in the -// order they appear in the JSON document. It returns nil when the field is -// absent or is not a JSON object. func objectKeyOrder(data []byte, field string) ([]string, error) { var raw map[string]json.RawMessage if err := json.Unmarshal(data, &raw); err != nil { @@ -29,10 +26,9 @@ func objectKeyOrder(data []byte, field string) ([]string, error) { return decodeObjectKeys(value) } -// decodeObjectKeys reads the keys of a JSON object in document order. func decodeObjectKeys(value json.RawMessage) ([]string, error) { dec := json.NewDecoder(bytes.NewReader(value)) - if _, err := dec.Token(); err != nil { // consume opening '{' + if _, err := dec.Token(); err != nil { return nil, err } @@ -123,15 +119,9 @@ type FeatureConfig struct { // Origin is the path where the feature was loaded from Origin string `json:"-"` - // dependsOnOrder preserves the declaration order of DependsOn keys as they - // appear in devcontainer-feature.json. The reference devcontainer CLI emits - // lockfile dependsOn arrays in this order (Object.keys), so we retain it to - // produce byte-identical lockfiles. dependsOnOrder []string `json:"-"` } -// UnmarshalJSON decodes a FeatureConfig while capturing the declaration order -// of the dependsOn object's keys, which a plain map cannot preserve. func (c *FeatureConfig) UnmarshalJSON(data []byte) error { type alias FeatureConfig if err := json.Unmarshal(data, (*alias)(c)); err != nil { @@ -145,10 +135,6 @@ func (c *FeatureConfig) UnmarshalJSON(data []byte) error { return nil } -// DependsOnKeys returns the dependency feature identifiers in their original -// declaration order, matching the reference devcontainer CLI's lockfile output. -// It falls back to sorted keys when order was not captured (e.g. a -// programmatically constructed config). func (c *FeatureConfig) DependsOnKeys() []string { if c.capturedOrderMatchesDeps() { return c.dependsOnOrder @@ -161,10 +147,6 @@ func (c *FeatureConfig) DependsOnKeys() []string { return keys } -// capturedOrderMatchesDeps reports whether the captured declaration order still -// describes exactly the current DependsOn keys. It guards against DependsOn -// being mutated after unmarshal (e.g. a key swapped without changing the count), -// in which case the sorted fallback is used instead of a stale order. func (c *FeatureConfig) capturedOrderMatchesDeps() bool { if len(c.dependsOnOrder) != len(c.DependsOn) { return false diff --git a/pkg/devcontainer/config/feature_test.go b/pkg/devcontainer/config/feature_test.go index 4c199dcbb..48565250e 100644 --- a/pkg/devcontainer/config/feature_test.go +++ b/pkg/devcontainer/config/feature_test.go @@ -13,8 +13,6 @@ const ( ) func TestFeatureConfig_DependsOnKeysPreservesDeclarationOrder(t *testing.T) { - // Keys are intentionally not alphabetical to prove declaration order is - // preserved rather than sorted, matching the reference devcontainer CLI. data := []byte(`{ "id": "example", "dependsOn": { @@ -36,8 +34,6 @@ func TestFeatureConfig_DependsOnKeysPreservesDeclarationOrder(t *testing.T) { } func TestFeatureConfig_DependsOnKeysFallsBackToSorted(t *testing.T) { - // A programmatically constructed config has no captured order; keys must be - // returned deterministically (sorted). cfg := FeatureConfig{DependsOn: DependsOnField{ depZeta: map[string]any{}, depAlpha: map[string]any{}, @@ -50,9 +46,6 @@ func TestFeatureConfig_DependsOnKeysFallsBackToSorted(t *testing.T) { } func TestFeatureConfig_DependsOnKeysFallsBackWhenKeyMutated(t *testing.T) { - // Declaration order is captured for zeta+alpha, then a key is swapped for a - // different one without changing the map size. The stale captured order must - // be rejected in favor of the sorted fallback. data := []byte( `{"id": "example", "dependsOn": {"ghcr.io/x/zeta:1": {}, "ghcr.io/x/alpha:1": {}}}`, ) diff --git a/pkg/devcontainer/feature/lockfile.go b/pkg/devcontainer/feature/lockfile.go index fb9e5e4a3..61483daf6 100644 --- a/pkg/devcontainer/feature/lockfile.go +++ b/pkg/devcontainer/feature/lockfile.go @@ -7,7 +7,6 @@ import ( "fmt" "os" "path/filepath" - "sort" "strings" "github.com/devsy-org/devsy/pkg/devcontainer/config" @@ -23,51 +22,6 @@ type LockedFeature struct { DependsOn []string `json:"dependsOn,omitempty"` } -// UnmarshalJSON decodes a LockedFeature, normalizing dependsOn into the array -// form regardless of how it was persisted. Older lockfiles (and those written -// by pre-fix devsy builds) stored dependsOn as an object mapping feature IDs to -// option objects; accepting both keeps pinning working across the transition. -func (f *LockedFeature) UnmarshalJSON(data []byte) error { - type alias LockedFeature - aux := struct { - DependsOn json.RawMessage `json:"dependsOn,omitempty"` - *alias - }{alias: (*alias)(f)} - if err := json.Unmarshal(data, &aux); err != nil { - return err - } - deps, err := normalizeLockDependsOn(aux.DependsOn) - if err != nil { - return err - } - f.DependsOn = deps - return nil -} - -// normalizeLockDependsOn converts a persisted dependsOn value into the array -// form. Array values are returned as-is; legacy object values contribute their -// keys in sorted order (declaration order is unavailable and irrelevant, as the -// loaded value is only used for pinning, never rewritten). -func normalizeLockDependsOn(raw json.RawMessage) ([]string, error) { - if len(bytes.TrimSpace(raw)) == 0 { - return nil, nil - } - var arr []string - if err := json.Unmarshal(raw, &arr); err == nil { - return arr, nil - } - var obj map[string]json.RawMessage - if err := json.Unmarshal(raw, &obj); err != nil { - return nil, fmt.Errorf("lockfile dependsOn must be an array or object: %w", err) - } - keys := make([]string, 0, len(obj)) - for k := range obj { - keys = append(keys, k) - } - sort.Strings(keys) - return keys, nil -} - // Lockfile mirrors the devcontainer-lock.json structure: a map of feature // identifier to its pinned resolution. type Lockfile struct { diff --git a/pkg/devcontainer/feature/lockfile_test.go b/pkg/devcontainer/feature/lockfile_test.go index 8ed3d6498..2b93056c2 100644 --- a/pkg/devcontainer/feature/lockfile_test.go +++ b/pkg/devcontainer/feature/lockfile_test.go @@ -3,7 +3,6 @@ package feature import ( "os" "path/filepath" - "reflect" "strings" "testing" @@ -93,65 +92,6 @@ func TestReadLockfile_ParsesEntries(t *testing.T) { } } -func TestReadLockfile_NormalizesLegacyObjectDependsOn(t *testing.T) { - path := filepath.Join(t.TempDir(), "devcontainer-lock.json") - // Legacy/pre-fix lockfiles stored dependsOn as an object mapping IDs to - // option objects. ReadLockfile must accept it and keep pinning working. - content := `{ - "features": { - "ghcr.io/x/go-task:1": { - "version": "1.0.0", - "resolved": "ghcr.io/x/go-task@sha256:t", - "integrity": "sha256:t", - "dependsOn": { - "ghcr.io/x/picolayer:1": {} - } - } - } -}` - if err := os.WriteFile(path, []byte(content), 0o600); err != nil { - t.Fatal(err) - } - - lf, err := ReadLockfile(path) - if err != nil { - t.Fatalf("ReadLockfile: %v", err) - } - entry := lf.Features["ghcr.io/x/go-task:1"] - if entry.Integrity != "sha256:t" { - t.Errorf("integrity lost: %+v", entry) - } - want := []string{"ghcr.io/x/picolayer:1"} - if !reflect.DeepEqual(entry.DependsOn, want) { - t.Errorf("dependsOn = %v, want %v", entry.DependsOn, want) - } -} - -func TestReadLockfile_KeepsArrayDependsOn(t *testing.T) { - path := filepath.Join(t.TempDir(), "devcontainer-lock.json") - content := `{ - "features": { - "ghcr.io/x/go-task:1": { - "version": "1.0.0", - "integrity": "sha256:t", - "dependsOn": ["ghcr.io/x/picolayer:1"] - } - } -}` - if err := os.WriteFile(path, []byte(content), 0o600); err != nil { - t.Fatal(err) - } - - lf, err := ReadLockfile(path) - if err != nil { - t.Fatalf("ReadLockfile: %v", err) - } - want := []string{"ghcr.io/x/picolayer:1"} - if got := lf.Features["ghcr.io/x/go-task:1"].DependsOn; !reflect.DeepEqual(got, want) { - t.Errorf("dependsOn = %v, want %v", got, want) - } -} - func TestWriteLockfile_CreatesSortedStable(t *testing.T) { path := filepath.Join(t.TempDir(), "devcontainer-lock.json") lf := &Lockfile{Features: map[string]LockedFeature{ @@ -205,8 +145,6 @@ func TestWriteLockfile_DependsOnIsArray(t *testing.T) { t.Fatal(err) } content := string(data) - // The reference devcontainer CLI serializes dependsOn as an array of - // feature identifiers, not an object. if !strings.Contains(content, "\"dependsOn\": [") { t.Errorf("expected dependsOn as JSON array, got:\n%s", content) }