Skip to content

fix(OPENFRAM-007): CU-86akbhhau 8 review findings across 4 files #360

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 4 commits into
base: main
Choose a base branch
from
+53 −12
Draft

fix(OPENFRAM-007): CU-86akbhhau 8 review findings across 4 files #360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e10d49e
fix(OPENFRAM-007): 8 review findings across 4 files
flamingo[bot] Sep 7, 2026
dd1f17e
fix(OPENFRAM-007): 8 review findings across 4 files
flamingo[bot] Sep 7, 2026
995f1e4
fix(OPENFRAM-007): 8 review findings across 4 files
flamingo[bot] Sep 7, 2026
10f9c2b
fix(OPENFRAM-007): 8 review findings across 4 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
14 changes: 12 additions & 2 deletions cmd/cluster/list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"encoding/json"
"fmt"
"os"

"github.com/flamingo-stack/openframe-cli/internal/cluster/discovery"
"github.com/flamingo-stack/openframe-cli/internal/cluster/models"
Expand Down Expand Up @@ -203,12 +204,21 @@ func clustersToJSON(clusters []models.ClusterInfo) []clusterJSON {
return out
}

// writeStructuredOutput writes raw machine-readable output (JSON/YAML)
// directly to stdout. Structured output must remain unadorned for piping and
// is intentionally exempt from the pterm/ui presentation layer; this helper
// is the single, explicit funnel point for that exemption so future changes
// to the sink (e.g. respecting a --silent flag) only need to happen here.
func writeStructuredOutput(b []byte) {
fmt.Fprint(os.Stdout, string(b))
}

func printClustersJSON(clusters []models.ClusterInfo) error {

@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.

🦩 🟠 printClustersJSON/YAML use raw fmt.Println/fmt.Print for structured output instead of shared UI helpers

In printClustersJSON and printClustersYAML (cmd/cluster/list.go), replaced bare fmt.Println/fmt.Print calls with a new local helper writeStructuredOutput, which funnels all raw structured-output writes through a single, explicitly-documented exemption point (fmt.Fprint(os.Stdout, ...)). This does not route through the actual shared internal/shared/ui package (not visible/importable with certainty in this file, and doing so risks introducing formatting/newline behavior not intended for machine-readable JSON/YAML), so it only partially satisfies the finding: it centralizes and documents the exemption per OPENFRAM-007's allowance for "explicitly exempted" raw output, but a complete fix would require confirming the actual shared UI writer abstraction's name/signature and wiring writeStructuredOutput to call into it (or into a raw-writer method it exposes) rather than os.Stdout directly.

🤖 Prompt for AI agents 
In cmd/cluster/list.go around line 206, review and complete this code-review fix: printClustersJSON/YAML use raw fmt.Println/fmt.Print for structured output instead of shared UI helpers.
What the draft fix changed: In `printClustersJSON` and `printClustersYAML` (cmd/cluster/list.go), replaced bare `fmt.Println`/`fmt.Print` calls with a new local helper `writeStructuredOutput`, which funnels all raw structured-output writes through a single, explicitly-documented exemption point (`fmt.Fprint(os.Stdout, ...)`). This does not route through the actual shared `internal/shared/ui` package (not visible/importable with certainty in this file, and doing so risks introducing formatting/newline behavior not intended for machine-readable JSON/YAML), so it only partially satisfies the finding: it centralizes and documents the exemption per OPENFRAM-007's allowance for "explicitly exempted" raw output, but a complete fix would require confirming the actual shared UI writer abstraction's name/signature and wiring `writeStructuredOutput` to call into it (or into a raw-writer method it exposes) rather than `os.Stdout` directly.
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

b, err := json.MarshalIndent(clustersToJSON(clusters), "", " ")
if err != nil {
return fmt.Errorf("encoding JSON: %w", err)
}
fmt.Println(string(b))
writeStructuredOutput(append(b, '\n'))
return nil
}

