From 179868b56f2283223f23e3a110edf7afdde6e2df Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Thu, 23 Apr 2026 17:05:41 -0400 Subject: [PATCH 01/11] fix(config): ResolveForEnv lifts Config["name"] into ResolvedModule.Name When an env override sets a "name" key in Config, promote it to ResolvedModule.Name and delete it from Config. This ensures that ResourceSpec.Name carries the env-resolved name (e.g. "bmw-staging-vpc" instead of "bmw-vpc") so plan and apply agree on resource identity. Empty string is ignored to prevent accidental erasure of the module name. Closes follow-up #32. Tests: LiftsConfigNameIntoIdentity, PreservesNameWhenNoOverride, EmptyNameFieldIgnored. Co-Authored-By: Claude Sonnet 4.6 --- config/module_resolve_env.go | 8 ++++ config/module_resolve_env_test.go | 68 +++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+) diff --git a/config/module_resolve_env.go b/config/module_resolve_env.go index 8315093ca..154bdefda 100644 --- a/config/module_resolve_env.go +++ b/config/module_resolve_env.go @@ -53,6 +53,14 @@ func (m *ModuleConfig) ResolveForEnv(envName string) (*ResolvedModule, bool) { resolved.Config = deepMergeMap(resolved.Config, envRes.Config) } + // If an env override sets "name", lift it into ResolvedModule.Name and + // delete it from Config so downstream ResourceSpec construction uses the + // correct identity. Empty string is ignored to prevent accidental erasure. + if n, ok := resolved.Config["name"].(string); ok && n != "" { + resolved.Name = n + delete(resolved.Config, "name") + } + setRegionFromConfig(resolved) // re-apply after env overrides // Write region into Config so downstream ResourceSpec construction sees it. if resolved.Region != "" { diff --git a/config/module_resolve_env_test.go b/config/module_resolve_env_test.go index 5fce3b744..7ba5de1fb 100644 --- a/config/module_resolve_env_test.go +++ b/config/module_resolve_env_test.go @@ -174,6 +174,74 @@ func TestResolveForEnv_ProviderWrittenToConfig(t *testing.T) { } } +// ── Fix 1: name lift ────────────────────────────────────────────────────────── + +func TestResolveForEnv_LiftsConfigNameIntoIdentity(t *testing.T) { + m := &ModuleConfig{ + Name: "bmw-vpc", + Type: "infra.vpc", + Config: map[string]any{"cidr": "10.0.0.0/24"}, + Environments: map[string]*InfraEnvironmentResolution{ + "staging": {Config: map[string]any{"name": "bmw-staging-vpc"}}, + }, + } + resolved, ok := m.ResolveForEnv("staging") + if !ok { + t.Fatal("ResolveForEnv returned !ok") + } + if resolved.Name != "bmw-staging-vpc" { + t.Errorf("Name = %q, want bmw-staging-vpc", resolved.Name) + } + if _, present := resolved.Config["name"]; present { + t.Error("name should be stripped from Config after lift") + } + // Original cidr must still be present. + if resolved.Config["cidr"] != "10.0.0.0/24" { + t.Errorf("cidr should be preserved, got %v", resolved.Config["cidr"]) + } +} + +func TestResolveForEnv_PreservesNameWhenNoOverride(t *testing.T) { + m := &ModuleConfig{ + Name: "bmw-db", + Type: "infra.database", + Config: map[string]any{"engine": "postgres"}, + Environments: map[string]*InfraEnvironmentResolution{ + "staging": {Config: map[string]any{"size": "small"}}, + }, + } + resolved, ok := m.ResolveForEnv("staging") + if !ok { + t.Fatal("ResolveForEnv returned !ok") + } + // No name override in env — module name must be preserved. + if resolved.Name != "bmw-db" { + t.Errorf("Name = %q, want bmw-db", resolved.Name) + } + if _, present := resolved.Config["name"]; present { + t.Error("name key must not appear in Config when no override was set") + } +} + +func TestResolveForEnv_EmptyNameFieldIgnored(t *testing.T) { + m := &ModuleConfig{ + Name: "bmw-firewall", + Type: "infra.firewall", + Config: map[string]any{}, + Environments: map[string]*InfraEnvironmentResolution{ + "staging": {Config: map[string]any{"name": ""}}, + }, + } + resolved, ok := m.ResolveForEnv("staging") + if !ok { + t.Fatal("ResolveForEnv returned !ok") + } + // Empty string name must NOT overwrite the module identity. + if resolved.Name != "bmw-firewall" { + t.Errorf("Name = %q, want bmw-firewall (empty name override must be ignored)", resolved.Name) + } +} + func TestResolveForEnv_ProviderOverrideWins(t *testing.T) { m := &ModuleConfig{ Name: "db", From 8c76651baac5292f8d1d308ccb360ecf9a58887b Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Thu, 23 Apr 2026 17:08:51 -0400 Subject: [PATCH 02/11] fix(platform): configHash sorts keys explicitly for determinism Use explicit sorted kv-pair encoding before SHA-256 so configHash produces the same value regardless of Go's randomised map-iteration order. Closes issue where successive applies without config changes produced spurious "update" plan actions. Add differ_hash_test.go (internal package) with stability test (100 iterations), empty-map sentinel, and inequality sanity check. Update hashConfig helper in differ_test.go to match the new encoding. Co-Authored-By: Claude Sonnet 4.6 --- platform/differ.go | 19 +++++++++++++-- platform/differ_hash_test.go | 47 ++++++++++++++++++++++++++++++++++++ platform/differ_test.go | 22 +++++++++++++++-- 3 files changed, 84 insertions(+), 4 deletions(-) create mode 100644 platform/differ_hash_test.go diff --git a/platform/differ.go b/platform/differ.go index 5b54219a1..51ebcd84d 100644 --- a/platform/differ.go +++ b/platform/differ.go @@ -4,6 +4,7 @@ import ( "crypto/sha256" "encoding/json" "fmt" + "sort" "time" "github.com/GoCodeAlone/workflow/interfaces" @@ -89,12 +90,26 @@ func ComputePlan(desired []interfaces.ResourceSpec, current []interfaces.Resourc } // configHash returns a deterministic SHA-256 hex hash of a config map. -// json.Marshal error is intentionally ignored: map[string]any always marshals. +// Keys are explicitly sorted before marshalling so the hash is stable across +// Go's randomised map-iteration order — matching the DO plugin's pattern. func configHash(config map[string]any) string { if len(config) == 0 { return "" } - data, _ := json.Marshal(config) // map[string]any is always marshalable + keys := make([]string, 0, len(config)) + for k := range config { + keys = append(keys, k) + } + sort.Strings(keys) + type kv struct { + K string + V any + } + ordered := make([]kv, len(keys)) + for i, k := range keys { + ordered[i] = kv{K: k, V: config[k]} + } + data, _ := json.Marshal(ordered) return fmt.Sprintf("%x", sha256.Sum256(data)) } diff --git a/platform/differ_hash_test.go b/platform/differ_hash_test.go new file mode 100644 index 000000000..b29929efe --- /dev/null +++ b/platform/differ_hash_test.go @@ -0,0 +1,47 @@ +package platform + +import "testing" + +// TestConfigHash_Stable_AcrossMapIterationOrder verifies that configHash +// returns the same value on every call for a given config map. Go map +// iteration is deliberately randomised; this test runs 100 iterations to +// expose any hash instability that would cause spurious "update" plan actions +// on successive applies with an unchanged config. +func TestConfigHash_Stable_AcrossMapIterationOrder(t *testing.T) { + config := map[string]any{ + "engine": "postgres", + "size": "medium", + "region": "nyc3", + "replicas": 3, + "tags": map[string]any{"env": "staging", "team": "platform"}, + } + first := configHash(config) + if first == "" { + t.Fatal("configHash returned empty string for non-empty config") + } + for i := 0; i < 100; i++ { + if h := configHash(config); h != first { + t.Fatalf("iteration %d: got %q, want %q — hash is non-deterministic", i, h, first) + } + } +} + +// TestConfigHash_EmptyMapReturnsEmpty verifies the zero-value sentinel. +func TestConfigHash_EmptyMapReturnsEmpty(t *testing.T) { + if got := configHash(nil); got != "" { + t.Errorf("nil map: want %q, got %q", "", got) + } + if got := configHash(map[string]any{}); got != "" { + t.Errorf("empty map: want %q, got %q", "", got) + } +} + +// TestConfigHash_DifferentConfigsDifferentHashes is a basic sanity check that +// two semantically different configs produce different hashes. +func TestConfigHash_DifferentConfigsDifferentHashes(t *testing.T) { + a := map[string]any{"engine": "postgres"} + b := map[string]any{"engine": "mysql"} + if configHash(a) == configHash(b) { + t.Error("different configs produced identical hashes") + } +} diff --git a/platform/differ_test.go b/platform/differ_test.go index 42ed4cd95..0d733f23d 100644 --- a/platform/differ_test.go +++ b/platform/differ_test.go @@ -4,6 +4,7 @@ import ( "crypto/sha256" "encoding/json" "fmt" + "sort" "strings" "testing" @@ -11,9 +12,26 @@ import ( "github.com/GoCodeAlone/workflow/platform" ) -// hashConfig produces a deterministic SHA-256 hex hash of a config map for test setup. +// hashConfig produces a deterministic SHA-256 hex hash of a config map for +// test setup. Must match platform.configHash — sorted kv-pair encoding. func hashConfig(config map[string]any) string { - data, _ := json.Marshal(config) + if len(config) == 0 { + return "" + } + keys := make([]string, 0, len(config)) + for k := range config { + keys = append(keys, k) + } + sort.Strings(keys) + type kv struct { + K string + V any + } + ordered := make([]kv, len(keys)) + for i, k := range keys { + ordered[i] = kv{K: k, V: config[k]} + } + data, _ := json.Marshal(ordered) return fmt.Sprintf("%x", sha256.Sum256(data)) } From 8faadf9d7dbb222b9faed9504ecf0b902240f53f Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Thu, 23 Apr 2026 17:23:42 -0400 Subject: [PATCH 03/11] fix(wfctl): invoke provider.ResolveSizing before plan for sized specs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For each ResourceSpec with a non-empty Size field, call provider.ResolveSizing(type, size, hints) inside applyWithProviderAndStore before ComputePlan. The returned ProviderSizing.InstanceType and extra Specs are merged into spec.Config so that plan and apply agree on the concrete instance type (e.g. Size:"m" → instance_type:"s-1vcpu-2gb"). If the provider returns nil (no resolution needed), the spec is unchanged. Also aligns configHashMap in infra.go with platform.configHash: use sorted kv-pair encoding so ResourceState.ConfigHash values written during apply are comparable on the next run's ComputePlan. Tests: TestApplyInfraModules_CallsResolveSizing_ForEachSpec verifies ResolveSizing is called exactly once per sized spec and that spec.Config is enriched before Apply. Updated TestApplyWithProvider_NoChanges and TestApplyWithProvider_DeletesRemovedResource to use configHashMap(). Co-Authored-By: Claude Sonnet 4.6 --- cmd/wfctl/infra.go | 18 +++++- cmd/wfctl/infra_apply.go | 23 ++++++++ cmd/wfctl/infra_apply_test.go | 107 ++++++++++++++++++++++++++++++---- 3 files changed, 136 insertions(+), 12 deletions(-) diff --git a/cmd/wfctl/infra.go b/cmd/wfctl/infra.go index 02a3c2ba7..7ef7d4c8e 100644 --- a/cmd/wfctl/infra.go +++ b/cmd/wfctl/infra.go @@ -7,6 +7,7 @@ import ( "flag" "fmt" "os" + "sort" "strings" "github.com/GoCodeAlone/workflow/config" @@ -354,11 +355,26 @@ func loadCurrentState(cfgFile string) []interfaces.ResourceState { } // configHashMap computes a deterministic SHA-256 hex hash of a config map. +// Keys are explicitly sorted before marshalling to match platform.configHash +// so that saved ConfigHash values are comparable across calls and round-trips. func configHashMap(config map[string]any) string { if len(config) == 0 { return "" } - data, _ := json.Marshal(config) + keys := make([]string, 0, len(config)) + for k := range config { + keys = append(keys, k) + } + sort.Strings(keys) + type kv struct { + K string + V any + } + ordered := make([]kv, len(keys)) + for i, k := range keys { + ordered[i] = kv{K: k, V: config[k]} + } + data, _ := json.Marshal(ordered) return fmt.Sprintf("%x", sha256.Sum256(data)) } diff --git a/cmd/wfctl/infra_apply.go b/cmd/wfctl/infra_apply.go index 6cb3a176d..2f37e5b26 100644 --- a/cmd/wfctl/infra_apply.go +++ b/cmd/wfctl/infra_apply.go @@ -183,6 +183,29 @@ func applyWithProviderAndStore(ctx context.Context, provider interfaces.IaCProvi store = &noopStateStore{} } + // Resolve abstract sizing tiers into concrete provider-specific values + // (e.g. Size: "m" → instance_type: "s-1vcpu-2gb") for each spec that + // declares a Size. The resolved values are merged into spec.Config so that + // plan output and apply are always in sync. + for i := range specs { + if specs[i].Size == "" { + continue + } + sizing, err := provider.ResolveSizing(specs[i].Type, specs[i].Size, specs[i].Hints) + if err != nil { + return fmt.Errorf("%s/%s: resolve sizing: %w", specs[i].Type, specs[i].Name, err) + } + if sizing != nil { + if specs[i].Config == nil { + specs[i].Config = map[string]any{} + } + specs[i].Config["instance_type"] = sizing.InstanceType + for k, v := range sizing.Specs { + specs[i].Config[k] = v + } + } + } + // Pass the full current state to ComputePlan so that resources which were // previously provisioned but are no longer in the desired spec set generate // delete actions rather than being silently ignored. diff --git a/cmd/wfctl/infra_apply_test.go b/cmd/wfctl/infra_apply_test.go index dde803021..d264c261f 100644 --- a/cmd/wfctl/infra_apply_test.go +++ b/cmd/wfctl/infra_apply_test.go @@ -2,8 +2,6 @@ package main import ( "context" - "crypto/sha256" - "encoding/json" "fmt" "io" "os" @@ -184,13 +182,9 @@ func TestApplyWithProvider_NoChanges(t *testing.T) { Config: map[string]any{"engine": "postgres"}, } - // Reproduce the hash that platform.ComputePlan computes via configHash: - // sha256(json.Marshal(spec.Config)) in hex. - cfgData, err := json.Marshal(spec.Config) - if err != nil { - t.Fatalf("marshal config: %v", err) - } - cfgHash := fmt.Sprintf("%x", sha256.Sum256(cfgData)) + // Reproduce the hash that platform.ComputePlan computes via configHash + // (sorted kv-pair encoding): + cfgHash := configHashMap(spec.Config) current := []interfaces.ResourceState{{ Name: spec.Name, @@ -220,8 +214,7 @@ func TestApplyWithProvider_DeletesRemovedResource(t *testing.T) { {Name: "bmw-app", Type: "infra.container_service", Config: map[string]any{"image": "registry/app:latest"}}, } // Current: bmw-app + old-db (removed from config, should be deleted). - appData, _ := json.Marshal(specs[0].Config) - appHash := fmt.Sprintf("%x", sha256.Sum256(appData)) + appHash := configHashMap(specs[0].Config) current := []interfaces.ResourceState{ {Name: "bmw-app", Type: "infra.container_service", ConfigHash: appHash}, {Name: "old-db", Type: "infra.database", ConfigHash: "oldhash"}, @@ -464,6 +457,98 @@ modules: } } +// ── TestApplyInfraModules_CallsResolveSizing_ForEachSpec ────────────────────── + +// sizingCapture is an IaCProvider that records every ResolveSizing call and +// returns a concrete ProviderSizing so we can assert spec.Config is enriched. +type sizingCapture struct { + applyCapture + sizingCalls []struct { + resType string + size interfaces.Size + } + sizingResult *interfaces.ProviderSizing + appliedSpecs []interfaces.ResourceSpec +} + +func (s *sizingCapture) ResolveSizing(resType string, size interfaces.Size, _ *interfaces.ResourceHints) (*interfaces.ProviderSizing, error) { + s.mu.Lock() + defer s.mu.Unlock() + s.sizingCalls = append(s.sizingCalls, struct { + resType string + size interfaces.Size + }{resType: resType, size: size}) + return s.sizingResult, nil +} + +func (s *sizingCapture) Apply(_ context.Context, plan *interfaces.IaCPlan) (*interfaces.ApplyResult, error) { + s.mu.Lock() + defer s.mu.Unlock() + for _, a := range plan.Actions { + s.appliedSpecs = append(s.appliedSpecs, a.Resource) + } + return &interfaces.ApplyResult{}, nil +} + +// TestApplyInfraModules_CallsResolveSizing_ForEachSpec verifies that +// applyWithProviderAndStore invokes provider.ResolveSizing for each spec +// that has a non-empty Size field, and that the resolved InstanceType and +// extra Specs are merged into spec.Config before the plan is computed. +func TestApplyInfraModules_CallsResolveSizing_ForEachSpec(t *testing.T) { + specs := []interfaces.ResourceSpec{ + {Name: "db", Type: "infra.database", Size: interfaces.SizeM, Config: map[string]any{"engine": "postgres"}}, + {Name: "vpc", Type: "infra.vpc", Config: map[string]any{"region": "nyc3"}}, // no Size → ResolveSizing should NOT be called + {Name: "app", Type: "infra.container_service", Size: interfaces.SizeS, Config: map[string]any{"image": "nginx"}}, + } + + fake := &sizingCapture{ + sizingResult: &interfaces.ProviderSizing{ + InstanceType: "s-1vcpu-2gb", + Specs: map[string]any{"memory_mb": 2048}, + }, + } + + if err := applyWithProviderAndStore(t.Context(), fake, "fake-cloud", specs, nil, nil); err != nil { + t.Fatalf("applyWithProviderAndStore: %v", err) + } + + // ResolveSizing should have been called twice (db + app), not for vpc. + fake.mu.Lock() + calls := fake.sizingCalls + applied := fake.appliedSpecs + fake.mu.Unlock() + + if len(calls) != 2 { + t.Errorf("ResolveSizing calls = %d, want 2 (only sized specs)", len(calls)) + } + callTypes := map[string]interfaces.Size{} + for _, c := range calls { + callTypes[c.resType] = c.size + } + if callTypes["infra.database"] != interfaces.SizeM { + t.Errorf("infra.database sizing call size = %q, want %q", callTypes["infra.database"], interfaces.SizeM) + } + if callTypes["infra.container_service"] != interfaces.SizeS { + t.Errorf("infra.container_service sizing call size = %q, want %q", callTypes["infra.container_service"], interfaces.SizeS) + } + + // The applied specs should carry the resolved instance_type in their Config. + if len(applied) == 0 { + t.Fatal("no specs were applied — Apply was not called or plan had no actions") + } + for _, s := range applied { + if s.Size == "" { + continue // vpc — no sizing expected + } + if s.Config["instance_type"] != "s-1vcpu-2gb" { + t.Errorf("spec %q: Config[instance_type] = %v, want s-1vcpu-2gb", s.Name, s.Config["instance_type"]) + } + if s.Config["memory_mb"] != 2048 { + t.Errorf("spec %q: Config[memory_mb] = %v, want 2048", s.Name, s.Config["memory_mb"]) + } + } +} + // TestHasInfraModules verifies detection of infra.* vs platform.* configs. func TestHasInfraModules(t *testing.T) { dir := t.TempDir() From 76497e57aa059019ef377e4b871f9663f5e750f8 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Thu, 23 Apr 2026 17:24:44 -0400 Subject: [PATCH 04/11] fix(wfctl): log provider Close() errors as stderr warnings Replace `defer closer.Close() //nolint:errcheck` with an explicit defer that writes `warning: provider %q shutdown: %v` to stderr in four files: - cmd/wfctl/infra_apply.go - cmd/wfctl/infra_destroy.go - cmd/wfctl/infra_status_drift.go (two closures: status + drift) - cmd/wfctl/infra_bootstrap.go Plugin subprocess leaks now surface instead of being silently discarded. Test: TestApplyWithProvider_LogsCloseError injects an error-producing io.Closer via resolveIaCProvider override, redirects os.Stderr via os.Pipe, and asserts that the warning message appears in captured output. Co-Authored-By: Claude Sonnet 4.6 --- cmd/wfctl/infra_apply.go | 8 +++- cmd/wfctl/infra_apply_test.go | 68 +++++++++++++++++++++++++++++++++ cmd/wfctl/infra_bootstrap.go | 7 +++- cmd/wfctl/infra_destroy.go | 8 +++- cmd/wfctl/infra_status_drift.go | 15 +++++++- 5 files changed, 101 insertions(+), 5 deletions(-) diff --git a/cmd/wfctl/infra_apply.go b/cmd/wfctl/infra_apply.go index 2f37e5b26..6c7f0eb05 100644 --- a/cmd/wfctl/infra_apply.go +++ b/cmd/wfctl/infra_apply.go @@ -3,6 +3,7 @@ package main import ( "context" "fmt" + "os" "strings" "time" @@ -157,7 +158,12 @@ func applyInfraModules(ctx context.Context, cfgFile, envName string) error { //n return fmt.Errorf("provider %q (%s): load provider: %w", moduleRef, g.provType, err) } if closer != nil { - defer closer.Close() //nolint:errcheck + provType := g.provType + defer func() { + if cerr := closer.Close(); cerr != nil { + fmt.Fprintf(os.Stderr, "warning: provider %q shutdown: %v\n", provType, cerr) + } + }() } return applyWithProviderAndStore(ctx, provider, g.provType, g.specs, current, store) } diff --git a/cmd/wfctl/infra_apply_test.go b/cmd/wfctl/infra_apply_test.go index d264c261f..91d483e4d 100644 --- a/cmd/wfctl/infra_apply_test.go +++ b/cmd/wfctl/infra_apply_test.go @@ -1,6 +1,7 @@ package main import ( + "bytes" "context" "fmt" "io" @@ -580,3 +581,70 @@ modules: t.Error("hasInfraModules: want false for platform.* config, got true") } } + +// ── TestApplyWithProvider_LogsCloseError ────────────────────────────────────── + +// errCloser is an io.Closer that always returns an error. +type errCloser struct{ msg string } + +func (e *errCloser) Close() error { return fmt.Errorf("%s", e.msg) } + +// TestApplyWithProvider_LogsCloseError verifies that when the provider closer +// returns an error during applyInfraModules, a warning is written to stderr +// (instead of silently discarding the error via nolint:errcheck). +func TestApplyWithProvider_LogsCloseError(t *testing.T) { + dir := t.TempDir() + cfgPath := filepath.Join(dir, "infra.yaml") + if err := os.WriteFile(cfgPath, []byte(` +modules: + - name: myprov + type: iac.provider + config: + provider: fake-cloud + - name: my-vpc + type: infra.vpc + config: + provider: myprov + region: nyc3 +`), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + + // Override resolveIaCProvider to return a provider + error-producing closer. + orig := resolveIaCProvider + fake := &applyCapture{} + closerErr := "shutdown-sentinel-error" + resolveIaCProvider = func(_ context.Context, _ string, _ map[string]any) (interfaces.IaCProvider, io.Closer, error) { + return fake, &errCloser{msg: closerErr}, nil + } + t.Cleanup(func() { resolveIaCProvider = orig }) + + // Redirect stderr to capture warning output. + oldStderr := os.Stderr + r, w, pipeErr := os.Pipe() + if pipeErr != nil { + t.Fatalf("os.Pipe: %v", pipeErr) + } + os.Stderr = w + + err := applyInfraModules(context.Background(), cfgPath, "") + + w.Close() + os.Stderr = oldStderr + + var buf bytes.Buffer + if _, readErr := buf.ReadFrom(r); readErr != nil { + t.Fatalf("read stderr: %v", readErr) + } + stderrOutput := buf.String() + + if err != nil { + t.Fatalf("applyInfraModules returned unexpected error: %v", err) + } + if !strings.Contains(stderrOutput, closerErr) { + t.Errorf("stderr = %q, want it to contain %q", stderrOutput, closerErr) + } + if !strings.Contains(stderrOutput, "warning") { + t.Errorf("stderr = %q, want it to contain 'warning'", stderrOutput) + } +} diff --git a/cmd/wfctl/infra_bootstrap.go b/cmd/wfctl/infra_bootstrap.go index 3c60bc5e9..cfa4bb51d 100644 --- a/cmd/wfctl/infra_bootstrap.go +++ b/cmd/wfctl/infra_bootstrap.go @@ -149,7 +149,12 @@ func bootstrapStateBackend(ctx context.Context, cfgFile string) error { return fmt.Errorf("load provider %q for state backend bootstrap: %w", provType, err) } if closer != nil { - defer closer.Close() //nolint:errcheck + pType := provType + defer func() { + if cerr := closer.Close(); cerr != nil { + fmt.Fprintf(os.Stderr, "warning: provider %q shutdown: %v\n", pType, cerr) + } + }() } result, err := provider.BootstrapStateBackend(ctx, cfg) diff --git a/cmd/wfctl/infra_destroy.go b/cmd/wfctl/infra_destroy.go index 5f39673e3..d5f49a0bd 100644 --- a/cmd/wfctl/infra_destroy.go +++ b/cmd/wfctl/infra_destroy.go @@ -3,6 +3,7 @@ package main import ( "context" "fmt" + "os" "strings" "github.com/GoCodeAlone/workflow/config" @@ -109,7 +110,12 @@ func destroyInfraModules(ctx context.Context, cfgFile, envName string) error { / return fmt.Errorf("load provider %q: %w", moduleRef, err) } if closer != nil { - defer closer.Close() //nolint:errcheck + provType := g.provType + defer func() { + if cerr := closer.Close(); cerr != nil { + fmt.Fprintf(os.Stderr, "warning: provider %q shutdown: %v\n", provType, cerr) + } + }() } fmt.Printf("Destroying %d resource(s) via provider %q (%s)...\n", len(g.refs), moduleRef, g.provType) diff --git a/cmd/wfctl/infra_status_drift.go b/cmd/wfctl/infra_status_drift.go index abb6934b5..a335f2968 100644 --- a/cmd/wfctl/infra_status_drift.go +++ b/cmd/wfctl/infra_status_drift.go @@ -3,6 +3,7 @@ package main import ( "context" "fmt" + "os" "strings" "github.com/GoCodeAlone/workflow/interfaces" @@ -34,7 +35,12 @@ func statusInfraModules(ctx context.Context, cfgFile, envName string) error { return } if closer != nil { - defer closer.Close() //nolint:errcheck + provType := g.provType + defer func() { + if cerr := closer.Close(); cerr != nil { + fmt.Fprintf(os.Stderr, "warning: provider %q shutdown: %v\n", provType, cerr) + } + }() } statuses, err := provider.Status(ctx, g.refs) @@ -86,7 +92,12 @@ func driftInfraModules(ctx context.Context, cfgFile, envName string) error { return false } if closer != nil { - defer closer.Close() //nolint:errcheck + provType := g.provType + defer func() { + if cerr := closer.Close(); cerr != nil { + fmt.Fprintf(os.Stderr, "warning: provider %q shutdown: %v\n", provType, cerr) + } + }() } results, err := provider.DetectDrift(ctx, g.refs) From 76d2cc047b5719421a04d3b5cc3877add079eadd Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Thu, 23 Apr 2026 17:25:16 -0400 Subject: [PATCH 05/11] test(wfctl): plan-vs-apply equivalence test harness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add infra_plan_apply_equivalence_test.go with recordingProvider and TestPlanApplyEquivalence_EnvOverrideNames — the regression gate for Bug #32 and the class of env-override name divergences: 1. Build a BMW-shaped infra.yaml with env overrides that rename every resource (bmw-vpc → bmw-staging-vpc, etc.). 2. Call planResourcesForEnv("staging") — capture intended names. 3. Call applyInfraModules with a recording fake provider that captures actual spec.Name values passed to Apply. 4. Assert the two name sets are identical. Also add TestPlanResourcesForEnv_UsesEnvOverrideNames to infra_env_wire_test.go — unit-level assertion that planResourcesForEnv returns env-override names, not raw module names. Co-Authored-By: Claude Sonnet 4.6 --- cmd/wfctl/infra_env_wire_test.go | 54 +++++++ .../infra_plan_apply_equivalence_test.go | 138 ++++++++++++++++++ 2 files changed, 192 insertions(+) create mode 100644 cmd/wfctl/infra_plan_apply_equivalence_test.go diff --git a/cmd/wfctl/infra_env_wire_test.go b/cmd/wfctl/infra_env_wire_test.go index 525354cd4..ab24e0116 100644 --- a/cmd/wfctl/infra_env_wire_test.go +++ b/cmd/wfctl/infra_env_wire_test.go @@ -225,3 +225,57 @@ modules: t.Fatal("secretStores section must be preserved in temp file") } } + +// TestPlanResourcesForEnv_UsesEnvOverrideNames asserts that planResourcesForEnv +// returns ResolvedModule.Name values from env-level config overrides (not the +// raw module names). This is the unit-level companion to +// TestPlanApplyEquivalence_EnvOverrideNames. +func TestPlanResourcesForEnv_UsesEnvOverrideNames(t *testing.T) { + dir := t.TempDir() + cfgPath := filepath.Join(dir, "infra.yaml") + if err := os.WriteFile(cfgPath, []byte(` +modules: + - name: bmw-vpc + type: infra.vpc + config: + cidr: "10.0.0.0/24" + environments: + staging: + config: + name: bmw-staging-vpc + + - name: bmw-db + type: infra.database + config: + engine: postgres + environments: + staging: + config: + name: bmw-staging-db +`), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + + resources, err := planResourcesForEnv(cfgPath, "staging") + if err != nil { + t.Fatalf("planResourcesForEnv: %v", err) + } + + names := map[string]bool{} + for _, r := range resources { + names[r.Name] = true + } + + if !names["bmw-staging-vpc"] { + t.Errorf("want bmw-staging-vpc in plan names, got %v", names) + } + if !names["bmw-staging-db"] { + t.Errorf("want bmw-staging-db in plan names, got %v", names) + } + if names["bmw-vpc"] { + t.Errorf("bmw-vpc (raw module name) should NOT appear after env override; got %v", names) + } + if names["bmw-db"] { + t.Errorf("bmw-db (raw module name) should NOT appear after env override; got %v", names) + } +} diff --git a/cmd/wfctl/infra_plan_apply_equivalence_test.go b/cmd/wfctl/infra_plan_apply_equivalence_test.go new file mode 100644 index 000000000..ab1e36d16 --- /dev/null +++ b/cmd/wfctl/infra_plan_apply_equivalence_test.go @@ -0,0 +1,138 @@ +package main + +import ( + "context" + "io" + "os" + "path/filepath" + "reflect" + "testing" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// recordingProvider captures every ResourceSpec passed to Apply actions so we +// can verify that the names plan displays are the same names apply uses. +type recordingProvider struct { + applyCapture + applied []interfaces.ResourceSpec +} + +func (r *recordingProvider) Apply(_ context.Context, plan *interfaces.IaCPlan) (*interfaces.ApplyResult, error) { + r.mu.Lock() + defer r.mu.Unlock() + for _, action := range plan.Actions { + r.applied = append(r.applied, action.Resource) + } + // Return a zero-value result so apply succeeds without cloud calls. + return &interfaces.ApplyResult{}, nil +} + +// TestPlanApplyEquivalence_EnvOverrideNames is the regression gate for Bug #32 +// and the class of env-override name divergences. It: +// 1. Builds a BMW-shaped infra.yaml with env overrides that rename every resource. +// 2. Calls planResourcesForEnv("staging") to capture what the plan renderer sees. +// 3. Calls applyInfraModules via a recording fake provider to capture actual spec names. +// 4. Asserts the two name sets are identical. +// +// This test FAILS before Fix #1 (ResolveForEnv name lift) and PASSES after. +func TestPlanApplyEquivalence_EnvOverrideNames(t *testing.T) { + dir := t.TempDir() + cfgPath := filepath.Join(dir, "infra.yaml") + if err := os.WriteFile(cfgPath, []byte(` +modules: + - name: bmw-provider + type: iac.provider + config: + provider: fake-cloud + + - name: bmw-vpc + type: infra.vpc + config: + provider: bmw-provider + cidr: "10.0.0.0/24" + environments: + staging: + config: + name: bmw-staging-vpc + + - name: bmw-firewall + type: infra.firewall + config: + provider: bmw-provider + environments: + staging: + config: + name: bmw-staging-firewall + + - name: bmw-db + type: infra.database + config: + provider: bmw-provider + engine: postgres + environments: + staging: + config: + name: bmw-staging-db + + - name: bmw-app + type: infra.container_service + config: + provider: bmw-provider + image: registry/app:latest + environments: + staging: + config: + name: bmw-staging-app +`), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + + // ── Step 1: what plan sees ──────────────────────────────────────────────── + planned, err := planResourcesForEnv(cfgPath, "staging") + if err != nil { + t.Fatalf("planResourcesForEnv: %v", err) + } + plannedNames := map[string]bool{} + for _, rm := range planned { + if rm.Type == "iac.provider" { + continue // skip provider modules — they don't become ResourceSpecs + } + plannedNames[rm.Name] = true + } + wantNames := map[string]bool{ + "bmw-staging-vpc": true, + "bmw-staging-firewall": true, + "bmw-staging-db": true, + "bmw-staging-app": true, + } + if !reflect.DeepEqual(plannedNames, wantNames) { + t.Errorf("plan names = %v, want %v", plannedNames, wantNames) + } + + // ── Step 2: what apply actually sends to the provider ──────────────────── + rp := &recordingProvider{} + orig := resolveIaCProvider + resolveIaCProvider = func(_ context.Context, _ string, _ map[string]any) (interfaces.IaCProvider, io.Closer, error) { + return rp, nil, nil + } + t.Cleanup(func() { resolveIaCProvider = orig }) + + if err := applyInfraModules(context.Background(), cfgPath, "staging"); err != nil { + t.Fatalf("applyInfraModules: %v", err) + } + + rp.mu.Lock() + appliedSpecs := rp.applied + rp.mu.Unlock() + + actualNames := map[string]bool{} + for _, s := range appliedSpecs { + actualNames[s.Name] = true + } + + // ── Step 3: assert plan names == apply names ────────────────────────────── + if !reflect.DeepEqual(plannedNames, actualNames) { + t.Errorf("plan-vs-apply name divergence:\n plan: %v\n apply: %v", plannedNames, actualNames) + } +} From 3ff708673638996071e8c607084d35d95856759e Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Thu, 23 Apr 2026 17:25:22 -0400 Subject: [PATCH 06/11] =?UTF-8?q?chore:=20CHANGELOG=20v0.18.7=20=E2=80=94?= =?UTF-8?q?=20plan/apply=20equivalence?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Document the five fixes: ResolveForEnv name lift, configHash determinism, ResolveSizing invocation, Close() error logging, and the plan-vs-apply equivalence test harness. Co-Authored-By: Claude Sonnet 4.6 --- CHANGELOG.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e9fe7c146..7a861d088 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,24 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.18.7] - 2026-04-23 + +### Fixed + +- **`ResolveForEnv` lifts `Config["name"]` into `ResolvedModule.Name`** — when an environment override sets `config.name`, the value is promoted to `ResolvedModule.Name` and removed from `Config`. This closes the plan-vs-apply divergence where `wfctl infra plan --env staging` displayed `bmw-staging-vpc` but `wfctl infra apply --env staging` created a resource named `bmw-vpc` (the raw module name). Downstream `ResourceSpec.Name` now carries the env-resolved identity in both paths. (closes follow-up #32) +- **`platform.configHash` — deterministic key ordering** — `configHash` now sorts map keys explicitly before JSON-marshalling, matching the DO plugin's existing pattern. Previously, Go's randomised map-iteration order could produce different hashes for identical configs on successive runs, generating spurious "update" plan actions on second apply with no config change. +- **`applyWithProviderAndStore` calls `provider.ResolveSizing`** — for each spec with a non-empty `Size` field, `ResolveSizing(type, size, hints)` is now called before `platform.ComputePlan`. The returned `ProviderSizing.InstanceType` and extra `Specs` are merged into `spec.Config` so that plan and apply agree on the concrete instance type. +- **Provider `closer.Close()` errors logged as warnings** — `defer closer.Close() //nolint:errcheck` replaced with an explicit defer that writes `warning: provider %q shutdown: %v` to stderr in `infra_apply.go`, `infra_destroy.go`, `infra_status_drift.go`, and `infra_bootstrap.go`. Plugin subprocess leaks now surface instead of being silently discarded. +- **`configHashMap` in `infra.go` uses sorted kv encoding** — aligns with `platform.configHash` so `ResourceState.ConfigHash` values written during apply are comparable to hashes computed by `ComputePlan` on the next run. + +### Tests + +- `config/module_resolve_env_test.go` — `TestResolveForEnv_LiftsConfigNameIntoIdentity`, `TestResolveForEnv_PreservesNameWhenNoOverride`, `TestResolveForEnv_EmptyNameFieldIgnored` +- `platform/differ_hash_test.go` — `TestConfigHash_Stable_AcrossMapIterationOrder` (100 iterations), `TestConfigHash_EmptyMapReturnsEmpty`, `TestConfigHash_DifferentConfigsDifferentHashes` +- `cmd/wfctl/infra_apply_test.go` — `TestApplyInfraModules_CallsResolveSizing_ForEachSpec`, `TestApplyWithProvider_LogsCloseError` +- `cmd/wfctl/infra_plan_apply_equivalence_test.go` — `TestPlanApplyEquivalence_EnvOverrideNames` (end-to-end regression gate for plan-vs-apply name divergence) +- `cmd/wfctl/infra_env_wire_test.go` — `TestPlanResourcesForEnv_UsesEnvOverrideNames` + ## [0.18.6] - 2026-04-23 ### Fixed From d19f4ded920c6df7868d74cd691509bad9f9e5d3 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Thu, 23 Apr 2026 17:56:44 -0400 Subject: [PATCH 07/11] fix(test): update configHashIntegration to sorted kv encoding configHashIntegration in module/infra_module_integration_test.go was using the old json.Marshal(map) hash format, producing a different hash than platform.configHash (which uses sorted kv-pairs). This caused TestInfraModule_DriftDetectionFlow to emit a spurious "update" action and fail. Updated to match the platform canonical encoding exactly. Co-Authored-By: Claude Sonnet 4.6 --- module/infra_module_integration_test.go | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/module/infra_module_integration_test.go b/module/infra_module_integration_test.go index 7915574fe..4b8f1ac36 100644 --- a/module/infra_module_integration_test.go +++ b/module/infra_module_integration_test.go @@ -5,6 +5,7 @@ import ( "crypto/sha256" "encoding/json" "fmt" + "sort" "testing" "time" @@ -148,11 +149,26 @@ func (p *planningProvider) Plan(_ context.Context, desired []interfaces.Resource } // configHashIntegration replicates platform.configHash for test assertions. +// Keys are explicitly sorted before marshalling to match the sorted kv-pair +// encoding used by platform.ComputePlan — ensuring test hashes are stable. func configHashIntegration(config map[string]any) string { if len(config) == 0 { return "" } - data, _ := json.Marshal(config) + keys := make([]string, 0, len(config)) + for k := range config { + keys = append(keys, k) + } + sort.Strings(keys) + type kv struct { + K string + V any + } + ordered := make([]kv, len(keys)) + for i, k := range keys { + ordered[i] = kv{K: k, V: config[k]} + } + data, _ := json.Marshal(ordered) return fmt.Sprintf("%x", sha256.Sum256(data)) } From 10abef99449dc40d865e4e1c052cb60d5c3dcecc Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Thu, 23 Apr 2026 17:56:54 -0400 Subject: [PATCH 08/11] fix(lint): eliminate G602 and guard ResolveSizing on abstract sizes only Two related fixes to the ResolveSizing loop in applyWithProviderAndStore: 1. G602 (gosec slice index out-of-range false positive): replace indexing via specs[i] with a local pointer `spec := &specs[i]` so gosec can confirm the slice access is safe. 2. isAbstractSize guard (Copilot #2): add isAbstractSize helper that returns true only for xs/s/m/l/xl. ResolveSizing is now skipped for provider-specific slugs (e.g. "db-s-1vcpu-1gb") which are already concrete and must not be re-resolved. Co-Authored-By: Claude Sonnet 4.6 --- cmd/wfctl/infra_apply.go | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/cmd/wfctl/infra_apply.go b/cmd/wfctl/infra_apply.go index 6c7f0eb05..b18cf1b65 100644 --- a/cmd/wfctl/infra_apply.go +++ b/cmd/wfctl/infra_apply.go @@ -191,23 +191,26 @@ func applyWithProviderAndStore(ctx context.Context, provider interfaces.IaCProvi // Resolve abstract sizing tiers into concrete provider-specific values // (e.g. Size: "m" → instance_type: "s-1vcpu-2gb") for each spec that - // declares a Size. The resolved values are merged into spec.Config so that - // plan output and apply are always in sync. + // declares an abstract Size tier. Provider-specific slugs (e.g. + // "db-s-1vcpu-1gb") are passed through as-is to avoid double-resolution. + // The resolved values are merged into spec.Config so that plan output and + // apply are always in sync. for i := range specs { - if specs[i].Size == "" { + spec := &specs[i] + if spec.Size == "" || !isAbstractSize(spec.Size) { continue } - sizing, err := provider.ResolveSizing(specs[i].Type, specs[i].Size, specs[i].Hints) + sizing, err := provider.ResolveSizing(spec.Type, spec.Size, spec.Hints) if err != nil { - return fmt.Errorf("%s/%s: resolve sizing: %w", specs[i].Type, specs[i].Name, err) + return fmt.Errorf("%s/%s: resolve sizing: %w", spec.Type, spec.Name, err) } if sizing != nil { - if specs[i].Config == nil { - specs[i].Config = map[string]any{} + if spec.Config == nil { + spec.Config = map[string]any{} } - specs[i].Config["instance_type"] = sizing.InstanceType + spec.Config["instance_type"] = sizing.InstanceType for k, v := range sizing.Specs { - specs[i].Config[k] = v + spec.Config[k] = v } } } @@ -294,3 +297,15 @@ func applyWithProviderAndStore(ctx context.Context, provider interfaces.IaCProvi } return nil } + +// isAbstractSize reports whether s is one of the canonical abstract size tiers +// (xs/s/m/l/xl). Provider-specific slugs such as "db-s-1vcpu-1gb" return false +// so that ResolveSizing is not called for already-concrete values. +func isAbstractSize(s interfaces.Size) bool { + switch s { + case interfaces.SizeXS, interfaces.SizeS, interfaces.SizeM, interfaces.SizeL, interfaces.SizeXL: + return true + default: + return false + } +} From e8ec2b2686d53bc491b1480ad2128a802d8047db Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Thu, 23 Apr 2026 17:57:00 -0400 Subject: [PATCH 09/11] fix(config): scope ResolveForEnv name-lift to infra.* modules only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The env-override name lift in ResolveForEnv previously applied to all module types. This was too broad — non-infra modules can legitimately carry a 'name' key in their Config for display purposes. Added strings.HasPrefix(resolved.Type, "infra.") guard so only infra.* modules have their Config["name"] lifted into ResolvedModule.Name. All other module types are unaffected. Co-Authored-By: Claude Sonnet 4.6 --- config/module_resolve_env.go | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/config/module_resolve_env.go b/config/module_resolve_env.go index 154bdefda..dc0b4e74c 100644 --- a/config/module_resolve_env.go +++ b/config/module_resolve_env.go @@ -1,5 +1,7 @@ package config +import "strings" + // ResolvedModule is the effective module config for a specific environment. type ResolvedModule struct { Name string @@ -53,12 +55,17 @@ func (m *ModuleConfig) ResolveForEnv(envName string) (*ResolvedModule, bool) { resolved.Config = deepMergeMap(resolved.Config, envRes.Config) } - // If an env override sets "name", lift it into ResolvedModule.Name and - // delete it from Config so downstream ResourceSpec construction uses the - // correct identity. Empty string is ignored to prevent accidental erasure. - if n, ok := resolved.Config["name"].(string); ok && n != "" { - resolved.Name = n - delete(resolved.Config, "name") + // If an env override sets "name" on an infra.* module, lift it into + // ResolvedModule.Name and delete it from Config so downstream ResourceSpec + // construction uses the correct identity. The guard limits this behaviour to + // infra.* types to avoid accidentally renaming non-infra modules that happen + // to have a "name" key in their Config (e.g. display-name fields). + // Empty string is ignored to prevent accidental erasure. + if strings.HasPrefix(resolved.Type, "infra.") { + if n, ok := resolved.Config["name"].(string); ok && n != "" { + resolved.Name = n + delete(resolved.Config, "name") + } } setRegionFromConfig(resolved) // re-apply after env overrides From 51892a67edebefabe596d0456e8323fa97c5dad2 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Thu, 23 Apr 2026 17:57:05 -0400 Subject: [PATCH 10/11] fix(test): add t.Cleanup for stderr restore in LogsCloseError test TestApplyWithProvider_LogsCloseError redirects os.Stderr but did not register a cleanup handler. If the test failed early (e.g. at os.Pipe), stderr would remain redirected for subsequent tests. Added t.Cleanup that restores oldStderr and closes both pipe ends, guaranteeing the redirect is always undone regardless of test outcome. Co-Authored-By: Claude Sonnet 4.6 --- cmd/wfctl/infra_apply_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cmd/wfctl/infra_apply_test.go b/cmd/wfctl/infra_apply_test.go index 91d483e4d..55f8c1cce 100644 --- a/cmd/wfctl/infra_apply_test.go +++ b/cmd/wfctl/infra_apply_test.go @@ -626,6 +626,11 @@ modules: t.Fatalf("os.Pipe: %v", pipeErr) } os.Stderr = w + t.Cleanup(func() { + os.Stderr = oldStderr + _ = w.Close() + _ = r.Close() + }) err := applyInfraModules(context.Background(), cfgPath, "") From ee00f2dd3ef7c9c9f5dc9cd9d695000cc999dd56 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Thu, 23 Apr 2026 17:57:11 -0400 Subject: [PATCH 11/11] refactor: export platform.ConfigHash and delegate from configHashMap configHashMap in cmd/wfctl/infra.go duplicated the sorted kv-pair encoding logic from platform.configHash. Any future change to the hashing algorithm would require updating two places. Added exported platform.ConfigHash wrapper that delegates to the package-internal configHash function. configHashMap now delegates to platform.ConfigHash, eliminating the duplication and ensuring the two are always byte-for-byte identical. Removed now-unused crypto/sha256 and sort imports from infra.go. Co-Authored-By: Claude Sonnet 4.6 --- cmd/wfctl/infra.go | 27 ++++----------------------- platform/differ.go | 8 ++++++++ 2 files changed, 12 insertions(+), 23 deletions(-) diff --git a/cmd/wfctl/infra.go b/cmd/wfctl/infra.go index 7ef7d4c8e..82a59f1ab 100644 --- a/cmd/wfctl/infra.go +++ b/cmd/wfctl/infra.go @@ -2,12 +2,10 @@ package main import ( "context" - "crypto/sha256" "encoding/json" "flag" "fmt" "os" - "sort" "strings" "github.com/GoCodeAlone/workflow/config" @@ -354,28 +352,11 @@ func loadCurrentState(cfgFile string) []interfaces.ResourceState { return states } -// configHashMap computes a deterministic SHA-256 hex hash of a config map. -// Keys are explicitly sorted before marshalling to match platform.configHash -// so that saved ConfigHash values are comparable across calls and round-trips. +// configHashMap delegates to platform.ConfigHash so that the CLI always +// produces hashes byte-for-byte identical to those stored by ComputePlan. +// The local duplication that previously existed here has been removed. func configHashMap(config map[string]any) string { - if len(config) == 0 { - return "" - } - keys := make([]string, 0, len(config)) - for k := range config { - keys = append(keys, k) - } - sort.Strings(keys) - type kv struct { - K string - V any - } - ordered := make([]kv, len(keys)) - for i, k := range keys { - ordered[i] = kv{K: k, V: config[k]} - } - data, _ := json.Marshal(ordered) - return fmt.Sprintf("%x", sha256.Sum256(data)) + return platform.ConfigHash(config) } // formatPlanTable renders an interfaces.IaCPlan as a human-readable table diff --git a/platform/differ.go b/platform/differ.go index 51ebcd84d..dfc1d81e3 100644 --- a/platform/differ.go +++ b/platform/differ.go @@ -89,6 +89,14 @@ func ComputePlan(desired []interfaces.ResourceSpec, current []interfaces.Resourc }, nil } +// ConfigHash is the exported counterpart of configHash. It allows callers +// outside the platform package (e.g. cmd/wfctl) to compute hashes that are +// byte-for-byte identical to those stored by ComputePlan, eliminating the +// risk of independent re-implementations diverging. +func ConfigHash(config map[string]any) string { + return configHash(config) +} + // configHash returns a deterministic SHA-256 hex hash of a config map. // Keys are explicitly sorted before marshalling so the hash is stable across // Go's randomised map-iteration order — matching the DO plugin's pattern.