Skip to content

fix(OPENFRAM-004): CU-86akbhhau 4 review findings in service.go #376

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 1 commit into
base: main
Choose a base branch
from
+27 −4
Draft

fix(OPENFRAM-004): CU-86akbhhau 4 review findings in service.go #376

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
31 changes: 27 additions & 4 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

@flamingo flamingo Bot Sep 14, 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 errors

In NewClusterService and NewClusterServiceSuppressed (service.go), replaced the silently-discarded _ error from provider.New(models.ClusterTypeK3d, exec) with an explicit check that panics with a descriptive message if construction ever fails. This converts a silent nil-pointer-dereference-at-first-use bug into an immediate, loud, diagnosable failure at construction time, consistent with the documented invariant that k3d construction never fails. A full fix would change both constructors' signatures to return (*ClusterService, error) and propagate the error to all callers (main, bootstrap, tests), which is a larger, cross-file API change outside this file's scope — hence panic as the safe, minimal, same-file mitigation.

🤖 Prompt for AI agents 
In internal/cluster/service.go around line 49, review and complete this code-review fix: NewClusterService/NewClusterServiceSuppressed silently discard provider construction errors.
What the draft fix changed: In `NewClusterService` and `NewClusterServiceSuppressed` (service.go), replaced the silently-discarded `_` error from `provider.New(models.ClusterTypeK3d, exec)` with an explicit check that panics with a descriptive message if construction ever fails. This converts a silent nil-pointer-dereference-at-first-use bug into an immediate, loud, diagnosable failure at construction time, consistent with the documented invariant that k3d construction never fails. A full fix would change both constructors' signatures to return `(*ClusterService, error)` and propagate the error to all callers (main, bootstrap, tests), which is a larger, cross-file API change outside this file's scope — hence panic as the safe, minimal, same-file mitigation.
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

func NewClusterService(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 always construct successfully; if this
// invariant is ever violated, fail loudly here instead of leaving a
// nil manager that panics on first use.
panic(fmt.Sprintf("failed to construct k3d provider: %v", err))
}
return &ClusterService{
manager: manager,
executor: exec,
Expand All @@ -58,7 +64,11 @@ 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 {
// See NewClusterService: same invariant, same fail-fast handling.
panic(fmt.Sprintf("failed to construct k3d provider: %v", err))
}
return &ClusterService{
manager: manager,
executor: exec,
Expand Down Expand Up @@ -247,10 +257,13 @@ func (s *ClusterService) ListClusters() ([]models.ClusterInfo, 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
// goes to stderr so json/yaml output on stdout stays machine-clean.
// goes to stderr so json/yaml output on stdout stays machine-clean, and it
// honors --silent like every other status message in this file.
clusters, err := s.manager.ListAllClusters(ctx)

@flamingo flamingo Bot Sep 14, 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.Warning.WithWriter(os.Stderr) call in ListClusters bypasses --silent suppression

In ListClusters (service.go), gated the pterm.Warning.WithWriter(os.Stderr).Printf(...) call behind if !s.suppressUI, matching the suppression pattern used elsewhere in this file (showNextSteps, showExistingClusterReuse). This directly addresses the described inconsistency for --silent/suppressed mode. Did not route through a shared internal/shared/ui helper since none matching this warning-to-stderr pattern was shown to exist; introducing one would risk importing a non-existent module, so the minimal, verifiable fix (adding the existing suppress flag check) was made instead.

🤖 Prompt for AI agents 
In internal/cluster/service.go around line 251, review and complete this code-review fix: pterm.Warning.WithWriter(os.Stderr) call in ListClusters bypasses --silent suppression.
What the draft fix changed: In `ListClusters` (service.go), gated the `pterm.Warning.WithWriter(os.Stderr).Printf(...)` call behind `if !s.suppressUI`, matching the suppression pattern used elsewhere in this file (`showNextSteps`, `showExistingClusterReuse`). This directly addresses the described inconsistency for `--silent`/suppressed mode. Did not route through a shared `internal/shared/ui` helper since none matching this warning-to-stderr pattern was shown to exist; introducing one would risk importing a non-existent module, so the minimal, verifiable fix (adding the existing suppress flag check) was made instead.
Verify the change is correct and complete; do not refactor unrelated code.

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

if err != nil {
pterm.Warning.WithWriter(os.Stderr).Printf("local (k3d) clusters could not be listed (is Docker running?): %v\n", err)
if !s.suppressUI {
pterm.Warning.WithWriter(os.Stderr).Printf("local (k3d) clusters could not be listed (is Docker running?): %v\n", err)
}
clusters = nil
}
for _, cloud := range s.cloudProviders() {
Expand Down Expand Up @@ -310,6 +323,13 @@ func (s *ClusterService) DetectClusterType(name string) (models.ClusterType, err
// is how a routine cleanup destroyed a working install. Tearing the platform
// down is `app uninstall`'s job; tearing the cluster down is `cluster
// delete`'s.
//
// The cloud/unsupported-type errors below are returned plain (not wrapped in
// AlreadyHandledError): they have not been displayed to the user anywhere in
// this path, so the command layer's single print path (via
// sharedErrors.HandleGlobalError in cmd/cluster/cleanup.go) is expected to
// print them exactly once — the same contract CreateCluster/DeleteCluster
// rely on for their provider errors.
func (s *ClusterService) CleanupCluster(ctx context.Context, name string, clusterType models.ClusterType, verbose bool) (models.CleanupResult, error) {

@flamingo flamingo Bot Sep 14, 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.

🦩 🔴 CleanupCluster's fmt.Errorf paths for cloud/unsupported types are not wrapped as AlreadyHandledError but also never displayed — inconsistent with CreateCluster/DeleteCluster pattern

In CleanupCluster (service.go), left the cloud/unsupported-type fmt.Errorf returns plain (not wrapped in AlreadyHandledError), and added a doc comment above the function explaining the contract: these errors are not yet displayed anywhere in this file, so the command layer (cmd/cluster/cleanup.go) is expected to route them through sharedErrors.HandleGlobalError exactly once, matching CreateCluster/DeleteCluster's pattern. I could not see or modify cmd/cluster/cleanup.go to verify/enforce that it actually calls HandleGlobalError on this path — that audit is the remaining, unverified half of this finding.

🤖 Prompt for AI agents 
In internal/cluster/service.go around line 313, review and complete this code-review fix: CleanupCluster's fmt.Errorf paths for cloud/unsupported types are not wrapped as AlreadyHandledError but also never displayed — inconsistent with CreateCluster/DeleteCluster pattern.
What the draft fix changed: In `CleanupCluster` (service.go), left the cloud/unsupported-type `fmt.Errorf` returns plain (not wrapped in AlreadyHandledError), and added a doc comment above the function explaining the contract: these errors are not yet displayed anywhere in this file, so the command layer (`cmd/cluster/cleanup.go`) is expected to route them through `sharedErrors.HandleGlobalError` exactly once, matching CreateCluster/DeleteCluster's pattern. I could not see or modify `cmd/cluster/cleanup.go` to verify/enforce that it actually calls `HandleGlobalError` on this path — that audit is the remaining, unverified half of this finding.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

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

switch clusterType {
case models.ClusterTypeK3d:
Expand Down Expand Up @@ -371,6 +391,9 @@ func (s *ClusterService) cleanupNodeImages(ctx context.Context, clusterName stri
nodeNames, err := s.getK3dClusterNodes(ctx, clusterName)

@flamingo flamingo Bot Sep 14, 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.

🦩 🟠 cleanupNodeImages returns generic error instead of preserving executor.CommandError exit code

In cleanupNodeImages (service.go), the fmt.Errorf("could not discover cluster nodes: %w", err) line was already using %w, which does preserve an underlying *executor.CommandError (and its exit code) through errors.As/errors.Unwrap — added a comment clarifying this so the wrapping intent is explicit and not mistaken for the lossy pattern the finding describes. No functional change was needed since %w was already in use; the risk is that some caller further up may compare error strings instead of unwrapping, which is outside this file's visibility.

🤖 Prompt for AI agents 
In internal/cluster/service.go around line 371, review and complete this code-review fix: cleanupNodeImages returns generic error instead of preserving executor.CommandError exit code.
What the draft fix changed: In `cleanupNodeImages` (service.go), the `fmt.Errorf("could not discover cluster nodes: %w", err)` line was already using `%w`, which does preserve an underlying `*executor.CommandError` (and its exit code) through `errors.As`/`errors.Unwrap` — added a comment clarifying this so the wrapping intent is explicit and not mistaken for the lossy pattern the finding describes. No functional change was needed since `%w` was already in use; the risk is that some caller further up may compare error strings instead of unwrapping, which is outside this file's visibility.
Verify the change is correct and complete; do not refactor unrelated code.

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

if err != nil {
// Not fatal: a cluster whose nodes are already gone still cleans up.
// Wrap with %w so any underlying *executor.CommandError (and its exit
// code) survives the wrap for main.exitCode() to inspect, rather than
// being discarded.
return 0, fmt.Errorf("could not discover cluster nodes: %w", err)
}

Expand Down
Loading