Add MacOS Compatibility

[dingyifei]

The commits are mostly generated by Claude Code. I've reviewed most parts of it

This is a minimal implementation that enable MacOS to install winapps using homebrew and run winapps-setup and winapps. A few wrappers were made to make winapps compatible with macos, such as waNotify. Due to limitations on MacOS, most features utilizing waNotify are not compatible with macOS yet

Since xquartz + xfreerdp have been terrible (glitching, black screen,...) on newer macOS, xfreerdp is only used for the winapps-setup to run app detection script. For connecting, Windows App is used with .rdp for app mode. It appears to work well (without dock icons unfortunately, it's a limitation). However, there seems to be some difficulties in disconnecting the client from host after all the windows are closed.

Features such as using winapp <app_name> <file_path> are tested and works on my machine.

note that xquartz is a homebrew cask, so it needs to be manually installed and can not be a prereq of the formula.

Closes #35

[dingyifei]

issue #35

[oskardotglobal]

This is certainly an interesting PR. I did not know that the macOS client supported RemoteApp. However, in the current state I would not be willing to merge this. The following issues will need to be addressed:

1. Currently, the commits are not signed off; see Checks -> DCO
2. This PR contains unrelated changes and fixes; either make seperate PRs for these or split them into commits
3. The docs read like AI-generated texts, scrap them and ensure their style matches the other documentation.

Also, thank you for disclosing this PR was AI-generated.

[oskardotglobal]

does this have to be semver or can we use something like 0-unstable (which is what we use for the Nix packages)

Also, any chance we could make this compatible with brew on Linux?

[dingyifei]

Based on my understanding 0-unstable might cause problems updating.

I don't see a reason it'd be incompatible with brew on linux if we remove the macos requirement and change post install message to inform the user to run setup? but probably needs some testing

[oskardotglobal]

@CodeRabbit review

[coderabbitai]

Actionable comments posted: 11

Caution

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

⚠️ Outside diff range comments (2)

bin/winapps (2)

22-30: ⚠️ Potential issue | 🔴 Critical

BREW_APP_PATH references SCRIPT_DIR_PATH before it is defined — it will always be "/../share/winapps".

Line 24 uses SCRIPT_DIR_PATH which is not assigned until Line 30. Since both are readonly, BREW_APP_PATH is permanently set to "/../share/winapps". This breaks the Homebrew app info lookups at Lines 702 and 779.

Move BREW_APP_PATH after SCRIPT_DIR_PATH, or remove the readonly and compute it lazily.

🐛 Proposed fix — reorder declarations

 # PATHS
 readonly APPDATA_PATH="${HOME}/.local/share/winapps"
 readonly SYS_APP_PATH="/usr/local/share/winapps"
-readonly BREW_APP_PATH="${SCRIPT_DIR_PATH}/../share/winapps"
 readonly LASTRUN_PATH="${APPDATA_PATH}/lastrun"
 readonly LOG_PATH="${APPDATA_PATH}/winapps.log"
 readonly CONFIG_PATH="${HOME}/.config/winapps/winapps.conf"
 readonly COMPOSE_PATH="${HOME}/.config/winapps/compose.yaml"
 # shellcheck disable=SC2155 # Silence warnings regarding masking return values through simultaneous declaration and assignment.
 readonly SCRIPT_DIR_PATH="$(cd "$(dirname "${BASH_SOURCE[0]}")" &>/dev/null && pwd)"
+readonly BREW_APP_PATH="${SCRIPT_DIR_PATH}/../share/winapps"
 readonly SLEEP_DETECT_PATH="${APPDATA_PATH}/last_activity"
 readonly SLEEP_MARKER="${APPDATA_PATH}/sleep_marker"

---

34-41: ⚠️ Potential issue | 🟠 Major

RDP_PORT is readonly here but overridable via config in setup.sh.

setup.sh Line 86 declares RDP_PORT=3389 without readonly, allowing the config file to override it. But here in bin/winapps Line 40, readonly RDP_PORT=3389 prevents any config override. Users who set a custom RDP_PORT in winapps.conf will find it works during setup but fails at runtime. Either make it overridable here too (assign before waLoadConfig, drop readonly), or document that RDP_PORT is not configurable at runtime.

Proposed fix

-readonly RDP_PORT=3389
+RDP_PORT=3389

🤖 Fix all issues with AI agents

In `@bin/winapps`:
- Around line 176-186: The waNotify function interpolates TITLE and BODY
directly into osascript which breaks when those strings contain double quotes or
backslashes; update waNotify to escape AppleScript-special characters before
calling osascript (e.g., replace backslashes "\" with "\\" and double quotes """
with "\""), use the escaped TITLE and BODY in the osascript -e invocation, and
keep the existing 2>/dev/null || echo fallback; refer to the waNotify function
and the TITLE/BODY variables when making the change so all macOS notification
calls are safe.
- Around line 275-284: VOL_NAME extracted from diskutil can contain double
quotes which will break the RDP path; update the code that builds TSCLIENT_HOME
after VOL_NAME is set to escape any double-quote characters (e.g. produce a new
variable like escaped_VOL_NAME by replacing " with \" using shell parameter
expansion or sed) and use escaped_VOL_NAME when constructing TSCLIENT_HOME so
the resulting remoteapplicationcmdline string remains syntactically valid.

In `@docs/macOS.md`:
- Line 103: The docs paragraph claiming the installer uses "sdl-freerdp3" is
incorrect; update docs/macOS.md to state the installer/scanner uses xfreerdp
(xfreerdp / xfreerdp3) to match setup.sh's macOS branch which probes for
"xfreerdp" and "xfreerdp3" (see the setup.sh comment "xfreerdp (not
sdl-freerdp)"). Replace the reference to sdl-freerdp3 with xfreerdp/xfreerdp3
and adjust the sentence so it accurately reflects that setup.sh invokes xfreerdp
for RDP/app scanning and logging.
- Around line 62-72: Update the "Install App Shortcuts (Scan-and-Generate)"
section to clearly distinguish Homebrew vs manual clone installs: add a short
subsection or note showing the Homebrew command "winapps-setup --user" for users
who installed via Homebrew, and keep the existing manual clone commands "bash
setup.sh --user" and "bash setup.sh --system" for those who cloned the repo;
reference the section heading and the setup scripts (setup.sh and winapps-setup)
so readers know which command applies to which installation method.

In `@Formula/winapps.rb`:
- Around line 49-51: Update the test block in Formula/winapps.rb to also verify
the setup helper is installed: add an assertion that runs the installed
"winapps-setup" binary (e.g., shell_output("#{bin}/winapps-setup --help" or
shell_output("#{bin}/winapps-setup 2>&1", 0)) or assert the file exists and is
executable via bin/"winapps-setup". This complements the existing assert_match
for "winapps" and will catch regressions where the setup script isn't installed.
- Around line 4-5: The formula currently pins the source via url
"https://github.com/winapps-org/winapps.git", branch: "main" while also
hardcoding version "1.0.0", which is non-reproducible; replace the git-branch
URL with a stable tagged release tarball URL and add its sha256 (or
alternatively move the git URL into a head block) so that url and version are
consistent and audit-friendly—update the url value, remove branch: "main", add
the corresponding sha256 for the tarball, and keep or update version to match
the tag (or convert the existing git entry into head do ... end).

In `@README.md`:
- Around line 332-343: The markdown heading "### GNU/Linux" (and similarly "##
Underlying Mechanism") lacks a blank line after the heading which triggers
markdownlint MD022; insert a single blank line immediately after the "###
GNU/Linux" heading (and add a blank line after "## Underlying Mechanism" if you
fix that too) so each heading is followed by an empty line before the next
paragraph or heading.

In `@setup.sh`:
- Around line 741-753: The startup logic for XQuartz uses fixed sleeps ("open -a
XQuartz" followed by "sleep 2" and a later "sleep 5") which is unreliable;
replace the arbitrary delays with a readiness loop that launches XQuartz if
needed (the pgrep/open -a XQuartz branch) and then polls for a usable X11
display (check that DISPLAY is set and that a connection can be made or that
XQuartz process has an X socket) with a timeout and short sleeps between
attempts, then only proceed to export DISPLAY and print the done/info messages
once the readiness check succeeds (use the same symbols: pgrep -x Xquartz/X11,
open -a XQuartz, export DISPLAY, and the echo/sleep sequence).
- Around line 412-418: Summary: On macOS the existing macOS-specific
BIN_PATH/SOURCE_PATH override doesn't handle OPT_SYSTEM=1 so APP_PATH and
SOURCE_PATH fall back to Linux locations; either disallow system installs on
Darwin or set macOS-appropriate system paths. Fix: in the PLATFORM="Darwin" && [
-n "$HOMEBREW_PREFIX" ] branch check OPT_SYSTEM (e.g. inspect OPT_SYSTEM or
function that sets system mode) and if OPT_SYSTEM=1 either exit with a clear
error message refusing system installs on macOS or set APP_PATH and SOURCE_PATH
to macOS-appropriate system locations (for example set APP_PATH to
"$HOME/Applications" or "/Applications" and SOURCE_PATH to
"$HOMEBREW_PREFIX/share/winapps" or another macOS convention), ensuring you also
set BIN_PATH="$HOMEBREW_PREFIX/bin" as before; update any help/error text to
reflect the choice.
- Around line 1336-1344: The batch loop :WAIT_DRIVE currently spins
indefinitely; change the block that writes to BATCH_SCRIPT_PATH to include a
retry counter and terminate the loop after a max attempts value (derived from
APP_SCAN_TIMEOUT or a new MAX_ATTEMPTS). Concretely, initialize a counter
variable (e.g., set /A COUNT=0) before :WAIT_DRIVE, increment it inside the loop
(set /A COUNT+=1) after each ping, compare it against MAX_ATTEMPTS (if COUNT GEQ
MAX_ATTEMPTS goto DRIVE_TIMEOUT) and add a :DRIVE_TIMEOUT label that logs an
error and exits with a non-zero code (exit /b 1) instead of looping forever;
keep the existing :DRIVE_READY label unchanged. Ensure these changes are
produced into the same block that writes to BATCH_SCRIPT_PATH so the generated
batch file includes the counter logic.
- Around line 1240-1250: The script currently uses /cert:ignore when invoking
the FreeRDP client via the FREERDP_COMMAND invocation (the block that sets
RDP_FLAGS_NON_WINDOWS and calls $FREERDP_COMMAND with /v:"$RDP_IP":"$RDP_PORT"),
which disables TLS certificate validation; change the flag to /cert:tofu instead
and apply the same replacement for the other FreeRDP invocation used for the app
scan (the similar $FREERDP_COMMAND call near the app-scan code) so both RDP
access test and app scan use trust-on-first-use rather than ignoring
certificates.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

waNotify on macOS does not escape AppleScript special characters in $BODY / $TITLE.

If BODY or TITLE contain double quotes or backslashes, the osascript -e call will produce a syntax error or behave unexpectedly. While current call sites use mostly hardcoded strings, VM_NAME and other config-derived values could contain such characters.

🛡️ Proposed fix — escape for AppleScript

     if [ "$PLATFORM" = "Darwin" ]; then
-        osascript -e "display notification \"$BODY\" with title \"$TITLE\"" 2>/dev/null || echo "$TITLE: $BODY"
+        local ESCAPED_BODY="${BODY//\\/\\\\}"
+        ESCAPED_BODY="${ESCAPED_BODY//\"/\\\"}"
+        local ESCAPED_TITLE="${TITLE//\\/\\\\}"
+        ESCAPED_TITLE="${ESCAPED_TITLE//\"/\\\"}"
+        osascript -e "display notification \"$ESCAPED_BODY\" with title \"$ESCAPED_TITLE\"" 2>/dev/null || echo "$TITLE: $BODY"
     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.

Suggested change

	function waNotify() {
	local TITLE="$1"
	local BODY="$2"
	local ICON="${3:-dialog-info}"
	local EXPIRE="${4:-8000}"
	if [ "$PLATFORM" = "Darwin" ]; then
	osascript -e "display notification \"$BODY\" with title \"$TITLE\"" 2>/dev/null || echo "$TITLE: $BODY"
	else
	notify-send --expire-time="$EXPIRE" --icon="$ICON" --app-name="WinApps" --urgency="low" "$TITLE" "$BODY"
	fi
	}
	function waNotify() {
	local TITLE="$1"
	local BODY="$2"
	local ICON="${3:-dialog-info}"
	local EXPIRE="${4:-8000}"
	if [ "$PLATFORM" = "Darwin" ]; then
	local ESCAPED_BODY="${BODY//\\/\\\\}"
	ESCAPED_BODY="${ESCAPED_BODY//\"/\\\"}"
	local ESCAPED_TITLE="${TITLE//\\/\\\\}"
	ESCAPED_TITLE="${ESCAPED_TITLE//\"/\\\"}"
	osascript -e "display notification \"$ESCAPED_BODY\" with title \"$ESCAPED_TITLE\"" 2>/dev/null || echo "$TITLE: $BODY"
	else
	notify-send --expire-time="$EXPIRE" --icon="$ICON" --app-name="WinApps" --urgency="low" "$TITLE" "$BODY"
	fi
	}

🤖 Prompt for AI Agents

In `@bin/winapps` around lines 176 - 186, The waNotify function interpolates TITLE
and BODY directly into osascript which breaks when those strings contain double
quotes or backslashes; update waNotify to escape AppleScript-special characters
before calling osascript (e.g., replace backslashes "\" with "\\" and double
quotes """ with "\""), use the escaped TITLE and BODY in the osascript -e
invocation, and keep the existing 2>/dev/null || echo fallback; refer to the
waNotify function and the TITLE/BODY variables when making the change so all
macOS notification calls are safe.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, locate and examine the bin/winapps file
find . -name "winapps" -type f | head -5

Repository: winapps-org/winapps

Length of output: 76

---

🏁 Script executed:

# Check if bin/winapps exists and read lines around 275-284
if [ -f "bin/winapps" ]; then
  echo "=== File found, reading lines 270-290 ===:"
  sed -n '270,290p' bin/winapps
fi

Repository: winapps-org/winapps

Length of output: 744

---

🏁 Script executed:

# Run the verification script from the review comment
HOME="/Users/testuser"
VOL_NAME="Macintosh HD"
TSCLIENT_HOME="\\\\tsclient\\${VOL_NAME}${HOME//\//\\}"
echo "Test 1 - Normal volume name with space:"
echo "TSCLIENT_HOME: $TSCLIENT_HOME"
echo ""

# Test with special characters
VOL_NAME="Macintosh HD's"
TSCLIENT_HOME="\\\\tsclient\\${VOL_NAME}${HOME//\//\\}"
echo "Test 2 - Volume name with apostrophe:"
echo "TSCLIENT_HOME: $TSCLIENT_HOME"
echo ""

# Test with quotes to see if that helps
VOL_NAME="Macintosh HD"
TSCLIENT_HOME="\\\\tsclient\\\"${VOL_NAME}\"${HOME//\//\\}"
echo "Test 3 - With quotes (attempted):"
echo "TSCLIENT_HOME: $TSCLIENT_HOME"

Repository: winapps-org/winapps

Length of output: 342

---

🏁 Script executed:

# Also test the awk extraction pattern from the code
# Simulate diskutil output with volume name containing space
diskutil_output=$(cat <<'EOF'
   Device Identifier:        disk0
   Device Node:              /dev/disk0
   Whole Device:             Yes
   Part of Whole:            disk0
   Device/Media Name:        APPLE SSD SM0256F
   Volume Name:              Macintosh HD
   Mounted:                  Yes
   Mount Point:              /
   Partition Type:           APFS Container
   File System Type:         apfs
   Preferred Block Size:     4096
   Allocation Block Size:    4096
   Media Type:               SSD
   Protocol:                 SATA
   SMART Status:             Verified
   Volume UUID:              12345678-1234-1234-1234-123456789ABC
   Owners:                   Disabled
   Number of Volumes:        1 APFS volumes currently attached to this APFS Container.  Use "diskutil apfs list" to show all APFS volumes.
EOF
)

echo "Simulating awk extraction with space in volume name:"
VOL_NAME=$(echo "$diskutil_output" | awk -F: '/Volume Name/{gsub(/^ +| +$/,"",$2); print $2}')
echo "Extracted VOL_NAME: '$VOL_NAME'"
echo ""

# Now test the full path construction
HOME="/Users/testuser"
TSCLIENT_HOME="\\\\tsclient\\${VOL_NAME}${HOME//\//\\}"
echo "Full path: $TSCLIENT_HOME"

Repository: winapps-org/winapps

Length of output: 201

---

🏁 Script executed:

# Find all usages of TSCLIENT_HOME
rg "TSCLIENT_HOME" bin/winapps -n -A 3 -B 1

Repository: winapps-org/winapps

Length of output: 972

---

🏁 Script executed:

# Check if there are any rdp-related functions or file generation
rg "\.rdp|rdp" bin/winapps -n -A 2 -B 1 | head -50

Repository: winapps-org/winapps

Length of output: 2492

---

🏁 Script executed:

# Look for how paths are handled in rdp content
rg "tsclient" bin/winapps -n -A 2 -B 2

Repository: winapps-org/winapps

Length of output: 974

---

🏁 Script executed:

# Check how FILE_PATH is used after line 720 - is it used in rdp file generation?
sed -n '715,750p' bin/winapps

Repository: winapps-org/winapps

Length of output: 1445

---

🏁 Script executed:

# Check the generate_rdp function more thoroughly to see how paths are handled
sed -n '188,250p' bin/winapps

Repository: winapps-org/winapps

Length of output: 2835

---

🏁 Script executed:

# Look for any escaping of FILE_PATH or path handling for rdp content
rg "FILE_PATH|generate_rdp" bin/winapps -n -A 5 -B 2 | head -100

Repository: winapps-org/winapps

Length of output: 3046

---

🏁 Script executed:

# Verify the actual .rdp file generation with spaces in volume name
HOME="/Users/testuser"
VOL_NAME="Macintosh HD"
TSCLIENT_HOME="\\\\tsclient\\${VOL_NAME}${HOME//\//\\}"

# Simulate the path substitution at line 720
FILE_PATH="/Users/testuser/Documents/file.txt"
FILE_PATH="${FILE_PATH/#${HOME}/${TSCLIENT_HOME}}"
FILE_PATH="${FILE_PATH//\//\\}"
echo "FILE_PATH after line 721: $FILE_PATH"

# Simulate line 724 - quoting
APP_CMDLINE="\"${FILE_PATH}\""
echo "APP_CMDLINE (line 724): $APP_CMDLINE"

# Simulate what gets written to .rdp file (line 212 in generate_rdp)
echo "What goes into .rdp file:"
echo "remoteapplicationcmdline:s:${APP_CMDLINE}"

Repository: winapps-org/winapps

Length of output: 340

---

🏁 Script executed:

# Check if there are any other usages of TSCLIENT_HOME that might not be protected
rg "TSCLIENT_HOME" bin/winapps -B 2 -A 2 -n

Repository: winapps-org/winapps

Length of output: 989

---

🏁 Script executed:

# Verify behavior with special characters in volume names
HOME="/Users/testuser"

# Test cases: spaces, apostrophes, double quotes
for VOL_NAME in "Macintosh HD" "HD's Drive" "Macintosh \"Quoted\""; do
  TSCLIENT_HOME="\\\\tsclient\\${VOL_NAME}${HOME//\//\\}"
  FILE_PATH="/Users/testuser/test.txt"
  FILE_PATH="${FILE_PATH/#${HOME}/${TSCLIENT_HOME}}"
  FILE_PATH="${FILE_PATH//\//\\}"
  APP_CMDLINE="\"${FILE_PATH}\""
  echo "Volume: $VOL_NAME"
  echo "  Result: remoteapplicationcmdline:s:${APP_CMDLINE}"
  echo ""
done

Repository: winapps-org/winapps

Length of output: 394

---

🏁 Script executed:

# Test what happens with quotes in volume name and how RDP parsers might handle it
# Also verify if this is even a realistic concern on macOS
diskutil info / 2>/dev/null | grep "Volume Name" || echo "Testing volume name handling"

Repository: winapps-org/winapps

Length of output: 91

---

🏁 Script executed:

# Check if there's any escaping of quotes in RDP generation or handling
rg "rdp|RDP" bin/winapps | rg -i "escap|quot" || echo "No quote escaping found"

Repository: winapps-org/winapps

Length of output: 86

---

🏁 Script executed:

# Test if the RDP file would actually be malformed with embedded quotes
cat <<'EOF' > /tmp/test_rdp.rdp
full address:s:192.168.1.100:3389
remoteapplicationcmdline:s:"\tsclient\Macintosh "Quoted"\Users\testuser\test.txt"
EOF
echo "=== RDP file with embedded quote ===" 
cat /tmp/test_rdp.rdp
echo ""
echo "This RDP syntax is ambiguous - the parser may not correctly identify the command line value."

Repository: winapps-org/winapps

Length of output: 309

---

🏁 Script executed:

# Check realistic macOS volume names - can they contain quotes?
# Test if diskutil allows such names
echo "Checking if macOS volume names can contain double quotes (theoretical):"
echo "macOS allows most characters in volume names except : and null byte"
echo "Double quotes are technically possible but extremely rare in practice"

Repository: winapps-org/winapps

Length of output: 272

---

Volume names containing double quotes will create malformed RDP syntax; spaces are already handled by the existing path quoting.

The awk extraction correctly handles volume names with spaces, and the path is already quoted at line 724 when written to the RDP file. However, volume names containing unescaped double quotes would break RDP syntax—for example, a volume named Macintosh "Quoted" would produce remoteapplicationcmdline:s:"\tsclient\Macintosh "Quoted"\...", which RDP parsers may not interpret correctly. Consider escaping double quotes in VOL_NAME during path construction.

🤖 Prompt for AI Agents

In `@bin/winapps` around lines 275 - 284, VOL_NAME extracted from diskutil can
contain double quotes which will break the RDP path; update the code that builds
TSCLIENT_HOME after VOL_NAME is set to escape any double-quote characters (e.g.
produce a new variable like escaped_VOL_NAME by replacing " with \" using shell
parameter expansion or sed) and use escaped_VOL_NAME when constructing
TSCLIENT_HOME so the resulting remoteapplicationcmdline string remains
syntactically valid.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Install instructions should distinguish Homebrew vs. manual clone paths.

Lines 67–72 show bash setup.sh --user / bash setup.sh --system, which only works after a manual git clone. For the recommended Homebrew installation, the command is winapps-setup --user. Consider splitting this into two subsections or adding a note.

Suggested clarification

 ## Install App Shortcuts (Scan-and-Generate)
 
 Run the installer to scan for installed Windows apps and create CLI wrappers:
 
 ```bash
+# If installed via Homebrew:
+winapps-setup --user
+
+# If installed via manual git clone:
 # User install (~/.local/bin)
 bash setup.sh --user
 
 # System install (/usr/local/bin)
 bash setup.sh --system
 ```

📝 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.

Suggested change

	## Install App Shortcuts (Scan-and-Generate)
	Run the installer to scan for installed Windows apps and create CLI wrappers:
	```bash
	# User install (~/.local/bin)
	bash setup.sh --user
	# System install (/usr/local/bin)
	bash setup.sh --system
	```
	## Install App Shortcuts (Scan-and-Generate)
	Run the installer to scan for installed Windows apps and create CLI wrappers:

🤖 Prompt for AI Agents

In `@docs/macOS.md` around lines 62 - 72, Update the "Install App Shortcuts
(Scan-and-Generate)" section to clearly distinguish Homebrew vs manual clone
installs: add a short subsection or note showing the Homebrew command
"winapps-setup --user" for users who installed via Homebrew, and keep the
existing manual clone commands "bash setup.sh --user" and "bash setup.sh
--system" for those who cloned the repo; reference the section heading and the
setup scripts (setup.sh and winapps-setup) so readers know which command applies
to which installation method.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Documentation references sdl-freerdp3 but setup.sh actually uses xfreerdp / xfreerdp3.

Line 103 states the installer uses sdl-freerdp3, but the macOS branch in setup.sh (Lines 714–726) only probes for xfreerdp and xfreerdp3. The comment at Line 710 in setup.sh even notes "xfreerdp (not sdl-freerdp)". Update the docs to match the actual implementation.

🤖 Prompt for AI Agents

In `@docs/macOS.md` at line 103, The docs paragraph claiming the installer uses
"sdl-freerdp3" is incorrect; update docs/macOS.md to state the installer/scanner
uses xfreerdp (xfreerdp / xfreerdp3) to match setup.sh's macOS branch which
probes for "xfreerdp" and "xfreerdp3" (see the setup.sh comment "xfreerdp (not
sdl-freerdp)"). Replace the reference to sdl-freerdp3 with xfreerdp/xfreerdp3
and adjust the sentence so it accurately reflects that setup.sh invokes xfreerdp
for RDP/app scanning and logging.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

Homebrew formula best practices git branch url version pinning

💡 Result:

1) Don’t use a moving Git branch as the stable url

For a formula’s stable spec, Homebrew expects the url to be reproducible—typically a release tarball plus a checksum (sha256). If you’re pulling from a repository, the url should be pinned to a tag and/or revision, not an unpinned moving branch. [1]

2) If you need “latest from git”, use head (and prefer pinning it)

Put Git checkouts under a head stanza (installed via brew install --HEAD ...). Homebrew supports specifying a branch, tag, and/or revision for head (pinning to a specific tag/revision is the most reproducible; a branch is intentionally moving). [1] [2]

3) Version pinning: distinguish user pinning vs packaging older versions

• Local “don’t upgrade this” pinning: use brew pin <formula> / brew unpin <formula> to prevent it being upgraded during a general brew upgrade. [3]
• Packaging / distributing a pinned older version: Homebrew’s docs explicitly say Homebrew’s versions “should not be used to ‘pin’ formulae to your personal requirements”; instead, maintain the version in your own tap (often via brew extract). [4]
• Versioned formulae (foo@1, foo@2) are supported, but only under specific policy constraints (not meant as a general personal pinning mechanism). [4]

Practical rule of thumb

• Want repeatable installs for others? Use a tagged release tarball (url + sha256) and bump versions normally.
• Want “latest main branch” for developers? Provide head ... (optionally :branch), and tell users to install with --HEAD.
• Want to freeze a version long-term? Create/maintain a tap (and/or a foo@X formula when appropriate), not a branch-based stable url. [4]

Sources: [1] [2] [3] [4]

---

🌐 Web query:

Homebrew formula conventions using branch main vs tagged release

💡 Result:

Convention: stable (default install) uses an immutable release, head uses main

• Homebrew expects a formula to have a stable, tagged version (i.e., not “just a GitHub repo with no versions”) for normal installs. (docs.brew.sh)
• If you want to offer the cutting-edge code from main/master/trunk, you do that via a head spec, installable with brew install --HEAD …. (docs.brew.sh)
• For head from Git, Homebrew’s convention is to always specify the branch, e.g. branch: "main". (docs.brew.sh)
• In Homebrew/core specifically, a HEAD-only formula (no stable download) is flagged by audit. (docs.brew.sh)

What to put in the formula

Stable: prefer a release tarball + SHA-256

Use the project’s release artifact (GitHub “refs/tags/…tar.gz”, upstream tarball, etc.) with a pinned checksum. Homebrew update tooling reflects this expectation: if you change url, you provide --sha256 (or, for git-tag style, --tag + --revision). (docs.oldtimes.me)

head: point to the main branch (moving target)

Put the branch build under head (optionally in a head do block) and include branch: "main" for Git repos. (docs.brew.sh)

Minimal example pattern

class Foo < Formula
  homepage "https://example.com"
  url "https://github.com/org/foo/archive/refs/tags/v1.2.3.tar.gz"
  sha256 "…"
  license "…"

  head "https://github.com/org/foo.git", branch: "main"
end

If you tell me whether this is for homebrew/core or your own tap, and what your upstream provides (GitHub Releases? tags only? no releases?), I can suggest the most “brew-audit-friendly” stable URL/tag style for your specific repo.

Citations:

• 1: https://docs.brew.sh/Formula-Cookbook
• 2: https://docs.brew.sh/Formula-Cookbook?utm_source=openai
• 3: https://docs.brew.sh/Formula-Cookbook?utm_source=openai
• 4: https://docs.brew.sh/rubydoc/Homebrew/FormulaAuditor.html?utm_source=openai
• 5: https://docs.oldtimes.me/homebrew/Manpage.html?utm_source=openai
• 6: https://docs.brew.sh/Formula-Cookbook?utm_source=openai

---

Use a tagged release URL instead of branch: "main" for the stable version.

Homebrew requires stable formula URLs to be reproducible and pinned to a specific release. Using branch: "main" with a hardcoded version "1.0.0" violates this convention: the version will never update when main changes, version tracking (brew outdated/brew upgrade) will break, and this formula will fail Homebrew audit checks.

To fix: either point url to a tagged release tarball with a sha256 checksum, or move the git repository to a head block for users who need the latest development build.

🤖 Prompt for AI Agents

In `@Formula/winapps.rb` around lines 4 - 5, The formula currently pins the source
via url "https://github.com/winapps-org/winapps.git", branch: "main" while also
hardcoding version "1.0.0", which is non-reproducible; replace the git-branch
URL with a stable tagged release tarball URL and add its sha256 (or
alternatively move the git URL into a head block) so that url and version are
consistent and audit-friendly—update the url value, remove branch: "main", add
the corresponding sha256 for the tarball, and keep or update version to match
the tag (or convert the existing git entry into head do ... end).

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Add blank line after ### GNU/Linux heading to fix markdownlint MD022.

Static analysis flags that Line 343 (### GNU/Linux) has no blank line below it before the next heading. This is also flagged for Line 9 (## Underlying Mechanism), though that appears pre-existing.

Fix

 ### GNU/Linux
+
 ### Step 1: Configure a Windows VM

📝 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.

Suggested change

	### macOS
	For macOS installation and setup, see the [macOS guide](docs/macOS.md). WinApps on macOS uses Microsoft "Windows App" (free from the Mac App Store) instead of FreeRDP, and requires no virtual machine on the Mac — just a Windows machine with RDP enabled.
	```bash
	# Quick start (Homebrew)
	brew tap winapps-org/winapps
	brew install winapps
	```
	### GNU/Linux
	### macOS
	For macOS installation and setup, see the [macOS guide](docs/macOS.md). WinApps on macOS uses Microsoft "Windows App" (free from the Mac App Store) instead of FreeRDP, and requires no virtual machine on the Mac — just a Windows machine with RDP enabled.

🧰 Tools

🪛 markdownlint-cli2 (0.20.0)

[warning] 343-343: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

🤖 Prompt for AI Agents

In `@README.md` around lines 332 - 343, The markdown heading "### GNU/Linux" (and
similarly "## Underlying Mechanism") lacks a blank line after the heading which
triggers markdownlint MD022; insert a single blank line immediately after the
"### GNU/Linux" heading (and add a blank line after "## Underlying Mechanism" if
you fix that too) so each heading is followed by an empty line before the next
paragraph or heading.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

macOS path override only applies to --user installs; --system on macOS will use Linux paths.

When OPT_SYSTEM=1 on macOS, APP_PATH is set to /usr/share/applications and SOURCE_PATH to /usr/local/bin/winapps-src, which don't match macOS conventions. While most macOS users will likely use --user, a --system install would attempt to create Linux-specific directories. Consider either blocking --system on macOS or adjusting the paths.

🤖 Prompt for AI Agents

In `@setup.sh` around lines 412 - 418, Summary: On macOS the existing
macOS-specific BIN_PATH/SOURCE_PATH override doesn't handle OPT_SYSTEM=1 so
APP_PATH and SOURCE_PATH fall back to Linux locations; either disallow system
installs on Darwin or set macOS-appropriate system paths. Fix: in the
PLATFORM="Darwin" && [ -n "$HOMEBREW_PREFIX" ] branch check OPT_SYSTEM (e.g.
inspect OPT_SYSTEM or function that sets system mode) and if OPT_SYSTEM=1 either
exit with a clear error message refusing system installs on macOS or set
APP_PATH and SOURCE_PATH to macOS-appropriate system locations (for example set
APP_PATH to "$HOME/Applications" or "/Applications" and SOURCE_PATH to
"$HOMEBREW_PREFIX/share/winapps" or another macOS convention), ensuring you also
set BIN_PATH="$HOMEBREW_PREFIX/bin" as before; update any help/error text to
reflect the choice.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

XQuartz startup has a fixed 2-second sleep with no readiness check, then a 5-second informational delay.

open -a XQuartz; sleep 2 may not be sufficient for XQuartz to fully initialize on all machines. The subsequent sleep 5 at Line 752 adds more buffer, but a readiness check (e.g., polling for $DISPLAY availability) would be more reliable than arbitrary sleeps totaling 7 seconds.

🤖 Prompt for AI Agents

In `@setup.sh` around lines 741 - 753, The startup logic for XQuartz uses fixed
sleeps ("open -a XQuartz" followed by "sleep 2" and a later "sleep 5") which is
unreliable; replace the arbitrary delays with a readiness loop that launches
XQuartz if needed (the pgrep/open -a XQuartz branch) and then polls for a usable
X11 display (check that DISPLAY is set and that a connection can be made or that
XQuartz process has an X socket) with a timeout and short sleeps between
attempts, then only proceed to export DISPLAY and print the done/info messages
once the readiness check succeeds (use the same symbols: pgrep -x Xquartz/X11,
open -a XQuartz, export DISPLAY, and the echo/sleep sequence).

[coderabbitai]

⚠️ Potential issue | 🟠 Major

/cert:ignore bypasses all TLS certificate validation during setup, exposing credentials to MITM attacks.

Setup transmits RDP_USER/RDP_PASS over RDP. Using /cert:ignore silently accepts any certificate, including one presented by an attacker. This applies to both the RDP access test (Line 1242) and the app scan (Line 1385). Use /cert:tofu (trust-on-first-use) instead, which is the default recommended in the project's own config example.

🔒 Proposed fix

     $FREERDP_COMMAND \
         $RDP_FLAGS_NON_WINDOWS \
-        /cert:ignore \
+        /cert:tofu \
         /d:"$RDP_DOMAIN" \

Apply the same change at Line 1385.

📝 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.

Suggested change

	$FREERDP_COMMAND \
	$RDP_FLAGS_NON_WINDOWS \
	/cert:tofu \
	/cert:ignore \
	/d:"$RDP_DOMAIN" \
	/u:"$RDP_USER" \
	/p:"$RDP_PASS" \
	/scale:"$RDP_SCALE" \
	+auto-reconnect \
	+home-drive \
	/app:program:"C:\Windows\System32\cmd.exe",cmd:"/C type NUL > $TEST_PATH_WIN && tsdiscon" \
	/v:"$RDP_IP" &>"$FREERDP_LOG" &
	/app:program:"C:\Windows\System32\cmd.exe",cmd:"$CMD_STRING" \
	/v:"$RDP_IP":"$RDP_PORT" &>"$FREERDP_LOG" &
	$FREERDP_COMMAND \
	$RDP_FLAGS_NON_WINDOWS \
	/cert:tofu \
	/d:"$RDP_DOMAIN" \
	/u:"$RDP_USER" \
	/p:"$RDP_PASS" \
	/scale:"$RDP_SCALE" \
	auto-reconnect \
	home-drive \
	/app:program:"C:\Windows\System32\cmd.exe",cmd:"$CMD_STRING" \
	/v:"$RDP_IP":"$RDP_PORT" &>"$FREERDP_LOG" &

🤖 Prompt for AI Agents

In `@setup.sh` around lines 1240 - 1250, The script currently uses /cert:ignore
when invoking the FreeRDP client via the FREERDP_COMMAND invocation (the block
that sets RDP_FLAGS_NON_WINDOWS and calls $FREERDP_COMMAND with
/v:"$RDP_IP":"$RDP_PORT"), which disables TLS certificate validation; change the
flag to /cert:tofu instead and apply the same replacement for the other FreeRDP
invocation used for the app scan (the similar $FREERDP_COMMAND call near the
app-scan code) so both RDP access test and app scan use trust-on-first-use
rather than ignoring certificates.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Batch drive-polling loop has no iteration limit; relies solely on external timeout.

The :WAIT_DRIVE loop will spin indefinitely inside Windows if \\tsclient\home never appears. While APP_SCAN_TIMEOUT will eventually kill the FreeRDP process, adding a retry counter in the batch script would be more robust and prevent unnecessary CPU usage inside the Windows guest.

Proposed fix — add a counter

     if [ "$PLATFORM" = "Darwin" ]; then
         {
+            echo "SET /A RETRIES=0"
             echo ":WAIT_DRIVE"
+            echo "SET /A RETRIES+=1"
+            echo "IF %RETRIES% GEQ 30 goto DRIVE_READY"
             printf '%s\n' "if exist \\\\tsclient\\home goto DRIVE_READY"
             echo "ping -n 3 127.0.0.1 >NUL"
             echo "goto WAIT_DRIVE"
             echo ":DRIVE_READY"
         } >"$BATCH_SCRIPT_PATH"
     fi

🤖 Prompt for AI Agents

In `@setup.sh` around lines 1336 - 1344, The batch loop :WAIT_DRIVE currently
spins indefinitely; change the block that writes to BATCH_SCRIPT_PATH to include
a retry counter and terminate the loop after a max attempts value (derived from
APP_SCAN_TIMEOUT or a new MAX_ATTEMPTS). Concretely, initialize a counter
variable (e.g., set /A COUNT=0) before :WAIT_DRIVE, increment it inside the loop
(set /A COUNT+=1) after each ping, compare it against MAX_ATTEMPTS (if COUNT GEQ
MAX_ATTEMPTS goto DRIVE_TIMEOUT) and add a :DRIVE_TIMEOUT label that logs an
error and exits with a non-zero code (exit /b 1) instead of looping forever;
keep the existing :DRIVE_READY label unchanged. Ensure these changes are
produced into the same block that writes to BATCH_SCRIPT_PATH so the generated
batch file includes the counter logic.

[dingyifei]

@oskardotglobal Thank you for the comments. I think some of the weird workarounds were not correctly removed when I had claude code doing manual cherry-pick. It was also attributing some of the workarounds to incorrect reasoning (eg setup script require longer time-out actually due to xquartz xfreerdp glitches).

I'll clean up the commits and sign-offs once I sort out some of the weirdness of macOS. For example, Windows App don't correctly close connection for app mode connections that started via rdp file. Meanwhile I'll just continue making commits and see if I can make it a reasonable experience on macOS (eg correctly terminating rdp connection and integration with UTM).

(Also hopefully as a PR it raise visibility and can have someone more knowledgeable in windows app or macos can provide some suggestions)