feat(platform): add deletion-protection guardrail via ValidatingAdmissionPolicy - #2650

Code Review

This pull request implements a deletion protection mechanism for critical system resources by introducing a ValidatingAdmissionPolicy that blocks the deletion of objects labeled with platform.cozystack.io/no-delete: "true". The label is applied across various components, including CRDs, namespaces, and cluster issuers. Review feedback recommends wrapping the policy in a capability check to maintain compatibility with Kubernetes versions prior to 1.30 and suggests applying the protection label to the guardrail resources themselves to prevent their accidental deletion.

Actionable comments posted: 1

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/core/platform/templates/cozystack-version.yaml`:
- Around line 9-10: Existing cozystack-version ConfigMaps aren’t backfilled
because the current template only renders when lookup() returns nothing; add an
upgrade-time render/patch path that runs when
lookup("v1","ConfigMap","cozystack-version") returns a result and applies the
label platform.cozystack.io/no-delete: "true" to the existing object.
Concretely, add a second manifest conditional (the inverse of the current lookup
check) that emits a patch/ConfigMap (or Kubernetes strategic-merge patch)
targeting metadata.name: cozystack-version and sets
metadata.labels.platform.cozystack.io/no-delete = "true" so upgrades will
backfill the label for existing resources.

Actionable comments posted: 1

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/core/platform/images/migrations/migrations/40`:
- Line 1: The script uses bash-specific "set -o pipefail" but currently has a
POSIX shebang (#!/bin/sh); update the shebang to a bash interpreter (e.g.
#!/usr/bin/env bash or #!/bin/bash) so "set -o pipefail" works and the pipeline
around the kubectl create commands (the pipeline invoked near lines with kubectl
create at lines ~30-32) correctly fails on errors; ensure the script header is
changed in the migration file (the current first line with "#!/bin/sh") to the
bash shebang so functions/commands relying on bash semantics run reliably.

NOT LGTM

Reviewing against main. 4 commits on top of e4f9a18.

Scope: adds a ValidatingAdmissionPolicy (+Binding) that blocks DELETE on objects labeled platform.cozystack.io/no-delete=true, stamps the label on a fixed set of platform objects (CRDs, namespaces, ConfigMap, Repository, ClusterIssuers, LinstorCluster, tenant-root HelmRelease, the VAP/Binding themselves), bumps targetVersion 40 → 41, and adds migration 40 to backfill the label on the pre-existing cozystack-version ConfigMap.

The architecture is sound (in-process CEL, objectSelector-scoped binding, self-protection via labeling the VAP/Binding, capability gate for 1.30+, migration backfill for the one resource helm can't re-render). The blockers below are about untested behavior on the Go side, a fragile one-shot pattern in the migration script, and missing test coverage for every new behavior the PR introduces.

Blockers

1. internal/crdinstall/install.go — label-stamping has no test, and the label-merge contract is unasserted

internal/crdinstall/install.go:92-101:

// Stamp the deletion-protection label so the platform VAP guards every
// Cozystack CRD against accidental kubectl delete.
for _, obj := range objects {
    labels := obj.GetLabels()
    if labels == nil {
        labels = make(map[string]string)
    }
    labels["platform.cozystack.io/no-delete"] = "true"
    obj.SetLabels(labels)
}

The Patch call below uses client.Apply (SSA) with FieldManager: "cozystack-operator" and Force: true. SSA semantics: every field set by this field manager is owned by it; fields ABSENT from this apply that were previously owned by the same field manager get removed from the object.

The code is fine in itself — obj.GetLabels() preserves whatever the manifest YAML carried, and the new key is merged in. But internal/crdinstall/install_test.go does not assert the label is actually present on the applied object. TestInstall_appliesAllCRDs only checks that Install returns no error; it does not read the applied CRD back from the fake client and inspect labels. If a future refactor drops the loop, the test stays green and the protection is silently gone.

Required fix:

• Extend TestInstall_appliesAllCRDs (or add a new test) to read the applied CRDs back from the fake client after Install returns and assert labels["platform.cozystack.io/no-delete"] == "true" on each.
• Add a second test variant where the input manifest YAML already carries other labels (e.g. app.kubernetes.io/managed-by: foo) and assert both labels survive — exercises the GetLabels() == nil branch AND the merge path.

Test that fails without the fix: a unit test that builds Install with a manifest YAML carrying app.kubernetes.io/managed-by: foo, runs Install against a fake client, then fakeClient.Get for the CRD and asserts both app.kubernetes.io/managed-by == foo AND platform.cozystack.io/no-delete == true.

2. packages/core/platform/images/migrations/migrations/40 — no test, and the applied manifest does not carry the label

packages/core/platform/images/migrations/migrations/40:26-32:

kubectl label --namespace "$NAMESPACE" --overwrite configmap cozystack-version \
  platform.cozystack.io/no-delete=true
echo "Labeled configmap $NAMESPACE/cozystack-version"

kubectl create configmap --namespace "$NAMESPACE" cozystack-version \
  --from-literal=version=41 --dry-run=client --output yaml \
  | kubectl apply --filename -

The script first stamps the label imperatively, then re-applies the ConfigMap to bump data.version=41. The applied YAML carries no labels — the script relies on kubectl apply's 3-way strategic merge to leave the imperatively-set label alone. This DOES work in the steady state (the label is never in last-applied-configuration, so 3-way merge has no claim to remove it), BUT the contract is fragile and inconsistent with the helm template at packages/core/platform/templates/cozystack-version.yaml, which bakes the label inline.

Required fix: replace the imperative-label-then-apply dance with a single declarative apply that includes the label, matching the helm template:

cat <<EOF | kubectl apply --filename -
apiVersion: v1
kind: ConfigMap
metadata:
  name: cozystack-version
  namespace: ${NAMESPACE}
  labels:
    platform.cozystack.io/no-delete: "true"
data:
  version: "41"
EOF

One source of truth, no last-applied-merge subtlety.

Test that fails without the fix: a bats test (the project already runs bats-unit-tests) that creates a cozy-system/cozystack-version ConfigMap with data.version=40 and no label, runs the migration script, and asserts the resulting ConfigMap has data.version=41 AND metadata.labels["platform.cozystack.io/no-delete"] == "true". Without this, any future migration that forgets the imperative kubectl label step ships green.

3. packages/core/platform/templates/deletion-protection.yaml — the VAP itself has no test coverage

There is zero automated coverage of the actual contract this PR delivers: "a DELETE on an object carrying platform.cozystack.io/no-delete=true is denied with the documented message." This is the e2e-shaped test — a unit test that greps the template is the wrong shape, but an integration test against a real apiserver IS the right shape.

The repo already has packages/core/testing/ with make test. Add a test there that:

1. Installs the platform chart on a 1.30+ cluster.
2. Asserts kubectl delete configmap cozystack-version --namespace cozy-system exits non-zero AND the error message includes "Deletion blocked: object carries platform.cozystack.io/no-delete=true".
3. Runs kubectl label configmap cozystack-version --namespace cozy-system platform.cozystack.io/no-delete-, then kubectl delete, and asserts that succeeds.

Catches every regression the PR is meant to prevent: capability gate inverted, binding objectSelector mistyped, validationActions flipped from Deny to Warn, expression flipped from false to true, label key drift between the binding and the manifests.

Test that fails without the fix: the test above.

4. Documentation drift — the bypass instruction omits the namespace flag

• packages/core/platform/templates/deletion-protection.yaml:11
• packages/core/platform/templates/deletion-protection.yaml:33-35 (CEL message)

Bypass: remove the label, then delete.
  kubectl label <kind> <name> platform.cozystack.io/no-delete-

The bypass command is missing --namespace <ns> for namespaced resources. Of the protected set, several are namespaced:

• cozystack-version ConfigMap (cozy-system)
• cozystack-packages Repository (cozy-system)
• tenant-root HelmRelease (tenant-root)

If the operator runs the documented bypass against their default context, kubectl returns "configmap cozystack-version not found" and they think the bypass mechanism is broken. The CEL message (the thing the user sees the moment their delete is denied) has the same bug.

Required fix: change both the template comment and the CEL message to show the namespace flag, e.g.:

To bypass, first remove the label:
  kubectl label <kind> <name> --namespace <ns> platform.cozystack.io/no-delete-
  (omit --namespace for cluster-scoped resources)

Test that fails without the fix: a helm-template test that asserts the rendered VAP's spec.validations[0].message contains the literal --namespace. Yes, this is content-on-template testing — justified here because the contract is user-facing help text that is otherwise un-asserted.

5. Recommended to fix — failurePolicy: Fail is dead config for a constant CEL expression

packages/core/platform/templates/deletion-protection.yaml:22

The CEL expression is the literal false — it cannot fail to evaluate (no parameters, no variables, no resolved references). failurePolicy: Fail is harmless but misleading. Either drop the field (it defaults to Fail in admissionregistration.k8s.io/v1) or add a comment noting it's redundant.

Test that fails without the fix: none — readability nit.

Reminder: "Pre-existing" is not a thing. If a problem is flagged in this review, it must be fixed in this PR.

Every issue listed above must have a corresponding test. Untested fixes are not fixes.

Independent second-look — concurring with Timofei Larkin (@lllamnyp)'s NOT LGTM. Adding three observations that don't duplicate the existing blockers, plus a disproven hypothesis worth recording so it doesn't come back later.

Concurring: Timofei Larkin (@lllamnyp)'s four blockers (Go-side label test, migration's imperative-label-then-apply, missing VAP integration test, missing --namespace in the bypass instruction) all still apply on b6b0d8a8. The only change since 2026-05-18 is a main merge — none of the blockers are addressed. The migration in question moved from migrations/40 to migrations/42 (to land after #2454 → 41 and #2694 → 40), but the same imperative-label-then-declarative-apply pattern carries over verbatim into migrations/42:26-32. Fix lllamnyp prescribed (single declarative apply with the label baked in) is unchanged.

Independent additions:

1. helm uninstall of the platform chart is now a bricked operation. Once any labeled object is in the cluster — and this PR labels CRDs, ClusterIssuers, the cozy-system Namespace, the tenant-root Namespace + HelmRelease, cozystack-version ConfigMap, cozystack-packages Repository, LinstorCluster, plus the VAP/Binding themselves — helm uninstall will issue DELETE on each, and DELETE is denied for the labeled ones. There is no documented teardown procedure. For development clusters and disaster recovery this matters. Recommendation: ship a sibling migration (or a documented make uninstall recipe) that strips the label off the protected set first, then uninstalls. Otherwise every developer hitting helm uninstall cozy-platform to retry an install gets a half-unwound chart, the VAP staying around, and CRDs they can't remove without manual kubectl label. This is not lllamnyp's documentation-drift blocker — that one was about the per-object bypass message; this one is about the missing teardown story for the whole chart.

2. Capability gate gives no visibility when it skips. On a pre-1.30 cluster Capabilities.APIVersions.Has "admissionregistration.k8s.io/v1/ValidatingAdmissionPolicy" evaluates to false and the whole template renders to nothing — no warning, no hint in helm install output. Operators on older clusters silently lose the guardrail and have no way to discover that from the install log. Either render a NOTES.txt warning when the gate fails, or add a chart-level value (e.g. deletionProtection.requireSupport: true) that fails the install instead of skipping silently. This compounds with Timofei Larkin (@lllamnyp)'s blocker 3 (no integration test): if someone runs this on K8s 1.28 by accident, every other manifest applies, the chart "succeeds", and the guardrail is silently absent.

3. Self-protection bypass via Binding-first deletion needs to be acknowledged. The VAP and Binding both carry the no-delete label, so each individually requires the bypass dance. But the order matters: removing the label from the Binding and deleting it first leaves the VAP labeled but with no enforcement → VAP becomes deletable. Removing the label from the VAP first works too. Either order works, and that's by design (the guardrail is operational, not adversarial). Worth a one-liner in the deletion-protection.yaml comment header so a future reader doesn't waste time wondering why the symmetric self-protection isn't truly bilateral. Not a blocker.

Disproven hypothesis (recording so it doesn't get re-raised): an automated reviewer flagged matchConstraints.resourceRules[0].resources: ["*/*"] as supposedly matching only subresources, suggesting the guardrail never fires for top-level deletes like kubectl delete configmap. This is incorrect. Verified against k8s.io/apiserver/pkg/admission/plugin/webhook/predicates/rules/rules.go (Matcher.resource() + splitResource()): */* splits into res="*", sub="*", and the operation's empty subresource matches the sub=="*" wildcard branch. Pattern ["*"] would be the bug (matches only when opSub is empty AND requires explicit subresources to be enumerated separately). ["*/*"] is the correct "everything" form. The current template is right on this dimension.

Leaving the verdict at Timofei Larkin (@lllamnyp)'s NOT LGTM — this review is intended to add context, not to flip the gate. Posting as COMMENT rather than another REQUEST_CHANGES to avoid stacking duplicate change-requests on the same PR (cozystack branch protection treats latest review per author as authoritative, but multiple maintainers asking for the same fixes inflates the noise without changing the gate).

Actionable comments posted: 2

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/core/platform/hack/unprotect.sh`:
- Around line 45-61: The script currently masks all kubectl failures by
appending "2>/dev/null || true" to the `kubectl get` calls (used when building
`names` and `pairs`), which hides real errors; update both places (the
`names=$(kubectl get "$kind" ... )` block and the `pairs=$(kubectl get "$kind"
--all-namespaces ... )` block) to capture the kubectl exit status and error
output, allow continuing only when the error indicates the resource kind is
unknown (e.g., error text like "the server doesn't have a resource type" or
similar), but for any other non-zero exit code print the kubectl stderr and exit
non-zero (or skip that kind after logging) so RBAC/API/context failures are not
silenced; ensure you still suppress stdout when continuing for unknown kinds and
keep using variables LABEL, NS_KINDS, names, pairs and the existing loops.

In `@packages/core/platform/images/migrations/migrations/42`:
- Line 16: The migration script uses a POSIX shell shebang (#!/bin/sh) but sets
non-portable pipefail in the line containing "set -euo pipefail"; change this to
a portable form by removing pipefail (use "set -eu") or, if you rely on pipefail
semantics, change the shebang to a Bash interpreter (e.g., bash) so "pipefail"
is supported; update the line containing "set -euo pipefail" and/or the shebang
accordingly to ensure the migration runs on systems where /bin/sh is non-Bash.

LGTM — every blocker from my prior comment review and from Timofei Larkin (@lllamnyp)'s NOT LGTM has a dedicated fix commit. Walking through each:

Timofei Larkin (@lllamnyp) B1 (Go test for install.go label stamping): 84c2aaacd test(crdinstall): assert no-delete label survives Install + merge. TestInstall_appliesAllCRDs now reads CRDs back through the fake client and asserts the label is present. TestInstall_preservesExistingLabels exercises the merge path with a pre-existing app.kubernetes.io/managed-by: foo and verifies both labels survive. Covers the exact regression vector lllamnyp called out — drop the loop, the test fails.

Timofei Larkin (@lllamnyp) B2 (migration imperative-label-then-apply): 572e08a32 fix(platform): migration 42 uses a single declarative apply. The heredoc-built manifest now bakes the label inline; one declarative apply, no 3-way-merge subtlety, idempotent. Matches the helm template at templates/cozystack-version.yaml so the steady-state source of truth is unified.

Timofei Larkin (@lllamnyp) B3 (VAP integration test): 6782761a2 test(platform): e2e check that no-delete VAP denies + bypass works adds the bats e2e covering capability-skip on pre-1.30, label-present precondition, deny-path with the documented message including --namespace, and the round-trip bypass + restore. 23c8dab58 adds tests/deletion_protection_test.yaml for the helm-unittest layer (self-label on both VAP and Binding, message regex, binding selector contract, failurePolicy absence). Both layers cover every regression vector lllamnyp enumerated (binding objectSelector mistyped, validationActions flipped Deny→Warn, expression false→true, label-key drift).

Timofei Larkin (@lllamnyp) B4 (--namespace missing in bypass): 23c8dab58 — both the template comment and the CEL message now include kubectl label <kind> <name> --namespace <ns> platform.cozystack.io/no-delete- with an explicit hint about omitting --namespace for cluster-scoped objects. helm-unittest pins both substrings via matchRegex.

Timofei Larkin (@lllamnyp) Nit (failurePolicy redundant): 602685743 fix(platform): drop redundant failurePolicy on no-delete VAP. Field is gone from the spec and a comment in the template header explains why (CEL expression is the literal false, evaluation cannot fail). helm-unittest asserts notExists: spec.failurePolicy.

My N1 (helm uninstall bricked): bc753f676 feat(platform): make unprotect strips no-delete label for teardown + 67f7934e8 fix(platform): do not swallow kubectl errors in unprotect.sh. The make unprotect recipe runs hack/unprotect.sh, which iterates over the cluster-scoped guarded kinds (CRD, ClusterIssuer, Namespace, VAP, Binding, LinstorCluster) and the namespaced ones (ConfigMap, OCIRepository, HelmRelease) and strips the label. Critically, kubectl_get_or_skip_unknown_kind tolerates only "unknown resource type" (so LinstorCluster on non-LINSTOR clusters doesn't fail the script) and is fatal on any other kubectl error (RBAC, wrong context, API unreachable) — exactly the behavior a teardown helper needs to avoid silent partial unprotection. The follow-up commit also keeps stdout (the resource list) and stderr (warnings/errors) separate so unrelated kubectl warnings can't sneak into the loop variable. Solid.

My N2 (capability gate silent skip): c1ac16e38 feat(platform): NOTES.txt warns when no-delete VAP is silently skipped. helm output now ends with a WARNING block on pre-1.30 clusters explaining that the policy was not rendered, the label is still on objects, and that an upgrade to 1.30+ followed by helm upgrade activates the guardrail. tests/notes_capability_gate_test.yaml exercises both branches.

My N3 (sequential self-protection): 57f7f50cb docs(platform): clarify sequential self-protection on no-delete VAP. Template header now spells out that the label on VAP + Binding is operational guidance, not adversarial defense, and that either one can be unlabeled and deleted first — once the Binding is gone the VAP is unenforced, and vice versa. References make unprotect for the whole-chart path.

One operational note before merge: mergeStateStatus is DIRTY — two conflicts with origin/main:

• packages/system/cert-manager-issuers/templates/cluster-issuers.yaml
• packages/system/cozystack-basics/templates/tenant-root.yaml

Both are files this PR adds the no-delete label to; main received unrelated changes to the same templates. Rebase, then this PR is good to land. CI is green on the current SHA (Build / E2E / pre-commit / Verify generated all pass).

This APPROVE supersedes my prior COMMENT (#4358008995). Timofei Larkin (@lllamnyp)'s CHANGES_REQUESTED from the previous SHA still stands as the gate — cozystack stale-review auto-dismiss is off, so a fresh look from him is needed to unblock the merge.

NOT LGTM

Reviewing against target branch: main.

Scope note

The branch merged origin/main (b6b0d8a83), so the raw merge-base diff drags in unrelated work (etcd backup-strategy controller, scorecard, maintainers, postgres). I reviewed only the deletion-protection feature: the VAP/Binding template, cozystack-version.yaml/repository.yaml/NOTES.txt, migration 42, unprotect.sh, the targetVersion bump, the helm unittests, internal/crdinstall/install.go + test, the e2e bats deny check, and the no-delete label additions across tenant-root/cluster-issuers/linstor/installer namespace.

What I verified works: the VAP design is correct (on DELETE the Binding's objectSelector matches the oldObject's labels, so labeled objects are denied; broad matchConstraints + narrow objectSelector is the right split). go test ./internal/crdinstall/... passes. helm unittest packages/core/platform passes 18/18. The VAP matches only DELETE, so helm/flux updates and the migration's own apply are not blocked.

Blocking issues

1. unprotect.sh still swallows kubectl label failures — the exact bug the last commit claimed to fix

packages/core/platform/hack/unprotect.sh:78-82 and :89-92. Commit 67f7934e8 made the kubectl get path fatal via the helper, but the actual mutating kubectl label ... - calls run inside printf | while read pipelines. Under set -eu with no pipefail (dash on CI), a non-zero exit inside a piped while subshell does not abort the script. A failed label removal (RBAC, transient API error) is swallowed and the script still prints "Done. helm uninstall is now unblocked." and exits 0 — leaving the object protected. That is the "report success while leaving labels behind" failure the helper comment calls fatal; it's only guarded on the read path. Fix: iterate without the pipe-subshell (for ref in $names; do kubectl label "$ref" "$LABEL-"; done), or track an rc and exit 1 if any label call failed. Test: a shim where kubectl label exits 1 for one object; assert the script exits non-zero and doesn't print "unblocked".

2. unprotect.sh doesn't cover a GitRepository source

unprotect.sh:63-67 lists ocirepository but not gitrepository. repository.yaml labels a resource of kind {{ $sourceRef.kind }} (configurable; source.toolkit.fluxcd.io/v1 also serves GitRepository). On a GitRepository-sourced cluster the label is never stripped and helm uninstall stays denied, with the script exiting 0. Fix: add gitrepository to NS_KINDS or derive the kind from sourceRef.kind. Test: run against a fixture whose only labeled source is a GitRepository, assert the label is removed.

Recommended (non-blocking)

• Runner silently skips missing migrations. run-migrations.sh:27-38 prints "Migration N not found, skipping" and continues when a migration file below TARGET_VERSION is absent. In a shipped release this is harmless — the release build (make image-migrations) rebuilds the migrations image from the same source tree that carries the migration files and restamps the digest in the same commit, so image and targetVersion are always consistent at release time; the digest in values.yaml is a generated artifact, never hand-set in feature PRs. But hardening the runner to hard-error on a missing migration below TARGET_VERSION (instead of skipping) would turn a future packaging mistake into a loud failure rather than a silent version stall. Out of scope for this PR (it doesn't touch the runner), noting for a follow-up.
• Migration 42's "no 3-way-merge subtlety" comment is slightly overstated (prior migrations stamp the version with a label-less apply, so a client-side last-applied interaction does exist across migrations) — correct in outcome, not worth blocking.
• The e2e bypass-path teardown re-applies cozystack-version without the helm.sh/resource-policy: keep annotation the chart bakes in, so it doesn't fully restore cluster state.
• cluster-issuers.yaml has pervasive trailing whitespace on lines this PR touched; a sed cleanup pass would be welcome.

Reminder: "Pre-existing" is not a thing. If a problem is flagged in this review, it must be fixed in this PR — no exceptions.

Every issue listed above must have a corresponding test. No matter the severity — bug, logic error, security issue, error handling gap, documentation drift — each one must be covered by a test that fails without the fix and passes with it. Untested fixes are not fixes.

Sources for the VAP DELETE/objectSelector behavior I verified: ValidatingAdmissionPolicyBinding | Kubernetes, Validating Admission Policy | Kubernetes.

LGTM on 76fd5d179. Walked through Timofei Larkin (@lllamnyp)'s second-round review against this SHA: blocker B2 + all 4 recommended items are landed; blocker B1 is empirically a false positive — the pipe-subshell does abort under set -eu, against the migrations image's actual shell (busybox ash on alpine 3.23).

Timofei Larkin (@lllamnyp) blocker check

B1 (unprotect.sh pipe-subshell allegedly swallows kubectl label failures) — DISPROVEN

The claim was: "under set -eu with no pipefail (dash on CI), a non-zero exit inside a piped while subshell does not abort the script. A failed label removal ... is swallowed and the script still prints 'Done. helm uninstall is now unblocked.' and exits 0."

Tested against the four interpreters the script could meet on the rendered surface:

# alpine 3.23 (migrations image), shell is BusyBox v1.37.0 ash
$ docker run --rm alpine:3.23 sh -c '
    set -eu
    printf "a\nb\nc\n" | while read x; do
      counter=$((${counter:-0} + 1))
      if [ "$counter" = "2" ]; then
        echo "kubectl label failed on iteration $counter" >&2
        false
      else
        echo "labeled object iteration $counter"
      fi
    done
    echo "still alive (BAD)"'
# → labeled object iteration 1
# → kubectl label failed on iteration 2
# → exit 1, "still alive" NEVER prints

Same result on dash (POSIX strict), on bash without pipefail, and on bash with pipefail. Mechanism: set -e inside the while-subshell triggers on the first non-zero (false or kubectl label's exit code), the subshell aborts with that code, the pipeline's last command is the subshell so the pipe exits non-zero, and the outer shell's set -e then propagates. POSIX rule "set -e is ignored in command lists immediately following while/until" applies to the condition, not the body — man bash is explicit. So the unprotect.sh structure on this SHA does fail loud on a kubectl label error rather than silently exiting 0.

There's a narrow scenario where lllamnyp's claim would be true — if the kubectl label line were on the last logical iteration AND the pipeline's exit code were captured into a variable ($(... ) or || true) — but the current script does neither. The bare printf | while read; do ... ; done form propagates correctly.

B2 (unprotect.sh missing gitrepository) — ADDRESSED in 76fd5d179

packages/core/platform/hack/unprotect.sh:64-72 now lists gitrepository alongside ocirepository under NS_KINDS, with the new comment correctly attributing it to repository.yaml templating kind: {{ $sourceRef.kind }} unrestricted. On a GitRepository-sourced cluster, helm uninstall is no longer left denied.

Timofei Larkin (@lllamnyp) recommended items

1. Runner silently skipped missing migrations — run-migrations.sh:25-44 now hard-errors with "Migration $i not found in image — refusing to advance past a missing migration" and exit 1 instead of skipping. Daniil went above the recommended bar — actually flipped the default from "silent skip" to "loud fail" with a release-build-consistency comment explaining why. ✓
2. Migration 42 "no 3-way-merge subtlety" comment overstated — comment now clarifies the actual constraint: future migrations re-stamping cozystack-version under the same field manager MUST include the label inline. ✓
3. e2e bypass-path teardown missed helm.sh/resource-policy: keep — hack/e2e-install-cozystack.bats:340-356 now bakes the annotation into the reconstruct apply, restoring the same shape the chart template produces. ✓
4. Trailing whitespace in cluster-issuers.yaml — not addressed in 76fd5d179. Pure cosmetic, not a blocker.

Net

This APPROVE supersedes my prior APPROVE (#4361721935) that was auto-dismissed during the rebase, and addresses the new gate that Timofei Larkin (@lllamnyp) added on 67f7934e — B2 + recommended items landed in 76fd5d179, and B1 doesn't reproduce against the actual shells in play.

Gate-wise: Timofei Larkin (@lllamnyp)'s CHANGES_REQUESTED is still active on the prior SHA, and cozystack auto-dismiss is off — needs a fresh look from him to unblock the merge.

LGTM

The deletion-protection feature is sound and well-tested: the VAP design is correct (broad matchConstraints on DELETE narrowed by the binding's objectSelector on platform.cozystack.io/no-delete), helm unit tests pass 24/24, go test ./internal/crdinstall/... passes, and there's a real e2e deny+bypass test wired into CI via make -C packages/core/testing install-cozystack.

The one blocker from the prior round — a duplicate test: target in packages/core/platform/Makefile that made GNU Make print overriding recipe warnings on every make test (and thus in helm-unit-tests CI output) — is fixed in 3bac5caac. make -C packages/core/platform -n test now runs clean.

Non-blocking observations (author's discretion)

• unprotect.sh doesn't cover all sourceRef.kind values. repository.yaml templates kind: {{ $sourceRef.kind }} unrestricted, but unprotect.sh's NS_KINDS lists only ocirepository/gitrepository. source.toolkit.fluxcd.io/v1 also admits HelmRepository and Bucket. Default is OCIRepository so it's fine in practice, but a non-default sourceRef.kind would leave that object labeled and block helm uninstall. Deriving the kind from sourceRef.kind would close the gap.
• e2e recovery fragility on retry. The new bats test strips the label, deletes cozystack-version, then reconstructs it, and the CI step wraps the file in an until attempt<3 retry. An abort between strip and reconstruct leaves the grep -qx 'true' precondition failing on retry with no recovery. Low-risk (likely abort points are assertions that already recorded the failure), but a teardown()/trap re-stamp would make the retry meaningful.

Neither blocks merge.