Skip to content

Relaunch apps via open as console user - #43842

Merged
allenhouchins merged 2 commits into
mainfrom
allenhouchins-fma-fix
Apr 22, 2026
Merged

Relaunch apps via open as console user#43842
allenhouchins merged 2 commits into
mainfrom
allenhouchins-fma-fix

Conversation

@allenhouchins

@allenhouchins allenhouchins commented Apr 21, 2026

Copy link
Copy Markdown
Member

This pull request updates the application quit and relaunch logic in the Homebrew ingester scripts to more robustly detect valid GUI user sessions and improve how applications are relaunched after installation. The main improvements ensure that actions are only attempted when a real user is logged in, and that relaunching applications works reliably in the correct user context, especially when running as root.

User session validation:

  • The checks for a valid GUI session in quit_application, quit_and_track_application, and relaunch_application have been expanded to skip actions if the console user is empty, root, or loginwindow, preventing attempts to interact with the GUI when no real user is logged in. [1] [2] [3]

Application relaunch improvements:

  • The relaunch_application logic now uses launchctl asuser with sudo -u to launch the application in the correct user's GUI session, ensuring that the app appears in the user's Dock and GUI, even when the script runs as root. This replaces the previous approach of using osascript, which could fail in root contexts. [1] [2]
  • Additional comments were added to explain why these changes are necessary and how the new approach works. [1] [2]

Summary by CodeRabbit

  • Bug Fixes
    • More reliable app relaunch after installations, reducing failures when GUI apps are reinstalled.
    • Avoids attempting to quit or relaunch when no active console user is present (including the system login window), preventing unintended actions.
    • Uses a more robust method to launch apps in the user GUI session and reports success based on the launcher outcome for clearer results.

Ensure GUI apps are relaunched in the logged-in user's session instead of root. Update relaunch_application to skip when no non-root GUI user is present (empty/"root"/"loginwindow"), and use `sudo -u "$console_user" open -b <bundle_id>` when the installer runs as root so the app appears in the user's Dock/GUI. Remove the previous osascript activation attempt and add status handling and explanatory comments.
@codecov

codecov Bot commented Apr 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.92%. Comparing base (a060f3e) to head (272e653).
⚠️ Report is 40 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #43842      +/-   ##
==========================================
- Coverage   66.92%   66.92%   -0.01%     
==========================================
  Files        2603     2603              
  Lines      209044   209073      +29     
  Branches     9235     9235              
==========================================
+ Hits       139905   139913       +8     
- Misses      56392    56413      +21     
  Partials    12747    12747              
Flag Coverage Δ
backend 68.70% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@allenhouchins
allenhouchins marked this pull request as ready for review April 21, 2026 13:54
@allenhouchins
allenhouchins requested a review from a team as a code owner April 21, 2026 13:54
Copilot AI review requested due to automatic review settings April 21, 2026 13:54
fleet-release
fleet-release previously approved these changes Apr 21, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@coderabbitai

coderabbitai Bot commented Apr 21, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

The embedded shell functions in the Homebrew ingester were updated. The quit_application() and quit_and_track_application() skip conditions now also skip when console_user is empty or equals loginwindow, in addition to the existing EUID=0 && console_user=root check. The relaunch_application() implementation changed from using osascript ... to activate to launching the bundle with open -b <bundle_id>; when running as root it uses launchctl asuser <console_uid> sudo -u <console_user> open -b <bundle_id>. Success/failure is now based on open exit status.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description provides clear context about the changes, improvements made, and references to specific code locations, but lacks several required checklist items from the template (changes files, testing, database migrations, etc.). Complete the required checklist sections including whether a changes file was added, testing was performed, and other applicable requirements from the template.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: using the open command to relaunch apps in the console user's context instead of root.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch allenhouchins-fma-fix

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@ee/maintained-apps/ingesters/homebrew/scripts.go`:
- Around line 649-656: Replace the direct sudo -u "$console_user" open -b
"$bundle_id" call with the same launchctl asuser pattern used in send_signal so
the open runs inside the console user's Mach/GU I namespace; specifically, when
EUID is 0 invoke /bin/launchctl asuser "$console_uid" sudo -u "$console_user"
open -b "$bundle_id" (preserving the >/dev/null 2>&1 || open_status=$? handling
and the open_status variable), otherwise keep the non-root open path unchanged;
reference the existing send_signal use of /bin/launchctl asuser to mirror its
context-bootstrapping approach.
🪄 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: ab8a3fe6-3f00-475a-adfd-ed470012d327

📥 Commits

Reviewing files that changed from the base of the PR and between a8e90a8 and 1d6c482.

📒 Files selected for processing (1)
  • ee/maintained-apps/ingesters/homebrew/scripts.go

Comment thread ee/maintained-apps/ingesters/homebrew/scripts.go

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the Homebrew install-script generator so previously-running GUI apps are relaunched in the logged-in user’s session (rather than root), improving Dock/GUI visibility and reliability when installers run as root.

Changes:

  • Add explanatory comments describing why relaunch must happen in the console user’s GUI session.
  • Update relaunch_application to skip relaunch when no non-root GUI user is present (empty/root/loginwindow).
  • Replace osascript ... activate relaunching with open -b, using sudo -u "$console_user" when running as root and capturing exit status.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread ee/maintained-apps/ingesters/homebrew/scripts.go
Comment thread ee/maintained-apps/ingesters/homebrew/scripts.go
@allenhouchins
allenhouchins marked this pull request as draft April 21, 2026 16:11

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
ee/maintained-apps/ingesters/homebrew/scripts.go (1)

663-667: ⚠️ Potential issue | 🟡 Minor

open -b exit status is not a strong signal of successful GUI relaunch.

/usr/bin/open can return 0 even when LSOpenURLsWithRole() ultimately fails to surface the app in the Dock (e.g., stale Launch Services cache, bundle not yet re-registered after the move/install), and can return non-zero transiently right after a fresh install when the new bundle hasn't been indexed. Consider either (a) polling pgrep -f "$bundle_id" for a few seconds after open as a liveness check (mirroring the quit loop), or (b) softening the failure message to something like "relaunch requested" to avoid misleading log output. Not blocking — just flagging that the success/failure log may not always reflect reality.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ee/maintained-apps/ingesters/homebrew/scripts.go` around lines 663 - 667, The
current check uses open_status from the `open -b` call to declare
success/failure for relaunching `bundle_id`, which can be unreliable; update the
relaunch logic in the block that inspects `open_status` to either (a) poll for
the process after calling `open` by running `pgrep -f "$bundle_id"` in a short
loop for a few seconds (mirror the quit loop behavior) and treat presence as the
true success signal, or (b) change the echoed failure message around `bundle_id`
to a non-definitive string like "relaunch requested" so we don't claim a
successful GUI relaunch based solely on `open_status`; modify the code around
the `open_status` check and echo statements to implement one of these
approaches.
🧹 Nitpick comments (1)
ee/maintained-apps/ingesters/homebrew/scripts.go (1)

