Skip to content

fix(adhoc-sweep-fixes): CU-86akdypw4 5 review findings across 5 files #36

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
flamingo wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

fix(adhoc-sweep-fixes): CU-86akdypw4 5 review findings across 5 files #36

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
13a8b36
fix(adhoc-sweep-fixes): 5 review findings across 5 files
flamingo[bot] Sep 7, 2026
11ac035
fix(adhoc-sweep-fixes): 5 review findings across 5 files
flamingo[bot] Sep 7, 2026
f076d66
fix(adhoc-sweep-fixes): 5 review findings across 5 files
flamingo[bot] Sep 7, 2026
4c24acd
fix(adhoc-sweep-fixes): 5 review findings across 5 files
flamingo[bot] Sep 7, 2026
3799682
fix(adhoc-sweep-fixes): 5 review findings across 5 files
flamingo[bot] Sep 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions .github/workflows/cleanup.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,35 @@ jobs:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
REPO: ${{ github.repository }}
MAX_AGE_DAYS: "60"
# Comma-separated list of glob patterns (matched with bash's [[ == ]])
# that must never be auto-deleted, even if unprotected and stale.
# This guards branches referenced elsewhere (e.g. tenant YAML
# `branch:` pins per OPENFRAM-005-5/006-5) that are not marked
# "protected" in GitHub but must not be silently removed.
BRANCH_DENYLIST: "release/*,tenants/*,main,master"
run: |
set -o pipefail
CUTOFF=$(date -u -d "$MAX_AGE_DAYS days ago" +%Y-%m-%dT%H:%M:%SZ)
echo "Deleting branches with no commit since $CUTOFF"

IFS=',' read -r -a DENY_PATTERNS <<< "$BRANCH_DENYLIST"

gh api --paginate "repos/$REPO/branches?protected=false&per_page=100" \

@flamingo flamingo Bot Sep 7, 2026

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.

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

--jq '.[].name' |
while read -r br; do
skip=false
for pattern in "${DENY_PATTERNS[@]}"; do
if [[ "$br" == $pattern ]]; then
echo "Skipping $br (matches denylist pattern '$pattern')"
skip=true
break
fi
done
$skip && continue

last=$(gh api "repos/$REPO/commits/$br" --jq '.commit.committer.date')
[[ "$last" < "$CUTOFF" ]] || continue
echo "Deleting $br (last commit $last)"
gh api --silent -X DELETE "repos/$REPO/git/refs/heads/$br"
done

13 changes: 7 additions & 6 deletions .github/workflows/release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,12 +283,6 @@ jobs:
} > RELEASE_HEADER.md
cat RELEASE_HEADER.md

- name: Delete existing latest release

@flamingo flamingo Bot Sep 7, 2026

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.

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

if: ${{ github.event_name == 'push' }}
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
run: gh release delete latest --yes --cleanup-tag || true

- name: Create Release
uses: softprops/action-gh-release@v2
with:
Expand All @@ -300,3 +294,10 @@ jobs:
body_path: RELEASE_HEADER.md
files: |
final-artifacts/*

- name: Delete previous latest release
if: ${{ github.event_name == 'push' }}
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
run: gh release delete latest --yes --cleanup-tag || true

29 changes: 22 additions & 7 deletions .github/workflows/version.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,20 +39,33 @@ jobs:
echo "last published release: ${latest:-<none>}"
echo "latest=${latest}" >> "$GITHUB_OUTPUT"

- name: Resolve version (push)
id: version_push
if: github.event_name != 'workflow_dispatch'
- name: Compute next patch version
id: next_patch
env:
LATEST: ${{ steps.published.outputs.latest }}
RUN_NUMBER: ${{ github.run_number }}
run: |
set -euo pipefail
if [[ -n "$LATEST" ]]; then

@flamingo flamingo Bot Sep 7, 2026

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.

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

IFS='.' read -r cur_major cur_minor cur_patch <<< "$LATEST"
app_version="${cur_major}.${cur_minor}.$((cur_patch + 1))-dev.${RUN_NUMBER}"
if ! [[ "$cur_major" =~ ^[0-9]+$ && "$cur_minor" =~ ^[0-9]+$ && "$cur_patch" =~ ^[0-9]+$ ]]; then
echo "::error::Invalid published version format: ${LATEST} (expected x.y.z)"
exit 1
fi
next_patch="${cur_major}.${cur_minor}.$((cur_patch + 1))"
else
app_version="0.0.0-dev.${RUN_NUMBER}"
next_patch="0.0.0"
fi
echo "next_patch=${next_patch}" >> "$GITHUB_OUTPUT"

- name: Resolve version (push)
id: version_push
if: github.event_name != 'workflow_dispatch'
env:
NEXT_PATCH: ${{ steps.next_patch.outputs.next_patch }}
RUN_NUMBER: ${{ github.run_number }}
run: |
set -euo pipefail
app_version="${NEXT_PATCH}-dev.${RUN_NUMBER}"
echo "push event: version resolves to latest, app version ${app_version}"
echo "version=latest" >> "$GITHUB_OUTPUT"
echo "app_version=${app_version}" >> "$GITHUB_OUTPUT"
Expand All @@ -63,6 +76,7 @@ jobs:
env:
INPUT_VERSION: ${{ inputs.version }}
LATEST: ${{ steps.published.outputs.latest }}
NEXT_PATCH: ${{ steps.next_patch.outputs.next_patch }}
run: |
set -euo pipefail

Expand All @@ -85,7 +99,7 @@ jobs:
IFS='.' read -r cur_major cur_minor cur_patch <<< "$LATEST"
next_major="$((cur_major + 1)).0.0"
next_minor="${cur_major}.$((cur_minor + 1)).0"
next_patch="${cur_major}.${cur_minor}.$((cur_patch + 1))"
next_patch="$NEXT_PATCH"

if [ -z "$INPUT_VERSION" ]; then
version="$next_patch"
Expand All @@ -101,3 +115,4 @@ jobs:

echo "version=${version}" >> "$GITHUB_OUTPUT"
echo "app_version=${version}" >> "$GITHUB_OUTPUT"

10 changes: 10 additions & 0 deletions src-tauri/src/macos_wake.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
// refreshes the gate then refuses.

use std::ptr::NonNull;
use std::sync::atomic::{AtomicBool, Ordering};

use block2::RcBlock;
use objc2_app_kit::NSWorkspace;
Expand All @@ -26,7 +27,16 @@ use tauri::AppHandle;
/// 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.
Comment on lines 27 to 42

@flamingo flamingo Bot Sep 7, 2026

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.

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

Expand Down
17 changes: 17 additions & 0 deletions src-tauri/src/updater.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,19 @@ pub async fn update_apply_now(app: AppHandle, window: WebviewWindow) -> Result<(
}
}

/// NATS subjects are dot-delimited, with `*` and `>` reserved as wildcards, so a
/// `user_id` containing any of those could smuggle the message into a different
/// subject hierarchy than the literal `user.<id>.installed-agent` we intend.
/// The claim is expected to be an opaque server-issued identifier and should
/// never contain these characters; reject it outright rather than attempt to
/// escape it, since NATS subjects have no escaping mechanism.
fn is_valid_nats_subject_token(user_id: &str) -> bool {
!user_id.is_empty()
&& !user_id
.chars()
.any(|c| c == '.' || c == '*' || c == '>' || c.is_whitespace())
}

pub(crate) async fn publish_version_report(app: &AppHandle, client: &async_nats::Client) {
let Some(user_id) = tokens::load_tokens(app)
.access_token
Expand All @@ -388,6 +401,10 @@ pub(crate) async fn publish_version_report(app: &AppHandle, client: &async_nats:
log::debug!("[updater] no userId in token — skipping version report");
return;
};
if !is_valid_nats_subject_token(&user_id) {
log::warn!("[updater] userId claim is not a valid NATS subject token — skipping version report");
return;
}
let version = app.package_info().version.to_string();
let subject = format!("user.{user_id}.installed-agent");

@flamingo flamingo Bot Sep 7, 2026

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.

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

let payload = match serde_json::to_vec(&InstalledAgentReport {
Expand Down
Loading