From 87ebd7b5c012daa4a597a67703846d6a03766469 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 4 May 2026 03:09:07 -0400 Subject: [PATCH 1/2] =?UTF-8?q?fix(iac):=20T3.6/T3.5=20=E2=80=94=20configH?= =?UTF-8?q?ashErr=20surfaces=20marshal=20failure;=20cache=20bypassed=20for?= =?UTF-8?q?=20unhashable=20inputs=20(Copilot=20review=20round=209)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit configHash silently swallowed json.Marshal errors via `data, _ := json.Marshal(ordered)`, so any input containing a non-marshalable value (channels, functions, cycles, types with broken MarshalJSON) collapsed to the constant sha256("") hash. For the diff-cache key path that meant two genuinely-different resources with such inputs would share the same SHAOutputs (or SHAConfig) and serve each other's cached DiffResult — silently misclassifying actions. In practice IaC configs come from YAML and cannot contain those types, so the bug is unreachable for the common case. Defensive coverage matters for custom-provider Outputs that could conceivably have types with broken MarshalJSON, and for any future code path that passes non-YAML-derived configs into ComputePlan. Fix: 1. **Add configHashErr(map) (string, error)** — error-aware variant that returns ("", err) on marshal failure. Backward compat: keep configHash(map) string as a wrapper that ignores the error (legacy callers operating on YAML-derived configs don't reach this path; documented in updated docstring). 2. **ComputePlan + classifyModification thread a `hashable` flag** — The candidate-bucketing loop calls configHashErr on spec.Config; on failure it sets `hashable=false` on the modCandidate. The cache- keying call site at differ.go:235 ANDs `hashable` into the `cacheable` predicate AND calls configHashErr on rs.Outputs (also bypassing on failure there). Both bypass paths fall through to unconditional driver.Diff dispatch — same defensive treatment as the empty-ProviderID path. Cost is one extra Diff call per resource with unhashable inputs, never correctness. 3. **New pins**: - TestComputePlan_UnhashableInputs_BypassCache — two resources with channel-poisoned Outputs both re-Diff on the second ComputePlan (4 driver calls across 2 invocations); without the fix the two would collide on SHAOutputs="" and one would bogus-hit the other's cache (3 driver calls). - TestConfigHashErr_PropagatesMarshalFailure — unit-level assertion that configHashErr returns ("", err) on chan-input and that the un-suffixed configHash returns "" silently for the same input (backward-compat). Addresses Copilot inline comment on PR #528 (round 9): - platform/differ.go:253 (configHash silently collapses unmarshalable inputs) Tests: GOWORK=off go test -race -count=1 ./platform/... ./cmd/wfctl/... ./iac/... ./interfaces/... ./plugin/sdk/... — all green. Co-Authored-By: Claude Opus 4.7 (1M context) --- platform/differ.go | 112 ++++++++++++++++++++++++++++------ platform/differ_cache_test.go | 84 +++++++++++++++++++++++++ 2 files changed, 176 insertions(+), 20 deletions(-) diff --git a/platform/differ.go b/platform/differ.go index a925129f..d7e9a58b 100644 --- a/platform/differ.go +++ b/platform/differ.go @@ -88,13 +88,20 @@ func ComputePlan(ctx context.Context, p interfaces.IaCProvider, desired []interf // emitted synchronously since they don't need the provider. var creates []interfaces.PlanAction type modCandidate struct { - spec interfaces.ResourceSpec - rs interfaces.ResourceState - hash string // precomputed configHash(spec.Config); reused by classifyModification + spec interfaces.ResourceSpec + rs interfaces.ResourceState + hash string // precomputed configHash(spec.Config); reused by classifyModification + hashable bool // true when configHashErr succeeded; false → cache-bypass for this candidate } var candidates []modCandidate for _, spec := range desired { - hash := configHash(spec.Config) + // Use configHashErr so a non-marshalable spec.Config doesn't + // collapse to the constant sha256("") hash silently. The + // returned hash is stored on the action either way (legacy + // callers compare ResolvedConfigHash on YAML-derived configs + // where marshal can't fail), but the hashable flag gates + // cache participation for the modification path. + hash, hashErr := configHashErr(spec.Config) if rs, exists := currentMap[spec.Name]; !exists { creates = append(creates, interfaces.PlanAction{ Action: "create", @@ -102,7 +109,12 @@ func ComputePlan(ctx context.Context, p interfaces.IaCProvider, desired []interf ResolvedConfigHash: hash, }) } else { - candidates = append(candidates, modCandidate{spec: spec, rs: rs, hash: hash}) + candidates = append(candidates, modCandidate{ + spec: spec, + rs: rs, + hash: hash, + hashable: hashErr == nil, + }) } } @@ -116,7 +128,7 @@ func ComputePlan(ctx context.Context, p interfaces.IaCProvider, desired []interf g.SetLimit(planDiffConcurrency()) for i := range candidates { g.Go(func() error { - return classifyModification(gctx, p, candidates[i].spec, candidates[i].rs, candidates[i].hash, &mods[i]) + return classifyModification(gctx, p, candidates[i].spec, candidates[i].rs, candidates[i].hash, candidates[i].hashable, &mods[i]) }) } if err := g.Wait(); err != nil { @@ -180,8 +192,13 @@ func ComputePlan(ctx context.Context, p interfaces.IaCProvider, desired []interf // via the legacy ConfigHash compare. The hash argument is the // precomputed configHash(spec.Config), threaded in by the caller so // the per-candidate hashing happens once during candidate-bucketing -// rather than redundantly here on every Diff dispatch. -func classifyModification(ctx context.Context, p interfaces.IaCProvider, spec interfaces.ResourceSpec, rs interfaces.ResourceState, hash string, out **interfaces.PlanAction) error { +// rather than redundantly here on every Diff dispatch. The hashable +// argument is true when configHashErr succeeded for spec.Config; when +// false, the cache is bypassed for this resource (same defensive +// treatment as the empty-ProviderID path) — collapsing all +// non-marshalable inputs to the constant sha256("") hash would risk +// cache-key collisions across distinct resources. +func classifyModification(ctx context.Context, p interfaces.IaCProvider, spec interfaces.ResourceSpec, rs interfaces.ResourceState, hash string, hashable bool, out **interfaces.PlanAction) error { rsCopy := rs // Nil-provider fallback: legacy ConfigHash compare. @@ -231,25 +248,45 @@ func classifyModification(ctx context.Context, p interfaces.IaCProvider, spec in // Always re-dispatch in that case; the cost is one extra Diff call, // not correctness. var diff *interfaces.DiffResult - cacheable := rs.ProviderID != "" + // cacheable = (1) ProviderID is the disambiguator across same-Type + // resources (see the empty-ProviderID bypass docstring just above + // the if-block) AND (2) we can deterministically hash both + // spec.Config and rs.Outputs (configHashErr surfaces marshal + // failures on non-YAML-derivable values so we don't collapse all + // unmarshalable inputs to the constant sha256("") hash and serve + // each other's cached DiffResult). Bypass cases all fall through + // to the unconditional driver.Diff dispatch below. + cacheable := rs.ProviderID != "" && hashable var ( - cache diffcache.Cache - key diffcache.Key + cache diffcache.Cache + key diffcache.Key + shaOut string ) + if cacheable { + var outErr error + shaOut, outErr = configHashErr(rs.Outputs) + if outErr != nil { + // Outputs contained a non-marshalable value; bypass cache + // for this resource (same defensive treatment as the + // empty-ProviderID and unhashable-spec.Config paths) and + // re-Diff every time. + cacheable = false + } + } if cacheable { // getDiffCache lives inside this branch so the empty-ProviderID - // bypass path is fully side-effect free — no sync.Once init - // firing, no atomic load, and (most importantly) no eager - // lazy-construction of the filesystem cache backend creating - // ~/.cache/wfctl/diff/ on the operator's machine for resources - // that won't use the cache anyway. + // AND unhashable-inputs bypass paths are fully side-effect free + // — no sync.Once init firing, no atomic load, and (most + // importantly) no eager lazy-construction of the filesystem + // cache backend creating ~/.cache/wfctl/diff/ on the operator's + // machine for resources that won't use the cache anyway. cache = getDiffCache() key = diffcache.Key{ PluginVersion: pluginVersionKey(p), Type: spec.Type, ProviderID: rs.ProviderID, SHAConfig: hash, - SHAOutputs: configHash(rs.Outputs), + SHAOutputs: shaOut, } if cached, hit := cache.Get(key); hit { c := cached @@ -473,9 +510,41 @@ func ConfigHash(config map[string]any) string { // 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. +// +// configHash IGNORES json.Marshal errors. If the config contains a value +// json.Marshal cannot encode (channels, functions, cycles, custom types +// with broken MarshalJSON), this collapses to the constant +// sha256("") hash. That is fine for the legacy callers that compare +// hashes for "did anything change" semantics on YAML-derived configs +// (YAML can't express those types, so the failure mode is unreachable +// in practice for those callers). Cache-key callers MUST use +// configHashErr instead — collapsing all unmarshalable inputs to the +// same constant hash would cause cache-key collisions and let two +// genuinely-different resources serve each other's cached DiffResult. +// See the differ.go:235 comment block for the cache-bypass plumbing. func configHash(config map[string]any) string { + h, _ := configHashErr(config) + return h +} + +// configHashErr is the error-aware variant of configHash, used by the +// diff-cache keying path so non-marshalable inputs bypass caching for +// that resource (rather than silently collapsing to the constant +// sha256("") hash and risking cache-key collisions across distinct +// resources). When err != nil, the returned string is the empty +// hash and callers MUST NOT use it for cache keys; they should +// re-Diff every time for that resource (same defensive bypass as +// the empty-ProviderID path documented at differ.go:235). +// +// In practice IaC configs come from YAML and cannot contain +// non-marshalable values (channels, functions, cycles), so this +// path is unreachable for the common case. Defensive coverage is +// for custom-provider Outputs that could conceivably contain +// types with broken MarshalJSON, or for future code paths that +// pass non-YAML-derived configs into ComputePlan. +func configHashErr(config map[string]any) (string, error) { if len(config) == 0 { - return "" + return "", nil } keys := make([]string, 0, len(config)) for k := range config { @@ -490,8 +559,11 @@ func configHash(config map[string]any) string { for i, k := range keys { ordered[i] = kv{K: k, V: config[k]} } - data, _ := json.Marshal(ordered) - return fmt.Sprintf("%x", sha256.Sum256(data)) + data, err := json.Marshal(ordered) + if err != nil { + return "", fmt.Errorf("configHash: marshal: %w", err) + } + return fmt.Sprintf("%x", sha256.Sum256(data)), nil } // planID generates a simple unique plan ID based on current time. diff --git a/platform/differ_cache_test.go b/platform/differ_cache_test.go index 2d23d5d3..349a6fd1 100644 --- a/platform/differ_cache_test.go +++ b/platform/differ_cache_test.go @@ -575,3 +575,87 @@ func TestComputePlan_NilDiffResult_CachesAsZeroValue(t *testing.T) { t.Errorf("after second ComputePlan: Diff calls = %d, want 1 (cache hit on zero-value DiffResult; round-5 fix)", got) } } + +// TestComputePlan_UnhashableInputs_BypassCache pins the round-9 fix: +// when spec.Config or rs.Outputs contains a non-marshalable value +// (channel, function, cycle), configHashErr surfaces the marshal +// failure and ComputePlan bypasses the diff cache for that resource — +// rather than collapsing all unmarshalable inputs to the constant +// sha256("") hash and risking cache-key collisions across distinct +// resources serving each other's cached DiffResult. +// +// In practice IaC configs come from YAML and cannot contain these +// types, so this is defensive coverage for custom-provider Outputs +// or future code paths. +func TestComputePlan_UnhashableInputs_BypassCache(t *testing.T) { + setDiffCacheForTest(t, diffcache.NewMemory()) + + driver := &cacheTestDriver{diff: &interfaces.DiffResult{NeedsUpdate: true}} + provider := &cacheTestProvider{name: "fake", version: "0.0.0-test", driver: driver} + + // Two resources, both with non-marshalable values in Outputs (a + // channel, which json.Marshal rejects). Without the bypass, both + // would hash to the constant sha256("") and the second resource + // would be served the first's cached DiffResult. + unmarshalable := make(chan int) + desired := []interfaces.ResourceSpec{ + {Name: "vpc-a", Type: "infra.vpc", Config: map[string]any{"region": "nyc3"}}, + {Name: "vpc-b", Type: "infra.vpc", Config: map[string]any{"region": "sfo3"}}, + } + current := []interfaces.ResourceState{ + {Name: "vpc-a", Type: "infra.vpc", ProviderID: "pid-a", Outputs: map[string]any{"poison": unmarshalable}}, + {Name: "vpc-b", Type: "infra.vpc", ProviderID: "pid-b", Outputs: map[string]any{"poison": unmarshalable}}, + } + + if _, err := ComputePlan(context.Background(), provider, desired, current); err != nil { + t.Fatalf("ComputePlan: %v", err) + } + if got := driver.diffCount.Load(); got != 2 { + t.Errorf("Diff calls = %d, want 2 (one per resource on first pass)", got) + } + // Second pass: both resources should re-Diff (cache bypassed), + // not share a cached entry. Without the round-9 fix, both + // resources collapse to the same SHAOutputs="" key on the first + // pass, only one cache.Put happens (overwriting), and the + // second pass hits cache for both — diffCount would stop at 3 + // (one re-dispatch + one bogus hit). + if _, err := ComputePlan(context.Background(), provider, desired, current); err != nil { + t.Fatalf("second ComputePlan: %v", err) + } + if got := driver.diffCount.Load(); got != 4 { + t.Errorf("Diff calls after 2nd ComputePlan = %d, want 4 (no cache hits when Outputs are unhashable; round-9 fix)", got) + } +} + +// TestConfigHashErr_PropagatesMarshalFailure is a unit-level pin for +// configHashErr. Marshalable inputs return (hash, nil); a non- +// marshalable value (channel) returns ("", non-nil-err). Backward +// compatibility: the un-suffixed configHash returns the empty hash +// for the same input (silently swallowing the error, as documented +// in its docstring; legacy callers operating on YAML-derived configs +// don't reach this path). +func TestConfigHashErr_PropagatesMarshalFailure(t *testing.T) { + good := map[string]any{"region": "nyc3", "size": 4} + gotHash, gotErr := configHashErr(good) + if gotErr != nil { + t.Fatalf("configHashErr(good): unexpected err: %v", gotErr) + } + if len(gotHash) != 64 { + t.Errorf("configHashErr(good): hash len = %d, want 64 (sha256 hex)", len(gotHash)) + } + + bad := map[string]any{"poison": make(chan int)} + gotHash, gotErr = configHashErr(bad) + if gotErr == nil { + t.Errorf("configHashErr(bad): expected non-nil error for unmarshalable input; got hash=%q", gotHash) + } + if gotHash != "" { + t.Errorf("configHashErr(bad): hash = %q, want empty string when err != nil", gotHash) + } + + // Backward-compatible wrapper: configHash silently returns "" for + // the same bad input (legacy contract; documented in its docstring). + if got := configHash(bad); got != "" { + t.Errorf("configHash(bad) = %q, want empty string (backward-compat: errors swallowed)", got) + } +} From 34ca7c1705a26b36152b5986654db0321affec02 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 4 May 2026 03:45:25 -0400 Subject: [PATCH 2/2] =?UTF-8?q?fix(iac):=20R10=20review=20=E2=80=94=20rest?= =?UTF-8?q?ore=20configHash=20legacy=20sha256(nil)=20on=20marshal=20failur?= =?UTF-8?q?e=20for=20backward-compat=20(Copilot=20review)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot R9 caught a silent ABI break in T3.6: the new configHash wrapper returned "" for unmarshalable inputs, whereas the pre-T3.6 implementation returned sha256(nil) = e3b0c44... (the well-known sha256 of empty bytes; data is nil after json.Marshal failure). That changes any persisted ResolvedConfigHash / ConfigHash values computed for an unhashable input — they would no longer compare equal across the upgrade boundary. While unmarshalable values are unreachable for YAML-derived configs in practice, the public platform.ConfigHash function is part of the API surface, so the defensive choice is byte-for-byte stability. Restored configHash to the pre-T3.6 sha256-of-best-effort body. Cache-key callers continue to use configHashErr (the strict variant that surfaces the marshal error so the cache is bypassed for that resource — collapsing all unmarshalable inputs to a constant hash would risk cache-key collisions across distinct resources). ComputePlan now falls back to configHash for the stored ResolvedConfigHash when configHashErr fails, while still using the hashable flag to gate cache participation in classifyModification. Test TestConfigHashErr_PropagatesMarshalFailure pinned to the restored legacy contract. Co-Authored-By: Claude Opus 4.7 (1M context) --- platform/differ.go | 49 +++++++++++++++++++++++++---------- platform/differ_cache_test.go | 28 +++++++++++++------- 2 files changed, 55 insertions(+), 22 deletions(-) diff --git a/platform/differ.go b/platform/differ.go index d7e9a58b..74d98f50 100644 --- a/platform/differ.go +++ b/platform/differ.go @@ -95,13 +95,18 @@ func ComputePlan(ctx context.Context, p interfaces.IaCProvider, desired []interf } var candidates []modCandidate for _, spec := range desired { - // Use configHashErr so a non-marshalable spec.Config doesn't - // collapse to the constant sha256("") hash silently. The - // returned hash is stored on the action either way (legacy - // callers compare ResolvedConfigHash on YAML-derived configs - // where marshal can't fail), but the hashable flag gates - // cache participation for the modification path. + // Use configHashErr to detect non-marshalable spec.Config values + // so we can bypass the diff-cache for those resources. For the + // stored ResolvedConfigHash we fall back to the legacy + // configHash (sha256-of-best-effort) when configHashErr fails; + // that preserves byte-for-byte stability with any persisted + // plans + ConfigHash values produced under the pre-T3.6 + // implementation, while the hashable flag gates cache + // participation for the modification path. hash, hashErr := configHashErr(spec.Config) + if hashErr != nil { + hash = configHash(spec.Config) + } if rs, exists := currentMap[spec.Name]; !exists { creates = append(creates, interfaces.PlanAction{ Action: "create", @@ -514,17 +519,35 @@ func ConfigHash(config map[string]any) string { // configHash IGNORES json.Marshal errors. If the config contains a value // json.Marshal cannot encode (channels, functions, cycles, custom types // with broken MarshalJSON), this collapses to the constant -// sha256("") hash. That is fine for the legacy callers that compare -// hashes for "did anything change" semantics on YAML-derived configs +// sha256() = e3b0c4429... hash. That preserves the legacy "did +// anything change" comparison semantics for YAML-derived configs // (YAML can't express those types, so the failure mode is unreachable -// in practice for those callers). Cache-key callers MUST use -// configHashErr instead — collapsing all unmarshalable inputs to the -// same constant hash would cause cache-key collisions and let two +// in practice for those callers) and keeps the exported ConfigHash +// byte-for-byte stable against any persisted ResolvedConfigHash values +// computed under the pre-T3.6 implementation. Cache-key callers MUST +// use configHashErr instead — collapsing all unmarshalable inputs to +// the same constant hash would cause cache-key collisions and let two // genuinely-different resources serve each other's cached DiffResult. // See the differ.go:235 comment block for the cache-bypass plumbing. func configHash(config map[string]any) string { - h, _ := configHashErr(config) - return h + 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)) } // configHashErr is the error-aware variant of configHash, used by the diff --git a/platform/differ_cache_test.go b/platform/differ_cache_test.go index 349a6fd1..ca5ba427 100644 --- a/platform/differ_cache_test.go +++ b/platform/differ_cache_test.go @@ -629,11 +629,15 @@ func TestComputePlan_UnhashableInputs_BypassCache(t *testing.T) { // TestConfigHashErr_PropagatesMarshalFailure is a unit-level pin for // configHashErr. Marshalable inputs return (hash, nil); a non- -// marshalable value (channel) returns ("", non-nil-err). Backward -// compatibility: the un-suffixed configHash returns the empty hash -// for the same input (silently swallowing the error, as documented -// in its docstring; legacy callers operating on YAML-derived configs -// don't reach this path). +// marshalable value (channel) returns ("", non-nil-err) so cache-key +// callers can deterministically bypass the cache. +// +// Backward compatibility: the un-suffixed configHash silently swallows +// the marshal error and returns the sha256-of-best-effort-bytes hash +// (matches the pre-T3.6 implementation byte-for-byte) so any persisted +// ResolvedConfigHash / ConfigHash values computed under the old +// behavior stay stable. configHashErr is the strict variant that must +// be used for cache keys; configHash is the legacy ABI. func TestConfigHashErr_PropagatesMarshalFailure(t *testing.T) { good := map[string]any{"region": "nyc3", "size": 4} gotHash, gotErr := configHashErr(good) @@ -653,9 +657,15 @@ func TestConfigHashErr_PropagatesMarshalFailure(t *testing.T) { t.Errorf("configHashErr(bad): hash = %q, want empty string when err != nil", gotHash) } - // Backward-compatible wrapper: configHash silently returns "" for - // the same bad input (legacy contract; documented in its docstring). - if got := configHash(bad); got != "" { - t.Errorf("configHash(bad) = %q, want empty string (backward-compat: errors swallowed)", got) + // Backward-compatible wrapper: configHash returns the legacy + // sha256-of-best-effort hash for unmarshalable inputs (matches the + // pre-T3.6 implementation byte-for-byte; cache callers MUST use + // configHashErr instead). The exact value here is sha256(nil), + // which is what json.Marshal followed by sha256.Sum256 produces + // when Marshal returns nil + err. + got := configHash(bad) + const sha256OfNil = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" + if got != sha256OfNil { + t.Errorf("configHash(bad) = %q, want %q (legacy sha256(nil) — pre-T3.6 byte-for-byte stability)", got, sha256OfNil) } }