feat: make --three-way-merge work without patch permissions

[oreonl]

What

--three-way-merge currently requires the patch permission on every diffed resource. It computes the merge patch locally but asks the API server to apply it as a dry-run:

helper.ServerDryRun=truetargetObj, err:=helper.Patch(info.Namespace, info.Name, patchType, patch, nil)

A read-only account gets cannot patch "x" with kind Deployment: ... is forbidden, so the feature is unavailable to exactly the credentials a "show me what this upgrade would do" tool tends to run under.

This applies the patch 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.

Interface

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

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

auto means a read-only account works out of the box; it only degrades when the accurate path is unavailable anyway, and says so on stderr.

manifest.Generate takes variadic options rather than a new parameter, so existing callers (helmfile vendors this package) keep compiling.

Matching what the server returns

A naive local merge produces a diff full of noise, because the API server does more than apply the patch. scheme.Scheme.Default() is a no-op in client-go — the SetDefaults_* functions live in k8s.io/kubernetes and are not importable — so the defaults cannot simply be recomputed. Two post-processing steps recover most of the gap:

1. Round-trip through the 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: [], supplementalGroups: [] — which otherwise appear as additions the upgrade does not make. (Helm's kube.Client.Build returns unstructured objects, so these survive from the chart YAML verbatim.)

2. Copy a field back from the live object when the old and the new release manifest agree about it. Values disappear from the merged object two ways:

• the patch replaces retainKeys structs (spec.strategy) and atomic lists (ports, volumeClaimTemplates) as a whole, dropping what the server had defaulted into them;
• a manifest that renders an unset value as an explicit null — a bare replicas:, which is what a chart writes for a value it leaves unset — puts the key in both manifests, so it reaches the patch as a change rather than a deletion. That deletes a defaulted value even when the chart did not change at all.

The server re-defaults both cases immediately after applying the patch. Locally, a field is restored only when nothing in either manifest asked for it to go. Where the two manifests disagree the deletion is honoured, because that is a change the chart really makes — restoring there would hide real removals (a dropped nodeSelector entry looks identical at the JSON level, and the server does not default that back). Values already present in the merged object are never overwritten, so drift between the cluster and the chart is still reported and still corrected.

Known deviation from server mode

When the manifests genuinely disagree about a field the API server defaults, client mode reports it as removed where server mode shows it changing to the default — a chart that stops pinning replicas: 3 shows the field going away rather than 3 → 1. The change is reported either way; client mode just cannot name the value that replaces it. Mutating webhooks and validation are also not applied. All of this is documented in the new README section, along with the minimal RBAC role.

Tests

manifest/generate_test.go is new. Eight TestLocalMerge_* cases are built from real manifests (unstructured, as kube.Client.Build supplies them; the live side written as the API server returns it) and cover each symptom above, plus:

• a chart that genuinely stops setting a field — still reported as removed;
• a hand-scaled replicas and hand-edited image in the cluster — still reset to the chart values, i.e. drift detection intact.

I checked the tests are not vacuous by reverting each fix independently; each fails without the step it covers. cmd/upgrade_test.go adds flag/env validation cases.

go test ./..., go vet and gofmt pass, and the README flag tables are regenerated with make readme.

Testing done

Built with make install/helm and run against a real cluster with an account lacking patch, on a release of ~15 resources (LibreChat and its subcharts). Before the two post-processing steps the client-side output differed from --three-way-merge-mode=server on defaulted fields; the cases in the tests are taken from that output.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

auto mode does not currently “fall back per run” (it keeps attempting forbidden PATCHes) and the docs/RBAC wording plus env-var handling need small corrections to match intended behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds a new three-way-merge “mode” capability so helm diff upgrade --three-way-merge can work with read-only Kubernetes credentials by falling back (or opting in) to a client-side patch application that approximates the API server’s results.

Changes:

• Introduces --three-way-merge-mode (auto/server/client) and env HELM_DIFF_THREE_WAY_MERGE_MODE, wiring it through cmd/upgrade into manifest.Generate(...) via variadic options.
• Implements client-side patch application for three-way-merge, including normalization (round-trip through Go types) and a restoration pass for server-populated/defaulted fields.
• Adds focused unit tests for local merge behavior and CLI flag/env validation; updates README and Copilot instructions.

File summaries

File	Description
README.md	Documents --three-way-merge-mode, behavior tradeoffs, and RBAC guidance.
manifest/generate.go	Adds merge-mode options and client-side patch application with post-processing to reduce diff noise.
manifest/generate_test.go	New tests covering strategic/merge patch local application and default-restoration behaviors.
cmd/upgrade.go	Adds flag/env plumbing and validation; passes merge-mode into manifest generation.
cmd/upgrade_test.go	Adds validation coverage for the new flag/env behavior.
.github/copilot-instructions.md	Documents the new environment variable.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

The new validation error messages format the allowed-mode slice with %q, which will render as %!q([]string=...) and should be corrected before merge.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

cmd/upgrade.go:199

• The format string uses %q for manifest.ValidThreeWayMergeModes (a []string), which will render as %!q([]string=...) and produce an unhelpful validation error. Use %v (or join the slice) when printing the allowed values.

This issue also appears on line 226 of the same file.

cmd/upgrade.go:228

• The env-var validation error formats manifest.ValidThreeWayMergeModes (a []string) with %q, which will print as %!q([]string=...). Use %v (or join the slice) so users see the allowed values clearly.

 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)
}

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[oreonl]

Checked this one and I don't think it holds — %q on a []string isn't malformed. fmt applies a verb recursively to the elements of a compound operand (docs: "For compound objects, the elements are printed using these rules, recursively... array, slice: [elem0 elem1 ...]"). %!q(...) only appears when the verb is invalid for the type, e.g. %q on a struct.

Actual output from the current branch, for both lines flagged:

$ helm diff upgrade rel ./chart --three-way-merge-mode local
Error: flag "three-way-merge-mode" must be one of ["auto" "server" "client"], but got "local"
$ HELM_DIFF_THREE_WAY_MERGE_MODE=local helm diff upgrade rel ./chart --three-way-merge
Error: env var "HELM_DIFF_THREE_WAY_MERGE_MODE" must be one of ["auto" "server" "client"], but got "local"

%v would print [auto server client], dropping the quotes that delimit the values — and the adjacent --server-side check quotes each one:

Error: flag "server-side" must be "true", "false" or "auto", but got "yes"

So I've left these as they are.

[Copilot]

🔵 Needs a closer look

It changes core diff computation behavior with non-trivial Kubernetes patch/defaulting semantics, so it warrants final human review despite good test coverage.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It changes core three-way-merge behavior with substantial new merge/restoration logic whose correctness and compatibility impact are difficult to fully validate via automated review alone.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[yxxhero]

@oreonl PTAL

[Copilot]

🟡 Changes recommended

Local restoration can hide real drift and CRD changes, while automatic fallback can mask admission failures.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 4
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Fallback handling and field restoration have correctness gaps, and several usage and RBAC examples are inaccurate.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (5)

Previously missed (3) — in code that hasn't changed since the last review.

README.md:276

• The release-storage requirement is understated here: the default revision is resolved by helm get manifest without --revision, which lists the Helm storage backend to find the newest release. A role with only get on Secrets/ConfigMaps therefore fails for the normal invocation; document list as required as well.

This issue also appears on line 297 of the same file.
README.md:404

• This generated example does not enable three-way merge. Since the mode environment variable is intentionally ignored unless three-way merge is already active, the command performs a normal two-way diff and does not demonstrate client-side merging.
  cmd/upgrade.go:170
• This example does not enable three-way merge. The command only reads this environment variable when HELM_DIFF_THREE_WAY_MERGE=true (or --three-way-merge/--take-ownership) is already active, so as written it performs the ordinary two-way diff rather than a local merge.

manifest/generate.go:233

