fix(adhoc-sweep-fixes): CU-86akbhhau 14 review findings across 12 files - #377
flamingo[bot] wants to merge 12 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| @@ -131,7 +131,9 @@ func (m *Manager) WaitForApplications(ctx context.Context, config config.ChartIn | |||
|
|
|||
| // Function to stop spinner safely | |||
| stopSpinner := func() { | |||
There was a problem hiding this comment.
🦩 🔴 stopSpinner unconditionally calls dash.Stop() even when dash is nil
In WaitForApplications, the stopSpinner closure now guards dash.Stop() with if dash != nil before calling it, matching the suggested fix exactly. This prevents a nil-pointer panic in silent/verbose modes when stopSpinner runs (via defer or the cancellation goroutine).
🤖 Prompt for AI agents
In internal/chart/providers/argocd/wait.go around line 133, review and complete this code-review fix: stopSpinner unconditionally calls dash.Stop() even when dash is nil.
What the draft fix changed: In `WaitForApplications`, the `stopSpinner` closure now guards `dash.Stop()` with `if dash != nil` before calling it, matching the suggested fix exactly. This prevents a nil-pointer panic in silent/verbose modes when `stopSpinner` runs (via `defer` or the cancellation goroutine).
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| @@ -290,7 +292,9 @@ func (m *Manager) WaitForApplications(ctx context.Context, config config.ChartIn | |||
| case <-ticker.C: | |||
| // Check timeout | |||
| if time.Since(startTime) > timeout { | |||
There was a problem hiding this comment.
🦩 🔴 dash.Fail() called unconditionally in the timeout branch even when dash may be nil
In the ticker-driven timeout branch of WaitForApplications, dash.Fail(fmt.Sprintf("Timeout after %v", timeout)) is now wrapped in if dash != nil { ... }, matching the suggested fix. I also applied the same nil-guard pattern to the other three unconditional dash.Fail(...)/dash.Update(...) call sites found in the same function (fatal-manifest fail-fast, degraded fail-fast, ref-mismatch fail, and the per-iteration dash.Update progress call) since they share the identical defect and are directly adjacent to the reported evidence; these were not separately itemized in the findings but are the same bug pattern within the same function, so leaving them unconditional would still panic in silent/verbose mode on those code paths.
🤖 Prompt for AI agents
In internal/chart/providers/argocd/wait.go around line 292, review and complete this code-review fix: dash.Fail() called unconditionally in the timeout branch even when dash may be nil.
What the draft fix changed: In the ticker-driven timeout branch of `WaitForApplications`, `dash.Fail(fmt.Sprintf("Timeout after %v", timeout))` is now wrapped in `if dash != nil { ... }`, matching the suggested fix. I also applied the same nil-guard pattern to the other three unconditional `dash.Fail(...)`/`dash.Update(...)` call sites found in the same function (fatal-manifest fail-fast, degraded fail-fast, ref-mismatch fail, and the per-iteration `dash.Update` progress call) since they share the identical defect and are directly adjacent to the reported evidence; these were not separately itemized in the findings but are the same bug pattern within the same function, so leaving them unconditional would still panic in silent/verbose mode on those code paths.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| @@ -31,16 +31,7 @@ func InitGlobalFlags() { | |||
|
|
|||
| // GetCommandService creates a command service for business logic operations | |||
There was a problem hiding this comment.
🦩 🔴 CommandExecutor() and GetCommandService() construct two independent RealCommandExecutor instances instead of sharing one
Changed GetCommandService() (in internal/cluster/utils/cmd_helpers.go) to call CommandExecutor() instead of duplicating the injected-executor check and dryRun/verbose resolution + executor.NewRealCommandExecutor construction. Now both GetCommandService() and CommandExecutor() funnel through the single CommandExecutor() implementation, eliminating the duplicated construction logic and the drift risk; CommandExecutor() itself is unchanged as the single source of truth.
🤖 Prompt for AI agents
In internal/cluster/utils/cmd_helpers.go around line 32, review and complete this code-review fix: CommandExecutor() and GetCommandService() construct two independent RealCommandExecutor instances instead of sharing one.
What the draft fix changed: Changed GetCommandService() (in internal/cluster/utils/cmd_helpers.go) to call CommandExecutor() instead of duplicating the injected-executor check and dryRun/verbose resolution + executor.NewRealCommandExecutor construction. Now both GetCommandService() and CommandExecutor() funnel through the single CommandExecutor() implementation, eliminating the duplicated construction logic and the drift risk; CommandExecutor() itself is unchanged as the single source of truth.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer
| // meaningful to gcloud's --filter expression syntax (spaces, "OR", | ||
| // parentheses, quotes), so it cannot alter the filter's semantics and bypass | ||
| // this collision guard. | ||
| func (p *Provider) preflightNameCollision(ctx context.Context, config models.ClusterConfig) error { |
There was a problem hiding this comment.
🦩 🔴 GKE preflightNameCollision filter uses unescaped user-controlled cluster name in gcloud --filter, risking filter-expression injection
Added a safeClusterNameRE regexp matching GKE's own valid cluster-name character set (lowercase alphanumeric + hyphens, must start with a letter), and a new check at the top of preflightNameCollision (internal/cluster/providers/gke/provider.go) that rejects config.Name with a descriptive error before it is ever interpolated into the --filter=name=... gcloud argument. This guarantees the name used in the filter cannot contain spaces, OR, parentheses, quotes, or other characters meaningful to gcloud's filter expression syntax, closing the injection/bypass path described in the finding, while still accepting every name GKE itself would accept. Confidence is not higher because I cannot see whether validate(config) elsewhere in the CLI already enforces an equivalent (or looser) pattern before this point is reached — if it allows characters this regex rejects for otherwise-legitimate names, this introduces a new user-facing validation error at create/resume time; the safe fix errs on rejecting rather than risking bypass.
🤖 Prompt for AI agents
In internal/cluster/providers/gke/provider.go around line 156, review and complete this code-review fix: GKE preflightNameCollision filter uses unescaped user-controlled cluster name in gcloud --filter, risking filter-expression injection.
What the draft fix changed: Added a `safeClusterNameRE` regexp matching GKE's own valid cluster-name character set (lowercase alphanumeric + hyphens, must start with a letter), and a new check at the top of `preflightNameCollision` (`internal/cluster/providers/gke/provider.go`) that rejects `config.Name` with a descriptive error before it is ever interpolated into the `--filter=name=...` gcloud argument. This guarantees the name used in the filter cannot contain spaces, `OR`, parentheses, quotes, or other characters meaningful to gcloud's filter expression syntax, closing the injection/bypass path described in the finding, while still accepting every name GKE itself would accept. Confidence is not higher because I cannot see whether `validate(config)` elsewhere in the CLI already enforces an equivalent (or looser) pattern before this point is reached — if it allows characters this regex rejects for otherwise-legitimate names, this introduces a new user-facing validation error at create/resume time; the safe fix errs on rejecting rather than risking bypass.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| if err != nil { | ||
| return nil, "", fmt.Errorf("could not use context %q: %w", contextName, err) | ||
| } | ||
| return cfg, clusterNameArg(args), nil | ||
| // A positional cluster name, if also given, is only a label for this | ||
| // --context-resolved config (it does not re-resolve the context), so | ||
| // only use it when the two cannot conflict: no --context-derived name | ||
| // of its own exists. Otherwise prefer contextName itself so the | ||
| // ClusterName reported in wait/status output matches the config that | ||
| // was actually resolved. | ||
| if name := clusterNameArg(args); name != "" { | ||
| return cfg, name, nil | ||
| } | ||
| return cfg, contextName, nil | ||
| } | ||
|
|
||
| if name := clusterNameArg(args); name != "" { |
There was a problem hiding this comment.
🦩 🟠 resolveUpgradeTarget silently discards clusterName in --context branch causing empty ClusterName in later WaitForApplications config
In resolveUpgradeTarget (cmd/app/upgrade.go), the --context branch previously always returned clusterNameArg(args) as the cluster name even though cfg was resolved from contextName, so a mismatched positional name could be reported alongside a --context-derived config. The branch now falls back to contextName itself as the ClusterName when no positional name is supplied, keeping prior documented behavior (name used as a label) for the common case, but does not change the underlying config resolution (still derived solely from contextName) since re-deriving cfg from the positional name when --context is also given would change existing documented precedence and risks resolving to a different cluster than the user explicitly requested via --context. A complete fix would require deciding product intent (should --context + positional name be an error, or should --context win outright for both cfg and label) which is architectural and out of scope for a minimal patch.
🤖 Prompt for AI agents
In cmd/app/upgrade.go around line 201, review and complete this code-review fix: resolveUpgradeTarget silently discards clusterName in --context branch causing empty ClusterName in later WaitForApplications config.
What the draft fix changed: In `resolveUpgradeTarget` (cmd/app/upgrade.go), the `--context` branch previously always returned `clusterNameArg(args)` as the cluster name even though `cfg` was resolved from `contextName`, so a mismatched positional name could be reported alongside a `--context`-derived config. The branch now falls back to `contextName` itself as the `ClusterName` when no positional name is supplied, keeping prior documented behavior (name used as a label) for the common case, but does not change the underlying config resolution (still derived solely from `contextName`) since re-deriving `cfg` from the positional name when `--context` is also given would change existing documented precedence and risks resolving to a different cluster than the user explicitly requested via `--context`. A complete fix would require deciding product intent (should `--context` + positional name be an error, or should `--context` win outright for both cfg and label) which is architectural and out of scope for a minimal patch.
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
| fmt.Fprintf(&b, " - %s is on %q, not %q\n", x.App, x.Got, x.Want) | ||
| } | ||
| b.WriteString("Use a branch whose chart reads repository.branch, or pin these applications' targetRevision by hand.") | ||
| return fmt.Errorf("%s", b.String()) | ||
| return selfDiagnosedError{msg: b.String()} | ||
| } | ||
|
|
There was a problem hiding this comment.
🦩 🟠 refMismatchError does not use selfDiagnosedError despite embedding per-app diagnostic text
Changed refMismatchError in refassert.go to return selfDiagnosedError{msg: b.String()} instead of fmt.Errorf("%s", b.String()), matching the established pattern used by degradedAppError in this package so the generic handler's pattern-matched hints don't misfire on the embedded per-app branch/revision diagnostic text. This assumes selfDiagnosedError (with a msg field) is already defined elsewhere in the argocd package, as referenced by the finding regarding degradedAppError; I did not redefine it here since it must already exist in another file of the same package.
🤖 Prompt for AI agents
In internal/chart/providers/argocd/refassert.go around line 94, review and complete this code-review fix: refMismatchError does not use selfDiagnosedError despite embedding per-app diagnostic text.
What the draft fix changed: Changed `refMismatchError` in refassert.go to return `selfDiagnosedError{msg: b.String()}` instead of `fmt.Errorf("%s", b.String())`, matching the established pattern used by `degradedAppError` in this package so the generic handler's pattern-matched hints don't misfire on the embedded per-app branch/revision diagnostic text. This assumes `selfDiagnosedError` (with a `msg` field) is already defined elsewhere in the `argocd` package, as referenced by the finding regarding `degradedAppError`; I did not redefine it here since it must already exist in another file of the same package.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer
| // labeled install falls into defaultSyncGroup, mirroring the template default. | ||
| // A non-numeric value (e.g. a typo like "1a") is surfaced with a warning | ||
| // rather than silently absorbed, since it is likely a misconfiguration. | ||
| func groupChildren(children []unstructured.Unstructured) (groups []syncGroup, labeled bool) { |
There was a problem hiding this comment.
🦩 🟠 groupChildren silently misclassifies non-numeric sync-group labels into the default group without warning
In groupChildren (internal/chart/providers/argocd/sync.go), added an else branch to the strconv.Atoi check that emits a pterm.Warning.Printf naming the offending Application, the raw label value, and the group it defaults to, whenever the SyncGroupLabel value fails to parse as an integer. The misclassification behavior (falling back to defaultSyncGroup) is unchanged — only the previously-silent case now produces a diagnostic, matching the pattern already used elsewhere in this file (e.g. the tracking-label fallback warning in syncChildApplications).
🤖 Prompt for AI agents
In internal/chart/providers/argocd/sync.go around line 249, review and complete this code-review fix: groupChildren silently misclassifies non-numeric sync-group labels into the default group without warning.
What the draft fix changed: In `groupChildren` (internal/chart/providers/argocd/sync.go), added an `else` branch to the `strconv.Atoi` check that emits a `pterm.Warning.Printf` naming the offending Application, the raw label value, and the group it defaults to, whenever the `SyncGroupLabel` value fails to parse as an integer. The misclassification behavior (falling back to `defaultSyncGroup`) is unchanged — only the previously-silent case now produces a diagnostic, matching the pattern already used elsewhere in this file (e.g. the tracking-label fallback warning in `syncChildApplications`).
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| continue | ||
| } | ||
|
|
||
| if !confirmed { |
There was a problem hiding this comment.
🦩 🟠 Recursive wizard restart on 'no' answer grows the call stack indefinitely and loses the ability to go back to a specific step
In ConfigWizard.Run (internal/cluster/ui/wizard.go), replaced the recursive return w.Run() call on a "no" answer with a for { ... } loop wrapping the entire step sequence; the "not confirmed" branch now does continue instead of recursing. This reuses the same stack frame and the same w.config/steps state across repeated "modify" answers instead of re-invoking the function, eliminating unbounded stack growth. All original per-step logic and error returns are preserved unchanged inside the loop body.
🤖 Prompt for AI agents
In internal/cluster/ui/wizard.go around line 158, review and complete this code-review fix: Recursive wizard restart on 'no' answer grows the call stack indefinitely and loses the ability to go back to a specific step.
What the draft fix changed: In `ConfigWizard.Run` (internal/cluster/ui/wizard.go), replaced the recursive `return w.Run()` call on a "no" answer with a `for { ... }` loop wrapping the entire step sequence; the "not confirmed" branch now does `continue` instead of recursing. This reuses the same stack frame and the same `w.config`/`steps` state across repeated "modify" answers instead of re-invoking the function, eliminating unbounded stack growth. All original per-step logic and error returns are preserved unchanged inside the loop body.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| warned bool | ||
| } | ||
|
|
||
| func (t *bestEffortTee) Write(p []byte) (int, error) { |
There was a problem hiding this comment.
🦩 🟠 opSinks opens the terraform.log file with O_APPEND but never closes it if opFailure/callers panic before defer runs — minor, but more importantly bestEffortTee silently drops disk-full errors without ever surfacing them to the user, even in verbose mode
In bestEffortTee.Write (internal/cluster/providers/terraform/engine.go), added logPath and warned fields; on the first sink write failure it now calls pterm.Warning.Printfln(...) once, naming the log path and the underlying error, before dropping the sink — surfacing the previously-silent log-write failure to the user. opSinks now populates logPath when constructing the tee, and also warns (once, via the same mechanism) if the initial header fmt.Fprintf to the freshly-opened log fails, since that failure was previously ignored entirely and would otherwise leave the tee wrongly believing the sink is healthy. The pterm import was added; it is already a transitive dependency of this codebase (used elsewhere in the CLI for terminal output) so no new module needs to be written. The underlying "never closes file if a panic occurs before defer runs" half of the finding is not addressed — os.OpenFile's handle in opSinks is only closed via the deferred close() in the callers (Apply/Destroy/ApplyPlan), and a panic between open and defer-registration is a pre-existing, extremely narrow window not touched by this change; a complete fix would require restructuring opSinks to register the close before returning to callers, which is a larger structural change than this finding's stated primary concern (silent error suppression) warrants.
🤖 Prompt for AI agents
In internal/cluster/providers/terraform/engine.go around line 154, review and complete this code-review fix: opSinks opens the terraform.log file with O_APPEND but never closes it if opFailure/callers panic before defer runs — minor, but more importantly bestEffortTee silently drops disk-full errors without ever surfacing them to the user, even in verbose mode.
What the draft fix changed: In `bestEffortTee.Write` (internal/cluster/providers/terraform/engine.go), added `logPath` and `warned` fields; on the first sink write failure it now calls `pterm.Warning.Printfln(...)` once, naming the log path and the underlying error, before dropping the sink — surfacing the previously-silent log-write failure to the user. `opSinks` now populates `logPath` when constructing the tee, and also warns (once, via the same mechanism) if the initial header `fmt.Fprintf` to the freshly-opened log fails, since that failure was previously ignored entirely and would otherwise leave the tee wrongly believing the sink is healthy. The `pterm` import was added; it is already a transitive dependency of this codebase (used elsewhere in the CLI for terminal output) so no new module needs to be written. The underlying "never closes file if a panic occurs before defer runs" half of the finding is not addressed — `os.OpenFile`'s handle in `opSinks` is only closed via the deferred `close()` in the callers (Apply/Destroy/ApplyPlan), and a panic between open and defer-registration is a pre-existing, extremely narrow window not touched by this change; a complete fix would require restructuring `opSinks` to register the close before returning to callers, which is a larger structural change than this finding's stated primary concern (silent error suppression) warrants.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer
| defer CleanupTestCluster(name) | ||
|
|
||
| err := CreateTestCluster(name) | ||
| if err != nil { | ||
| t.Logf("Failed to create cluster (may be expected): %v", err) | ||
| } | ||
| assert.NoError(t, err, "Failed to create cluster") | ||
| } | ||
|
|
||
| func TestClusterExists(t *testing.T) { |
There was a problem hiding this comment.
🦩 🔵 TestCreateTestCluster only logs a failure instead of asserting cluster creation succeeded
In TestCreateTestCluster (tests/integration/common/cluster_management_test.go), replaced the if err != nil { t.Logf(...) } pattern with assert.NoError(t, err, "Failed to create cluster"), so the test now actually fails when cluster creation errors instead of silently logging and passing.
🤖 Prompt for AI agents
In tests/integration/common/cluster_management_test.go around line 21, review and complete this code-review fix: TestCreateTestCluster only logs a failure instead of asserting cluster creation succeeded.
What the draft fix changed: In TestCreateTestCluster (tests/integration/common/cluster_management_test.go), replaced the `if err != nil { t.Logf(...) }` pattern with `assert.NoError(t, err, "Failed to create cluster")`, so the test now actually fails when cluster creation errors instead of silently logging and passing.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
flamingo
Bot
changed the title
Closes 14 review findings across 12 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
internal/chart/providers/argocd/wait.go:133internal/chart/providers/argocd/wait.go:292internal/cluster/utils/cmd_helpers.go:32internal/cluster/providers/gke/provider.go:156cmd/app/upgrade.go:201cmd/app/upgrade.go:109internal/cluster/discovery/gke.go:72internal/cluster/discovery/eks.go:194internal/chart/prerequisites/certificates/certificates.go:176internal/chart/providers/argocd/refassert.go:94internal/chart/providers/argocd/sync.go:249internal/cluster/ui/wizard.go:158internal/cluster/providers/terraform/engine.go:154tests/integration/common/cluster_management_test.go:21What 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-727226d68389Merging 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)