From ec4e15382c684f7f752f85b27b81f082f7a78228 Mon Sep 17 00:00:00 2001
From: Oreon Lothamer <oreon@metabase.com>
Date: Mon, 31 Aug 2026 11:23:18 -1000
Subject: [PATCH 1/7] feat: make --three-way-merge work without patch
 permissions

--three-way-merge computed the merge patch locally but asked the API
server to apply it as a dry-run, which needs the `patch` verb on every
diffed resource. A read-only account got:

    cannot patch "x" with kind Deployment: ... is forbidden

The patch is now applied locally when the server round-trip is not
available, using the same strategic-merge (or JSON merge patch, for
custom resources) logic the API server would use. Only `get` is
required.

A new --three-way-merge-mode flag (env HELM_DIFF_THREE_WAY_MERGE_MODE)
selects between:

  auto    server dry-run, falling back to the local merge on Forbidden
          or MethodNotAllowed. Any other error still aborts, so a
          genuinely bad patch is not masked. The default.
  server  the previous behaviour; a missing permission is a hard error.
  client  never sends the patch at all.

Two post-processing steps keep the local result close to what the API
server returns, since client-go does not ship the defaulting functions:

  * The merged object is round-tripped through its Go type, the way the
    API server does before it answers. That drops the empty values a
    manifest spells out but the type omits - `initialDelaySeconds: 0`,
    `hostNetwork: false`, `sysctls: []` - which would otherwise show up
    as additions the upgrade does not make.

  * A field is copied back from the live object when the old and the new
    release manifest agree about it. The patch replaces `retainKeys`
    structs and atomic lists as a whole, and a manifest that renders an
    unset value as an explicit `null` (a bare `replicas:`) reaches the
    patch as a change rather than a deletion; both wipe values the API
    server had defaulted in and re-defaults immediately after. Fields
    the two manifests disagree about are left deleted, because that is a
    change the chart really makes.

manifest.Generate takes variadic options rather than a new parameter, so
existing callers keep compiling.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---
 .github/copilot-instructions.md |   1 +
 README.md                       |  37 ++
 cmd/upgrade.go                  |  24 +-
 cmd/upgrade_test.go             |  46 +++
 manifest/generate.go            | 327 ++++++++++++++++--
 manifest/generate_test.go       | 583 ++++++++++++++++++++++++++++++++
 6 files changed, 996 insertions(+), 22 deletions(-)
 create mode 100644 manifest/generate_test.go

diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md
index be06c72b..ac3f88c3 100644
--- a/.github/copilot-instructions.md
+++ b/.github/copilot-instructions.md
@@ -91,6 +91,7 @@ Always reference these instructions first and fallback to search or bash command
 - `HELM_BIN` - Path to helm binary (for direct testing)
 - `HELM_DIFF_USE_UPGRADE_DRY_RUN` - Use helm upgrade --dry-run instead of template
 - `HELM_DIFF_THREE_WAY_MERGE` - Enable three-way merge diffing
+- `HELM_DIFF_THREE_WAY_MERGE_MODE` - Apply the three-way merge patch via the API server (`auto`/`server`) or locally (`client`)
 - `HELM_DIFF_NORMALIZE_MANIFESTS` - Normalize YAML before diffing
 - `HELM_DIFF_OUTPUT_CONTEXT` - Configure output context lines
 
diff --git a/README.md b/README.md
index 51a89a3c..26af9c10 100644
--- a/README.md
+++ b/README.md
@@ -188,6 +188,7 @@ Flags:
   -q, --suppress-secrets                         suppress secrets in the output
       --take-ownership                           if set, upgrade will ignore the check for helm annotations and take ownership of the existing resources
       --three-way-merge                          use three-way-merge to compute patch and generate diff output
+      --three-way-merge-mode string              how --three-way-merge applies the computed patch. Must be "auto", "server" or "client". "server" dry-runs the patch against the API server, which requires the patch permission. "client" merges locally and needs read access only, at the cost of not applying server-side defaulting and mutating webhooks. "auto" uses the server and falls back to the client when patching is not permitted (default "auto")
   -f, --values valueFiles                        specify values in a YAML file (can specify multiple) (default [])
       --version string                           specify the exact chart version to use. If this is not specified, the latest version is used
 
@@ -256,6 +257,35 @@ Notes:
 - helm-diff's own exit code is unaffected by the tool: `--detailed-exitcode` still returns `2` based on the changes helm-diff detected.
 - `--context`/`-C` is not applied; use the equivalent option of the external tool (for example `diff -U3`).
 
+### Three-way merge
+
+`--three-way-merge` diffs against what is actually in the cluster rather than against the manifests of the last release, so changes made outside of Helm show up too. To do that helm-diff has to compute the object that the upgrade would produce: it reads the live object, builds a three-way merge patch from the old release manifest, the new release manifest and the live object, and then applies that patch.
+
+`--three-way-merge-mode` controls how the patch is applied:
+
+- `server` sends the patch to the API server as a dry-run (`PATCH ...?dryRun=All`). The API server fills in defaults and runs mutating webhooks, so the result is the most faithful preview of the upgrade — but the credentials need the `patch` permission on every diffed resource.
+- `client` applies the patch locally, using the same strategic-merge (or JSON merge patch, for custom resources) logic the API server would use. Only `get` is required. The merged object is then round-tripped through its Go type, the way the API server does before it answers, and a field is copied back from the live object whenever the old and the new release manifest agree about it — without that, the defaults the API server re-applies after patching would show up as spurious removals. Validation and mutating webhooks are still not applied.
+- `auto` (the default) tries `server` first and falls back to `client` per run when the API server rejects the dry-run with `Forbidden` or `MethodNotAllowed`, printing a note on stderr. Any other error still aborts the diff.
+
+Because the defaulting functions are not part of client-go, `client` mode cannot reproduce them exactly. What it can do is leave the live value alone: a field is only reported as gone when the two release manifests disagree about it. That matters more than it sounds, because a chart that leaves a value unset usually renders the field as an explicit `null` — a bare `replicas:` — and a `null` in a manifest reaches the patch as a change rather than a deletion, wiping a value the API server had defaulted in even when the chart did not change at all.
+
+The remaining known deviation from `server` mode is the case where the manifests really do disagree and the field is one the API server defaults: a chart that stops pinning `replicas: 3` is reported as removing the field, where `server` mode shows it changing from `3` to the defaulted `1`. The change is reported either way, but `client` mode cannot name the value that replaces it.
+
+So a read-only account is enough for a three-way merge diff out of the box. Set `--three-way-merge-mode=client` (or `HELM_DIFF_THREE_WAY_MERGE_MODE=client`) to skip the rejected dry-run request entirely, and `--three-way-merge-mode=server` to make a missing `patch` permission a hard error instead of silently degrading the diff.
+
+The minimal RBAC for the `client` mode is read access to the diffed kinds plus the release storage:
+
+```yaml
+apiVersion: rbac.authorization.k8s.io/v1
+kind: Role
+metadata:
+  name: helm-diff
+rules:
+  - apiGroups: ["*"]
+    resources: ["*"]
+    verbs: ["get", "list"]
+```
+
 ## Commands:
 
 ### local:
@@ -349,6 +379,12 @@ Examples:
   # Read the flag usage below for more information on --three-way-merge.
   HELM_DIFF_THREE_WAY_MERGE=true helm diff upgrade my-release datadog/datadog
 
+  # Set HELM_DIFF_THREE_WAY_MERGE_MODE=client to compute the three-way merge
+  # locally, so that no permission to patch the cluster resources is needed.
+  # This is equivalent to specifying the --three-way-merge-mode flag.
+  # Read the flag usage below for more information on --three-way-merge-mode.
+  HELM_DIFF_THREE_WAY_MERGE_MODE=client helm diff upgrade my-release datadog/datadog
+
   # Set HELM_DIFF_NORMALIZE_MANIFESTS=true to
   # normalize the yaml file content when using helm diff.
   # This is equivalent to specifying the --normalize-manifests flag.
@@ -418,6 +454,7 @@ Flags:
   -q, --suppress-secrets                         suppress secrets in the output
       --take-ownership                           if set, upgrade will ignore the check for helm annotations and take ownership of the existing resources
       --three-way-merge                          use three-way-merge to compute patch and generate diff output
+      --three-way-merge-mode string              how --three-way-merge applies the computed patch. Must be "auto", "server" or "client". "server" dry-runs the patch against the API server, which requires the patch permission. "client" merges locally and needs read access only, at the cost of not applying server-side defaulting and mutating webhooks. "auto" uses the server and falls back to the client when patching is not permitted (default "auto")
   -f, --values valueFiles                        specify values in a YAML file (can specify multiple) (default [])
       --version string                           specify the exact chart version to use. If this is not specified, the latest version is used
 
diff --git a/cmd/upgrade.go b/cmd/upgrade.go
index e47e1933..63d112b0 100644
--- a/cmd/upgrade.go
+++ b/cmd/upgrade.go
@@ -68,6 +68,7 @@ type diffCmd struct {
 	normalizeManifests       bool
 	takeOwnership            bool
 	threeWayMerge            bool
+	threeWayMergeMode        string
 	serverSide               string
 	extraAPIs                []string
 	kubeVersion              string
@@ -162,6 +163,12 @@ func newChartCommand() *cobra.Command {
 			"  # Read the flag usage below for more information on --three-way-merge.",
 			"  HELM_DIFF_THREE_WAY_MERGE=true helm diff upgrade my-release datadog/datadog",
 			"",
+			"  # Set HELM_DIFF_THREE_WAY_MERGE_MODE=client to compute the three-way merge",
+			"  # locally, so that no permission to patch the cluster resources is needed.",
+			"  # This is equivalent to specifying the --three-way-merge-mode flag.",
+			"  # Read the flag usage below for more information on --three-way-merge-mode.",
+			"  HELM_DIFF_THREE_WAY_MERGE_MODE=client helm diff upgrade my-release datadog/datadog",
+			"",
 			"  # Set HELM_DIFF_NORMALIZE_MANIFESTS=true to",
 			"  # normalize the yaml file content when using helm diff.",
 			"  # This is equivalent to specifying the --normalize-manifests flag.",
@@ -187,6 +194,10 @@ func newChartCommand() *cobra.Command {
 				return fmt.Errorf("flag %q must be %q, %q or %q, but got %q", "server-side", envTrue, envFalse, serverSideAuto, diff.serverSide)
 			}
 
+			if !slices.Contains(manifest.ValidThreeWayMergeModes, diff.threeWayMergeMode) {
+				return fmt.Errorf("flag %q must be one of %q, but got %q", "three-way-merge-mode", manifest.ValidThreeWayMergeModes, diff.threeWayMergeMode)
+			}
+
 			if err := diff.validateRevision(cmd.Flags().Changed("revision")); err != nil {
 				return err
 			}
@@ -206,6 +217,15 @@ func newChartCommand() *cobra.Command {
 				}
 			}
 
+			if !cmd.Flags().Changed("three-way-merge-mode") {
+				if mode := os.Getenv("HELM_DIFF_THREE_WAY_MERGE_MODE"); mode != "" {
+					if !slices.Contains(manifest.ValidThreeWayMergeModes, mode) {
+						return fmt.Errorf("env var %q must be one of %q, but got %q", "HELM_DIFF_THREE_WAY_MERGE_MODE", manifest.ValidThreeWayMergeModes, mode)
+					}
+					diff.threeWayMergeMode = mode
+				}
+			}
+
 			if !diff.normalizeManifests && !cmd.Flags().Changed("normalize-manifests") {
 				enabled := os.Getenv("HELM_DIFF_NORMALIZE_MANIFESTS") == envTrue
 				diff.normalizeManifests = enabled
@@ -241,6 +261,7 @@ func newChartCommand() *cobra.Command {
 	f.StringVar(&kubeconfig, "kubeconfig", "", "This flag is ignored, to allow passing of this top level flag to helm")
 	addNamespaceFlags(f, &diff.namespaces)
 	f.BoolVar(&diff.threeWayMerge, "three-way-merge", false, "use three-way-merge to compute patch and generate diff output")
+	f.StringVar(&diff.threeWayMergeMode, "three-way-merge-mode", string(manifest.ThreeWayMergeAuto), `how --three-way-merge applies the computed patch. Must be "auto", "server" or "client". "server" dry-runs the patch against the API server, which requires the patch permission. "client" merges locally and needs read access only, at the cost of not applying server-side defaulting and mutating webhooks. "auto" uses the server and falls back to the client when patching is not permitted`)
 	f.StringVar(&diff.kubeContext, "kube-context", "", "name of the kubeconfig context to use")
 	f.StringVar(&diff.chartVersion, "version", "", "specify the exact chart version to use. If this is not specified, the latest version is used")
 	f.StringVar(&diff.chartRepo, "repo", "", "specify the chart repository url to locate the requested chart")
@@ -344,7 +365,8 @@ func (d *diffCmd) runHelm3() error {
 	}
 
 	if d.threeWayMerge {
-		releaseManifest, installManifest, err = manifest.Generate(actionConfig, releaseManifest, installManifest)
+		releaseManifest, installManifest, err = manifest.Generate(actionConfig, releaseManifest, installManifest,
+			manifest.WithThreeWayMergeMode(manifest.ThreeWayMergeMode(d.threeWayMergeMode)))
 		if err != nil {
 			return fmt.Errorf("unable to generate manifests: %w", err)
 		}
diff --git a/cmd/upgrade_test.go b/cmd/upgrade_test.go
index 900cc375..008a48aa 100644
--- a/cmd/upgrade_test.go
+++ b/cmd/upgrade_test.go
@@ -367,3 +367,49 @@ data:
 		}
 	})
 }