• MethodNotAllowed never falls back when the authorization review says patching is allowed: isPatchNotAllowed routes it here, then this check returns false and aborts. That contradicts the new auto contract, which explicitly falls back for MethodNotAllowed. Restrict the authorization disambiguation to Forbidden; a 405 indicates the server path is unavailable regardless of RBAC.

	if f.canPatch != nil {
if allowed, err := f.canPatch(info); err == nil && allowed {
return false
}

README.md:299

• The least-privilege example cannot read the latest Helm release with the default revision. This code invokes helm get manifest without --revision (cmd/helm.go:139-158), so Helm must list the active Secret/ConfigMap storage backend to find the newest revision; granting only get causes the diff to fail before resource reads. Grant list to release storage while keeping workload kinds at get.

 - apiGroups: [""]
resources: ["configmaps", "secrets", "services", "serviceaccounts"]
verbs: ["get"]

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Balanced

[oreonl]

All five points from this review are addressed in d8d73ff. The four suppressed ones have no threads, so answering them here.

MethodNotAllowed aborting instead of falling back (manifest/generate.go:233) — correct, and a regression from the SelfSubjectAccessReview I added in ca059ad: the review ran for every refusal isPatchNotAllowed accepts, so a 405 against credentials that may patch was surfaced rather than falling back. Reproduced (patchDenied(405, canPatch=true) returned false) and fixed by restricting the disambiguation to Forbidden — a 405 says the API server takes no patch for that resource whoever is asking, so entitlement changes nothing. Covered by TestClientSideFallbackAlwaysFallsBackOnMethodNotAllowed.

Release storage needs list (README.md:276, :297, :299) — confirmed in the code: helmGetArgs omits --revision when the revision is 0 (cmd/helm.go:141-145), and helm's Secrets driver Query does a List to find the newest revision, so get alone fails before any resource is read. The least-privilege example now grants get, list on configmaps/secrets for the release storage and keeps the rendered kinds at get, with the reason spelled out in the surrounding prose.

The env var examples did not enable the three-way merge (README.md:404, cmd/upgrade.go:170) — right, and self-inflicted: making HELM_DIFF_THREE_WAY_MERGE_MODE deliberately ignored unless the merge is on invalidated my own examples. Both now pass --three-way-merge. Worth flagging for future edits that scripts/gen-readme.sh regenerates only the flag tables, so the README's copy of the example block is hand-maintained and had drifted from the binary; I diffed the two to confirm they match again.

The remaining inline comment, about live-only drift inside a replaced atomic list, is answered on its own thread. I have left that one open, since it is a design trade-off rather than something I have fixed.

For what it is worth while the checks are still pending: golangci-lint v2.12.2 against Go 1.26.4 — the pairing lint.yaml pins via go-version-file — reports 0 issues, and go test ./... passes.

[Copilot]

🔵 Needs a closer look

Local normalization can discard newer Kubernetes fields, while explicit-null restoration can hide real drift corrections.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

manifest/generate.go:365

• Decoding into the compiled Go type silently discards fields unknown to this client-go version. If helm-diff runs against a newer cluster and a chart adds a newly supported field to an otherwise known built-in kind, the local merge contains the field here, this round-trip removes it, and the restoration pass cannot recover it because it is absent from the live object. Client mode then omits a real upgrade change; preserve unknown fields from the merged object during normalization or avoid typed normalization when the typed schema does not cover the object.

manifest/generate.go:474

• This treats an explicit null exactly like a missing key, but null can be the chart-owned desired value for fields that are not defaulted. For example, if both manifests render runtimeClassName: null while the live Pod template drifted to runtimeClassName: gvisor, the three-way patch clears the field and normalize omits it; this condition then restores gvisor, hiding a correction that the real upgrade makes. Use presence-aware lookups and restrict null restoration to fields known to be server-defaulted rather than restoring every explicit-null field.

 if originalMap[key] == nil && modifiedMap[key] == nil {
mergedMap[key] = liveValue

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[oreonl]

What

--three-way-merge currently requires the patch permission on every diffed resource. It computes the merge patch locally but asks the API server to apply it as a dry-run:

helper.ServerDryRun=truetargetObj, err:=helper.Patch(info.Namespace, info.Name, patchType, patch, nil)

A read-only account gets cannot patch "x" with kind Deployment: ... is forbidden, so the feature is unavailable to exactly the credentials a "show me what this upgrade would do" tool tends to run under.

This applies the patch 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.

Interface

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

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

auto means a read-only account works out of the box; it only degrades when the accurate path is unavailable anyway, and says so on stderr.

manifest.Generate takes variadic options rather than a new parameter, so existing callers (helmfile vendors this package) keep compiling.

Matching what the server returns

A naive local merge produces a diff full of noise, because the API server does more than apply the patch. scheme.Scheme.Default() is a no-op in client-go — the SetDefaults_* functions live in k8s.io/kubernetes and are not importable — so the defaults cannot simply be recomputed. Two post-processing steps recover most of the gap:

1. Round-trip through the 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: [], supplementalGroups: [] — which otherwise appear as additions the upgrade does not make. (Helm's kube.Client.Build returns unstructured objects, so these survive from the chart YAML verbatim.)

2. Copy a field back from the live object when the old and the new release manifest agree about it. Values disappear from the merged object two ways:

• the patch replaces retainKeys structs (spec.strategy) and atomic lists (ports, volumeClaimTemplates) as a whole, dropping what the server had defaulted into them;
• a manifest that renders an unset value as an explicit null — a bare replicas:, which is what a chart writes for a value it leaves unset — puts the key in both manifests, so it reaches the patch as a change rather than a deletion. That deletes a defaulted value even when the chart did not change at all.

The server re-defaults both cases immediately after applying the patch. Locally, a field is restored only when nothing in either manifest asked for it to go. Where the two manifests disagree the deletion is honoured, because that is a change the chart really makes — restoring there would hide real removals (a dropped nodeSelector entry looks identical at the JSON level, and the server does not default that back). Values already present in the merged object are never overwritten, so drift between the cluster and the chart is still reported and still corrected.

Known deviation from server mode

When the manifests genuinely disagree about a field the API server defaults, client mode reports it as removed where server mode shows it changing to the default — a chart that stops pinning replicas: 3 shows the field going away rather than 3 → 1. The change is reported either way; client mode just cannot name the value that replaces it. Mutating webhooks and validation are also not applied. All of this is documented in the new README section, along with the minimal RBAC role.

Tests

manifest/generate_test.go is new. Eight TestLocalMerge_* cases are built from real manifests (unstructured, as kube.Client.Build supplies them; the live side written as the API server returns it) and cover each symptom above, plus:

• a chart that genuinely stops setting a field — still reported as removed;
• a hand-scaled replicas and hand-edited image in the cluster — still reset to the chart values, i.e. drift detection intact.

I checked the tests are not vacuous by reverting each fix independently; each fails without the step it covers. cmd/upgrade_test.go adds flag/env validation cases.

go test ./..., go vet and gofmt pass, and the README flag tables are regenerated with make readme.

Testing done

Built with make install/helm and run against a real cluster with an account lacking patch, on a release of ~15 resources (LibreChat and its subcharts). Before the two post-processing steps the client-side output differed from --three-way-merge-mode=server on defaulted fields; the cases in the tests are taken from that output.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

auto mode does not currently “fall back per run” (it keeps attempting forbidden PATCHes) and the docs/RBAC wording plus env-var handling need small corrections to match intended behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds a new three-way-merge “mode” capability so helm diff upgrade --three-way-merge can work with read-only Kubernetes credentials by falling back (or opting in) to a client-side patch application that approximates the API server’s results.

Changes:

• Introduces --three-way-merge-mode (auto/server/client) and env HELM_DIFF_THREE_WAY_MERGE_MODE, wiring it through cmd/upgrade into manifest.Generate(...) via variadic options.
• Implements client-side patch application for three-way-merge, including normalization (round-trip through Go types) and a restoration pass for server-populated/defaulted fields.
• Adds focused unit tests for local merge behavior and CLI flag/env validation; updates README and Copilot instructions.

File summaries

File	Description
README.md	Documents --three-way-merge-mode, behavior tradeoffs, and RBAC guidance.
manifest/generate.go	Adds merge-mode options and client-side patch application with post-processing to reduce diff noise.
manifest/generate_test.go	New tests covering strategic/merge patch local application and default-restoration behaviors.
cmd/upgrade.go	Adds flag/env plumbing and validation; passes merge-mode into manifest generation.
cmd/upgrade_test.go	Adds validation coverage for the new flag/env behavior.
.github/copilot-instructions.md	Documents the new environment variable.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

The new validation error messages format the allowed-mode slice with %q, which will render as %!q([]string=...) and should be corrected before merge.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

cmd/upgrade.go:199

• The format string uses %q for manifest.ValidThreeWayMergeModes (a []string), which will render as %!q([]string=...) and produce an unhelpful validation error. Use %v (or join the slice) when printing the allowed values.

This issue also appears on line 226 of the same file.

cmd/upgrade.go:228

• The env-var validation error formats manifest.ValidThreeWayMergeModes (a []string) with %q, which will print as %!q([]string=...). Use %v (or join the slice) so users see the allowed values clearly.

 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)
}

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[oreonl]

Checked this one and I don't think it holds — %q on a []string isn't malformed. fmt applies a verb recursively to the elements of a compound operand (docs: "For compound objects, the elements are printed using these rules, recursively... array, slice: [elem0 elem1 ...]"). %!q(...) only appears when the verb is invalid for the type, e.g. %q on a struct.

Actual output from the current branch, for both lines flagged:

$ helm diff upgrade rel ./chart --three-way-merge-mode local
Error: flag "three-way-merge-mode" must be one of ["auto" "server" "client"], but got "local"
$ HELM_DIFF_THREE_WAY_MERGE_MODE=local helm diff upgrade rel ./chart --three-way-merge
Error: env var "HELM_DIFF_THREE_WAY_MERGE_MODE" must be one of ["auto" "server" "client"], but got "local"

%v would print [auto server client], dropping the quotes that delimit the values — and the adjacent --server-side check quotes each one:

Error: flag "server-side" must be "true", "false" or "auto", but got "yes"

So I've left these as they are.

[Copilot]

🔵 Needs a closer look

It changes core diff computation behavior with non-trivial Kubernetes patch/defaulting semantics, so it warrants final human review despite good test coverage.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It changes core three-way-merge behavior with substantial new merge/restoration logic whose correctness and compatibility impact are difficult to fully validate via automated review alone.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[yxxhero]

@oreonl PTAL

[Copilot]

🟡 Changes recommended

Local restoration can hide real drift and CRD changes, while automatic fallback can mask admission failures.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 4
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Fallback handling and field restoration have correctness gaps, and several usage and RBAC examples are inaccurate.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (5)

Previously missed (3) — in code that hasn't changed since the last review.

README.md:276

• The release-storage requirement is understated here: the default revision is resolved by helm get manifest without --revision, which lists the Helm storage backend to find the newest release. A role with only get on Secrets/ConfigMaps therefore fails for the normal invocation; document list as required as well.

This issue also appears on line 297 of the same file.
README.md:404

• This generated example does not enable three-way merge. Since the mode environment variable is intentionally ignored unless three-way merge is already active, the command performs a normal two-way diff and does not demonstrate client-side merging.
  cmd/upgrade.go:170
• This example does not enable three-way merge. The command only reads this environment variable when HELM_DIFF_THREE_WAY_MERGE=true (or --three-way-merge/--take-ownership) is already active, so as written it performs the ordinary two-way diff rather than a local merge.

manifest/generate.go:233

• MethodNotAllowed never falls back when the authorization review says patching is allowed: isPatchNotAllowed routes it here, then this check returns false and aborts. That contradicts the new auto contract, which explicitly falls back for MethodNotAllowed. Restrict the authorization disambiguation to Forbidden; a 405 indicates the server path is unavailable regardless of RBAC.

	if f.canPatch != nil {
if allowed, err := f.canPatch(info); err == nil && allowed {
return false
}

README.md:299

• The least-privilege example cannot read the latest Helm release with the default revision. This code invokes helm get manifest without --revision (cmd/helm.go:139-158), so Helm must list the active Secret/ConfigMap storage backend to find the newest revision; granting only get causes the diff to fail before resource reads. Grant list to release storage while keeping workload kinds at get.

 - apiGroups: [""]
resources: ["configmaps", "secrets", "services", "serviceaccounts"]
verbs: ["get"]

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Balanced

[oreonl]

All five points from this review are addressed in d8d73ff. The four suppressed ones have no threads, so answering them here.

MethodNotAllowed aborting instead of falling back (manifest/generate.go:233) — correct, and a regression from the SelfSubjectAccessReview I added in ca059ad: the review ran for every refusal isPatchNotAllowed accepts, so a 405 against credentials that may patch was surfaced rather than falling back. Reproduced (patchDenied(405, canPatch=true) returned false) and fixed by restricting the disambiguation to Forbidden — a 405 says the API server takes no patch for that resource whoever is asking, so entitlement changes nothing. Covered by TestClientSideFallbackAlwaysFallsBackOnMethodNotAllowed.

Release storage needs list (README.md:276, :297, :299) — confirmed in the code: helmGetArgs omits --revision when the revision is 0 (cmd/helm.go:141-145), and helm's Secrets driver Query does a List to find the newest revision, so get alone fails before any resource is read. The least-privilege example now grants get, list on configmaps/secrets for the release storage and keeps the rendered kinds at get, with the reason spelled out in the surrounding prose.

The env var examples did not enable the three-way merge (README.md:404, cmd/upgrade.go:170) — right, and self-inflicted: making HELM_DIFF_THREE_WAY_MERGE_MODE deliberately ignored unless the merge is on invalidated my own examples. Both now pass --three-way-merge. Worth flagging for future edits that scripts/gen-readme.sh regenerates only the flag tables, so the README's copy of the example block is hand-maintained and had drifted from the binary; I diffed the two to confirm they match again.

The remaining inline comment, about live-only drift inside a replaced atomic list, is answered on its own thread. I have left that one open, since it is a design trade-off rather than something I have fixed.

For what it is worth while the checks are still pending: golangci-lint v2.12.2 against Go 1.26.4 — the pairing lint.yaml pins via go-version-file — reports 0 issues, and go test ./... passes.

[Copilot]

🔵 Needs a closer look

Local normalization can discard newer Kubernetes fields, while explicit-null restoration can hide real drift corrections.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

manifest/generate.go:365

• Decoding into the compiled Go type silently discards fields unknown to this client-go version. If helm-diff runs against a newer cluster and a chart adds a newly supported field to an otherwise known built-in kind, the local merge contains the field here, this round-trip removes it, and the restoration pass cannot recover it because it is absent from the live object. Client mode then omits a real upgrade change; preserve unknown fields from the merged object during normalization or avoid typed normalization when the typed schema does not cover the object.

manifest/generate.go:474

• This treats an explicit null exactly like a missing key, but null can be the chart-owned desired value for fields that are not defaulted. For example, if both manifests render runtimeClassName: null while the live Pod template drifted to runtimeClassName: gvisor, the three-way patch clears the field and normalize omits it; this condition then restores gvisor, hiding a correction that the real upgrade makes. Use presence-aware lookups and restrict null restoration to fields known to be server-defaulted rather than restoring every explicit-null field.

 if originalMap[key] == nil && modifiedMap[key] == nil {
mergedMap[key] = liveValue

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[oreonl]

What

--three-way-merge currently requires the patch permission on every diffed resource. It computes the merge patch locally but asks the API server to apply it as a dry-run:

helper.ServerDryRun=truetargetObj, err:=helper.Patch(info.Namespace, info.Name, patchType, patch, nil)

A read-only account gets cannot patch "x" with kind Deployment: ... is forbidden, so the feature is unavailable to exactly the credentials a "show me what this upgrade would do" tool tends to run under.

This applies the patch 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.

Interface

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

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

auto means a read-only account works out of the box; it only degrades when the accurate path is unavailable anyway, and says so on stderr.

manifest.Generate takes variadic options rather than a new parameter, so existing callers (helmfile vendors this package) keep compiling.

Matching what the server returns

A naive local merge produces a diff full of noise, because the API server does more than apply the patch. scheme.Scheme.Default() is a no-op in client-go — the SetDefaults_* functions live in k8s.io/kubernetes and are not importable — so the defaults cannot simply be recomputed. Two post-processing steps recover most of the gap:

1. Round-trip through the 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: [], supplementalGroups: [] — which otherwise appear as additions the upgrade does not make. (Helm's kube.Client.Build returns unstructured objects, so these survive from the chart YAML verbatim.)

2. Copy a field back from the live object when the old and the new release manifest agree about it. Values disappear from the merged object two ways:

• the patch replaces retainKeys structs (spec.strategy) and atomic lists (ports, volumeClaimTemplates) as a whole, dropping what the server had defaulted into them;
• a manifest that renders an unset value as an explicit null — a bare replicas:, which is what a chart writes for a value it leaves unset — puts the key in both manifests, so it reaches the patch as a change rather than a deletion. That deletes a defaulted value even when the chart did not change at all.

The server re-defaults both cases immediately after applying the patch. Locally, a field is restored only when nothing in either manifest asked for it to go. Where the two manifests disagree the deletion is honoured, because that is a change the chart really makes — restoring there would hide real removals (a dropped nodeSelector entry looks identical at the JSON level, and the server does not default that back). Values already present in the merged object are never overwritten, so drift between the cluster and the chart is still reported and still corrected.

Known deviation from server mode

When the manifests genuinely disagree about a field the API server defaults, client mode reports it as removed where server mode shows it changing to the default — a chart that stops pinning replicas: 3 shows the field going away rather than 3 → 1. The change is reported either way; client mode just cannot name the value that replaces it. Mutating webhooks and validation are also not applied. All of this is documented in the new README section, along with the minimal RBAC role.

Tests

manifest/generate_test.go is new. Eight TestLocalMerge_* cases are built from real manifests (unstructured, as kube.Client.Build supplies them; the live side written as the API server returns it) and cover each symptom above, plus:

• a chart that genuinely stops setting a field — still reported as removed;
• a hand-scaled replicas and hand-edited image in the cluster — still reset to the chart values, i.e. drift detection intact.

I checked the tests are not vacuous by reverting each fix independently; each fails without the step it covers. cmd/upgrade_test.go adds flag/env validation cases.

go test ./..., go vet and gofmt pass, and the README flag tables are regenerated with make readme.

Testing done

Built with make install/helm and run against a real cluster with an account lacking patch, on a release of ~15 resources (LibreChat and its subcharts). Before the two post-processing steps the client-side output differed from --three-way-merge-mode=server on defaulted fields; the cases in the tests are taken from that output.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

auto mode does not currently “fall back per run” (it keeps attempting forbidden PATCHes) and the docs/RBAC wording plus env-var handling need small corrections to match intended behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds a new three-way-merge “mode” capability so helm diff upgrade --three-way-merge can work with read-only Kubernetes credentials by falling back (or opting in) to a client-side patch application that approximates the API server’s results.

Changes:

• Introduces --three-way-merge-mode (auto/server/client) and env HELM_DIFF_THREE_WAY_MERGE_MODE, wiring it through cmd/upgrade into manifest.Generate(...) via variadic options.
• Implements client-side patch application for three-way-merge, including normalization (round-trip through Go types) and a restoration pass for server-populated/defaulted fields.
• Adds focused unit tests for local merge behavior and CLI flag/env validation; updates README and Copilot instructions.

File summaries

File	Description
README.md	Documents --three-way-merge-mode, behavior tradeoffs, and RBAC guidance.
manifest/generate.go	Adds merge-mode options and client-side patch application with post-processing to reduce diff noise.
manifest/generate_test.go	New tests covering strategic/merge patch local application and default-restoration behaviors.
cmd/upgrade.go	Adds flag/env plumbing and validation; passes merge-mode into manifest generation.
cmd/upgrade_test.go	Adds validation coverage for the new flag/env behavior.
.github/copilot-instructions.md	Documents the new environment variable.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

The new validation error messages format the allowed-mode slice with %q, which will render as %!q([]string=...) and should be corrected before merge.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

cmd/upgrade.go:199

• The format string uses %q for manifest.ValidThreeWayMergeModes (a []string), which will render as %!q([]string=...) and produce an unhelpful validation error. Use %v (or join the slice) when printing the allowed values.

This issue also appears on line 226 of the same file.

cmd/upgrade.go:228

• The env-var validation error formats manifest.ValidThreeWayMergeModes (a []string) with %q, which will print as %!q([]string=...). Use %v (or join the slice) so users see the allowed values clearly.

 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)
}

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[oreonl]

Checked this one and I don't think it holds — %q on a []string isn't malformed. fmt applies a verb recursively to the elements of a compound operand (docs: "For compound objects, the elements are printed using these rules, recursively... array, slice: [elem0 elem1 ...]"). %!q(...) only appears when the verb is invalid for the type, e.g. %q on a struct.

Actual output from the current branch, for both lines flagged:

$ helm diff upgrade rel ./chart --three-way-merge-mode local
Error: flag "three-way-merge-mode" must be one of ["auto" "server" "client"], but got "local"
$ HELM_DIFF_THREE_WAY_MERGE_MODE=local helm diff upgrade rel ./chart --three-way-merge
Error: env var "HELM_DIFF_THREE_WAY_MERGE_MODE" must be one of ["auto" "server" "client"], but got "local"

%v would print [auto server client], dropping the quotes that delimit the values — and the adjacent --server-side check quotes each one:

Error: flag "server-side" must be "true", "false" or "auto", but got "yes"

So I've left these as they are.

[Copilot]

🔵 Needs a closer look

It changes core diff computation behavior with non-trivial Kubernetes patch/defaulting semantics, so it warrants final human review despite good test coverage.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It changes core three-way-merge behavior with substantial new merge/restoration logic whose correctness and compatibility impact are difficult to fully validate via automated review alone.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[yxxhero]

@oreonl PTAL

[Copilot]

🟡 Changes recommended

Local restoration can hide real drift and CRD changes, while automatic fallback can mask admission failures.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 4
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Fallback handling and field restoration have correctness gaps, and several usage and RBAC examples are inaccurate.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (5)

Previously missed (3) — in code that hasn't changed since the last review.

README.md:276

• The release-storage requirement is understated here: the default revision is resolved by helm get manifest without --revision, which lists the Helm storage backend to find the newest release. A role with only get on Secrets/ConfigMaps therefore fails for the normal invocation; document list as required as well.

This issue also appears on line 297 of the same file.
README.md:404

• This generated example does not enable three-way merge. Since the mode environment variable is intentionally ignored unless three-way merge is already active, the command performs a normal two-way diff and does not demonstrate client-side merging.
  cmd/upgrade.go:170
• This example does not enable three-way merge. The command only reads this environment variable when HELM_DIFF_THREE_WAY_MERGE=true (or --three-way-merge/--take-ownership) is already active, so as written it performs the ordinary two-way diff rather than a local merge.

manifest/generate.go:233

• MethodNotAllowed never falls back when the authorization review says patching is allowed: isPatchNotAllowed routes it here, then this check returns false and aborts. That contradicts the new auto contract, which explicitly falls back for MethodNotAllowed. Restrict the authorization disambiguation to Forbidden; a 405 indicates the server path is unavailable regardless of RBAC.

	if f.canPatch != nil {
if allowed, err := f.canPatch(info); err == nil && allowed {
return false
}

README.md:299

• The least-privilege example cannot read the latest Helm release with the default revision. This code invokes helm get manifest without --revision (cmd/helm.go:139-158), so Helm must list the active Secret/ConfigMap storage backend to find the newest revision; granting only get causes the diff to fail before resource reads. Grant list to release storage while keeping workload kinds at get.

 - apiGroups: [""]
resources: ["configmaps", "secrets", "services", "serviceaccounts"]
verbs: ["get"]

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Balanced

[oreonl]

All five points from this review are addressed in d8d73ff. The four suppressed ones have no threads, so answering them here.

MethodNotAllowed aborting instead of falling back (manifest/generate.go:233) — correct, and a regression from the SelfSubjectAccessReview I added in ca059ad: the review ran for every refusal isPatchNotAllowed accepts, so a 405 against credentials that may patch was surfaced rather than falling back. Reproduced (patchDenied(405, canPatch=true) returned false) and fixed by restricting the disambiguation to Forbidden — a 405 says the API server takes no patch for that resource whoever is asking, so entitlement changes nothing. Covered by TestClientSideFallbackAlwaysFallsBackOnMethodNotAllowed.

Release storage needs list (README.md:276, :297, :299) — confirmed in the code: helmGetArgs omits --revision when the revision is 0 (cmd/helm.go:141-145), and helm's Secrets driver Query does a List to find the newest revision, so get alone fails before any resource is read. The least-privilege example now grants get, list on configmaps/secrets for the release storage and keeps the rendered kinds at get, with the reason spelled out in the surrounding prose.

The env var examples did not enable the three-way merge (README.md:404, cmd/upgrade.go:170) — right, and self-inflicted: making HELM_DIFF_THREE_WAY_MERGE_MODE deliberately ignored unless the merge is on invalidated my own examples. Both now pass --three-way-merge. Worth flagging for future edits that scripts/gen-readme.sh regenerates only the flag tables, so the README's copy of the example block is hand-maintained and had drifted from the binary; I diffed the two to confirm they match again.

The remaining inline comment, about live-only drift inside a replaced atomic list, is answered on its own thread. I have left that one open, since it is a design trade-off rather than something I have fixed.

For what it is worth while the checks are still pending: golangci-lint v2.12.2 against Go 1.26.4 — the pairing lint.yaml pins via go-version-file — reports 0 issues, and go test ./... passes.

[Copilot]

🔵 Needs a closer look

Local normalization can discard newer Kubernetes fields, while explicit-null restoration can hide real drift corrections.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

manifest/generate.go:365

• Decoding into the compiled Go type silently discards fields unknown to this client-go version. If helm-diff runs against a newer cluster and a chart adds a newly supported field to an otherwise known built-in kind, the local merge contains the field here, this round-trip removes it, and the restoration pass cannot recover it because it is absent from the live object. Client mode then omits a real upgrade change; preserve unknown fields from the merged object during normalization or avoid typed normalization when the typed schema does not cover the object.

manifest/generate.go:474

• This treats an explicit null exactly like a missing key, but null can be the chart-owned desired value for fields that are not defaulted. For example, if both manifests render runtimeClassName: null while the live Pod template drifted to runtimeClassName: gvisor, the three-way patch clears the field and normalize omits it; this condition then restores gvisor, hiding a correction that the real upgrade makes. Use presence-aware lookups and restrict null restoration to fields known to be server-defaulted rather than restoring every explicit-null field.

 if originalMap[key] == nil && modifiedMap[key] == nil {
mergedMap[key] = liveValue

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[oreonl]

What

--three-way-merge currently requires the patch permission on every diffed resource. It computes the merge patch locally but asks the API server to apply it as a dry-run:

helper.ServerDryRun=truetargetObj, err:=helper.Patch(info.Namespace, info.Name, patchType, patch, nil)

A read-only account gets cannot patch "x" with kind Deployment: ... is forbidden, so the feature is unavailable to exactly the credentials a "show me what this upgrade would do" tool tends to run under.

This applies the patch 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.

Interface

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

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

auto means a read-only account works out of the box; it only degrades when the accurate path is unavailable anyway, and says so on stderr.

manifest.Generate takes variadic options rather than a new parameter, so existing callers (helmfile vendors this package) keep compiling.

Matching what the server returns

A naive local merge produces a diff full of noise, because the API server does more than apply the patch. scheme.Scheme.Default() is a no-op in client-go — the SetDefaults_* functions live in k8s.io/kubernetes and are not importable — so the defaults cannot simply be recomputed. Two post-processing steps recover most of the gap:

1. Round-trip through the 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: [], supplementalGroups: [] — which otherwise appear as additions the upgrade does not make. (Helm's kube.Client.Build returns unstructured objects, so these survive from the chart YAML verbatim.)

2. Copy a field back from the live object when the old and the new release manifest agree about it. Values disappear from the merged object two ways:

• the patch replaces retainKeys structs (spec.strategy) and atomic lists (ports, volumeClaimTemplates) as a whole, dropping what the server had defaulted into them;
• a manifest that renders an unset value as an explicit null — a bare replicas:, which is what a chart writes for a value it leaves unset — puts the key in both manifests, so it reaches the patch as a change rather than a deletion. That deletes a defaulted value even when the chart did not change at all.

The server re-defaults both cases immediately after applying the patch. Locally, a field is restored only when nothing in either manifest asked for it to go. Where the two manifests disagree the deletion is honoured, because that is a change the chart really makes — restoring there would hide real removals (a dropped nodeSelector entry looks identical at the JSON level, and the server does not default that back). Values already present in the merged object are never overwritten, so drift between the cluster and the chart is still reported and still corrected.

Known deviation from server mode

When the manifests genuinely disagree about a field the API server defaults, client mode reports it as removed where server mode shows it changing to the default — a chart that stops pinning replicas: 3 shows the field going away rather than 3 → 1. The change is reported either way; client mode just cannot name the value that replaces it. Mutating webhooks and validation are also not applied. All of this is documented in the new README section, along with the minimal RBAC role.

Tests

manifest/generate_test.go is new. Eight TestLocalMerge_* cases are built from real manifests (unstructured, as kube.Client.Build supplies them; the live side written as the API server returns it) and cover each symptom above, plus:

• a chart that genuinely stops setting a field — still reported as removed;
• a hand-scaled replicas and hand-edited image in the cluster — still reset to the chart values, i.e. drift detection intact.

I checked the tests are not vacuous by reverting each fix independently; each fails without the step it covers. cmd/upgrade_test.go adds flag/env validation cases.

go test ./..., go vet and gofmt pass, and the README flag tables are regenerated with make readme.

Testing done

Built with make install/helm and run against a real cluster with an account lacking patch, on a release of ~15 resources (LibreChat and its subcharts). Before the two post-processing steps the client-side output differed from --three-way-merge-mode=server on defaulted fields; the cases in the tests are taken from that output.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

auto mode does not currently “fall back per run” (it keeps attempting forbidden PATCHes) and the docs/RBAC wording plus env-var handling need small corrections to match intended behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds a new three-way-merge “mode” capability so helm diff upgrade --three-way-merge can work with read-only Kubernetes credentials by falling back (or opting in) to a client-side patch application that approximates the API server’s results.

Changes:

• Introduces --three-way-merge-mode (auto/server/client) and env HELM_DIFF_THREE_WAY_MERGE_MODE, wiring it through cmd/upgrade into manifest.Generate(...) via variadic options.
• Implements client-side patch application for three-way-merge, including normalization (round-trip through Go types) and a restoration pass for server-populated/defaulted fields.
• Adds focused unit tests for local merge behavior and CLI flag/env validation; updates README and Copilot instructions.

File summaries

File	Description
README.md	Documents --three-way-merge-mode, behavior tradeoffs, and RBAC guidance.
manifest/generate.go	Adds merge-mode options and client-side patch application with post-processing to reduce diff noise.
manifest/generate_test.go	New tests covering strategic/merge patch local application and default-restoration behaviors.
cmd/upgrade.go	Adds flag/env plumbing and validation; passes merge-mode into manifest generation.
cmd/upgrade_test.go	Adds validation coverage for the new flag/env behavior.
.github/copilot-instructions.md	Documents the new environment variable.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

The new validation error messages format the allowed-mode slice with %q, which will render as %!q([]string=...) and should be corrected before merge.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

cmd/upgrade.go:199

• The format string uses %q for manifest.ValidThreeWayMergeModes (a []string), which will render as %!q([]string=...) and produce an unhelpful validation error. Use %v (or join the slice) when printing the allowed values.

This issue also appears on line 226 of the same file.

cmd/upgrade.go:228

• The env-var validation error formats manifest.ValidThreeWayMergeModes (a []string) with %q, which will print as %!q([]string=...). Use %v (or join the slice) so users see the allowed values clearly.

 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)
}

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[oreonl]

Checked this one and I don't think it holds — %q on a []string isn't malformed. fmt applies a verb recursively to the elements of a compound operand (docs: "For compound objects, the elements are printed using these rules, recursively... array, slice: [elem0 elem1 ...]"). %!q(...) only appears when the verb is invalid for the type, e.g. %q on a struct.

Actual output from the current branch, for both lines flagged:

$ helm diff upgrade rel ./chart --three-way-merge-mode local
Error: flag "three-way-merge-mode" must be one of ["auto" "server" "client"], but got "local"
$ HELM_DIFF_THREE_WAY_MERGE_MODE=local helm diff upgrade rel ./chart --three-way-merge
Error: env var "HELM_DIFF_THREE_WAY_MERGE_MODE" must be one of ["auto" "server" "client"], but got "local"

%v would print [auto server client], dropping the quotes that delimit the values — and the adjacent --server-side check quotes each one:

Error: flag "server-side" must be "true", "false" or "auto", but got "yes"

So I've left these as they are.

[Copilot]

🔵 Needs a closer look

It changes core diff computation behavior with non-trivial Kubernetes patch/defaulting semantics, so it warrants final human review despite good test coverage.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It changes core three-way-merge behavior with substantial new merge/restoration logic whose correctness and compatibility impact are difficult to fully validate via automated review alone.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[yxxhero]

@oreonl PTAL

[Copilot]

🟡 Changes recommended

Local restoration can hide real drift and CRD changes, while automatic fallback can mask admission failures.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 4
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Fallback handling and field restoration have correctness gaps, and several usage and RBAC examples are inaccurate.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (5)

Previously missed (3) — in code that hasn't changed since the last review.

README.md:276

• The release-storage requirement is understated here: the default revision is resolved by helm get manifest without --revision, which lists the Helm storage backend to find the newest release. A role with only get on Secrets/ConfigMaps therefore fails for the normal invocation; document list as required as well.

This issue also appears on line 297 of the same file.
README.md:404

• This generated example does not enable three-way merge. Since the mode environment variable is intentionally ignored unless three-way merge is already active, the command performs a normal two-way diff and does not demonstrate client-side merging.
  cmd/upgrade.go:170
• This example does not enable three-way merge. The command only reads this environment variable when HELM_DIFF_THREE_WAY_MERGE=true (or --three-way-merge/--take-ownership) is already active, so as written it performs the ordinary two-way diff rather than a local merge.

manifest/generate.go:233

• MethodNotAllowed never falls back when the authorization review says patching is allowed: isPatchNotAllowed routes it here, then this check returns false and aborts. That contradicts the new auto contract, which explicitly falls back for MethodNotAllowed. Restrict the authorization disambiguation to Forbidden; a 405 indicates the server path is unavailable regardless of RBAC.

	if f.canPatch != nil {
if allowed, err := f.canPatch(info); err == nil && allowed {
return false
}

README.md:299

• The least-privilege example cannot read the latest Helm release with the default revision. This code invokes helm get manifest without --revision (cmd/helm.go:139-158), so Helm must list the active Secret/ConfigMap storage backend to find the newest revision; granting only get causes the diff to fail before resource reads. Grant list to release storage while keeping workload kinds at get.

 - apiGroups: [""]
resources: ["configmaps", "secrets", "services", "serviceaccounts"]
verbs: ["get"]

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Balanced

[oreonl]

All five points from this review are addressed in d8d73ff. The four suppressed ones have no threads, so answering them here.

MethodNotAllowed aborting instead of falling back (manifest/generate.go:233) — correct, and a regression from the SelfSubjectAccessReview I added in ca059ad: the review ran for every refusal isPatchNotAllowed accepts, so a 405 against credentials that may patch was surfaced rather than falling back. Reproduced (patchDenied(405, canPatch=true) returned false) and fixed by restricting the disambiguation to Forbidden — a 405 says the API server takes no patch for that resource whoever is asking, so entitlement changes nothing. Covered by TestClientSideFallbackAlwaysFallsBackOnMethodNotAllowed.

Release storage needs list (README.md:276, :297, :299) — confirmed in the code: helmGetArgs omits --revision when the revision is 0 (cmd/helm.go:141-145), and helm's Secrets driver Query does a List to find the newest revision, so get alone fails before any resource is read. The least-privilege example now grants get, list on configmaps/secrets for the release storage and keeps the rendered kinds at get, with the reason spelled out in the surrounding prose.

The env var examples did not enable the three-way merge (README.md:404, cmd/upgrade.go:170) — right, and self-inflicted: making HELM_DIFF_THREE_WAY_MERGE_MODE deliberately ignored unless the merge is on invalidated my own examples. Both now pass --three-way-merge. Worth flagging for future edits that scripts/gen-readme.sh regenerates only the flag tables, so the README's copy of the example block is hand-maintained and had drifted from the binary; I diffed the two to confirm they match again.

The remaining inline comment, about live-only drift inside a replaced atomic list, is answered on its own thread. I have left that one open, since it is a design trade-off rather than something I have fixed.

For what it is worth while the checks are still pending: golangci-lint v2.12.2 against Go 1.26.4 — the pairing lint.yaml pins via go-version-file — reports 0 issues, and go test ./... passes.

[Copilot]

🔵 Needs a closer look

Local normalization can discard newer Kubernetes fields, while explicit-null restoration can hide real drift corrections.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

manifest/generate.go:365

• Decoding into the compiled Go type silently discards fields unknown to this client-go version. If helm-diff runs against a newer cluster and a chart adds a newly supported field to an otherwise known built-in kind, the local merge contains the field here, this round-trip removes it, and the restoration pass cannot recover it because it is absent from the live object. Client mode then omits a real upgrade change; preserve unknown fields from the merged object during normalization or avoid typed normalization when the typed schema does not cover the object.

manifest/generate.go:474

• This treats an explicit null exactly like a missing key, but null can be the chart-owned desired value for fields that are not defaulted. For example, if both manifests render runtimeClassName: null while the live Pod template drifted to runtimeClassName: gvisor, the three-way patch clears the field and normalize omits it; this condition then restores gvisor, hiding a correction that the real upgrade makes. Use presence-aware lookups and restrict null restoration to fields known to be server-defaulted rather than restoring every explicit-null field.

 if originalMap[key] == nil && modifiedMap[key] == nil {
mergedMap[key] = liveValue

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[oreonl]

What

--three-way-merge currently requires the patch permission on every diffed resource. It computes the merge patch locally but asks the API server to apply it as a dry-run:

helper.ServerDryRun=truetargetObj, err:=helper.Patch(info.Namespace, info.Name, patchType, patch, nil)

A read-only account gets cannot patch "x" with kind Deployment: ... is forbidden, so the feature is unavailable to exactly the credentials a "show me what this upgrade would do" tool tends to run under.

This applies the patch 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.

Interface

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

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

auto means a read-only account works out of the box; it only degrades when the accurate path is unavailable anyway, and says so on stderr.

manifest.Generate takes variadic options rather than a new parameter, so existing callers (helmfile vendors this package) keep compiling.

Matching what the server returns

A naive local merge produces a diff full of noise, because the API server does more than apply the patch. scheme.Scheme.Default() is a no-op in client-go — the SetDefaults_* functions live in k8s.io/kubernetes and are not importable — so the defaults cannot simply be recomputed. Two post-processing steps recover most of the gap:

1. Round-trip through the 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: [], supplementalGroups: [] — which otherwise appear as additions the upgrade does not make. (Helm's kube.Client.Build returns unstructured objects, so these survive from the chart YAML verbatim.)

2. Copy a field back from the live object when the old and the new release manifest agree about it. Values disappear from the merged object two ways:

• the patch replaces retainKeys structs (spec.strategy) and atomic lists (ports, volumeClaimTemplates) as a whole, dropping what the server had defaulted into them;
• a manifest that renders an unset value as an explicit null — a bare replicas:, which is what a chart writes for a value it leaves unset — puts the key in both manifests, so it reaches the patch as a change rather than a deletion. That deletes a defaulted value even when the chart did not change at all.

The server re-defaults both cases immediately after applying the patch. Locally, a field is restored only when nothing in either manifest asked for it to go. Where the two manifests disagree the deletion is honoured, because that is a change the chart really makes — restoring there would hide real removals (a dropped nodeSelector entry looks identical at the JSON level, and the server does not default that back). Values already present in the merged object are never overwritten, so drift between the cluster and the chart is still reported and still corrected.

Known deviation from server mode

When the manifests genuinely disagree about a field the API server defaults, client mode reports it as removed where server mode shows it changing to the default — a chart that stops pinning replicas: 3 shows the field going away rather than 3 → 1. The change is reported either way; client mode just cannot name the value that replaces it. Mutating webhooks and validation are also not applied. All of this is documented in the new README section, along with the minimal RBAC role.

Tests

manifest/generate_test.go is new. Eight TestLocalMerge_* cases are built from real manifests (unstructured, as kube.Client.Build supplies them; the live side written as the API server returns it) and cover each symptom above, plus:

• a chart that genuinely stops setting a field — still reported as removed;
• a hand-scaled replicas and hand-edited image in the cluster — still reset to the chart values, i.e. drift detection intact.

I checked the tests are not vacuous by reverting each fix independently; each fails without the step it covers. cmd/upgrade_test.go adds flag/env validation cases.

go test ./..., go vet and gofmt pass, and the README flag tables are regenerated with make readme.

Testing done

Built with make install/helm and run against a real cluster with an account lacking patch, on a release of ~15 resources (LibreChat and its subcharts). Before the two post-processing steps the client-side output differed from --three-way-merge-mode=server on defaulted fields; the cases in the tests are taken from that output.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

auto mode does not currently “fall back per run” (it keeps attempting forbidden PATCHes) and the docs/RBAC wording plus env-var handling need small corrections to match intended behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds a new three-way-merge “mode” capability so helm diff upgrade --three-way-merge can work with read-only Kubernetes credentials by falling back (or opting in) to a client-side patch application that approximates the API server’s results.

Changes:

• Introduces --three-way-merge-mode (auto/server/client) and env HELM_DIFF_THREE_WAY_MERGE_MODE, wiring it through cmd/upgrade into manifest.Generate(...) via variadic options.
• Implements client-side patch application for three-way-merge, including normalization (round-trip through Go types) and a restoration pass for server-populated/defaulted fields.
• Adds focused unit tests for local merge behavior and CLI flag/env validation; updates README and Copilot instructions.

File summaries

File	Description
README.md	Documents --three-way-merge-mode, behavior tradeoffs, and RBAC guidance.
manifest/generate.go	Adds merge-mode options and client-side patch application with post-processing to reduce diff noise.
manifest/generate_test.go	New tests covering strategic/merge patch local application and default-restoration behaviors.
cmd/upgrade.go	Adds flag/env plumbing and validation; passes merge-mode into manifest generation.
cmd/upgrade_test.go	Adds validation coverage for the new flag/env behavior.
.github/copilot-instructions.md	Documents the new environment variable.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

The new validation error messages format the allowed-mode slice with %q, which will render as %!q([]string=...) and should be corrected before merge.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

cmd/upgrade.go:199

• The format string uses %q for manifest.ValidThreeWayMergeModes (a []string), which will render as %!q([]string=...) and produce an unhelpful validation error. Use %v (or join the slice) when printing the allowed values.

This issue also appears on line 226 of the same file.

cmd/upgrade.go:228

• The env-var validation error formats manifest.ValidThreeWayMergeModes (a []string) with %q, which will print as %!q([]string=...). Use %v (or join the slice) so users see the allowed values clearly.

 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)
}

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[oreonl]

Checked this one and I don't think it holds — %q on a []string isn't malformed. fmt applies a verb recursively to the elements of a compound operand (docs: "For compound objects, the elements are printed using these rules, recursively... array, slice: [elem0 elem1 ...]"). %!q(...) only appears when the verb is invalid for the type, e.g. %q on a struct.

Actual output from the current branch, for both lines flagged:

$ helm diff upgrade rel ./chart --three-way-merge-mode local
Error: flag "three-way-merge-mode" must be one of ["auto" "server" "client"], but got "local"
$ HELM_DIFF_THREE_WAY_MERGE_MODE=local helm diff upgrade rel ./chart --three-way-merge
Error: env var "HELM_DIFF_THREE_WAY_MERGE_MODE" must be one of ["auto" "server" "client"], but got "local"

%v would print [auto server client], dropping the quotes that delimit the values — and the adjacent --server-side check quotes each one:

Error: flag "server-side" must be "true", "false" or "auto", but got "yes"

So I've left these as they are.

[Copilot]

🔵 Needs a closer look

It changes core diff computation behavior with non-trivial Kubernetes patch/defaulting semantics, so it warrants final human review despite good test coverage.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It changes core three-way-merge behavior with substantial new merge/restoration logic whose correctness and compatibility impact are difficult to fully validate via automated review alone.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[yxxhero]

@oreonl PTAL

[Copilot]

🟡 Changes recommended

Local restoration can hide real drift and CRD changes, while automatic fallback can mask admission failures.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 4
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Fallback handling and field restoration have correctness gaps, and several usage and RBAC examples are inaccurate.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (5)

Previously missed (3) — in code that hasn't changed since the last review.

README.md:276

• The release-storage requirement is understated here: the default revision is resolved by helm get manifest without --revision, which lists the Helm storage backend to find the newest release. A role with only get on Secrets/ConfigMaps therefore fails for the normal invocation; document list as required as well.

This issue also appears on line 297 of the same file.
README.md:404

• This generated example does not enable three-way merge. Since the mode environment variable is intentionally ignored unless three-way merge is already active, the command performs a normal two-way diff and does not demonstrate client-side merging.
  cmd/upgrade.go:170
• This example does not enable three-way merge. The command only reads this environment variable when HELM_DIFF_THREE_WAY_MERGE=true (or --three-way-merge/--take-ownership) is already active, so as written it performs the ordinary two-way diff rather than a local merge.

manifest/generate.go:233

• MethodNotAllowed never falls back when the authorization review says patching is allowed: isPatchNotAllowed routes it here, then this check returns false and aborts. That contradicts the new auto contract, which explicitly falls back for MethodNotAllowed. Restrict the authorization disambiguation to Forbidden; a 405 indicates the server path is unavailable regardless of RBAC.

	if f.canPatch != nil {
if allowed, err := f.canPatch(info); err == nil && allowed {
return false
}

README.md:299

• The least-privilege example cannot read the latest Helm release with the default revision. This code invokes helm get manifest without --revision (cmd/helm.go:139-158), so Helm must list the active Secret/ConfigMap storage backend to find the newest revision; granting only get causes the diff to fail before resource reads. Grant list to release storage while keeping workload kinds at get.

 - apiGroups: [""]
resources: ["configmaps", "secrets", "services", "serviceaccounts"]
verbs: ["get"]

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Balanced

[oreonl]

All five points from this review are addressed in d8d73ff. The four suppressed ones have no threads, so answering them here.

MethodNotAllowed aborting instead of falling back (manifest/generate.go:233) — correct, and a regression from the SelfSubjectAccessReview I added in ca059ad: the review ran for every refusal isPatchNotAllowed accepts, so a 405 against credentials that may patch was surfaced rather than falling back. Reproduced (patchDenied(405, canPatch=true) returned false) and fixed by restricting the disambiguation to Forbidden — a 405 says the API server takes no patch for that resource whoever is asking, so entitlement changes nothing. Covered by TestClientSideFallbackAlwaysFallsBackOnMethodNotAllowed.

Release storage needs list (README.md:276, :297, :299) — confirmed in the code: helmGetArgs omits --revision when the revision is 0 (cmd/helm.go:141-145), and helm's Secrets driver Query does a List to find the newest revision, so get alone fails before any resource is read. The least-privilege example now grants get, list on configmaps/secrets for the release storage and keeps the rendered kinds at get, with the reason spelled out in the surrounding prose.

The env var examples did not enable the three-way merge (README.md:404, cmd/upgrade.go:170) — right, and self-inflicted: making HELM_DIFF_THREE_WAY_MERGE_MODE deliberately ignored unless the merge is on invalidated my own examples. Both now pass --three-way-merge. Worth flagging for future edits that scripts/gen-readme.sh regenerates only the flag tables, so the README's copy of the example block is hand-maintained and had drifted from the binary; I diffed the two to confirm they match again.

The remaining inline comment, about live-only drift inside a replaced atomic list, is answered on its own thread. I have left that one open, since it is a design trade-off rather than something I have fixed.

For what it is worth while the checks are still pending: golangci-lint v2.12.2 against Go 1.26.4 — the pairing lint.yaml pins via go-version-file — reports 0 issues, and go test ./... passes.

[Copilot]

🔵 Needs a closer look

Local normalization can discard newer Kubernetes fields, while explicit-null restoration can hide real drift corrections.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

manifest/generate.go:365

• Decoding into the compiled Go type silently discards fields unknown to this client-go version. If helm-diff runs against a newer cluster and a chart adds a newly supported field to an otherwise known built-in kind, the local merge contains the field here, this round-trip removes it, and the restoration pass cannot recover it because it is absent from the live object. Client mode then omits a real upgrade change; preserve unknown fields from the merged object during normalization or avoid typed normalization when the typed schema does not cover the object.

manifest/generate.go:474

• This treats an explicit null exactly like a missing key, but null can be the chart-owned desired value for fields that are not defaulted. For example, if both manifests render runtimeClassName: null while the live Pod template drifted to runtimeClassName: gvisor, the three-way patch clears the field and normalize omits it; this condition then restores gvisor, hiding a correction that the real upgrade makes. Use presence-aware lookups and restrict null restoration to fields known to be server-defaulted rather than restoring every explicit-null field.

 if originalMap[key] == nil && modifiedMap[key] == nil {
mergedMap[key] = liveValue

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[oreonl]

What

--three-way-merge currently requires the patch permission on every diffed resource. It computes the merge patch locally but asks the API server to apply it as a dry-run:

helper.ServerDryRun=truetargetObj, err:=helper.Patch(info.Namespace, info.Name, patchType, patch, nil)

A read-only account gets cannot patch "x" with kind Deployment: ... is forbidden, so the feature is unavailable to exactly the credentials a "show me what this upgrade would do" tool tends to run under.

This applies the patch 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.

Interface

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

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

auto means a read-only account works out of the box; it only degrades when the accurate path is unavailable anyway, and says so on stderr.

manifest.Generate takes variadic options rather than a new parameter, so existing callers (helmfile vendors this package) keep compiling.

Matching what the server returns

A naive local merge produces a diff full of noise, because the API server does more than apply the patch. scheme.Scheme.Default() is a no-op in client-go — the SetDefaults_* functions live in k8s.io/kubernetes and are not importable — so the defaults cannot simply be recomputed. Two post-processing steps recover most of the gap:

1. Round-trip through the 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: [], supplementalGroups: [] — which otherwise appear as additions the upgrade does not make. (Helm's kube.Client.Build returns unstructured objects, so these survive from the chart YAML verbatim.)

2. Copy a field back from the live object when the old and the new release manifest agree about it. Values disappear from the merged object two ways:

• the patch replaces retainKeys structs (spec.strategy) and atomic lists (ports, volumeClaimTemplates) as a whole, dropping what the server had defaulted into them;
• a manifest that renders an unset value as an explicit null — a bare replicas:, which is what a chart writes for a value it leaves unset — puts the key in both manifests, so it reaches the patch as a change rather than a deletion. That deletes a defaulted value even when the chart did not change at all.

The server re-defaults both cases immediately after applying the patch. Locally, a field is restored only when nothing in either manifest asked for it to go. Where the two manifests disagree the deletion is honoured, because that is a change the chart really makes — restoring there would hide real removals (a dropped nodeSelector entry looks identical at the JSON level, and the server does not default that back). Values already present in the merged object are never overwritten, so drift between the cluster and the chart is still reported and still corrected.

Known deviation from server mode

When the manifests genuinely disagree about a field the API server defaults, client mode reports it as removed where server mode shows it changing to the default — a chart that stops pinning replicas: 3 shows the field going away rather than 3 → 1. The change is reported either way; client mode just cannot name the value that replaces it. Mutating webhooks and validation are also not applied. All of this is documented in the new README section, along with the minimal RBAC role.

Tests

manifest/generate_test.go is new. Eight TestLocalMerge_* cases are built from real manifests (unstructured, as kube.Client.Build supplies them; the live side written as the API server returns it) and cover each symptom above, plus:

• a chart that genuinely stops setting a field — still reported as removed;
• a hand-scaled replicas and hand-edited image in the cluster — still reset to the chart values, i.e. drift detection intact.

I checked the tests are not vacuous by reverting each fix independently; each fails without the step it covers. cmd/upgrade_test.go adds flag/env validation cases.

go test ./..., go vet and gofmt pass, and the README flag tables are regenerated with make readme.

Testing done

Built with make install/helm and run against a real cluster with an account lacking patch, on a release of ~15 resources (LibreChat and its subcharts). Before the two post-processing steps the client-side output differed from --three-way-merge-mode=server on defaulted fields; the cases in the tests are taken from that output.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

auto mode does not currently “fall back per run” (it keeps attempting forbidden PATCHes) and the docs/RBAC wording plus env-var handling need small corrections to match intended behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds a new three-way-merge “mode” capability so helm diff upgrade --three-way-merge can work with read-only Kubernetes credentials by falling back (or opting in) to a client-side patch application that approximates the API server’s results.

Changes:

• Introduces --three-way-merge-mode (auto/server/client) and env HELM_DIFF_THREE_WAY_MERGE_MODE, wiring it through cmd/upgrade into manifest.Generate(...) via variadic options.
• Implements client-side patch application for three-way-merge, including normalization (round-trip through Go types) and a restoration pass for server-populated/defaulted fields.
• Adds focused unit tests for local merge behavior and CLI flag/env validation; updates README and Copilot instructions.

File summaries

File	Description
README.md	Documents --three-way-merge-mode, behavior tradeoffs, and RBAC guidance.
manifest/generate.go	Adds merge-mode options and client-side patch application with post-processing to reduce diff noise.
manifest/generate_test.go	New tests covering strategic/merge patch local application and default-restoration behaviors.
cmd/upgrade.go	Adds flag/env plumbing and validation; passes merge-mode into manifest generation.
cmd/upgrade_test.go	Adds validation coverage for the new flag/env behavior.
.github/copilot-instructions.md	Documents the new environment variable.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

The new validation error messages format the allowed-mode slice with %q, which will render as %!q([]string=...) and should be corrected before merge.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

cmd/upgrade.go:199

• The format string uses %q for manifest.ValidThreeWayMergeModes (a []string), which will render as %!q([]string=...) and produce an unhelpful validation error. Use %v (or join the slice) when printing the allowed values.

This issue also appears on line 226 of the same file.

cmd/upgrade.go:228

• The env-var validation error formats manifest.ValidThreeWayMergeModes (a []string) with %q, which will print as %!q([]string=...). Use %v (or join the slice) so users see the allowed values clearly.

 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)
}

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[oreonl]

Checked this one and I don't think it holds — %q on a []string isn't malformed. fmt applies a verb recursively to the elements of a compound operand (docs: "For compound objects, the elements are printed using these rules, recursively... array, slice: [elem0 elem1 ...]"). %!q(...) only appears when the verb is invalid for the type, e.g. %q on a struct.

Actual output from the current branch, for both lines flagged:

$ helm diff upgrade rel ./chart --three-way-merge-mode local
Error: flag "three-way-merge-mode" must be one of ["auto" "server" "client"], but got "local"
$ HELM_DIFF_THREE_WAY_MERGE_MODE=local helm diff upgrade rel ./chart --three-way-merge
Error: env var "HELM_DIFF_THREE_WAY_MERGE_MODE" must be one of ["auto" "server" "client"], but got "local"

%v would print [auto server client], dropping the quotes that delimit the values — and the adjacent --server-side check quotes each one:

Error: flag "server-side" must be "true", "false" or "auto", but got "yes"

So I've left these as they are.

[Copilot]

🔵 Needs a closer look

It changes core diff computation behavior with non-trivial Kubernetes patch/defaulting semantics, so it warrants final human review despite good test coverage.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It changes core three-way-merge behavior with substantial new merge/restoration logic whose correctness and compatibility impact are difficult to fully validate via automated review alone.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[yxxhero]

@oreonl PTAL

[Copilot]

🟡 Changes recommended

Local restoration can hide real drift and CRD changes, while automatic fallback can mask admission failures.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 4
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Fallback handling and field restoration have correctness gaps, and several usage and RBAC examples are inaccurate.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (5)

Previously missed (3) — in code that hasn't changed since the last review.

README.md:276

• The release-storage requirement is understated here: the default revision is resolved by helm get manifest without --revision, which lists the Helm storage backend to find the newest release. A role with only get on Secrets/ConfigMaps therefore fails for the normal invocation; document list as required as well.

This issue also appears on line 297 of the same file.
README.md:404

• This generated example does not enable three-way merge. Since the mode environment variable is intentionally ignored unless three-way merge is already active, the command performs a normal two-way diff and does not demonstrate client-side merging.
  cmd/upgrade.go:170
• This example does not enable three-way merge. The command only reads this environment variable when HELM_DIFF_THREE_WAY_MERGE=true (or --three-way-merge/--take-ownership) is already active, so as written it performs the ordinary two-way diff rather than a local merge.

manifest/generate.go:233

• MethodNotAllowed never falls back when the authorization review says patching is allowed: isPatchNotAllowed routes it here, then this check returns false and aborts. That contradicts the new auto contract, which explicitly falls back for MethodNotAllowed. Restrict the authorization disambiguation to Forbidden; a 405 indicates the server path is unavailable regardless of RBAC.

	if f.canPatch != nil {
if allowed, err := f.canPatch(info); err == nil && allowed {
return false
}

README.md:299

• The least-privilege example cannot read the latest Helm release with the default revision. This code invokes helm get manifest without --revision (cmd/helm.go:139-158), so Helm must list the active Secret/ConfigMap storage backend to find the newest revision; granting only get causes the diff to fail before resource reads. Grant list to release storage while keeping workload kinds at get.

 - apiGroups: [""]
resources: ["configmaps", "secrets", "services", "serviceaccounts"]
verbs: ["get"]

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Balanced

[oreonl]

All five points from this review are addressed in d8d73ff. The four suppressed ones have no threads, so answering them here.

MethodNotAllowed aborting instead of falling back (manifest/generate.go:233) — correct, and a regression from the SelfSubjectAccessReview I added in ca059ad: the review ran for every refusal isPatchNotAllowed accepts, so a 405 against credentials that may patch was surfaced rather than falling back. Reproduced (patchDenied(405, canPatch=true) returned false) and fixed by restricting the disambiguation to Forbidden — a 405 says the API server takes no patch for that resource whoever is asking, so entitlement changes nothing. Covered by TestClientSideFallbackAlwaysFallsBackOnMethodNotAllowed.

Release storage needs list (README.md:276, :297, :299) — confirmed in the code: helmGetArgs omits --revision when the revision is 0 (cmd/helm.go:141-145), and helm's Secrets driver Query does a List to find the newest revision, so get alone fails before any resource is read. The least-privilege example now grants get, list on configmaps/secrets for the release storage and keeps the rendered kinds at get, with the reason spelled out in the surrounding prose.

The env var examples did not enable the three-way merge (README.md:404, cmd/upgrade.go:170) — right, and self-inflicted: making HELM_DIFF_THREE_WAY_MERGE_MODE deliberately ignored unless the merge is on invalidated my own examples. Both now pass --three-way-merge. Worth flagging for future edits that scripts/gen-readme.sh regenerates only the flag tables, so the README's copy of the example block is hand-maintained and had drifted from the binary; I diffed the two to confirm they match again.

The remaining inline comment, about live-only drift inside a replaced atomic list, is answered on its own thread. I have left that one open, since it is a design trade-off rather than something I have fixed.

For what it is worth while the checks are still pending: golangci-lint v2.12.2 against Go 1.26.4 — the pairing lint.yaml pins via go-version-file — reports 0 issues, and go test ./... passes.

[Copilot]

🔵 Needs a closer look

Local normalization can discard newer Kubernetes fields, while explicit-null restoration can hide real drift corrections.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

manifest/generate.go:365

• Decoding into the compiled Go type silently discards fields unknown to this client-go version. If helm-diff runs against a newer cluster and a chart adds a newly supported field to an otherwise known built-in kind, the local merge contains the field here, this round-trip removes it, and the restoration pass cannot recover it because it is absent from the live object. Client mode then omits a real upgrade change; preserve unknown fields from the merged object during normalization or avoid typed normalization when the typed schema does not cover the object.

manifest/generate.go:474

• This treats an explicit null exactly like a missing key, but null can be the chart-owned desired value for fields that are not defaulted. For example, if both manifests render runtimeClassName: null while the live Pod template drifted to runtimeClassName: gvisor, the three-way patch clears the field and normalize omits it; this condition then restores gvisor, hiding a correction that the real upgrade makes. Use presence-aware lookups and restrict null restoration to fields known to be server-defaulted rather than restoring every explicit-null field.

 if originalMap[key] == nil && modifiedMap[key] == nil {
mergedMap[key] = liveValue

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[oreonl]

What

--three-way-merge currently requires the patch permission on every diffed resource. It computes the merge patch locally but asks the API server to apply it as a dry-run:

helper.ServerDryRun=truetargetObj, err:=helper.Patch(info.Namespace, info.Name, patchType, patch, nil)

A read-only account gets cannot patch "x" with kind Deployment: ... is forbidden, so the feature is unavailable to exactly the credentials a "show me what this upgrade would do" tool tends to run under.

This applies the patch 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.

Interface

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

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

auto means a read-only account works out of the box; it only degrades when the accurate path is unavailable anyway, and says so on stderr.

manifest.Generate takes variadic options rather than a new parameter, so existing callers (helmfile vendors this package) keep compiling.

Matching what the server returns

A naive local merge produces a diff full of noise, because the API server does more than apply the patch. scheme.Scheme.Default() is a no-op in client-go — the SetDefaults_* functions live in k8s.io/kubernetes and are not importable — so the defaults cannot simply be recomputed. Two post-processing steps recover most of the gap:

1. Round-trip through the 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: [], supplementalGroups: [] — which otherwise appear as additions the upgrade does not make. (Helm's kube.Client.Build returns unstructured objects, so these survive from the chart YAML verbatim.)

2. Copy a field back from the live object when the old and the new release manifest agree about it. Values disappear from the merged object two ways:

• the patch replaces retainKeys structs (spec.strategy) and atomic lists (ports, volumeClaimTemplates) as a whole, dropping what the server had defaulted into them;
• a manifest that renders an unset value as an explicit null — a bare replicas:, which is what a chart writes for a value it leaves unset — puts the key in both manifests, so it reaches the patch as a change rather than a deletion. That deletes a defaulted value even when the chart did not change at all.

The server re-defaults both cases immediately after applying the patch. Locally, a field is restored only when nothing in either manifest asked for it to go. Where the two manifests disagree the deletion is honoured, because that is a change the chart really makes — restoring there would hide real removals (a dropped nodeSelector entry looks identical at the JSON level, and the server does not default that back). Values already present in the merged object are never overwritten, so drift between the cluster and the chart is still reported and still corrected.

Known deviation from server mode

When the manifests genuinely disagree about a field the API server defaults, client mode reports it as removed where server mode shows it changing to the default — a chart that stops pinning replicas: 3 shows the field going away rather than 3 → 1. The change is reported either way; client mode just cannot name the value that replaces it. Mutating webhooks and validation are also not applied. All of this is documented in the new README section, along with the minimal RBAC role.

Tests

manifest/generate_test.go is new. Eight TestLocalMerge_* cases are built from real manifests (unstructured, as kube.Client.Build supplies them; the live side written as the API server returns it) and cover each symptom above, plus:

• a chart that genuinely stops setting a field — still reported as removed;
• a hand-scaled replicas and hand-edited image in the cluster — still reset to the chart values, i.e. drift detection intact.

I checked the tests are not vacuous by reverting each fix independently; each fails without the step it covers. cmd/upgrade_test.go adds flag/env validation cases.

go test ./..., go vet and gofmt pass, and the README flag tables are regenerated with make readme.

Testing done

Built with make install/helm and run against a real cluster with an account lacking patch, on a release of ~15 resources (LibreChat and its subcharts). Before the two post-processing steps the client-side output differed from --three-way-merge-mode=server on defaulted fields; the cases in the tests are taken from that output.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

auto mode does not currently “fall back per run” (it keeps attempting forbidden PATCHes) and the docs/RBAC wording plus env-var handling need small corrections to match intended behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds a new three-way-merge “mode” capability so helm diff upgrade --three-way-merge can work with read-only Kubernetes credentials by falling back (or opting in) to a client-side patch application that approximates the API server’s results.

Changes:

• Introduces --three-way-merge-mode (auto/server/client) and env HELM_DIFF_THREE_WAY_MERGE_MODE, wiring it through cmd/upgrade into manifest.Generate(...) via variadic options.
• Implements client-side patch application for three-way-merge, including normalization (round-trip through Go types) and a restoration pass for server-populated/defaulted fields.
• Adds focused unit tests for local merge behavior and CLI flag/env validation; updates README and Copilot instructions.

File summaries

File	Description
README.md	Documents --three-way-merge-mode, behavior tradeoffs, and RBAC guidance.
manifest/generate.go	Adds merge-mode options and client-side patch application with post-processing to reduce diff noise.
manifest/generate_test.go	New tests covering strategic/merge patch local application and default-restoration behaviors.
cmd/upgrade.go	Adds flag/env plumbing and validation; passes merge-mode into manifest generation.
cmd/upgrade_test.go	Adds validation coverage for the new flag/env behavior.
.github/copilot-instructions.md	Documents the new environment variable.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

The new validation error messages format the allowed-mode slice with %q, which will render as %!q([]string=...) and should be corrected before merge.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

cmd/upgrade.go:199

• The format string uses %q for manifest.ValidThreeWayMergeModes (a []string), which will render as %!q([]string=...) and produce an unhelpful validation error. Use %v (or join the slice) when printing the allowed values.

This issue also appears on line 226 of the same file.

cmd/upgrade.go:228

• The env-var validation error formats manifest.ValidThreeWayMergeModes (a []string) with %q, which will print as %!q([]string=...). Use %v (or join the slice) so users see the allowed values clearly.

 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)
}

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[oreonl]

Checked this one and I don't think it holds — %q on a []string isn't malformed. fmt applies a verb recursively to the elements of a compound operand (docs: "For compound objects, the elements are printed using these rules, recursively... array, slice: [elem0 elem1 ...]"). %!q(...) only appears when the verb is invalid for the type, e.g. %q on a struct.

Actual output from the current branch, for both lines flagged:

$ helm diff upgrade rel ./chart --three-way-merge-mode local
Error: flag "three-way-merge-mode" must be one of ["auto" "server" "client"], but got "local"
$ HELM_DIFF_THREE_WAY_MERGE_MODE=local helm diff upgrade rel ./chart --three-way-merge
Error: env var "HELM_DIFF_THREE_WAY_MERGE_MODE" must be one of ["auto" "server" "client"], but got "local"

%v would print [auto server client], dropping the quotes that delimit the values — and the adjacent --server-side check quotes each one:

Error: flag "server-side" must be "true", "false" or "auto", but got "yes"

So I've left these as they are.

[Copilot]

🔵 Needs a closer look

It changes core diff computation behavior with non-trivial Kubernetes patch/defaulting semantics, so it warrants final human review despite good test coverage.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It changes core three-way-merge behavior with substantial new merge/restoration logic whose correctness and compatibility impact are difficult to fully validate via automated review alone.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[yxxhero]

@oreonl PTAL

[Copilot]

🟡 Changes recommended

Local restoration can hide real drift and CRD changes, while automatic fallback can mask admission failures.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 4
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Fallback handling and field restoration have correctness gaps, and several usage and RBAC examples are inaccurate.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (5)

Previously missed (3) — in code that hasn't changed since the last review.

README.md:276

• The release-storage requirement is understated here: the default revision is resolved by helm get manifest without --revision, which lists the Helm storage backend to find the newest release. A role with only get on Secrets/ConfigMaps therefore fails for the normal invocation; document list as required as well.

This issue also appears on line 297 of the same file.
README.md:404

• This generated example does not enable three-way merge. Since the mode environment variable is intentionally ignored unless three-way merge is already active, the command performs a normal two-way diff and does not demonstrate client-side merging.
  cmd/upgrade.go:170
• This example does not enable three-way merge. The command only reads this environment variable when HELM_DIFF_THREE_WAY_MERGE=true (or --three-way-merge/--take-ownership) is already active, so as written it performs the ordinary two-way diff rather than a local merge.

manifest/generate.go:233

• MethodNotAllowed never falls back when the authorization review says patching is allowed: isPatchNotAllowed routes it here, then this check returns false and aborts. That contradicts the new auto contract, which explicitly falls back for MethodNotAllowed. Restrict the authorization disambiguation to Forbidden; a 405 indicates the server path is unavailable regardless of RBAC.

	if f.canPatch != nil {
if allowed, err := f.canPatch(info); err == nil && allowed {
return false
}

README.md:299

• The least-privilege example cannot read the latest Helm release with the default revision. This code invokes helm get manifest without --revision (cmd/helm.go:139-158), so Helm must list the active Secret/ConfigMap storage backend to find the newest revision; granting only get causes the diff to fail before resource reads. Grant list to release storage while keeping workload kinds at get.

 - apiGroups: [""]
resources: ["configmaps", "secrets", "services", "serviceaccounts"]
verbs: ["get"]

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Balanced

[oreonl]

All five points from this review are addressed in d8d73ff. The four suppressed ones have no threads, so answering them here.

MethodNotAllowed aborting instead of falling back (manifest/generate.go:233) — correct, and a regression from the SelfSubjectAccessReview I added in ca059ad: the review ran for every refusal isPatchNotAllowed accepts, so a 405 against credentials that may patch was surfaced rather than falling back. Reproduced (patchDenied(405, canPatch=true) returned false) and fixed by restricting the disambiguation to Forbidden — a 405 says the API server takes no patch for that resource whoever is asking, so entitlement changes nothing. Covered by TestClientSideFallbackAlwaysFallsBackOnMethodNotAllowed.

Release storage needs list (README.md:276, :297, :299) — confirmed in the code: helmGetArgs omits --revision when the revision is 0 (cmd/helm.go:141-145), and helm's Secrets driver Query does a List to find the newest revision, so get alone fails before any resource is read. The least-privilege example now grants get, list on configmaps/secrets for the release storage and keeps the rendered kinds at get, with the reason spelled out in the surrounding prose.

The env var examples did not enable the three-way merge (README.md:404, cmd/upgrade.go:170) — right, and self-inflicted: making HELM_DIFF_THREE_WAY_MERGE_MODE deliberately ignored unless the merge is on invalidated my own examples. Both now pass --three-way-merge. Worth flagging for future edits that scripts/gen-readme.sh regenerates only the flag tables, so the README's copy of the example block is hand-maintained and had drifted from the binary; I diffed the two to confirm they match again.

The remaining inline comment, about live-only drift inside a replaced atomic list, is answered on its own thread. I have left that one open, since it is a design trade-off rather than something I have fixed.

For what it is worth while the checks are still pending: golangci-lint v2.12.2 against Go 1.26.4 — the pairing lint.yaml pins via go-version-file — reports 0 issues, and go test ./... passes.

[Copilot]

🔵 Needs a closer look

Local normalization can discard newer Kubernetes fields, while explicit-null restoration can hide real drift corrections.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

manifest/generate.go:365

• Decoding into the compiled Go type silently discards fields unknown to this client-go version. If helm-diff runs against a newer cluster and a chart adds a newly supported field to an otherwise known built-in kind, the local merge contains the field here, this round-trip removes it, and the restoration pass cannot recover it because it is absent from the live object. Client mode then omits a real upgrade change; preserve unknown fields from the merged object during normalization or avoid typed normalization when the typed schema does not cover the object.

manifest/generate.go:474

• This treats an explicit null exactly like a missing key, but null can be the chart-owned desired value for fields that are not defaulted. For example, if both manifests render runtimeClassName: null while the live Pod template drifted to runtimeClassName: gvisor, the three-way patch clears the field and normalize omits it; this condition then restores gvisor, hiding a correction that the real upgrade makes. Use presence-aware lookups and restrict null restoration to fields known to be server-defaulted rather than restoring every explicit-null field.

 if originalMap[key] == nil && modifiedMap[key] == nil {
mergedMap[key] = liveValue

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[oreonl]

What

--three-way-merge currently requires the patch permission on every diffed resource. It computes the merge patch locally but asks the API server to apply it as a dry-run:

helper.ServerDryRun=truetargetObj, err:=helper.Patch(info.Namespace, info.Name, patchType, patch, nil)

A read-only account gets cannot patch "x" with kind Deployment: ... is forbidden, so the feature is unavailable to exactly the credentials a "show me what this upgrade would do" tool tends to run under.

This applies the patch 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.

Interface

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

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

auto means a read-only account works out of the box; it only degrades when the accurate path is unavailable anyway, and says so on stderr.

manifest.Generate takes variadic options rather than a new parameter, so existing callers (helmfile vendors this package) keep compiling.

Matching what the server returns

A naive local merge produces a diff full of noise, because the API server does more than apply the patch. scheme.Scheme.Default() is a no-op in client-go — the SetDefaults_* functions live in k8s.io/kubernetes and are not importable — so the defaults cannot simply be recomputed. Two post-processing steps recover most of the gap:

1. Round-trip through the 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: [], supplementalGroups: [] — which otherwise appear as additions the upgrade does not make. (Helm's kube.Client.Build returns unstructured objects, so these survive from the chart YAML verbatim.)

2. Copy a field back from the live object when the old and the new release manifest agree about it. Values disappear from the merged object two ways:

• the patch replaces retainKeys structs (spec.strategy) and atomic lists (ports, volumeClaimTemplates) as a whole, dropping what the server had defaulted into them;
• a manifest that renders an unset value as an explicit null — a bare replicas:, which is what a chart writes for a value it leaves unset — puts the key in both manifests, so it reaches the patch as a change rather than a deletion. That deletes a defaulted value even when the chart did not change at all.

The server re-defaults both cases immediately after applying the patch. Locally, a field is restored only when nothing in either manifest asked for it to go. Where the two manifests disagree the deletion is honoured, because that is a change the chart really makes — restoring there would hide real removals (a dropped nodeSelector entry looks identical at the JSON level, and the server does not default that back). Values already present in the merged object are never overwritten, so drift between the cluster and the chart is still reported and still corrected.

Known deviation from server mode

When the manifests genuinely disagree about a field the API server defaults, client mode reports it as removed where server mode shows it changing to the default — a chart that stops pinning replicas: 3 shows the field going away rather than 3 → 1. The change is reported either way; client mode just cannot name the value that replaces it. Mutating webhooks and validation are also not applied. All of this is documented in the new README section, along with the minimal RBAC role.

Tests

manifest/generate_test.go is new. Eight TestLocalMerge_* cases are built from real manifests (unstructured, as kube.Client.Build supplies them; the live side written as the API server returns it) and cover each symptom above, plus:

• a chart that genuinely stops setting a field — still reported as removed;
• a hand-scaled replicas and hand-edited image in the cluster — still reset to the chart values, i.e. drift detection intact.

I checked the tests are not vacuous by reverting each fix independently; each fails without the step it covers. cmd/upgrade_test.go adds flag/env validation cases.

go test ./..., go vet and gofmt pass, and the README flag tables are regenerated with make readme.

Testing done

Built with make install/helm and run against a real cluster with an account lacking patch, on a release of ~15 resources (LibreChat and its subcharts). Before the two post-processing steps the client-side output differed from --three-way-merge-mode=server on defaulted fields; the cases in the tests are taken from that output.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

auto mode does not currently “fall back per run” (it keeps attempting forbidden PATCHes) and the docs/RBAC wording plus env-var handling need small corrections to match intended behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds a new three-way-merge “mode” capability so helm diff upgrade --three-way-merge can work with read-only Kubernetes credentials by falling back (or opting in) to a client-side patch application that approximates the API server’s results.

Changes:

• Introduces --three-way-merge-mode (auto/server/client) and env HELM_DIFF_THREE_WAY_MERGE_MODE, wiring it through cmd/upgrade into manifest.Generate(...) via variadic options.
• Implements client-side patch application for three-way-merge, including normalization (round-trip through Go types) and a restoration pass for server-populated/defaulted fields.
• Adds focused unit tests for local merge behavior and CLI flag/env validation; updates README and Copilot instructions.

File summaries

File	Description
README.md	Documents --three-way-merge-mode, behavior tradeoffs, and RBAC guidance.
manifest/generate.go	Adds merge-mode options and client-side patch application with post-processing to reduce diff noise.
manifest/generate_test.go	New tests covering strategic/merge patch local application and default-restoration behaviors.
cmd/upgrade.go	Adds flag/env plumbing and validation; passes merge-mode into manifest generation.
cmd/upgrade_test.go	Adds validation coverage for the new flag/env behavior.
.github/copilot-instructions.md	Documents the new environment variable.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

The new validation error messages format the allowed-mode slice with %q, which will render as %!q([]string=...) and should be corrected before merge.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

cmd/upgrade.go:199

• The format string uses %q for manifest.ValidThreeWayMergeModes (a []string), which will render as %!q([]string=...) and produce an unhelpful validation error. Use %v (or join the slice) when printing the allowed values.

This issue also appears on line 226 of the same file.

cmd/upgrade.go:228

• The env-var validation error formats manifest.ValidThreeWayMergeModes (a []string) with %q, which will print as %!q([]string=...). Use %v (or join the slice) so users see the allowed values clearly.

 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)
}

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[oreonl]

Checked this one and I don't think it holds — %q on a []string isn't malformed. fmt applies a verb recursively to the elements of a compound operand (docs: "For compound objects, the elements are printed using these rules, recursively... array, slice: [elem0 elem1 ...]"). %!q(...) only appears when the verb is invalid for the type, e.g. %q on a struct.

Actual output from the current branch, for both lines flagged:

$ helm diff upgrade rel ./chart --three-way-merge-mode local
Error: flag "three-way-merge-mode" must be one of ["auto" "server" "client"], but got "local"
$ HELM_DIFF_THREE_WAY_MERGE_MODE=local helm diff upgrade rel ./chart --three-way-merge
Error: env var "HELM_DIFF_THREE_WAY_MERGE_MODE" must be one of ["auto" "server" "client"], but got "local"

%v would print [auto server client], dropping the quotes that delimit the values — and the adjacent --server-side check quotes each one:

Error: flag "server-side" must be "true", "false" or "auto", but got "yes"

So I've left these as they are.

[Copilot]

🔵 Needs a closer look

It changes core diff computation behavior with non-trivial Kubernetes patch/defaulting semantics, so it warrants final human review despite good test coverage.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It changes core three-way-merge behavior with substantial new merge/restoration logic whose correctness and compatibility impact are difficult to fully validate via automated review alone.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[yxxhero]

@oreonl PTAL

[Copilot]

🟡 Changes recommended

Local restoration can hide real drift and CRD changes, while automatic fallback can mask admission failures.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 4
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Fallback handling and field restoration have correctness gaps, and several usage and RBAC examples are inaccurate.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (5)

Previously missed (3) — in code that hasn't changed since the last review.

README.md:276

• The release-storage requirement is understated here: the default revision is resolved by helm get manifest without --revision, which lists the Helm storage backend to find the newest release. A role with only get on Secrets/ConfigMaps therefore fails for the normal invocation; document list as required as well.

This issue also appears on line 297 of the same file.
README.md:404

• This generated example does not enable three-way merge. Since the mode environment variable is intentionally ignored unless three-way merge is already active, the command performs a normal two-way diff and does not demonstrate client-side merging.
  cmd/upgrade.go:170
• This example does not enable three-way merge. The command only reads this environment variable when HELM_DIFF_THREE_WAY_MERGE=true (or --three-way-merge/--take-ownership) is already active, so as written it performs the ordinary two-way diff rather than a local merge.

manifest/generate.go:233

• MethodNotAllowed never falls back when the authorization review says patching is allowed: isPatchNotAllowed routes it here, then this check returns false and aborts. That contradicts the new auto contract, which explicitly falls back for MethodNotAllowed. Restrict the authorization disambiguation to Forbidden; a 405 indicates the server path is unavailable regardless of RBAC.

	if f.canPatch != nil {
if allowed, err := f.canPatch(info); err == nil && allowed {
return false
}

README.md:299

• The least-privilege example cannot read the latest Helm release with the default revision. This code invokes helm get manifest without --revision (cmd/helm.go:139-158), so Helm must list the active Secret/ConfigMap storage backend to find the newest revision; granting only get causes the diff to fail before resource reads. Grant list to release storage while keeping workload kinds at get.

 - apiGroups: [""]
resources: ["configmaps", "secrets", "services", "serviceaccounts"]
verbs: ["get"]

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Balanced

[oreonl]

All five points from this review are addressed in d8d73ff. The four suppressed ones have no threads, so answering them here.

MethodNotAllowed aborting instead of falling back (manifest/generate.go:233) — correct, and a regression from the SelfSubjectAccessReview I added in ca059ad: the review ran for every refusal isPatchNotAllowed accepts, so a 405 against credentials that may patch was surfaced rather than falling back. Reproduced (patchDenied(405, canPatch=true) returned false) and fixed by restricting the disambiguation to Forbidden — a 405 says the API server takes no patch for that resource whoever is asking, so entitlement changes nothing. Covered by TestClientSideFallbackAlwaysFallsBackOnMethodNotAllowed.

Release storage needs list (README.md:276, :297, :299) — confirmed in the code: helmGetArgs omits --revision when the revision is 0 (cmd/helm.go:141-145), and helm's Secrets driver Query does a List to find the newest revision, so get alone fails before any resource is read. The least-privilege example now grants get, list on configmaps/secrets for the release storage and keeps the rendered kinds at get, with the reason spelled out in the surrounding prose.

The env var examples did not enable the three-way merge (README.md:404, cmd/upgrade.go:170) — right, and self-inflicted: making HELM_DIFF_THREE_WAY_MERGE_MODE deliberately ignored unless the merge is on invalidated my own examples. Both now pass --three-way-merge. Worth flagging for future edits that scripts/gen-readme.sh regenerates only the flag tables, so the README's copy of the example block is hand-maintained and had drifted from the binary; I diffed the two to confirm they match again.

The remaining inline comment, about live-only drift inside a replaced atomic list, is answered on its own thread. I have left that one open, since it is a design trade-off rather than something I have fixed.

For what it is worth while the checks are still pending: golangci-lint v2.12.2 against Go 1.26.4 — the pairing lint.yaml pins via go-version-file — reports 0 issues, and go test ./... passes.

[Copilot]

🔵 Needs a closer look

Local normalization can discard newer Kubernetes fields, while explicit-null restoration can hide real drift corrections.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

manifest/generate.go:365

• Decoding into the compiled Go type silently discards fields unknown to this client-go version. If helm-diff runs against a newer cluster and a chart adds a newly supported field to an otherwise known built-in kind, the local merge contains the field here, this round-trip removes it, and the restoration pass cannot recover it because it is absent from the live object. Client mode then omits a real upgrade change; preserve unknown fields from the merged object during normalization or avoid typed normalization when the typed schema does not cover the object.

manifest/generate.go:474

• This treats an explicit null exactly like a missing key, but null can be the chart-owned desired value for fields that are not defaulted. For example, if both manifests render runtimeClassName: null while the live Pod template drifted to runtimeClassName: gvisor, the three-way patch clears the field and normalize omits it; this condition then restores gvisor, hiding a correction that the real upgrade makes. Use presence-aware lookups and restrict null restoration to fields known to be server-defaulted rather than restoring every explicit-null field.

 if originalMap[key] == nil && modifiedMap[key] == nil {
mergedMap[key] = liveValue

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Balanced