cmd/cozypkg/cmd/del.go (1)

73-96: Consider extracting shared Kubernetes client setup.

This kubeconfig loading and scheme registration pattern is duplicated across add.go, del.go, list.go, and dependencies.go. Consider extracting to a shared helper function.

🔎 Example helper function

// In a shared file like client.go
func newK8sClient(kubeconfig string) (client.Client, error) {
    var config *rest.Config
    var err error

    if kubeconfig != "" {
        config, err = clientcmd.BuildConfigFromFlags("", kubeconfig)
    } else {
        config, err = ctrl.GetConfig()
    }
    if err != nil {
        return nil, fmt.Errorf("failed to get kubeconfig: %w", err)
    }

    scheme := runtime.NewScheme()
    utilruntime.Must(clientgoscheme.AddToScheme(scheme))
    utilruntime.Must(cozyv1alpha1.AddToScheme(scheme))

    return client.New(config, client.Options{Scheme: scheme})
}

cmd/cozypkg/cmd/dependencies.go (2)

110-125: Consider consolidating init() functions.

Having two init() functions in the same file is valid Go but can reduce readability. Consider merging them.

🔎 Proposed consolidation

 func init() {
 	rootCmd.AddCommand(dotCmd)
 	dotCmd.Flags().BoolVarP(&dotCmdFlags.installed, "installed", "i", false, "show dependencies only for installed Package resources")
 	dotCmd.Flags().BoolVar(&dotCmdFlags.components, "components", true, "show component-level dependencies (default: true)")
 	dotCmd.Flags().StringArrayVarP(&dotCmdFlags.files, "file", "f", []string{}, "Read packages from file or directory (can be specified multiple times)")
 	dotCmd.Flags().StringVar(&dotCmdFlags.kubeconfig, "kubeconfig", "", "Path to kubeconfig file (defaults to ~/.kube/config or KUBECONFIG env var)")
+
+	utilruntime.Must(clientgoscheme.AddToScheme(dependenciesScheme))
+	utilruntime.Must(cozyv1alpha1.AddToScheme(dependenciesScheme))
 }
 
 var (
 	dependenciesScheme = runtime.NewScheme()
 )
-
-func init() {
-	utilruntime.Must(clientgoscheme.AddToScheme(dependenciesScheme))
-	utilruntime.Must(cozyv1alpha1.AddToScheme(dependenciesScheme))
-}

---

263-275: Avoid redundant string splitting.

Line 268 splits the node string twice. Consider splitting once and reusing.

