Fix correctness, safety, and UX issues across 7 areas

- Scan duration: add upper bound (24h) and lower bound (1s) validation
- init.go: replace silent fmt.Sscanf failures with strconv.Atoi
- init.go: include error reason in skipped ingress/NetworkPolicy messages
- init.go: add stderr warnings when namespace discovery fails
- graph.go: validate --output path parent directory before processing
- define.go: add Cobra mutual exclusivity and one-required constraints
- gadget.go: refactor with dependency injection (CommandRunner) for
  testability, extract buildArgs() and calculateTimeout() helpers,
  convert check functions to methods with backward-compat wrappers

Author: fenio

pkg/cli/define.go

@@ -77,6 +77,14 @@ func init() {
 	defineCmd.Flags().BoolVarP(&defineInteractive, "interactive", "i", false, "Interactive policy creation")
 	defineCmd.Flags().StringVar(&defineValidate, "validate", "", "Validate a policy file")
 	defineCmd.Flags().StringVar(&defineMerge, "merge", "", "Merge multiple policy files (comma-separated)")
+
+	defineCmd.MarkFlagsMutuallyExclusive("validate", "merge")
+	defineCmd.MarkFlagsMutuallyExclusive("validate", "from")
+	defineCmd.MarkFlagsMutuallyExclusive("validate", "interactive")
+	defineCmd.MarkFlagsMutuallyExclusive("merge", "from")
+	defineCmd.MarkFlagsMutuallyExclusive("merge", "interactive")
+	// --from + --interactive is intentionally allowed (interactive modifies --from behavior)
+	defineCmd.MarkFlagsOneRequired("from", "interactive", "validate", "merge")
 }
 
 func runDefine(cmd *cobra.Command, args []string) error {
@@ -103,6 +111,8 @@ func runDefine(cmd *cobra.Command, args []string) error {
 		return defineInteractively(defineOutput)
 	}
 
+	// MarkFlagsOneRequired ensures at least one mode flag is set,
+	// so this is unreachable under normal usage.
 	return fmt.Errorf("specify --from, --interactive, --validate, or --merge")
 }
 

pkg/cli/define_test.go

@@ -0,0 +1,93 @@
+package cli
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/spf13/pflag"
+)
+
+func TestDefineFlags_MutualExclusivity(t *testing.T) {
+	tests := []struct {
+		name    string
+		args    []string
+		wantErr string
+	}{
+		{
+			name:    "validate and merge are mutually exclusive",
+			args:    []string{"define", "--validate", "p.yaml", "--merge", "a.yaml,b.yaml"},
+			wantErr: "if any flags in the group [validate merge] are set none of the others can be",
+		},
+		{
+			name:    "validate and from are mutually exclusive",
+			args:    []string{"define", "--validate", "p.yaml", "--from", "obs.yaml"},
+			wantErr: "if any flags in the group [validate from] are set none of the others can be",
+		},
+		{
+			name:    "validate and interactive are mutually exclusive",
+			args:    []string{"define", "--validate", "p.yaml", "--interactive"},
+			wantErr: "if any flags in the group [validate interactive] are set none of the others can be",
+		},
+		{
+			name:    "merge and from are mutually exclusive",
+			args:    []string{"define", "--merge", "a.yaml,b.yaml", "--from", "obs.yaml"},
+			wantErr: "if any flags in the group [merge from] are set none of the others can be",
+		},
+		{
+			name:    "merge and interactive are mutually exclusive",
+			args:    []string{"define", "--merge", "a.yaml,b.yaml", "--interactive"},
+			wantErr: "if any flags in the group [merge interactive] are set none of the others can be",
+		},
+		{
+			name: "from and interactive are allowed together",
+			args: []string{"define", "--from", "obs.yaml", "--interactive"},
+			// This combination is intentionally allowed; the error here would be from
+			// trying to actually load the file, not from flag validation.
+			wantErr: "",
+		},
+		{
+			name:    "no mode flags produces error",
+			args:    []string{"define"},
+			wantErr: "at least one of the flags",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Reset all define flag variables to defaults before each test
+			defineFrom = ""
+			defineBase = ""
+			defineOutput = "policy.yaml"
+			defineInteractive = false
+			defineValidate = ""
+			defineMerge = ""
+
+			// Reset the cobra flag state so mutual exclusivity checks work correctly
+			defineCmd.Flags().VisitAll(func(f *pflag.Flag) {
+				f.Changed = false
+			})
+
+			rootCmd.SetArgs(tt.args)
+			err := rootCmd.Execute()
+
+			if tt.wantErr == "" {
+				// We expect no flag-level error. The command may fail later
+				// (e.g. file not found), but not from flag validation.
+				if err != nil && strings.Contains(err.Error(), "if any flags in the group") {
+					t.Errorf("unexpected flag exclusivity error: %v", err)
+				}
+				if err != nil && strings.Contains(err.Error(), "at least one of the flags") {
+					t.Errorf("unexpected required flag error: %v", err)
+				}
+				return
+			}
+
+			if err == nil {
+				t.Fatalf("expected error containing %q, got nil", tt.wantErr)
+			}
+			if !strings.Contains(err.Error(), tt.wantErr) {
+				t.Errorf("error %q does not contain %q", err.Error(), tt.wantErr)
+			}
+		})
+	}
+}

