Sync GitHub Desktop FMA install script with current quit/relaunch helpers - #49030
Merged
Conversation
…pers (#48639) The custom install script embedded pre-#42951/#43842 copies of quit_and_track_application and relaunch_application. The stale 'if ! osascript' check treats any non-erroring osascript call as "app is running" (osascript exits 0 whether it prints true or false), so the app was marked for relaunch on every install and launched after every patch, even from a fully-quit state. Replace both functions with the current scripts.go constants (output-based running check, launchctl-asuser relaunch, updated console-user guards) and regenerate the darwin manifest.
2 tasks
fleet-release
approved these changes
Jul 9, 2026
Contributor
Script Diff Resultsee/maintained-apps/outputs/github/darwin.json=== Install // 98ab6ed8 -> c91ea2b5 ===
--- /tmp/old.gcdJNb 2026-07-09 14:30:00.720352821 +0000
+++ /tmp/new.FZeXt9 2026-07-09 14:30:00.721352824 +0000
@@ -12,14 +12,16 @@
local timeout_duration=10
# check if the application is running
- if ! osascript -e "application id \"$bundle_id\" is running" 2>/dev/null; then
+ local app_running
+ app_running=$(osascript -e "application id \"$bundle_id\" is running" 2>/dev/null)
+ if [[ "$app_running" != "true" ]]; then
eval "export $var_name=0"
return
fi
local console_user
console_user=$(stat -f "%Su" /dev/console)
- if [[ $EUID -eq 0 && "$console_user" == "root" ]]; then
+ if [[ -z "$console_user" || "$console_user" == "root" || "$console_user" == "loginwindow" ]]; then
echo "Not logged into a non-root GUI; skipping quitting application ID '$bundle_id'."
eval "export $var_name=0"
return
@@ -63,15 +65,28 @@
local console_user
console_user=$(stat -f "%Su" /dev/console)
- if [[ $EUID -eq 0 && "$console_user" == "root" ]]; then
+ if [[ -z "$console_user" || "$console_user" == "root" || "$console_user" == "loginwindow" ]]; then
echo "Not logged into a non-root GUI; skipping relaunching application ID '$bundle_id'."
return
fi
echo "Relaunching application '$bundle_id'..."
- # Try to launch the application
- if osascript -e "tell application id \"$bundle_id\" to activate" >/dev/null 2>&1; then
+ # Launch the app in the logged-in user's GUI session. Apps launched by root
+ # won't register with the user's Dock/GUI, so run 'open' as the console user.
+ # Use 'launchctl asuser' to bootstrap into the console user's Mach namespace
+ # and GUI session — 'sudo -u' alone doesn't do this, which can cause
+ # LSOpenURLsWithRole() failures even when 'open' exits 0.
+ local open_status=0
+ if [[ $EUID -eq 0 ]]; then
+ local console_uid
+ console_uid=$(id -u "$console_user")
+ /bin/launchctl asuser "$console_uid" sudo -u "$console_user" open -b "$bundle_id" >/dev/null 2>&1 || open_status=$?
+ else
+ open -b "$bundle_id" >/dev/null 2>&1 || open_status=$?
+ fi
+
+ if [[ $open_status -eq 0 ]]; then
echo "Application '$bundle_id' relaunched successfully."
else
echo "Failed to relaunch application '$bundle_id'."
=== Uninstall Script (no changes) === |
fleet-release
approved these changes
Jul 9, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the GitHub Desktop Fleet-maintained app (FMA) install flow on macOS to use the latest shared “quit + track running state” and “relaunch” helper logic, and regenerates the published manifest so managed instances receive the updated script ref.
Changes:
- Update
quit_and_track_applicationto checkosascriptoutput ("true"/"false") instead of exit status. - Update
relaunch_applicationto relaunch vialaunchctl asuser … open -bin the console user’s GUI session, with expanded console-user guards. - Regenerate
ee/maintained-apps/outputs/github/darwin.jsonto point to the newinstall_script_ref.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ee/maintained-apps/outputs/github/darwin.json | Updates GitHub Desktop’s published manifest to reference the new install script content by script ref. |
| ee/maintained-apps/inputs/homebrew/scripts/github-desktop-install.sh | Syncs GitHub Desktop’s custom install script helpers to match the current shared Homebrew ingester helper implementations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
14
to
+17
| # check if the application is running | ||
| if ! osascript -e "application id \"$bundle_id\" is running" 2>/dev/null; then | ||
| local app_running | ||
| app_running=$(osascript -e "application id \"$bundle_id\" is running" 2>/dev/null) | ||
| if [[ "$app_running" != "true" ]]; then |
Contributor
WalkthroughThe GitHub Desktop Homebrew install script (github-desktop-install.sh) was updated to refine app lifecycle handling on macOS. The running-app check in quit_and_track_application now captures osascript's result into a variable and broadens skip-quitting conditions to cover empty, root, or loginwindow console users. relaunch_application now uses open -b instead of AppleScript activate, tracking success via open_status, and when running as root, uses launchctl asuser combined with sudo -u to relaunch within the console user's GUI session. The corresponding darwin.json manifest was updated to reference the new script content via an updated install_script_ref and refs entry. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ee/maintained-apps/inputs/homebrew/scripts/github-desktop-install.sh`:
- Around line 15-24: In the GitHub Desktop install script, the current running
check in the helper around application id and APP_WAS_RUNNING_* only verifies
that the app process exists, which still includes hidden/background sessions.
Update the logic to also confirm there is a visible window before exporting the
relaunch marker, and keep the existing console user handling intact. Add or
adjust the check in the same flow that uses app_running so hidden sessions do
not trigger the relaunch path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 288de736-ccab-4854-83bd-6ea91b427cfb
📒 Files selected for processing (2)
ee/maintained-apps/inputs/homebrew/scripts/github-desktop-install.shee/maintained-apps/outputs/github/darwin.json
Comment on lines
+15
to
+24
| local app_running | ||
| app_running=$(osascript -e "application id \"$bundle_id\" is running" 2>/dev/null) | ||
| if [[ "$app_running" != "true" ]]; then | ||
| eval "export $var_name=0" | ||
| return | ||
| fi | ||
|
|
||
| local console_user | ||
| console_user=$(stat -f "%Su" /dev/console) | ||
| if [[ $EUID -eq 0 && "$console_user" == "root" ]]; then | ||
| if [[ -z "$console_user" || "$console_user" == "root" || "$console_user" == "loginwindow" ]]; then |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'FILE: %s\n' ee/maintained-apps/inputs/homebrew/scripts/github-desktop-install.sh
wc -l ee/maintained-apps/inputs/homebrew/scripts/github-desktop-install.sh
sed -n '1,220p' ee/maintained-apps/inputs/homebrew/scripts/github-desktop-install.sh
printf '\nSEARCH APP_WAS_RUNNING / visible / hidden / window references:\n'
rg -n "APP_WAS_RUNNING|visible|hidden|window|osascript|bundle_id" ee/maintained-apps/inputs/homebrew/scripts/github-desktop-install.shRepository: fleetdm/fleet
Length of output: 5362
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Search for visible-window predicates and relaunch markers in maintained-app scripts:\n'
rg -n "window 1|visible window|visible|hidden|APP_WAS_RUNNING|relaunch" ee/maintained-apps -g '*.sh'
printf '\nPotentially related GitHub Desktop scripts or docs:\n'
rg -n "GitHub Desktop|GitHubClient|github-desktop" ee/maintained-apps -g '*'Repository: fleetdm/fleet
Length of output: 17511
Check for a visible window before exporting the relaunch marker. application id ... is running still matches hidden/background-only GitHub Desktop sessions, so APP_WAS_RUNNING_* gets set and the app is relaunched unnecessarily. Add coverage for the running-but-no-visible-window case.
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 17-17: eval is invoked on a variable, parameter expansion, or command-substitution result, which re-parses the value as shell code. If any part of that value is attacker-controlled (arguments, environment, file contents, network output), it allows arbitrary command execution. Do not eval dynamic data: invoke the command directly with proper quoting (e.g. "$cmd" "$arg"), use arrays for argument lists (cmd=(prog --flag "$value"); "${cmd[@]}"), or restrict input to a validated allowlist before running it.
Context: eval "export $var_name=0"
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(eval-on-variable-bash)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ee/maintained-apps/inputs/homebrew/scripts/github-desktop-install.sh` around
lines 15 - 24, In the GitHub Desktop install script, the current running check
in the helper around application id and APP_WAS_RUNNING_* only verifies that the
app process exists, which still includes hidden/background sessions. Update the
logic to also confirm there is a visible window before exporting the relaunch
marker, and keep the existing console user handling intact. Add or adjust the
check in the same flow that uses app_running so hidden sessions do not trigger
the relaunch path.
allenhouchins
deleted the
48639-github-desktop-fleet-maintained-app-relaunches-the-app-after-a-patch-even-when-it-had-no-open-window-macos
branch
July 9, 2026 14:36
3 tasks
allenhouchins
added a commit
that referenced
this pull request
Jul 9, 2026
**Related issue:** Resolves #48638, resolves #48225 # Checklist for submitter If some of the following don't apply, delete the relevant line. - [ ] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters. - [x] Timeouts are implemented and retries are limited to avoid infinite loops ## Testing - [x] QA'd all new/changed functionality manually ## Details Follow-up to #49030 (GitHub Desktop, #48639), which fixed one instance of this bug. An audit for the raw pattern found **15 more custom FMA scripts** carrying the same broken check that #42951 fixed in the generated helpers: gating on the **exit status** of `osascript -e 'application id "..." is running'`. osascript exits 0 whether it prints `true` or `false`, so a fully-quit app is misclassified as running whenever the bundle id resolves. Impact by script: - **Relaunch after every patch (user-visible — the filed bugs):** `zoom_install.sh` (#48638) and `google_chrome_install.sh` (#48225) set `*_WAS_RUNNING=true` unconditionally and reopen the app after `installer`, even when the user had nothing open. - **Broken check, no relaunch step (needless quit attempts, misleading logs):** install scripts for 1Password, Adobe CC, ExpressVPN, Grammarly, LogiTune, Microsoft Edge, P4V, Slack; uninstall scripts for Adobe CC, CleanMyMac, GPG Suite, Microsoft Word, P4V. Note `tell application id X to quit` against a not-running app can briefly launch it to deliver the quit event, so these aren't purely cosmetic either. The fix is the same one-line pattern everywhere, style-matched to each script (`local`/POSIX `[ ]`/top-level variants preserved): capture osascript output and compare it to `"true"`. No other behavior changed — this PR deliberately does not touch relaunch methods or console-user guards. Regenerated the 13 affected darwin manifests with `go run ./cmd/maintained-apps -slug <slug>`. All diffs are script-ref-only except `google-chrome/darwin.json`, which also picked up the legitimate upstream 150.0.7871.115 version bump during regeneration (the daily ingest cron would publish it tonight regardless). An unrelated `google-chrome/windows.json` winget bump was excluded. ## Manual QA Reproduced the bug live on macOS with the shipped Zoom script logic (ref `05e6a85c`) against a **fully-quit** Zoom (verified `is running` = `false`, zero processes): the exit-status check set `ZOOM_WAS_RUNNING=true` and the relaunch step launched Zoom — exactly the customer report, no background helpers needed. The corrected output-compare check on the same state correctly reported not running. Equivalent verification for the shared-helper variant was done against GitHub Desktop in #49030 (both the fully-quit and running→quit→relaunch paths). Verified for all 16 scripts: `bash -n` passes, no `if [!] osascript -e "application id ...` pattern remains anywhere under `inputs/homebrew/scripts/`, and every regenerated manifest ref carries the output-compare check. Remaining by-design behavior (unchanged): an app running with a dock icon but no visible window is genuinely running and will still be quit and relaunched; window-aware relaunching would be a separate enhancement. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved app detection before install/uninstall steps, reducing unnecessary quit attempts and making setup flows more reliable. * Updated several app install/uninstall workflows to better handle running apps, cleanup, and restart behavior. * Refined a few app package definitions to point to newer supported versions. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue: Resolves #48639
Checklist for submitter
If some of the following don't apply, delete the relevant line.
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Testing
Details
The GitHub Desktop FMA uses a custom install script (
ee/maintained-apps/inputs/homebrew/scripts/github-desktop-install.sh) that embeds its own copies ofquit_and_track_applicationandrelaunch_application. Those copies were frozen before two fixes landed in the generated helpers inee/maintained-apps/ingesters/homebrew/scripts.go:osascript -e '... is running'exits 0 whether it printstrueorfalse, so the staleif ! osascript ...guard never fired. The app was markedAPP_WAS_RUNNING=1on every install with a GUI user logged in and relaunched after every patch — even from a fully-quit state. This is the root cause of GitHub Desktop Fleet-maintained app relaunches the app after a patch even when it had no open window (macOS) #48639.launchctl asuser ... open -bas the console user instead ofosascript ... to activate(which is unreliable from a root context), plus the updated empty/root/loginwindow console-user guards.This PR replaces both embedded functions with the current scripts.go constants (verified byte-for-byte identical) and regenerates
ee/maintained-apps/outputs/github/darwin.jsonviago run ./cmd/maintained-apps -slug github. The manifest diff is script-ref-only (98ab6ed8→c91ea2b5); version and uninstall script are unchanged. The other five custom scripts (Docker Desktop, OpenVPN Connect, Webex, Max, Pd) already carry the updated helpers — GitHub Desktop was the only one missed.Manual QA
Tested the updated
quit_and_track_application/relaunch_applicationfunctions on macOS against GitHub Desktop itself (com.github.GitHubClient):is runningreturnsfalseand zeroGitHub Desktop.appprocesses. Fixed functions setAPP_WAS_RUNNING=0and the app stays closed. Running the old shipped check (if ! osascript ...) against the same state misclassifies the app as running (osascript exits 0 with outputfalse) and would have relaunched it.APP_WAS_RUNNING=1, app relaunches successfully afterward.bash -npasses on the updated script.Note: hosts where the FMA was already added keep the baked
98ab6ed8script until their instance refreshes the manifest. The by-design behavior "app running with dock icon but no visible window → relaunched with a window" is unchanged; window-aware relaunching would be a separate enhancement.Summary by CodeRabbit