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

[flamingo]

Closes 4 review findings in internal/cluster/service.go.

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

#	Fix confidence	Finding	Location
1	🔴 30 low — review closely	CleanupCluster's fmt.Errorf paths for cloud/unsupported types are not wrapped as AlreadyHandledError but also never displayed — inconsistent with CreateCluster/DeleteCluster pattern	internal/cluster/service.go:313
2	🔴 55 low — review closely	NewClusterService/NewClusterServiceSuppressed silently discard provider construction errors	internal/cluster/service.go:49
3	🟡 60 medium	cleanupNodeImages returns generic error instead of preserving executor.CommandError exit code	internal/cluster/service.go:371
4	🟡 75 medium	pterm.Warning.WithWriter(os.Stderr) call in ListClusters bypasses --silent suppression	internal/cluster/service.go:251

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

---

Run: https://product-hub.flamingo.so/admin/code-review
Run id: 82c9c576-edf0-420c-92b6-727226d68389

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

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

[flamingo]

🦩 What this fix changed, finding by finding

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

[flamingo]

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

[flamingo]

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

[flamingo]

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

[flamingo]

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