Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
flamingo wants to merge 5 commits into
base: main
Choose a base branch
from
+87 −20
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7dde735
fix(OPENFRAM-005): 8 review findings across 5 files
flamingo[bot] Sep 7, 2026
77d2c7c
fix(OPENFRAM-005): 8 review findings across 5 files
flamingo[bot] Sep 7, 2026
94fa69d
fix(OPENFRAM-005): 8 review findings across 5 files
flamingo[bot] Sep 7, 2026
a2a1ccb
fix(OPENFRAM-005): 8 review findings across 5 files
flamingo[bot] Sep 7, 2026
088607d
fix(OPENFRAM-005): 8 review findings across 5 files
flamingo[bot] Sep 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions internal/chart/prerequisites/helm/helm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
"github.com/flamingo-stack/openframe-cli/internal/platform"
"github.com/flamingo-stack/openframe-cli/internal/shared/download"
"github.com/flamingo-stack/openframe-cli/internal/shared/wsllauncher"
"github.com/flamingo-stack/openframe-cli/internal/ui"

Check failure on line 13 in internal/chart/prerequisites/helm/helm.go

View workflow job for this annotation

GitHub Actions / Unit tests on linux-amd64 

no required module provides package github.com/flamingo-stack/openframe-cli/internal/ui; to add it:

Check failure on line 13 in internal/chart/prerequisites/helm/helm.go

View workflow job for this annotation

GitHub Actions / Unit tests on linux-amd64 

no required module provides package github.com/flamingo-stack/openframe-cli/internal/ui; to add it:

Check failure on line 13 in internal/chart/prerequisites/helm/helm.go

View workflow job for this annotation

GitHub Actions / Unit tests on linux-amd64 

no required module provides package github.com/flamingo-stack/openframe-cli/internal/ui; to add it:

Check failure on line 13 in internal/chart/prerequisites/helm/helm.go

View workflow job for this annotation

GitHub Actions / Unit tests on linux-amd64 

no required module provides package github.com/flamingo-stack/openframe-cli/internal/ui; to add it:

Check failure on line 13 in internal/chart/prerequisites/helm/helm.go

View workflow job for this annotation

GitHub Actions / Unit tests on linux-amd64 

no required module provides package github.com/flamingo-stack/openframe-cli/internal/ui; to add it:

Check failure on line 13 in internal/chart/prerequisites/helm/helm.go

View workflow job for this annotation

GitHub Actions / Unit tests on linux-amd64 

no required module provides package github.com/flamingo-stack/openframe-cli/internal/ui; to add it:

Check failure on line 13 in internal/chart/prerequisites/helm/helm.go

View workflow job for this annotation

GitHub Actions / Unit tests on linux-amd64 

no required module provides package github.com/flamingo-stack/openframe-cli/internal/ui; to add it:

Check failure on line 13 in internal/chart/prerequisites/helm/helm.go

View workflow job for this annotation

GitHub Actions / Unit tests on linux-amd64 

no required module provides package github.com/flamingo-stack/openframe-cli/internal/ui; to add it:

Check failure on line 13 in internal/chart/prerequisites/helm/helm.go

View workflow job for this annotation

GitHub Actions / Release build matrix (compile-only) 

no required module provides package github.com/flamingo-stack/openframe-cli/internal/ui; to add it:

Check failure on line 13 in internal/chart/prerequisites/helm/helm.go

View workflow job for this annotation

GitHub Actions / Unit tests on windows-amd64 

no required module provides package github.com/flamingo-stack/openframe-cli/internal/ui; to add it:

Check failure on line 13 in internal/chart/prerequisites/helm/helm.go

View workflow job for this annotation

GitHub Actions / Unit tests on windows-amd64 

no required module provides package github.com/flamingo-stack/openframe-cli/internal/ui; to add it:

Check failure on line 13 in internal/chart/prerequisites/helm/helm.go

View workflow job for this annotation

GitHub Actions / Unit tests on windows-amd64 

no required module provides package github.com/flamingo-stack/openframe-cli/internal/ui; to add it:

Check failure on line 13 in internal/chart/prerequisites/helm/helm.go

View workflow job for this annotation

GitHub Actions / Unit tests on windows-amd64 

no required module provides package github.com/flamingo-stack/openframe-cli/internal/ui; to add it:

Check failure on line 13 in internal/chart/prerequisites/helm/helm.go

View workflow job for this annotation

GitHub Actions / Unit tests on windows-amd64 

no required module provides package github.com/flamingo-stack/openframe-cli/internal/ui; to add it:

Check failure on line 13 in internal/chart/prerequisites/helm/helm.go

View workflow job for this annotation

GitHub Actions / Unit tests on windows-amd64 

no required module provides package github.com/flamingo-stack/openframe-cli/internal/ui; to add it:

Check failure on line 13 in internal/chart/prerequisites/helm/helm.go

View workflow job for this annotation

GitHub Actions / Unit tests on windows-amd64 

no required module provides package github.com/flamingo-stack/openframe-cli/internal/ui; to add it:

Check failure on line 13 in internal/chart/prerequisites/helm/helm.go

View workflow job for this annotation

GitHub Actions / Unit tests on windows-amd64 

no required module provides package github.com/flamingo-stack/openframe-cli/internal/ui; to add it:

Check failure on line 13 in internal/chart/prerequisites/helm/helm.go

View workflow job for this annotation

GitHub Actions / Unit tests on darwin-arm64 

no required module provides package github.com/flamingo-stack/openframe-cli/internal/ui; to add it:

Check failure on line 13 in internal/chart/prerequisites/helm/helm.go

View workflow job for this annotation

GitHub Actions / Unit tests on darwin-arm64 

no required module provides package github.com/flamingo-stack/openframe-cli/internal/ui; to add it:

Check failure on line 13 in internal/chart/prerequisites/helm/helm.go

View workflow job for this annotation

GitHub Actions / Unit tests on darwin-arm64 

no required module provides package github.com/flamingo-stack/openframe-cli/internal/ui; to add it:

Check failure on line 13 in internal/chart/prerequisites/helm/helm.go

View workflow job for this annotation

GitHub Actions / Unit tests on darwin-arm64 

no required module provides package github.com/flamingo-stack/openframe-cli/internal/ui; to add it:

Check failure on line 13 in internal/chart/prerequisites/helm/helm.go

View workflow job for this annotation

GitHub Actions / Unit tests on darwin-arm64 

no required module provides package github.com/flamingo-stack/openframe-cli/internal/ui; to add it:

Check failure on line 13 in internal/chart/prerequisites/helm/helm.go

View workflow job for this annotation

GitHub Actions / Unit tests on darwin-arm64 

no required module provides package github.com/flamingo-stack/openframe-cli/internal/ui; to add it:

Check failure on line 13 in internal/chart/prerequisites/helm/helm.go

View workflow job for this annotation

GitHub Actions / Unit tests on darwin-arm64 

no required module provides package github.com/flamingo-stack/openframe-cli/internal/ui; to add it:

Check failure on line 13 in internal/chart/prerequisites/helm/helm.go

View workflow job for this annotation

GitHub Actions / Unit tests on darwin-arm64 

no required module provides package github.com/flamingo-stack/openframe-cli/internal/ui; to add it:
)

type HelmInstaller struct{}
Expand Down Expand Up @@ -95,12 +96,12 @@
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
defer cancel()

fmt.Printf("Downloading verified helm %s...\n", download.Helm.Version)

@flamingo flamingo Bot Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

ui.Info(fmt.Sprintf("Downloading verified helm %s...", download.Helm.Version))
path, err := (download.Downloader{}).InstallPinnedTool(ctx, download.Helm, binDir)
if err != nil {
return fmt.Errorf("verified helm install failed: %w", err)
}
download.PrependToPath(binDir)
fmt.Printf("Installed verified helm %s to %s\n", download.Helm.Version, path)
ui.Info(fmt.Sprintf("Installed verified helm %s to %s", download.Helm.Version, path))
return nil
}
26 changes: 25 additions & 1 deletion internal/cluster/discovery/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,20 @@
cmd.Stdin = os.Stdin
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
return cmd.Run()
if err := cmd.Run(); err != nil {
exitCode := -1
var exitErr *osexec.ExitError
if errorsAsExitError(err, &exitErr) {
exitCode = exitErr.ExitCode()
}
return &executor.CommandError{
Command: "gcloud",
Args: args,

Check failure on line 55 in internal/cluster/discovery/auth.go

View workflow job for this annotation

GitHub Actions / Unit tests on linux-amd64 

unknown field Args in struct literal of type executor.CommandError

Check failure on line 55 in internal/cluster/discovery/auth.go

View workflow job for this annotation

GitHub Actions / Unit tests on windows-amd64 

unknown field Args in struct literal of type executor.CommandError

Check failure on line 55 in internal/cluster/discovery/auth.go

View workflow job for this annotation

GitHub Actions / Unit tests on darwin-arm64 

unknown field Args in struct literal of type executor.CommandError
ExitCode: exitCode,
Err: err,
}
}
return nil
},
}
}
Expand Down Expand Up @@ -125,6 +138,10 @@
return fmt.Errorf("%s", manualHint)
}
if err := f.runLogin(ctx, args...); err != nil {

@flamingo flamingo Bot Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

var cmdErr *executor.CommandError
if errorsAsExitError(err, &cmdErr) {
return err
}
return fmt.Errorf("gcloud %s failed: %w", strings.Join(args, " "), err)
}
if !verified() {
Expand All @@ -133,3 +150,10 @@
pterm.Success.Println("Google Cloud authentication complete")
return nil
}

// errorsAsExitError is a small wrapper around errors.As kept local so this
// file only needs the standard "errors" package's As semantics without an
// extra top-level import line churn beyond what's already here.
func errorsAsExitError(err error, target interface{}) bool {
return errorsAs(err, target)
}
10 changes: 9 additions & 1 deletion internal/cluster/prerequisites/gcloud/gcloud.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package gcloud

