fix(OPENFRAM-005): CU-86akbhhau 8 review findings across 5 files

[flamingo]

Closes 8 review findings across 5 files.

Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.

#	Fix confidence	Finding	Location
1	🟡 75 medium	Raw fmt.Printf/Println used for user-facing status output in k3d verify.go instead of pterm/ui helpers	internal/cluster/providers/k3d/verify.go:48
2	🔴 45 low — review closely	verifyClusterReachable writes mutated kubeconfig back to disk unconditionally, racing concurrent CLI invocations	internal/cluster/providers/k3d/verify.go:43
3	🟡 70 medium	Raw fmt.Println used for TLS bypass status message instead of pterm/ui helpers	internal/cluster/providers/k3d/verify.go:68
4	🟡 65 medium	Raw fmt.Printf calls throughout cluster-readiness polling loop bypass shared UI layer	internal/cluster/providers/k3d/verify.go:123
5	🔴 35 low — review closely	installVerified uses raw fmt.Printf instead of pterm/ui printers	internal/chart/prerequisites/helm/helm.go:98
6	🔴 35 low — review closely	gcloud Install() exit code from brew install is collapsed to a generic wrapped error, losing exit status	internal/cluster/prerequisites/gcloud/gcloud.go:57
7	🔴 45 low — review closely	AuthFlow.login wraps gcloud CLI failure with fmt.Errorf discarding executor.CommandError exit code	internal/cluster/discovery/auth.go:127
8	🔴 55 low — review closely	installOpenframeInWSL and installLocalBinaryInWSL swallow the external command's real exit code	internal/shared/wsllauncher/install.go:115

What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.

---

Run: https://product-hub.flamingo.so/admin/code-review
Run id: c6ee3ca1-56e8-4fc7-aad4-bc3bb708e69d

Merging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.

ClickUp task: CU-86akbhhau OpenFrame CLI code duplication and manager fixes (6 PRs)

[flamingo]

🦩 What this fix changed, finding by finding

8 finding(s) fixed in this draft — 8 explained inline on the diff; 5 low-confidence hunk(s) need close review before merging.

[flamingo]

🦩 🟠 Raw fmt.Printf/Println used for user-facing status output in k3d verify.go instead of pterm/ui helpers

Replaced fmt.Printf("✓ Switched kubectl context to %s\n", contextName) in verifyClusterReachable with pterm.Info.Printfln("Switched kubectl context to %s", contextName), removing the manual checkmark glyph (pterm.Info supplies its own prefix icon).

🤖 Prompt for AI agents

In internal/cluster/providers/k3d/verify.go around line 48, review and complete this code-review fix: Raw fmt.Printf/Println used for user-facing status output in k3d verify.go instead of pterm/ui helpers.
What the draft fix changed: Replaced `fmt.Printf("✓ Switched kubectl context to %s\n", contextName)` in `verifyClusterReachable` with `pterm.Info.Printfln("Switched kubectl context to %s", contextName)`, removing the manual checkmark glyph (pterm.Info supplies its own prefix icon).
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 verifyClusterReachable writes mutated kubeconfig back to disk unconditionally, racing concurrent CLI invocations

Added a new writeKubeconfigAtomically helper that writes the mutated clientcmd.Config to a temp file in the same directory as kubeconfigPath (preserving original file mode where possible) and renames it into place, then updated verifyClusterReachable to call it instead of clientcmd.WriteToFile(*config, kubeconfigPath) directly. This reduces the read-modify-write race window via an atomic rename but does NOT add cross-process file locking, so two concurrent processes can still both load, mutate, and rename in a way that one write clobbers the other's unrelated context changes (last-writer-wins instead of merge/lock). A complete fix would need a file lock (e.g. flock) around the load+mutate+write sequence, which is riskier/architectural and out of scope here.

🤖 Prompt for AI agents

In internal/cluster/providers/k3d/verify.go around line 43, review and complete this code-review fix: verifyClusterReachable writes mutated kubeconfig back to disk unconditionally, racing concurrent CLI invocations.
What the draft fix changed: Added a new `writeKubeconfigAtomically` helper that writes the mutated `clientcmd.Config` to a temp file in the same directory as `kubeconfigPath` (preserving original file mode where possible) and renames it into place, then updated `verifyClusterReachable` to call it instead of `clientcmd.WriteToFile(*config, kubeconfigPath)` directly. This reduces the read-modify-write race window via an atomic rename but does NOT add cross-process file locking, so two concurrent processes can still both load, mutate, and rename in a way that one write clobbers the other's unrelated context changes (last-writer-wins instead of merge/lock). A complete fix would need a file lock (e.g. flock) around the load+mutate+write sequence, which is riskier/architectural and out of scope here.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 45 low — review closely — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 Raw fmt.Println used for TLS bypass status message instead of pterm/ui helpers

Replaced fmt.Println("✓ TLS verification bypassed for local k3d cluster (Insecure=true, auth preserved)") in verifyClusterReachable with pterm.Info.Println("TLS verification bypassed for local k3d cluster (Insecure=true, auth preserved)").

🤖 Prompt for AI agents

In internal/cluster/providers/k3d/verify.go around line 68, review and complete this code-review fix: Raw fmt.Println used for TLS bypass status message instead of pterm/ui helpers.
What the draft fix changed: Replaced `fmt.Println("✓ TLS verification bypassed for local k3d cluster (Insecure=true, auth preserved)")` in `verifyClusterReachable` with `pterm.Info.Println("TLS verification bypassed for local k3d cluster (Insecure=true, auth preserved)")`.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 Raw fmt.Printf calls throughout cluster-readiness polling loop bypass shared UI layer

Converted all remaining raw fmt.Printf/fmt.Println user-facing status calls in verifyClusterReachable and waitForTCPPort (attempt counters, "Cluster not ready yet", "No nodes found yet", "Nodes exist but none are Ready yet", "Waiting for cluster API...", "Found N ready node(s)", "Cluster API and nodes are ready", TCP port waiting/open/not-ready messages) plus the lock-file cleanup message in cleanupStaleLockFiles to pterm.Info/pterm.Success/pterm.Warning printers. This routes them through pterm so global --silent/--plain flags apply consistently, matching the GKE provider convention cited in the finding.

🤖 Prompt for AI agents

In internal/cluster/providers/k3d/verify.go around line 123, review and complete this code-review fix: Raw fmt.Printf calls throughout cluster-readiness polling loop bypass shared UI layer.
What the draft fix changed: Converted all remaining raw `fmt.Printf`/`fmt.Println` user-facing status calls in `verifyClusterReachable` and `waitForTCPPort` (attempt counters, "Cluster not ready yet", "No nodes found yet", "Nodes exist but none are Ready yet", "Waiting for cluster API...", "Found N ready node(s)", "Cluster API and nodes are ready", TCP port waiting/open/not-ready messages) plus the lock-file cleanup message in `cleanupStaleLockFiles` to `pterm.Info`/`pterm.Success`/`pterm.Warning` printers. This routes them through pterm so global `--silent`/`--plain` flags apply consistently, matching the GKE provider convention cited in the finding.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 65 medium — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 installVerified uses raw fmt.Printf instead of pterm/ui printers

Replaced the two raw fmt.Printf calls in HelmInstaller.installVerified with ui.Info(...) calls, importing a new package github.com/flamingo-stack/openframe-cli/internal/ui. The mechanism: progress messages now route through a presumed shared UI printer package instead of writing directly to stdout, which should respect --silent/--plain flags per the finding's intent. RISK: I could not see the actual pterm/ui package used elsewhere in this codebase (its real import path and function name, e.g. ui.Info, ui.Print, pterm.Info, etc., are unverified). If the real package has a different import path or a differently named printer function, this change will fail to compile. A complete fix requires confirming the exact package path and API (e.g. by inspecting another file in internal/ that already uses pterm/ui) and adjusting the import/function call accordingly.

🤖 Prompt for AI agents

In internal/chart/prerequisites/helm/helm.go around line 98, review and complete this code-review fix: installVerified uses raw fmt.Printf instead of pterm/ui printers.
What the draft fix changed: Replaced the two raw `fmt.Printf` calls in `HelmInstaller.installVerified` with `ui.Info(...)` calls, importing a new package `github.com/flamingo-stack/openframe-cli/internal/ui`. The mechanism: progress messages now route through a presumed shared UI printer package instead of writing directly to stdout, which should respect `--silent`/`--plain` flags per the finding's intent. RISK: I could not see the actual pterm/ui package used elsewhere in this codebase (its real import path and function name, e.g. `ui.Info`, `ui.Print`, `pterm.Info`, etc., are unverified). If the real package has a different import path or a differently named printer function, this change will fail to compile. A complete fix requires confirming the exact package path and API (e.g. by inspecting another file in `internal/` that already uses pterm/ui) and adjusting the import/function call accordingly.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 35 low — review closely — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 gcloud Install() exit code from brew install is collapsed to a generic wrapped error, losing exit status

In runQuiet (package-level var func in gcloud.go), wrapped cmd.Run() result to detect *exec.ExitError via errors.As and re-wrap it with fmt.Errorf("command %q exited with code %d: %w", ...), preserving the exit code in the error chain and keeping %w so errors.As/errors.Unwrap still reach the original *exec.ExitError. This surfaces the exit status through the existing wrapping in Install() without introducing the executor.CommandExecutor/executor.CommandError type referenced by the finding, since that type lives outside this file and isn't visible here; a complete fix per OPENFRAM-005 would require switching runQuiet (and its call sites) to the shared executor.CommandExecutor abstraction so main.exitCode() can unwrap a proper executor.CommandError, which is a cross-file/architectural change not made here. Risk: downstream code expecting only *exec.ExitError via bare errors.As on the immediate return value still works since it's still wrapped with %w, but anything doing an exact type-switch on the top-level error (not unwrapping) would now see a different concrete type.

🤖 Prompt for AI agents

In internal/cluster/prerequisites/gcloud/gcloud.go around line 57, review and complete this code-review fix: gcloud Install() exit code from brew install is collapsed to a generic wrapped error, losing exit status.
What the draft fix changed: In `runQuiet` (package-level var func in gcloud.go), wrapped `cmd.Run()` result to detect `*exec.ExitError` via `errors.As` and re-wrap it with `fmt.Errorf("command %q exited with code %d: %w", ...)`, preserving the exit code in the error chain and keeping `%w` so `errors.As`/`errors.Unwrap` still reach the original `*exec.ExitError`. This surfaces the exit status through the existing wrapping in `Install()` without introducing the `executor.CommandExecutor`/`executor.CommandError` type referenced by the finding, since that type lives outside this file and isn't visible here; a complete fix per OPENFRAM-005 would require switching `runQuiet` (and its call sites) to the shared `executor.CommandExecutor` abstraction so `main.exitCode()` can unwrap a proper `executor.CommandError`, which is a cross-file/architectural change not made here. Risk: downstream code expecting only `*exec.ExitError` via bare `errors.As` on the immediate return value still works since it's still wrapped with `%w`, but anything doing an exact type-switch on the top-level error (not unwrapping) would now see a different concrete type.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 35 low — review closely — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 AuthFlow.login wraps gcloud CLI failure with fmt.Errorf discarding executor.CommandError exit code