🔎 Proposed fix

 		// Style nodes based on type
 		if strings.Contains(node, ".") && !strings.HasPrefix(node, "cozystack.") {
 			// Component node
+			parts := strings.Split(node, ".")
 			n.Attr("shape", "box")
 			n.Attr("style", "rounded,filled")
 			n.Attr("fillcolor", "lightyellow")
-			n.Attr("label", strings.Split(node, ".")[len(strings.Split(node, "."))-1])
+			n.Attr("label", parts[len(parts)-1])
 		} else {

cmd/cozypkg/cmd/add.go (3)

111-118: Silent fallback may hide legitimate errors.

When createPackageFromFile fails, the code silently falls back to interactive installation. This could hide issues like YAML parse errors or permission problems. Consider logging when falling back.

🔎 Proposed improvement

 			// Check if package comes from a file
 			if filePath, fromFile := packagesFromFiles[packageName]; fromFile {
 				// Try to create Package directly from file
-				if err := createPackageFromFile(ctx, k8sClient, filePath, packageName); err == nil {
+				err := createPackageFromFile(ctx, k8sClient, filePath, packageName)
+				if err == nil {
 					fmt.Fprintf(os.Stderr, "✓ Added Package %s\n", packageName)
 					continue
 				}
-				// If failed, fall back to interactive installation
+				// If failed, fall back to interactive installation
+				fmt.Fprintf(os.Stderr, "ℹ Package definition not found in %s, using interactive mode for %s\n", filePath, packageName)
 			}

---

286-296: Dependency requester tracking overwrites previous requesters.

If multiple packages depend on the same package (e.g., A→X and B→X), only the last requester is stored. The error message at line 470 will only show one requester. This is acceptable but could be enhanced to show all requesters.

---

536-557: Consider adding an exit option for interactive variant selection.

The infinite loop requires Ctrl+C to abort. Consider accepting 'q' or empty input (when multiple variants) to cancel gracefully.

🔎 Example enhancement

 	for {
 		fmt.Fprintf(os.Stderr, prompt)
 		input, err := reader.ReadString('\n')
 		if err != nil {
 			return "", fmt.Errorf("failed to read input: %w", err)
 		}

 		input = strings.TrimSpace(input)

+		// Allow user to quit
+		if input == "q" || input == "quit" {
+			return "", fmt.Errorf("installation cancelled by user")
+		}
+
 		// If input is empty and there's a default variant, use it
 		if input == "" && len(ps.Spec.Variants) == 1 {
 			return defaultVariant, nil
 		}

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: NickVolynkin
Repo: cozystack/cozystack PR: 1117
File: packages/apps/mysql/Makefile:8-8
Timestamp: 2025-06-26T04:29:24.830Z
Learning: The cozystack project uses yq v4+ on their CI runner, so yq v4 syntax (-o json --indent 4) is compatible and version checks are not needed.

cmd/cozypkg/cmd/del.go (2)

101-109: Consider reusing the installed packages list.

The installed packages are listed here (lines 102-109) and then listed again in findPackagesToDelete (lines 161-169). Consider passing installedMap to findPackagesToDelete to avoid the redundant API call.

🔎 Proposed refactor

-		// Find all packages to delete (including dependents)
-		packagesToDelete, err := findPackagesToDelete(ctx, k8sClient, packageNames)
+		// Find all packages to delete (including dependents)
+		packagesToDelete, err := findPackagesToDelete(ctx, k8sClient, packageNames, installedMap)

Then update the function signature and remove the duplicate listing inside findPackagesToDelete.

---

246-260: Consider sorting package lists for consistent output.

Map iteration order is non-deterministic in Go. Sorting the requested and dependents slices before printing would provide consistent, reproducible output across runs.

🔎 Proposed fix

Add import for sort and sort the slices:

+import "sort"
...
+	sort.Strings(requested)
+	sort.Strings(dependents)
+
 	fmt.Fprintf(os.Stderr, "\nThe following packages will be deleted:\n\n")

cmd/cozypkg/cmd/dependencies.go (3)

110-125: Consider consolidating init() functions.

Having two init() functions in the same file is valid Go but can reduce readability. Consider combining them or adding a comment explaining why they're separate.

🔎 Proposed fix

 func init() {
 	rootCmd.AddCommand(dotCmd)
 	dotCmd.Flags().BoolVarP(&dotCmdFlags.installed, "installed", "i", false, "show dependencies only for installed Package resources")
 	dotCmd.Flags().BoolVar(&dotCmdFlags.components, "components", true, "show component-level dependencies (default: true)")
 	dotCmd.Flags().StringArrayVarP(&dotCmdFlags.files, "file", "f", []string{}, "Read packages from file or directory (can be specified multiple times)")
 	dotCmd.Flags().StringVar(&dotCmdFlags.kubeconfig, "kubeconfig", "", "Path to kubeconfig file (defaults to ~/.kube/config or KUBECONFIG env var)")
-}
-
-var (
-	dependenciesScheme = runtime.NewScheme()
-)
-
-func init() {
+
+	// Initialize scheme for Kubernetes client
 	utilruntime.Must(clientgoscheme.AddToScheme(dependenciesScheme))
 	utilruntime.Must(cozyv1alpha1.AddToScheme(dependenciesScheme))
 }
+
+var (
+	dependenciesScheme = runtime.NewScheme()
+)

---

180-197: Redundant filter logic when single package is specified.

When exactly one package is specified, both packageName (line 181-183) and selectedPackages (line 186-197) filters are applied, which is redundant. This works correctly but could be simplified.

🔎 Proposed simplification

Consider using only selectedPackages for filtering and removing the packageName parameter:

-func buildGraphFromCluster(ctx context.Context, kubeconfig string, packagesOnly bool, installedOnly bool, packageName string, selectedPackages []string) (map[string][]string, map[string]bool, error) {
+func buildGraphFromCluster(ctx context.Context, kubeconfig string, packagesOnly bool, installedOnly bool, selectedPackages []string) (map[string][]string, map[string]bool, error) {
 ...
-		// Filter by package name if specified
-		if packageName != "" && psName != packageName {
-			continue
-		}
-
 		// Filter by selected packages if specified
 		if len(selectedPackages) > 0 {

Then update the caller to always use selectedPackages.

---

272-272: Avoid duplicate strings.Split call.

The node string is split twice on the same line. Extract to a variable for efficiency.

🔎 Proposed fix

-			n.Attr("label", strings.Split(node, ".")[len(strings.Split(node, "."))-1])
+			parts := strings.Split(node, ".")
+			n.Attr("label", parts[len(parts)-1])

cmd/cozypkg/cmd/add.go (4)

114-121: Silent fallback may hide important errors.

When createPackageFromFile fails, the code silently falls back to interactive installation without logging the error. This could hide issues like permission errors or malformed YAML. Consider logging the error before falling back.

🔎 Proposed fix

 			// Check if package comes from a file
 			if filePath, fromFile := packagesFromFiles[packageName]; fromFile {
 				// Try to create Package directly from file
-				if err := createPackageFromFile(ctx, k8sClient, filePath, packageName); err == nil {
+				err := createPackageFromFile(ctx, k8sClient, filePath, packageName)
+				if err == nil {
 					fmt.Fprintf(os.Stderr, "✓ Added Package %s\n", packageName)
 					continue
 				}
-				// If failed, fall back to interactive installation
+				// Log the error and fall back to interactive installation
+				fmt.Fprintf(os.Stderr, "⚠ Could not create Package %s from file (%v), falling back to interactive installation\n", packageName, err)
 			}

---

274-276: Dependency requester may be overwritten.

If multiple packages depend on the same dependency, dependencyRequesters[dep] will only store the last requester. This affects error messages only, but could be misleading when debugging missing dependencies.

🔎 Proposed fix

Consider storing all requesters or the first one:

 		for dep := range deps {
 			if _, exists := tree[pkgName]; !exists {
 				tree[pkgName] = []string{}
 			}
 			tree[pkgName] = append(tree[pkgName], dep)
 			// Track who requested this dependency
-			dependencyRequesters[dep] = pkgName
+			if _, exists := dependencyRequesters[dep]; !exists {
+				dependencyRequesters[dep] = pkgName
+			}

---

520-541: Consider adding a maximum retry limit.

The infinite loop for variant selection will retry indefinitely on invalid input. While this is intentional, consider adding a retry limit to prevent user frustration or issues when stdin is unexpectedly non-interactive.

---

380-383: Handle "already exists" case explicitly.

If the Package already exists in the cluster, k8sClient.Create will return an error. This bubbles up and causes a fallback to interactive installation, which will then report "already installed". Consider checking for apierrors.IsAlreadyExists and returning success in that case.

🔎 Proposed fix

+			apierrors "k8s.io/apimachinery/pkg/api/errors"
...
 			// Create Package
 			if err := k8sClient.Create(ctx, &pkg); err != nil {
+				if apierrors.IsAlreadyExists(err) {
+					return nil // Already exists is not an error
+				}
 				return fmt.Errorf("failed to create Package: %w", err)
 			}

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 pkg/**/*.go : Follow controller-runtime patterns and kubebuilder style for Go code in pkg directory

Learnt from: lllamnyp
Repo: cozystack/cozystack PR: 1025
File: packages/apps/kafka/charts/cozy-lib:1-1
Timestamp: 2025-06-04T06:22:17.306Z
Learning: Files in packages/apps/*/charts/cozy-lib that contain the path "../../../library/cozy-lib" are symbolic links, not regular files. Git diffs may display symlinks as regular files with their target path as content, which can be misleading in code review.

cmd/cozypkg/cmd/del.go (1)

346-366: Add cycle detection to prevent incomplete deletions.

The topological sort using Kahn's algorithm will terminate early if cycles exist, returning an incomplete list without any error. This could lead to some packages not being deleted silently.

🔎 Proposed fix

 	var result []string
 	for len(queue) > 0 {
 		node := queue[0]
 		queue = queue[1:]
 		result = append(result, node)

 		// Process dependents
 		for _, dependent := range reverseGraph[node] {
 			inDegree[dependent]--
 			if inDegree[dependent] == 0 {
 				queue = append(queue, dependent)
 			}
 		}
 	}

+	// Check for cycles - if not all nodes were processed, a cycle exists
+	if len(result) != len(allNodes) {
+		return nil, fmt.Errorf("dependency cycle detected, cannot determine safe delete order")
+	}
+
 	// Reverse the result to get dependents first, then dependencies
 	for i, j := 0, len(result)-1; i < j; i, j = i+1, j-1 {
 		result[i], result[j] = result[j], result[i]
 	}

 	return result, nil

cmd/cozypkg/cmd/dependencies.go (1)

170-402: Consider extracting helper functions to reduce complexity.

The buildGraphFromCluster function is quite long (~270 lines) with deeply nested loops. While the logic is correct, extracting helper functions for component dependency processing (lines 300-348) and edge variant tracking (lines 351-399) would improve readability and testability.

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: lllamnyp
Repo: cozystack/cozystack PR: 1025
File: packages/apps/kafka/charts/cozy-lib:1-1
Timestamp: 2025-06-04T06:22:17.306Z
Learning: Files in packages/apps/*/charts/cozy-lib that contain the path "../../../library/cozy-lib" are symbolic links, not regular files. Git diffs may display symlinks as regular files with their target path as content, which can be misleading in code review.

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/cozypkg/cmd/dependencies.go (1)

110-116: Consider consolidating init functions.

While valid Go, having two separate init() functions in the same file is unusual. You could merge them into a single init() for clarity.

🔎 Consolidation example

-var dotCmd = &cobra.Command{
-	// ...
-}
-
-func init() {
-	rootCmd.AddCommand(dotCmd)
-	dotCmd.Flags().BoolVarP(&dotCmdFlags.installed, "installed", "i", false, "show dependencies only for installed Package resources")
-	dotCmd.Flags().BoolVar(&dotCmdFlags.components, "components", false, "show component-level dependencies")
-	dotCmd.Flags().StringArrayVarP(&dotCmdFlags.files, "file", "f", []string{}, "Read packages from file or directory (can be specified multiple times)")
-	dotCmd.Flags().StringVar(&dotCmdFlags.kubeconfig, "kubeconfig", "", "Path to kubeconfig file (defaults to ~/.kube/config or KUBECONFIG env var)")
-}
-
-var (
-	dependenciesScheme = runtime.NewScheme()
-)
-
 func init() {
+	// Initialize scheme
 	utilruntime.Must(clientgoscheme.AddToScheme(dependenciesScheme))
 	utilruntime.Must(cozyv1alpha1.AddToScheme(dependenciesScheme))
+	
+	// Register command
+	rootCmd.AddCommand(dotCmd)
+	dotCmd.Flags().BoolVarP(&dotCmdFlags.installed, "installed", "i", false, "show dependencies only for installed Package resources")
+	dotCmd.Flags().BoolVar(&dotCmdFlags.components, "components", false, "show component-level dependencies")
+	dotCmd.Flags().StringArrayVarP(&dotCmdFlags.files, "file", "f", []string{}, "Read packages from file or directory (can be specified multiple times)")
+	dotCmd.Flags().StringVar(&dotCmdFlags.kubeconfig, "kubeconfig", "", "Path to kubeconfig file (defaults to ~/.kube/config or KUBECONFIG env var)")
 }

Also applies to: 122-125

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 pkg/**/*.go : Follow controller-runtime patterns and kubebuilder style for Go code in pkg directory

Learnt from: lllamnyp
Repo: cozystack/cozystack PR: 1025
File: packages/apps/kafka/charts/cozy-lib:1-1
Timestamp: 2025-06-04T06:22:17.306Z
Learning: Files in packages/apps/*/charts/cozy-lib that contain the path "../../../library/cozy-lib" are symbolic links, not regular files. Git diffs may display symlinks as regular files with their target path as content, which can be misleading in code review.