feat: cross-platform controllers (Linux + macOS + Windows stub) - #87
Open
mikejgray wants to merge 4 commits into
Open
feat: cross-platform controllers (Linux + macOS + Windows stub)#87mikejgray wants to merge 4 commits into
mikejgray wants to merge 4 commits into
Conversation
Refactors the skill to delegate all OS-specific work to a controller
selected at runtime by sys.platform. The skill class itself becomes
pure orchestration; platform-specific code lives entirely in
controllers/{linux,macos,windows}.py.
Architecture:
- controllers/base.py defines an abstract ApplicationController with
app_aliases, launch_app, close_app, is_running as required, and
can_switch_windows / switch_to_app / match_process / refresh_app_cache
as optional overrides with safe defaults.
- controllers/linux.py is a verbatim refactor of the existing Linux
logic (.desktop files, wmctrl, psutil) with no behaviour changes.
- controllers/macos.py is the macOS controller from
OscillateLabsLLC/skill-mac-application-launcher: .app bundle
parsing via plistlib, AppleScript via osascript for window
switching and graceful quit, `open` / `open -a` for launching,
and psutil-based process matching as a fallback.
- controllers/windows.py is a stub that raises NotImplementedError
with a design sketch in the module docstring (registry-based app
discovery, os.startfile / Get-StartApps, optional pygetwindow for
window management). Future contributors can implement this without
re-architecting the skill.
- controllers/__init__.py exposes get_controller(), which dispatches
on sys.platform with a `controller_override` setting escape hatch
for testing and unusual environments.
Backward compatibility:
- ApplicationLauncherSkill still exposes the historical public methods
(parse_desktop_file, get_desktop_apps, match_window, close_by_window,
close_by_process, match_process, switch_window, close_window,
applist, wmctrl). These are now thin delegates — the static methods
go to LinuxApplicationController (Linux remains the canonical
implementation for the legacy public API), the instance methods
delegate to whichever controller is active. Third-party skill
consumers should not see any breakage.
Tests:
- The upstream had no tests despite an existing build-tests workflow
pointing at a non-existent test/ directory; pytest was effectively
a no-op for the entire history of the skill. This PR adds 24 tests
in test/test_controllers.py covering:
* factory dispatch (linux, linux2, darwin, macos, win32, unknown
fallback, native_langs threading, controller_override)
* abstract base class contract (cannot instantiate, default
methods are safe no-ops)
* Linux launch/close paths with subprocess.Popen mocked
* macOS launch with `open` vs `open -a` for paths vs bare names
* macOS AppleScript activate / quit codepaths
* macOS .app bundle parsing (missing plist, full metadata,
directory-name fallback)
* Windows stub raises NotImplementedError
- All tests run on any platform; OS-specific calls are mocked.
- 24/24 passing locally.
- Manually smoke-tested on macOS: factory selects MacOSApplicationController,
discovers 252 real .app bundles, can_switch_windows() reports True,
legacy ApplicationLauncherSkill.parse_desktop_file is still callable.
setup.py:
- packages = [SKILL_PKG, f"{SKILL_PKG}.controllers"] so the new
subpackage installs cleanly via the existing
package_dir={SKILL_PKG: ""} root-layout pattern.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The repo's ovoscope.yml workflow has been pointing at a non-existent test/end2end/ directory since the skill was created, so the e2e suite has been a silent no-op alongside the unit test suite. Add a minimal scaffold: - test_ovoscope_imports_cleanly: import smoke that proves the ovoscope dep installs and its public End2EndTest API is intact - test_launch_firefox_intent_match: real e2e for the launch path, marked skip until a per-test monkeypatch of ApplicationController.launch_app is wired in to avoid actually launching GUI applications on the CI runner. Module docstring documents the design. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughRefactors the skill to use a runtime ApplicationController abstraction selected by Changes
Sequence Diagram(s)sequenceDiagram
participant Skill as ApplicationLauncherSkill
participant Controller as ApplicationController
participant OS as Operating System
Note over Skill: Initialize and bind controller
Skill->>Controller: get_controller(settings, native_langs)
Skill->>Skill: self.controller = Controller
Note over Skill: Handle Launch Intent
Skill->>Skill: handle_async_prompt(message)
Skill->>Controller: launch_app(app_name)
Controller->>Controller: match alias / decide method
alt spawn command
Controller->>OS: spawn process / run command
else window activation
Controller->>OS: activate window (wmctrl / osascript)
end
OS-->>Controller: result
Controller-->>Skill: success / failure
Note over Skill: Close Flow
Skill->>Skill: handle_fallback(message)
Skill->>Controller: close_app(app_name)
Controller->>Controller: try window-close or process termination
Controller->>OS: close/terminate
OS-->>Controller: result
Controller-->>Skill: success / failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
test/end2end/test_launch_intent.py (1)
77-81:expected_messagesappears to be a placeholder.The
expected_messagesis set to[utterance](the input message), which likely isn't the actual expected response. This seems like a placeholder that should contain the expected bus messages (e.g.,mycroft.acknowledge, skill responses). Since the test is skipped anyway, consider adding a comment clarifying this is a placeholder or updating to show the intended expected messages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/end2end/test_launch_intent.py` around lines 77 - 81, The test currently uses End2EndTest(... expected_messages=[utterance] ...) which looks like a placeholder instead of the actual expected bus messages; update the call to either populate expected_messages with the intended bus events (e.g., "mycroft.acknowledge", any skill response message IDs or payloads) or add an inline comment explaining this is intentionally a placeholder while the test is skipped. Locate the End2EndTest invocation in test_launch_intent.py (references: End2EndTest, expected_messages, SKILL_ID, utterance, execute) and replace [utterance] with the real expected message list or add a clear comment above the call describing the placeholder intent.controllers/macos.py (2)
75-101: Consider extracting the repeated match-and-rebuild pattern.The same pattern appears four times: call
match_one, catch exception, check cache validity, attempt rebuild, retrymatch_one. This could be extracted to a helper method to reduce duplication:def _match_app(self, app: str) -> Optional[Tuple[str, float]]: """Match app name with automatic cache rebuild on failure.""" try: return match_one(app.title(), self.app_aliases) except (IndexError, ValueError): if not self.is_cache_valid() and self._ensure_cache_or_rebuild(): try: return match_one(app.title(), self.app_aliases) except (IndexError, ValueError): pass return NoneAlso applies to: 117-152, 279-295, 317-352
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/macos.py` around lines 75 - 101, Extract the repeated match-and-rebuild logic into a helper (e.g. create a method named _match_app) that encapsulates calling match_one(app.title(), self.app_aliases), catching (IndexError, ValueError), checking self.is_cache_valid(), invoking self._ensure_cache_or_rebuild() and retrying match_one once; update launch_app (and the other locations mentioned) to call _match_app and handle a None return (meaning no match) instead of duplicating the try/except/cache-rebuild flow. Ensure _match_app returns a tuple (cmd, score) or None and preserve existing score-threshold and subprocess launching behavior in launch_app.
304-315: Sorting all processes on every call may be expensive.
match_processsorts all system processes bycreate_timeon each invocation. For systems with many processes, this could add noticeable latency. Consider whether sorting is necessary (the docstring doesn't specify ordering requirements) or cache the sorted list briefly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/macos.py` around lines 304 - 315, The current code builds a sorted list named processes by calling psutil.process_iter([...]) and sorting by proc.info["create_time"] on every invocation, which is expensive; update the function (the block using processes, psutil.process_iter, fuzzy_match, app_name, bundle_name and yielding proc) to avoid full sorting on each call: either remove the sorting and iterate psutil.process_iter(...) directly applying the fuzzy_match checks, or if recent processes are required, use heapq.nlargest to pick the top N by create_time or introduce a short-lived cache (TTL) for the sorted list and reuse it between calls; implement one of these approaches and ensure the rest of the logic (skipping zombies via proc.status(), computing score1/score2 and yielding proc) remains unchanged.controllers/linux.py (1)
134-134: Rename loop variableltolangfor clarity.Single-letter variable
l(lowercase L) is easily confused with1(one) and shadows the implicit built-in. Uselanginstead.♻️ Suggested fix
- extra_langs = [standardize_lang_tag(l) for l in extra_langs] + extra_langs = [standardize_lang_tag(lang) for lang in extra_langs]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/linux.py` at line 134, The list comprehension uses a single-letter loop variable `l` which is unclear and can be confused with `1`; change it to a descriptive name by replacing `l` with `lang` so the line becomes extra_langs = [standardize_lang_tag(lang) for lang in extra_langs]; update any nearby usages or similar comprehensions in the same scope that use `l` to `lang` to keep naming consistent and avoid shadowing or readability issues (referencing function standardize_lang_tag and variable extra_langs).controllers/__init__.py (1)
46-64: Add test coverage for additional platform aliases.The factory supports aliases
"mac","win", and"windows"(lines 50 and 54), but the parameterized tests only cover"linux","linux2","darwin", and"macos". Extend the test parameters to include"mac"for macOS and"win"/"windows"for Windows to ensure these aliases dispatch correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/__init__.py` around lines 46 - 64, Update the parameterized tests that exercise the platform dispatch to include the missing aliases so the factory branches for Mac and Windows are covered; add "mac" to the macOS test cases and add both "win" and "windows" to the Windows test cases so calls dispatch to MacOSApplicationController and WindowsApplicationController respectively (the factory uses platform values compared against ("darwin","macos","mac") and ("win32","windows","win")). Ensure the tests still assert the correct controller classes (LinuxApplicationController, MacOSApplicationController, WindowsApplicationController) and keep existing cases for "linux" and "linux2".__init__.py (2)
137-138: Missing error feedback when launch fails.If
self.launch_app(app)returnsFalse, the user receives no feedback sinceacknowledge()is only called on success. Consider adding a failure dialog or at minimum logging the result.💡 Suggested improvement
# launch - self.launch_app(app) + if not self.launch_app(app): + self.speak_dialog("launch_failed", {"application": app})Note: This assumes a
launch_failed.dialogexists or will be added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__init__.py` around lines 137 - 138, The call to self.launch_app(app) doesn't handle failures; update the launcher flow in the method containing self.launch_app(app) so it checks the boolean return value and on False emits feedback: either open the failure dialog (e.g. show "launch_failed.dialog") or at minimum call the logger to record the failure (use the same logger used elsewhere), while keeping acknowledge() for the success path; ensure you reference self.launch_app and acknowledge so callers get appropriate UI or log feedback when launching fails.
45-48: Consider graceful degradation on unsupported platforms.If
WindowsApplicationController.__init__raisesNotImplementedError, the skill will fail to initialize entirely on Windows. Consider wrapping theget_controller()call in a try/except that either falls back to a no-op controller or logs a warning and disables the skill gracefully.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__init__.py` around lines 45 - 48, Wrap the get_controller(...) call in a try/except that catches NotImplementedError (raised by WindowsApplicationController.__init__) and gracefully substitutes a no-op or disabled ApplicationController; for example, in the except block construct or import a NoOpApplicationController and assign it to self.controller, and log a warning explaining the skill is disabled on this platform via the existing logger. Ensure the code still sets self.controller to an object conforming to the ApplicationController interface so callers do not error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controllers/linux.py`:
- Around line 285-293: The subprocess.run call in switch_window should include a
timeout to avoid hanging: add a timeout argument (preferably a configurable
attribute like self.subprocess_timeout or a constant, e.g., timeout=5) to
subprocess.run([...]) and catch subprocess.TimeoutExpired separately, logging
the timeout exception via LOG.error with details and returning False; maintain
existing handling for non-zero return codes. Apply the same pattern (add
timeout, catch subprocess.TimeoutExpired, log and return False) to the other
wmctrl-invoking methods referenced around lines 295-303 and 305-327 so all
subprocess.run usages are protected from hangs.
- Line 58: The current call to subprocess.Popen using shlex.split(cmd) and
self.settings.get("shell", False) can execute user-controlled commands with
shell=True; update the call to avoid shell execution by default and either
remove the shell option or validate/require an explicit, auditable opt-in:
ensure subprocess.Popen is always invoked with shell=False and pass a
safely-split argv (use shlex.split(cmd) or a pre-validated list), or if you must
support shell=True keep it behind a strict setting (e.g., require
settings["allow_untrusted_shell"] True), validate/sanitize cmd, and emit a
security warning/log when shell=True; change the code around subprocess.Popen,
shlex.split, and self.settings.get("shell", False) accordingly.
- Around line 223-237: The call to match_one in match_process can raise when the
alias cache is empty or no match is found; wrap the match_one(app.title(),
self.app_aliases) invocation in a try/except (catch the same exception type the
macOS controller uses, or a broad Exception if the project uses that pattern)
and handle failure by returning/continuing without raising (e.g., return an
empty iterator or simply exit the function). Ensure you still normalize cmd as
before only when match_one succeeds, and keep the rest of match_process (process
iteration and fuzzy_match) unchanged; reference the match_process method and the
match_one(app.title(), self.app_aliases) call to locate where to add the
try/except.
- Around line 53-62: Wrap the call to match_one in launch_app so
IndexError/ValueError from an empty cache or no matches are caught (same as
macOS controller): move match_one and its score check inside a try/except that
catches IndexError and ValueError, log or ignore the exception, and return False
if matching fails; ensure the existing try/except around subprocess.Popen still
handles process launch errors. Apply the same pattern to match_process: catch
IndexError/ValueError thrown by match_one, return an empty result (or False as
appropriate) and prevent those exceptions from propagating.
In `@controllers/macos.py`:
- Around line 137-142: Sanitize the derived app_name before embedding it into
the AppleScript string: replace or escape any double quotes, backslashes and
newlines in app_name so the f-string applescript (the variable applescript where
app_name is interpolated) cannot break AppleScript syntax; apply the same
sanitization to the other occurrence of app_name (the block referenced around
lines 337-342). Implement a small helper (e.g., sanitize_app_name) and call it
where app_name is computed so the AppleScript always receives a safe, escaped
application name.
---
Nitpick comments:
In `@__init__.py`:
- Around line 137-138: The call to self.launch_app(app) doesn't handle failures;
update the launcher flow in the method containing self.launch_app(app) so it
checks the boolean return value and on False emits feedback: either open the
failure dialog (e.g. show "launch_failed.dialog") or at minimum call the logger
to record the failure (use the same logger used elsewhere), while keeping
acknowledge() for the success path; ensure you reference self.launch_app and
acknowledge so callers get appropriate UI or log feedback when launching fails.
- Around line 45-48: Wrap the get_controller(...) call in a try/except that
catches NotImplementedError (raised by WindowsApplicationController.__init__)
and gracefully substitutes a no-op or disabled ApplicationController; for
example, in the except block construct or import a NoOpApplicationController and
assign it to self.controller, and log a warning explaining the skill is disabled
on this platform via the existing logger. Ensure the code still sets
self.controller to an object conforming to the ApplicationController interface
so callers do not error.
In `@controllers/__init__.py`:
- Around line 46-64: Update the parameterized tests that exercise the platform
dispatch to include the missing aliases so the factory branches for Mac and
Windows are covered; add "mac" to the macOS test cases and add both "win" and
"windows" to the Windows test cases so calls dispatch to
MacOSApplicationController and WindowsApplicationController respectively (the
factory uses platform values compared against ("darwin","macos","mac") and
("win32","windows","win")). Ensure the tests still assert the correct controller
classes (LinuxApplicationController, MacOSApplicationController,
WindowsApplicationController) and keep existing cases for "linux" and "linux2".
In `@controllers/linux.py`:
- Line 134: The list comprehension uses a single-letter loop variable `l` which
is unclear and can be confused with `1`; change it to a descriptive name by
replacing `l` with `lang` so the line becomes extra_langs =
[standardize_lang_tag(lang) for lang in extra_langs]; update any nearby usages
or similar comprehensions in the same scope that use `l` to `lang` to keep
naming consistent and avoid shadowing or readability issues (referencing
function standardize_lang_tag and variable extra_langs).
In `@controllers/macos.py`:
- Around line 75-101: Extract the repeated match-and-rebuild logic into a helper
(e.g. create a method named _match_app) that encapsulates calling
match_one(app.title(), self.app_aliases), catching (IndexError, ValueError),
checking self.is_cache_valid(), invoking self._ensure_cache_or_rebuild() and
retrying match_one once; update launch_app (and the other locations mentioned)
to call _match_app and handle a None return (meaning no match) instead of
duplicating the try/except/cache-rebuild flow. Ensure _match_app returns a tuple
(cmd, score) or None and preserve existing score-threshold and subprocess
launching behavior in launch_app.
- Around line 304-315: The current code builds a sorted list named processes by
calling psutil.process_iter([...]) and sorting by proc.info["create_time"] on
every invocation, which is expensive; update the function (the block using
processes, psutil.process_iter, fuzzy_match, app_name, bundle_name and yielding
proc) to avoid full sorting on each call: either remove the sorting and iterate
psutil.process_iter(...) directly applying the fuzzy_match checks, or if recent
processes are required, use heapq.nlargest to pick the top N by create_time or
introduce a short-lived cache (TTL) for the sorted list and reuse it between
calls; implement one of these approaches and ensure the rest of the logic
(skipping zombies via proc.status(), computing score1/score2 and yielding proc)
remains unchanged.
In `@test/end2end/test_launch_intent.py`:
- Around line 77-81: The test currently uses End2EndTest(...
expected_messages=[utterance] ...) which looks like a placeholder instead of the
actual expected bus messages; update the call to either populate
expected_messages with the intended bus events (e.g., "mycroft.acknowledge", any
skill response message IDs or payloads) or add an inline comment explaining this
is intentionally a placeholder while the test is skipped. Locate the End2EndTest
invocation in test_launch_intent.py (references: End2EndTest, expected_messages,
SKILL_ID, utterance, execute) and replace [utterance] with the real expected
message list or add a clear comment above the call describing the placeholder
intent.
🪄 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: be1230ec-f966-44e0-95b3-5c8250ccc599
📒 Files selected for processing (11)
__init__.pycontrollers/__init__.pycontrollers/base.pycontrollers/linux.pycontrollers/macos.pycontrollers/windows.pysetup.pytest/__init__.pytest/end2end/__init__.pytest/end2end/test_launch_intent.pytest/test_controllers.py
| if score >= self.settings.get("thresh", 0.85): | ||
| LOG.info(f"Matched application: {app} (command: {cmd})") | ||
| try: | ||
| subprocess.Popen(shlex.split(cmd), shell=self.settings.get("shell", False)) |
Contributor
There was a problem hiding this comment.
Consider the security implications of shell=True in settings.
The shell setting defaults to False, but if set to True, user-controlled commands would be executed through the shell. While this may be intentional for flexibility, document the security implications or consider removing this option if not needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controllers/linux.py` at line 58, The current call to subprocess.Popen using
shlex.split(cmd) and self.settings.get("shell", False) can execute
user-controlled commands with shell=True; update the call to avoid shell
execution by default and either remove the shell option or validate/require an
explicit, auditable opt-in: ensure subprocess.Popen is always invoked with
shell=False and pass a safely-split argv (use shlex.split(cmd) or a
pre-validated list), or if you must support shell=True keep it behind a strict
setting (e.g., require settings["allow_untrusted_shell"] True),
validate/sanitize cmd, and emit a security warning/log when shell=True; change
the code around subprocess.Popen, shlex.split, and self.settings.get("shell",
False) accordingly.
When the skill runs under a launchd LaunchAgent, subprocess.Popen of `open` inherits the agent's restricted spawn context. macOS frequently refuses with `RBSRequestErrorDomain Code=5 / Launchd job spawn failed` (errno 163) — the LaunchAgent does not have the right entitlements to spawn arbitrary GUI apps. Routing the launch through `osascript` -> `tell application "X" to activate` hands the request off to the user's loginwindow session, which has the entitlements GUI app spawning needs. This is the same trick the controller already uses for switch_to_app and close_by_applescript, just applied to launch. Falls back to the original `open` / `open -a` codepath if osascript is missing or returns non-zero, so behavior on systems without osascript is unchanged. Reproduced live: "launch book trove" matched correctly through the fallback pipeline, controller resolved to /Applications/BookTrove.app, but launchd refused with error 163. Tests: - AppleScript path is preferred when osascript is present, `open` is not invoked - Fallback to `open` (.app bundle path) on AppleScript failure - Fallback to `open -a` (bare name) on AppleScript failure - Direct `open` path when osascript is unavailable Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Linux: - launch_app and match_process now catch (IndexError, ValueError) from match_one on empty alias caches instead of letting them bubble. Pre-existing latent bug that the macOS fork already handled — the Linux side never matched the same defensive shape. - Add a 5s timeout to all wmctrl subprocess.run calls (switch_window, close_window, get_window_process_mapping) so a wedged X server can't hang the skill. subprocess.TimeoutExpired is logged and surfaced as a clean failure. - Rename ambiguous loop var `l` -> `lang` in parse_desktop_file comprehensions (silences ruff E741, addresses CodeRabbit comment). macOS: - Extract the duplicated match-rebuild-rematch dance into a single _match_app() helper used by launch_app, switch_to_app, close_by_applescript, and match_process. Previously each method carried its own copy of the logic. - Sanitize app names before AppleScript interpolation via a new _escape_applescript() helper. App names rarely contain quotes or backslashes, but the controller also accepts user-defined aliases, so the safest move is to belt-and-braces escape every value before it lands inside `tell application "..."`. - match_process no longer pre-sorts psutil.process_iter() by create_time. The function never promised any ordering and the close_by_process consumer doesn't depend on one — sorting was a per-call O(n log n) waste. Also tightened the iteration to use ["pid", "name"] only and to handle NoSuchProcess/AccessDenied during status() inspection. Tests: - Linux launch_app / match_process empty-cache paths - Linux switch_window / get_window_process_mapping timeout handling - macOS _match_app success and unrecoverable-miss returns None - macOS _escape_applescript covers plain names, quotes, backslashes, and combined edge cases - macOS launch_app with a hostile alias proves the escape lands in the AppleScript string literal without breaking syntax End2end: - Tightened the placeholder comment on the skipped launch test so future contributors know expected_messages=[utterance] is a documented placeholder, not a real assertion. 35 passing, 1 skipped (was 27/1). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
controllers/macos.py (3)
147-166: Consider adding a timeout to the AppleScript subprocess call.The
subprocess.runcall lacks a timeout. Ifosascripthangs (e.g., waiting for user interaction on a modal dialog), this could block the skill indefinitely. The PR objectives mention 5s timeouts were added towmctrlcalls on Linux for similar reasons.⏱️ Suggested timeout
try: result = subprocess.run( [self.osascript, "-e", applescript], capture_output=True, text=True, check=False, + timeout=5, ) if result.returncode == 0: return True LOG.warning( "AppleScript activate failed for %s (%s); falling back to `open`", app_name, result.stderr.strip(), ) - except Exception as e: + except subprocess.TimeoutExpired: + LOG.warning( + "AppleScript activate timed out for %s; falling back to `open`", + app_name, + ) + except Exception as e: LOG.warning( "AppleScript activate raised for %s (%s); falling back to `open`", app_name, e, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/macos.py` around lines 147 - 166, The subprocess.run call invoking self.osascript with the applescript should include a timeout (e.g., 5 seconds) to avoid hangs; update the call in the activate block (where subprocess.run is used and result.returncode is checked) to pass timeout=5 (or a configurable constant) and handle subprocess.TimeoutExpired in the except clause—log a warning via LOG including app_name and the timeout exception and then fall back to the existing `open` behavior.
396-405: Same timeout concern applies here.The
subprocess.runcall to execute the AppleScriptquitcommand also lacks a timeout. For consistency with the recommended changes in_spawnandswitch_to_app, consider addingtimeout=5.⏱️ Suggested timeout
try: result = subprocess.run( - [self.osascript, "-e", applescript], capture_output=True, text=True, check=False + [self.osascript, "-e", applescript], capture_output=True, text=True, check=False, timeout=5 ) if result.returncode == 0: return True LOG.debug(f"AppleScript quit failed for {app_name}: {result.stderr}") + except subprocess.TimeoutExpired: + LOG.debug(f"AppleScript quit timed out for {app_name}") except Exception as e: LOG.exception(f"Failed to close {app} via AppleScript: {e}") return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/macos.py` around lines 396 - 405, Add a timeout=5 to the subprocess.run call that runs self.osascript with the applescript (the call that sets result = subprocess.run(...)), and handle subprocess.TimeoutExpired by logging via LOG.exception or LOG.debug consistent with surrounding error handling (keep the existing return False on failure). Ensure the call still sets capture_output=True, text=True, check=False, and that the existing success check (result.returncode == 0) remains unchanged.
210-219: Same timeout concern applies here.Like
_spawn, thissubprocess.runcall toosascriptlacks a timeout. Consider addingtimeout=5and handlingsubprocess.TimeoutExpiredfor consistency.⏱️ Suggested timeout
try: result = subprocess.run( - [self.osascript, "-e", applescript], capture_output=True, text=True, check=False + [self.osascript, "-e", applescript], capture_output=True, text=True, check=False, timeout=5 ) if result.returncode == 0: return True LOG.error(f"AppleScript error: {result.stderr}") + except subprocess.TimeoutExpired: + LOG.error(f"AppleScript timed out switching to {app}") except Exception as e: LOG.exception(f"Failed to switch to {app}: {e}") return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/macos.py` around lines 210 - 219, The subprocess.run call that invokes self.osascript with the applescript (the same pattern as _spawn) needs a timeout to avoid hanging; add timeout=5 to the subprocess.run call in the method that switches to an app (the block using self.osascript, applescript and returning True on returncode==0), catch subprocess.TimeoutExpired specifically (in addition to the generic Exception) and log a clear message (e.g., LOG.warning/LOG.error including the app and timeout), then return False on timeout; keep check=False and existing handling for nonzero returncode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@controllers/macos.py`:
- Around line 147-166: The subprocess.run call invoking self.osascript with the
applescript should include a timeout (e.g., 5 seconds) to avoid hangs; update
the call in the activate block (where subprocess.run is used and
result.returncode is checked) to pass timeout=5 (or a configurable constant) and
handle subprocess.TimeoutExpired in the except clause—log a warning via LOG
including app_name and the timeout exception and then fall back to the existing
`open` behavior.
- Around line 396-405: Add a timeout=5 to the subprocess.run call that runs
self.osascript with the applescript (the call that sets result =
subprocess.run(...)), and handle subprocess.TimeoutExpired by logging via
LOG.exception or LOG.debug consistent with surrounding error handling (keep the
existing return False on failure). Ensure the call still sets
capture_output=True, text=True, check=False, and that the existing success check
(result.returncode == 0) remains unchanged.
- Around line 210-219: The subprocess.run call that invokes self.osascript with
the applescript (the same pattern as _spawn) needs a timeout to avoid hanging;
add timeout=5 to the subprocess.run call in the method that switches to an app
(the block using self.osascript, applescript and returning True on
returncode==0), catch subprocess.TimeoutExpired specifically (in addition to the
generic Exception) and log a clear message (e.g., LOG.warning/LOG.error
including the app and timeout), then return False on timeout; keep check=False
and existing handling for nonzero returncode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e37d8ad-62d2-44f3-89a6-405a3457ee2d
📒 Files selected for processing (4)
controllers/linux.pycontrollers/macos.pytest/end2end/test_launch_intent.pytest/test_controllers.py
✅ Files skipped from review due to trivial changes (1)
- test/test_controllers.py
🚧 Files skipped from review as they are similar to previous changes (2)
- test/end2end/test_launch_intent.py
- controllers/linux.py
The automated pipeline has reached its destination. 🏁I've aggregated the results of the automated checks for this PR below. 🔒 Security (pip-audit)I've performed a digital frisk of this contribution. 👮♂️ ✅ No known vulnerabilities found (68 packages scanned). 📊 CoverageLet's see how much of the code is actually being tested... 🧐 ❌ 0.0% total coverage Files below 80% coverage (10 files)
Full report: download the 📋 Repo HealthThe repo's annual physical is complete! 🩺 ✅ All required files present. Latest Version: ✅ 🔌 Skill Tests (ovoscope)I ran the end-to-end skill tests to see how your skill behaves in the real world! 🎤 ✅ 1/2 passed, 1 skipped ❌ **test_launch_firefox_intent_match** — 0/1
🚌 Bus CoverageIs the code wearing its bus-suit? Let's see. 👔 🔍 LintEnsuring the codebase remains stable and healthy. 🛡️ ❌ ruff: issues found — see job log 🏷️ Release PreviewEnsuring the 'Thanks' section includes your name! 🤝 Current:
✅ PR title follows conventional commit format. 🚀 Release Channel Compatibility Predicted next version:
⚖️ License CheckI've performed a legal sanity check on this PR. 🧠 ✅ No license violations found (49 packages). License distribution: 12× MIT License, 8× MIT, 6× Apache Software License, 5× Apache-2.0, 3× BSD-3-Clause, 2× ISC License (ISCL), 2× PSF-2.0, 2× Python Software Foundation License, +8 more Full breakdown — 49 packages
Copyright (c) 2022 Phil Ewels Permission is hereby granted, free of charge, to any person obtaining a copy The above copyright notice and this permission notice shall be included in all THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. 🎙️ SkillDoes the skill know its own name? Checking manifest... 🏷️ 🎙️ (unknown skill_id) — 12 languages en-us: 2 intents · 3 dialogs · skill.json Translation coverage — 11 languages (8 complete, 3 incomplete)
🔨 Build TestsThe build bots have finished their assembly. 🤖 ✅ All versions pass
Keeping the OVOS ecosystem thriving 🌿 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Refactors the skill to delegate all OS-specific work to a controller selected at runtime by
sys.platform. Brings macOS support over fromOscillateLabsLLC/skill-mac-application-launcherand leaves a clean hook for a future Windows contribution.The skill class becomes pure orchestration; platform-specific code lives entirely in
controllers/{linux,macos,windows}.py.Architecture
controllers/base.pydefines the abstract interface:app_aliases,launch_app,close_app,is_runningare required;can_switch_windows,switch_to_app,match_process,refresh_app_cacheare optional with safe defaults.controllers/linux.pyis a verbatim refactor of the existing Linux logic (.desktopfiles,wmctrl,psutil) — no behaviour changes, just moved into a class.controllers/macos.pyis the controller from the macOS fork:.appbundle parsing viaplistlib, AppleScript viaosascriptfor window switching and graceful quit,open/open -afor launching,psutilprocess matching as a fallback. Discovers apps from/Applications,/System/Applications,/Applications/Utilities,/System/Library/CoreServices,~/Applications.controllers/windows.pyis a stub that raisesNotImplementedErrorwith a design sketch in the module docstring (registry-based discovery viaApp Paths,Get-StartAppsvia PowerShell,os.startfile/subprocess.Popenfor launch, optionalpygetwindow/pywin32for window management). A future contributor can fill this in without re-architecting the skill.controllers/__init__.pyexposesget_controller(settings, native_langs)which dispatches onsys.platformand accepts acontroller_overridesetting for testing or unusual environments.Backward compatibility
The historical public methods on
ApplicationLauncherSkillare preserved as thin delegates so any third-party caller that didfrom ovos_skill_application_launcher import ApplicationLauncherSkilland calledparse_desktop_file,get_desktop_apps,match_window,close_by_window,close_by_process,match_process,switch_window,close_window,applist, orwmctrlcontinues to work:parse_desktop_file,get_desktop_apps) delegate toLinuxApplicationControllerbecause Linux remains the canonical implementation for the legacy public API.Tests
Adds 24 unit tests in
test/test_controllers.pyand 2 e2e scaffold tests intest/end2end/test_launch_intent.py, matching the OVOS singular-test/convention used by the existing CI workflows.Unit coverage:
linux,linux2,darwin,macos,win32, unknown fallback,native_langsthreading,controller_overrideescape hatchsubprocess.Popenmocked, threshold cutoffopenvsopen -afor paths vs bare namessubprocess.runmocked.appbundle parsing — missing plist, full metadata, directory-name fallbackNotImplementedErrorAll unit tests run on any platform; OS-specific calls are mocked. 24/24 unit + 1 e2e import smoke passing locally, 1 e2e marked skip pending a launch_app monkeypatch (documented in the module docstring).
Manual verification on macOS
End-to-end real Mac integration works: factory selected the right controller, discovered 252 real
.appbundles,can_switch_windows()reports True (osascriptpresent), and the legacyApplicationLauncherSkill.parse_desktop_fileis still callable.setup.py
Adds the new subpackage cleanly under the existing root-layout pattern:
Test plan
pytest test/— 25 passing, 1 skipped, covers all three controllers.appdiscovery and the factory dispatchwmctrlpresent)Out of scope
controllers/windows.pydocstring for the design sketch)psutilwas already present and is used by both Linux and macOS controllerslaunch_appmonkeypatch is a follow-up)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests