Skip to content

fix(FLEETMDM-002-2): CU-86aknprf9 11 review findings across 8 files #413

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 8 commits into
base: main
Choose a base branch
from
Draft

fix(FLEETMDM-002-2): CU-86aknprf9 11 review findings across 8 files #413

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
56a19ca
fix(FLEETMDM-002-2): 11 review findings across 8 files
flamingo[bot] Oct 12, 2026
25a7b09
fix(FLEETMDM-002-2): 11 review findings across 8 files
flamingo[bot] Oct 12, 2026
22aa207
fix(FLEETMDM-002-2): 11 review findings across 8 files
flamingo[bot] Oct 12, 2026
baee5dd
fix(FLEETMDM-002-2): 11 review findings across 8 files
flamingo[bot] Oct 12, 2026
94b8e77
fix(FLEETMDM-002-2): 11 review findings across 8 files
flamingo[bot] Oct 12, 2026
4fe0bb3
fix(FLEETMDM-002-2): 11 review findings across 8 files
flamingo[bot] Oct 12, 2026
558000e
fix(FLEETMDM-002-2): 11 review findings across 8 files
flamingo[bot] Oct 12, 2026
80b7531
fix(FLEETMDM-002-2): 11 review findings across 8 files
flamingo[bot] Oct 12, 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
7 changes: 4 additions & 3 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 @@ import (
"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/pterm/pterm"
)

