Check osascript output instead of exit status - #42951
Merged
Merged
Conversation
Capture osascript output into a variable and compare it to "true" when checking if an app is running. Updated quit_application and quit_and_track_application to use app_running=$(osascript ...) and [[ "$app_running" != "true" ]] rather than relying on the command's exit status. This makes the running check more reliable across osascript behaviors and avoids depending on its exit code.
fleet-release
approved these changes
Apr 3, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #42951 +/- ##
==========================================
- Coverage 66.89% 66.88% -0.01%
==========================================
Files 2571 2571
Lines 206307 206307
Branches 9242 9243 +1
==========================================
- Hits 137999 137997 -2
+ Misses 55757 55755 -2
- Partials 12551 12555 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fleet-release
approved these changes
Apr 3, 2026
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the reliability of the “is the app running?” check in the generated Homebrew uninstall shell functions by validating osascript’s boolean output (true/false) instead of relying on its exit status.
Changes:
- Capture
osascriptoutput intoapp_runninginquit_applicationand return early unless it equals"true". - Apply the same output-based check in
quit_and_track_application, ensuring the tracking env var is set to0when the app isn’t running.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
iansltx
approved these changes
Apr 3, 2026
Contributor
|
@allenhouchins Guessing you'll take responsibility for merging this and the resulting script regen? Or do we want to regen scripts pre-merge and run at least a subset of them through QA? |
Member
Author
|
@iansltx I will get this merged and monitor script regen and FMA validation. I manually tested a few apps before and after this script change and it works as expected now. |
allenhouchins
added a commit
that referenced
this pull request
Jul 9, 2026
…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.
3 tasks
allenhouchins
added a commit
that referenced
this pull request
Jul 9, 2026
…pers (#49030) **Related issue:** Resolves #48639 # 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 The GitHub Desktop FMA uses a custom install script (`ee/maintained-apps/inputs/homebrew/scripts/github-desktop-install.sh`) that embeds its own copies of `quit_and_track_application` and `relaunch_application`. Those copies were frozen before two fixes landed in the generated helpers in `ee/maintained-apps/ingesters/homebrew/scripts.go`: - #42951 — check osascript **output** instead of exit status. `osascript -e '... is running'` exits 0 whether it prints `true` or `false`, so the stale `if ! osascript ...` guard never fired. The app was marked `APP_WAS_RUNNING=1` on **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 #48639. - #43842 — relaunch via `launchctl asuser ... open -b` as the console user instead of `osascript ... 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.json` via `go 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_application` functions on macOS against GitHub Desktop itself (`com.github.GitHubClient`): - **Fully quit (the bug scenario):** verified `is running` returns `false` and zero `GitHub Desktop.app` processes. Fixed functions set `APP_WAS_RUNNING=0` and the app stays closed. Running the old shipped check (`if ! osascript ...`) against the same state misclassifies the app as running (osascript exits 0 with output `false`) and would have relaunched it. - **Running:** quit succeeds, `APP_WAS_RUNNING=1`, app relaunches successfully afterward. - `bash -n` passes on the updated script. Note: hosts where the FMA was already added keep the baked `98ab6ed8` script 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. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved GitHub Desktop installation behavior on macOS so the app is more reliably closed and reopened after install. * Better handles login/session edge cases, helping ensure the app relaunches in the correct user’s desktop session. * Reduces failed or missed relaunches when the installer is run with elevated permissions. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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.
Capture osascript output into a variable and compare it to "true" when checking if an app is running. Updated quit_application and quit_and_track_application to use app_running=$(osascript ...) and [[ "$app_running" != "true" ]] rather than relying on the command's exit status. This makes the running check more reliable across osascript behaviors and avoids depending on its exit code.