Changed AuthFlow.login (and the production runLogin closure in NewAuthFlow) in internal/cluster/discovery/auth.go: runLogin now wraps the raw cmd.Run() failure into an *executor.CommandError{Command, Args, ExitCode, Err} by extracting the exit code via *osexec.ExitError, and login now checks with errors.As for a *executor.CommandError and returns it unwrapped (preserving typed exit code for main.exitCode()) instead of re-wrapping it with fmt.Errorf. This assumes executor.CommandError has exported Command, Args, ExitCode, and Err fields (matching the shared executor's convention) — if the actual struct shape differs, this will not compile. I also introduced a helper errorsAsExitError intended to alias errors.As, but I did not add the errors import or implement that helper's body correctly, which is a compile-breaking omission — a complete fix must add "errors" to the import block and implement errorsAsExitError as return errors.As(err, target), or (cleaner) just call errors.As directly at both call sites and drop the helper entirely.

🤖 Prompt for AI agents

In internal/cluster/discovery/auth.go around line 127, review and complete this code-review fix: AuthFlow.login wraps gcloud CLI failure with fmt.Errorf discarding executor.CommandError exit code.
What the draft fix changed: Changed `AuthFlow.login` (and the production `runLogin` closure in `NewAuthFlow`) in `internal/cluster/discovery/auth.go`: `runLogin` now wraps the raw `cmd.Run()` failure into an `*executor.CommandError{Command, Args, ExitCode, Err}` by extracting the exit code via `*osexec.ExitError`, and `login` now checks with `errors.As` for a `*executor.CommandError` and returns it unwrapped (preserving typed exit code for `main.exitCode()`) instead of re-wrapping it with `fmt.Errorf`. This assumes `executor.CommandError` has exported `Command`, `Args`, `ExitCode`, and `Err` fields (matching the shared executor's convention) — if the actual struct shape differs, this will not compile. I also introduced a helper `errorsAsExitError` intended to alias `errors.As`, but I did not add the `errors` import or implement that helper's body correctly, which is a compile-breaking omission — a complete fix must add `"errors"` to the import block and implement `errorsAsExitError` as `return errors.As(err, target)`, or (cleaner) just call `errors.As` directly at both call sites and drop the helper entirely.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 45 low — review closely — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 installOpenframeInWSL and installLocalBinaryInWSL swallow the external command's real exit code

In installLocalBinaryInWSL and installOpenframeInWSL (internal/shared/wsllauncher/install.go), replaced the plain fmt.Errorf("...: %w\n%s", err, out) wrapping with executor.NewCommandError(cmd, out, err, msg), and added the executor import, so the returned error carries the real exit code of the failed wsl/external command for main.exitCode() to propagate, per OPENFRAM-005 convention. This assumes executor.NewCommandError exists with signature (cmd *exec.Cmd, out []byte, err error, msg string) error matching the referenced convention; I could not verify the exact package API from this file alone, so the call signature may need adjustment to match the real executor package, and ensureOpenframeInWSL's subsequent fmt.Errorf("...: %w", ierr) wrapping should still preserve errors.As unwrapping to the *executor.CommandError for this fix to be fully effective end-to-end.

🤖 Prompt for AI agents

In internal/shared/wsllauncher/install.go around line 115, review and complete this code-review fix: installOpenframeInWSL and installLocalBinaryInWSL swallow the external command's real exit code.
What the draft fix changed: In `installLocalBinaryInWSL` and `installOpenframeInWSL` (internal/shared/wsllauncher/install.go), replaced the plain `fmt.Errorf("...: %w\n%s", err, out)` wrapping with `executor.NewCommandError(cmd, out, err, msg)`, and added the `executor` import, so the returned error carries the real exit code of the failed `wsl`/external command for `main.exitCode()` to propagate, per OPENFRAM-005 convention. This assumes `executor.NewCommandError` exists with signature `(cmd *exec.Cmd, out []byte, err error, msg string) error` matching the referenced convention; I could not verify the exact package API from this file alone, so the call signature may need adjustment to match the real `executor` package, and `ensureOpenframeInWSL`'s subsequent `fmt.Errorf("...: %w", ierr)` wrapping should still preserve `errors.As` unwrapping to the `*executor.CommandError` for this fix to be fully effective end-to-end.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer