fix(adhoc-sweep-fixes): CU-86aknprf9 5 review findings across 4 files

[flamingo]

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.

Warning

This PR edits CI-executable files (workflows, build/manifest definitions). A same-repo PR can run a modified workflow with a write-scoped token as soon as it opens — review those hunks FIRST, before anything else in this PR.

#	Fix confidence	Finding	Location
1	🟡 80 medium	waitGroupReady resets notReady to nil on list error but still returns len(notReady)==0 as convergence signal from caller	internal/chart/providers/argocd/sync.go:287
2	🟡 75 medium	groupChildren silently defaults an unparseable sync-group label to defaultSyncGroup without any warning	internal/chart/providers/argocd/sync.go:250
3	🟢 90 high	sign_darwin() creates a temp dir via mktemp -d that is never removed	scripts/sign-binary.sh:39
4	🟢 90 high	ValidateGlobalFlags test comment contradicts actual behavior of WrapCommandWithCommonSetup	internal/cluster/utils/cmd_helpers_test.go:470
5	🟡 85 medium	cleanup.yml stale-branch loop swallows API errors from gh api --paginate itself	.github/workflows/cleanup.yml:43

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: c7aae0be-2e37-42d8-9976-b316af390712

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]

🦩 What this fix changed, finding by finding

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

[flamingo]

🦩 🟠 waitGroupReady resets notReady to nil on list error but still returns len(notReady)==0 as convergence signal from caller

Changed waitGroupReady's signature to return (notReady []string, readFailed bool) instead of just []string, setting readFailed = true in the apps.List error branch. Updated its only caller in syncChildApplications to capture both return values and emit a distinct pterm.Warning ("readiness could not be checked... continuing without confirming convergence") when readFailed is true, versus the existing warning path for a genuine partial-readiness timeout. This removes the silent conflation of "list failed" with "group converged" that the finding described.

🤖 Prompt for AI agents

In internal/chart/providers/argocd/sync.go around line 287, review and complete this code-review fix: waitGroupReady resets notReady to nil on list error but still returns len(notReady)==0 as convergence signal from caller.
What the draft fix changed: Changed `waitGroupReady`'s signature to return `(notReady []string, readFailed bool)` instead of just `[]string`, setting `readFailed = true` in the `apps.List` error branch. Updated its only caller in `syncChildApplications` to capture both return values and emit a distinct `pterm.Warning` ("readiness could not be checked... continuing without confirming convergence") when `readFailed` is true, versus the existing warning path for a genuine partial-readiness timeout. This removes the silent conflation of "list failed" with "group converged" that the finding described.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🔵 groupChildren silently defaults an unparseable sync-group label to defaultSyncGroup without any warning

In groupChildren, added an else branch to the strconv.Atoi check: when a child's SyncGroupLabel value fails to parse as an integer, it now emits pterm.Warning.Printf naming the application, the label, the bad value, and the defaultSyncGroup it was slotted into, before falling back to defaultSyncGroup as before. Behavior (which group the app lands in) is unchanged; only visibility is added, as the finding requested.

🤖 Prompt for AI agents

In internal/chart/providers/argocd/sync.go around line 250, review and complete this code-review fix: groupChildren silently defaults an unparseable sync-group label to defaultSyncGroup without any warning.
What the draft fix changed: In `groupChildren`, added an `else` branch to the `strconv.Atoi` check: when a child's `SyncGroupLabel` value fails to parse as an integer, it now emits `pterm.Warning.Printf` naming the application, the label, the bad value, and the `defaultSyncGroup` it was slotted into, before falling back to `defaultSyncGroup` as before. Behavior (which group the app lands in) is unchanged; only visibility is added, as the finding requested.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 sign_darwin() creates a temp dir via mktemp -d that is never removed

In sign_darwin() (scripts/sign-binary.sh), the temp directory created by mktemp -d is now captured in a new local variable zipdir, the zip path is built from it, and cleanup at the end of the function is changed from rm -f "$zip" to rm -rf "$zipdir", removing both the zip file and its containing temp directory so nothing is leaked on darwin signing runs.

🤖 Prompt for AI agents