+
+func TestThreeWayMergeModeFlag(t *testing.T) {
+	if f := newChartCommand().Flags().Lookup("three-way-merge-mode"); f == nil {
+		t.Fatal("expected flag --three-way-merge-mode to be registered")
+	} else if f.DefValue != "auto" {
+		t.Errorf("expected --three-way-merge-mode to default to auto, got %q", f.DefValue)
+	}
+
+	cases := []struct {
+		name      string
+		args      []string
+		env       string
+		expectErr string
+	}{
+		{name: "server", args: []string{"--three-way-merge-mode", "server"}},
+		{name: "client", args: []string{"--three-way-merge-mode", "client"}},
+		{name: "auto", args: []string{"--three-way-merge-mode", "auto"}},
+		{name: "invalid flag", args: []string{"--three-way-merge-mode", "local"}, expectErr: "three-way-merge-mode"},
+		{name: "empty flag", args: []string{"--three-way-merge-mode", ""}, expectErr: "three-way-merge-mode"},
+		{name: "env var", env: "client"},
+		{name: "invalid env var", env: "local", expectErr: "HELM_DIFF_THREE_WAY_MERGE_MODE"},
+		{name: "flag wins over invalid env var", args: []string{"--three-way-merge-mode", "client"}, env: "local"},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Setenv("HELM_DIFF_THREE_WAY_MERGE_MODE", tc.env)
+
+			chartDir := t.TempDir()
+			setupFakeHelm(t, "capture_args", "", chartDir+"/args", "")
+
+			cmd := newChartCommand()
+			cmd.SetArgs(append([]string{"my-release", chartDir}, tc.args...))
+
+			err := cmd.Execute()
+			switch {
+			case tc.expectErr == "" && err != nil:
+				t.Fatalf("unexpected error: %v", err)
+			case tc.expectErr != "" && err == nil:
+				t.Fatalf("expected an error mentioning %q, got none", tc.expectErr)
+			case tc.expectErr != "" && !strings.Contains(err.Error(), tc.expectErr):
+				t.Fatalf("expected error mentioning %q, got %v", tc.expectErr, err)
+			}
+		})
+	}
+}
diff --git a/manifest/generate.go b/manifest/generate.go
index 23b8f6bd..803adc77 100644
--- a/manifest/generate.go
+++ b/manifest/generate.go
@@ -4,6 +4,8 @@ import (
 	"bytes"
 	"encoding/json"
 	"fmt"
+	"os"
+	"reflect"
 
 	jsonpatch "github.com/evanphx/json-patch/v5"
 	jsoniter "github.com/json-iterator/go"
@@ -23,7 +25,51 @@ const (
 	Helm3TestHook        = "test"
 )
 
-func Generate(actionConfig *action.Configuration, originalManifest, targetManifest []byte) ([]byte, []byte, error) {
+// ThreeWayMergeMode selects how the three-way merge patch is turned into the
+// object that the diff is computed against.
+type ThreeWayMergeMode string
+
+const (
+	// ThreeWayMergeAuto sends the patch to the API server as a dry-run and
+	// falls back to merging locally when the server refuses the request because
+	// the user has no permission to patch the resource.
+	ThreeWayMergeAuto ThreeWayMergeMode = "auto"
+	// ThreeWayMergeServer always sends the patch to the API server as a dry-run
+	// and fails when that is not permitted.
+	ThreeWayMergeServer ThreeWayMergeMode = "server"
+	// ThreeWayMergeClient always merges locally and never sends a patch to the
+	// API server, so that only read permissions are required.
+	ThreeWayMergeClient ThreeWayMergeMode = "client"
+)
+
+// ValidThreeWayMergeModes lists every accepted ThreeWayMergeMode value.
+var ValidThreeWayMergeModes = []string{
+	string(ThreeWayMergeAuto),
+	string(ThreeWayMergeServer),
+	string(ThreeWayMergeClient),
+}
+
+type generateOptions struct {
+	mergeMode ThreeWayMergeMode
+}
+
+// GenerateOption customizes the behaviour of Generate.
+type GenerateOption func(*generateOptions)
+
+// WithThreeWayMergeMode selects how the three-way merge patch is applied.
+// It defaults to ThreeWayMergeAuto.
+func WithThreeWayMergeMode(mode ThreeWayMergeMode) GenerateOption {
+	return func(o *generateOptions) {
+		o.mergeMode = mode
+	}
+}
+
+func Generate(actionConfig *action.Configuration, originalManifest, targetManifest []byte, opts ...GenerateOption) ([]byte, []byte, error) {
+	options := generateOptions{mergeMode: ThreeWayMergeAuto}
+	for _, opt := range opts {
+		opt(&options)
+	}
+
 	var err error
 	original, err := actionConfig.KubeClient.Build(bytes.NewBuffer(originalManifest), false)
 	if err != nil {
@@ -72,6 +118,19 @@ func Generate(actionConfig *action.Configuration, originalManifest, targetManife
 		return nil
 	})
 
+	// The warning about the fallback to the local merge is only interesting
+	// once, no matter how many resources the release contains.
+	warned := false
+	warnClientSideMerge := func(cause error) {
+		if warned {
+			return
+		}
+		warned = true
+		fmt.Fprintf(os.Stderr, "Not allowed to dry-run the patch against the cluster (%v).\n"+
+			"Falling back to computing the three-way merge locally. The diff may deviate from the\n"+
+			"actual upgrade result because server-side defaulting and mutating webhooks are not applied.\n", cause)
+	}
+
 	err = target.Visit(func(info *resource.Info, err error) error {
 		if err != nil {
 			return err
@@ -109,18 +168,18 @@ func Generate(actionConfig *action.Configuration, originalManifest, targetManife
 			return fmt.Errorf("could not find %q", info.Name)
 		}
 
-		patch, patchType, err := createPatch(originalInfo.Object, currentObj, info)
+		patch, err := createPatch(originalInfo.Object, currentObj, info)
 		if err != nil {
 			return err
 		}
 
-		helper.ServerDryRun = true
-		targetObj, err := helper.Patch(info.Namespace, info.Name, patchType, patch, nil)
+		// `out` still holds the live object, which is what the patch applies to.
+		merged, err := applyPatch(helper, info, patch, out, options.mergeMode, warnClientSideMerge)
 		if err != nil {
-			return fmt.Errorf("cannot patch %q with kind %s: %w", info.Name, kind, err)
+			return err
 		}
-		out, _ = jsoniter.ConfigCompatibleWithStandardLibrary.Marshal(targetObj)
-		pruneObj, err = deleteStatusAndTidyMetadata(out)
+
+		pruneObj, err = deleteStatusAndTidyMetadata(merged)
 		if err != nil {
 			return fmt.Errorf("prune current obj %q with kind %s: %w", info.Name, kind, err)
 		}
@@ -136,26 +195,238 @@ func Generate(actionConfig *action.Configuration, originalManifest, targetManife
 	return releaseManifest, installManifest, err
 }
 
-func createPatch(originalObj, currentObj runtime.Object, target *resource.Info) ([]byte, types.PatchType, error) {
+// resourcePatch is the patch computed for a single resource, together with
+// everything that is needed to apply it locally instead of on the API server.
+type resourcePatch struct {
+	data      []byte
+	patchType types.PatchType
+	// patchMeta describes how the individual fields of the object have to be
+	// merged. It is only set for strategic merge patches.
+	patchMeta strategicpatch.LookupPatchMeta
+	// versionedObject is the target object in its versioned type. It is nil for
+	// unstructured resources, which have no Go type to normalize against.
+	versionedObject runtime.Object
+	// originalData and modifiedData are the manifests of the old and the new
+	// release, the two inputs that tell which fields a chart actually asks for.
+	originalData []byte
+	modifiedData []byte
+}
+
+// apply merges the patch into the live object without contacting the API
+// server. Unlike the server-side dry-run this needs no permission to patch, but
+// it also skips defaulting, validation and mutating webhooks, so the result is
+// post-processed to stay as close to the server's answer as possible.
+func (p *resourcePatch) apply(liveData []byte) ([]byte, error) {
+	var merged []byte
+	var err error
+
+	switch p.patchType {
+	case types.MergePatchType:
+		merged, err = jsonpatch.MergePatch(liveData, p.data)
+	case types.StrategicMergePatchType:
+		merged, err = strategicpatch.StrategicMergePatchUsingLookupPatchMeta(liveData, p.data, p.patchMeta)
+	default:
+		return nil, fmt.Errorf("unsupported patch type %q", p.patchType)
+	}
+	if err != nil {
+		return nil, err
+	}
+
+	if merged, err = p.normalize(merged); err != nil {
+		return nil, err
+	}
+
+	return p.restoreServerPopulatedFields(merged, liveData)
+}
+
+// normalize round-trips the merged object through its Go type, the way the API
+// server does before it answers a patch request. That drops the empty values
+// the manifests spell out explicitly but the type omits, such as
+// `initialDelaySeconds: 0`, `hostNetwork: false` or `sysctls: []`, which would
+// otherwise show up as additions that the upgrade does not actually make.
+func (p *resourcePatch) normalize(merged []byte) ([]byte, error) {
+	if p.versionedObject == nil {
+		return merged, nil
+	}
+
+	objType := reflect.TypeOf(p.versionedObject)
+	if objType.Kind() != reflect.Ptr {
+		return merged, nil
+	}
+	typed, ok := reflect.New(objType.Elem()).Interface().(runtime.Object)
+	if !ok {
+		return merged, nil
+	}
+	if err := json.Unmarshal(merged, typed); err != nil {
+		return nil, fmt.Errorf("decoding the merged object: %w", err)
+	}
+	out, err := json.Marshal(typed)
+	if err != nil {
+		return nil, fmt.Errorf("encoding the merged object: %w", err)
+	}
+	return out, nil
+}
+
+// restoreServerPopulatedFields copies back the fields that only the API server
+// knows how to fill in.
+//
+// The merged object loses defaulted values in two ways. A three-way merge patch
+// replaces `retainKeys` structs and atomic lists as a whole, which drops what
+// the API server had defaulted into them - the rolling update strategy of a
+// Deployment, the `protocol: TCP` of a NetworkPolicy port, the `volumeMode` of a
+// volume claim template. And a manifest that renders a field as an explicit
+// `null`, the way a chart writes `replicas:` for a value it leaves unset, turns
+// into a `null` in the patch: the key is in both manifests, so it counts as a
+// change rather than a deletion and deletes the live value. Either way the API
+// server defaults the field straight back, but client-go does not ship the
+// defaulting functions, so the local merge has to recover the value differently.
+//
+// A field is copied back from the live object when the old and the new release
+// manifest agree about it - neither mentions it, or both give it the same value,
+// `null` included. Nothing then asked for the live value to go, so its
+// disappearance is an artifact of the patch rather than a change to report. Once
+// the two manifests disagree the deletion is honoured, because that is a change
+// the chart really makes.
+//
+// Only fields missing from the merged object are restored, never values that it
+// already carries, so drift between the cluster and the chart is still
+// reported.
+func (p *resourcePatch) restoreServerPopulatedFields(merged, liveData []byte) ([]byte, error) {
+	var mergedObj, liveObj, originalObj, modifiedObj interface{}
+
+	for _, in := range []struct {
+		data []byte
+		out  *interface{}
+	}{
+		{merged, &mergedObj},
+		{liveData, &liveObj},
+		{p.originalData, &originalObj},
+		{p.modifiedData, &modifiedObj},
+	} {
+		if len(in.data) == 0 {
+			continue
+		}
+		if err := json.Unmarshal(in.data, in.out); err != nil {
+			return nil, fmt.Errorf("decoding the object to restore defaulted fields: %w", err)
+		}
+	}
+
+	restored := restoreMissing(mergedObj, liveObj, originalObj, modifiedObj)
+
+	out, err := json.Marshal(restored)
+	if err != nil {
+		return nil, fmt.Errorf("encoding the object with the restored defaulted fields: %w", err)
+	}
+	return out, nil
+}
+
+// restoreMissing walks merged and live in parallel and copies over the parts of
+// live that merged lost without either release manifest asking for it. It
+// returns the updated merged value and never overwrites a value merged already
+// has.
+func restoreMissing(merged, live, original, modified interface{}) interface{} {
+	switch live := live.(type) {
+	case map[string]interface{}:
+		mergedMap, ok := merged.(map[string]interface{})
+		if !ok {
+			return merged
+		}
+		originalMap, _ := original.(map[string]interface{})
+		modifiedMap, _ := modified.(map[string]interface{})
+
+		for key, liveValue := range live {
+			mergedValue, inMerged := mergedMap[key]
+			if !inMerged {
+				// A key that is absent and a key that holds `null` both read as
+				// nil here, which is what makes an unset `replicas:` in the
+				// chart compare equal to the field the manifests never mention.
+				if reflect.DeepEqual(originalMap[key], modifiedMap[key]) {
+					mergedMap[key] = liveValue
+				}
+				continue
+			}
+			mergedMap[key] = restoreMissing(mergedValue, liveValue, originalMap[key], modifiedMap[key])
+		}
+		return mergedMap
+
+	case []interface{}:
+		mergedList, ok := merged.([]interface{})
+		if !ok || len(mergedList) != len(live) {
+			return merged
+		}
+		// Elements are paired by position, which is only sound as long as the
+		// chart itself left the list alone. Once the old and the new manifest
+		// disagree about it, the positions may mean different things and the
+		// list is left as the patch produced it.
+		originalList, _ := original.([]interface{})
+		modifiedList, _ := modified.([]interface{})
+		if len(originalList) != len(mergedList) || !reflect.DeepEqual(originalList, modifiedList) {
+			return mergedList
+		}
+
+		for i := range mergedList {
+			mergedList[i] = restoreMissing(mergedList[i], live[i], originalList[i], modifiedList[i])
+		}
+		return mergedList
+
+	default:
+		return merged
+	}
+}
+
+// applyPatch computes the patched object, either by letting the API server
+// dry-run the patch or by merging locally, depending on mode. warn is called at
+// most once, when mode is ThreeWayMergeAuto and the API server refused the
+// dry-run.
+func applyPatch(helper *resource.Helper, info *resource.Info, patch *resourcePatch, liveData []byte, mode ThreeWayMergeMode, warn func(cause error)) ([]byte, error) {
+	kind := info.Mapping.GroupVersionKind.Kind
+
+	if mode != ThreeWayMergeClient {
+		helper.ServerDryRun = true
+		targetObj, err := helper.Patch(info.Namespace, info.Name, patch.patchType, patch.data, nil)
+		switch {
+		case err == nil:
+			out, err := jsoniter.ConfigCompatibleWithStandardLibrary.Marshal(targetObj)
+			if err != nil {
+				return nil, fmt.Errorf("serializing patched %q with kind %s: %w", info.Name, kind, err)
+			}
+			return out, nil
+		case mode == ThreeWayMergeAuto && isPatchNotAllowed(err):
+			warn(err)
+		default:
+			return nil, fmt.Errorf("cannot patch %q with kind %s: %w", info.Name, kind, err)
+		}
+	}
+
+	out, err := patch.apply(liveData)
+	if err != nil {
+		return nil, fmt.Errorf("cannot merge %q with kind %s: %w", info.Name, kind, err)
+	}
+	return out, nil
+}
+
+// isPatchNotAllowed reports whether the API server rejected the patch because
+// the caller is not allowed to perform it, rather than because the patch itself
+// is bad.
+func isPatchNotAllowed(err error) bool {
+	return apierrors.IsForbidden(err) || apierrors.IsMethodNotSupported(err)
+}
+
+func createPatch(originalObj, currentObj runtime.Object, target *resource.Info) (*resourcePatch, error) {
 	oldData, err := json.Marshal(originalObj)
 	if err != nil {
-		return nil, types.StrategicMergePatchType, fmt.Errorf("serializing current configuration: %w", err)
+		return nil, fmt.Errorf("serializing current configuration: %w", err)
 	}
 	newData, err := json.Marshal(target.Object)
 	if err != nil {
-		return nil, types.StrategicMergePatchType, fmt.Errorf("serializing target configuration: %w", err)
+		return nil, fmt.Errorf("serializing target configuration: %w", err)
 	}
 
 	// Even if currentObj is nil (because it was not found), it will marshal just fine
 	currentData, err := json.Marshal(currentObj)
 	if err != nil {
-		return nil, types.StrategicMergePatchType, fmt.Errorf("serializing live configuration: %w", err)
+		return nil, fmt.Errorf("serializing live configuration: %w", err)
 	}
-	// kind := target.Mapping.GroupVersionKind.Kind
-	// if kind == "Deployment" {
-	// 	curr, _ := yaml.Marshal(currentObj)
-	// 	fmt.Println(string(curr))
-	// }
 
 	// Get a versioned object
 	versionedObject := kube.AsVersioned(target)
@@ -169,19 +440,33 @@ func createPatch(originalObj, currentObj runtime.Object, target *resource.Info)
 	// On newer K8s versions, CRDs aren't unstructured but has this dedicated type
 	_, isCRD := versionedObject.(*apiextv1.CustomResourceDefinition)
 
+	patch := &resourcePatch{originalData: oldData, modifiedData: newData}
+	if !isUnstructured {
+		patch.versionedObject = versionedObject
+	}
+
 	if isUnstructured || isCRD {
 		// fall back to generic JSON merge patch
-		patch, err := jsonpatch.CreateMergePatch(oldData, newData)
-		return patch, types.MergePatchType, err
+		patch.data, err = jsonpatch.CreateMergePatch(oldData, newData)
+		if err != nil {
+			return nil, err
+		}
+		patch.patchType = types.MergePatchType
+		return patch, nil
 	}
 
 	patchMeta, err := strategicpatch.NewPatchMetaFromStruct(versionedObject)
 	if err != nil {
-		return nil, types.StrategicMergePatchType, fmt.Errorf("unable to create patch metadata from object: %w", err)
+		return nil, fmt.Errorf("unable to create patch metadata from object: %w", err)
 	}
 
-	patch, err := strategicpatch.CreateThreeWayMergePatch(oldData, newData, currentData, patchMeta, true)
-	return patch, types.StrategicMergePatchType, err
+	patch.data, err = strategicpatch.CreateThreeWayMergePatch(oldData, newData, currentData, patchMeta, true)
+	if err != nil {
+		return nil, err
+	}
+	patch.patchType = types.StrategicMergePatchType
+	patch.patchMeta = patchMeta
+	return patch, nil
 }
 
 func objectKey(r *resource.Info) string {
diff --git a/manifest/generate_test.go b/manifest/generate_test.go
new file mode 100644
index 00000000..4020b892
--- /dev/null
+++ b/manifest/generate_test.go
@@ -0,0 +1,583 @@
+package manifest
+
+import (
+	"encoding/json"
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	appsv1 "k8s.io/api/apps/v1"
+	corev1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/meta"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/cli-runtime/pkg/resource"
+	"sigs.k8s.io/yaml"
+)
+
+func infoFor(t *testing.T, obj runtime.Object, gvk schema.GroupVersionKind) *resource.Info {
+	t.Helper()
+	return &resource.Info{
+		Object:    obj,
+		Namespace: "default",
+		Name:      "nginx",
+		Mapping:   &meta.RESTMapping{GroupVersionKind: gvk},
+	}
+}
+
+func deployment(replicas int32, image string, labels map[string]string) *appsv1.Deployment {
+	return &appsv1.Deployment{
+		TypeMeta:   metav1.TypeMeta{APIVersion: "apps/v1", Kind: "Deployment"},
+		ObjectMeta: metav1.ObjectMeta{Name: "nginx", Namespace: "default", Labels: labels},
+		Spec: appsv1.DeploymentSpec{
+			Replicas: &replicas,
+			Template: corev1.PodTemplateSpec{
+				Spec: corev1.PodSpec{
+					Containers: []corev1.Container{{Name: "nginx", Image: image}},
+				},
+			},
+		},
+	}
+}
+
+// TestCreatePatchApplyLocally_Strategic asserts that applying the three-way
+// merge patch locally yields the same object the API server would have returned
+// from the dry-run patch: the change from the chart is applied, while the field
+// only present in the cluster is preserved.
+func TestCreatePatchApplyLocally_Strategic(t *testing.T) {
+	gvk := appsv1.SchemeGroupVersion.WithKind("Deployment")
+
+	original := deployment(1, "nginx:1.0", nil)
+	target := deployment(2, "nginx:2.0", nil)
+
+	// The live object carries a field nobody in the release manifests knows
+	// about, e.g. set by a mutating webhook or another controller.
+	live := deployment(1, "nginx:1.0", nil)
+	live.Spec.Template.Spec.Containers[0].Env = []corev1.EnvVar{{Name: "INJECTED", Value: "yes"}}
+
+	patch, err := createPatch(original, live, infoFor(t, target, gvk))
+	require.NoError(t, err)
+	require.Equal(t, types.StrategicMergePatchType, patch.patchType)
+	require.NotNil(t, patch.patchMeta)
+
+	liveData, err := yaml.Marshal(live)
+	require.NoError(t, err)
+	liveJSON, err := yaml.YAMLToJSON(liveData)
+	require.NoError(t, err)
+
+	merged, err := patch.apply(liveJSON)
+	require.NoError(t, err)
+
+	var got appsv1.Deployment
+	require.NoError(t, yaml.Unmarshal(merged, &got))
+
+	assert.Equal(t, int32(2), *got.Spec.Replicas)
+	assert.Equal(t, "nginx:2.0", got.Spec.Template.Spec.Containers[0].Image)
+	assert.Equal(t,
+		[]corev1.EnvVar{{Name: "INJECTED", Value: "yes"}},
+		got.Spec.Template.Spec.Containers[0].Env,
+		"a field only present in the cluster must survive the local merge")
+}
+
+// TestCreatePatchApplyLocally_StrategicRemoval asserts that a field dropped from
+// the chart is removed by the local merge, which is what distinguishes the
+// three-way merge from a plain two-way merge.
+func TestCreatePatchApplyLocally_StrategicRemoval(t *testing.T) {
+	gvk := appsv1.SchemeGroupVersion.WithKind("Deployment")
+
+	original := deployment(1, "nginx:1.0", map[string]string{"keep": "me", "drop": "me"})
+	target := deployment(1, "nginx:1.0", map[string]string{"keep": "me"})
+	live := deployment(1, "nginx:1.0", map[string]string{"keep": "me", "drop": "me"})
+
+	patch, err := createPatch(original, live, infoFor(t, target, gvk))
+	require.NoError(t, err)
+
+	liveJSON, err := yaml.Marshal(live)
+	require.NoError(t, err)
+	liveJSON, err = yaml.YAMLToJSON(liveJSON)
+	require.NoError(t, err)
+
+	merged, err := patch.apply(liveJSON)
+	require.NoError(t, err)
+
+	var got appsv1.Deployment
+	require.NoError(t, yaml.Unmarshal(merged, &got))
+
+	assert.Equal(t, map[string]string{"keep": "me"}, got.ObjectMeta.Labels)
+}
+
+// TestCreatePatchApplyLocally_Unstructured covers custom resources, which use a
+// plain JSON merge patch instead of a strategic merge patch.
+func TestCreatePatchApplyLocally_Unstructured(t *testing.T) {
+	gvk := schema.GroupVersionKind{Group: "example.com", Version: "v1", Kind: "Widget"}
+
+	newWidget := func(size string) *unstructured.Unstructured {
+		return &unstructured.Unstructured{Object: map[string]interface{}{
+			"apiVersion": "example.com/v1",
+			"kind":       "Widget",
+			"metadata":   map[string]interface{}{"name": "nginx", "namespace": "default"},
+			"spec":       map[string]interface{}{"size": size},
+		}}
+	}
+
+	original := newWidget("small")
+	target := newWidget("large")
+
+	live := newWidget("small")
+	require.NoError(t, unstructured.SetNestedField(live.Object, "set-by-the-cluster", "spec", "extra"))
+
+	patch, err := createPatch(original, live, infoFor(t, target, gvk))
+	require.NoError(t, err)
+	require.Equal(t, types.MergePatchType, patch.patchType)
+
+	liveJSON, err := live.MarshalJSON()
+	require.NoError(t, err)
+
+	merged, err := patch.apply(liveJSON)
+	require.NoError(t, err)
+
+	var got unstructured.Unstructured
+	require.NoError(t, got.UnmarshalJSON(merged))
+
+	size, _, _ := unstructured.NestedString(got.Object, "spec", "size")
+	assert.Equal(t, "large", size)
+	extra, _, _ := unstructured.NestedString(got.Object, "spec", "extra")
+	assert.Equal(t, "set-by-the-cluster", extra, "a field only present in the cluster must survive the local merge")
+}
+
+func TestIsPatchNotAllowed(t *testing.T) {
+	gr := schema.GroupResource{Group: "apps", Resource: "deployments"}
+
+	assert.True(t, isPatchNotAllowed(apierrors.NewForbidden(gr, "nginx", assert.AnError)))
+	assert.True(t, isPatchNotAllowed(apierrors.NewMethodNotSupported(gr, "patch")))
+	assert.False(t, isPatchNotAllowed(apierrors.NewNotFound(gr, "nginx")))
+	assert.False(t, isPatchNotAllowed(apierrors.NewInternalError(assert.AnError)))
+	assert.False(t, isPatchNotAllowed(assert.AnError))
+}
+
+func TestResourcePatchApplyUnsupportedType(t *testing.T) {
+	p := &resourcePatch{data: []byte("{}"), patchType: types.JSONPatchType}
+	_, err := p.apply([]byte("{}"))
+	assert.ErrorContains(t, err, "unsupported patch type")
+}
+
+// The cases below reproduce the differences that were reported between the
+// client-side merge and the server-side dry-run.
+//
+// The manifests are decoded into unstructured objects because that is what
+// helm's kube.Client.Build hands to Generate: they keep whatever the chart
+// spelled out, including the zero values a typed object would omit. The live
+// object is written the way the API server returns it, with the defaults filled
+// in and the zero values gone.
+
+func fromYAML(t *testing.T, manifest string) *unstructured.Unstructured {
+	t.Helper()
+	obj := &unstructured.Unstructured{}
+	require.NoError(t, yaml.Unmarshal([]byte(manifest), &obj.Object))
+	return obj
+}
+
+// applyLocally is the client-side path of applyPatch: build the patch from the
+// three inputs and merge it into the live object without the API server.
+func applyLocally(t *testing.T, original, live, target string) map[string]interface{} {
+	t.Helper()
+
+	targetObj := fromYAML(t, target)
+	liveObj := fromYAML(t, live)
+	gvk := targetObj.GroupVersionKind()
+
+	patch, err := createPatch(fromYAML(t, original), liveObj, infoFor(t, targetObj, gvk))
+	require.NoError(t, err)
+
+	liveJSON, err := liveObj.MarshalJSON()
+	require.NoError(t, err)
+
+	merged, err := patch.apply(liveJSON)
+	require.NoError(t, err)
+
+	var got map[string]interface{}
+	require.NoError(t, json.Unmarshal(merged, &got))
+	return got
+}
+
+func nested(t *testing.T, obj map[string]interface{}, fields ...string) (interface{}, bool) {
+	t.Helper()
+	value, found, err := unstructured.NestedFieldNoCopy(obj, fields...)
+	require.NoError(t, err)
+	return value, found
+}
+
+// The rolling update strategy is defaulted by the API server and pruned by the
+// $retainKeys directive the patch carries for spec.strategy.
+func TestLocalMerge_KeepsDefaultedRollingUpdate(t *testing.T) {
+	chart := `
+apiVersion: apps/v1
+kind: Deployment
+metadata: {name: nginx, namespace: default}
+spec:
+  strategy: {type: RollingUpdate}
+  selector: {matchLabels: {app: nginx}}
+  template:
+    metadata: {labels: {app: nginx}}
+    spec:
+      containers: [{name: nginx, image: "%s"}]
+`
+	live := `
+apiVersion: apps/v1
+kind: Deployment
+metadata: {name: nginx, namespace: default}
+spec:
+  replicas: 1
+  strategy:
+    type: RollingUpdate
+    rollingUpdate: {maxSurge: 25%, maxUnavailable: 25%}
+  selector: {matchLabels: {app: nginx}}
+  template:
+    metadata: {labels: {app: nginx}}
+    spec:
+      containers: [{name: nginx, image: "nginx:1.0"}]
+`
+	got := applyLocally(t, fmt.Sprintf(chart, "nginx:1.0"), live, fmt.Sprintf(chart, "nginx:2.0"))
+
+	rollingUpdate, found := nested(t, got, "spec", "strategy", "rollingUpdate")
+	require.True(t, found, "the defaulted rolling update strategy must not be dropped")
+	assert.Equal(t, map[string]interface{}{"maxSurge": "25%", "maxUnavailable": "25%"}, rollingUpdate)
+
+	replicas, found := nested(t, got, "spec", "replicas")
+	require.True(t, found, "the defaulted replica count must not be dropped")
+	assert.EqualValues(t, 1, replicas)
+
+	containers, _ := nested(t, got, "spec", "template", "spec", "containers")
+	assert.Equal(t, "nginx:2.0", containers.([]interface{})[0].(map[string]interface{})["image"],
+		"the actual change must still be applied")
+}
+
+// Values the chart spells out but the Go type omits must not show up as
+// additions: the API server drops them when it answers the patch.
+func TestLocalMerge_DropsExplicitZeroValues(t *testing.T) {
+	chart := `
+apiVersion: apps/v1
+kind: StatefulSet
+metadata: {name: meilisearch, namespace: default}
+spec:
+  serviceName: meilisearch
+  selector: {matchLabels: {app: meilisearch}}
+  template:
+    metadata: {labels: {app: meilisearch}}
+    spec:
+      hostIPC: false
+      hostNetwork: false
+      securityContext: {supplementalGroups: [], sysctls: []}
+      containers:
+        - name: meilisearch
+          image: "%s"
+          livenessProbe: {initialDelaySeconds: 0, exec: {command: [ok]}}
+          readinessProbe: {initialDelaySeconds: 0, exec: {command: [ok]}}
+`
+	live := `
+apiVersion: apps/v1
+kind: StatefulSet
+metadata: {name: meilisearch, namespace: default}
+spec:
+  serviceName: meilisearch
+  selector: {matchLabels: {app: meilisearch}}
+  template:
+    metadata: {labels: {app: meilisearch}}
+    spec:
+      securityContext: {}
+      containers:
+        - name: meilisearch
+          image: getmeili/meilisearch:v1.0
+          livenessProbe: {exec: {command: [ok]}}
+          readinessProbe: {exec: {command: [ok]}}
+`
+	got := applyLocally(t, fmt.Sprintf(chart, "getmeili/meilisearch:v1.0"), live, fmt.Sprintf(chart, "getmeili/meilisearch:v1.1"))
+
+	podSpec, found := nested(t, got, "spec", "template", "spec")
+	require.True(t, found)
+	pod := podSpec.(map[string]interface{})
+
+	assert.NotContains(t, pod, "hostIPC", "hostIPC: false must be normalized away")
+	assert.NotContains(t, pod, "hostNetwork", "hostNetwork: false must be normalized away")
+	assert.Equal(t, map[string]interface{}{}, pod["securityContext"],
+		"supplementalGroups: [] and sysctls: [] must be normalized away")
+
+	container := pod["containers"].([]interface{})[0].(map[string]interface{})
+	assert.Equal(t, "getmeili/meilisearch:v1.1", container["image"], "the actual change must still be applied")
+	for _, probe := range []string{"livenessProbe", "readinessProbe"} {
+		assert.NotContains(t, container[probe], "initialDelaySeconds",
+			"initialDelaySeconds: 0 must be normalized away in the %s", probe)
+	}
+}
+
+// NetworkPolicy ports are an atomic list, so the patch replaces the whole list
+// and drops the protocol the API server defaulted in.
+func TestLocalMerge_KeepsDefaultedFieldsInAtomicLists(t *testing.T) {
+	chart := `
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata: {name: mongodb, namespace: default}
+spec:
+  podSelector: {matchLabels: {app: "%s"}}
+  ingress:
+    - ports: [{port: 27017}]
+`
+	live := `
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata: {name: mongodb, namespace: default}
+spec:
+  podSelector: {matchLabels: {app: mongo}}
+  policyTypes: [Ingress]
+  ingress:
+    - ports: [{port: 27017, protocol: TCP}]
+`
+	got := applyLocally(t, fmt.Sprintf(chart, "mongo"), live, fmt.Sprintf(chart, "mongodb"))
+
+	ingress, found := nested(t, got, "spec", "ingress")
+	require.True(t, found)
+	port := ingress.([]interface{})[0].(map[string]interface{})["ports"].([]interface{})[0].(map[string]interface{})
+	assert.Equal(t, "TCP", port["protocol"], "the defaulted protocol must not be dropped")
+	assert.EqualValues(t, 27017, port["port"])
+
+	app, _ := nested(t, got, "spec", "podSelector", "matchLabels", "app")
+	assert.Equal(t, "mongodb", app, "the actual change must still be applied")
+}
+
+// Volume claim templates are an atomic list too, and the API server populates
+// them with a type, a status and a volume mode.
+func TestLocalMerge_KeepsServerPopulatedVolumeClaimTemplates(t *testing.T) {
+	chart := `
+apiVersion: apps/v1
+kind: StatefulSet
+metadata: {name: mongodb, namespace: default}
+spec:
+  serviceName: mongodb
+  selector: {matchLabels: {app: mongodb}}
+  template:
+    metadata: {labels: {app: mongodb}}
+    spec:
+      containers: [{name: mongodb, image: "%s"}]
+  volumeClaimTemplates:
+    - metadata: {name: data}
+      spec:
+        accessModes: [ReadWriteOnce]
+        resources: {requests: {storage: 8Gi}}
+`
+	live := `
+apiVersion: apps/v1
+kind: StatefulSet
+metadata: {name: mongodb, namespace: default}
+spec:
+  serviceName: mongodb
+  selector: {matchLabels: {app: mongodb}}
+  template:
+    metadata: {labels: {app: mongodb}}
+    spec:
+      containers: [{name: mongodb, image: "mongo:6.0"}]
+  volumeClaimTemplates:
+    - apiVersion: v1
+      kind: PersistentVolumeClaim
+      metadata: {name: data}
+      spec:
+        accessModes: [ReadWriteOnce]
+        resources: {requests: {storage: 8Gi}}
+        volumeMode: Filesystem
+      status: {phase: Pending}
+`
+	got := applyLocally(t, fmt.Sprintf(chart, "mongo:6.0"), live, fmt.Sprintf(chart, "mongo:7.0"))
+
+	templates, found := nested(t, got, "spec", "volumeClaimTemplates")
+	require.True(t, found)
+	claim := templates.([]interface{})[0].(map[string]interface{})
+
+	assert.Equal(t, "v1", claim["apiVersion"], "the server-populated apiVersion must not be dropped")
+	assert.Equal(t, "PersistentVolumeClaim", claim["kind"], "the server-populated kind must not be dropped")
+	assert.Equal(t, map[string]interface{}{"phase": "Pending"}, claim["status"], "the status must not be dropped")
+	assert.Equal(t, "Filesystem", claim["spec"].(map[string]interface{})["volumeMode"],
+		"the defaulted volumeMode must not be dropped")
+}
+
+// A field the chart itself stops setting is a real change and must stay
+// visible, even where the API server would default it back.
+func TestLocalMerge_ReportsFieldsTheChartRemoves(t *testing.T) {
+	deploy := `
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: nginx
+  namespace: default
+  labels: {%s}
+spec:
+  selector: {matchLabels: {app: nginx}}
+  template:
+    metadata: {labels: {app: nginx}}
+    spec:
+      containers: [{name: nginx, image: "nginx:1.0"}]
+`
+	withLabel := fmt.Sprintf(deploy, "drop: me")
+	got := applyLocally(t, withLabel, withLabel, fmt.Sprintf(deploy, ""))
+
+	labels, found := nested(t, got, "metadata", "labels")
+	assert.False(t, found, "a label the chart no longer sets must be reported as removed, got %v", labels)
+}
+
+// Drift must survive the restoration pass: a value the cluster and the chart
+// disagree about is not a defaulted field.
+func TestLocalMerge_KeepsReportingDrift(t *testing.T) {
+	chart := `
+apiVersion: apps/v1
+kind: Deployment
+metadata: {name: nginx, namespace: default}
+spec:
+  replicas: 1
+  selector: {matchLabels: {app: nginx}}
+  template:
+    metadata: {labels: {app: nginx}}
+    spec:
+      containers: [{name: nginx, image: nginx:1.0}]
+`
+	// Someone scaled and re-imaged the deployment by hand.
+	live := `
+apiVersion: apps/v1
+kind: Deployment
+metadata: {name: nginx, namespace: default}
+spec:
+  replicas: 5
+  selector: {matchLabels: {app: nginx}}
+  template:
+    metadata: {labels: {app: nginx}}
+    spec:
+      containers: [{name: nginx, image: nginx:9.9}]
+`
+	got := applyLocally(t, chart, live, chart)
+
+	replicas, _ := nested(t, got, "spec", "replicas")
+	assert.EqualValues(t, 1, replicas, "drifted replicas must be reset to the chart value")
+	containers, _ := nested(t, got, "spec", "template", "spec", "containers")
+	assert.Equal(t, "nginx:1.0", containers.([]interface{})[0].(map[string]interface{})["image"],
+		"a drifted image must be reset to the chart value")
+}
+
+// A chart that leaves a value unset renders the field as an explicit `null`
+// (`replicas:` with nothing after it). The key is present in both manifests, so
+// the patch carries it as a change rather than a deletion and wipes the value
+// the API server had defaulted into the cluster - even though the chart itself
+// did not change at all.
+func TestLocalMerge_KeepsDefaultsUnderExplicitNulls(t *testing.T) {
+	chart := `
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: librechat-librechat-rag-api
+  namespace: librechat
+  labels: {app.kubernetes.io/instance: librechat}
+spec:
+  replicas:
+  selector:
+    matchLabels: {app.kubernetes.io/name: rag}
+  template:
+    metadata:
+      annotations:
+      labels: {app.kubernetes.io/name: rag}
+    spec:
+      securityContext: {}
+      containers:
+        - name: rag
+          image: "ghcr.io/danny-avila/librechat-rag-api-dev-lite:%s"
+          imagePullPolicy: IfNotPresent
+          ports: [{name: http, containerPort: 8000, protocol: TCP}]
+          livenessProbe:
+            null
+          readinessProbe:
+            null
+          resources: {}
+      volumes:
+`
+	live := `
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: librechat-librechat-rag-api
+  namespace: librechat
+  labels: {app.kubernetes.io/instance: librechat}
+spec:
+  replicas: 1
+  revisionHistoryLimit: 10
+  progressDeadlineSeconds: 600
+  selector:
+    matchLabels: {app.kubernetes.io/name: rag}
+  strategy:
+    type: RollingUpdate
+    rollingUpdate: {maxSurge: 25%, maxUnavailable: 25%}
+  template:
+    metadata:
+      labels: {app.kubernetes.io/name: rag}
+    spec:
+      securityContext: {}
+      dnsPolicy: ClusterFirst
+      restartPolicy: Always
+      containers:
+        - name: rag
+          image: "ghcr.io/danny-avila/librechat-rag-api-dev-lite:latest"
+          imagePullPolicy: IfNotPresent
+          ports: [{name: http, containerPort: 8000, protocol: TCP}]
+          resources: {}
+          terminationMessagePath: /dev/termination-log
+`
+	// The chart is byte-for-byte the same on both sides.
+	unchanged := fmt.Sprintf(chart, "latest")
+	got := applyLocally(t, unchanged, live, unchanged)
+
+	replicas, found := nested(t, got, "spec", "replicas")
+	require.True(t, found, "an unset `replicas:` must not wipe the replica count the API server defaulted in")
+	assert.EqualValues(t, 1, replicas)
+
+	rollingUpdate, found := nested(t, got, "spec", "strategy", "rollingUpdate")
+	require.True(t, found, "the defaulted rolling update strategy must survive too")
+	assert.Equal(t, map[string]interface{}{"maxSurge": "25%", "maxUnavailable": "25%"}, rollingUpdate)
+
+	for _, field := range []string{"revisionHistoryLimit", "progressDeadlineSeconds"} {
+		_, found = nested(t, got, "spec", field)
+		assert.True(t, found, "the defaulted %s must survive too", field)
+	}
+}
+
+// The same explicit `null`, but this time the chart really does change what it
+// asks for. The removal is then a change and has to stay visible.
+func TestLocalMerge_ReportsExplicitNullThatTheChartIntroduces(t *testing.T) {
+	deploy := `
+apiVersion: apps/v1
+kind: Deployment
+metadata: {name: rag, namespace: default}
+spec:
+  replicas: %s
+  selector: {matchLabels: {app: rag}}
+  template:
+    metadata: {labels: {app: rag}}
+    spec:
+      containers: [{name: rag, image: rag:1.0}]
+`
+	live := `
+apiVersion: apps/v1
+kind: Deployment
+metadata: {name: rag, namespace: default}
+spec:
+  replicas: 3
+  selector: {matchLabels: {app: rag}}
+  template:
+    metadata: {labels: {app: rag}}
+    spec:
+      containers: [{name: rag, image: rag:1.0}]
+`
+	// The chart used to pin three replicas and now leaves the field unset.
+	got := applyLocally(t, fmt.Sprintf(deploy, "3"), live, fmt.Sprintf(deploy, ""))
+
+	_, found := nested(t, got, "spec", "replicas")
+	assert.False(t, found, "a replica count the chart stops pinning is a real change and must be reported")
+}

From e4cae5b806e0f369b677efbad4f5ad9911b71bb1 Mon Sep 17 00:00:00 2001
From: Oreon Lothamer <oreon@metabase.com>
Date: Mon, 31 Aug 2026 12:27:24 -1000
Subject: [PATCH 2/7] fix: treat empty collections and null as the same value
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

A chart that renders an empty collection literally - `rules: []`, the
branch grafana's Role and ClusterRole take when no sidecar is enabled -
disagrees with the cluster about how to write "nothing". Kubernetes
stores objects as protobuf, which cannot tell an empty repeated field
from an absent one, so the API server answers with `rules: null`.

The patch then carries `{"rules":[]}` even though the chart did not
change, and the client-side merge reported:

    rules  (rbac.authorization.k8s.io/v1/Role/grafana/grafana)
    ± type change from <nil> to list

server mode shows nothing, because its answer goes back through the same
storage round-trip. The local merge now keeps whichever spelling the
cluster reports whenever the merged and the live value are both empty.
Emptying a collection that actually had entries is still a change and is
still reported.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---
 README.md                 |  2 +-
 manifest/generate.go      | 24 +++++++++++++++++++++
 manifest/generate_test.go | 44 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 69 insertions(+), 1 deletion(-)

diff --git a/README.md b/README.md
index 26af9c10..2a723896 100644
--- a/README.md
+++ b/README.md
@@ -264,7 +264,7 @@ Notes:
 `--three-way-merge-mode` controls how the patch is applied:
 
 - `server` sends the patch to the API server as a dry-run (`PATCH ...?dryRun=All`). The API server fills in defaults and runs mutating webhooks, so the result is the most faithful preview of the upgrade — but the credentials need the `patch` permission on every diffed resource.
-- `client` applies the patch locally, using the same strategic-merge (or JSON merge patch, for custom resources) logic the API server would use. Only `get` is required. The merged object is then round-tripped through its Go type, the way the API server does before it answers, and a field is copied back from the live object whenever the old and the new release manifest agree about it — without that, the defaults the API server re-applies after patching would show up as spurious removals. Validation and mutating webhooks are still not applied.
+- `client` applies the patch locally, using the same strategic-merge (or JSON merge patch, for custom resources) logic the API server would use. Only `get` is required. The merged object is then round-tripped through its Go type, the way the API server does before it answers, and a field is copied back from the live object whenever the old and the new release manifest agree about it — without that, the defaults the API server re-applies after patching would show up as spurious removals. An empty list, an empty map and a `null` are also treated as the same value, because Kubernetes stores objects as protobuf and cannot tell them apart: a chart that writes `rules: []` gets `rules: null` back from the cluster. Validation and mutating webhooks are still not applied.
 - `auto` (the default) tries `server` first and falls back to `client` per run when the API server rejects the dry-run with `Forbidden` or `MethodNotAllowed`, printing a note on stderr. Any other error still aborts the diff.
 
 Because the defaulting functions are not part of client-go, `client` mode cannot reproduce them exactly. What it can do is leave the live value alone: a field is only reported as gone when the two release manifests disagree about it. That matters more than it sounds, because a chart that leaves a value unset usually renders the field as an explicit `null` — a bare `replicas:` — and a `null` in a manifest reaches the patch as a change rather than a deletion, wiping a value the API server had defaulted in even when the chart did not change at all.
diff --git a/manifest/generate.go b/manifest/generate.go
index 803adc77..4f08017c 100644
--- a/manifest/generate.go
+++ b/manifest/generate.go
@@ -320,6 +320,21 @@ func (p *resourcePatch) restoreServerPopulatedFields(merged, liveData []byte) ([
 	return out, nil
 }
 
+// isEmpty reports whether v carries nothing - a null, an empty list or an empty
+// map. The three are interchangeable in a stored Kubernetes object, so telling
+// them apart in a diff only ever produces noise.
+func isEmpty(v interface{}) bool {
+	switch v := v.(type) {
+	case nil:
+		return true
+	case []interface{}:
+		return len(v) == 0
+	case map[string]interface{}:
+		return len(v) == 0
+	}
+	return false
+}
+
 // restoreMissing walks merged and live in parallel and copies over the parts of
 // live that merged lost without either release manifest asking for it. It
 // returns the updated merged value and never overwrites a value merged already
@@ -336,6 +351,15 @@ func restoreMissing(merged, live, original, modified interface{}) interface{} {
 
 		for key, liveValue := range live {
 			mergedValue, inMerged := mergedMap[key]
+			if inMerged && isEmpty(mergedValue) && isEmpty(liveValue) {
+				// Two empty values that differ only in how they are written are
+				// not a change: the API server stores objects as protobuf,
+				// which cannot tell an empty list from an absent one, so a
+				// chart's `rules: []` comes back from the cluster as
+				// `rules: null`. Keep whichever the cluster reports.
+				mergedMap[key] = liveValue
+				continue
+			}
 			if !inMerged {
 				// A key that is absent and a key that holds `null` both read as
 				// nil here, which is what makes an unset `replicas:` in the
diff --git a/manifest/generate_test.go b/manifest/generate_test.go
index 4020b892..e4537f11 100644
--- a/manifest/generate_test.go
+++ b/manifest/generate_test.go
@@ -581,3 +581,47 @@ spec:
 	_, found := nested(t, got, "spec", "replicas")
 	assert.False(t, found, "a replica count the chart stops pinning is a real change and must be reported")
 }
+
+// A chart that renders an empty collection literally (`rules: []`, the branch
+// grafana takes when no sidecar is enabled) disagrees with the cluster about how
+// to write "nothing": the API server stores objects as protobuf, which cannot
+// tell an empty list from an absent one, and answers with `rules: null`. The two
+// are the same object and must not be reported as a change.
+func TestLocalMerge_TreatsEmptyCollectionsAsEqual(t *testing.T) {
+	role := func(rules string) string {
+		return fmt.Sprintf(`
+apiVersion: rbac.authorization.k8s.io/v1
+kind: Role
+metadata: {name: grafana, namespace: grafana}
+rules: %s
+`, rules)
+	}
+
+	t.Run("chart writes [], cluster answers null", func(t *testing.T) {
+		got := applyLocally(t, role("[]"), role("null"), role("[]"))
+		rules, _ := nested(t, got, "rules")
+		assert.Nil(t, rules, "an empty list must not be reported as a change against a null")
+	})
+
+	t.Run("chart writes null, cluster answers []", func(t *testing.T) {
+		got := applyLocally(t, role("null"), role("[]"), role("null"))
+		rules, _ := nested(t, got, "rules")
+		assert.Equal(t, []interface{}{}, rules, "a null must not be reported as a change against an empty list")
+	})
+
+	// Emptying a collection that actually had entries is a real change.
+	t.Run("chart empties a populated list", func(t *testing.T) {
+		populated := `
+apiVersion: rbac.authorization.k8s.io/v1
+kind: Role
+metadata: {name: grafana, namespace: grafana}
+rules:
+  - apiGroups: [""]
+    resources: [configmaps]
+    verbs: [get]
+`
+		got := applyLocally(t, populated, populated, role("[]"))
+		rules, _ := nested(t, got, "rules")
+		assert.Empty(t, rules, "emptying a populated list is a real change and must be reported")
+	})
+}

From 3d2f839cae55f6df658ccfff809067bf4503e2ad Mon Sep 17 00:00:00 2001
From: Oreon Lothamer <oreon@metabase.com>
Date: Tue, 1 Sep 2026 15:47:57 -1000
Subject: [PATCH 3/7] fix: address review feedback on --three-way-merge-mode

auto mode now falls back for the whole run, as documented. It warned
once but kept sending a dry-run PATCH for every remaining resource, so a
release of N objects produced N denied requests - N audit log entries and
N round-trips - to reach the same conclusion each time. A refused dry-run
says something about the credentials rather than about the resource, so
the first refusal now moves the rest of the run to the local merge. The
decision moved into a clientSideFallback type so it can be tested without
a cluster.

HELM_DIFF_THREE_WAY_MERGE_MODE is read only when the run actually
performs a three-way merge (--three-way-merge, or --take-ownership which
implies it). A stale value in the environment could otherwise fail an
unrelated `helm diff upgrade` over a mode it was never going to use. The
flag stays validated unconditionally: passing it is deliberate, so a typo
is worth reporting either way.

The README no longer calls a wildcard role "minimal RBAC" - it grants
read access to every resource in every API group. It is now described as
the blunt option, next to a scoped example and a note that a kind missing
from the role fails the diff for that resource.

Also fixes the grammar in the two createPatch comments the review
flagged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---
 README.md                 | 20 +++++++++++++-
 cmd/upgrade.go            |  6 ++++-
 cmd/upgrade_test.go       | 44 ++++++++++++++++++++++++++++--
 manifest/generate.go      | 57 ++++++++++++++++++++++++++-------------
 manifest/generate_test.go | 34 +++++++++++++++++++++++
 5 files changed, 139 insertions(+), 22 deletions(-)

diff --git a/README.md b/README.md
index 2a723896..bffe7b40 100644
--- a/README.md
+++ b/README.md
@@ -273,7 +273,7 @@ The remaining known deviation from `server` mode is the case where the manifests
 
 So a read-only account is enough for a three-way merge diff out of the box. Set `--three-way-merge-mode=client` (or `HELM_DIFF_THREE_WAY_MERGE_MODE=client`) to skip the rejected dry-run request entirely, and `--three-way-merge-mode=server` to make a missing `patch` permission a hard error instead of silently degrading the diff.
 
-The minimal RBAC for the `client` mode is read access to the diffed kinds plus the release storage:
+`client` mode needs `get` on every kind the chart renders, plus `get` on the Secret or ConfigMap holding the release. The role below is the blunt version — read access to everything, which is convenient but broader than a diff requires:
 
 ```yaml
 apiVersion: rbac.authorization.k8s.io/v1
@@ -286,6 +286,24 @@ rules:
     verbs: ["get", "list"]
 ```
 
+For least privilege, list only the kinds the chart actually renders, for example:
+
+```yaml
+apiVersion: rbac.authorization.k8s.io/v1
+kind: Role
+metadata:
+  name: helm-diff
+rules:
+  - apiGroups: [""]
+    resources: ["configmaps", "secrets", "services", "serviceaccounts"]
+    verbs: ["get"]
+  - apiGroups: ["apps"]
+    resources: ["deployments", "statefulsets", "daemonsets"]
+    verbs: ["get"]
+```
+
+A kind that is missing from the role makes the diff fail on that resource, so the set has to cover everything the chart renders — including the kinds it only renders under some values.
+
 ## Commands:
 
 ### local:
diff --git a/cmd/upgrade.go b/cmd/upgrade.go
index 63d112b0..79250897 100644
--- a/cmd/upgrade.go
+++ b/cmd/upgrade.go
@@ -217,7 +217,11 @@ func newChartCommand() *cobra.Command {
 				}
 			}
 
-			if !cmd.Flags().Changed("three-way-merge-mode") {
+			// Only consulted when the run actually performs a three-way merge:
+			// the variable may well be set for a different invocation, and an
+			// unrelated `helm diff upgrade` should not fail over a value it is
+			// never going to use. --take-ownership turns the merge on as well.
+			if (diff.threeWayMerge || diff.takeOwnership) && !cmd.Flags().Changed("three-way-merge-mode") {
 				if mode := os.Getenv("HELM_DIFF_THREE_WAY_MERGE_MODE"); mode != "" {
 					if !slices.Contains(manifest.ValidThreeWayMergeModes, mode) {
 						return fmt.Errorf("env var %q must be one of %q, but got %q", "HELM_DIFF_THREE_WAY_MERGE_MODE", manifest.ValidThreeWayMergeModes, mode)
diff --git a/cmd/upgrade_test.go b/cmd/upgrade_test.go
index 008a48aa..283e4597 100644
--- a/cmd/upgrade_test.go
+++ b/cmd/upgrade_test.go
@@ -386,8 +386,10 @@ func TestThreeWayMergeModeFlag(t *testing.T) {
 		{name: "auto", args: []string{"--three-way-merge-mode", "auto"}},
 		{name: "invalid flag", args: []string{"--three-way-merge-mode", "local"}, expectErr: "three-way-merge-mode"},
 		{name: "empty flag", args: []string{"--three-way-merge-mode", ""}, expectErr: "three-way-merge-mode"},
-		{name: "env var", env: "client"},
-		{name: "invalid env var", env: "local", expectErr: "HELM_DIFF_THREE_WAY_MERGE_MODE"},
+		// The env var is only consulted when the run performs a three-way merge,
+		// so on its own it can neither take effect nor fail the command.
+		{name: "env var without three-way-merge", env: "client"},
+		{name: "invalid env var without three-way-merge", env: "local"},
 		{name: "flag wins over invalid env var", args: []string{"--three-way-merge-mode", "client"}, env: "local"},
 	}
 
@@ -413,3 +415,41 @@ func TestThreeWayMergeModeFlag(t *testing.T) {
 		})
 	}
 }
+
+// The env var is rejected only where it is actually used, so that a value left
+// over in the environment cannot fail an unrelated `helm diff upgrade`.
+func TestThreeWayMergeModeEnvVarOnlyAppliesToThreeWayMerge(t *testing.T) {
+	cases := []struct {
+		name      string
+		args      []string
+		env       string
+		expectErr bool
+	}{
+		{name: "--three-way-merge", args: []string{"--three-way-merge"}, env: "local", expectErr: true},
+		{name: "--take-ownership", args: []string{"--take-ownership"}, env: "local", expectErr: true},
+		{name: "neither", env: "local", expectErr: false},
+		{name: "valid value with --three-way-merge", args: []string{"--three-way-merge"}, env: "client", expectErr: false},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Setenv("HELM_DIFF_THREE_WAY_MERGE_MODE", tc.env)
+
+			chartDir := t.TempDir()
+			setupFakeHelm(t, "capture_args", "", chartDir+"/args", "")
+
+			cmd := newChartCommand()
+			cmd.SetArgs(append([]string{"my-release", chartDir}, tc.args...))
+
+			err := cmd.Execute()
+			mentionsEnvVar := err != nil && strings.Contains(err.Error(), "HELM_DIFF_THREE_WAY_MERGE_MODE")
+
+			// A run that gets past validation goes on to reach for the cluster,
+			// which is not available here, so only the env var complaint itself
+			// is meaningful - not whether the command as a whole succeeded.
+			if mentionsEnvVar != tc.expectErr {
+				t.Fatalf("expected the env var to be rejected=%v, got err=%v", tc.expectErr, err)
+			}
+		})
+	}
+}
diff --git a/manifest/generate.go b/manifest/generate.go
index 4f08017c..639ba179 100644
--- a/manifest/generate.go
+++ b/manifest/generate.go
@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"encoding/json"
 	"fmt"
+	"io"
 	"os"
 	"reflect"
 
@@ -118,18 +119,7 @@ func Generate(actionConfig *action.Configuration, originalManifest, targetManife
 		return nil
 	})
 
-	// The warning about the fallback to the local merge is only interesting
-	// once, no matter how many resources the release contains.
-	warned := false
-	warnClientSideMerge := func(cause error) {
-		if warned {
-			return
-		}
-		warned = true
-		fmt.Fprintf(os.Stderr, "Not allowed to dry-run the patch against the cluster (%v).\n"+
-			"Falling back to computing the three-way merge locally. The diff may deviate from the\n"+
-			"actual upgrade result because server-side defaulting and mutating webhooks are not applied.\n", cause)
-	}
+	fallback := &clientSideFallback{mode: options.mergeMode, warn: os.Stderr}
 
 	err = target.Visit(func(info *resource.Info, err error) error {
 		if err != nil {
@@ -174,7 +164,7 @@ func Generate(actionConfig *action.Configuration, originalManifest, targetManife
 		}
 
 		// `out` still holds the live object, which is what the patch applies to.
-		merged, err := applyPatch(helper, info, patch, out, options.mergeMode, warnClientSideMerge)
+		merged, err := applyPatch(helper, info, patch, out, fallback.currentMode(), fallback.patchDenied)
 		if err != nil {
 			return err
 		}
@@ -195,6 +185,36 @@ func Generate(actionConfig *action.Configuration, originalManifest, targetManife
 	return releaseManifest, installManifest, err
 }
 
+// clientSideFallback decides, for the whole run, whether a patch still goes to
+// the API server.
+//
+// A refused dry-run says something about the credentials rather than about the
+// resource, so once the server has turned one patch down the remaining
+// resources are merged locally as well. Retrying each of them would only collect
+// one denied request - and one audit log entry - per object in the release.
+type clientSideFallback struct {
+	mode ThreeWayMergeMode
+	warn io.Writer
+}
+
+// currentMode is how the next resource should be merged.
+func (f *clientSideFallback) currentMode() ThreeWayMergeMode {
+	return f.mode
+}
+
+// patchDenied records that the API server refused to dry-run a patch and moves
+// the rest of the run over to the local merge, reporting the switch once.
+func (f *clientSideFallback) patchDenied(cause error) {
+	if f.mode == ThreeWayMergeClient {
+		return
+	}
+	f.mode = ThreeWayMergeClient
+	fmt.Fprintf(f.warn, "Not allowed to dry-run the patch against the cluster (%v).\n"+
+		"Falling back to computing the three-way merge locally for the rest of this run. The diff\n"+
+		"may deviate from the actual upgrade result because server-side defaulting and mutating\n"+
+		"webhooks are not applied.\n", cause)
+}
+
 // resourcePatch is the patch computed for a single resource, together with
 // everything that is needed to apply it locally instead of on the API server.
 type resourcePatch struct {
@@ -455,13 +475,14 @@ func createPatch(originalObj, currentObj runtime.Object, target *resource.Info)
 	// Get a versioned object
 	versionedObject := kube.AsVersioned(target)
 
-	// Unstructured objects, such as CRDs, may not have an not registered error
-	// returned from ConvertToVersion. Anything that's unstructured should
-	// use the jsonpatch.CreateMergePatch. Strategic Merge Patch is not supported
-	// on objects like CRDs.
+	// Unstructured objects, such as CRDs, may not return a "not registered"
+	// error from ConvertToVersion. Anything that is unstructured should use
+	// jsonpatch.CreateMergePatch, because a strategic merge patch is not
+	// supported on objects like CRDs.
 	_, isUnstructured := versionedObject.(runtime.Unstructured)
 
-	// On newer K8s versions, CRDs aren't unstructured but has this dedicated type
+	// On newer Kubernetes versions CRDs are not unstructured but have this
+	// dedicated type.
 	_, isCRD := versionedObject.(*apiextv1.CustomResourceDefinition)
 
 	patch := &resourcePatch{originalData: oldData, modifiedData: newData}
diff --git a/manifest/generate_test.go b/manifest/generate_test.go
index e4537f11..f2ca722f 100644
--- a/manifest/generate_test.go
+++ b/manifest/generate_test.go
@@ -1,6 +1,7 @@
 package manifest
 
 import (
+	"bytes"
 	"encoding/json"
 	"fmt"
 	"testing"
@@ -625,3 +626,36 @@ rules:
 		assert.Empty(t, rules, "emptying a populated list is a real change and must be reported")
 	})
 }
+
+// Once the API server has refused one dry-run patch it will refuse the rest, so
+// the whole run switches to the local merge instead of collecting a denied
+// request, and an audit log entry, per resource.
+func TestClientSideFallbackAppliesToTheWholeRun(t *testing.T) {
+	forbidden := apierrors.NewForbidden(
+		schema.GroupResource{Group: "apps", Resource: "deployments"}, "nginx", assert.AnError)
+
+	var warnings bytes.Buffer
+	fallback := &clientSideFallback{mode: ThreeWayMergeAuto, warn: &warnings}
+	require.Equal(t, ThreeWayMergeAuto, fallback.currentMode())
+
+	fallback.patchDenied(forbidden)
+	assert.Equal(t, ThreeWayMergeClient, fallback.currentMode(),
+		"the rest of the run must not retry a patch the API server already refused")
+
+	firstWarning := warnings.String()
+	assert.Contains(t, firstWarning, "Falling back to computing the three-way merge locally")
+
+	fallback.patchDenied(forbidden)
+	assert.Equal(t, firstWarning, warnings.String(), "the fallback must be reported once per run")
+	assert.Equal(t, ThreeWayMergeClient, fallback.currentMode())
+}
+
+// The explicit modes are never overridden by the fallback.
+func TestClientSideFallbackLeavesExplicitModesAlone(t *testing.T) {
+	var warnings bytes.Buffer
+	fallback := &clientSideFallback{mode: ThreeWayMergeClient, warn: &warnings}
+
+	fallback.patchDenied(assert.AnError)
+	assert.Equal(t, ThreeWayMergeClient, fallback.currentMode())
+	assert.Empty(t, warnings.String(), "a run that never asks the server has nothing to report")
+}

From f0976bf1d6291f28ea1fde0c6c9d2d9bba9c5d25 Mon Sep 17 00:00:00 2001
From: Oreon Lothamer <oreon@metabase.com>
Date: Thu, 3 Sep 2026 10:51:21 -1000
Subject: [PATCH 4/7] fix: name the right object in a createPatch error message

The marshal of originalObj reported "serializing current configuration",
while the live object marshalled a few lines below reported "serializing
live configuration". Two different objects, and the first was named after
the second, which is misleading when a failure has to be diagnosed. The
three messages now say original, target and live.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---
 manifest/generate.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/manifest/generate.go b/manifest/generate.go
index 639ba179..0d806fed 100644
--- a/manifest/generate.go
+++ b/manifest/generate.go
@@ -459,7 +459,7 @@ func isPatchNotAllowed(err error) bool {
 func createPatch(originalObj, currentObj runtime.Object, target *resource.Info) (*resourcePatch, error) {
 	oldData, err := json.Marshal(originalObj)
 	if err != nil {
-		return nil, fmt.Errorf("serializing current configuration: %w", err)
+		return nil, fmt.Errorf("serializing original configuration: %w", err)
 	}
 	newData, err := json.Marshal(target.Object)
 	if err != nil {

From ffa112379fe3ed1c4e385f358a08dd658cc13bfe Mon Sep 17 00:00:00 2001
From: Oreon Lothamer <oreon@metabase.com>
Date: Thu, 3 Sep 2026 13:55:34 -1000
Subject: [PATCH 5/7] fix lint

---
 .golangci.yaml       |  2 ++
 manifest/generate.go | 12 +++++++-----
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/.golangci.yaml b/.golangci.yaml
index 70dd2b73..776b6935 100644
--- a/.golangci.yaml
+++ b/.golangci.yaml
@@ -57,8 +57,10 @@ linters:
             - github.com/spf13/pflag
             - golang.org/x/term
             - gopkg.in/yaml.v2
+            - github.com/stretchr/testify/assert
             - github.com/stretchr/testify/require
             - helm.sh/helm/v4
+            - k8s.io/api/apps/v1
             - k8s.io/api/core/v1
             - k8s.io/apiextensions-apiserver
             - k8s.io/apimachinery
diff --git a/manifest/generate.go b/manifest/generate.go
index 0d806fed..da6d3a12 100644
--- a/manifest/generate.go
+++ b/manifest/generate.go
@@ -54,7 +54,7 @@ type generateOptions struct {
 	mergeMode ThreeWayMergeMode
 }
 
-// GenerateOption customizes the behaviour of Generate.
+// GenerateOption customizes the behavior of Generate.
 type GenerateOption func(*generateOptions)
 
 // WithThreeWayMergeMode selects how the three-way merge patch is applied.
@@ -209,10 +209,12 @@ func (f *clientSideFallback) patchDenied(cause error) {
 		return
 	}
 	f.mode = ThreeWayMergeClient
-	fmt.Fprintf(f.warn, "Not allowed to dry-run the patch against the cluster (%v).\n"+
+	if _, err := fmt.Fprintf(f.warn, "Not allowed to dry-run the patch against the cluster (%v).\n"+
 		"Falling back to computing the three-way merge locally for the rest of this run. The diff\n"+
 		"may deviate from the actual upgrade result because server-side defaulting and mutating\n"+
-		"webhooks are not applied.\n", cause)
+		"webhooks are not applied.\n", cause); err != nil {
+		_, _ = fmt.Fprintf(os.Stderr, "failed writing fallback warning: %v\n", err)
+	}
 }
 
 // resourcePatch is the patch computed for a single resource, together with
@@ -270,7 +272,7 @@ func (p *resourcePatch) normalize(merged []byte) ([]byte, error) {
 	}
 
 	objType := reflect.TypeOf(p.versionedObject)
-	if objType.Kind() != reflect.Ptr {
+	if objType.Kind() != reflect.Pointer {
 		return merged, nil
 	}
 	typed, ok := reflect.New(objType.Elem()).Interface().(runtime.Object)
@@ -305,7 +307,7 @@ func (p *resourcePatch) normalize(merged []byte) ([]byte, error) {
 // manifest agree about it - neither mentions it, or both give it the same value,
 // `null` included. Nothing then asked for the live value to go, so its
 // disappearance is an artifact of the patch rather than a change to report. Once
-// the two manifests disagree the deletion is honoured, because that is a change
+// the two manifests disagree the deletion is honored, because that is a change
 // the chart really makes.
 //
 // Only fields missing from the merged object are restored, never values that it

From ca059ad71bf6b78ce07a71b75b76379521a39d26 Mon Sep 17 00:00:00 2001
From: Oreon Lothamer <oreon@metabase.com>
Date: Thu, 3 Sep 2026 14:12:47 -1000
Subject: [PATCH 6/7] fix: stop the local merge from hiding real changes

Four defects from review, all confirmed by reproduction before the fix.

restoreMissing copied a live value back whenever the two release
manifests agreed about a key, including when they agreed on an actual
value. normalize drops an explicit `hostNetwork: false` through
omitempty, so a chart pinning false against a cluster that drifted to
true had the drift restored and the correction vanished from the diff -
the opposite of what three-way merge is for. A value is now restored only
where neither manifest carries one, an absent key and a `null` counting
alike, which is all the unset `replicas:` case ever needed.

The restoration pass no longer runs behind a JSON merge patch. That patch
carries only what the chart changed between the two manifests, so it
never prunes a field the cluster populated and there is nothing to put
back. Custom resources are also stored as JSON rather than protobuf,
which keeps `null`, `[]` and `{}` apart, so the empty-value equivalence
was undoing a chart deliberately changing one into another.

auto no longer reads every 403 as a missing patch permission. An
admission webhook or a quota controller can refuse a request the
credentials are entitled to make, and that refusal is exactly what the
upgrade would hit, so falling back locally hid the outcome the diff
exists to predict. A SelfSubjectAccessReview now settles whether patching
is permitted before the refusal is treated as a permission problem; where
the review cannot be made the previous behaviour stands. The fallback is
already once per run, so this costs one extra request.

The README documents a second deviation from server mode: changing an
entry inside an atomic list also drops the server defaults elsewhere in
that entry, because positions in a list the chart rewrote cannot be
matched up safely.

k8s.io/api/authorization/v1 is added to the depguard allowlist, which
golangci-lint requires for the new import.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---
 .golangci.yaml            |   1 +
 README.md                 |   2 +-
 manifest/generate.go      | 106 +++++++++++++++++++++++++++----
 manifest/generate_test.go | 129 +++++++++++++++++++++++++++++++++++++-
 4 files changed, 222 insertions(+), 16 deletions(-)

diff --git a/.golangci.yaml b/.golangci.yaml
index 776b6935..03ef9b49 100644
--- a/.golangci.yaml
+++ b/.golangci.yaml
@@ -61,6 +61,7 @@ linters:
             - github.com/stretchr/testify/require
             - helm.sh/helm/v4
             - k8s.io/api/apps/v1
+            - k8s.io/api/authorization/v1
             - k8s.io/api/core/v1
             - k8s.io/apiextensions-apiserver
             - k8s.io/apimachinery
diff --git a/README.md b/README.md
index bffe7b40..80f57029 100644
--- a/README.md
+++ b/README.md
@@ -269,7 +269,7 @@ Notes:
 
 Because the defaulting functions are not part of client-go, `client` mode cannot reproduce them exactly. What it can do is leave the live value alone: a field is only reported as gone when the two release manifests disagree about it. That matters more than it sounds, because a chart that leaves a value unset usually renders the field as an explicit `null` — a bare `replicas:` — and a `null` in a manifest reaches the patch as a change rather than a deletion, wiping a value the API server had defaulted in even when the chart did not change at all.
 
-The remaining known deviation from `server` mode is the case where the manifests really do disagree and the field is one the API server defaults: a chart that stops pinning `replicas: 3` is reported as removing the field, where `server` mode shows it changing from `3` to the defaulted `1`. The change is reported either way, but `client` mode cannot name the value that replaces it.
+Two known deviations from `server` mode remain. Where the manifests really do disagree and the field is one the API server defaults, a chart that stops pinning `replicas: 3` is reported as removing the field, while `server` mode shows it changing from `3` to the defaulted `1` — the change is reported either way, but `client` mode cannot name the value that replaces it. And where a chart changes an entry inside an atomic list, the server defaults elsewhere in that entry are reported as removed: changing a NetworkPolicy port from `27017` to `27018` also drops the defaulted `protocol: TCP` from the diff, because positions in a list the chart rewrote can no longer be matched up safely.
 
 So a read-only account is enough for a three-way merge diff out of the box. Set `--three-way-merge-mode=client` (or `HELM_DIFF_THREE_WAY_MERGE_MODE=client`) to skip the rejected dry-run request entirely, and `--three-way-merge-mode=server` to make a missing `patch` permission a hard error instead of silently degrading the diff.
 
diff --git a/manifest/generate.go b/manifest/generate.go
index da6d3a12..859dc412 100644
--- a/manifest/generate.go
+++ b/manifest/generate.go
@@ -2,6 +2,7 @@ package manifest
 
 import (
 	"bytes"
+	"context"
 	"encoding/json"
 	"fmt"
 	"io"
@@ -12,8 +13,10 @@ import (
 	jsoniter "github.com/json-iterator/go"
 	"helm.sh/helm/v4/pkg/action"
 	"helm.sh/helm/v4/pkg/kube"
+	authorizationv1 "k8s.io/api/authorization/v1"
 	apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/strategicpatch"
@@ -119,7 +122,11 @@ func Generate(actionConfig *action.Configuration, originalManifest, targetManife
 		return nil
 	})
 
-	fallback := &clientSideFallback{mode: options.mergeMode, warn: os.Stderr}
+	fallback := &clientSideFallback{
+		mode:     options.mergeMode,
+		warn:     os.Stderr,
+		canPatch: patchPermissionCheck(actionConfig),
+	}
 
 	err = target.Visit(func(info *resource.Info, err error) error {
 		if err != nil {
@@ -195,6 +202,10 @@ func Generate(actionConfig *action.Configuration, originalManifest, targetManife
 type clientSideFallback struct {
 	mode ThreeWayMergeMode
 	warn io.Writer
+	// canPatch reports whether the credentials may patch a resource at all. It
+	// is nil when the question cannot be put, and a refusal is then taken at
+	// face value.
+	canPatch func(*resource.Info) (bool, error)
 }
 
 // currentMode is how the next resource should be merged.
@@ -202,12 +213,26 @@ func (f *clientSideFallback) currentMode() ThreeWayMergeMode {
 	return f.mode
 }
 
-// patchDenied records that the API server refused to dry-run a patch and moves
-// the rest of the run over to the local merge, reporting the switch once.
-func (f *clientSideFallback) patchDenied(cause error) {
+// patchDenied records that the API server refused to dry-run a patch. It reports
+// whether the run may carry on with the local merge; false means the refusal was
+// not about permissions, so the caller has to surface it instead.
+func (f *clientSideFallback) patchDenied(info *resource.Info, cause error) bool {
 	if f.mode == ThreeWayMergeClient {
-		return
+		return true
 	}
+
+	// A 403 need not come from RBAC. An admission webhook or a quota controller
+	// can turn down a request the credentials are entitled to make, and that
+	// refusal is precisely what the upgrade would run into as well - working
+	// around it locally would hide the outcome the diff exists to predict. Ask
+	// whether patching is permitted at all before reading a refusal as a
+	// permission problem.
+	if f.canPatch != nil {
+		if allowed, err := f.canPatch(info); err == nil && allowed {
+			return false
+		}
+	}
+
 	f.mode = ThreeWayMergeClient
 	if _, err := fmt.Fprintf(f.warn, "Not allowed to dry-run the patch against the cluster (%v).\n"+
 		"Falling back to computing the three-way merge locally for the rest of this run. The diff\n"+
@@ -215,6 +240,47 @@ func (f *clientSideFallback) patchDenied(cause error) {
 		"webhooks are not applied.\n", cause); err != nil {
 		_, _ = fmt.Fprintf(os.Stderr, "failed writing fallback warning: %v\n", err)
 	}
+
+	return true
+}
+
+// patchPermissionCheck asks the API server whether the credentials may patch a
+// resource. It returns nil when the question cannot be put - helm hands over a
+// *kube.Client in practice, but the interface does not promise one - and a
+// refusal is then assumed to be about permissions, which is how the fallback
+// behaved before the check existed.
+func patchPermissionCheck(actionConfig *action.Configuration) func(*resource.Info) (bool, error) {
+	client, ok := actionConfig.KubeClient.(*kube.Client)
+	if !ok || client.Factory == nil {
+		return nil
+	}
+
+	clientset, err := client.Factory.KubernetesClientSet()
+	if err != nil {
+		return nil
+	}
+
+	return func(info *resource.Info) (bool, error) {
+		review := &authorizationv1.SelfSubjectAccessReview{
+			Spec: authorizationv1.SelfSubjectAccessReviewSpec{
+				ResourceAttributes: &authorizationv1.ResourceAttributes{
+					Namespace: info.Namespace,
+					Name:      info.Name,
+					Verb:      "patch",
+					Group:     info.Mapping.Resource.Group,
+					Resource:  info.Mapping.Resource.Resource,
+				},
+			},
+		}
+
+		result, err := clientset.AuthorizationV1().SelfSubjectAccessReviews().
+			Create(context.Background(), review, metav1.CreateOptions{})
+		if err != nil {
+			return false, err
+		}
+
+		return result.Status.Allowed, nil
+	}
 }
 
 // resourcePatch is the patch computed for a single resource, together with
@@ -258,6 +324,16 @@ func (p *resourcePatch) apply(liveData []byte) ([]byte, error) {
 		return nil, err
 	}
 
+	// A JSON merge patch carries only what the chart changed between the two
+	// release manifests, so it never prunes a field the cluster populated and
+	// there is nothing to put back. Custom resources are also stored as JSON
+	// rather than protobuf, which keeps `null`, `[]` and `{}` apart, so the
+	// restoration pass would misread a chart changing one into another as noise
+	// and undo it.
+	if p.patchType == types.MergePatchType {
+		return merged, nil
+	}
+
 	return p.restoreServerPopulatedFields(merged, liveData)
 }
 
@@ -360,7 +436,7 @@ func isEmpty(v interface{}) bool {
 // restoreMissing walks merged and live in parallel and copies over the parts of
 // live that merged lost without either release manifest asking for it. It
 // returns the updated merged value and never overwrites a value merged already
-// has.
+// has, so a field the chart does specify keeps whatever the patch made of it.
 func restoreMissing(merged, live, original, modified interface{}) interface{} {
 	switch live := live.(type) {
 	case map[string]interface{}:
@@ -383,10 +459,14 @@ func restoreMissing(merged, live, original, modified interface{}) interface{} {
 				continue
 			}
 			if !inMerged {
-				// A key that is absent and a key that holds `null` both read as
-				// nil here, which is what makes an unset `replicas:` in the
-				// chart compare equal to the field the manifests never mention.
-				if reflect.DeepEqual(originalMap[key], modifiedMap[key]) {
+				// Only where neither manifest carries a value for the key: an
+				// absent key and a `null` both read as nil here, which is what
+				// lets an unset `replicas:` count as "the chart says nothing".
+				// Agreeing on an actual value is not enough - two manifests that
+				// both say `hostNetwork: false` are asking for false, and
+				// copying a drifted `true` back would hide the correction the
+				// upgrade makes.
+				if originalMap[key] == nil && modifiedMap[key] == nil {
 					mergedMap[key] = liveValue
 				}
 				continue
@@ -424,7 +504,7 @@ func restoreMissing(merged, live, original, modified interface{}) interface{} {
 // dry-run the patch or by merging locally, depending on mode. warn is called at
 // most once, when mode is ThreeWayMergeAuto and the API server refused the
 // dry-run.
-func applyPatch(helper *resource.Helper, info *resource.Info, patch *resourcePatch, liveData []byte, mode ThreeWayMergeMode, warn func(cause error)) ([]byte, error) {
+func applyPatch(helper *resource.Helper, info *resource.Info, patch *resourcePatch, liveData []byte, mode ThreeWayMergeMode, patchDenied func(*resource.Info, error) bool) ([]byte, error) {
 	kind := info.Mapping.GroupVersionKind.Kind
 
 	if mode != ThreeWayMergeClient {
@@ -438,7 +518,9 @@ func applyPatch(helper *resource.Helper, info *resource.Info, patch *resourcePat
 			}
 			return out, nil
 		case mode == ThreeWayMergeAuto && isPatchNotAllowed(err):
-			warn(err)
+			if !patchDenied(info, err) {
+				return nil, fmt.Errorf("cannot patch %q with kind %s: %w", info.Name, kind, err)
+			}
 		default:
 			return nil, fmt.Errorf("cannot patch %q with kind %s: %w", info.Name, kind, err)
 		}
diff --git a/manifest/generate_test.go b/manifest/generate_test.go
index f2ca722f..3e19233b 100644
--- a/manifest/generate_test.go
+++ b/manifest/generate_test.go
@@ -634,18 +634,20 @@ func TestClientSideFallbackAppliesToTheWholeRun(t *testing.T) {
 	forbidden := apierrors.NewForbidden(
 		schema.GroupResource{Group: "apps", Resource: "deployments"}, "nginx", assert.AnError)
 
+	info := infoFor(t, deployment(1, "nginx:1.0", nil), appsv1.SchemeGroupVersion.WithKind("Deployment"))
+
 	var warnings bytes.Buffer
 	fallback := &clientSideFallback{mode: ThreeWayMergeAuto, warn: &warnings}
 	require.Equal(t, ThreeWayMergeAuto, fallback.currentMode())
 
-	fallback.patchDenied(forbidden)
+	assert.True(t, fallback.patchDenied(info, forbidden))
 	assert.Equal(t, ThreeWayMergeClient, fallback.currentMode(),
 		"the rest of the run must not retry a patch the API server already refused")
 
 	firstWarning := warnings.String()
 	assert.Contains(t, firstWarning, "Falling back to computing the three-way merge locally")
 
-	fallback.patchDenied(forbidden)
+	assert.True(t, fallback.patchDenied(info, forbidden))
 	assert.Equal(t, firstWarning, warnings.String(), "the fallback must be reported once per run")
 	assert.Equal(t, ThreeWayMergeClient, fallback.currentMode())
 }
@@ -655,7 +657,128 @@ func TestClientSideFallbackLeavesExplicitModesAlone(t *testing.T) {
 	var warnings bytes.Buffer
 	fallback := &clientSideFallback{mode: ThreeWayMergeClient, warn: &warnings}
 
-	fallback.patchDenied(assert.AnError)
+	assert.True(t, fallback.patchDenied(infoFor(t, deployment(1, "nginx:1.0", nil), appsv1.SchemeGroupVersion.WithKind("Deployment")), assert.AnError))
 	assert.Equal(t, ThreeWayMergeClient, fallback.currentMode())
 	assert.Empty(t, warnings.String(), "a run that never asks the server has nothing to report")
 }
+
+// Both manifests asking for the same value is not the same as neither asking for
+// anything. normalize drops an explicit `hostNetwork: false` through omitempty,
+// and restoring the live value there would undo the very correction the upgrade
+// makes.
+func TestLocalMerge_KeepsReportingDriftOnChartOwnedZeroValues(t *testing.T) {
+	pod := func(hostNetwork string) string {
+		return fmt.Sprintf(`
+apiVersion: apps/v1
+kind: Deployment
+metadata: {name: nginx, namespace: default}
+spec:
+  selector: {matchLabels: {app: nginx}}
+  template:
+    metadata: {labels: {app: nginx}}
+    spec:
+      hostNetwork: %s
+      containers: [{name: nginx, image: nginx:1.0}]
+`, hostNetwork)
+	}
+
+	// The chart pins false on both sides; someone flipped the cluster to true.
+	got := applyLocally(t, pod("false"), pod("true"), pod("false"))
+
+	value, found := nested(t, got, "spec", "template", "spec", "hostNetwork")
+	assert.False(t, found && value == true,
+		"a chart-owned value must not be restored from the cluster, got hostNetwork=%v", value)
+}
+
+// A custom resource is stored as JSON, which keeps null, [] and {} apart, and a
+// JSON merge patch already leaves untouched live fields alone. Neither the
+// empty-value equivalence nor the restoration pass may run over one.
+func TestLocalMerge_LeavesCustomResourcesToTheMergePatch(t *testing.T) {
+	widget := func(items string) string {
+		return fmt.Sprintf(`
+apiVersion: example.com/v1
+kind: Widget
+metadata: {name: w, namespace: default}
+spec:
+  items: %s
+  size: large
+`, items)
+	}
+
+	t.Run("null to empty list is a change the chart asked for", func(t *testing.T) {
+		got := applyLocally(t, widget("null"), widget("null"), widget("[]"))
+		items, _ := nested(t, got, "spec", "items")
+		assert.Equal(t, []interface{}{}, items, "the chart changed null to [], which must survive")
+	})
+
+	t.Run("empty list to null is a change the chart asked for", func(t *testing.T) {
+		got := applyLocally(t, widget("[]"), widget("[]"), widget("null"))
+		items, found := nested(t, got, "spec", "items")
+		assert.Nil(t, items, "the chart changed [] to null, which must survive (found=%v)", found)
+	})
+
+	t.Run("fields the chart never mentions are left alone by the merge patch", func(t *testing.T) {
+		live := `
+apiVersion: example.com/v1
+kind: Widget
+metadata: {name: w, namespace: default}
+spec:
+  items: null
+  size: large
+  status: {observed: 3}
+`
+		got := applyLocally(t, widget("null"), live, widget("null"))
+		observed, found := nested(t, got, "spec", "status", "observed")
+		require.True(t, found, "a JSON merge patch must not prune a field it never mentions")
+		assert.EqualValues(t, 3, observed)
+	})
+}
+
+// A Forbidden that RBAC did not produce - an admission webhook or a quota
+// controller refusing an authorized request - is the upgrade's own outcome and
+// must not be worked around locally.
+func TestClientSideFallbackDistinguishesAdmissionFromPermissions(t *testing.T) {
+	info := infoFor(t, deployment(1, "nginx:1.0", nil), appsv1.SchemeGroupVersion.WithKind("Deployment"))
+	info.Mapping.Resource = appsv1.SchemeGroupVersion.WithResource("deployments")
+
+	denial := apierrors.NewForbidden(
+		schema.GroupResource{Group: "apps", Resource: "deployments"}, "nginx", assert.AnError)
+
+	t.Run("patching is allowed, so the refusal came from elsewhere", func(t *testing.T) {
+		var warnings bytes.Buffer
+		fallback := &clientSideFallback{
+			mode:     ThreeWayMergeAuto,
+			warn:     &warnings,
+			canPatch: func(*resource.Info) (bool, error) { return true, nil },
+		}
+
+		assert.False(t, fallback.patchDenied(info, denial), "the caller must surface an admission refusal")
+		assert.Equal(t, ThreeWayMergeAuto, fallback.currentMode(), "the run must not degrade")
+		assert.Empty(t, warnings.String())
+	})
+
+	t.Run("patching is not allowed, so it is a permission problem", func(t *testing.T) {
+		var warnings bytes.Buffer
+		fallback := &clientSideFallback{
+			mode:     ThreeWayMergeAuto,
+			warn:     &warnings,
+			canPatch: func(*resource.Info) (bool, error) { return false, nil },
+		}
+
+		assert.True(t, fallback.patchDenied(info, denial))
+		assert.Equal(t, ThreeWayMergeClient, fallback.currentMode())
+		assert.Contains(t, warnings.String(), "Falling back")
+	})
+
+	t.Run("the check itself fails, so the refusal is taken at face value", func(t *testing.T) {
+		var warnings bytes.Buffer
+		fallback := &clientSideFallback{
+			mode:     ThreeWayMergeAuto,
+			warn:     &warnings,
+			canPatch: func(*resource.Info) (bool, error) { return false, assert.AnError },
+		}
+
+		assert.True(t, fallback.patchDenied(info, denial))
+		assert.Equal(t, ThreeWayMergeClient, fallback.currentMode())
+	})
+}

From d8d73ff719f134ee746bd9ed52b1745131e3a376 Mon Sep 17 00:00:00 2001
From: Oreon Lothamer <oreon@metabase.com>
Date: Thu, 3 Sep 2026 16:04:55 -1000
Subject: [PATCH 7/7] fix: correct the 405 fallback, the RBAC role and the env
 var examples

A MethodNotAllowed no longer aborts. The authorization review added in
the previous commit ran for every refusal isPatchNotAllowed accepts, so a
405 against credentials that may patch was read as "not a permission
problem" and surfaced. A 405 says the API server takes no patch for the
resource whoever is asking, so being entitled to send one changes
nothing; only a 403 is ambiguous enough to be worth the review.

The least-privilege role could not read the release. Without an explicit
--revision helm lists the storage backend to find the newest one, so
`get` on Secrets and ConfigMaps is not enough - the release storage needs
`list` as well, while the kinds a chart renders still only need `get`.

The HELM_DIFF_THREE_WAY_MERGE_MODE examples did not enable the three-way
merge, so they demonstrated an ordinary two-way diff. Now that the
variable is deliberately ignored unless the merge is on, they have to
pass --three-way-merge to show anything. The README copy of the example
block is maintained by hand rather than by scripts/gen-readme.sh, which
only regenerates the flag tables, so it had drifted from the binary.

The README also documents that client mode can be quieter than the truth
in one place: the restoration copies back every live-only field a
wholesale replacement removed, and nothing local can tell a value the API
server defaulted from one somebody set by hand. A NetworkPolicy port with
a hand-added endPort keeps it, where the upgrade would drop it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---
 README.md                 | 22 +++++++++++++++++-----
 cmd/upgrade.go            |  3 ++-
 manifest/generate.go      |  6 +++++-
 manifest/generate_test.go | 25 +++++++++++++++++++++++++
 4 files changed, 49 insertions(+), 7 deletions(-)

diff --git a/README.md b/README.md
index 80f57029..b6a139f1 100644
--- a/README.md
+++ b/README.md
@@ -269,11 +269,17 @@ Notes:
 
 Because the defaulting functions are not part of client-go, `client` mode cannot reproduce them exactly. What it can do is leave the live value alone: a field is only reported as gone when the two release manifests disagree about it. That matters more than it sounds, because a chart that leaves a value unset usually renders the field as an explicit `null` — a bare `replicas:` — and a `null` in a manifest reaches the patch as a change rather than a deletion, wiping a value the API server had defaulted in even when the chart did not change at all.
 
-Two known deviations from `server` mode remain. Where the manifests really do disagree and the field is one the API server defaults, a chart that stops pinning `replicas: 3` is reported as removing the field, while `server` mode shows it changing from `3` to the defaulted `1` — the change is reported either way, but `client` mode cannot name the value that replaces it. And where a chart changes an entry inside an atomic list, the server defaults elsewhere in that entry are reported as removed: changing a NetworkPolicy port from `27017` to `27018` also drops the defaulted `protocol: TCP` from the diff, because positions in a list the chart rewrote can no longer be matched up safely.
+Three known deviations from `server` mode remain.
 
-So a read-only account is enough for a three-way merge diff out of the box. Set `--three-way-merge-mode=client` (or `HELM_DIFF_THREE_WAY_MERGE_MODE=client`) to skip the rejected dry-run request entirely, and `--three-way-merge-mode=server` to make a missing `patch` permission a hard error instead of silently degrading the diff.
+**A field the chart stops pinning is reported as removed** rather than as changing to its default. `server` mode shows `replicas: 3` becoming the defaulted `1`; `client` mode reports the field going away, because it cannot name the value that replaces it. The change is reported either way.
 
-`client` mode needs `get` on every kind the chart renders, plus `get` on the Secret or ConfigMap holding the release. The role below is the blunt version — read access to everything, which is convenient but broader than a diff requires:
+**Drift can be hidden inside a `retainKeys` struct or an atomic list.** The restoration described above copies back every live-only field that the patch replaced wholesale, and locally there is no way to tell a value the API server defaulted from one somebody set by hand — both are simply fields neither manifest mentions. If a NetworkPolicy port carries a hand-added `endPort`, the upgrade removes it and `server` mode says so, while `client` mode restores it along with the defaulted `protocol: TCP` and reports nothing. This is the one case where `client` mode can be quieter than the truth; everywhere else it errs towards reporting a change that does not happen. Use `--three-way-merge-mode=server` where that matters.
+
+**Server defaults are dropped from an atomic-list entry the chart changes.** Changing a NetworkPolicy port from `27017` to `27018` also drops the defaulted `protocol: TCP` from the diff, because positions in a list the chart rewrote can no longer be matched up safely.
+
+So a read-only account is enough for a three-way merge diff out of the box. Set `--three-way-merge-mode=client` (or `HELM_DIFF_THREE_WAY_MERGE_MODE=client`, which is only read once the three-way merge is enabled) to skip the rejected dry-run request entirely, and `--three-way-merge-mode=server` to make a missing `patch` permission a hard error instead of silently degrading the diff.
+
+`client` mode needs `get` on every kind the chart renders, plus `get` **and `list`** on the Secret or ConfigMap holding the release — without an explicit `--revision`, Helm lists the storage backend to find the newest one. The role below is the blunt version, read access to everything, which is convenient but broader than a diff requires:
 
 ```yaml
 apiVersion: rbac.authorization.k8s.io/v1
@@ -294,8 +300,13 @@ kind: Role
 metadata:
   name: helm-diff
 rules:
+  # Release storage: `list` is what lets Helm find the newest revision.
+  - apiGroups: [""]
+    resources: ["configmaps", "secrets"]
+    verbs: ["get", "list"]
+  # Everything else the chart renders needs `get` only.
   - apiGroups: [""]
-    resources: ["configmaps", "secrets", "services", "serviceaccounts"]
+    resources: ["services", "serviceaccounts"]
     verbs: ["get"]
   - apiGroups: ["apps"]
     resources: ["deployments", "statefulsets", "daemonsets"]
@@ -399,9 +410,10 @@ Examples:
 
   # Set HELM_DIFF_THREE_WAY_MERGE_MODE=client to compute the three-way merge
   # locally, so that no permission to patch the cluster resources is needed.
+  # It is only read once the three-way merge is on, hence the flag below.
   # This is equivalent to specifying the --three-way-merge-mode flag.
   # Read the flag usage below for more information on --three-way-merge-mode.
-  HELM_DIFF_THREE_WAY_MERGE_MODE=client helm diff upgrade my-release datadog/datadog
+  HELM_DIFF_THREE_WAY_MERGE_MODE=client helm diff upgrade my-release datadog/datadog --three-way-merge
 
   # Set HELM_DIFF_NORMALIZE_MANIFESTS=true to
   # normalize the yaml file content when using helm diff.
diff --git a/cmd/upgrade.go b/cmd/upgrade.go
index 79250897..7e824937 100644
--- a/cmd/upgrade.go
+++ b/cmd/upgrade.go
@@ -165,9 +165,10 @@ func newChartCommand() *cobra.Command {
 			"",
 			"  # Set HELM_DIFF_THREE_WAY_MERGE_MODE=client to compute the three-way merge",
 			"  # locally, so that no permission to patch the cluster resources is needed.",
+			"  # It is only read once the three-way merge is on, hence the flag below.",
 			"  # This is equivalent to specifying the --three-way-merge-mode flag.",
 			"  # Read the flag usage below for more information on --three-way-merge-mode.",
-			"  HELM_DIFF_THREE_WAY_MERGE_MODE=client helm diff upgrade my-release datadog/datadog",
+			"  HELM_DIFF_THREE_WAY_MERGE_MODE=client helm diff upgrade my-release datadog/datadog --three-way-merge",
 			"",
 			"  # Set HELM_DIFF_NORMALIZE_MANIFESTS=true to",
 			"  # normalize the yaml file content when using helm diff.",
diff --git a/manifest/generate.go b/manifest/generate.go
index 859dc412..95425fcf 100644
--- a/manifest/generate.go
+++ b/manifest/generate.go
@@ -227,7 +227,11 @@ func (f *clientSideFallback) patchDenied(info *resource.Info, cause error) bool
 	// around it locally would hide the outcome the diff exists to predict. Ask
 	// whether patching is permitted at all before reading a refusal as a
 	// permission problem.
-	if f.canPatch != nil {
+	//
+	// Only a 403 is ambiguous that way. A 405 says the API server takes no patch
+	// for this resource whoever is asking, so being entitled to send one changes
+	// nothing and the local merge is the only way left.
+	if apierrors.IsForbidden(cause) && f.canPatch != nil {
 		if allowed, err := f.canPatch(info); err == nil && allowed {
 			return false
 		}
diff --git a/manifest/generate_test.go b/manifest/generate_test.go
index 3e19233b..e4fef573 100644
--- a/manifest/generate_test.go
+++ b/manifest/generate_test.go
@@ -782,3 +782,28 @@ func TestClientSideFallbackDistinguishesAdmissionFromPermissions(t *testing.T) {
 		assert.Equal(t, ThreeWayMergeClient, fallback.currentMode())
 	})
 }
+
+// A 405 says the API server takes no patch for this resource whoever is asking,
+// so being entitled to send one changes nothing: the run still has to fall back.
+// Only a 403 is ambiguous enough to be worth an authorization review.
+func TestClientSideFallbackAlwaysFallsBackOnMethodNotAllowed(t *testing.T) {
+	info := infoFor(t, deployment(1, "nginx:1.0", nil), appsv1.SchemeGroupVersion.WithKind("Deployment"))
+	notSupported := apierrors.NewMethodNotSupported(
+		schema.GroupResource{Group: "apps", Resource: "deployments"}, "patch")
+
+	reviewed := false
+	var warnings bytes.Buffer
+	fallback := &clientSideFallback{
+		mode: ThreeWayMergeAuto,
+		warn: &warnings,
+		canPatch: func(*resource.Info) (bool, error) {
+			reviewed = true
+			return true, nil
+		},
+	}
+
+	assert.True(t, fallback.patchDenied(info, notSupported),
+		"a 405 must fall back even where the credentials may patch")
+	assert.Equal(t, ThreeWayMergeClient, fallback.currentMode())
+	assert.False(t, reviewed, "a 405 is unambiguous, so it is not worth an authorization review")
+}