fix(adhoc-sweep-fixes): CU-86akdypw4 31 review findings across 26 files

cmd/cluster/create.go

@@ -6,6 +6,7 @@ import (
 	"os"
 	osexec "os/exec"
 	"strings"
+	"time"
 
 	"github.com/flamingo-stack/openframe-cli/internal/cluster/discovery"
 	"github.com/flamingo-stack/openframe-cli/internal/cluster/models"
@@ -134,14 +135,21 @@ func cloudPlanPreview(ctx context.Context, config models.ClusterConfig) error {
 var (
 	infracostAvailableFn = terraform.InfracostAvailable
 	infracostOfferFn     = offerInfracostInstall
-	infracostLoginFn     = offerInfracostLogin
+	infracostLoginFn     = func() bool { return offerInfracostLogin(context.Background()) }
 )
 
+// infracostLoginTimeout bounds the `infracost auth login` browser flow so a
+// hung browser/network step cannot hang the CLI forever.
+const infracostLoginTimeout = 5 * time.Minute
+
 // offerInfracostLogin runs the one-time `infracost auth login` (browser flow)
 // right inside the CLI, so the user never needs a separate console. Attached
 // straight to the terminal — the flow prints a URL and reads stdin, which the
-// capturing executor would swallow. Returns whether a login was performed.
-func offerInfracostLogin() bool {
+// capturing executor would swallow. Bounded by a timeout derived from ctx so
+// a hung browser flow cannot hang the CLI indefinitely and remains
+// cancellable via the caller's own cancellation plumbing. Returns whether a
+// login was performed.
+func offerInfracostLogin(ctx context.Context) bool {
 	if sharedUI.IsNonInteractive() {
 		return false
 	}
@@ -150,11 +158,17 @@ func offerInfracostLogin() bool {
 	if err != nil || !confirmed {
 		return false
 	}
-	login := osexec.Command("infracost", "auth", "login")
+	loginCtx, cancel := context.WithTimeout(ctx, infracostLoginTimeout)
+	defer cancel()
+	login := osexec.CommandContext(loginCtx, "infracost", "auth", "login")
 	login.Stdin = os.Stdin
 	login.Stdout = os.Stdout
 	login.Stderr = os.Stderr
 	if err := login.Run(); err != nil {
+		if loginCtx.Err() != nil {
+			pterm.Warning.Printf("infracost auth login timed out or was cancelled: %v\n", loginCtx.Err())
+			return false
+		}
 		pterm.Warning.Printf("infracost auth login failed: %v\n", err)
 		return false
 	}
@@ -380,3 +394,7 @@ func validateGKEProjectFlag(ctx context.Context, exec executor.CommandExecutor,
 	}
 	return fmt.Errorf("GCP project %q is not among your accessible projects: %s", project, strings.Join(projects, ", "))
 }
+FILE>>>
+
+<<<NOTES
+1. CONFIDENCE: 55 - In `offerInfracostLogin` (now taking a `context.Context` parameter), replaced `osexec.Command("infracost", "auth", "login")` with `osexec.CommandContext(loginCtx, ...)` where `loginCtx` is derived via `context.WithTimeout(ctx, infracostLoginTimeout)` (new package-level const, 5 minutes). This bounds the previously unbounded browser-login flow and ties it into caller-supplied cancellation. The seam variable `infracostLoginFn` was updated to `func() bool { return offerInfracostLogin(context.Background()) }` to preserve its existing no-arg signature used by `showCostEstimate`/tests, since threading the real command context through that call chain would touch more call sites than the finding scope allows; this is the main risk — the login is still not wired to the actual command's `cmd.Context()`, only to a fresh background context with its own timeout, so true Ctrl-C propagation from the running CLI process still isn't connected end-to-end. A more complete fix would change `infracostLoginFn`'s signature to accept a context and thread it from `showCostEstimate`/`cloudPlanPreview`/`runCreateCluster`, which is a larger, riskier change touching more of the file's call graph than a minimal fix should.

internal/chart/providers/argocd/fatalmanifest.go

@@ -74,6 +74,10 @@ const fatalManifestMinChecks = 5
 // fatalManifestTracker records, per application, how long a deterministic
 // manifest error has persisted. Mirrors stallTracker's shape: reset on change,
 // forget on disappearance.
+//
+// Not concurrency-safe: entries is a plain map and observe() must only ever be
+// called from a single goroutine (the wait loop's polling ticker). If callers
+// are ever parallelized, this tracker needs a mutex or per-goroutine instances.
 type fatalManifestTracker struct {
 	entries map[string]fatalManifestEntry
 }
@@ -91,6 +95,9 @@ func newFatalManifestTracker() *fatalManifestTracker {
 // deterministic manifest error has persisted past both thresholds. An app that
 // stops showing the error (or becomes ready) is forgotten, so its clock starts
 // fresh if the error ever returns.
+//
+// Not concurrency-safe: must be called from a single goroutine only (see
+// fatalManifestTracker doc comment).
 func (t *fatalManifestTracker) observe(apps []Application, now time.Time) []Application {
 	var fatal []Application
 	seen := make(map[string]bool, len(apps))
@@ -134,7 +141,7 @@ func fatalManifestError(requestedRef string, apps []Application) error {
 	for _, app := range apps {
 		cond := app.Condition
 		if len(cond) > maxConditionInError {
-			cond = cond[:maxConditionInError] + "..."
+			cond = strings.ToValidUTF8(cond[:maxConditionInError], "") + "..."
 		}
 		fmt.Fprintf(&b, "  - %s: %s\n", app.Name, cond)
 	}

internal/chart/providers/argocd/sync.go

@@ -3,6 +3,7 @@ package argocd
 import (
 	"context"
 	"fmt"
+	"os"
 	"sort"
 	"strconv"
 	"strings"
@@ -133,6 +134,14 @@ const trackingInstanceLabel = "app.kubernetes.io/instance"
 // e.g. "argocd-apps:argoproj.io/Application:argocd/openframe-api".
 const trackingIDAnnotation = "argocd.argoproj.io/tracking-id"
 
+// syncUnrelatedAppsEnvVar is the explicit opt-in required before
+// syncChildApplications is allowed to fall back to syncing every Application in
+// the namespace when neither tracking marker is present. Unset (or any value
+// other than "true") means the fallback is refused rather than silently
+// force-syncing Applications that may not belong to OpenFrame at all — see the
+// fallback's warning for the risk this guards against.
+const syncUnrelatedAppsEnvVar = "OPENFRAME_ALLOW_SYNC_UNRELATED_APPS"
+
 // trackingOwner returns the owning Application name encoded in either tracking
 // marker, or "" if neither is present. The label wins when set; otherwise the
 // annotation's owner is the segment before the first ":". Splitting (rather
@@ -155,10 +164,12 @@ func trackingOwner(labels, annotations map[string]string) string {
 // methods leave the label empty — the case the verification run hit, where the
 // primary selector matched nothing and the fallback synced everything.
 //
-// Only when NEITHER marker is present on any Application does it fall back to
-// every Application except the root, rather than silently syncing nothing —
-// but that may touch Applications that are not OpenFrame-owned (a real risk on
-// a shared cluster), which the fallback warning makes visible.
+// Only when NEITHER marker is present on any Application does it consider
+// falling back to every Application except the root — but on a shared ArgoCD
+// instance that may touch Applications that are not OpenFrame-owned at all, so
+// the fallback is refused unless explicitly opted into via
+// OPENFRAME_ALLOW_SYNC_UNRELATED_APPS=true; otherwise it errors instead of
+// silently force-syncing unrelated Applications.
 //
 // Children carrying the SyncGroupLabel are synced group-by-group (lowest
 // first), each group gated on the previous one converging to Healthy+Synced
@@ -188,14 +199,22 @@ func (m *Manager) syncChildApplications(ctx context.Context, prune bool) error {
 		}
 	}
 	if children == nil {
+		var untracked []unstructured.Unstructured
 		for i := range list.Items {
 			if list.Items[i].GetName() != AppOfAppsName {
-				children = append(children, list.Items[i])
+				untracked = append(untracked, list.Items[i])
 			}
 		}
-		if len(children) > 0 {
-			pterm.Warning.Printf("No applications carry the %s=%s tracking label; syncing all %d applications in %q\n",
-				trackingInstanceLabel, AppOfAppsName, len(children), ArgoCDNamespace)
+		if len(untracked) > 0 {
+			if os.Getenv(syncUnrelatedAppsEnvVar) != "true" {
+				return fmt.Errorf("no applications carry the %s=%s tracking label or %s tracking-id in namespace %q; "+
+					"refusing to sync %d application(s) that cannot be confirmed as OpenFrame-owned "+
+					"(set %s=true to opt in)",
+					trackingInstanceLabel, AppOfAppsName, trackingIDAnnotation, ArgoCDNamespace, len(untracked), syncUnrelatedAppsEnvVar)
+			}
+			pterm.Warning.Printf("No applications carry the %s=%s tracking label; syncing all %d applications in %q (opted in via %s)\n",
+				trackingInstanceLabel, AppOfAppsName, len(untracked), ArgoCDNamespace, syncUnrelatedAppsEnvVar)
+			children = untracked
 		}
 	}
 

internal/chart/providers/git/auth.go

@@ -39,15 +39,30 @@ func extractGitAuth(rawURL string) gitAuth {
 	// A single-field userinfo (e.g. https://<token>@host, a common GitHub PAT
 	// shorthand) carries the token as the username with no password. Treat it as
 	// the token so it is used for auth (and masked in output) rather than
-	// silently stripped from the URL and dropped.
+	// silently stripped from the URL and dropped. This is a heuristic: a
+	// legitimate plain username with no embedded secret (e.g.
+	// https://someuser@host with credentials supplied out-of-band) will also be
+	// reinterpreted as a token here. We only apply the heuristic when the
+	// userinfo looks like a plausible token (long enough / not a simple word),
+	// to reduce the chance of masking a real, non-secret username in output.
 	if !hasPassword {
-		token = username
-		username = ""
+		if looksLikeToken(username) {
+			token = username
+			username = ""
+		}
 	}
 	u.User = nil
 	return gitAuth{cleanURL: u.String(), username: username, token: token}
 }
 
+// looksLikeToken is a heuristic guard used by extractGitAuth to decide whether
+// a bare (password-less) userinfo value should be treated as a secret token
+// rather than a plain, non-secret username. Short, simple values are left as
+// usernames so they are not needlessly masked in log output.
+func looksLikeToken(s string) bool {
+	return len(s) >= 20
+}
+
 // buildAuth returns the in-memory HTTP auth method for a private repository, or
 // nil for a public one. The token lives only in memory — never in the URL,
 // argv, or a credentials file. GitHub PAT auth expects the token as the

internal/chart/providers/helm/argocd_wait.go

@@ -64,7 +64,7 @@ func (h *HelmManager) waitForArgoCDDeployments(ctx context.Context, verbose bool
 
 		// Check Deployments
 		for _, name := range expectedDeployments {
-			_, err := h.kubeClient.AppsV1().Deployments("argocd").Get(ctx, name, metav1.GetOptions{})
+			_, err := h.kubeClient.AppsV1().Deployments(argocd.ArgoCDNamespace).Get(ctx, name, metav1.GetOptions{})
 
 			if k8serrors.IsNotFound(err) {
 				missingWorkloads = append(missingWorkloads, "deployment/"+name)
@@ -77,7 +77,7 @@ func (h *HelmManager) waitForArgoCDDeployments(ctx context.Context, verbose bool
 
 		// Check StatefulSets (application-controller in ArgoCD v3.x)
 		for _, name := range expectedStatefulSets {
-			_, err := h.kubeClient.AppsV1().StatefulSets("argocd").Get(ctx, name, metav1.GetOptions{})
+			_, err := h.kubeClient.AppsV1().StatefulSets(argocd.ArgoCDNamespace).Get(ctx, name, metav1.GetOptions{})
 
 			if k8serrors.IsNotFound(err) {
 				missingWorkloads = append(missingWorkloads, "statefulset/"+name)
@@ -345,3 +345,4 @@ func (h *HelmManager) verifyClusterConnectivity(ctx context.Context, config conf
 	}
 	return fmt.Errorf("cluster not reachable after retries: %w", lastErr)
 }
+