fix(notify): repair WSL click-to-focus launcher for WScript.Shell.Run#6
fix(notify): repair WSL click-to-focus launcher for WScript.Shell.Run#6karotkriss wants to merge 4 commits into
Conversation
…tifier-free macOS host The osascript fallback path is only reached on macOS when terminal-notifier is absent, so the test asserted a path that is not taken on a host where terminal-notifier is present (some Linux runners ship it as a Ruby gem binstub). Skip cleanly via fm_detect_platform plus a terminal-notifier presence check, emitting one passing line so the harness count stays consistent.
The Windows toast's "Go to firstmate" click ran a VBS launcher whose wsl.exe invocation quoted the distro (-d "Ubuntu") and passed the home through an `env FM_HOME=...` wrapper. WScript.Shell.Run preserves neither: the quotes reach wsl.exe literally so the distro never resolves, and the env wrapper is mangled, so the click landed on a dead path and the terminal was never raised. Pass the distro unquoted and hand the home to fm-focus.sh as its positional argument, which survives the VBS -> wsl.exe chain. fm-focus.sh now reads $1 as the home, with the FM_HOME env and the self-derived repo root remaining the fallbacks for the macOS/Linux backends. Update the launcher comment and the WSL install test to the new format, and add focus coverage proving the positional home wins over an ambient FM_HOME.
… positional-arg launcher
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesWSL Positional Home Argument
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/fm-notify.sh`:
- Around line 453-456: The WSL launcher path in the `fm-notify.sh` branch that
builds `wsl_cmd` uses `WSL_DISTRO_NAME` unquoted, so distro names with spaces
break the generated command. Update the logic around the `wsl_cmd` assignment to
either reject `WSL_DISTRO_NAME` values containing whitespace or properly
quote/escape the distro name before inserting it into the `wsl.exe -d ...`
command so the VBS output remains valid.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e1b4908-ad59-4252-b15d-ebe4152603c0
📒 Files selected for processing (5)
CONTRIBUTING.mdbin/fm-focus.shbin/fm-notify.shtests/fm-focus.test.shtests/fm-notify.test.sh
| if [ -n "${WSL_DISTRO_NAME:-}" ]; then | ||
| vbs_distro=${WSL_DISTRO_NAME//\"/\"\"} | ||
| wsl_cmd="wsl.exe -d \"\"${vbs_distro}\"\" -e env FM_HOME=\"\"${vbs_home}\"\" bash \"\"${vbs_focus}\"\"" | ||
| wsl_cmd="wsl.exe -d ${WSL_DISTRO_NAME} -e bash \"\"${vbs_focus}\"\" \"\"${vbs_home}\"\"" | ||
| else | ||
| wsl_cmd="wsl.exe -e env FM_HOME=\"\"${vbs_home}\"\" bash \"\"${vbs_focus}\"\"" | ||
| wsl_cmd="wsl.exe -e bash \"\"${vbs_focus}\"\" \"\"${vbs_home}\"\"" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect header and relevant code sections
echo '--- header ---'
sed -n '1,120p' bin/fm-notify.sh
echo '--- relevant lines ---'
sed -n '430,470p' bin/fm-notify.sh
echo '--- search WSL_DISTRO_NAME ---'
rg -n "WSL_DISTRO_NAME|wsl\.exe -d|TestDistro|distro name" -S .
echo '--- file inventory for tests around fm-notify ---'
git ls-files | rg '(^|/)(test|tests|spec|__tests__|.*test\.).*fm-notify|fm-notify.*(test|spec)'Repository: karotkriss/firstmate
Length of output: 10648
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- tests around distro pinning ---'
sed -n '520,570p' tests/fm-notify.test.sh
echo '--- more install-path tests ---'
sed -n '570,640p' tests/fm-notify.test.sh
echo '--- any docs for WSL_DISTRO_NAME ---'
rg -n "WSL_DISTRO_NAME|wsl.exe -d|distro" -S README.md AGENTS.md docs tests binRepository: karotkriss/firstmate
Length of output: 6727
🌐 Web query:
WSL wsl.exe -d distro name with spaces quoting documentation
💡 Result:
In the Windows Subsystem for Linux (WSL), the use of spaces in distribution names is problematic and generally discouraged, as it often causes commands to fail with errors such as Wsl/Service/WSL_E_DISTRO_NOT_FOUND or Wsl/Service/E_INVALIDARG [1][2][3]. While some users have reported that it is possible to name or rename a distribution to include a space, interacting with such distributions via wsl.exe is inconsistent and frequently unsupported by the command-line interface [4][2][3][5]. Attempting to quote the distribution name (e.g., wsl -d "Distro Name") often fails because the underlying service parsing frequently does not treat the quoted string as a single argument for the distribution name [1][2]. If you are encountering issues: 1. Distro Name Requirements: Although not explicitly documented with strict character lists in all areas, it is best practice to avoid spaces and special characters. Valid names typically consist of alphanumeric characters, hyphens, underscores, and periods [5]. 2. Troubleshooting Quoting: If a distribution name does have a space and you must interact with it, behavior may vary significantly depending on whether you are using Command Prompt (cmd.exe) or PowerShell [1]. PowerShell sometimes handles quotes differently than cmd.exe, but success is not guaranteed [1]. 3. Workarounds: If you have a distribution with a space in its name that is causing issues, the most reliable solution is to rename the distribution to a name without spaces [3]. If you are developing a custom distribution, ensure the default name specified in /etc/wsl-distribution.conf follows standard naming conventions (no spaces) to avoid these management and lifecycle issues [5][6].
Citations:
- 1: quoting distribution name parameter fails microsoft/WSL#9792
- 2: Quoted distribution name leads to Wsl/Service/E_INVALIDARG microsoft/WSL#9859
- 3: wsl --import/--export does not handle paths with spaces or quoted paths correctly microsoft/WSL#3932
- 4: Cannot run command with distribution whose name contains space microsoft/WSL#3751
- 5: Bug: Distros with a space in their name are unmanageable — validate_distro_name rejects spaces that the parser/terminal/rename paths support octasoft-ltd/wsl-ui#124
- 6: https://learn.microsoft.com/en-us/windows/wsl/build-custom-distro
Reject whitespace in WSL_DISTRO_NAME
wsl.exe -d ${WSL_DISTRO_NAME} is written unquoted, so a distro like Ubuntu Preview produces a broken launcher even though install returns success. Reject whitespace here or escape/quote the distro before emitting the VBS.
🤖 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 `@bin/fm-notify.sh` around lines 453 - 456, The WSL launcher path in the
`fm-notify.sh` branch that builds `wsl_cmd` uses `WSL_DISTRO_NAME` unquoted, so
distro names with spaces break the generated command. Update the logic around
the `wsl_cmd` assignment to either reject `WSL_DISTRO_NAME` values containing
whitespace or properly quote/escape the distro name before inserting it into the
`wsl.exe -d ...` command so the VBS output remains valid.
1 participant
Intent
Repair the WSL click-to-focus launcher in the native notifier so the Windows toast's 'Go to firstmate' click actually focuses the firstmate terminal. The previously shipped launcher built a wsl.exe command that quoted the distro (-d "Ubuntu") and passed the firstmate home through an 'env FM_HOME=...' wrapper; neither survives Windows' WScript.Shell.Run - the quotes reach wsl.exe literally so the distro never resolves, and the env wrapper is mangled - so the click hit a dead path and the terminal never raised. The fix passes the distro UNQUOTED and hands the home to fm-focus.sh as a positional argument, which does survive the VBS -> wsl.exe chain; fm-focus.sh now reads $1 as the home, keeping the FM_HOME env and self-derived repo root as fallbacks for the macOS/Linux backends. This working pattern was already click-tested by the captain on a real Windows toast, so the WSL click-to-focus is E2E-verified; this no-mistakes pass is code-level (review/tests/shellcheck). The launcher comment, the WSL install test (which previously pinned the broken quoted-distro/env-FM_HOME format), and CONTRIBUTING's coverage note were updated to the new format, and a focus test was added proving the positional home wins over an ambient FM_HOME. This must update the existing PR kunchenguid#148 on origin (do not open a new PR).
What Changed
bin/fm-notify.shso the toast's "Go to firstmate" click actually focuses the terminal: the distro is now interpolated unquoted and the firstmate home rides asfm-focus.sh's positional argument instead of anenv FM_HOME=...wrapper, since neither the quotes nor the env wrapper surviveWScript.Shell.Run(so the old form resolved no distro and hit a dead path).bin/fm-focus.shnow reads$1as the home with precedence launcher-arg →FM_HOMEenv → self-derived repo root, preserving the env path that the macOS/Linux backends still use.CONTRIBUTING.mdcoverage note to match: the WSL install assertions now expect the unquoted distro / positional home / noenv FM_HOME=format, a newfm-focustest proves the positional$1home wins over an ambientFM_HOME, and the macOSosascript-fallback test is gated to a terminal-notifier-free macOS host. The Review step flagged the unquoted distro only as a known, benign limitation (standard distro names contain no spaces); all test suites pass.Risk Assessment
✅ Low: A small, well-bounded fix to a WSL-only launcher that the author already E2E click-tested on a real Windows toast, with matching test updates, no dead code, and no impact on the macOS/Linux/no-arg call paths.
Testing
The baseline test command already ran the full tmux-based suite green; I re-ran every test file and confirmed all pass, focusing on the two affected suites (fm-focus.test.sh and fm-notify.test.sh). Because the user-facing surface is a Windows VBS launcher, I produced reviewer-visible evidence by generating the actualfm-launch.vbsfrom both the broken base commit and the fixed target on a mocked WSL host - showing the broken quoted-distro/env FM_HOME=form versus the fixed unquoted-distro/positional-home form - and then ran the click-timebash fm-focus.sh "<home>"invocation live with a decoy FM_HOME, demonstrating the positional $1 home wins and the correct tmux pane is selected. The real Windows-toast click was already E2E-verified by the author; my artifacts confirm the code-level launcher-generation → positional-arg-consumption chain end-to-end. No findings.Evidence: Before/after VBS launcher comparison (annotated)
Evidence: BROKEN launcher (base 4240e17) - quoted distro + env FM_HOME wrapper
CreateObject("WScript.Shell").Run "wsl.exe -d ""Ubuntu"" -e env FM_HOME=""...My firstmate home"" bash ""...fm-focus.sh""", 0, FalseEvidence: FIXED launcher (target 7f44803) - unquoted distro + positional home
CreateObject("WScript.Shell").Run "wsl.exe -d Ubuntu -e bash ""...fm-focus.sh"" ""...My firstmate home""", 0, FalseEvidence: Live click-time transcript: positional $1 home wins over decoy FM_HOME, selects pane %4
$ FM_HOME="<decoy>" bash bin/fm-focus.sh "<real home>" (exit 0) tmux list-panes -a -F #{pane_id} tmux display-message -p -t %4 #{session_name}:#{window_index} tmux select-window -t work:7 tmux select-pane -t %4 RESULT: selected pane %4 (from the positional $1 home) and NEVER touched the decoy's %9. FIX VERIFIED.Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-notify.sh:454- The WSL distro is now interpolated UNQUOTED into the launcher command (wsl.exe -d ${WSL_DISTRO_NAME} -e bash ...). This is the deliberate, captain-verified fix - quoting was the original bug and standard WSL distro names (Ubuntu, Debian, kali-linux, etc.) never contain spaces - so it is strictly better than the old broken quoted form. Worth noting only as a known limitation: a distro name containing a space would split the-dargument (the prior quoted form was already broken for such names too, so this is not a new regression). No action needed.✅ **Test** - passed
✅ No issues found.
command -v tmux >/dev/null || { echo "tmux is required for e2e tests" >&2; exit 1; }; tmux -V; rc=0; for t in tests/*.test.sh; do echo "== $t =="; bash "$t" || rc=1; done; exit "$rc"bash tests/fm-focus.test.sh- 8 ok, including the newtest_positional_home_arg_wins_over_envbash tests/fm-notify.test.sh- 25 ok, including the rewritten WSL install assertions (unquoted distro, positional home, noenv FM_HOME=)Ran everytests/*.test.shin the repo; all files green (e.g. fm-watcher-lock 20 ok, fm-daemon 45 ok, fm-x-mode 53 ok)Generated the realfm-launch.vbsviafm_notify_installon a mocked WSL host (reg.exe/cmd.exe/wslpath/tmux stubbed, WSL_DISTRO_NAME=Ubuntu, space-containing home) from BOTH base 4240e17 and target 7f44803 to capture the before/after launcher commandRanFM_HOME=<decoy> bash bin/fm-focus.sh "<real home>"(the click-time invocation) with stubbed tmux, confirming it selects the positional home's pane %4 and never the decoy's %9✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests