fix(iac): configHashErr surfaces marshal failure; cache bypassed for unhashable inputs#533
Merged
Merged
Conversation
…ypassed for unhashable inputs (Copilot review round 9)
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) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens IaC diff-cache keying in platform.ComputePlan by surfacing config/output hash failures and bypassing cache use for unhashable inputs, which avoids incorrect cache hits during provider Diff dispatch.
Changes:
- Add an error-aware
configHashErrhelper and thread ahashableflag through modification classification. - Bypass diff-cache reads/writes when
spec.Configorrs.Outputscannot be marshaled deterministically. - Add regression tests covering unhashable cache inputs and the new hash helper behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
platform/differ.go |
Threads hashability through ComputePlan/classifyModification and adds configHashErr for cache-safe hashing. |
platform/differ_cache_test.go |
Adds regression coverage for cache bypass on unhashable inputs and for configHashErr error propagation. |
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
…al failure for backward-compat (Copilot review) 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) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to W-3b (PR #528) addressing Copilot's round-9 review finding, which arrived after #528 was admin-merged.
platform.configHashsilently swallowedjson.Marshalerrors viadata, _ := json.Marshal(ordered). Any input containing a non-marshalable value (channels, functions, cycles, types with brokenMarshalJSON) collapsed to the constantsha256("")hash. For the diff-cache key path that meant two genuinely-different resources with such inputs would share the sameSHAOutputs(orSHAConfig) and serve each other's cachedDiffResult— silently misclassifying actions.In practice IaC configs come from YAML and cannot contain those types, so this 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 intoComputePlan.Fix
Add
configHashErr(map) (string, error)— error-aware variant that returns("", err)on marshal failure. Backward compat: keepconfigHash(map) stringas a wrapper that ignores the error (legacy callers operating on YAML-derived configs don't reach this path).ComputePlan+classifyModificationthread ahashableflag — The candidate-bucketing loop callsconfigHashErronspec.Config; on failure it setshashable=falseon the candidate. The cache-keying call site atdiffer.go:235ANDshashableinto thecacheablepredicate AND callsconfigHashErronrs.Outputs(also bypassing on failure there). Both bypass paths fall through to unconditionaldriver.Diffdispatch — same defensive treatment as the empty-ProviderIDpath. Cost is one extra Diff call per resource with unhashable inputs, never correctness.New pins:
TestComputePlan_UnhashableInputs_BypassCache— two resources with channel-poisoned Outputs both re-Diff on the secondComputePlan(4 driver calls across 2 invocations); without the fix the two would collide onSHAOutputs=""and one would bogus-hit the other's cache (3 driver calls).TestConfigHashErr_PropagatesMarshalFailure— unit-level assertion thatconfigHashErrreturns("", err)on chan-input and that the un-suffixedconfigHashreturns""silently for the same input (backward-compat).Source
Copilot inline comment on PR #528 round 9: #528 (comment)
Test plan
GOWORK=off go test -race -count=1 ./platform/... ./cmd/wfctl/... ./iac/... ./interfaces/... ./plugin/sdk/...— all greenTestComputePlan_UnhashableInputs_BypassCacheasserts 4 driver calls (vs. bogus 3 without the fix)TestConfigHashErr_PropagatesMarshalFailurepins the err-aware contract + backward-compat wrapperTestComputePlan_EmptyProviderID_BypassesCache,TestComputePlan_NilDiffResult_CachesAsZeroValue,TestComputePlan_CacheHitSkipsDiff, etc.) still pass🤖 Generated with Claude Code