fix(adhoc-sweep-fixes): CU-86akdypw4 5 review findings across 5 files - #36
flamingo[bot] wants to merge 5 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| } > RELEASE_HEADER.md | ||
| cat RELEASE_HEADER.md | ||
|
|
||
| - name: Delete existing latest release |
There was a problem hiding this comment.
🦩 🟠 Release workflow deletes 'latest' release/tag unconditionally on push without confirming a new one will succeed
In the release job, reordered the steps so that Create Release (softprops/action-gh-release@v2) now runs before the Delete previous latest release step (renamed from "Delete existing latest release"). The deletion of the old latest release/tag now only happens after the new release has been successfully created, so if action-gh-release fails, the previous latest release/tag remains intact for updaters to fall back on. Note: this relies on tag_name for the new release differing from the literal tag latest (it uses needs.version.outputs.version), which the existing code already assumes; a complete fix would also want to verify how latest is assigned/aliased elsewhere in the repo (not visible in this file) to confirm no atomic "move latest tag" step is required instead.
🤖 Prompt for AI agents
In .github/workflows/release.yml around line 286, review and complete this code-review fix: Release workflow deletes 'latest' release/tag unconditionally on push without confirming a new one will succeed.
What the draft fix changed: In the `release` job, reordered the steps so that `Create Release` (softprops/action-gh-release@v2) now runs before the `Delete previous latest release` step (renamed from "Delete existing latest release"). The deletion of the old `latest` release/tag now only happens after the new release has been successfully created, so if action-gh-release fails, the previous `latest` release/tag remains intact for updaters to fall back on. Note: this relies on `tag_name` for the new release differing from the literal tag `latest` (it uses `needs.version.outputs.version`), which the existing code already assumes; a complete fix would also want to verify how `latest` is assigned/aliased elsewhere in the repo (not visible in this file) to confirm no atomic "move latest tag" step is required instead.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
|
|
||
| IFS=',' read -r -a DENY_PATTERNS <<< "$BRANCH_DENYLIST" | ||
|
|
||
| gh api --paginate "repos/$REPO/branches?protected=false&per_page=100" \ |
There was a problem hiding this comment.
🦩 🟠 cleanup.yml stale-branch deletion has no dry-run or protected-default-branch exclusion beyond protected=false filter
In the "Delete branches with no commits for MAX_AGE_DAYS" step of the stale-branches job, added a BRANCH_DENYLIST env var (glob patterns for release/*, tenants/*, main, master) and a pre-deletion check in the while read -r br loop that skips any branch matching a denylist pattern before the age/commit check runs. This reduces the risk of silently deleting branches referenced by tenant branch: pins, but it is a naming-convention heuristic, not a true cross-reference against actual tenant YAML files (which live outside this workflow/file and were not available to inspect), so a tenant-pinned branch with a name outside these patterns would still be deletable. A complete fix would additionally require either (a) querying the tenant config source for in-use branch names and excluding them dynamically, or (b) converting the deletion into a dry-run/report-only step requiring manual confirmation, neither of which was implemented here to keep the change minimal and file-scoped.
🤖 Prompt for AI agents
In .github/workflows/cleanup.yml around line 38, review and complete this code-review fix: cleanup.yml stale-branch deletion has no dry-run or protected-default-branch exclusion beyond protected=false filter.
What the draft fix changed: In the "Delete branches with no commits for MAX_AGE_DAYS" step of the `stale-branches` job, added a `BRANCH_DENYLIST` env var (glob patterns for `release/*`, `tenants/*`, `main`, `master`) and a pre-deletion check in the `while read -r br` loop that skips any branch matching a denylist pattern before the age/commit check runs. This reduces the risk of silently deleting branches referenced by tenant `branch:` pins, but it is a naming-convention heuristic, not a true cross-reference against actual tenant YAML files (which live outside this workflow/file and were not available to inspect), so a tenant-pinned branch with a name outside these patterns would still be deletable. A complete fix would additionally require either (a) querying the tenant config source for in-use branch names and excluding them dynamically, or (b) converting the deletion into a dry-run/report-only step requiring manual confirmation, neither of which was implemented here to keep the change minimal and file-scoped.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 45 low — review closely — react 👍/👎 to teach the reviewer
| return; | ||
| } | ||
| let version = app.package_info().version.to_string(); | ||
| let subject = format!("user.{user_id}.installed-agent"); |
There was a problem hiding this comment.
🦩 🟠 publish_version_report retry loop truncated mid-expression in sample but subject/payload construction has no upper bound guard on user_id source
In publish_version_report (src-tauri/src/updater.rs), added a new helper is_valid_nats_subject_token that rejects empty strings and any user_id containing ., *, >, or whitespace, and inserted a validation check immediately after the user_id is extracted from the JWT claim (before the subject is formatted). If validation fails, the function logs a warning and returns early without publishing, preventing subject-string injection into the NATS hierarchy via a malformed/forged user_id claim. This is the smallest change that closes the injection vector described in the finding; it does not attempt broader JWT validation/auth hardening, which would be a separate, larger change outside this file's scope.
🤖 Prompt for AI agents
In src-tauri/src/updater.rs around line 392, review and complete this code-review fix: publish_version_report retry loop truncated mid-expression in sample but subject/payload construction has no upper bound guard on user_id source.
What the draft fix changed: In `publish_version_report` (src-tauri/src/updater.rs), added a new helper `is_valid_nats_subject_token` that rejects empty strings and any `user_id` containing `.`, `*`, `>`, or whitespace, and inserted a validation check immediately after the `user_id` is extracted from the JWT claim (before the `subject` is formatted). If validation fails, the function logs a warning and returns early without publishing, preventing subject-string injection into the NATS hierarchy via a malformed/forged `user_id` claim. This is the smallest change that closes the injection vector described in the finding; it does not attempt broader JWT validation/auth hardening, which would be a separate, larger change outside this file's scope.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| RUN_NUMBER: ${{ github.run_number }} | ||
| run: | | ||
| set -euo pipefail | ||
| if [[ -n "$LATEST" ]]; then |
There was a problem hiding this comment.
🦩 🟠 version.yml dispatch path silently accepts empty INPUT_VERSION and auto-bumps patch, but push path always resolves to 'latest' regardless of dispatch semantics — potential drift between two version-resolution algorithms
Extracted the duplicated IFS='.' read next-patch-bump arithmetic out of both the version_push and version_dispatch steps into a single new shared step next_patch (id: next_patch), which also validates that LATEST's components are numeric (previously only the dispatch path's chosen version was format-validated, not the parsed LATEST itself). version_push now consumes steps.next_patch.outputs.next_patch via env var NEXT_PATCH instead of recomputing cur_patch + 1 independently, and version_dispatch uses the same shared output for its next_patch candidate instead of recomputing it, while still computing next_major/next_minor locally since those weren't duplicated. This directly addresses the "two independent copies of bump logic can diverge" finding by making patch-bump arithmetic single-sourced. Risk: this is a workflow-structure change (new step, new step dependency ordering) rather than a pure one-line fix, so it should be reviewed carefully for GitHub Actions output-passing correctness (string outputs, job/step if conditions still guard push vs dispatch correctly) — I could not execute the workflow to confirm end-to-end behavior, so a full validation via CI run is recommended before merge.
🤖 Prompt for AI agents
In .github/workflows/version.yml around line 50, review and complete this code-review fix: version.yml dispatch path silently accepts empty INPUT_VERSION and auto-bumps patch, but push path always resolves to 'latest' regardless of dispatch semantics — potential drift between two version-resolution algorithms.
What the draft fix changed: Extracted the duplicated `IFS='.' read` next-patch-bump arithmetic out of both the `version_push` and `version_dispatch` steps into a single new shared step `next_patch` (id: `next_patch`), which also validates that `LATEST`'s components are numeric (previously only the dispatch path's chosen version was format-validated, not the parsed `LATEST` itself). `version_push` now consumes `steps.next_patch.outputs.next_patch` via env var `NEXT_PATCH` instead of recomputing `cur_patch + 1` independently, and `version_dispatch` uses the same shared output for its `next_patch` candidate instead of recomputing it, while still computing `next_major`/`next_minor` locally since those weren't duplicated. This directly addresses the "two independent copies of bump logic can diverge" finding by making patch-bump arithmetic single-sourced. Risk: this is a workflow-structure change (new step, new step dependency ordering) rather than a pure one-line fix, so it should be reviewed carefully for GitHub Actions output-passing correctness (string outputs, job/step `if` conditions still guard push vs dispatch correctly) — I could not execute the workflow to confirm end-to-end behavior, so a full validation via CI run is recommended before merge.
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
| /// nothing from AppKit but the one class it calls. | ||
| const DID_WAKE: &str = "NSWorkspaceDidWakeNotification"; | ||
|
|
||
| /// Guards against registering more than one observer per process: `observe` | ||
| /// intentionally leaks its token (see below), so a second call would leak a | ||
| /// second observer and duplicate wake refreshes. | ||
| static OBSERVED: AtomicBool = AtomicBool::new(false); | ||
|
|
||
| pub(crate) fn observe(app: AppHandle) { | ||
| if OBSERVED.swap(true, Ordering::SeqCst) { | ||
| log::warn!("[wake] observe() called more than once; ignoring duplicate registration"); | ||
| return; | ||
| } | ||
| let handler = RcBlock::new(move |_notification: NonNull<NSNotification>| { | ||
| // Before the nudge, not after: the nudge is what would otherwise rotate | ||
| // at resume+0, ahead of the wake watch's next tick. |
There was a problem hiding this comment.
🦩 🟠 macos_wake.rs intentionally leaks the NSWorkspace observer via mem::forget with no unregister path
Added a process-wide AtomicBool guard (OBSERVED) checked and set via swap at the top of observe() in src-tauri/src/macos_wake.rs. A second call now logs a warning and returns early instead of registering (and leaking) another NSWorkspace observer. This directly addresses the finding's concern about no runtime protection against repeated calls, following the same pattern cited in other files (autostart.rs, windows_activator.rs). Unverified: whether any existing call site actually calls observe() more than once (the finding notes this is only a theoretical risk), and whether silently no-op'ing on a second call is the desired behavior versus panicking or returning a Result — a complete fix might want the caller to be able to detect/handle the duplicate-call case rather than just logging.
🤖 Prompt for AI agents
In src-tauri/src/macos_wake.rs around line 48, review and complete this code-review fix: macos_wake.rs intentionally leaks the NSWorkspace observer via mem::forget with no unregister path.
What the draft fix changed: Added a process-wide `AtomicBool` guard (`OBSERVED`) checked and set via `swap` at the top of `observe()` in `src-tauri/src/macos_wake.rs`. A second call now logs a warning and returns early instead of registering (and leaking) another NSWorkspace observer. This directly addresses the finding's concern about no runtime protection against repeated calls, following the same pattern cited in other files (autostart.rs, windows_activator.rs). Unverified: whether any existing call site actually calls `observe()` more than once (the finding notes this is only a theoretical risk), and whether silently no-op'ing on a second call is the desired behavior versus panicking or returning a `Result` — a complete fix might want the caller to be able to detect/handle the duplicate-call case rather than just logging.
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
Bot
changed the title
Closes 5 review findings across 5 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.
.github/workflows/release.yml:286.github/workflows/cleanup.yml:38src-tauri/src/updater.rs:392.github/workflows/version.yml:50src-tauri/src/macos_wake.rs:48What 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:
e713e9e3-5fcf-4a8e-ac9a-2724454a32e8Merging 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-86akdypw4 Ad hoc sweep fixes across services (14 PRs)