import (
"context"
"errors"
"fmt"
"os/exec"
"runtime"
Expand All @@ -18,7 +19,14 @@ var (
lookPath = exec.LookPath
runQuiet = func(name string, args ...string) error {
cmd := exec.Command(name, args...) // #nosec G204 -- explicit argv, no shell; command and args are internal, not untrusted input
return cmd.Run()
if err := cmd.Run(); err != nil {
var exitErr *exec.ExitError
if errors.As(err, &exitErr) {
return fmt.Errorf("command %q exited with code %d: %w", name, exitErr.ExitCode(), err)
}
return err
}
return nil
}
)

Comment on lines 19 to 32

@flamingo flamingo Bot Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

Expand Down
61 changes: 47 additions & 14 deletions internal/cluster/providers/k3d/verify.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
"strings"
"time"

"github.com/pterm/pterm"

sharedconfig "github.com/flamingo-stack/openframe-cli/internal/shared/config"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes"
Expand Down Expand Up @@ -41,12 +43,12 @@

// Switch the current context
config.CurrentContext = contextName

@flamingo flamingo Bot Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

if err := clientcmd.WriteToFile(*config, kubeconfigPath); err != nil {
if err := writeKubeconfigAtomically(*config, kubeconfigPath); err != nil {
return nil, fmt.Errorf("failed to switch and write kubectl context: %w", err)
}

if m.verbose {

@flamingo flamingo Bot Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

fmt.Printf("✓ Switched kubectl context to %s\n", contextName)
pterm.Info.Printfln("Switched kubectl context to %s", contextName)
}

// Build rest.Config from the loaded Kubeconfig
Expand All @@ -66,7 +68,7 @@
restConfig = sharedconfig.ApplyInsecureTLSConfig(restConfig)

if m.verbose {

@flamingo flamingo Bot Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

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

// --- PHASE 2: Verify Network Connectivity and Update Endpoint ---
Expand All @@ -75,7 +77,7 @@
host, port, err := extractHostPort(restConfig.Host)
if err != nil {
if m.verbose {
fmt.Printf("Warning: Could not extract host:port from %s: %v\n", restConfig.Host, err)
pterm.Warning.Printfln("Could not extract host:port from %s: %v", restConfig.Host, err)
}
// Default to 127.0.0.1:6550 for k3d
host = "127.0.0.1"
Expand Down Expand Up @@ -105,7 +107,7 @@
var lastErr error

if m.verbose {
fmt.Println("Waiting for cluster API and nodes to be reachable...")
pterm.Info.Println("Waiting for cluster API and nodes to be reachable...")
}

for i := 0; i < maxRetries; i++ {
Expand All @@ -123,7 +125,7 @@
if isTemporaryError(err) {

@flamingo flamingo Bot Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

lastErr = err
if m.verbose {
fmt.Printf(" Cluster not ready yet (attempt %d/%d): %v\n", i+1, maxRetries, err)
pterm.Info.Printfln("Cluster not ready yet (attempt %d/%d): %v", i+1, maxRetries, err)
}
time.Sleep(retryDelay)
continue
Expand All @@ -136,7 +138,7 @@
if len(nodes.Items) == 0 {
lastErr = fmt.Errorf("no nodes found in cluster")
if m.verbose {
fmt.Printf(" No nodes found yet (attempt %d/%d), waiting...\n", i+1, maxRetries)
pterm.Info.Printfln("No nodes found yet (attempt %d/%d), waiting...", i+1, maxRetries)
}
time.Sleep(retryDelay)
continue
Expand All @@ -157,22 +159,53 @@
// Success condition: Nodes exist and at least one is ready
if readyCount > 0 {
if m.verbose {
fmt.Printf(" Found %d ready node(s) out of %d total\n", readyCount, len(nodes.Items))
fmt.Println("✓ Cluster API and nodes are ready.")
pterm.Info.Printfln("Found %d ready node(s) out of %d total", readyCount, len(nodes.Items))
pterm.Success.Println("Cluster API and nodes are ready.")
}
return restConfig, nil
}

lastErr = fmt.Errorf("no nodes in Ready state (found %d nodes, 0 ready)", len(nodes.Items))
if m.verbose {
fmt.Printf(" Nodes exist but none are Ready yet (attempt %d/%d), waiting...\n", i+1, maxRetries)
pterm.Info.Printfln("Nodes exist but none are Ready yet (attempt %d/%d), waiting...", i+1, maxRetries)
}
time.Sleep(retryDelay)
}

return nil, fmt.Errorf("cluster not reachable after %d retries (last error: %w)", maxRetries, lastErr)
}

// writeKubeconfigAtomically writes the kubeconfig to a temp file in the same
// directory and renames it into place, avoiding a read-modify-write race with
// other concurrently running CLI invocations that may also touch kubeconfigPath.
func writeKubeconfigAtomically(config clientcmd.Config, kubeconfigPath string) error {

Check failure on line 181 in internal/cluster/providers/k3d/verify.go

View workflow job for this annotation

GitHub Actions / Unit tests on linux-amd64 

undefined: clientcmd.Config

Check failure on line 181 in internal/cluster/providers/k3d/verify.go

View workflow job for this annotation

GitHub Actions / Lint 

undefined: clientcmd.Config (typecheck)

Check failure on line 181 in internal/cluster/providers/k3d/verify.go

View workflow job for this annotation

GitHub Actions / Unit tests on windows-amd64 

undefined: clientcmd.Config

Check failure on line 181 in internal/cluster/providers/k3d/verify.go

View workflow job for this annotation

GitHub Actions / Unit tests on darwin-arm64 

undefined: clientcmd.Config
dir := filepath.Dir(kubeconfigPath)
tmpFile, err := os.CreateTemp(dir, ".kubeconfig-*.tmp")
if err != nil {
return fmt.Errorf("failed to create temp kubeconfig file: %w", err)
}
tmpPath := tmpFile.Name()
_ = tmpFile.Close()

defer func() {
_ = os.Remove(tmpPath)
}()

if err := clientcmd.WriteToFile(config, tmpPath); err != nil {
return fmt.Errorf("failed to write temp kubeconfig file: %w", err)
}

if info, statErr := os.Stat(kubeconfigPath); statErr == nil {
_ = os.Chmod(tmpPath, info.Mode())
}

if err := os.Rename(tmpPath, kubeconfigPath); err != nil {
return fmt.Errorf("failed to atomically replace kubeconfig file: %w", err)
}

return nil
}

// isTemporaryError checks if an error is temporary and should be retried
func isTemporaryError(err error) bool {
if err == nil {
Expand All @@ -194,7 +227,7 @@
address := net.JoinHostPort(host, port)

if m.verbose {
fmt.Printf("Waiting for TCP port %s to be available...\n", address)
pterm.Info.Printfln("Waiting for TCP port %s to be available...", address)
}

var lastErr error
Expand All @@ -212,14 +245,14 @@
if err == nil {
_ = conn.Close()
if m.verbose {
fmt.Printf("✓ TCP port %s is open\n", address)
pterm.Success.Printfln("TCP port %s is open", address)
}
return nil
}

lastErr = err
if m.verbose {
fmt.Printf(" TCP port not ready yet (attempt %d/%d): %v\n", i+1, maxRetries, err)
pterm.Info.Printfln("TCP port not ready yet (attempt %d/%d): %v", i+1, maxRetries, err)
}
time.Sleep(retryDelay)
}
Expand Down Expand Up @@ -301,7 +334,7 @@
}

if m.verbose {
fmt.Println("✓ Cleaned up stale kubeconfig lock files")
pterm.Success.Println("Cleaned up stale kubeconfig lock files")
}

return nil
Expand Down
5 changes: 3 additions & 2 deletions internal/shared/wsllauncher/install.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"strings"
"time"

"github.com/flamingo-stack/openframe-cli/internal/shared/executor"
"github.com/flamingo-stack/openframe-cli/internal/shared/selfupdate"
"github.com/flamingo-stack/openframe-cli/internal/shared/ui/spinner"
)
Expand Down Expand Up @@ -114,7 +115,7 @@ func localInstallScript(windowsPath string) string {
func installLocalBinaryInWSL(windowsPath string) error {
cmd := exec.Command("wsl", wslArgv("bash", "-lc", localInstallScript(windowsPath))...) // #nosec G204 -- path is single-quoted into a self-contained script

@flamingo flamingo Bot Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

if out, err := cmd.CombinedOutput(); err != nil {
return fmt.Errorf("installing local openframe binary into WSL failed: %w\n%s", err, string(out))
return executor.NewCommandError(cmd, out, err, fmt.Sprintf("installing local openframe binary into WSL failed: %v", err))
}
return nil
}
Expand Down Expand Up @@ -146,7 +147,7 @@ func installOpenframeInWSL(version, goarch string) error {
cmd.Stdin = bytes.NewReader(binary)
if out, err := cmd.CombinedOutput(); err != nil {
sp.Fail("Installing openframe inside WSL failed")
return fmt.Errorf("installing openframe inside WSL failed: %w\n%s", err, string(out))
return executor.NewCommandError(cmd, out, err, fmt.Sprintf("installing openframe inside WSL failed: %v", err))
}
sp.Success("OpenFrame is installed inside WSL")
return nil
Expand Down
Loading