type HelmInstaller struct{}
Expand Down Expand Up @@ -89,18 +90,18 @@ func (h *HelmInstaller) installLinux() error {
func (h *HelmInstaller) installVerified() error {
binDir, err := download.UserBinDir()

@flamingo flamingo Bot Oct 12, 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.

🦩 🔵 UserBinDir error returned bare without context wrapping

In installVerified (internal/chart/prerequisites/helm/helm.go), wrapped the bare return err from download.UserBinDir() with fmt.Errorf("resolving user bin dir: %w", err), matching the suggested fix exactly.

🤖 Prompt for AI agents 
In internal/chart/prerequisites/helm/helm.go around line 90, review and complete this code-review fix: UserBinDir error returned bare without context wrapping.
What the draft fix changed: In installVerified (internal/chart/prerequisites/helm/helm.go), wrapped the bare `return err` from download.UserBinDir() with `fmt.Errorf("resolving user bin dir: %w", err)`, matching the suggested fix exactly.
Verify the change is correct and complete; do not refactor unrelated code.

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

if err != nil {
return err
return fmt.Errorf("resolving user bin dir: %w", err)
}

ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
defer cancel()

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

@flamingo flamingo Bot Oct 12, 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 used for user-facing progress output in installVerified (helm)

In installVerified, replaced fmt.Printf calls for the "Downloading verified helm..." and "Installed verified helm..." messages with pterm.Info.Printf and pterm.Success.Printf respectively, and added the github.com/pterm/pterm import (already used elsewhere in the project per the k3d installer reference), matching the suggested fix.

🤖 Prompt for AI agents 
In internal/chart/prerequisites/helm/helm.go around line 98, review and complete this code-review fix: Raw fmt.Printf used for user-facing progress output in installVerified (helm).
What the draft fix changed: In installVerified, replaced `fmt.Printf` calls for the "Downloading verified helm..." and "Installed verified helm..." messages with `pterm.Info.Printf` and `pterm.Success.Printf` respectively, and added the `github.com/pterm/pterm` import (already used elsewhere in the project per the k3d installer reference), matching the suggested fix.
Verify the change is correct and complete; do not refactor unrelated code.

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

pterm.Info.Printf("Downloading verified helm %s...\n", 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.Printf("Installed verified helm %s to %s\n", download.Helm.Version, path)
return nil
}
6 changes: 3 additions & 3 deletions internal/chart/providers/argocd/sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func syncOperationPatch(prune bool) string {
func (m *Manager) RefreshAndSync(ctx context.Context, prune bool) error {
if m.dynamicClient == nil {

@flamingo flamingo Bot Oct 12, 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.

🦩 🟠 initKubernetesClients error returned bare without wrapping in RefreshAndSync

In RefreshAndSync, the m.initKubernetesClients() error path now wraps with fmt.Errorf("initializing kubernetes clients: %w", err) instead of returning err bare, matching the suggested fix exactly.

🤖 Prompt for AI agents 
In internal/chart/providers/argocd/sync.go around line 55, review and complete this code-review fix: initKubernetesClients error returned bare without wrapping in RefreshAndSync.
What the draft fix changed: In `RefreshAndSync`, the `m.initKubernetesClients()` error path now wraps with `fmt.Errorf("initializing kubernetes clients: %w", err)` instead of returning `err` bare, matching the suggested fix exactly.
Verify the change is correct and complete; do not refactor unrelated code.

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

if err := m.initKubernetesClients(); err != nil {
return err
return fmt.Errorf("initializing kubernetes clients: %w", err)
}
}
if m.dynamicClient == nil {
Expand Down Expand Up @@ -85,7 +85,7 @@ func (m *Manager) RefreshAndSync(ctx context.Context, prune bool) error {
// 2) Don't clobber an operation already in flight — setting .operation over a
// running one is racy; wait briefly, then refuse rather than stomp it.
if err := m.ensureNoRunningOperation(ctx, budget); err != nil {

@flamingo flamingo Bot Oct 12, 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.

🦩 🔵 ensureNoRunningOperation error returned bare without wrapping

In RefreshAndSync, the m.ensureNoRunningOperation(ctx, budget) error path now wraps with fmt.Errorf("waiting for in-flight operation to clear: %w", err) instead of returning err bare, matching the suggested fix exactly.

🤖 Prompt for AI agents 
In internal/chart/providers/argocd/sync.go around line 87, review and complete this code-review fix: ensureNoRunningOperation error returned bare without wrapping.
What the draft fix changed: In `RefreshAndSync`, the `m.ensureNoRunningOperation(ctx, budget)` error path now wraps with `fmt.Errorf("waiting for in-flight operation to clear: %w", err)` instead of returning `err` bare, matching the suggested fix exactly.
Verify the change is correct and complete; do not refactor unrelated code.

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

return err
return fmt.Errorf("waiting for in-flight operation to clear: %w", err)
}

// 3) Trigger the sync via the top-level .operation field.
Expand Down Expand Up @@ -327,7 +327,7 @@ func (m *Manager) SyncApplications(ctx context.Context, names []string) error {
func (m *Manager) syncApplicationsByName(ctx context.Context, names []string, prune bool) (patched, failed int, firstErr error) {
if m.dynamicClient == nil {

@flamingo flamingo Bot Oct 12, 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.

🦩 🔵 syncApplicationsByName propagates initKubernetesClients error unwrapped

In syncApplicationsByName, the m.initKubernetesClients() error path now wraps with fmt.Errorf("initializing kubernetes clients: %w", err) instead of returning err bare, matching the suggested fix exactly.

🤖 Prompt for AI agents 
In internal/chart/providers/argocd/sync.go around line 328, review and complete this code-review fix: syncApplicationsByName propagates initKubernetesClients error unwrapped.
What the draft fix changed: In `syncApplicationsByName`, the `m.initKubernetesClients()` error path now wraps with `fmt.Errorf("initializing kubernetes clients: %w", err)` instead of returning `err` bare, matching the suggested fix exactly.
Verify the change is correct and complete; do not refactor unrelated code.

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

if err := m.initKubernetesClients(); err != nil {
return 0, len(names), err
return 0, len(names), fmt.Errorf("initializing kubernetes clients: %w", err)
}
}
apps := m.dynamicClient.Resource(applicationGVR).Namespace(ArgoCDNamespace)
Expand Down
3 changes: 2 additions & 1 deletion internal/chart/providers/helm/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ func (h *HelmManager) IsChartInstalled(ctx context.Context, releaseName, namespa
Env: h.getHelmEnv(),
})
if err != nil {
return false, err
return false, fmt.Errorf("checking install status of release %s in namespace %s: %w", releaseName, namespace, err)
}

releases := strings.Split(strings.TrimSpace(result.Stdout), "\n")
Comment on lines 154 to 160

@flamingo flamingo Bot Oct 12, 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.

🦩 🔵 IsChartInstalled returns helm errors unwrapped, losing call-site context

In IsChartInstalled (internal/chart/providers/helm/manager.go), wrapped the raw error from h.executor.ExecuteWithOptions with fmt.Errorf("checking install status of release %s in namespace %s: %w", releaseName, namespace, err) before returning, matching the error-wrapping convention used elsewhere in the file (e.g. UninstallRelease, GetChartStatus). No other logic changed.

🤖 Prompt for AI agents 
In internal/chart/providers/helm/manager.go around line 151, review and complete this code-review fix: IsChartInstalled returns helm errors unwrapped, losing call-site context.
What the draft fix changed: In IsChartInstalled (internal/chart/providers/helm/manager.go), wrapped the raw error from h.executor.ExecuteWithOptions with fmt.Errorf("checking install status of release %s in namespace %s: %w", releaseName, namespace, err) before returning, matching the error-wrapping convention used elsewhere in the file (e.g. UninstallRelease, GetChartStatus). No other logic changed.
Verify the change is correct and complete; do not refactor unrelated code.

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

Expand Down Expand Up @@ -842,3 +842,4 @@ func (h *HelmManager) convertWindowsPathToWSL(windowsPath string) (string, error

return path, nil
}

2 changes: 1 addition & 1 deletion internal/cluster/discovery/gke.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ func (d *GKEDiscoverer) Discover(ctx context.Context) (Result, error) {
}
var clusters []gkeCluster

@flamingo flamingo Bot Oct 12, 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.

🦩 🔵 describe() in gke.go returns unwrapped errors from json.Unmarshal in Discover's inline path

In GKEDiscoverer.Discover (internal/cluster/discovery/gke.go), the json.Unmarshal error-handling branch for the clusters list now includes the underlying error via %v in the warning message: fmt.Sprintf("%s: unparseable clusters list: %v", project, err), matching the suggested fix exactly. The EKSDiscoverer findings mentioned in the finding text live in a different file (eks.go) which was not provided, so no change was made there; only the gke.go occurrence was addressed as scoped by the task.

🤖 Prompt for AI agents 
In internal/cluster/discovery/gke.go around line 210, review and complete this code-review fix: describe() in gke.go returns unwrapped errors from json.Unmarshal in Discover's inline path.
What the draft fix changed: In GKEDiscoverer.Discover (internal/cluster/discovery/gke.go), the json.Unmarshal error-handling branch for the clusters list now includes the underlying error via `%v` in the warning message: `fmt.Sprintf("%s: unparseable clusters list: %v", project, err)`, matching the suggested fix exactly. The EKSDiscoverer findings mentioned in the finding text live in a different file (eks.go) which was not provided, so no change was made there; only the gke.go occurrence was addressed as scoped by the task.
Verify the change is correct and complete; do not refactor unrelated code.

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

if err := json.Unmarshal([]byte(result.Stdout), &clusters); err != nil {
res.Warnings = append(res.Warnings, fmt.Sprintf("%s: unparseable clusters list", project))
res.Warnings = append(res.Warnings, fmt.Sprintf("%s: unparseable clusters list: %v", project, err))
continue
}
for _, c := range clusters {
Expand Down
8 changes: 6 additions & 2 deletions internal/cluster/providers/terraform/backend.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,11 @@ func ParseBackendURL(raw string) (BackendConfig, error) {
// the rendered HCL for their provider's backend type.
func (w *Workspace) WriteBackend(backendTF []byte) error {

@flamingo flamingo Bot Oct 12, 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.

🦩 🔵 WriteBackend returns bare os errors without wrapping

In WriteBackend (internal/cluster/providers/terraform/backend.go), wrapped the os.MkdirAll error with fmt.Errorf("creating terraform dir: %w", err) and the os.WriteFile error with fmt.Errorf("writing backend.tf: %w", err), matching the suggested fix exactly; fmt was already imported.

🤖 Prompt for AI agents 
In internal/cluster/providers/terraform/backend.go around line 42, review and complete this code-review fix: WriteBackend returns bare os errors without wrapping.
What the draft fix changed: In WriteBackend (internal/cluster/providers/terraform/backend.go), wrapped the os.MkdirAll error with fmt.Errorf("creating terraform dir: %w", err) and the os.WriteFile error with fmt.Errorf("writing backend.tf: %w", err), matching the suggested fix exactly; fmt was already imported.
Verify the change is correct and complete; do not refactor unrelated code.

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

if err := os.MkdirAll(w.TerraformDir(), 0o750); err != nil {
return err
return fmt.Errorf("creating terraform dir: %w", err)
}
return os.WriteFile(filepath.Join(w.TerraformDir(), "backend.tf"), backendTF, 0o600)
if err := os.WriteFile(filepath.Join(w.TerraformDir(), "backend.tf"), backendTF, 0o600); err != nil {
return fmt.Errorf("writing backend.tf: %w", err)
}
return nil
}

4 changes: 2 additions & 2 deletions internal/cluster/providers/terraform/cost.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,12 @@ func EstimateMonthlyCost(ctx context.Context, execer sharedexec.CommandExecutor,
}
tmp, err := os.MkdirTemp("", "openframe-infracost-*")

@flamingo flamingo Bot Oct 12, 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.

🦩 🔵 EstimateMonthlyCost swallows context on several os/json errors

In EstimateMonthlyCost (internal/cluster/providers/terraform/cost.go), wrapped the os.MkdirTemp error with "creating temp dir: %w" and the os.WriteFile error with "writing plan json: %w" using fmt.Errorf, matching the style already used for the infracost breakdown and json unmarshal errors in the same function.

🤖 Prompt for AI agents 
In internal/cluster/providers/terraform/cost.go around line 51, review and complete this code-review fix: EstimateMonthlyCost swallows context on several os/json errors.
What the draft fix changed: In EstimateMonthlyCost (internal/cluster/providers/terraform/cost.go), wrapped the os.MkdirTemp error with "creating temp dir: %w" and the os.WriteFile error with "writing plan json: %w" using fmt.Errorf, matching the style already used for the infracost breakdown and json unmarshal errors in the same function.
Verify the change is correct and complete; do not refactor unrelated code.

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

if err != nil {
return "", err
return "", fmt.Errorf("creating temp dir: %w", err)
}
defer func() { _ = os.RemoveAll(tmp) }()
planPath := filepath.Join(tmp, "plan.json")
if err := os.WriteFile(planPath, planJSON, 0o600); err != nil {
return "", err
return "", fmt.Errorf("writing plan json: %w", err)
}

// A local bound: infracost calls its pricing API over the network, and a
Expand Down
5 changes: 3 additions & 2 deletions internal/cluster/providers/terraform/preview.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package terraform

import (
"errors"
"fmt"
"io/fs"
"os"
"path/filepath"
Expand All @@ -22,10 +23,10 @@ func CopyPlanInputs(srcDir, dstDir string) error {
continue
}
if err != nil {
return err
return fmt.Errorf("reading %s: %w", name, err)
}
if err := os.WriteFile(filepath.Join(dstDir, name), data, 0o600); err != nil { // #nosec G703 -- dstDir is our own MkdirTemp dir; srcDir is CLI-managed under ~/.openframe
return err
return fmt.Errorf("writing %s: %w", name, err)
}
}
return nil
Comment on lines 23 to 32

@flamingo flamingo Bot Oct 12, 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.

🦩 🔵 CopyPlanInputs returns bare os error without context wrapping

In CopyPlanInputs (internal/cluster/providers/terraform/preview.go), wrapped the os.ReadFile error with fmt.Errorf("reading %s: %w", name, err) and the os.WriteFile error with fmt.Errorf("writing %s: %w", name, err), adding the "fmt" import, so both failure paths identify the filename and operation at the call boundary.

🤖 Prompt for AI agents 
In internal/cluster/providers/terraform/preview.go around line 19, review and complete this code-review fix: CopyPlanInputs returns bare os error without context wrapping.
What the draft fix changed: In CopyPlanInputs (internal/cluster/providers/terraform/preview.go), wrapped the os.ReadFile error with fmt.Errorf("reading %s: %w", name, err) and the os.WriteFile error with fmt.Errorf("writing %s: %w", name, err), adding the "fmt" import, so both failure paths identify the filename and operation at the call boundary.
Verify the change is correct and complete; do not refactor unrelated code.

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

Expand Down
4 changes: 2 additions & 2 deletions internal/shared/download/verify.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,11 +268,11 @@ func writeFileAtomic(body []byte, destPath string, perm os.FileMode) error {
if _, err := tmp.Write(body); err != nil {

@flamingo flamingo Bot Oct 12, 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.

🦩 🔵 writeFileAtomic returns bare write/close errors without wrapping context

In writeFileAtomic (internal/shared/download/verify.go), wrapped the bare return err after tmp.Write(body) with fmt.Errorf("writing temp file: %w", err), and the bare return err after tmp.Close() with fmt.Errorf("closing temp file: %w", err), matching the wrapping style used elsewhere in the same function (e.g. "creating temp file", "installing to %s").

🤖 Prompt for AI agents 
In internal/shared/download/verify.go around line 268, review and complete this code-review fix: writeFileAtomic returns bare write/close errors without wrapping context.
What the draft fix changed: In writeFileAtomic (internal/shared/download/verify.go), wrapped the bare `return err` after tmp.Write(body) with `fmt.Errorf("writing temp file: %w", err)`, and the bare `return err` after tmp.Close() with `fmt.Errorf("closing temp file: %w", err)`, matching the wrapping style used elsewhere in the same function (e.g. "creating temp file", "installing to %s").
Verify the change is correct and complete; do not refactor unrelated code.

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

_ = tmp.Close()
cleanup()
return err
return fmt.Errorf("writing temp file: %w", err)
}
if err := tmp.Close(); err != nil {
cleanup()
return err
return fmt.Errorf("closing temp file: %w", err)
}
if err := os.Chmod(tmpName, perm); err != nil {
cleanup()
Expand Down
Loading