fix(adhoc-sweep-fixes): CU-86akbhhau 14 review findings across 12 files

cmd/app/upgrade.go

@@ -106,7 +106,9 @@ func runUpgradeChangeRef(cmd *cobra.Command, args []string, flags *InstallFlags,
 	// let the wait sync them once progress stalls instead of timing out (N3).
 	req.SyncStragglersOnStall = true
 
-	pterm.Info.Printf("Upgrading OpenFrame to ref %q\n", flags.resolvedRef())
+	if !flags.Silent {
+		pterm.Info.Printf("Upgrading OpenFrame to ref %q\n", flags.resolvedRef())
+	}
 	if err := services.InstallChartsWithConfigContext(cmd.Context(), req); err != nil {
 		return sharedErrors.HandleGlobalError(err, verbose)
 	}
@@ -135,10 +137,12 @@ func runUpgradeForceSync(cmd *cobra.Command, args []string, flags *InstallFlags,
 		return sharedErrors.HandleGlobalError(previewOutOfSync(cmd.Context(), manager, verbose, prune), verbose)
 	}
 
-	if prune {
+	if prune && !flags.Silent {
 		pterm.Warning.Println("Refreshing and syncing with --prune: resources removed from git will be DELETED.")
 	}
-	pterm.Info.Println("Refreshing and syncing the OpenFrame platform...")
+	if !flags.Silent {
+		pterm.Info.Println("Refreshing and syncing the OpenFrame platform...")
+	}
 	if err := manager.RefreshAndSync(cmd.Context(), prune); err != nil {
 		return sharedErrors.HandleGlobalError(err, verbose)
 	}
@@ -157,7 +161,9 @@ func runUpgradeForceSync(cmd *cobra.Command, args []string, flags *InstallFlags,
 	if err := manager.WaitForApplications(cmd.Context(), waitCfg); err != nil {
 		return sharedErrors.HandleGlobalError(err, verbose)
 	}
-	pterm.Success.Println("OpenFrame platform re-synced.")
+	if !flags.Silent {
+		pterm.Success.Println("OpenFrame platform re-synced.")
+	}
 	return nil
 }
 
@@ -203,7 +209,16 @@ func resolveUpgradeTarget(cmd *cobra.Command, args []string, flags *InstallFlags
 		if err != nil {
 			return nil, "", fmt.Errorf("could not use context %q: %w", contextName, err)
 		}
-		return cfg, clusterNameArg(args), nil
+		// A positional cluster name, if also given, is only a label for this
+		// --context-resolved config (it does not re-resolve the context), so
+		// only use it when the two cannot conflict: no --context-derived name
+		// of its own exists. Otherwise prefer contextName itself so the
+		// ClusterName reported in wait/status output matches the config that
+		// was actually resolved.
+		if name := clusterNameArg(args); name != "" {
+			return cfg, name, nil
+		}
+		return cfg, contextName, nil
 	}
 
 	if name := clusterNameArg(args); name != "" {

internal/chart/prerequisites/certificates/certificates.go

@@ -177,11 +177,11 @@ func (c *CertificateInstaller) generateCertificates() error {
 			if commandExists("apt-get") {
 				updateCmd := exec.Command("sudo", "apt-get", "update", "-y")
 				if err := updateCmd.Run(); err != nil {
-					pterm.Debug.Printf("apt-get update failed (certutil install is optional): %v\n", err)
+					pterm.Warning.Printf("apt-get update failed (certutil install is optional, NSS trust for Firefox/Chrome will not be configured): %v\n", err)
 				}
 				installCmd := exec.Command("sudo", "apt-get", "install", "-y", "libnss3-tools", "ca-certificates")
 				if err := installCmd.Run(); err != nil {
-					pterm.Debug.Printf("apt-get install of certutil/ca-certificates failed (optional): %v\n", err)
+					pterm.Warning.Printf("apt-get install of certutil/ca-certificates failed (optional, NSS trust for Firefox/Chrome will not be configured): %v\n", err)
 				}
 			}
 		}

internal/chart/providers/argocd/refassert.go

@@ -101,5 +101,6 @@
 		fmt.Fprintf(&b, "  - %s is on %q, not %q\n", x.App, x.Got, x.Want)
 	}
 	b.WriteString("Use a branch whose chart reads repository.branch, or pin these applications' targetRevision by hand.")
-	return fmt.Errorf("%s", b.String())
+	return selfDiagnosedError{msg: b.String()}
 }
+

internal/chart/providers/argocd/sync.go

@@ -246,6 +246,8 @@ type syncGroup struct {
 // (legacy manifests → caller keeps the ungated single-pass behaviour). A child
 // missing the label — or carrying a non-numeric value — on an otherwise
 // labeled install falls into defaultSyncGroup, mirroring the template default.
+// A non-numeric value (e.g. a typo like "1a") is surfaced with a warning
+// rather than silently absorbed, since it is likely a misconfiguration.
 func groupChildren(children []unstructured.Unstructured) (groups []syncGroup, labeled bool) {
 	byNumber := map[int][]string{}
 	var all []string
@@ -257,6 +259,9 @@ func groupChildren(children []unstructured.Unstructured) (groups []syncGroup, la
 			labeled = true
 			if n, err := strconv.Atoi(strings.TrimSpace(v)); err == nil {
 				group = n
+			} else {
+				pterm.Warning.Printf("Application %q has a non-numeric %s label (%q); defaulting to sync group %d\n",
+					name, SyncGroupLabel, v, defaultSyncGroup)
 			}
 		}
 		byNumber[group] = append(byNumber[group], name)

internal/chart/providers/argocd/wait.go

@@ -131,7 +131,9 @@ func (m *Manager) WaitForApplications(ctx context.Context, config config.ChartIn
 
 	// Function to stop spinner safely
 	stopSpinner := func() {
-		dash.Stop()
+		if dash != nil {
+			dash.Stop()
+		}
 		spinnerMutex.Lock()
 		defer spinnerMutex.Unlock()
 		if !spinnerStopped && spinner != nil {
@@ -290,7 +292,9 @@ func (m *Manager) WaitForApplications(ctx context.Context, config config.ChartIn
 		case <-ticker.C:
 			// Check timeout
 			if time.Since(startTime) > timeout {
-				dash.Fail(fmt.Sprintf("Timeout after %v", timeout))
+				if dash != nil {
+					dash.Fail(fmt.Sprintf("Timeout after %v", timeout))
+				}
 				spinnerMutex.Lock()
 				if !spinnerStopped && spinner != nil {
 					spinner.Fail(fmt.Sprintf("Timeout after %v", timeout))
@@ -467,7 +471,9 @@ func (m *Manager) WaitForApplications(ctx context.Context, config config.ChartIn
 			// staleness checks use the same tick.
 			now := time.Now()
 			if fatal := fatalManifest.observe(apps, now); len(fatal) > 0 {
-				dash.Fail("Applications cannot render manifests from the deployed revision")
+				if dash != nil {
+					dash.Fail("Applications cannot render manifests from the deployed revision")
+				}
 				spinnerMutex.Lock()
 				if !spinnerStopped && spinner != nil {
 					spinner.Fail("Applications cannot render manifests from the deployed revision")
@@ -489,7 +495,9 @@ func (m *Manager) WaitForApplications(ctx context.Context, config config.ChartIn
 			// events, so it says WHY, not just that it hung.
 			if cand := degraded.observe(apps, now); len(cand) > 0 {
 				if diag, stuck := m.diagnoseFailingApps(localCtx, cand); len(stuck) > 0 {
-					dash.Fail("An application is Degraded with a workload that will not recover")
+					if dash != nil {
+						dash.Fail("An application is Degraded with a workload that will not recover")
+					}
 					spinnerMutex.Lock()
 					if !spinnerStopped && spinner != nil {
 						spinner.Fail("An application is Degraded with a workload that will not recover")
@@ -541,7 +549,9 @@ func (m *Manager) WaitForApplications(ctx context.Context, config config.ChartIn
 			// this the default experience was a static "Installing ArgoCD
 			// applications..." for up to the full 60m timeout, with no way to tell
 			// a working install from a wedged one.
-			dash.Update(currentlyReady, totalApps, apps)
+			if dash != nil {
+				dash.Update(currentlyReady, totalApps, apps)
+			}
 			if totalApps > 0 {
 				spinnerMutex.Lock()
 				if !spinnerStopped && spinner != nil {
@@ -724,7 +734,9 @@ func (m *Manager) WaitForApplications(ctx context.Context, config config.ChartIn
 					spinnerMutex.Unlock()
 
 					if len(mm) > 0 {
-						dash.Fail("Deployed ref does not match the requested ref")
+						if dash != nil {
+							dash.Fail("Deployed ref does not match the requested ref")
+						}
 						return refMismatchError(config.AppOfApps.GitHubBranch, mm)
 					}
 

internal/cluster/discovery/eks.go

@@ -200,6 +200,9 @@ func (d *EKSDiscoverer) describe(ctx context.Context, profile, region, name stri
 	if err != nil {
 		return models.ClusterInfo{}, "", fmt.Errorf("describing cluster %s: %w", name, err)
 	}
+	if result == nil {
+		return models.ClusterInfo{}, "", fmt.Errorf("unparseable describe-cluster for %s", name)
+	}
 	var c eksCluster
 	if err := json.Unmarshal([]byte(result.Stdout), &c); err != nil {
 		return models.ClusterInfo{}, "", fmt.Errorf("unparseable describe-cluster for %s", name)

internal/cluster/discovery/gke.go

@@ -74,6 +74,9 @@ func (d *GKEDiscoverer) configurations(ctx context.Context) ([]gcloudConfigurati
 	if err != nil {
 		return nil, fmt.Errorf("listing gcloud configurations: %w", err)
 	}
+	if result == nil {
+		return nil, nil
+	}
 	var configs []gcloudConfiguration
 	if err := json.Unmarshal([]byte(result.Stdout), &configs); err != nil {
 		return nil, fmt.Errorf("parsing gcloud configurations: %w", err)

internal/cluster/providers/gke/provider.go

@@ -26,6 +26,13 @@ import (
 // out of a terraform 409 error so an orphan can be named concretely.
 var gcpResourcePathRE = regexp.MustCompile(`projects/[^'"\s]+`)
 
+// safeClusterNameRE matches GKE's own cluster-name constraints (lowercase
+// alphanumeric and hyphens, starting with a letter). Any name that fails this
+// check cannot be a valid GKE cluster name anyway, so rejecting it here also
+// guarantees it can never be used to inject syntax into a gcloud --filter
+// expression (spaces, "OR", parentheses, quotes, etc.).
+var safeClusterNameRE = regexp.MustCompile(`^[a-z]([-a-z0-9]*[a-z0-9])?$`)
+
 // orphanFromInterruptedCreate detects the specific failure where terraform
 // tries to create a resource that already exists in GCP (HTTP 409 /
 // alreadyExists). This is the signature of a create interrupted (SIGINT) after
@@ -153,7 +160,17 @@ func (p *Provider) ensureProjectServices(ctx context.Context, project string) er
 // line equal to the cluster name. Anything else — non-zero exit, empty or
 // unrelated output — is treated as "does not exist"; a genuinely broken API
 // call fails later with a clearer terraform error anyway.
+//
+// config.Name is rejected up front unless it matches GKE's own cluster-name
+// character set (lowercase alphanumeric and hyphens). This is not just
+// defense in depth: it also guarantees the name cannot contain characters
+// meaningful to gcloud's --filter expression syntax (spaces, "OR",
+// parentheses, quotes), so it cannot alter the filter's semantics and bypass
+// this collision guard.
 func (p *Provider) preflightNameCollision(ctx context.Context, config models.ClusterConfig) error {
+	if !safeClusterNameRE.MatchString(config.Name) {
+		return fmt.Errorf("cluster name %q is not a valid GKE cluster name (must match %s)", config.Name, safeClusterNameRE.String())
+	}
 	result, err := p.executor.Execute(ctx, "gcloud", "container", "clusters", "list",
 		"--project", config.Cloud.Project, "--filter=name="+config.Name, "--format=value(name)")
 	if err != nil || result == nil {

internal/cluster/providers/terraform/engine.go

@@ -13,6 +13,7 @@ import (
 	"github.com/flamingo-stack/openframe-cli/internal/shared/download"
 	"github.com/hashicorp/terraform-exec/tfexec"
 	tfjson "github.com/hashicorp/terraform-json"
+	"github.com/pterm/pterm"
 )
 
 // Runner is the subset of *tfexec.Terraform the engine uses; an interface so
@@ -145,16 +146,25 @@ const OpLogName = "terraform.log"
 // propagates a sink error: exec.Cmd returns a stdout-writer error from Wait,
 // so a disk filling up mid-apply would report the terraform run as FAILED
 // while terraform actually completed and changed resources. The log is a
-// record of the operation — it must never decide its outcome.
+// record of the operation — it must never decide its outcome. The first sink
+// write failure is still surfaced once, at WARN level, so a user relying on
+// OpLogName for post-mortem debugging isn't silently left with a truncated
+// log and no indication it happened.
 type bestEffortTee struct {
 	progress io.Writer
 	sink     io.Writer // nil once a write failed; never re-enabled
+	logPath  string    // used only for the one-time warning below
+	warned   bool
 }
 
 func (t *bestEffortTee) Write(p []byte) (int, error) {
 	if t.sink != nil {
 		if _, err := t.sink.Write(p); err != nil {
 			t.sink = nil
+			if !t.warned {
+				t.warned = true
+				pterm.Warning.Printfln("operation log %s stopped recording (write error: %v); the on-disk log will be incomplete", t.logPath, err)
+			}
 		}
 	}
 	return t.progress.Write(p)
@@ -164,7 +174,8 @@ func (t *bestEffortTee) Write(p []byte) (int, error) {
 // writer, teed into dir's terraform.log when it can be opened. Logging is
 // best-effort end to end — a directory that cannot take the log (read-only,
 // gone) skips it, and a write failure after opening only stops the mirroring
-// (bestEffortTee above); neither may ever fail the operation itself.
+// (bestEffortTee above, which also warns once); neither may ever fail the
+// operation itself.
 // The returned close is always safe to call; logPath is empty when no log is
 // being written.
 func (e *Engine) opSinks(dir, op string) (w io.Writer, close func(), logPath string) {
@@ -173,8 +184,12 @@ func (e *Engine) opSinks(dir, op string) (w io.Writer, close func(), logPath str
 	if err != nil {
 		return progress, func() {}, ""
 	}
-	fmt.Fprintf(f, "=== terraform %s — %s ===\n", op, time.Now().UTC().Format(time.RFC3339))
-	return &bestEffortTee{progress: progress, sink: f}, func() { _ = f.Close() }, f.Name()
+	if _, err := fmt.Fprintf(f, "=== terraform %s — %s ===\n", op, time.Now().UTC().Format(time.RFC3339)); err != nil {
+		pterm.Warning.Printfln("operation log %s could not be written to (%v); the on-disk log will be incomplete", f.Name(), err)
+		_ = f.Close()
+		return progress, func() {}, ""
+	}
+	return &bestEffortTee{progress: progress, sink: f, logPath: f.Name()}, func() { _ = f.Close() }, f.Name()
 }
 
 // opFailure wraps a failed apply/destroy, pointing at the full log when one