internal/fluxinstall/manifests.embed.go (1)

44-44: Consider using more restrictive file permissions.

The file permission 0666 allows world-writable access. For manifest files, 0644 (owner read/write, group/others read-only) is more appropriate and follows the principle of least privilege.

🔎 Suggested fix

-		if err := os.WriteFile(outputPath, data, 0666); err != nil {
+		if err := os.WriteFile(outputPath, data, 0644); err != nil {

packages/core/flux-aio/Makefile (1)

15-15: Add PHONY declaration for the update target.

Per static analysis, the update target should be declared as .PHONY since it doesn't produce a file with that name.

🔎 Suggested fix

Add at the top of the file or before the update target:

.PHONY: show apply diff update update-old update-new

internal/fluxinstall/install.go (2)

184-186: Hardcoded sleep for CRD registration may be unreliable.

A fixed 2-second wait may be insufficient under load or too long in fast environments. Consider polling for CRD readiness or using a retry mechanism with exponential backoff.

---

63-75: Error variable shadowing in fallback YAML discovery.

The err variable on line 65 shadows the outer err from line 63. While functionally correct here, this pattern can cause confusion. Also, if fluxcd.yaml doesn't exist due to an error other than "not found," the code silently falls back.

🔎 Suggested fix

 	manifestPath := filepath.Join(manifestsDir, "fluxcd.yaml")
-	if _, err := os.Stat(manifestPath); err != nil {
+	if _, statErr := os.Stat(manifestPath); statErr != nil {
 		// Try to find any YAML file if fluxcd.yaml doesn't exist
-		entries, err := os.ReadDir(manifestsDir)
-		if err != nil {
+		entries, readErr := os.ReadDir(manifestsDir)
+		if readErr != nil {
 			return fmt.Errorf("failed to read manifests directory: %w", err)
 		}

internal/operator/package_reconciler.go (2)

182-182: Use time.Duration constants for readability.

The raw nanosecond calculation 5 * 60 * 1000000000 is error-prone. Use time.Minute for clarity.

🔎 Suggested fix

+import "time"
+
 			Spec: helmv2.HelmReleaseSpec{
-				Interval: metav1.Duration{Duration: 5 * 60 * 1000000000}, // 5m
+				Interval: metav1.Duration{Duration: 5 * time.Minute},

---

829-886: Watch handler lists all Packages on every Package change.

This EnqueueRequestsFromMapFunc iterates all Packages and their PackageSources on every Package event. With N packages, this is O(N) per event, potentially O(N²) overall. Consider using an index or caching the dependency graph.

For controller-runtime, you can use field indexers to efficiently query dependent packages:

// In SetupWithManager, add an indexer
mgr.GetFieldIndexer().IndexField(ctx, &cozyv1alpha1.Package{}, "spec.dependsOn", func(obj client.Object) []string {
    // Return package names this package depends on
})

cmd/cozystack-operator/main.go (2)

231-293: Consider extracting common resource apply logic.

installFluxResourcesFunc and installPlatformSourceResource share similar create-or-update logic for unstructured objects. Consider extracting a helper to reduce duplication.

Also applies to: 297-358

---

460-481: GitRepository ref always uses branch, but ref could be a tag or commit.

The generateGitRepository function always sets ref.branch, but the parsed ref from the URL could be a tag or commit SHA. Consider detecting the ref type.

🔎 Suggested enhancement

 func generateGitRepository(name, repoURL, ref string) (*unstructured.Unstructured, error) {
+	refSpec := map[string]interface{}{}
+	// Detect if ref looks like a commit SHA (40 hex chars) or semver tag
+	if len(ref) == 40 && isHexString(ref) {
+		refSpec["commit"] = ref
+	} else if strings.HasPrefix(ref, "v") && strings.Contains(ref, ".") {
+		refSpec["tag"] = ref
+	} else {
+		refSpec["branch"] = ref
+	}
+
 	spec := map[string]interface{}{
 		"interval": "5m0s",
 		"url":      repoURL,
-		"ref": map[string]interface{}{
-			"branch": ref,
-		},
+		"ref":      refSpec,
 	}

internal/operator/packagesource_reconciler.go (1)

309-364: Generic createOrUpdate contains type-specific logic.

The createOrUpdate method has special handling for ArtifactGenerator embedded within it (lines 347-361). This reduces reusability. Consider either making it ArtifactGenerator-specific or extracting the type-specific logic.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

cmd/cozystack-operator/main.go (3)

89-89: Unused cozystackVersion flag.

The cozystackVersion flag is parsed but never used. Consider logging it during startup or removing the flag if not needed.

🔎 Suggested fix

 	setupLog.Info("Starting controller manager")
+	setupLog.Info("Cozystack version", "version", cozystackVersion)

---

263-293: Consider server-side apply for consistency.

The Get/Create/Update pattern here is not atomic. Since internal/fluxinstall/install.go already uses server-side apply, consider using the same approach for consistency and robustness.

🔎 Server-side apply pattern

-		// Use server-side apply or create/update
-		existing := &unstructured.Unstructured{}
-		existing.SetGroupVersionKind(obj.GroupVersionKind())
-		key := client.ObjectKey{
-			Name:      obj.GetName(),
-			Namespace: obj.GetNamespace(),
-		}
-
-		err := k8sClient.Get(ctx, key, existing)
-		if err != nil {
-			if client.IgnoreNotFound(err) == nil {
-				// Resource doesn't exist, create it
-				if err := k8sClient.Create(ctx, &obj); err != nil {
-					return fmt.Errorf("failed to create resource %s/%s: %w", obj.GetKind(), obj.GetName(), err)
-				}
-				logger.Info("Created Flux resource", "kind", obj.GetKind(), "name", obj.GetName())
-			} else {
-				return fmt.Errorf("failed to check if resource exists: %w", err)
-			}
-		} else {
-			// Resource exists, update it
-			obj.SetResourceVersion(existing.GetResourceVersion())
-			if err := k8sClient.Update(ctx, &obj); err != nil {
-				return fmt.Errorf("failed to update resource %s/%s: %w", obj.GetKind(), obj.GetName(), err)
-			}
-			logger.Info("Updated Flux resource", "kind", obj.GetKind(), "name", obj.GetName())
-		}
+		// Use server-side apply
+		if err := k8sClient.Patch(ctx, &obj, client.Apply, client.FieldOwner("cozystack-operator")); err != nil {
+			return fmt.Errorf("failed to apply resource %s/%s: %w", obj.GetKind(), obj.GetName(), err)
+		}
+		logger.Info("Applied Flux resource", "kind", obj.GetKind(), "name", obj.GetName())

---

376-378: Use strings.LastIndex for robust digest parsing.

The comment correctly states that LastIndex should be used to handle paths with @ symbols, but the code uses strings.Index. This could incorrectly split the URL if the repository path contains @sha256:.

🔎 Proposed fix

-		sha256Idx := strings.Index(rest, "@sha256:")
+		sha256Idx := strings.LastIndex(rest, "@sha256:")

internal/operator/packagesource_reconciler.go (2)

87-103: Library name collision across variants.

If two variants define libraries with the same name but different paths, the last one overwrites the first without warning. This could cause unexpected behavior.

🔎 Suggested fix: log collision warning

 	for _, variant := range packageSource.Spec.Variants {
 		for _, lib := range variant.Libraries {
 			libName := lib.Name
 			if libName == "" {
 				libName = r.getPackageNameFromPath(lib.Path)
 			}
 			if libName != "" {
+				if existing, ok := libraryMap[libName]; ok && existing.Path != lib.Path {
+					logger.Info("library name collision detected, overwriting",
+						"libraryName", libName,
+						"existingPath", existing.Path,
+						"newPath", lib.Path,
+						"variant", variant.Name)
+				}
 				libraryMap[libName] = lib
 			}
 		}
 	}

---

309-364: Consider server-side apply for atomic updates.

The Get/Create/Update pattern is not atomic and can lead to race conditions. Server-side apply would be more robust and would simplify the label/annotation merging logic.

🔎 Server-side apply pattern

 func (r *PackageSourceReconciler) createOrUpdate(ctx context.Context, obj client.Object) error {
-	existing := obj.DeepCopyObject().(client.Object)
-	key := client.ObjectKeyFromObject(obj)
-
-	err := r.Get(ctx, key, existing)
-	if apierrors.IsNotFound(err) {
-		return r.Create(ctx, obj)
-	} else if err != nil {
-		return err
-	}
-	// ... rest of update logic
+	return r.Patch(ctx, obj, client.Apply, client.FieldOwner("cozystack-packagesource"), client.ForceOwnership)
 }

Note: Server-side apply requires the object to have GVK set and may need adjustments for owner references.

internal/operator/package_reconciler.go (4)

400-435: Duplicated variant lookup logic.

The pattern of fetching Package → PackageSource → finding Variant is repeated in buildDependsOn, validateVariantDependencies, and triggerDependentPackages. Consider extracting this into a helper function.

🔎 Suggested helper

// getPackageVariant returns the Package, PackageSource, and effective Variant for a package name
func (r *PackageReconciler) getPackageVariant(ctx context.Context, packageName string) (*cozyv1alpha1.Package, *cozyv1alpha1.PackageSource, *cozyv1alpha1.Variant, error) {
	pkg := &cozyv1alpha1.Package{}
	if err := r.Get(ctx, types.NamespacedName{Name: packageName}, pkg); err != nil {
		return nil, nil, nil, err
	}

	ps := &cozyv1alpha1.PackageSource{}
	if err := r.Get(ctx, types.NamespacedName{Name: packageName}, ps); err != nil {
		return nil, nil, nil, err
	}

	variantName := pkg.Spec.Variant
	if variantName == "" {
		variantName = "default"
	}

	for i := range ps.Spec.Variants {
		if ps.Spec.Variants[i].Name == variantName {
			return pkg, ps, &ps.Spec.Variants[i], nil
		}
	}

	return nil, nil, nil, fmt.Errorf("variant %s not found in PackageSource %s", variantName, packageName)
}

---

275-279: Redundant dependent triggering via annotation updates.

The triggerDependentPackages function updates annotations on dependent Packages to trigger reconciliation. However, the watch handler on Package (lines 830-887) already enqueues reconcile requests for dependent Packages when a Package changes. This annotation update is redundant and could cause cascading updates.

🔎 Suggested fix

Remove the triggerDependentPackages call since the watch handles this:

 	logger.Info("reconciled Package", "name", pkg.Name, "helmReleaseCount", helmReleaseCount)

-	// Trigger reconcile for Packages that depend on this Package
-	if err := r.triggerDependentPackages(ctx, pkg.Name); err != nil {
-		logger.Error(err, "failed to trigger dependent Packages", "package", pkg.Name)
-		// Don't return error, this is best-effort
-	}
-
 	return ctrl.Result{}, nil

---

284-333: Get/Create/Update pattern in createOrUpdateHelmRelease.

Same atomicity concern as other reconcilers. Server-side apply would be more robust.

---

612-652: Get/Create/Update pattern in createOrUpdateNamespace.

Same atomicity concern as other helper functions. Server-side apply would be more robust.

cmd/cozystack-operator/main.go (2)

150-192: Context cancellation with multiple sequential timeouts.

The code creates multiple context.WithTimeout calls with deferred cancels. While functionally correct (each defer captures its own cancel function), the pattern is unusual. Since each operation completes before the next begins, consider calling cancel immediately after each operation completes to release resources promptly.

🔎 Suggested refactor

 	// Install Flux before starting reconcile loop
 	if installFlux {
 		setupLog.Info("Installing Flux components before starting reconcile loop")
 		installCtx, installCancel := context.WithTimeout(context.Background(), 5*time.Minute)
-		defer installCancel()
 
 		// The namespace will be automatically extracted from the embedded manifests
 		if err := fluxinstall.Install(installCtx, mgr.GetClient(), fluxinstall.WriteEmbeddedManifests); err != nil {
 			setupLog.Error(err, "failed to install Flux, continuing anyway")
 		} else {
 			setupLog.Info("Flux installation completed successfully")
 		}
+		installCancel()
 	}

Apply the same pattern to the other timeout contexts.

---

468-474: Git ref always treated as branch.

The ref is always set as a branch, but the parsed reference could be a tag. Consider supporting tag references for Git repositories.

🔎 Suggested improvement

You could detect tags (e.g., semver patterns) or add a flag to specify the ref type:

 	spec := map[string]interface{}{
 		"interval": "5m0s",
 		"url":      repoURL,
-		"ref": map[string]interface{}{
-			"branch": ref,
-		},
 	}
+	// Determine if ref is a tag (starts with 'v' followed by digit, or contains dots)
+	if strings.HasPrefix(ref, "v") || strings.Contains(ref, ".") {
+		spec["ref"] = map[string]interface{}{
+			"tag": ref,
+		}
+	} else {
+		spec["ref"] = map[string]interface{}{
+			"branch": ref,
+		}
+	}

Alternatively, support explicit syntax like git://repo@branch:main vs git://repo@tag:v1.0.0.

internal/operator/packagesource_reconciler.go (2)

211-220: OwnerReference may have empty APIVersion/Kind.

When using r.Get(), the returned object may not have APIVersion and Kind populated (depends on the client configuration). Consider using ctrl.SetControllerReference() which handles this properly.

🔎 Suggested fix

-	// Set ownerReference
-	ag.OwnerReferences = []metav1.OwnerReference{
-		{
-			APIVersion: packageSource.APIVersion,
-			Kind:       packageSource.Kind,
-			Name:       packageSource.Name,
-			UID:        packageSource.UID,
-			Controller: func() *bool { b := true; return &b }(),
-		},
-	}
+	// Set ownerReference
+	if err := ctrl.SetControllerReference(packageSource, ag, r.Scheme); err != nil {
+		return fmt.Errorf("failed to set controller reference: %w", err)
+	}

---

423-431: LastTransitionTime copied from ArtifactGenerator may be misleading.

Copying LastTransitionTime from the ArtifactGenerator while setting ObservedGeneration from the PackageSource creates a mixed condition. Consider using the current time or documenting this behavior.

internal/operator/package_reconciler.go (2)

201-210: OwnerReference may have empty APIVersion/Kind.

Same issue as in PackageSourceReconciler. Consider using ctrl.SetControllerReference() for proper GVK handling.

🔎 Suggested fix

-	// Set ownerReference
-	hr.OwnerReferences = []metav1.OwnerReference{
-		{
-			APIVersion: pkg.APIVersion,
-			Kind:       pkg.Kind,
-			Name:       pkg.Name,
-			UID:        pkg.UID,
-			Controller: func() *bool { b := true; return &b }(),
-		},
-	}
+	// Set ownerReference
+	if err := ctrl.SetControllerReference(pkg, hr, r.Scheme); err != nil {
+		return ctrl.Result{}, fmt.Errorf("failed to set controller reference: %w", err)
+	}

---

830-888: O(n) enumeration in watch handler could be optimized at scale.

The watch handler lists all Packages on every Package change and iterates through them to find dependents. While acceptable for small deployments, consider using controller-runtime field indexes for more efficient lookups as the cluster scales.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T11:21:45.808Z
Learning: Applies to **/*.go : Use Controller-runtime patterns and kubebuilder style for Go code

cmd/cozystack-operator/main.go (3)

89-89: Unused variable: cozystackVersion.

The cozystackVersion flag is parsed but never used. Consider logging it during startup or removing the flag.

🔎 Suggested fix

Add to startup logging:

 	setupLog.Info("Starting controller manager")
+	setupLog.Info("Cozystack version", "version", cozystackVersion)

---

271-289: Prefer server-side apply for atomic resource installation.

Both installFluxResourcesFunc and installPlatformSourceResource use Get followed by Create or Update, which is not atomic. Using server-side apply (client.Patch with client.Apply) would be more robust and align with the pattern used in internal/fluxinstall/install.go.

🔎 Suggested refactor

Replace the Get/Create/Update logic with:

-	err := k8sClient.Get(ctx, key, existing)
-	if err != nil {
-		if client.IgnoreNotFound(err) == nil {
-			if err := k8sClient.Create(ctx, &obj); err != nil {
-				return fmt.Errorf("failed to create resource: %w", err)
-			}
-		} else {
-			return fmt.Errorf("failed to check if resource exists: %w", err)
-		}
-	} else {
-		obj.SetResourceVersion(existing.GetResourceVersion())
-		if err := k8sClient.Update(ctx, &obj); err != nil {
-			return fmt.Errorf("failed to update resource: %w", err)
-		}
-	}
+	patchOptions := &client.PatchOptions{
+		FieldManager: "cozystack-operator",
+		Force:        func() *bool { b := true; return &b }(),
+	}
+	if err := k8sClient.Patch(ctx, &obj, client.Apply, patchOptions); err != nil {
+		return fmt.Errorf("failed to apply resource: %w", err)
+	}

Also applies to: 337-355

---

443-451: Incorrect handling of OCI tags vs digests.

The code unconditionally prepends sha256: to any non-empty digest. However, if the ref is a tag (e.g., @v1.0.0), this produces an invalid reference like sha256:v1.0.0. This will break OCI repositories using tags instead of digests.

🔎 Suggested fix

Distinguish between tags and digests:

 	if digest != "" {
-		// Ensure digest starts with sha256:
-		if !strings.HasPrefix(digest, "sha256:") {
-			digest = "sha256:" + digest
+		// Only treat as digest if it looks like one
+		if strings.HasPrefix(digest, "sha256:") {
+			spec["ref"] = map[string]interface{}{
+				"digest": digest,
+			}
+		} else {
+			// Treat as tag
+			spec["ref"] = map[string]interface{}{
+				"tag": digest,
+			}
 		}
-		spec["ref"] = map[string]interface{}{
-			"digest": digest,
-		}
 	}

internal/operator/package_reconciler.go (6)

613-652: Prefer server-side apply for Namespace updates.

The createOrUpdateNamespace function uses Get/Create/Update which is not atomic. Server-side apply would be more robust.

🔎 Suggested refactor

Replace with server-side apply:

 func (r *PackageReconciler) createOrUpdateNamespace(ctx context.Context, namespace *corev1.Namespace) error {
-	existing := &corev1.Namespace{}
-	key := types.NamespacedName{Name: namespace.Name}
-	err := r.Get(ctx, key, existing)
-	if apierrors.IsNotFound(err) {
-		return r.Create(ctx, namespace)
-	} else if err != nil {
-		return err
-	}
-	// ... merge logic ...
-	return r.Update(ctx, namespace)
+	patchOptions := &client.PatchOptions{
+		FieldManager: "cozystack-operator",
+		Force:        func() *bool { b := true; return &b }(),
+	}
+	return r.Patch(ctx, namespace, client.Apply, patchOptions)
 }

---

714-801: Inefficient dependent Package lookup.

The triggerDependentPackages function lists all Packages in the cluster and iterates through them. This can be inefficient with many Packages. Consider using a controller-runtime index on dependency fields for efficient lookup.

---

275-279: Reconsider triggering dependent Packages via annotation updates.

Updating the cozystack.io/trigger-reconcile annotation on dependent Packages triggers their reconciliation, which in turn may call triggerDependentPackages again, potentially causing cascading updates across the dependency graph. The existing watch handler (lines 830-888) already enqueues dependent Packages when a Package changes, making this annotation-based trigger redundant.

🔎 Suggested alternative

Consider removing the triggerDependentPackages call since the watch-based approach already handles this:

 	logger.Info("reconciled Package", "name", pkg.Name, "helmReleaseCount", helmReleaseCount)
-
-	// Trigger reconcile for Packages that depend on this Package
-	if err := r.triggerDependentPackages(ctx, pkg.Name); err != nil {
-		logger.Error(err, "failed to trigger dependent Packages", "package", pkg.Name)
-		// Don't return error, this is best-effort
-	}
 
 	return ctrl.Result{}, nil

---

400-435: Refactor duplicated variant lookup logic.

The logic for finding the correct variant for a Package is repeated in buildDependsOn, validateVariantDependencies, and triggerDependentPackages. This code duplication reduces maintainability.

🔎 Suggested refactor

Extract into a helper function:

func (r *PackageReconciler) getVariantForPackage(ctx context.Context, packageName string) (*cozyv1alpha1.Variant, error) {
	pkg := &cozyv1alpha1.Package{}
	if err := r.Get(ctx, types.NamespacedName{Name: packageName}, pkg); err != nil {
		return nil, err
	}
	
	variantName := pkg.Spec.Variant
	if variantName == "" {
		variantName = "default"
	}
	
	packageSource := &cozyv1alpha1.PackageSource{}
	if err := r.Get(ctx, types.NamespacedName{Name: packageName}, packageSource); err != nil {
		return nil, err
	}
	
	for i := range packageSource.Spec.Variants {
		if packageSource.Spec.Variants[i].Name == variantName {
			return &packageSource.Spec.Variants[i], nil
		}
	}
	
	return nil, fmt.Errorf("variant %s not found in PackageSource %s", variantName, packageName)
}

Then use it in all three places.

Also applies to: 494-529, 740-757

---

285-333: Prefer server-side apply for HelmRelease updates.

The createOrUpdateHelmRelease function uses Get followed by Create or Update, which can lead to race conditions. Server-side apply is the recommended approach for operators.

🔎 Suggested refactor

Replace with server-side apply:

 func (r *PackageReconciler) createOrUpdateHelmRelease(ctx context.Context, hr *helmv2.HelmRelease) error {
-	existing := &helmv2.HelmRelease{}
-	key := types.NamespacedName{
-		Name:      hr.Name,
-		Namespace: hr.Namespace,
-	}
-	err := r.Get(ctx, key, existing)
-	if apierrors.IsNotFound(err) {
-		return r.Create(ctx, hr)
-	} else if err != nil {
-		return err
-	}
-	// ... merge logic ...
-	return r.Update(ctx, existing)
+	patchOptions := &client.PatchOptions{
+		FieldManager: "cozystack-operator",
+		Force:        func() *bool { b := true; return &b }(),
+	}
+	return r.Patch(ctx, hr, client.Apply, patchOptions)
 }

---

559-563: Inconsistent namespace defaulting.

The reconcileNamespaces function errors when component.Install.Namespace is empty, but HelmRelease creation (lines 156-159) defaults empty namespaces to cozy-system. This inconsistency could cause namespace reconciliation to fail for components relying on the default.

🔎 Suggested fix

Apply the same defaulting logic:

 		namespace := component.Install.Namespace
 		if namespace == "" {
-			return fmt.Errorf("component %s has empty namespace in Install section", component.Name)
+			namespace = "cozy-system"
 		}

internal/operator/packagesource_reconciler.go (2)

90-103: Potential library name collision across variants.

The library map is built from all variants, but if two variants define libraries with the same name but different paths, the last one silently overwrites earlier entries. This could cause components to reference incorrect library paths.

🔎 Suggested fix

Consider keying by variant+library name, or detecting and logging collisions:

 	for _, variant := range packageSource.Spec.Variants {
 		for _, lib := range variant.Libraries {
 			libName := lib.Name
 			if libName == "" {
 				libName = r.getPackageNameFromPath(lib.Path)
 			}
 			if libName != "" {
+				if existing, ok := libraryMap[libName]; ok && existing.Path != lib.Path {
+					logger.Info("library name collision detected, last one wins",
+						"name", libName,
+						"existingPath", existing.Path,
+						"newPath", lib.Path)
+				}
 				libraryMap[libName] = lib
 			}
 		}
 	}

---

310-364: Prefer server-side apply for atomic resource updates.

The createOrUpdate function uses Get followed by Create or Update, which is not atomic and can lead to race conditions. Server-side apply (client.Patch with client.Apply) is more robust and handles field ownership declaratively.

🔎 Suggested refactor

Replace with server-side apply:

 func (r *PackageSourceReconciler) createOrUpdate(ctx context.Context, obj client.Object) error {
-	existing := obj.DeepCopyObject().(client.Object)
-	key := client.ObjectKeyFromObject(obj)
-
-	err := r.Get(ctx, key, existing)
-	if apierrors.IsNotFound(err) {
-		return r.Create(ctx, obj)
-	} else if err != nil {
-		return err
-	}
-	// ... merge logic ...
-	return r.Update(ctx, obj)
+	patchOptions := &client.PatchOptions{
+		FieldManager: "cozystack-operator",
+		Force:        func() *bool { b := true; return &b }(),
+	}
+	return r.Patch(ctx, obj, client.Apply, patchOptions)
 }

internal/fluxinstall/install.go (1)

181-182: Unreliable CRD registration wait using time.Sleep.

Using time.Sleep to wait for CRDs is unreliable on slow or busy clusters. The 2-second duration may not be sufficient, leading to failures when applying resources that depend on those CRDs.

🔎 Suggested fix

Poll the API server to check if CRDs are established:

 	if len(stageOne) > 0 {
 		logger.Info("Applying cluster definitions", "count", len(stageOne))
 		if err := applyObjects(ctx, k8sClient, stageOne); err != nil {
 			return fmt.Errorf("failed to apply cluster definitions: %w", err)
 		}
-		// Wait a bit for CRDs to be registered
-		time.Sleep(2 * time.Second)
+		// Wait for CRDs to be established
+		for _, obj := range stageOne {
+			if obj.GetKind() == "CustomResourceDefinition" {
+				if err := waitForCRDEstablished(ctx, k8sClient, obj.GetName(), 30*time.Second); err != nil {
+					return fmt.Errorf("CRD %s not established: %w", obj.GetName(), err)
+				}
+			}
+		}
 	}

You'll need to implement waitForCRDEstablished to poll the CRD's status.

packages/core/flux-aio/Makefile (2)

15-15: Declare target as .PHONY for robustness.

The update target should be declared as .PHONY to ensure it runs even if a file named "update" exists in the directory.

🔎 Suggested fix

Add at the top of the Makefile or before the target:

+.PHONY: update update-old update-new
 update: update-old update-new

Based on static analysis hints.

---

33-34: TODO: Resolve DNS configuration for tenant clusters.

The commented-out dnsPolicy insertion indicates an unresolved DNS issue when using hostNetwork in tenant Kubernetes clusters. This could affect Flux's ability to install HelmReleases in tenant environments.

Do you want me to help investigate DNS configuration options or open an issue to track this work?

internal/fluxinstall/install.go (1)

255-309: Consider logging all environment injection failures.

The injectKubernetesServiceEnv function continues processing after errors and only returns the first error encountered. While this provides resilience, subsequent errors are silently skipped without any log output, making troubleshooting difficult.

🔎 Suggested improvement

Log each error as it occurs:

 		spec, found, err := unstructured.NestedMap(obj.Object, "spec", "template", "spec")
 		if !found {
 			continue
 		}
 		if err != nil {
+			logger.V(1).Info("failed to get spec for environment injection", "kind", kind, "name", obj.GetName(), "error", err)
 			if firstErr == nil {
 				firstErr = fmt.Errorf("failed to get spec for %s/%s: %w", kind, obj.GetName(), err)
 			}
 			continue
 		}

Apply similar logging for other error cases.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.511Z
Learning: Applies to cmd/**/*.go : Follow controller-runtime patterns and kubebuilder style for Go code in cmd directory

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.511Z
Learning: Applies to internal/**/*.go : Follow controller-runtime patterns and kubebuilder style for Go code in internal directory

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.511Z
Learning: Applies to pkg/**/*.go : Follow controller-runtime patterns and kubebuilder style for Go code in pkg directory

cmd/cozystack-operator/main.go (3)

76-77: Unused cozystackVersion variable.

The cozystackVersion flag is parsed but never used. Consider logging it at startup for operational visibility.

🔎 Suggested fix

 	ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts)))
+	setupLog.Info("Cozystack operator starting", "version", cozystackVersion)
 
 	config := ctrl.GetConfigOrDie()

---

242-260: Consider using server-side apply for consistency.

The current Get/Create/Update pattern is acceptable for bootstrap operations but could be simplified using server-side apply, which would also be consistent with the approach in internal/fluxinstall/install.go.

🔎 Suggested refactor using SSA

-	existing := &unstructured.Unstructured{}
-	existing.SetGroupVersionKind(obj.GroupVersionKind())
-	key := client.ObjectKey{
-		Name:      obj.GetName(),
-		Namespace: obj.GetNamespace(),
-	}
-
-	err = k8sClient.Get(ctx, key, existing)
-	if err != nil {
-		if client.IgnoreNotFound(err) == nil {
-			// Resource doesn't exist, create it
-			if err := k8sClient.Create(ctx, obj); err != nil {
-				return fmt.Errorf("failed to create resource %s/%s: %w", obj.GetKind(), obj.GetName(), err)
-			}
-			logger.Info("Created platform source resource", "kind", obj.GetKind(), "name", obj.GetName())
-		} else {
-			return fmt.Errorf("failed to check if resource exists: %w", err)
-		}
-	} else {
-		// Resource exists, update it
-		obj.SetResourceVersion(existing.GetResourceVersion())
-		if err := k8sClient.Update(ctx, obj); err != nil {
-			return fmt.Errorf("failed to update resource %s/%s: %w", obj.GetKind(), obj.GetName(), err)
-		}
-		logger.Info("Updated platform source resource", "kind", obj.GetKind(), "name", obj.GetName())
-	}
+	patchOptions := &client.PatchOptions{
+		FieldManager: "cozystack-operator",
+		Force:        func() *bool { b := true; return &b }(),
+	}
+	if err := k8sClient.Patch(ctx, obj, client.Apply, patchOptions); err != nil {
+		return fmt.Errorf("failed to apply resource %s/%s: %w", obj.GetKind(), obj.GetName(), err)
+	}
+	logger.Info("Applied platform source resource", "kind", obj.GetKind(), "name", obj.GetName())

---

348-356: Potential incorrect handling of OCI tags vs digests.

The code prepends sha256: to any ref that doesn't already have it. If the ref is a tag (e.g., v1.0.0), this produces an invalid reference like sha256:v1.0.0. The parsing logic at lines 289-293 can return non-digest refs.

🔎 Suggested fix

 	if digest != "" {
-		// Ensure digest starts with sha256:
-		if !strings.HasPrefix(digest, "sha256:") {
-			digest = "sha256:" + digest
+		// Check if it looks like a digest (sha256:... or 64-char hex)
+		if strings.HasPrefix(digest, "sha256:") {
+			spec["ref"] = map[string]interface{}{
+				"digest": digest,
+			}
+		} else if len(digest) == 64 && isHexString(digest) {
+			spec["ref"] = map[string]interface{}{
+				"digest": "sha256:" + digest,
+			}
+		} else {
+			// Treat as tag
+			spec["ref"] = map[string]interface{}{
+				"tag": digest,
+			}
 		}
-		spec["ref"] = map[string]interface{}{
-			"digest": digest,
-		}
 	}

Add helper function:

func isHexString(s string) bool {
	for _, c := range s {
		if !((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F')) {
			return false
		}
	}
	return true
}

internal/fluxinstall/install.go (1)

181-183: time.Sleep for CRD registration is unreliable.

A 2-second sleep may not be sufficient on slow or busy clusters. Consider polling the API server for CRD Established condition instead.

🔎 Suggested improvement

// waitForCRDs polls until CRDs are established or timeout
func waitForCRDs(ctx context.Context, k8sClient client.Client, crds []*unstructured.Unstructured) error {
	for _, crd := range crds {
		if crd.GetKind() != "CustomResourceDefinition" {
			continue
		}
		name := crd.GetName()
		for {
			select {
			case <-ctx.Done():
				return ctx.Err()
			default:
			}
			existing := &unstructured.Unstructured{}
			existing.SetGroupVersionKind(crd.GroupVersionKind())
			if err := k8sClient.Get(ctx, client.ObjectKey{Name: name}, existing); err != nil {
				time.Sleep(500 * time.Millisecond)
				continue
			}
			conditions, found, _ := unstructured.NestedSlice(existing.Object, "status", "conditions")
			if found {
				for _, c := range conditions {
					cond, _ := c.(map[string]interface{})
					if cond["type"] == "Established" && cond["status"] == "True" {
						goto nextCRD
					}
				}
			}
			time.Sleep(500 * time.Millisecond)
		}
	nextCRD:
	}
	return nil
}

internal/operator/packagesource_reconciler.go (2)

90-103: Potential library name collision across variants.

When building the library map, if two variants define libraries with the same name but different paths, the last one silently overwrites the first. This could cause unexpected behavior.

🔎 Suggested fix - log collision warning

 	for _, variant := range packageSource.Spec.Variants {
 		for _, lib := range variant.Libraries {
 			libName := lib.Name
 			if libName == "" {
 				libName = r.getPackageNameFromPath(lib.Path)
 			}
 			if libName != "" {
+				if existing, ok := libraryMap[libName]; ok && existing.Path != lib.Path {
+					logger.Info("library name collision detected, overwriting",
+						"name", libName,
+						"existingPath", existing.Path,
+						"newPath", lib.Path,
+						"variant", variant.Name)
+				}
 				libraryMap[libName] = lib
 			}
 		}
 	}

---

309-364: Consider using server-side apply instead of Get/Create/Update.

The current pattern is not atomic and the label/annotation merging logic could be simplified by using server-side apply. This would also be consistent with internal/fluxinstall/install.go.

internal/operator/package_reconciler.go (3)

294-343: Consider using server-side apply and extracting shared utility.

The Get/Create/Update pattern is duplicated here and in packagesource_reconciler.go. Consider:

1. Using server-side apply for atomicity
2. Extracting a shared utility function to avoid duplication

---

763-803: Consider using server-side apply for namespace management.

This is another instance of the Get/Create/Update pattern that could benefit from server-side apply.

---

710-714: Inconsistent handling of empty namespace.

In reconcileNamespaces, an empty Install.Namespace returns an error. However, in the HelmRelease creation loop (lines 162-165), an empty namespace defaults to cozy-system. This inconsistency could cause namespace reconciliation to fail for components that rely on the default.

🔎 Suggested fix - apply same defaulting

 		// Namespace must be set
 		namespace := component.Install.Namespace
 		if namespace == "" {
-			return fmt.Errorf("component %s has empty namespace in Install section", component.Name)
+			namespace = "cozy-system"
 		}

packages/core/flux-aio/Makefile (1)

15-15: Add .PHONY declaration for the new targets.

The static analysis tool correctly flagged that update, update-old, and update-new should be declared as .PHONY targets since they don't produce files with those names.

🔎 Suggested fix

+.PHONY: show apply diff update update-old update-new
+
 NAME=flux-aio
 NAMESPACE=cozy-$(NAME)

internal/fluxinstall/install.go (2)

234-312: Error handling captures first error but continues.

The function design allows partial success - if env injection fails for one workload, others still get processed. This is intentional per the docstring, but consider logging each failure rather than just returning the first error to aid debugging.

🔎 Consider logging each error

+import "sigs.k8s.io/controller-runtime/pkg/log"
+
 func injectKubernetesServiceEnv(objects []*unstructured.Unstructured) error {
+	logger := log.Log.WithName("fluxinstall")
 	// ...
 		if err != nil {
 			if firstErr == nil {
 				firstErr = fmt.Errorf("failed to get spec for %s/%s: %w", kind, obj.GetName(), err)
 			}
+			logger.V(1).Info("failed to get spec", "kind", kind, "name", obj.GetName(), "error", err)
 			continue
 		}

---

59-73: Fallback YAML file selection could be made more explicit.

If fluxcd.yaml doesn't exist, the code picks the first .yaml file found. Consider logging a warning when using the fallback, or being more specific about expected filenames.

🔎 Suggested improvement

 	if _, err := os.Stat(manifestPath); err != nil {
 		// Try to find any YAML file if fluxcd.yaml doesn't exist
 		entries, err := os.ReadDir(manifestsDir)
 		if err != nil {
 			return fmt.Errorf("failed to read manifests directory: %w", err)
 		}
+		logger.Info("fluxcd.yaml not found, searching for alternative manifest files")
 		for _, entry := range entries {
 			if strings.HasSuffix(entry.Name(), ".yaml") {
 				manifestPath = filepath.Join(manifestsDir, entry.Name())
+				logger.Info("using alternative manifest file", "file", entry.Name())
 				break
 			}
 		}
 	}

internal/operator/package_reconciler.go (2)

586-684: Listing all Packages on every reconcile could be expensive.

The updateDependentPackagesDependencies function lists all Packages in the cluster. For clusters with many packages, this could become a performance bottleneck.

Consider using a controller-runtime index to efficiently find dependent packages:

// In SetupWithManager, add an index:
if err := mgr.GetFieldIndexer().IndexField(ctx, &cozyv1alpha1.Package{}, "spec.dependencies", func(obj client.Object) []string {
    pkg := obj.(*cozyv1alpha1.Package)
    // Return list of dependency package names
    // (requires resolving variant dependencies)
    return dependencyNames
}); err != nil {
    return err
}

// Then use:
r.List(ctx, packageList, client.MatchingFields{"spec.dependencies": packageName})

---

891-949: Watch handler lists all Packages, which may impact performance.

The watch handler for Package changes lists all Packages to find dependents. This duplicates the inefficient pattern from updateDependentPackagesDependencies. For better performance, consider:

1. Using an index (as suggested above)
2. Or caching dependency relationships

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.511Z
Learning: Applies to cmd/**/*.go : Follow controller-runtime patterns and kubebuilder style for Go code in cmd directory

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.511Z
Learning: Applies to internal/**/*.go : Follow controller-runtime patterns and kubebuilder style for Go code in internal directory

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.511Z
Learning: Applies to pkg/**/*.go : Follow controller-runtime patterns and kubebuilder style for Go code in pkg directory

cmd/cozystack-operator/main.go (3)

76-77: cozystackVersion flag is parsed but never used.

Consider logging it during startup for debugging/observability, or remove if not needed.

🔎 Suggested fix

 	setupLog.Info("Starting controller manager")
+	setupLog.Info("Cozystack version", "version", cozystackVersion)

---

242-260: Consider using server-side apply for resource creation.

The Get/Create/Update pattern is not atomic. While this is a startup-time single-shot operation with lower race condition risk, using client.Patch with client.Apply would be more robust and consistent with modern controller patterns.

---

348-356: Potential incorrect handling of OCI tags vs digests persists.

If parsePlatformSource extracts a tag (e.g., v1.0.0 from oci://registry/repo@v1.0.0), this code will prepend sha256: producing an invalid reference sha256:v1.0.0.

🔎 Suggested fix

Distinguish between tags and digests based on format:

 	if digest != "" {
-		// Ensure digest starts with sha256:
-		if !strings.HasPrefix(digest, "sha256:") {
-			digest = "sha256:" + digest
+		// Only treat as digest if it looks like a sha256 digest
+		if strings.HasPrefix(digest, "sha256:") || isHexDigest(digest) {
+			if !strings.HasPrefix(digest, "sha256:") {
+				digest = "sha256:" + digest
+			}
+			spec["ref"] = map[string]interface{}{
+				"digest": digest,
+			}
+		} else {
+			// Treat as tag
+			spec["ref"] = map[string]interface{}{
+				"tag": digest,
+			}
 		}
-		spec["ref"] = map[string]interface{}{
-			"digest": digest,
-		}
 	}

Add a helper to check for hex digest format:

func isHexDigest(s string) bool {
	if len(s) != 64 {
		return false
	}
	for _, c := range s {
		if !((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f')) {
			return false
		}
	}
	return true
}

internal/operator/packagesource_reconciler.go (3)

90-103: Potential library name collision across variants.

If two variants define libraries with the same name but different paths, the last one silently overwrites the first. Consider logging a warning when a collision is detected.

🔎 Suggested fix

 	for _, variant := range packageSource.Spec.Variants {
 		for _, lib := range variant.Libraries {
 			libName := lib.Name
 			if libName == "" {
 				libName = r.getPackageNameFromPath(lib.Path)
 			}
 			if libName != "" {
+				if existing, ok := libraryMap[libName]; ok && existing.Path != lib.Path {
+					logger.Info("library name collision, overwriting",
+						"name", libName,
+						"existingPath", existing.Path,
+						"newPath", lib.Path)
+				}
 				libraryMap[libName] = lib
 			}
 		}
 	}

---

309-364: Consider using server-side apply for resource creation.

The createOrUpdate function uses Get/Create/Update which is not atomic and can lead to race conditions. Server-side apply with client.Patch(ctx, obj, client.Apply, ...) is more robust and declarative.

---

211-220: Owner reference may have empty APIVersion/Kind.

When packageSource is fetched from the API server, TypeMeta (APIVersion/Kind) is often not populated. Using packageSource.APIVersion and packageSource.Kind directly may result in empty values in the owner reference.

🔎 Suggested fix using GVK from scheme

+	gvk, err := apiutil.GVKForObject(packageSource, r.Scheme)
+	if err != nil {
+		return fmt.Errorf("failed to get GVK for PackageSource: %w", err)
+	}
 	ag.OwnerReferences = []metav1.OwnerReference{
 		{
-			APIVersion: packageSource.APIVersion,
-			Kind:       packageSource.Kind,
+			APIVersion: gvk.GroupVersion().String(),
+			Kind:       gvk.Kind,
 			Name:       packageSource.Name,
 			UID:        packageSource.UID,
 			Controller: func() *bool { b := true; return &b }(),
 		},
 	}

Add import:

import "sigs.k8s.io/controller-runtime/pkg/client/apiutil"

internal/operator/package_reconciler.go (3)

294-343: Consider using server-side apply for HelmRelease management.

The createOrUpdateHelmRelease function uses Get/Create/Update which is not atomic. Server-side apply with client.Patch(ctx, hr, client.Apply, ...) would be more robust.

---

207-216: Owner reference may have empty APIVersion/Kind.

Same issue as in packagesource_reconciler.go. When pkg is fetched from the API server, TypeMeta is often not populated. Use apiutil.GVKForObject to reliably get the GVK.

🔎 Suggested fix

+	gvk, err := apiutil.GVKForObject(pkg, r.Scheme)
+	if err != nil {
+		return ctrl.Result{}, fmt.Errorf("failed to get GVK for Package: %w", err)
+	}
 	hr.OwnerReferences = []metav1.OwnerReference{
 		{
-			APIVersion: pkg.APIVersion,
-			Kind:       pkg.Kind,
+			APIVersion: gvk.GroupVersion().String(),
+			Kind:       gvk.Kind,
 			Name:       pkg.Name,
 			UID:        pkg.UID,
 			Controller: func() *bool { b := true; return &b }(),
 		},
 	}

Add import:

import "sigs.k8s.io/controller-runtime/pkg/client/apiutil"

---

710-714: Inconsistent handling of empty namespace.

In reconcileNamespaces, an empty Install.Namespace causes an error. However, in the HelmRelease creation loop (lines 162-165), an empty namespace defaults to cozy-system. This inconsistency could cause namespace reconciliation to fail for components that rely on the default.

🔎 Suggested fix

Apply the same defaulting logic:

 		// Namespace must be set
 		namespace := component.Install.Namespace
 		if namespace == "" {
-			return fmt.Errorf("component %s has empty namespace in Install section", component.Name)
+			namespace = "cozy-system"
 		}

cmd/cozystack-operator/main.go (1)

373-379: Git ref always treated as branch.

The ref is always set as branch, but it could be a tag (e.g., git://repo@v1.0.0). While Flux GitRepository can often resolve branches by name even if they're tags, consider supporting explicit tag refs for correctness.

internal/operator/package_reconciler.go (2)

891-949: Watch handler lists all Packages for dependency lookup.

The handler lists all Package resources to find dependents, which can be inefficient in clusters with many Packages. Consider using a controller-runtime index for more efficient lookups.

🔎 Suggested approach

Add an index on Package dependencies:

// In SetupWithManager:
if err := mgr.GetFieldIndexer().IndexField(ctx, &cozyv1alpha1.Package{}, "spec.dependsOn", func(o client.Object) []string {
    pkg := o.(*cozyv1alpha1.Package)
    // Get variant dependencies from PackageSource
    // Return list of dependency package names
    return getDependencies(pkg)
}); err != nil {
    return err
}

// Then use client.MatchingFields in the handler instead of iterating all Packages

Note: This requires resolving the PackageSource at index time, which may not be straightforward. An alternative is to accept the current approach for initial implementation and optimize later if performance becomes an issue.

---

586-684: updateDependentPackagesDependencies duplicates watch handler logic.

This function lists all Packages and updates their dependency status, but the watch handler on lines 891-949 already enqueues dependent Packages when a Package changes. The status update here may be redundant, and updating pkg.Status.Dependencies (line 672-675) will trigger another reconcile via the watch.

Consider relying solely on the watch-based mechanism and letting each Package update its own dependencies status during its reconcile, rather than having the dependency push updates.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.511Z
Learning: Applies to pkg/**/*.go : Follow controller-runtime patterns and kubebuilder style for Go code in pkg directory

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.511Z
Learning: Applies to cmd/**/*.go : Follow controller-runtime patterns and kubebuilder style for Go code in cmd directory

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.511Z
Learning: Applies to internal/**/*.go : Follow controller-runtime patterns and kubebuilder style for Go code in internal directory

cmd/cozystack-operator/main.go (2)

76-76: Unused flag: cozystackVersion is never consumed.

The cozystackVersion flag is parsed but never used in the operator. Either log it during startup or remove the flag definition.

🔎 Suggested fix

Add to startup logging:

 	setupLog.Info("Starting controller manager")
+	setupLog.Info("Cozystack version", "version", cozystackVersion)

---

242-260: Prefer server-side apply over Get/Create/Update.

The current Get→Create/Update pattern is not atomic and can lead to race conditions. Use server-side apply (client.Patch with client.Apply) as done in internal/operator/packagesource_reconciler.go:313-327 for consistency and robustness.

🔎 Proposed fix

-	err = k8sClient.Get(ctx, key, existing)
-	if err != nil {
-		if client.IgnoreNotFound(err) == nil {
-			// Resource doesn't exist, create it
-			if err := k8sClient.Create(ctx, obj); err != nil {
-				return fmt.Errorf("failed to create resource %s/%s: %w", obj.GetKind(), obj.GetName(), err)
-			}
-			logger.Info("Created platform source resource", "kind", obj.GetKind(), "name", obj.GetName())
-		} else {
-			return fmt.Errorf("failed to check if resource exists: %w", err)
-		}
-	} else {
-		// Resource exists, update it
-		obj.SetResourceVersion(existing.GetResourceVersion())
-		if err := k8sClient.Update(ctx, obj); err != nil {
-			return fmt.Errorf("failed to update resource %s/%s: %w", obj.GetKind(), obj.GetName(), err)
-		}
-		logger.Info("Updated platform source resource", "kind", obj.GetKind(), "name", obj.GetName())
-	}
-
-	return nil
+	// Use server-side apply for atomic create-or-update
+	if err := k8sClient.Patch(ctx, obj, client.Apply, client.FieldOwner("cozystack-operator-platform-source")); err != nil {
+		return fmt.Errorf("failed to apply platform source resource %s/%s: %w", obj.GetKind(), obj.GetName(), err)
+	}
+	logger.Info("Applied platform source resource", "kind", obj.GetKind(), "name", obj.GetName())
+	return nil

internal/operator/packagesource_reconciler.go (1)

98-113: Potential library name collision across variants.

The libraryMap is built by iterating through all variants. If two variants define libraries with the same name but different paths, the last one processed will overwrite previous entries, potentially causing incorrect library paths to be used.

🔎 Suggested fix

Consider one of these approaches:

Option 1: Detect and warn about collisions

 		for _, lib := range variant.Libraries {
 			libName := lib.Name
 			if libName == "" {
 				libName = r.getPackageNameFromPath(lib.Path)
 			}
 			if libName != "" {
+				if existing, ok := libraryMap[libName]; ok && existing.Path != lib.Path {
+					logger.Info("library name collision detected, last variant wins",
+						"name", libName,
+						"existingPath", existing.Path,
+						"newPath", lib.Path,
+						"variant", variant.Name)
+				}
 				libraryMap[libName] = lib
 			}
 		}

Option 2: Namespace by variant

-		libraryMap := make(map[string]cozyv1alpha1.Library)
+		libraryMap := make(map[string]cozyv1alpha1.Library)
 		for _, lib := range variant.Libraries {
 			libName := lib.Name
 			if libName == "" {
 				libName = r.getPackageNameFromPath(lib.Path)
 			}
 			if libName != "" {
-				libraryMap[libName] = lib
+				// Namespace by variant to avoid collisions
+				libraryMap[variant.Name+":"+libName] = lib
 			}
 		}

Note: If using option 2, update the lookup on line 144 accordingly.

internal/operator/package_reconciler.go (1)

319-368: Prefer server-side apply over Get/Create/Update.

The current Get→Create/Update pattern is not atomic and can lead to race conditions. Server-side apply automatically handles label/annotation merging and is more robust. This would also align with createOrUpdateNamespace (line 795), which already uses server-side apply.

🔎 Proposed fix

 func (r *PackageReconciler) createOrUpdateHelmRelease(ctx context.Context, hr *helmv2.HelmRelease) error {
-	existing := &helmv2.HelmRelease{}
-	key := types.NamespacedName{
-		Name:      hr.Name,
-		Namespace: hr.Namespace,
-	}
-
-	err := r.Get(ctx, key, existing)
-	if apierrors.IsNotFound(err) {
-		return r.Create(ctx, hr)
-	} else if err != nil {
-		return err
-	}
-
-	// Preserve resource version
-	hr.SetResourceVersion(existing.GetResourceVersion())
-
-	// Merge labels
-	labels := hr.GetLabels()
-	if labels == nil {
-		labels = make(map[string]string)
-	}
-	for k, v := range existing.GetLabels() {
-		if _, ok := labels[k]; !ok {
-			labels[k] = v
-		}
-	}
-	hr.SetLabels(labels)
-
-	// Merge annotations
-	annotations := hr.GetAnnotations()
-	if annotations == nil {
-		annotations = make(map[string]string)
-	}
-	for k, v := range existing.GetAnnotations() {
-		if _, ok := annotations[k]; !ok {
-			annotations[k] = v
-		}
-	}
-	hr.SetAnnotations(annotations)
-
-	// Update Spec
-	existing.Spec = hr.Spec
-	existing.SetLabels(hr.GetLabels())
-	existing.SetAnnotations(hr.GetAnnotations())
-	existing.SetOwnerReferences(hr.GetOwnerReferences())
-
-	return r.Update(ctx, existing)
+	// Ensure TypeMeta is set for server-side apply
+	gvk, err := apiutil.GVKForObject(hr, r.Scheme)
+	if err != nil {
+		return fmt.Errorf("failed to get GVK for HelmRelease: %w", err)
+	}
+	hr.SetGroupVersionKind(gvk)
+
+	// Use server-side apply with field manager
+	return r.Patch(ctx, hr, client.Apply, client.FieldOwner("cozystack-package-controller"))
 }

cmd/cozystack-operator/main.go (1)

348-356: Validate digest format before prepending sha256:.

If the ref is a tag (e.g., @v1.0.0) rather than a digest, prepending sha256: produces an invalid digest like sha256:v1.0.0. Consider validating that the ref is actually a SHA256 digest before prepending the prefix, or treating non-digest refs as tags.

🔎 Proposed fix

 	if digest != "" {
-		// Ensure digest starts with sha256:
-		if !strings.HasPrefix(digest, "sha256:") {
-			digest = "sha256:" + digest
+		// Only treat as digest if it looks like a SHA256 hash
+		if strings.HasPrefix(digest, "sha256:") {
+			spec["ref"] = map[string]interface{}{
+				"digest": digest,
+			}
+		} else if len(digest) == 64 {
+			// Assume bare 64-char hex string is a SHA256 digest
+			spec["ref"] = map[string]interface{}{
+				"digest": "sha256:" + digest,
+			}
+		} else {
+			// Treat as tag
+			spec["ref"] = map[string]interface{}{
+				"tag": digest,
+			}
 		}
-		spec["ref"] = map[string]interface{}{
-			"digest": digest,
-		}
 	}

internal/operator/package_reconciler.go (1)

858-935: LGTM!

The controller setup with watches correctly implements dependency-driven reconciliation. The watch handler that lists all Packages to find dependents is acceptable for moderate cluster sizes.

Optional: Consider indexing for efficiency.

If the cluster has a large number of Packages, consider adding a controller-runtime index on dependencies to avoid listing all Packages. However, this optimization is not critical for most deployments.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.511Z
Learning: Applies to cmd/**/*.go : Follow controller-runtime patterns and kubebuilder style for Go code in cmd directory

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.511Z
Learning: Applies to internal/**/*.go : Follow controller-runtime patterns and kubebuilder style for Go code in internal directory

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.511Z
Learning: Applies to pkg/**/*.go : Follow controller-runtime patterns and kubebuilder style for Go code in pkg directory

cmd/cozystack-operator/main.go (3)

76-76: Unused cozystackVersion flag.

The cozystackVersion flag is parsed but never used. Either log it during startup or remove the flag definition to avoid dead code.

Also applies to: 90-91

---

242-260: Use server-side apply instead of Get/Create/Update.

The Get followed by Create or Update pattern is not atomic and can lead to race conditions. Use client.Patch with client.Apply for a more robust approach, similar to the pattern already adopted in packagesource_reconciler.go line 329.

---

348-356: Validate digest format before prepending sha256:.

Blindly prepending sha256: to any non-empty digest breaks valid tag references. For example, oci://registry/repo@v1.0.0 would produce an invalid sha256:v1.0.0 digest.

🔎 Suggested validation logic

 	if digest != "" {
-		// Ensure digest starts with sha256:
-		if !strings.HasPrefix(digest, "sha256:") {
-			digest = "sha256:" + digest
+		// Only treat as digest if it looks like a sha256 digest
+		if strings.HasPrefix(digest, "sha256:") {
+			spec["ref"] = map[string]interface{}{
+				"digest": digest,
+			}
+		} else if len(digest) == 64 {
+			// Bare 64-char hex string, prepend sha256:
+			spec["ref"] = map[string]interface{}{
+				"digest": "sha256:" + digest,
+			}
+		} else {
+			// Treat as tag
+			spec["ref"] = map[string]interface{}{
+				"tag": digest,
+			}
 		}
-		spec["ref"] = map[string]interface{}{
-			"digest": digest,
-		}
 	}

internal/operator/package_reconciler.go (2)

886-934: Consider indexing for improved watch handler performance.

The watch handler lists all Packages to find dependents (line 894-896), which works but may not scale well with hundreds of Packages. Consider adding a controller-runtime index on the dependency field for more efficient lookups in large deployments.

---

320-369: Refactor to use server-side apply.

This function still uses the Get/Create/Update pattern with manual label/annotation merging, which is not atomic. Use client.Patch with client.Apply for consistency with createOrUpdateNamespace (line 796) and packagesource_reconciler.go line 329.

internal/operator/packagesource_reconciler.go (2)

98-113: Consider detecting library name collisions within a variant.

Within a single variant, if two libraries resolve to the same name (either from explicit Name or extracted from Path), the second will silently overwrite the first in the map. This could lead to unexpected behavior where a component references a library by name but gets the wrong path.

🔎 Suggested enhancement to log collisions

 		for _, lib := range variant.Libraries {
 			libName := lib.Name
 			if libName == "" {
 				// If library name is not set, extract from path
 				libName = r.getPackageNameFromPath(lib.Path)
 			}
 			if libName != "" {
+				if existing, ok := libraryMap[libName]; ok {
+					logger.Info("library name collision within variant, overwriting",
+						"variant", variant.Name,
+						"name", libName,
+						"existingPath", existing.Path,
+						"newPath", lib.Path)
+				}
 				// Store library with the resolved name
 				libraryMap[libName] = lib
 			}
 		}

Note: The previous review concern about collisions across variants is not applicable since libraryMap is correctly scoped per variant (created on line 102 inside the variant loop).

---

313-330: LGTM! Race condition resolved with server-side apply.

The method now correctly uses server-side apply (client.Apply), which is atomic and avoids the race condition from the Get/Create/Update pattern flagged in previous reviews.

For future maintainability, consider:

1. Extract the field owner string to a constant:

   const FieldOwnerPackageSource = "cozystack-packagesource-controller"

2. Deduplicate with package_reconciler.go: As noted in previous reviews, this createOrUpdate logic is duplicated. Consider extracting it to a shared utility package (e.g., internal/operator/util.go) to reduce maintenance burden.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.511Z
Learning: Applies to cmd/**/*.go : Follow controller-runtime patterns and kubebuilder style for Go code in cmd directory

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.511Z
Learning: Applies to internal/**/*.go : Follow controller-runtime patterns and kubebuilder style for Go code in internal directory

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.511Z
Learning: Applies to pkg/**/*.go : Follow controller-runtime patterns and kubebuilder style for Go code in pkg directory

Learnt from: lllamnyp
Repo: cozystack/cozystack PR: 1130
File: hack/e2e-apps/kubernetes.bats:101-101
Timestamp: 2025-07-11T06:11:25.438Z
Learning: In cozystack, the plural form for the Kubernetes custom resource is `kuberneteses.apps.cozystack.io`, not `kubernetes.apps.cozystack.io`. This is defined in the API schema even though it's not grammatically perfect.