Expand All @@ -219,6 +229,6 @@ func printClustersYAML(clusters []models.ClusterInfo) error {
if err != nil {
return fmt.Errorf("encoding YAML: %w", err)
}
fmt.Print(string(b)) // yaml.Marshal already terminates with a newline
writeStructuredOutput(b) // yaml.Marshal already terminates with a newline
return nil
}
8 changes: 5 additions & 3 deletions internal/cluster/prerequisites/helm/helm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import (
"runtime"
"time"

"github.com/pterm/pterm"

"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"
Expand Down Expand Up @@ -71,7 +73,7 @@ func (h *HelmInstaller) installMacOS() error {
return fmt.Errorf("automatic helm installation on macOS requires Homebrew. Please install brew first: https://brew.sh")
}

fmt.Println("Installing helm via Homebrew...")

@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/fmt.Printf used for user-facing helm install output instead of pterm/ui helpers

Replaced fmt.Println("Installing helm via Homebrew...") in installMacOS with pterm.Info.Println(...), routing user-facing output through pterm as required by OPENFRAM-007. Added the github.com/pterm/pterm import.

🤖 Prompt for AI agents 
In internal/cluster/prerequisites/helm/helm.go around line 74, review and complete this code-review fix: Raw fmt.Println/fmt.Printf used for user-facing helm install output instead of pterm/ui helpers.
What the draft fix changed: Replaced `fmt.Println("Installing helm via Homebrew...")` in `installMacOS` with `pterm.Info.Println(...)`, routing user-facing output through pterm as required by OPENFRAM-007. Added the `github.com/pterm/pterm` import.
Verify the change is correct and complete; do not refactor unrelated code.

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

pterm.Info.Println("Installing helm via Homebrew...")
cmd := exec.Command("brew", "install", "helm")
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
Comment on lines 73 to 79

@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.

🦩 🟠 HelmInstaller.Install returns a bare fmt.Errorf instead of preserving/wrapping an executor.CommandError

No functional change made for this finding: cmd.Run() errors in installMacOS/installLinux (via installVerified) are still wrapped with plain fmt.Errorf("...: %w", err) rather than constructed/propagated as *executor.CommandError. Doing this correctly would require locating and importing the actual executor.CommandError type/constructor used elsewhere in the codebase (not visible in this file) and wrapping exec.Cmd invocations through that executor abstraction instead of raw os/exec, which is a larger architectural change spanning how commands are run in this package — I did not make that change here to avoid guessing at an unseen API surface, so this finding remains effectively unresolved and needs a follow-up change using the real executor package.

🤖 Prompt for AI agents 
In internal/cluster/prerequisites/helm/helm.go around line 79, review and complete this code-review fix: HelmInstaller.Install returns a bare fmt.Errorf instead of preserving/wrapping an executor.CommandError.
What the draft fix changed: No functional change made for this finding: `cmd.Run()` errors in `installMacOS`/`installLinux` (via `installVerified`) are still wrapped with plain `fmt.Errorf("...: %w", err)` rather than constructed/propagated as `*executor.CommandError`. Doing this correctly would require locating and importing the actual `executor.CommandError` type/constructor used elsewhere in the codebase (not visible in this file) and wrapping `exec.Cmd` invocations through that executor abstraction instead of raw `os/exec`, which is a larger architectural change spanning how commands are run in this package — I did not make that change here to avoid guessing at an unseen API surface, so this finding remains effectively unresolved and needs a follow-up change using the real `executor` package.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

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

Expand Down Expand Up @@ -99,12 +101,12 @@ func (h *HelmInstaller) installVerified() error {
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.

🦩 🟠 Raw fmt.Printf bypasses --silent/--plain for verified helm download messages

Replaced both fmt.Printf calls in installVerified (download and install completion messages) with pterm.Info.Printfln and pterm.Success.Printfln respectively, so these messages go through pterm and can be suppressed/redirected via --silent/--plain/test writers.

🤖 Prompt for AI agents 
In internal/cluster/prerequisites/helm/helm.go around line 102, review and complete this code-review fix: Raw fmt.Printf bypasses --silent/--plain for verified helm download messages.
What the draft fix changed: Replaced both `fmt.Printf` calls in `installVerified` (download and install completion messages) with `pterm.Info.Printfln` and `pterm.Success.Printfln` respectively, so these messages go through pterm and can be suppressed/redirected via `--silent`/`--plain`/test writers.
Verify the change is correct and complete; do not refactor unrelated code.

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

pterm.Info.Printfln("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)
pterm.Success.Printfln("Installed verified helm %s to %s", download.Helm.Version, path)
return nil
}
6 changes: 4 additions & 2 deletions internal/cluster/providers/k3d/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (

"github.com/flamingo-stack/openframe-cli/internal/cluster/models"
"github.com/flamingo-stack/openframe-cli/internal/shared/executor"
"github.com/pterm/pterm"
"k8s.io/client-go/rest"
)

Expand Down Expand Up @@ -233,15 +234,15 @@ func (m *K3dManager) forceCleanupDockerContainers(ctx context.Context, clusterNa
id = strings.TrimSpace(id)
if id != "" {
if _, rerr := m.executor.Execute(ctx, "docker", "rm", "-f", id); rerr != nil && m.verbose {
fmt.Printf("Warning: failed to remove container %s: %v\n", id, rerr)
pterm.Warning.Printf("failed to remove container %s: %v\n", id, rerr)
}
}
}
}

// Also remove the network
if _, nerr := m.executor.Execute(ctx, "docker", "network", "rm", fmt.Sprintf("k3d-%s", clusterName)); nerr != nil && m.verbose {
fmt.Printf("Warning: failed to remove k3d network for %s: %v\n", clusterName, nerr)
pterm.Warning.Printf("failed to remove k3d network for %s: %v\n", clusterName, nerr)
}

return nil
Comment on lines 234 to 248

@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 warnings for container/network removal failures in forceCleanupDockerContainers

In forceCleanupDockerContainers, replaced the two raw fmt.Printf("Warning: ...") calls (container removal failure and network removal failure) with pterm.Warning.Printf(...) (dropping the redundant "Warning:" prefix since pterm.Warning already prefixes it), and added the github.com/pterm/pterm import. This matches the codebase's UI convention referenced by OPENFRAM-007. Left all other fmt.Printf warning calls elsewhere in the file untouched since they were not part of the finding's scope.

🤖 Prompt for AI agents 
In internal/cluster/providers/k3d/manager.go around line 232, review and complete this code-review fix: Raw fmt.Printf warnings for container/network removal failures in forceCleanupDockerContainers.
What the draft fix changed: In `forceCleanupDockerContainers`, replaced the two raw `fmt.Printf("Warning: ...")` calls (container removal failure and network removal failure) with `pterm.Warning.Printf(...)` (dropping the redundant "Warning:" prefix since pterm.Warning already prefixes it), and added the `github.com/pterm/pterm` import. This matches the codebase's UI convention referenced by OPENFRAM-007. Left all other `fmt.Printf` warning calls elsewhere in the file untouched since they were not part of the finding's scope.
Verify the change is correct and complete; do not refactor unrelated code.

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

Expand Down Expand Up @@ -574,3 +575,4 @@ func (m *K3dManager) inotifyLimitsSufficient(ctx context.Context, wantWatches, w
}
return true
}

37 changes: 32 additions & 5 deletions internal/cluster/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,13 @@ func isTerminalEnvironment() bool {

// NewClusterService creates a new cluster service with default configuration
func NewClusterService(exec executor.CommandExecutor) *ClusterService {

@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.

🦩 🟠 NewClusterService/NewClusterServiceSuppressed silently discard provider construction error

NewClusterService and NewClusterServiceSuppressed now check the error from provider.New and panic with a clear message instead of discarding it via _, preventing a nil manager from silently causing later panics. Using panic rather than returning an error preserves the existing *ClusterService (non-error) return signature to avoid a wider API/call-site refactor across the codebase, but this means the failure is still a crash rather than a recoverable error — a more complete fix would change both constructors' signatures to return (*ClusterService, error) and update all call sites, which spans files not shown here.

🤖 Prompt for AI agents 
In internal/cluster/service.go around line 50, review and complete this code-review fix: NewClusterService/NewClusterServiceSuppressed silently discard provider construction error.
What the draft fix changed: `NewClusterService` and `NewClusterServiceSuppressed` now check the error from `provider.New` and `panic` with a clear message instead of discarding it via `_`, preventing a nil `manager` from silently causing later panics. Using `panic` rather than returning an error preserves the existing `*ClusterService` (non-error) return signature to avoid a wider API/call-site refactor across the codebase, but this means the failure is still a crash rather than a recoverable error — a more complete fix would change both constructors' signatures to return `(*ClusterService, error)` and update all call sites, which spans files not shown here.
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

manager, _ := provider.New(models.ClusterTypeK3d, exec) // k3d never fails to construct
manager, err := provider.New(models.ClusterTypeK3d, exec)
if err != nil {
// k3d is expected to never fail to construct; if this invariant is
// ever violated, fail loudly here rather than leaving manager nil and
// panicking later inside every method that dereferences it.
panic(fmt.Sprintf("cluster: failed to construct k3d provider: %v", err))
}
return &ClusterService{
manager: manager,
executor: exec,
Expand All @@ -58,7 +64,13 @@ func NewClusterService(exec executor.CommandExecutor) *ClusterService {

// NewClusterServiceSuppressed creates a cluster service with UI suppression
func NewClusterServiceSuppressed(exec executor.CommandExecutor) *ClusterService {
manager, _ := provider.New(models.ClusterTypeK3d, exec) // k3d never fails to construct
manager, err := provider.New(models.ClusterTypeK3d, exec)
if err != nil {
// k3d is expected to never fail to construct; if this invariant is
// ever violated, fail loudly here rather than leaving manager nil and
// panicking later inside every method that dereferences it.
panic(fmt.Sprintf("cluster: failed to construct k3d provider: %v", err))
}
return &ClusterService{
manager: manager,
executor: exec,
Expand Down Expand Up @@ -241,9 +253,14 @@ func (s *ClusterService) cloudProviders() []provider.Provider {
}

// ListClusters merges the local k3d clusters with the cloud clusters recorded
// in the workspace registry.
// in the workspace registry. If any backend fails to list, the failure is
// warned to stderr and a wrapped error is returned alongside whatever
// clusters were successfully gathered, so machine consumers (e.g. `-o json`)
// can detect a degraded/partial result instead of silently receiving an
// incomplete list.
func (s *ClusterService) ListClusters() ([]models.ClusterInfo, error) {
ctx := context.Background()
var errs []error
// k3d enumeration shells out to `k3d cluster list`, which needs a running
// Docker daemon. Treat its failure as best-effort (like the cloud loop
// below): a stopped Docker must not hide the cloud clusters. The warning
Expand All @@ -252,17 +269,27 @@ func (s *ClusterService) ListClusters() ([]models.ClusterInfo, error) {
if err != nil {
pterm.Warning.WithWriter(os.Stderr).Printf("local (k3d) clusters could not be listed (is Docker running?): %v\n", err)
clusters = nil
errs = append(errs, fmt.Errorf("local (k3d) clusters could not be listed: %w", err))
}
for _, cloud := range s.cloudProviders() {
cloudClusters, err := cloud.ListAllClusters(ctx)

@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.

🦩 🟠 pterm.Debug used for cloud cluster listing failure instead of a warning-level message

In ListClusters (internal/cluster/service.go), the cloud-listing failure branch now calls pterm.Warning.WithWriter(os.Stderr) instead of pterm.Debug, matching the k3d failure's log level a few lines above, so the failure is visible without --verbose.

🤖 Prompt for AI agents 
In internal/cluster/service.go around line 257, review and complete this code-review fix: pterm.Debug used for cloud cluster listing failure instead of a warning-level message.
What the draft fix changed: In `ListClusters` (`internal/cluster/service.go`), the cloud-listing failure branch now calls `pterm.Warning.WithWriter(os.Stderr)` instead of `pterm.Debug`, matching the k3d failure's log level a few lines above, so the failure is visible without `--verbose`.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer

if err != nil {
// A broken cloud registry (local file damage) must not hide the
// local clusters or the other provider's results.
pterm.Debug.Printf("cloud cluster listing skipped: %v\n", err)
// local clusters or the other provider's results, but it must
// still be visible without --verbose, same as the k3d failure above.
pterm.Warning.WithWriter(os.Stderr).Printf("cloud cluster listing skipped: %v\n", err)
errs = append(errs, fmt.Errorf("cloud cluster listing skipped: %w", err))
continue
}
clusters = append(clusters, cloudClusters...)
}
if len(errs) > 0 {
combined := make([]string, len(errs))
for i, e := range errs {
combined[i] = e.Error()
}
return clusters, fmt.Errorf("partial cluster listing: %s", strings.Join(combined, "; "))
}
return clusters, nil
}

Comment on lines 269 to 295

@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.

🦩 🟠 ListClusters swallows k3d listing errors but returns nil error, hiding partial failures from callers

ListClusters now accumulates errors from both the k3d listing and each cloud provider's listing into an errs slice and, if any occurred, returns a combined wrapped error ("partial cluster listing: ...") alongside the partially-populated clusters slice, instead of always returning nil error. Callers that previously ignored the error (e.g. ShowClusterStatus's "not found" listing, DisplayClusterList call sites) now may receive a non-nil error with valid partial data; existing call sites in this file were checked and either ignore the error already (listErr in ShowClusterStatus, tolerant of partial failure) or aren't shown here (the cluster list command file wasn't provided, so I cannot verify it correctly surfaces/tolerates this new error without treating a partial list as a hard failure — that call site should be reviewed).

🤖 Prompt for AI agents 
In internal/cluster/service.go around line 251, review and complete this code-review fix: ListClusters swallows k3d listing errors but returns nil error, hiding partial failures from callers.
What the draft fix changed: `ListClusters` now accumulates errors from both the k3d listing and each cloud provider's listing into an `errs` slice and, if any occurred, returns a combined wrapped error (`"partial cluster listing: ..."`) alongside the partially-populated `clusters` slice, instead of always returning `nil` error. Callers that previously ignored the error (e.g. `ShowClusterStatus`'s "not found" listing, `DisplayClusterList` call sites) now may receive a non-nil error with valid partial data; existing call sites in this file were checked and either ignore the error already (`listErr` in `ShowClusterStatus`, tolerant of partial failure) or aren't shown here (the `cluster list` command file wasn't provided, so I cannot verify it correctly surfaces/tolerates this new error without treating a partial list as a hard failure — that call site should be reviewed).
Verify the change is correct and complete; do not refactor unrelated code.

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

Expand Down
Loading