pkg/cli/graph.go

@@ -49,7 +49,29 @@ func init() {
 	graphCmd.MarkFlagRequired("policy")
 }
 
+func validateOutputPath(path string) error {
+	dir := filepath.Dir(filepath.Clean(path))
+	info, err := os.Stat(dir)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return fmt.Errorf("output directory %q does not exist", dir)
+		}
+		return fmt.Errorf("cannot access output directory %q: %w", dir, err)
+	}
+	if !info.IsDir() {
+		return fmt.Errorf("output path parent %q is not a directory", dir)
+	}
+	return nil
+}
+
 func runGraph(cmd *cobra.Command, args []string) error {
+	// Validate output path early
+	if graphOutput != "" {
+		if err := validateOutputPath(graphOutput); err != nil {
+			return err
+		}
+	}
+
 	// Load policy
 	pol, err := policy.LoadFromFile(graphPolicyFile)
 	if err != nil {

pkg/cli/graph_test.go

@@ -148,6 +148,50 @@ func TestGraphCommand_WriteToFile(t *testing.T) {
 	})
 }
 
+func TestValidateOutputPath(t *testing.T) {
+	tmpDir := t.TempDir()
+
+	t.Run("valid temp dir path", func(t *testing.T) {
+		path := filepath.Join(tmpDir, "output.dot")
+		if err := validateOutputPath(path); err != nil {
+			t.Errorf("unexpected error: %v", err)
+		}
+	})
+
+	t.Run("current dir path", func(t *testing.T) {
+		if err := validateOutputPath("output.dot"); err != nil {
+			t.Errorf("unexpected error: %v", err)
+		}
+	})
+
+	t.Run("nonexistent parent dir", func(t *testing.T) {
+		path := filepath.Join(tmpDir, "no-such-dir", "output.dot")
+		err := validateOutputPath(path)
+		if err == nil {
+			t.Fatal("expected error for nonexistent parent dir")
+		}
+		if !strings.Contains(err.Error(), "does not exist") {
+			t.Errorf("error %q should mention 'does not exist'", err.Error())
+		}
+	})
+
+	t.Run("file as parent", func(t *testing.T) {
+		// Create a file to use as a fake parent
+		fakePath := filepath.Join(tmpDir, "fakefile")
+		if err := os.WriteFile(fakePath, []byte("x"), 0644); err != nil {
+			t.Fatal(err)
+		}
+		path := filepath.Join(fakePath, "output.dot")
+		err := validateOutputPath(path)
+		if err == nil {
+			t.Fatal("expected error when parent is a file")
+		}
+		if !strings.Contains(err.Error(), "not a directory") {
+			t.Errorf("error %q should mention 'not a directory'", err.Error())
+		}
+	})
+}
+
 func min(a, b int) int {
 	if a < b {
 		return a

pkg/cli/init.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"os"
 	"sort"
+	"strconv"
 	"strings"
 	"time"
 
@@ -121,7 +122,7 @@ func runInit(cmd *cobra.Command, args []string) error {
 	fmt.Printf("%s Discovering ingresses... ", colorInfo("→"))
 	ingresses, err := discoverIngresses(ctx, namespaces)
 	if err != nil {
-		fmt.Println(colorWarning("⚠"), colorDim("(skipped)"))
+		fmt.Println(colorWarning("⚠"), colorDim(fmt.Sprintf("(skipped: %s)", err)))
 	} else {
 		fmt.Println(colorSuccess("✓"), colorDim(fmt.Sprintf("(%d found)", len(ingresses))))
 	}
@@ -130,7 +131,7 @@ func runInit(cmd *cobra.Command, args []string) error {
 	fmt.Printf("%s Checking existing NetworkPolicies... ", colorInfo("→"))
 	existingPolicies, err := discoverNetworkPolicies(ctx, namespaces)
 	if err != nil {
-		fmt.Println(colorWarning("⚠"), colorDim("(skipped)"))
+		fmt.Println(colorWarning("⚠"), colorDim(fmt.Sprintf("(skipped: %s)", err)))
 	} else {
 		fmt.Println(colorSuccess("✓"), colorDim(fmt.Sprintf("(%d found)", len(existingPolicies))))
 	}
@@ -272,6 +273,7 @@ func discoverWorkloads(ctx context.Context, namespaces []string) (*workloadInfo,
 			"-o", `jsonpath={range .items[*]}{.metadata.labels.app\.kubernetes\.io/name}{"|"}{.metadata.labels.app}{"|"}{range .spec.containers[*]}{range .ports[*]}{.containerPort}{" "}{end}{end}{"\n"}{end}`,
 		}, nil)
 		if err != nil {
+			fmt.Fprintf(os.Stderr, "  %s namespace %s: %s\n", colorWarning("⚠"), ns, err)
 			continue
 		}
 
@@ -305,8 +307,10 @@ func discoverWorkloads(ctx context.Context, namespaces []string) (*workloadInfo,
 			// Parse ports
 			if len(parts) > 2 {
 				for _, p := range strings.Fields(parts[2]) {
-					var port int
-					fmt.Sscanf(p, "%d", &port)
+					port, err := strconv.Atoi(strings.TrimSpace(p))
+					if err != nil {
+						continue
+					}
 					if port > 0 {
 						app.ports = appendUnique(app.ports, port)
 					}
@@ -321,6 +325,7 @@ func discoverWorkloads(ctx context.Context, namespaces []string) (*workloadInfo,
 			"-o", `jsonpath={range .items[*]}{.spec.selector.app\.kubernetes\.io/name}{"|"}{.spec.selector.app}{"|"}{range .spec.ports[*]}{.port}{" "}{end}{"\n"}{end}`,
 		}, nil)
 		if err != nil {
+			fmt.Fprintf(os.Stderr, "  %s namespace %s: %s\n", colorWarning("⚠"), ns, err)
 			continue
 		}
 
@@ -354,8 +359,10 @@ func discoverWorkloads(ctx context.Context, namespaces []string) (*workloadInfo,
 			// Parse ports from service
 			if len(parts) > 2 {
 				for _, p := range strings.Fields(parts[2]) {
-					var port int
-					fmt.Sscanf(p, "%d", &port)
+					port, err := strconv.Atoi(strings.TrimSpace(p))
+					if err != nil {
+						continue
+					}
 					if port > 0 {
 						app.ports = appendUnique(app.ports, port)
 					}
@@ -385,6 +392,7 @@ func discoverIngresses(ctx context.Context, namespaces []string) ([]ingressInfo,
 			"-o", "jsonpath={range .items[*]}{.metadata.name}|{.spec.rules[*].host}|{.spec.rules[*].http.paths[*].backend.service.name}|{.spec.rules[*].http.paths[*].backend.service.port.number}\\n{end}",
 		}, nil)
 		if err != nil {
+			fmt.Fprintf(os.Stderr, "  %s namespace %s: %s\n", colorWarning("⚠"), ns, err)
 			continue
 		}
 
@@ -404,7 +412,9 @@ func discoverIngresses(ctx context.Context, namespaces []string) ([]ingressInfo,
 				serviceName: parts[2],
 			}
 			if len(parts) > 3 {
-				fmt.Sscanf(parts[3], "%d", &ing.servicePort)
+				if port, err := strconv.Atoi(strings.TrimSpace(parts[3])); err == nil {
+					ing.servicePort = port
+				}
 			}
 			ingresses = append(ingresses, ing)
 		}
@@ -422,6 +432,7 @@ func discoverNetworkPolicies(ctx context.Context, namespaces []string) ([]string
 			"-o", "jsonpath={.items[*].metadata.name}",
 		}, nil)
 		if err != nil {
+			fmt.Fprintf(os.Stderr, "  %s namespace %s: %s\n", colorWarning("⚠"), ns, err)
 			continue
 		}
 		for _, name := range strings.Fields(result.Stdout) {

pkg/cli/scan.go

@@ -68,11 +68,11 @@ func runScan(cmd *cobra.Command, args []string) error {
 			return err
 		}
 
-		// Parse duration
+		// Parse and validate duration
 		var err error
-		duration, err = time.ParseDuration(scanDuration)
+		duration, err = validateScanDuration(scanDuration)
 		if err != nil {
-			return fmt.Errorf("invalid duration format: %w", err)
+			return err
 		}
 
 		fmt.Printf("Starting network scan...\n")
@@ -172,6 +172,20 @@ func runScan(cmd *cobra.Command, args []string) error {
 	return nil
 }
 
+func validateScanDuration(s string) (time.Duration, error) {
+	d, err := time.ParseDuration(s)
+	if err != nil {
+		return 0, fmt.Errorf("invalid duration format: %w", err)
+	}
+	if d > 24*time.Hour {
+		return 0, fmt.Errorf("scan duration %s exceeds maximum of 24h", d)
+	}
+	if d < 1*time.Second {
+		return 0, fmt.Errorf("scan duration must be at least 1s")
+	}
+	return d, nil
+}
+
 func splitNamespaces(ns string) []string {
 	if ns == "" {
 		return nil