Skip to content

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

Draft
flamingo[bot] wants to merge 4 commits into
mainfrom
ai-fix/openfram-007-eea812ad-82c9c576
Draft

flamingo[bot] wants to merge 4 commits into
mainfrom
ai-fix/openfram-007-eea812ad-82c9c576

Conversation

@flamingo

@flamingo flamingo Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Closes 5 review findings across 4 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 🟡 60 medium verify.go uses raw fmt.Printf/fmt.Println for user-facing verbose output instead of pterm/ui helpers internal/cluster/providers/k3d/verify.go:48
2 🟡 60 medium waitForTCPPort emits raw fmt.Printf for progress instead of using pterm internal/cluster/providers/k3d/verify.go:196
3 🟢 90 high Raw pterm.Info.Println/Printf calls bypass silent-mode gating in HelmManager internal/chart/providers/helm/manager.go:313
4 🟢 95 high Raw println() used as fallback instead of pterm-based output internal/cluster/ui/wizard_steps.go:271
5 🔴 55 low — review closely AppOfApps.Install uses pterm.Info.Printf directly instead of a silence-aware wrapper for the ref message internal/chart/services/appofapps.go:54

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

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

@@ -46,7 +48,7 @@ func (m *K3dManager) verifyClusterReachable(ctx context.Context, clusterName str
}

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.

🦩 🟠 verify.go uses raw fmt.Printf/fmt.Println for user-facing verbose output instead of pterm/ui helpers

Replaced raw fmt.Printf/fmt.Println calls in verifyClusterReachable (lines with context switch confirmation, TLS bypass notice, host:port extraction warning, and the node-readiness retry loop) with pterm.Success/pterm.Info/pterm.Warning printers (pterm.Success.Printfln, pterm.Info.Printfln, pterm.Warning.Printfln, etc.), all still gated by if m.verbose. This routes output through pterm's writer so it respects pterm's global output redirection used in tests, but I did not have visibility into a project-specific internal/shared/ui wrapper, so I used the pterm package directly rather than inventing an ui helper API that may not exist. --silent/--plain flag wiring is not addressed here since that logic is outside this file's visibility.

🤖 Prompt for AI agents
In internal/cluster/providers/k3d/verify.go around line 48, review and complete this code-review fix: verify.go uses raw fmt.Printf/fmt.Println for user-facing verbose output instead of pterm/ui helpers.
What the draft fix changed: Replaced raw fmt.Printf/fmt.Println calls in verifyClusterReachable (lines with context switch confirmation, TLS bypass notice, host:port extraction warning, and the node-readiness retry loop) with pterm.Success/pterm.Info/pterm.Warning printers (pterm.Success.Printfln, pterm.Info.Printfln, pterm.Warning.Printfln, etc.), all still gated by `if m.verbose`. This routes output through pterm's writer so it respects pterm's global output redirection used in tests, but I did not have visibility into a project-specific internal/shared/ui wrapper, so I used the pterm package directly rather than inventing an ui helper API that may not exist. --silent/--plain flag wiring is not addressed here since that logic 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

@@ -194,7 +196,7 @@ func (m *K3dManager) waitForTCPPort(ctx context.Context, host string, port strin
address := net.JoinHostPort(host, port)

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.

🦩 🟠 waitForTCPPort emits raw fmt.Printf for progress instead of using pterm

Replaced fmt.Printf calls in waitForTCPPort (the "Waiting for TCP port..." message, the "TCP port %s is open" success message, and the retry-attempt message) with pterm.Info.Printfln and pterm.Success.Printfln, preserving the existing if m.verbose guards. Also updated cleanupStaleLockFiles's fmt.Println to pterm.Success.Println for consistency, since it exhibited the same raw-fmt pattern called out generally in finding 1. Same caveat as above: used pterm directly since no internal/shared/ui module was visible to import safely.

🤖 Prompt for AI agents
In internal/cluster/providers/k3d/verify.go around line 196, review and complete this code-review fix: waitForTCPPort emits raw fmt.Printf for progress instead of using pterm.
What the draft fix changed: Replaced fmt.Printf calls in waitForTCPPort (the "Waiting for TCP port..." message, the "TCP port %s is open" success message, and the retry-attempt message) with pterm.Info.Printfln and pterm.Success.Printfln, preserving the existing `if m.verbose` guards. Also updated cleanupStaleLockFiles's fmt.Println to pterm.Success.Println for consistency, since it exhibited the same raw-fmt pattern called out generally in finding 1. Same caveat as above: used pterm directly since no internal/shared/ui module was visible to import safely.
Verify the change is correct and complete; do not refactor unrelated code.

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

@@ -313,7 +313,7 @@ func (h *HelmManager) InstallArgoCDWithProgress(ctx context.Context, config conf
var spinner *uispinner.Spinner

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 pterm.Info.Println/Printf calls bypass silent-mode gating in HelmManager

In InstallArgoCDWithProgress (internal/chart/providers/helm/manager.go), changed the else branch that unconditionally called pterm.Info.Println("Installing ArgoCD...") to else if !config.Silent, so the message is now only printed in non-interactive-but-not-silent mode, and fully suppressed when config.Silent is true — matching the suggested fix exactly and respecting the --silent flag per OPENFRAM-007.

🤖 Prompt for AI agents
In internal/chart/providers/helm/manager.go around line 313, review and complete this code-review fix: Raw pterm.Info.Println/Printf calls bypass silent-mode gating in HelmManager.
What the draft fix changed: In `InstallArgoCDWithProgress` (internal/chart/providers/helm/manager.go), changed the `else` branch that unconditionally called `pterm.Info.Println("Installing ArgoCD...")` to `else if !config.Silent`, so the message is now only printed in non-interactive-but-not-silent mode, and fully suppressed when `config.Silent` is true — matching the suggested fix exactly and respecting the --silent flag per OPENFRAM-007.
Verify the change is correct and complete; do not refactor unrelated code.

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

Comment on lines 274 to 280
if i == 0 {
continue // Skip header
}
println(row[0] + ": " + row[1])
pterm.Info.Printf("%s: %s\n", 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.

🦩 🟠 Raw println() used as fallback instead of pterm-based output

Replaced the raw println(row[0] + ": " + row[1]) builtin call in ConfirmConfiguration's fallback branch (used when renderConfigurationTable fails) with pterm.Info.Printf("%s: %s\n", row[0], row[1]), matching the suggested fix exactly so the fallback path goes through pterm and respects --silent/--plain and test writer redirection.

🤖 Prompt for AI agents
In internal/cluster/ui/wizard_steps.go around line 271, review and complete this code-review fix: Raw println() used as fallback instead of pterm-based output.
What the draft fix changed: Replaced the raw `println(row[0] + ": " + row[1])` builtin call in ConfirmConfiguration's fallback branch (used when renderConfigurationTable fails) with `pterm.Info.Printf("%s: %s\n", row[0], row[1])`, matching the suggested fix exactly so the fallback path goes through pterm and respects --silent/--plain and test writer redirection.
Verify the change is correct and complete; do not refactor unrelated code.

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

// wording read as if it reflected the cluster's current ref, which made a
// dry-run against a cluster on another ref confusing (verification report,
// minor observation).
pterm.Info.Printf("Deploying ref '%s'...\n", appConfig.GitHubBranch)

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.

🦩 🟠 AppOfApps.Install uses pterm.Info.Printf directly instead of a silence-aware wrapper for the ref message

In AppOfApps.Install (internal/chart/services/appofapps.go), wrapped the pterm.Info.Printf("Deploying ref '%s'...\n", ...) call with if !config.Silent { ... }, matching the existing config.Silent check already used for the clone spinner in the same function. This makes the ref message respect --silent consistently with the rest of the file's convention, without introducing a new shared silence-aware wrapper module (none exists in the given file to extract to, and creating one would exceed the "minimal fix in this file" scope). Risk: if other call sites rely on this exact message being unconditionally printed (e.g. tests asserting on stdout), this would break them; a complete fix per the finding's broader framing would add a genuine silence-aware output abstraction used repo-wide, which is out of scope for a single-file fix.

🤖 Prompt for AI agents
In internal/chart/services/appofapps.go around line 54, review and complete this code-review fix: AppOfApps.Install uses pterm.Info.Printf directly instead of a silence-aware wrapper for the ref message.
What the draft fix changed: In AppOfApps.Install (internal/chart/services/appofapps.go), wrapped the `pterm.Info.Printf("Deploying ref '%s'...\n", ...)` call with `if !config.Silent { ... }`, matching the existing `config.Silent` check already used for the clone spinner in the same function. This makes the ref message respect `--silent` consistently with the rest of the file's convention, without introducing a new shared silence-aware wrapper module (none exists in the given file to extract to, and creating one would exceed the "minimal fix in this file" scope). Risk: if other call sites rely on this exact message being unconditionally printed (e.g. tests asserting on stdout), this would break them; a complete fix per the finding's broader framing would add a genuine silence-aware output abstraction used repo-wide, which is out of scope for a single-file fix.
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 flamingo Bot changed the title fix(OPENFRAM-007): 5 review findings across 4 files fix(OPENFRAM-007): CU-86akbhhau 5 review findings across 4 files Sep 14, 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.

0 participants