Repository navigation
Add ug cursor app to open the Cursor desktop app - #1163
nachill-databricks wants to merge 4 commits into
Conversation
`ug cursor app [args]` opens the Cursor desktop app, which reads the same ~/.cursor/mcp.json that `ug mcp add` writes. It uses the `cursor` shell command when on PATH, falls back to `open -a Cursor.app` on macOS, and prints an install hint when the app is not found. Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
|
/ug-review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if sys.platform == "darwin": | ||
| for bundle in CURSOR_MACOS_APP_PATHS: | ||
| if bundle.exists(): | ||
| return ["open", "-a", str(bundle), *tool_args] |
There was a problem hiding this comment.
ug cursor app forwards arbitrary user args here. What happens with ug cursor app --new-window? With no --args separator, open parses the flag itself, so it errors or does the wrong thing on macOS. The same args work on Linux and Windows. Can we pass flags after --args, or just use cursor from PATH when there are flags?
| def test_uses_cursor_command_on_path(self, monkeypatch): | ||
| monkeypatch.setattr(cursor.shutil, "which", lambda name: "/usr/bin/cursor") |
There was a problem hiding this comment.
This test fails on a mac with /Applications/Cursor.app installed, because the macOS branch returns open -a ... first. Can we patch sys.platform (or CURSOR_MACOS_APP_PATHS) like the other tests do?
Suggestion:
| def test_uses_cursor_command_on_path(self, monkeypatch): | |
| monkeypatch.setattr(cursor.shutil, "which", lambda name: "/usr/bin/cursor") | |
| def test_uses_cursor_command_on_path(self, monkeypatch): | |
| monkeypatch.setattr(cursor.sys, "platform", "linux") | |
| monkeypatch.setattr(cursor.shutil, "which", lambda name: "/usr/bin/cursor") |
| assert kwargs["stderr"] is subprocess.DEVNULL | ||
| for key, value in cursor._DETACHED_POPEN_OPTIONS.items(): | ||
| assert kwargs[key] == value | ||
| proc.wait.assert_not_called() |
There was a problem hiding this comment.
Are the exec_or_spawn patch and proc.wait check necessary? launch_app never calls either, so they don't test anything. Can we drop them?
Polly AI ReviewBlocking issues
Security vulnerabilitiesNone identified in the changed code. Launch commands use argument lists, without shell interpolation. Non-blocking notes
ApproachPATH-first discovery, followed by the macOS fallback, matches the stated goal and avoids bypassing Cursor’s argument parser when its CLI is installed. The fallback still needs correct argument forwarding. SummaryDiff size (computed): 5 files; +149 / -1 lines (150 changed text lines); 0 binary files.
Shares use additions + deletions, including generated text and lockfiles. Binary files have no line count. Tests include fixtures/helpers under test paths and colocated test/spec files; categories are path-based, not coverage measurements. ChangesAdds desktop-app discovery and detached launch helpers, CLI dispatch and help text, focused tests, and a README example. No dependencies or generated files change. The implementation is proportional to the goal. The detached-process options duplicate those in TestsTest-by-test assessment
These are all added tests; no tests are removed and no reusable helpers are added. Existing agent-launch coverage remains relevant to preserving plain The concrete missing coverage is macOS flag forwarding; a unit case is sufficient to catch the demonstrated argument-construction bug. No additional E2E test is needed for that assertion. Tests were not executed during this review. The reported focused-suite and lint results, including the claimed baseline failures, are not independently verified. For manual verification on macOS, run ScopeAll changed files support the desktop-launch outcome. No clearly unrelated changes were found. The unresolved discovery-priority question concerns the intended behavior, not additional scope. Automated review by Polly · workflow run |
…t tests Co-authored-by: Isaac <no-reply@databricks.com>
|
/ug-review |
| for bundle in CURSOR_MACOS_APP_PATHS: | ||
| if bundle.exists(): | ||
| if any(arg.startswith("-") for arg in tool_args): | ||
| return ["open", "-a", str(bundle), "--args", *tool_args] |
There was a problem hiding this comment.
what happens with ug cursor app --new-window .? args after --args aren't resolved against the cwd (the app starts in /), and they're ignored if Cursor is already open. can we resolve paths to absolute ones, or use the cursor cli when flags are passed?
| argv, | ||
| stdin=subprocess.DEVNULL, | ||
| stdout=subprocess.DEVNULL, | ||
| stderr=subprocess.DEVNULL, |
There was a problem hiding this comment.
if open fails (e.g. a nonexistent path or a broken bundle), stderr goes to DEVNULL and the cli still prints success. can we keep stderr, or wait briefly on open and report a non-zero exit?
| print_success("Opening the Cursor app") | ||
| cursor.launch_app(argv) |
There was a problem hiding this comment.
what happens if popen raises OSError (e.g. broken cursor shim)? only RuntimeError is caught, so the user gets a traceback right after a success message. can we catch it and print_err?
| ".", | ||
| ] | ||
|
|
||
| def test_macos_paths_only_go_straight_to_open(self, tmp_path, monkeypatch): |
There was a problem hiding this comment.
are all these tests necessary? this checks the same paths-only branch as test_macos_prefers_open_app_bundle_over_cursor_command. can we fold it into that test?
Polly AI ReviewBlocking issuesNone confirmed. Security vulnerabilitiesNone identified. Launch arguments remain argument lists through the existing subprocess wrapper; the change introduces no configuration writes or explicit credential handling. Non-blocking notes
ApproachThe resolver plus detached launcher is a reasonable fit. Reusing SummaryDiff size (computed): 5 files; +187 / -1 lines (188 changed text lines); 0 binary files.
Shares use additions + deletions, including generated text and lockfiles. Binary files have no line count. Tests include fixtures/helpers under test paths and colocated test/spec files; categories are path-based, not coverage measurements. ChangesAdds desktop-app discovery, detached launching, CLI dispatch, a README example, and focused tests. No dependencies, lockfiles, or generated assets change. The size is proportionate to the outcome. Concrete reductions are the overlapping separator test and repeated MCP-auth explanation; the platform-specific argument handling otherwise serves distinct behavior. TestsTest-by-test assessmentAll changed tests are assessed below. No existing tests are modified or removed, and no shared fixtures or helpers are added.
Existing Missing coverage includes launch-time Tests and lint were not independently executed; the reported passing results remain author-provided evidence. Native macOS launch remains unverified. To verify manually, exercise ScopeAll five changed files support opening the Cursor desktop app. No unrelated changes were found. The unresolved launch-precedence question concerns the implementation’s contract, not an independent outcome. Automated review by Polly · workflow run |
…ilures Co-authored-by: Isaac <no-reply@databricks.com>
|
/ug-review |
| resolved.append(arg) | ||
| continue | ||
| bare = _LINE_COL_SUFFIX.sub("", arg) | ||
| if arg in (".", "..") or Path(arg).exists() or Path(bare).exists(): |
There was a problem hiding this comment.
is the (".", "..") check needed? Path(".").exists() and Path("..").exists() are always true, so that branch never adds anything.
| args = _absolutize_paths(tool_args) | ||
| if has_flags: | ||
| return ["open", "-a", str(bundle), "--args", *args] | ||
| return ["open", "-a", str(bundle), *args] |
There was a problem hiding this comment.
what happens with ug cursor app new.py on macOS? open -a exits non-zero when a file doesn't exist, but cursor new.py creates it. Can we prefer the cursor CLI whenever it's on PATH and use open only as a fallback?
| argv, | ||
| stdin=subprocess.DEVNULL, | ||
| stdout=subprocess.DEVNULL, | ||
| stderr=subprocess.PIPE if is_open else subprocess.DEVNULL, |
There was a problem hiding this comment.
what happens if cursor exits right away with an error (bad flag, broken install)? stderr goes to DEVNULL and the cli still prints "Opened the Cursor app". can we briefly poll for an early non-zero exit like we do for open?
| from ucode.agents import cursor | ||
|
|
||
| try: | ||
| if ctx.args and ctx.args[0] == "app": |
There was a problem hiding this comment.
ug cursor app ... used to pass app to cursor-agent as its prompt. Now it opens the desktop app instead, which breaks that call. Could we use a separate cursor-app command or a flag, so that existing forwarded args keep working?
Polly AI ReviewBlocking issues
Security vulnerabilitiesNone identified in the reviewed diff. Launches use argument lists without shell interpolation; the change adds no credential handling or configuration writes. Non-blocking notes
ApproachThe dispatch branch preserves ordinary Cursor Agent forwarding without introducing a subcommand group that could intercept agent arguments. Prefer the CLI unconditionally when available; this also removes the unnecessary flags/no-flags distinction from CLI selection. SummaryDiff size (computed): 5 files; +297 / -2 lines (299 changed text lines); 0 binary files.
Shares use additions + deletions, including generated text and lockfiles. Binary files have no line count. Tests include fixtures/helpers under test paths and colocated test/spec files; categories are path-based, not coverage measurements. ChangesAdds desktop discovery and detached launch, CLI dispatch and installation guidance, a README entry, and focused tests. There are no dependency or generated-file changes. The implementation is proportional to the outcome. Concrete reductions are simplifying macOS selection to CLI-first fallback and consolidating duplicated detached-launch setup. TestsTest-by-test assessmentExisting Cursor Agent forwarding, CLI branding, and MCP-preservation tests were inspected. They cover unchanged behavior; duplicating MCP persistence coverage is unnecessary. Unit tests suit platform selection and subprocess behavior; CLI component tests cover parsing, dispatch, and error presentation.
The concrete missing assertion is flag/path forwarding through the CLI parser. Native macOS behavior remains unverified, particularly fallback arguments when the app is already running. The PR reports 589 focused tests and lint passing; those results and the reported baseline failures were not independently reproduced. No tests or native application launches were executed during this review. For manual verification, run ScopeAll five changed files support opening and discovering the Cursor desktop app. No clearly unrelated changes were found. The widened CLI exception handler also affects ordinary The frozen snapshot’s checksum was verified, and both trusted policy files were read in full. Surrounding context came from the supplied trusted default-branch checkout rather than the exact PR base; findings on changed behavior are grounded in the frozen diff. Automated review by Polly · workflow run |
1 participant
Problem
There is no
ugentry point for the Cursor desktop app, which reads the same~/.cursor/mcp.jsonthatug mcp addwrites.Change
ug cursor app [args]opens the Cursor desktop app. It uses thecursorshell command when on PATH, falls back toopen -a Cursor.appon macOS, and prints an install hint when the app is not found. README updated.Testing
tests/test_agent_cursor.pyandtests/test_cli.py.uv run pytest -q tests/test_agent_cursor.py tests/test_cli.py: 589 passed.uvx --from rust-just just lint: passes (ruff check, ruff format --check, ty check).tests/test_agents_init.pyandtests/test_e2e_user_agent.pyfail onmaintoo, independent of this change.Not yet verified
Before / after
ug cursor --help, before (main):After (this branch): the help points to
ug cursor app:This pull request and its description were written by Isaac.