The table-driven tests cover several useful combinations of original/current/target, but they do not include the core scenario from issue #917 where a field that is explicitly defined in the chart remains unchanged between original and target while being modified only in the live object (e.g., spec.maxReplicaCount is 10 in both original and target, but 30 in current). Without a test for this pattern, it is easy for the unstructured/CRD three-way-merge logic to regress (or remain incorrect) while all tests here still pass. Please add a case that matches the reported bug scenario to assert that a non-empty patch is produced when live state diverges from the chart for a chart-owned field, even if the chart did not change that field between releases.

In the expectedChange == false branch, the test only logs an unexpected non-empty patch instead of failing the test when a non-trivial patch is returned. For cases like "CR with no manual changes", an empty (or effectively empty) patch is the expected behavior, so silently accepting a non-empty patch here would hide regressions in the three-way-merge logic. Consider making this branch assert that the patch is empty (allowing only {} or "null" as appropriate) so that any future behavior change that introduces unintended diffs will cause the test to fail.

The three-way merge implementation for unstructured/CRD objects here will still fail to detect drift when a field is unchanged between oldData and newData but modified only in the live object (the scenario described in #917 for values explicitly set in the chart). In that case jsonpatch.CreateMergePatch(oldData, newData) returns an empty patch, mergedData ends up identical to currentData, and CreateMergePatch(currentData, mergedData) also returns an empty patch, so no diff is produced even though the live state diverges from the chart. This contradicts the PR description that manual changes to CRDs/CRs are now detected "just like" built-in resources, and it means the fix does not yet cover the core bug scenario. To align behavior with the strategic three-way merge path used for built-in types, the unstructured/CRD branch should compute a patch that also captures differences between currentData and newData for chart-owned fields, even when newData equals oldData, rather than only replaying old -> new changes onto currentData.

In the step-by-step comment for the unstructured/CRD three-way merge, step 5 says "return new->current patch to restore chart state", but the implementation below calls jsonpatch.CreateMergePatch(currentData, newData), i.e. it creates a patch from current -> new. To avoid confusion for future readers, the comment should be updated to describe the same direction as the code (patch from current to new, applied to the current/live object).

In the chartChanged == false branch, CreateMergePatch(cleanedCurrentData, cleanedNewData) will generate deletions for any fields that exist only in the live object (e.g., manual annotations/labels, controllers’ injected fields, and potentially Helm’s own meta.helm.sh/* annotations if they’re not present in the rendered manifest). That makes the server dry-run "target" diverge from what a three-way merge should do and can produce noisy/incorrect diffs by removing user-owned fields when the chart didn’t change. Consider instead building a desired object by overlaying the chart state onto the live state (preserving unknown additions), then diffing current -> desired (e.g., apply a merge-patch of cleanedNewData onto cleanedCurrentData and compute a patch from current to that result).

cleanMetadataForPatch largely overlaps with deleteStatusAndTidyMetadata (manifest/util.go) but removes a different set of metadata fields/annotations (e.g., meta.helm.sh/* and deployment.kubernetes.io/revision are handled in the other helper but not here). Having two slightly different "tidy metadata" implementations risks inconsistent behavior between patch generation and manifest rendering. Suggest reusing/extracting the existing helper (or at least aligning the exact fields removed) so the same metadata is ignored everywhere.

This case asserts a non-empty patch when the only live change is an extra annotation not present in the chart. That implies the diff will attempt to remove live-only fields, which is typically not what three-way merge behavior should do (it should preserve unknown additions and only reconcile chart-owned fields). If the implementation is changed to preserve live-only fields (recommended), update this expectation to assert no effective change / that the extra annotation is preserved, and focus drift detection on chart fields (e.g. the issue #917 spec field).

cleanMetadataForPatch largely duplicates deleteStatusAndTidyMetadata (same metadata/status stripping + annotation cleanup) but now exists as a separate implementation with different JSON codec and no direct unit tests. To avoid the two clean-up paths drifting over time, consider extracting a shared helper (or reusing deleteStatusAndTidyMetadata and re-marshalling) so metadata stripping behavior is defined in one place and covered by the existing util tests.

The new unstructured three-way-merge tests exercise the runtime.Unstructured branch, but the dedicated CRD type branch (isCRD == true) isn’t covered. Since the PR claims to fix CRDs and CRs, add at least one test case that uses an apiextensions/v1 CustomResourceDefinition object to ensure createPatch takes the CRD path and produces the expected patch behavior.

cleanMetadataForPatch calls deleteStatusAndTidyMetadata, which assumes the JSON decodes to a map containing a metadata object. If currentObj is ever nil (the function comment above still claims this is possible), json.Marshal(currentObj) becomes null, which will cause a panic when deleteStatusAndTidyMetadata does objectMap["metadata"].(map[string]interface{}). Consider explicitly handling null/empty input in cleanMetadataForPatch (e.g., treat it as {} or skip cleaning) and/or update the comment about currentObj potentially being nil so behavior stays consistent and panic-free.

The error returned here should be wrapped with context for better debugging. Consider wrapping it like the other error returns in this block:

patch, err := jsonpatch.CreateMergePatch(cleanedCurrentData, desiredData)
if err != nil {
 return nil, types.MergePatchType, fmt.Errorf("creating patch from current to desired: %w", err)
}
return patch, types.MergePatchType, nil

This ensures consistent error handling within this function and provides clearer error messages when issues occur.

This type assertion can panic if annotations is not a map[string]any. Consider using a safe type assertion with the comma-ok idiom to prevent runtime panics. For example:

if annotations, ok := a.(map[string]any); ok {
 // delete operations
}

This would make the code more robust when handling unexpected annotation structures.

The error returned here should be wrapped with context for better debugging. Consider wrapping it like the other error returns in this block:

patch, err := jsonpatch.CreateMergePatch(cleanedCurrentData, mergedData)
if err != nil {
 return nil, types.MergePatchType, fmt.Errorf("creating patch from current to merged: %w", err)
}
return patch, types.MergePatchType, nil

This ensures consistent error handling and provides clearer error messages when issues occur.