654-661: Guard against empty console_uid before invoking launchctl asuser.

If id -u "$console_user" fails (e.g., directory service hiccup, user removed between checks), console_uid will be empty and the call becomes /bin/launchctl asuser "" sudo -u "$console_user" open -b "$bundle_id", which will emit a confusing launchctl error rather than the intended "Failed to relaunch" message. The upstream non-empty check on console_user doesn't cover id failure.

🛡️ Suggested hardening
   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=$?
+    if [[ -z "$console_uid" ]]; then
+      echo "Could not resolve UID for console user '$console_user'; skipping relaunch."
+      return
+    fi
+    /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
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ee/maintained-apps/ingesters/homebrew/scripts.go` around lines 654 - 661, The
call to `/bin/launchctl asuser` can receive an empty `console_uid` if `id -u
"$console_user"` fails; update the block that computes `console_uid` and invokes
`/bin/launchctl asuser` to validate `console_uid` is non-empty (and preferably
numeric) before using it. If `console_uid` is empty/invalid, avoid calling
`launchctl asuser` and instead set `open_status` to a failure value (or fall
back to the non-root `open -b "$bundle_id"` path) and emit the existing "Failed
to relaunch" handling so a clear error is produced; refer to the `console_uid`,
`console_user`, and `/bin/launchctl asuser` usage in this section when making
the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@ee/maintained-apps/ingesters/homebrew/scripts.go`:
- Around line 663-667: The current check uses open_status from the `open -b`
call to declare success/failure for relaunching `bundle_id`, which can be
unreliable; update the relaunch logic in the block that inspects `open_status`
to either (a) poll for the process after calling `open` by running `pgrep -f
"$bundle_id"` in a short loop for a few seconds (mirror the quit loop behavior)
and treat presence as the true success signal, or (b) change the echoed failure
message around `bundle_id` to a non-definitive string like "relaunch requested"
so we don't claim a successful GUI relaunch based solely on `open_status`;
modify the code around the `open_status` check and echo statements to implement
one of these approaches.

---

Nitpick comments:
In `@ee/maintained-apps/ingesters/homebrew/scripts.go`:
- Around line 654-661: The call to `/bin/launchctl asuser` can receive an empty
`console_uid` if `id -u "$console_user"` fails; update the block that computes
`console_uid` and invokes `/bin/launchctl asuser` to validate `console_uid` is
non-empty (and preferably numeric) before using it. If `console_uid` is
empty/invalid, avoid calling `launchctl asuser` and instead set `open_status` to
a failure value (or fall back to the non-root `open -b "$bundle_id"` path) and
emit the existing "Failed to relaunch" handling so a clear error is produced;
refer to the `console_uid`, `console_user`, and `/bin/launchctl asuser` usage in
this section when making the change.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8ce7520b-d65c-47bf-90af-484e3a00b355

📥 Commits

Reviewing files that changed from the base of the PR and between 1d6c482 and 272e653.

📒 Files selected for processing (1)
  • ee/maintained-apps/ingesters/homebrew/scripts.go

@allenhouchins
allenhouchins merged commit f35fad2 into main Apr 22, 2026
48 of 49 checks passed
@allenhouchins
allenhouchins deleted the allenhouchins-fma-fix branch April 22, 2026 01:51
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.
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 -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants