Revert "Support default pkg install script when cask lacks pkg artifact and URL override is used" - #46574
Conversation
…ct and U…" This reverts commit 9f46c84.
![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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #46574 +/- ##
==========================================
+ Coverage 66.83% 66.86% +0.03%
==========================================
Files 2809 2809
Lines 223561 223740 +179
Branches 11313 11313
==========================================
+ Hits 149417 149607 +190
+ Misses 60599 60548 -51
- Partials 13545 13585 +40
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:
|
There was a problem hiding this comment.
Pull request overview
This PR reverts #45893, which had added support for generating a default pkg install script when a Homebrew cask lacks a pkg artifact but has a URL override pointing at a pkg. The revert removes the dynamic generation logic and instead reintroduces static per-app install scripts (for Slack, 1Password, and Zoom) referenced via install_script_path in each app's input JSON.
Changes:
- Revert the
caskHasPkgArtifact/InstallPkgFromInstallerPath/RelaunchAndPropagateInstallStatuslogic inscripts.goand the corresponding tests inscripts_test.go. - Revert the
quit_application/quit_and_track_applicationhelpers to their pre-#45893 form (drop thelaunchctl asuser+sudo -ubootstrap when running as root). - Reintroduce static install scripts for Slack, 1Password, and Zoom and wire them up via
install_script_pathin the respective input JSONs.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| ee/maintained-apps/ingesters/homebrew/scripts.go | Reverts dynamic pkg-without-pkg-artifact handling and rolls back the root-aware osascript bootstrap in the quit helpers. |
| ee/maintained-apps/ingesters/homebrew/scripts_test.go | Removes tests that covered the reverted dynamic pkg-install behavior. |
| ee/maintained-apps/inputs/homebrew/slack.json | Points Slack at a static install script. |
| ee/maintained-apps/inputs/homebrew/1password.json | Points 1Password at a static install script. |
| ee/maintained-apps/inputs/homebrew/zoom.json | Points Zoom at a static install script. |
| ee/maintained-apps/inputs/homebrew/scripts/slack_install.sh | Restores Slack's static install script. |
| ee/maintained-apps/inputs/homebrew/scripts/1password_install.sh | Restores 1Password's static install script. |
| ee/maintained-apps/inputs/homebrew/scripts/zoom_install.sh | Restores Zoom's static install script (with restart-if-running behavior). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughThis PR refactors Homebrew pkg installation by removing centralized script-generation methods ( Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/ingesters/homebrew/scripts.go`:
- Around line 72-73: The installer exit status is being masked by the subsequent
relaunch_application call; capture the exit code immediately after the installer
command emitted by scriptBuilder.InstallPkg (store it in a variable like
installer_exit via InstallPkg or by appending a capture right after the install
invocation), then call relaunch_application '%s' (app.UniqueIdentifier) as now,
and finally exit or return using that stored installer_exit (e.g. exit
$installer_exit or test and exit non-zero) so the original installer failure is
preserved.
In `@ee/maintained-apps/inputs/homebrew/scripts/1password_install.sh`:
- Around line 7-10: The osascript check uses exit status instead of stdout so a
returned "false" still exits 0; capture the output of the osascript call that
checks application id "$bundle_id" is running and test its stdout string (e.g.,
compare to "true") before deciding to return/continue; update the invocation
referenced around the bundle_id variable and the osascript check to assign the
command output to a variable and explicitly test for "true" (or use a string
match) instead of relying on the command's exit code.
In `@ee/maintained-apps/inputs/homebrew/scripts/zoom_install.sh`:
- Around line 51-54: The code currently tests osascript's exit status instead of
the AppleScript boolean; change the check to capture osascript output and test
for the literal "true". For example, run running=$(osascript -e 'application id
"us.zoom.xos" is running' 2>/dev/null) and then if [ "$running" = "true" ]; then
set ZOOM_WAS_RUNNING=true and call quit_application 'us.zoom.xos'
"$CONSOLE_USER"; otherwise leave ZOOM_WAS_RUNNING unset/false. Ensure the
comparison is exact (string equality) and output is properly quoted.
- Around line 37-39: restart_zoom() currently uses sudo -u "$console_user" open
-a "zoom.us" which can fail to target the logged-in GUI/Mach session; change it
to use the same pattern as relaunch_application() by invoking /bin/launchctl
asuser "$console_uid" sudo -u "$console_user" open -a "zoom.us" so the restart
happens in the console user's GUI session, referencing the existing console_uid
and console_user variables and matching relaunch_application() behavior.
- Around line 56-61: The installer exit status is lost when restart_zoom runs;
capture the exit code immediately after calling installer (reference the
installer invocation and variable INSTALLER_PATH), e.g., save "$?" into a local
variable like INSTALLER_EXIT, then perform the conditional restart based on
ZOOM_WAS_RUNNING and call restart_zoom "$CONSOLE_USER" || true, and finally exit
or return using the saved INSTALLER_EXIT so the script returns the install
result rather than the restart result.
🪄 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: 9b8ebe57-c38b-447e-a274-26361e755f64
📒 Files selected for processing (8)
ee/maintained-apps/ingesters/homebrew/scripts.goee/maintained-apps/ingesters/homebrew/scripts_test.goee/maintained-apps/inputs/homebrew/1password.jsonee/maintained-apps/inputs/homebrew/scripts/1password_install.shee/maintained-apps/inputs/homebrew/scripts/slack_install.shee/maintained-apps/inputs/homebrew/scripts/zoom_install.shee/maintained-apps/inputs/homebrew/slack.jsonee/maintained-apps/inputs/homebrew/zoom.json
💤 Files with no reviewable changes (1)
- ee/maintained-apps/ingesters/homebrew/scripts_test.go
| // Relaunch the app if it was running before installation | ||
| sb.Writef("relaunch_application '%s'", app.UniqueIdentifier) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verifies the shell behavior this change relies on: the last command wins.
set -euo pipefail
bash -lc 'false; true'
echo "exit after false; true => $?" # expected: 0
bash -lc 'false; status=$?; true; exit "$status"'
echo "exit after capturing status => $?" # expected: 1Repository: fleetdm/fleet
Length of output: 301
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the relevant code region in the repo
FILE="ee/maintained-apps/ingesters/homebrew/scripts.go"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 2
fi
echo "---- $FILE (around lines 60-90) ----"
nl -ba "$FILE" | sed -n '60,90p'
# 2) Re-check exit-status behavior without set -e terminating the script early
echo "---- exit status experiments (no set -e) ----"
set +e
bash -lc 'false; true'
echo "exit after: false; true => $?" # expect 0
bash -lc 'false; status=$?; true; exit "$status"'
echo "exit after: capture then exit $status => $?" # expect 1
# 3) Minimal model: if installer fails but a later command succeeds, last command wins
bash -lc 'false; installer_status=$?; true; echo installer_status=$installer_status; exit 0'
echo "model: installer fails then last command succeeds => $?" # expect 0
bash -lc 'false; installer_status=$?; true; exit "$installer_status"'
echo "model: installer fails but we exit with captured status => $?" # expect 1Repository: fleetdm/fleet
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="ee/maintained-apps/ingesters/homebrew/scripts.go"
test -f "$FILE" || { echo "Missing file: $FILE" >&2; exit 2; }
echo "---- $FILE (around lines 60-90) ----"
awk 'NR>=60 && NR<=90 { printf "%d\t%s\n", NR, $0 }' "$FILE"
echo "---- exit status experiments (no set -e) ----"
set +e
bash -lc 'false; true'
echo "exit after false; true => $?" # expect 0
bash -lc 'false; status=$?; true; exit "$status"'
echo "exit after capture then exit $status => $?" # expect 1
bash -lc 'false; installer_status=$?; true; echo installer_status=$installer_status; exit 0'
echo "model: installer fails then last command succeeds => $?" # expect 0
bash -lc 'false; installer_status=$?; true; exit "$installer_status"'
echo "model: installer fails but we exit with captured status => $?" # expect 1Repository: fleetdm/fleet
Length of output: 1530
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="ee/maintained-apps/ingesters/homebrew/scripts.go"
test -f "$FILE"
echo "---- show more around InstallPkg call sites ----"
awk 'NR>=1 && NR<=140 {print}' "$FILE" | sed -n '90,140p'
echo "---- find InstallPkg implementation in repo ----"
rg -n "func \\(.*\\) InstallPkg\\(" -S . || true
echo "---- exit status experiments (clean) ----"
set +e
set +u
bash -lc 'false; true'
echo "exit after false; true => $?" # expect 0
bash -lc 'false; status=$?; true; exit $status'
echo "exit after capture then exit $status => $?" # expect 1
bash -lc 'false; installer_status=$?; true; exit 0'
echo "model: installer fails then last command succeeds => $?" # expect 0
bash -lc 'false; installer_status=$?; true; exit $installer_status'
echo "model: installer fails but we exit with captured status => $?" # expect 1Repository: fleetdm/fleet
Length of output: 2592
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="ee/maintained-apps/ingesters/homebrew/scripts.go"
test -f "$FILE"
echo "---- scriptBuilder.InstallPkg implementation (around line 380-460) ----"
awk 'NR>=360 && NR<=460 { printf "%d\t%s\n", NR, $0 }' "$FILE"
echo "---- search for relaunch_application and installer generation ----"
rg -n "relaunch_application|InstallPkg\\(|installer " "$FILE"
echo "---- show helpers: Write / Writef / AddVariable / String() (if nearby) ----"
rg -n "func \\(s \\*scriptBuilder\\) (Write|Writef|AddVariable|String)\\(" "$FILE"Repository: fleetdm/fleet
Length of output: 4779
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="ee/maintained-apps/ingesters/homebrew/scripts.go"
echo "---- search for set -e / pipefail in this file ----"
rg -n "set -e|set -o|pipefail" "$FILE" || true
echo "---- show top of String() where header is written ----"
awk 'NR>=430 && NR<=520 { printf "%d\t%s\n", NR, $0 }' "$FILE"
echo "---- show relaunchApplicationFunc definition ----"
awk 'NR>=600 && NR<=700 { printf "%d\t%s\n", NR, $0 }' "$FILE"
echo "---- show uninstall/install script builder ordering around 50-90 for pkg relaunch block ----"
awk 'NR>=1 && NR<=120 { if(NR>=50) { printf "%d\t%s\n", NR, $0 } }' "$FILE"Repository: fleetdm/fleet
Length of output: 9842
Preserve installer exit status before relaunching.
scriptBuilder.InstallPkg writes sudo installer ... without any set -e/error propagation, and the script then immediately calls relaunch_application ..., which can return success—masking a failed installer.
Suggested fix
// Relaunch the app if it was running before installation
+ sb.Write("install_status=$?")
sb.Writef("relaunch_application '%s'", app.UniqueIdentifier)
+ sb.Write("exit $install_status")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Relaunch the app if it was running before installation | |
| sb.Writef("relaunch_application '%s'", app.UniqueIdentifier) | |
| // Relaunch the app if it was running before installation | |
| sb.Write("install_status=$?") | |
| sb.Writef("relaunch_application '%s'", app.UniqueIdentifier) | |
| sb.Write("exit $install_status") |
🤖 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/ingesters/homebrew/scripts.go` around lines 72 - 73, The
installer exit status is being masked by the subsequent relaunch_application
call; capture the exit code immediately after the installer command emitted by
scriptBuilder.InstallPkg (store it in a variable like installer_exit via
InstallPkg or by appending a capture right after the install invocation), then
call relaunch_application '%s' (app.UniqueIdentifier) as now, and finally exit
or return using that stored installer_exit (e.g. exit $installer_exit or test
and exit non-zero) so the original installer failure is preserved.
| # check if the application is running | ||
| if ! osascript -e "application id \"$bundle_id\" is running" 2>/dev/null; then | ||
| return | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verifies the bash behavior involved here: `if ! cmd` inspects exit status, not stdout.
set -euo pipefail
if ! printf 'false\n' >/dev/null; then
echo "returned"
else
echo "continued"
fi
# expected: continuedRepository: fleetdm/fleet
Length of output: 67
🌐 Web query:
osascript exit status when AppleScript returns false (boolean) stdout "false" exit code 0
💡 Result:
When the AppleScript you run with osascript evaluates to the boolean false (and does not throw an error), osascript exits with status 0; the “false” shows up in stdout as the returned value, not as an exit code. [1][2] If you need a non-zero exit status based on that boolean, you must explicitly signal an error from AppleScript (e.g., using error number...), or have a wrapper shell script map stdout to an exit code. osascript’s exit status otherwise only reflects whether osascript itself hit an error, not the truthiness of the returned value. [1][2][3] Example patterns: 1) Treat returned false as a shell failure (wrapper approach): - osascript prints “false” to stdout when the script returns false; in bash you can do: osascript -e 'return false' | grep -qx false && exit 1 (or invert the logic). [1] 2) Convert failure into osascript error (error number approach): - Use AppleScript error number <N>; then osascript will exit non-zero, and you can capture the error message/number from stderr/stdout depending on your flags. By default osascript prints script errors to stderr. [3][4]
Citations:
- 1: https://stackoverflow.com/questions/78680599/how-to-return-a-status-code-from-a-script-executed-using-osascript
- 2: https://discussions.apple.com/thread/819796
- 3: https://stackoverflow.com/questions/54668945/applescript-forward-error-number-to-osascript-as-exit-status
- 4: https://www.manpagez.com/man/1/osascript/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,60p' ee/maintained-apps/inputs/homebrew/scripts/1password_install.shRepository: fleetdm/fleet
Length of output: 1164
Fix app running-state check to evaluate osascript stdout (true/false), not exit status.
if ! osascript ... is running only returns when osascript itself errors; when the app isn’t running AppleScript returns false, which still exits 0—so the quit flow continues.
Suggested fix
- 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
return
fi🤖 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/1password_install.sh` around lines
7 - 10, The osascript check uses exit status instead of stdout so a returned
"false" still exits 0; capture the output of the osascript call that checks
application id "$bundle_id" is running and test its stdout string (e.g., compare
to "true") before deciding to return/continue; update the invocation referenced
around the bundle_id variable and the osascript check to assign the command
output to a variable and explicitly test for "true" (or use a string match)
instead of relying on the command's exit code.
| if [[ -n "$console_user" && "$console_user" != "root" ]]; then | ||
| echo "Restarting Zoom for user: $console_user" | ||
| sudo -u "$console_user" open -a "zoom.us" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verifies the mismatch against the shared relaunch helper in this PR.
set -euo pipefail
sed -n '631,663p' ee/maintained-apps/ingesters/homebrew/scripts.go
echo '---'
sed -n '34,43p' ee/maintained-apps/inputs/homebrew/scripts/zoom_install.shRepository: fleetdm/fleet
Length of output: 1684
Use launchctl asuser for Zoom restarts to relaunch in the logged-in GUI session.
restart_zoom() currently does only sudo -u "$console_user" open -a "zoom.us", but the shared relaunch_application() helper explicitly uses /bin/launchctl asuser "$console_uid" sudo -u "$console_user" open ... because sudo -u ... open alone can miss the console user’s GUI/Mach session.
Suggested fix
if [[ -n "$console_user" && "$console_user" != "root" ]]; then
echo "Restarting Zoom for user: $console_user"
- sudo -u "$console_user" open -a "zoom.us"
+ local console_uid
+ console_uid=$(id -u "$console_user")
+ /bin/launchctl asuser "$console_uid" sudo -u "$console_user" open -a "zoom.us"
else📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [[ -n "$console_user" && "$console_user" != "root" ]]; then | |
| echo "Restarting Zoom for user: $console_user" | |
| sudo -u "$console_user" open -a "zoom.us" | |
| if [[ -n "$console_user" && "$console_user" != "root" ]]; then | |
| echo "Restarting Zoom for user: $console_user" | |
| local console_uid | |
| console_uid=$(id -u "$console_user") | |
| /bin/launchctl asuser "$console_uid" sudo -u "$console_user" open -a "zoom.us" |
🤖 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/zoom_install.sh` around lines 37 -
39, restart_zoom() currently uses sudo -u "$console_user" open -a "zoom.us"
which can fail to target the logged-in GUI/Mach session; change it to use the
same pattern as relaunch_application() by invoking /bin/launchctl asuser
"$console_uid" sudo -u "$console_user" open -a "zoom.us" so the restart happens
in the console user's GUI session, referencing the existing console_uid and
console_user variables and matching relaunch_application() behavior.
| if osascript -e "application id \"us.zoom.xos\" is running" 2>/dev/null; then | ||
| ZOOM_WAS_RUNNING=true | ||
| quit_application 'us.zoom.xos' "$CONSOLE_USER" | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verifies the bash behavior involved here: the `if cmd` condition uses exit status.
set -euo pipefail
if printf 'false\n' >/dev/null; then
echo "branch taken"
else
echo "branch skipped"
fi
# expected: branch takenRepository: fleetdm/fleet
Length of output: 70
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print the relevant section
FILE="ee/maintained-apps/inputs/homebrew/scripts/zoom_install.sh"
echo "== File = $FILE =="
nl -ba "$FILE" | sed -n '35,90p'
# Find all uses of ZOOM_WAS_RUNNING in the repo
echo
echo "== Uses of ZOOM_WAS_RUNNING =="
rg -n "ZOOM_WAS_RUNNING" -S .Repository: fleetdm/fleet
Length of output: 190
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="ee/maintained-apps/inputs/homebrew/scripts/zoom_install.sh"
echo "== File = $FILE =="
awk 'NR>=35 && NR<=90 {printf "%d\t%s\n", NR, $0}' "$FILE"
echo
echo "== Uses of ZOOM_WAS_RUNNING =="
rg -n "ZOOM_WAS_RUNNING" -S .Repository: fleetdm/fleet
Length of output: 3221
🌐 Web query:
osascript exit status when AppleScript expression returns true/false
💡 Result:
The exit status of the osascript command is not determined by the boolean value (true or false) returned by your AppleScript expression [1][2][3]. When you run a script using osascript, the exit status of the command reflects whether osascript itself executed successfully [1]. If the script runs without encountering an AppleScript-level error (via an error statement), osascript will exit with a status of 0, regardless of the logical result of the script [1][2]. Even if your script ends with return true or return false, the calling shell will receive an exit status of 0 [1][2]. To pass a custom exit code from AppleScript to your shell, you must explicitly use the AppleScript error statement [1][2][3]: error "An error occurred" number 123 When this statement is executed, osascript will terminate with an exit status of 1 (a general error) by default, because it treats any unhandled script error as a failure [1][2][3]. It does not natively forward the custom error number (e.g., 123) as the process exit status [1][3]. To handle this in your shell script: 1. Use the -s o option with osascript to redirect script errors to standard output (stdout) instead of standard error (stderr) [4][5][6]. 2. Parse the output or error number from the command output in your shell script [2][3][5]. Example pattern: # Run the script and capture both output and errors result=$(osascript -s o -e 'return true' 2>&1) # You can then check the script's output or the exit code of the osascript command itself [3][5] For production workflows, a common approach is to have the AppleScript output a specific string or numeric value to stdout upon success or failure, which the shell script then captures and evaluates to set its own exit status [2][3].
Citations:
- 1: https://stackoverflow.com/questions/78680599/how-to-return-a-status-code-from-a-script-executed-using-osascript
- 2: https://discussions.apple.com/thread/819796
- 3: https://stackoverflow.com/questions/54668945/applescript-forward-error-number-to-osascript-as-exit-status
- 4: https://ss64.com/mac/osascript.html
- 5: https://apple.stackexchange.com/questions/353040/how-to-get-error-status-back-to-bash-from-osascript
- 6: https://www.unix.com/man-page/osx/1/OSASCRIPT/
Fix ZOOM_WAS_RUNNING detection: use the AppleScript boolean result, not osascript exit status (ee/maintained-apps/inputs/homebrew/scripts/zoom_install.sh ~51-54)
if osascript ...; then tests the exit code of osascript (success is exit 0 even when the AppleScript expression evaluates to false), so ZOOM_WAS_RUNNING can be set to true and Zoom restarted even when it was closed.
Suggested fix
# Check if Zoom is running
ZOOM_WAS_RUNNING=false
-if osascript -e "application id \"us.zoom.xos\" is running" 2>/dev/null; then
+if [[ "$(osascript -e "application id \"us.zoom.xos\" is running" 2>/dev/null)" == "true" ]]; then
ZOOM_WAS_RUNNING=true
quit_application 'us.zoom.xos' "$CONSOLE_USER"
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if osascript -e "application id \"us.zoom.xos\" is running" 2>/dev/null; then | |
| ZOOM_WAS_RUNNING=true | |
| quit_application 'us.zoom.xos' "$CONSOLE_USER" | |
| fi | |
| # Check if Zoom is running | |
| ZOOM_WAS_RUNNING=false | |
| if [[ "$(osascript -e "application id \"us.zoom.xos\" is running" 2>/dev/null)" == "true" ]]; then | |
| ZOOM_WAS_RUNNING=true | |
| quit_application 'us.zoom.xos' "$CONSOLE_USER" | |
| fi |
🤖 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/zoom_install.sh` around lines 51 -
54, The code currently tests osascript's exit status instead of the AppleScript
boolean; change the check to capture osascript output and test for the literal
"true". For example, run running=$(osascript -e 'application id "us.zoom.xos" is
running' 2>/dev/null) and then if [ "$running" = "true" ]; then set
ZOOM_WAS_RUNNING=true and call quit_application 'us.zoom.xos' "$CONSOLE_USER";
otherwise leave ZOOM_WAS_RUNNING unset/false. Ensure the comparison is exact
(string equality) and output is properly quoted.
| installer -pkg "$INSTALLER_PATH" -target / | ||
|
|
||
| # Restart Zoom if it was running before installation | ||
| if [[ "$ZOOM_WAS_RUNNING" == "true" ]]; then | ||
| sleep 2 | ||
| restart_zoom "$CONSOLE_USER" || true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verifies the shell behavior this change relies on: the last command wins.
set -euo pipefail
bash -lc 'false; true'
echo "exit after false; true => $?" # expected: 0
bash -lc 'false; status=$?; true; exit "$status"'
echo "exit after capturing status => $?" # expected: 1Repository: fleetdm/fleet
Length of output: 301
Preserve installer exit status before restarting Zoom
If installer fails and ZOOM_WAS_RUNNING is true, the last command becomes restart_zoom "$CONSOLE_USER" || true (forces success), so the script’s exit code reflects the restart result instead of the install failure.
Suggested fix
installer -pkg "$INSTALLER_PATH" -target /
+INSTALL_STATUS=$?
# Restart Zoom if it was running before installation
if [[ "$ZOOM_WAS_RUNNING" == "true" ]]; then
sleep 2
restart_zoom "$CONSOLE_USER" || true
fi
+
+exit "$INSTALL_STATUS"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| installer -pkg "$INSTALLER_PATH" -target / | |
| # Restart Zoom if it was running before installation | |
| if [[ "$ZOOM_WAS_RUNNING" == "true" ]]; then | |
| sleep 2 | |
| restart_zoom "$CONSOLE_USER" || true | |
| installer -pkg "$INSTALLER_PATH" -target / | |
| INSTALL_STATUS=$? | |
| # Restart Zoom if it was running before installation | |
| if [[ "$ZOOM_WAS_RUNNING" == "true" ]]; then | |
| sleep 2 | |
| restart_zoom "$CONSOLE_USER" || true | |
| fi | |
| exit "$INSTALL_STATUS" |
🤖 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/zoom_install.sh` around lines 56 -
61, The installer exit status is lost when restart_zoom runs; capture the exit
code immediately after calling installer (reference the installer invocation and
variable INSTALLER_PATH), e.g., save "$?" into a local variable like
INSTALLER_EXIT, then perform the conditional restart based on ZOOM_WAS_RUNNING
and call restart_zoom "$CONSOLE_USER" || true, and finally exit or return using
the saved INSTALLER_EXIT so the script returns the install result rather than
the restart result.
Reverts #45893
Summary by CodeRabbit