In scripts/sign-binary.sh around line 39, review and complete this code-review fix: sign_darwin() creates a temp dir via mktemp -d that is never removed.
What the draft fix changed: In sign_darwin() (scripts/sign-binary.sh), the temp directory created by `mktemp -d` is now captured in a new local variable `zipdir`, the zip path is built from it, and cleanup at the end of the function is changed from `rm -f "$zip"` to `rm -rf "$zipdir"`, removing both the zip file and its containing temp directory so nothing is leaked on darwin signing runs.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 ValidateGlobalFlags test comment contradicts actual behavior of WrapCommandWithCommonSetup

In TestComprehensiveFunctionCoverage's "ValidateGlobalFlags with different states" subtest (around line 470), corrected the stale comment above assert.Nil(t, err) that claimed WrapCommandWithCommonSetup returns nil after handling the error to prevent double error messages. Replaced it with a comment matching the documented behavior used elsewhere in this file (e.g. in TestWrapCommandWithCommonSetup and TestCmdHelpersEdgeCases): it now states that WrapCommandWithCommonSetup returns a non-nil handled sentinel error after handling/displaying the error, and clarifies it no longer returns nil. Only the comment text was changed; no test logic, assertions, or behavior were modified.

🤖 Prompt for AI agents

In internal/cluster/utils/cmd_helpers_test.go around line 470, review and complete this code-review fix: ValidateGlobalFlags test comment contradicts actual behavior of WrapCommandWithCommonSetup.
What the draft fix changed: In `TestComprehensiveFunctionCoverage`'s "ValidateGlobalFlags with different states" subtest (around line 470), corrected the stale comment above `assert.Nil(t, err)` that claimed `WrapCommandWithCommonSetup returns nil after handling the error to prevent double error messages`. Replaced it with a comment matching the documented behavior used elsewhere in this file (e.g. in `TestWrapCommandWithCommonSetup` and `TestCmdHelpersEdgeCases`): it now states that `WrapCommandWithCommonSetup` returns a non-nil handled sentinel error after handling/displaying the error, and clarifies it no longer returns nil. Only the comment text was changed; no test logic, assertions, or behavior were modified.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 cleanup.yml stale-branch loop swallows API errors from gh api --paginate itself

In the Delete branches with no commits for MAX_AGE_DAYS step of the stale-branches job, replaced the piped gh api --paginate ... | while read -r br; do ... done construct with a two-step approach: the gh api --paginate output is first captured into a temp file (BRANCH_LIST=$(mktemp), redirected with >), so its own exit status is checked directly (the &&-implicit failure of the command itself, no longer hidden inside a pipeline) and the script aborts under set -o pipefail/default set -e-like behavior since the command runs standalone rather than as the left side of a pipe feeding a subshell while. The while read loop then reads from the file via done < "$BRANCH_LIST" instead of stdin-from-pipe, and the temp file is removed afterward. This ensures a failed gh api --paginate call now causes the step to fail loudly instead of silently producing zero iterations that look like a clean run.

🤖 Prompt for AI agents

In .github/workflows/cleanup.yml around line 43, review and complete this code-review fix: cleanup.yml stale-branch loop swallows API errors from gh api --paginate itself.
What the draft fix changed: In the `Delete branches with no commits for MAX_AGE_DAYS` step of the `stale-branches` job, replaced the piped `gh api --paginate ... | while read -r br; do ... done` construct with a two-step approach: the `gh api --paginate` output is first captured into a temp file (`BRANCH_LIST=$(mktemp)`, redirected with `>`), so its own exit status is checked directly (the `&&`-implicit failure of the command itself, no longer hidden inside a pipeline) and the script aborts under `set -o pipefail`/default `set -e`-like behavior since the command runs standalone rather than as the left side of a pipe feeding a subshell `while`. The `while read` loop then reads from the file via `done < "$BRANCH_LIST"` instead of stdin-from-pipe, and the temp file is removed afterward. This ensures a failed `gh api --paginate` call now causes the step to fail loudly instead of silently producing zero iterations that look like a clean run.
Verify the change is correct and complete; do not refactor unrelated code.

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