Skip to content

fix(OPENFRAM-007): CU-86aknprf9 3 review findings across 3 files - #416

Draft
flamingo[bot] wants to merge 3 commits into
mainfrom
ai-fix/openfram-007-22b7247c-11664740
Draft

flamingo[bot] wants to merge 3 commits into
mainfrom
ai-fix/openfram-007-22b7247c-11664740

Conversation

@flamingo

@flamingo flamingo Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Closes 3 review findings across 3 files.

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 🟡 85 medium Raw fmt.Println/fmt.Printf used for user-facing helm install output instead of pterm internal/cluster/prerequisites/helm/helm.go:74
2 🟢 95 high println(...) raw builtin bypasses pterm/--silent in wizard_steps.go fallback internal/cluster/ui/wizard_steps.go:277
3 🟡 75 medium Multiple raw fmt.Printf user-facing progress messages in verifyClusterReachable bypass pterm/ui internal/cluster/providers/k3d/verify.go:107

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: 11664740-3810-4fc3-b2cc-8de9b26fe218

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-86aknprf9 OpenFrame CLI code duplication and manager fixes (13 PRs)

@flamingo flamingo Bot left a comment

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.

🦩 What this fix changed, finding by finding

3 finding(s) fixed in this draft — 3 explained inline on the diff.

return fmt.Errorf("automatic helm installation on macOS requires Homebrew. Please install brew first: https://brew.sh")
}

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

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

Replaced raw fmt.Println/fmt.Printf user-facing output in installMacOS and installVerified with pterm.Info.Println/pterm.Info.Printf calls, and added the github.com/pterm/pterm import. This routes install progress messages ("Installing helm via Homebrew...", "Downloading verified helm...", "Installed verified helm...") through pterm so they respect --silent/--plain and pterm's writer redirection in tests, as requested in the finding.

🤖 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.
What the draft fix changed: Replaced raw `fmt.Println`/`fmt.Printf` user-facing output in `installMacOS` and `installVerified` with `pterm.Info.Println`/`pterm.Info.Printf` calls, and added the `github.com/pterm/pterm` import. This routes install progress messages ("Installing helm via Homebrew...", "Downloading verified helm...", "Installed verified helm...") through pterm so they respect `--silent`/`--plain` and pterm's writer redirection in tests, as requested in the finding.
Verify the change is correct and complete; do not refactor unrelated code.

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

if i == 0 {
continue // Skip header
}
println(row[0] + ": " + row[1])

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.

🦩 🔵 println(...) raw builtin bypasses pterm/--silent in wizard_steps.go fallback

In ConfirmConfiguration's fallback branch (triggered when renderConfigurationTable fails), replaced the raw builtin println(row[0] + ": " + row[1]) with pterm.DefaultBasicText.Println(row[0] + ": " + row[1]) as suggested, routing the fallback output through pterm so it respects --silent/--plain/writer redirection. No new imports required since pterm was already imported.

🤖 Prompt for AI agents
In internal/cluster/ui/wizard_steps.go around line 277, review and complete this code-review fix: println(...) raw builtin bypasses pterm/--silent in wizard_steps.go fallback.
What the draft fix changed: In ConfirmConfiguration's fallback branch (triggered when renderConfigurationTable fails), replaced the raw builtin `println(row[0] + ": " + row[1])` with `pterm.DefaultBasicText.Println(row[0] + ": " + row[1])` as suggested, routing the fallback output through pterm so it respects --silent/--plain/writer redirection. No new imports required since `pterm` was already imported.
Verify the change is correct and complete; do not refactor unrelated code.

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

@@ -105,7 +106,7 @@ func (m *K3dManager) verifyClusterReachable(ctx context.Context, clusterName str
var lastErr error

if m.verbose {

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.

🦩 🔵 Multiple raw fmt.Printf user-facing progress messages in verifyClusterReachable bypass pterm/ui

Replaced all fmt.Printf/fmt.Println calls gated by m.verbose in verifyClusterReachable, waitForTCPPort, and cleanupStaleLockFiles with pterm.Debug.Printfln/pterm.Debug.Println, and added the "github.com/pterm/pterm" import. This routes all previously raw progress output through pterm so it respects --silent/--plain and pterm's writer redirection used by tests, matching the OPENFRAM-007 output contract. Risk: pterm.Debug's default level visibility/formatting may differ slightly from the old verbose gate (e.g. prefix text), so the exact rendered output in verbose mode will change even though the information content is preserved; a complete fix would also verify pterm.Debug is enabled under the same conditions as the old m.verbose flag elsewhere in the codebase.

🤖 Prompt for AI agents
In internal/cluster/providers/k3d/verify.go around line 107, review and complete this code-review fix: Multiple raw fmt.Printf user-facing progress messages in verifyClusterReachable bypass pterm/ui.
What the draft fix changed: Replaced all fmt.Printf/fmt.Println calls gated by `m.verbose` in verifyClusterReachable, waitForTCPPort, and cleanupStaleLockFiles with pterm.Debug.Printfln/pterm.Debug.Println, and added the "github.com/pterm/pterm" import. This routes all previously raw progress output through pterm so it respects --silent/--plain and pterm's writer redirection used by tests, matching the OPENFRAM-007 output contract. Risk: pterm.Debug's default level visibility/formatting may differ slightly from the old verbose gate (e.g. prefix text), so the exact rendered output in verbose mode will change even though the information content is preserved; a complete fix would also verify pterm.Debug is enabled under the same conditions as the old `m.verbose` flag elsewhere in the codebase.
Verify the change is correct and complete; do not refactor unrelated code.

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

@flamingo flamingo Bot changed the title fix(OPENFRAM-007): 3 review findings across 3 files fix(OPENFRAM-007): CU-86aknprf9 3 review findings across 3 files Oct 12, 2026
@michaelassraf
michaelassraf deleted the ai-fix/openfram-007-22b7247c-11664740 branch October 13, 2026 02:01
@michaelassraf
michaelassraf restored the ai-fix/openfram-007-22b7247c-11664740 branch October 13, 2026 02:01
@michaelassraf michaelassraf reopened this Oct 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant