diff --git a/README.md b/README.md index 9aabc55f..15f5ac4d 100644 --- a/README.md +++ b/README.md @@ -148,7 +148,7 @@ Supported terminals: gnome-terminal, ptyxis, wezterm, blackbox, tmux, konsole, a Flatpak-installed terminals are also supported using their application ID (e.g., `org.kde.konsole`). -**macOS:** Terminal.app is always available. iTerm2 is detected when installed (`/Applications/iTerm.app`). Terminals installed as `.app` bundles (kitty, alacritty, wezterm) are auto-detected from `/Applications/` even when not on PATH. AppleScript terminals use wrapper scripts (`~/.cache/srepd/launch/`) for correct environment variable passing; stale scripts are cleaned up automatically. +**macOS:** Terminal.app is always available. iTerm2 is detected when installed. Terminals installed as `.app` bundles (kitty, alacritty, wezterm) are auto-detected from `/Applications/` and `~/Applications/` even when not on PATH. AppleScript terminals use wrapper scripts (`~/.cache/srepd/launch/`) for correct environment variable passing; stale scripts are cleaned up automatically. If macOS TCC blocks terminal automation, srepd shows an actionable error with remediation steps. See [docs/terminals.md](docs/terminals.md) for full details. When running inside a Fedora Toolbox, terminal commands are automatically prefixed with `flatpak-spawn --host` (controlled by `toolbox_mode`). diff --git a/docs/plans/416-macos-terminal-coverage.md b/docs/plans/416-macos-terminal-coverage.md index b7366da1..7013ef09 100644 --- a/docs/plans/416-macos-terminal-coverage.md +++ b/docs/plans/416-macos-terminal-coverage.md @@ -95,14 +95,8 @@ substitution, and configuration examples. - `SREPD_TEST_WRAPPER_DIR` is a test-only hook for wrapper script directory isolation; not intended as user-facing configuration. -- `~/Applications/` is not probed for bundle detection or iTerm2 - validation — only `/Applications/`. Per-user app installs are uncommon - enough that this is future work if users request it. -- Ghostty macOS bundle needs an `open -na Ghostty.app`-based launch - profile; currently only works when on `$PATH`. -- TCC error translation: macOS permission denials produce cryptic - `-1743` AppleScript errors. Future work to detect and surface a - user-friendly message. +- Ghostty macOS bundle needs a dedicated AppleScript profile (>=1.3); + currently only works when on `$PATH`. Planned for a follow-up PR. ## macOS testing note diff --git a/docs/plans/417-macos-user-apps-tcc-translation.md b/docs/plans/417-macos-user-apps-tcc-translation.md new file mode 100644 index 00000000..65580812 --- /dev/null +++ b/docs/plans/417-macos-user-apps-tcc-translation.md @@ -0,0 +1,66 @@ +# Plan 417: ~/Applications probing + TCC error translation + +**Branch:** `srepd/macos-terminal-phase3` + +## Problem + +Two remaining gaps from the Phase 3 macOS terminal plan: + +1. **`~/Applications/` not probed:** Bundle detection and iTerm2 validation + only check `/Applications/`. macOS users who install apps to their + per-user `~/Applications/` directory are not detected. +2. **TCC errors cryptic:** macOS TCC (Transparency, Consent, and Control) + permission denials produce raw osascript errors like `-1743` in the + error modal. Users don't know what to do about them. + +## Solution + +### 1a. Probe ~/Applications for bundles + +Changed `macOSBundleTerminal` struct to store `appName` (e.g., "kitty.app") +and `binaryRelPath` (e.g., "Contents/MacOS/kitty") instead of full paths. +`DetectTerminals` builds candidate roots `[/Applications, $HOME/Applications]` +using the injected `getenv("HOME")`, probes in order, first hit wins. The +full binary path is derived by joining the matched root + appName + +binaryRelPath. + +Same two-root treatment for the iTerm2 check in `DetectTerminals`. + +### 1b. Validate iTerm2 in both locations + +`validateTerminalExists` for iterm2 now checks both `/Applications/iTerm.app` +and `~/Applications/iTerm.app` before warning. + +### 2. Translate TCC / osascript failures + +New `translateLoginStderr(stderr string) string` in `commands.go` — a pure +function that returns appended guidance when known macOS error patterns match: + +- `-1743` / "Not authorized to send Apple events": guidance about System + Settings > Privacy & Security > Automation, toggle workaround, and + `tccutil reset AppleEvents`. +- `-600` / "application isn't running" / `-10810`: guidance to verify the + app is installed and can be opened manually. + +Wired into the `loginProcessExitedMsg` handler in `tui.go`: translation +appends to the detail string, never replaces the raw stderr. + +## Files changed + +| File | Change | +|------|--------| +| `pkg/launcher/detect.go` | `macOSBundleTerminal` struct refactored to relative paths; `~/Applications` probing; iTerm2 two-root check | +| `pkg/launcher/detect_test.go` | 4 new tests: user-home bundle, system precedence, no-HOME skip, iTerm2 from ~/Applications | +| `pkg/launcher/profiles.go` | `validateTerminalExists` checks both iTerm.app locations | +| `pkg/tui/commands.go` | `translateLoginStderr` pure function | +| `pkg/tui/commands_test.go` | Table-driven tests for `translateLoginStderr` | +| `pkg/tui/tui.go` | Wire `translateLoginStderr` into `loginProcessExitedMsg` handler | +| `pkg/tui/cluster_login_test.go` | Handler-level test for TCC -1743 guidance | +| `docs/terminals.md` | Updated detection docs, TCC section, removed resolved limitations | +| `docs/plans/416-macos-terminal-coverage.md` | Removed resolved limitations | + +## macOS testing note + +No macOS device available. Validated by unit tests with injectable +`fakeStat`/`fakeGetenv`/`fakeGetenv(HOME)` for detection, pure function +tests for TCC translation, and handler-level test for the wiring. diff --git a/docs/terminals.md b/docs/terminals.md index 282f28d6..3f69f2cd 100644 --- a/docs/terminals.md +++ b/docs/terminals.md @@ -64,28 +64,28 @@ SREPD prepends `flatpak run` automatically. Recognized Flatpak app IDs: **Detection:** - **Terminal.app** is always available (built into macOS). -- **iTerm2** is offered only when `/Applications/iTerm.app` exists. +- **iTerm2** is offered only when `iTerm.app` exists in `/Applications/` + or `~/Applications/`. - **kitty, Alacritty, WezTerm** are detected by checking for their - `.app` bundle in `/Applications/`. When found, the config value is - set to the full binary path inside the bundle (e.g., + `.app` bundle in `/Applications/` and `~/Applications/` (in that + order; `/Applications/` takes precedence). When found, the config + value is set to the full binary path inside the bundle (e.g., `/Applications/kitty.app/Contents/MacOS/kitty`) so that `exec.Command` can find the binary even when it's not on `$PATH`. If the terminal IS on `$PATH` (e.g., via Homebrew), the bare name is used instead. - **Ghostty** is not currently supported for bundle detection. Its - macOS CLI cannot reliably launch terminal windows — the supported - route is `open -na Ghostty.app`, which needs its own launch profile. - If Ghostty is on `$PATH`, it works normally via the flag profile. + macOS CLI cannot reliably launch terminal windows. Ghostty >=1.3 + supports AppleScript, which a future PR will add as a dedicated + profile. If Ghostty is on `$PATH`, it works via the flag profile. -**Current limitations:** +**TCC (Transparency, Consent, and Control):** -- Only `/Applications/` is probed for bundles, not `~/Applications/`. -- `validateTerminalExists` checks `/Applications/iTerm.app` for iterm2 - but does not check `~/Applications/iTerm.app`. -- macOS TCC (Transparency, Consent, and Control) may block AppleScript - automation. If you see a `-1743` error or the terminal doesn't open, - grant Automation permission in System Settings > Privacy & Security > - Automation for the application running srepd. +macOS TCC may block AppleScript automation on the first use. When this +happens, srepd shows an actionable error message explaining how to +grant permission in System Settings > Privacy & Security > Automation. +If the permission toggle is missing or stuck, toggling it off/on or +running `tccutil reset AppleEvents` in a terminal can help. ### Fedora Toolbox @@ -243,10 +243,6 @@ flatpak run org.kde.konsole -e ocm backplane login abc-123` ## Known Limitations and Future Work -- **`~/Applications/` not probed:** macOS bundle detection only checks - `/Applications/`, not per-user `~/Applications/`. -- **Ghostty macOS bundle:** Needs an `open -na`-based launch profile; - currently only works when installed on `$PATH`. -- **TCC error translation:** macOS TCC permission denials produce a - cryptic `-1743` AppleScript error. A future change could detect this - and show a user-friendly message explaining what to allow. +- **Ghostty macOS bundle:** Ghostty >=1.3 supports AppleScript, which + will be added as a dedicated profile in a future PR. Until then, + Ghostty only works on macOS when installed on `$PATH`. diff --git a/pkg/launcher/detect.go b/pkg/launcher/detect.go index 56779ffa..2497a430 100644 --- a/pkg/launcher/detect.go +++ b/pkg/launcher/detect.go @@ -2,6 +2,7 @@ package launcher import ( "os" + "path/filepath" "sort" ) @@ -44,43 +45,37 @@ var termProgramNames = map[string]string{ } // macOSBundleTerminal describes a terminal installed as a macOS .app bundle. +// appName is the .app directory name and binaryRelPath is the executable +// path relative to the bundle root (e.g., "Contents/MacOS/kitty"). +// Detection probes /Applications/ and $HOME/Applications/ +// in that order; the first hit wins. type macOSBundleTerminal struct { - appPath string // /Applications/.app — stat'd to detect installation - binaryPath string // full path to the executable inside the bundle + appName string // e.g., "kitty.app" + binaryRelPath string // e.g., "Contents/MacOS/kitty" } // macOSBundleTerminals maps terminal names to their bundle metadata. -// Command is set to binaryPath so exec.Command can find the binary even -// when it's not on PATH. DetectTerminalProfile resolves the profile from -// filepath.Base, so full paths work transparently. +// DetectTerminalProfile resolves the profile from filepath.Base, so full +// binary paths work transparently. // -// Ghostty is intentionally excluded: its macOS CLI cannot reliably launch -// the terminal — the supported route is `open -na Ghostty.app`, which -// needs its own profile. See ghostty#5739, #10203. +// Ghostty is intentionally excluded from this map: its macOS CLI cannot +// reliably launch terminal windows. Ghostty >=1.3 supports AppleScript, +// which will be handled by a dedicated profile. See ghostty#5739, #10203. var macOSBundleTerminals = map[string]macOSBundleTerminal{ - "alacritty": { - appPath: "/Applications/Alacritty.app", - binaryPath: "/Applications/Alacritty.app/Contents/MacOS/alacritty", - }, - "kitty": { - appPath: "/Applications/kitty.app", - binaryPath: "/Applications/kitty.app/Contents/MacOS/kitty", - }, - "wezterm": { - appPath: "/Applications/WezTerm.app", - binaryPath: "/Applications/WezTerm.app/Contents/MacOS/wezterm", - }, + "alacritty": {appName: "Alacritty.app", binaryRelPath: "Contents/MacOS/alacritty"}, + "kitty": {appName: "kitty.app", binaryRelPath: "Contents/MacOS/kitty"}, + "wezterm": {appName: "WezTerm.app", binaryRelPath: "Contents/MacOS/wezterm"}, } // DetectTerminals probes this system for known terminal emulators and // returns them ranked: the terminal identified by $TERM_PROGRAM first, tmux // next when running inside a session, then the rest in probe order. On // darwin, Terminal.app is always a candidate (built into macOS); iTerm2 is -// offered only when /Applications/iTerm.app exists; and bundle-installed -// terminals (kitty, alacritty, wezterm) are detected from /Applications/ -// with their full binary path as Command. lookPath, getenv, goos, and -// statFn are injectable for tests; production callers pass exec.LookPath, -// os.Getenv, runtime.GOOS, os.Stat. +// offered only when iTerm.app exists; and bundle-installed terminals +// (kitty, alacritty, wezterm) are detected from /Applications/ and +// ~/Applications/ with their full binary path as Command. lookPath, getenv, +// goos, and statFn are injectable for tests; production callers pass +// exec.LookPath, os.Getenv, runtime.GOOS, os.Stat. func DetectTerminals(lookPath func(string) (string, error), getenv func(string) string, goos string, statFn func(string) (os.FileInfo, error)) []DetectedTerminal { var found []DetectedTerminal foundSet := make(map[string]bool) @@ -101,6 +96,11 @@ func DetectTerminals(lookPath func(string) (string, error), getenv func(string) } if goos == "darwin" { + bundleRoots := []string{"/Applications"} + if home := getenv("HOME"); home != "" { + bundleRoots = append(bundleRoots, filepath.Join(home, "Applications")) + } + // Check for bundle-installed terminals not found via PATH. bundleNames := make([]string, 0, len(macOSBundleTerminals)) for name := range macOSBundleTerminals { @@ -113,9 +113,14 @@ func DetectTerminals(lookPath func(string) (string, error), getenv func(string) continue } bundle := macOSBundleTerminals[name] - if _, err := statFn(bundle.appPath); err == nil { - found = append(found, DetectedTerminal{Name: name, Command: bundle.binaryPath}) - foundSet[name] = true + for _, root := range bundleRoots { + appPath := filepath.Join(root, bundle.appName) + if _, err := statFn(appPath); err == nil { + binaryPath := filepath.Join(appPath, bundle.binaryRelPath) + found = append(found, DetectedTerminal{Name: name, Command: binaryPath}) + foundSet[name] = true + break + } } } @@ -123,10 +128,13 @@ func DetectTerminals(lookPath func(string) (string, error), getenv func(string) found = append(found, DetectedTerminal{Name: "terminal", Command: "terminal"}) foundSet["terminal"] = true - // iTerm2 only when installed. - if _, err := statFn("/Applications/iTerm.app"); err == nil { - found = append(found, DetectedTerminal{Name: "iterm2", Command: "iterm2"}) - foundSet["iterm2"] = true + // iTerm2 only when installed (check both roots). + for _, root := range bundleRoots { + if _, err := statFn(filepath.Join(root, "iTerm.app")); err == nil { + found = append(found, DetectedTerminal{Name: "iterm2", Command: "iterm2"}) + foundSet["iterm2"] = true + break + } } } diff --git a/pkg/launcher/detect_test.go b/pkg/launcher/detect_test.go index af3142e4..2860a8e1 100644 --- a/pkg/launcher/detect_test.go +++ b/pkg/launcher/detect_test.go @@ -174,6 +174,82 @@ func TestDetectTerminals_DarwinBundleSkippedOnLinux(t *testing.T) { "bundle detection should not run on linux") } +func TestDetectTerminals_DarwinBundleDetectionUserHome(t *testing.T) { + dts := DetectTerminals( + fakeLookPath(), + fakeGetenv(map[string]string{"HOME": "/Users/testuser"}), + "darwin", + fakeStat( + "/Users/testuser/Applications/kitty.app", + "/Users/testuser/Applications/Alacritty.app", + ), + ) + n := names(dts) + assert.Contains(t, n, "kitty", "kitty should be detected from ~/Applications bundle") + assert.Contains(t, n, "alacritty", "alacritty should be detected from ~/Applications bundle") + + for _, dt := range dts { + switch dt.Name { + case "kitty": + assert.Equal(t, "/Users/testuser/Applications/kitty.app/Contents/MacOS/kitty", dt.Command, + "~/Applications bundle-detected kitty must use full binary path") + case "alacritty": + assert.Equal(t, "/Users/testuser/Applications/Alacritty.app/Contents/MacOS/alacritty", dt.Command, + "~/Applications bundle-detected alacritty must use full binary path") + } + } +} + +func TestDetectTerminals_DarwinSystemBundleTakesPrecedenceOverUserHome(t *testing.T) { + dts := DetectTerminals( + fakeLookPath(), + fakeGetenv(map[string]string{"HOME": "/Users/testuser"}), + "darwin", + fakeStat( + "/Applications/kitty.app", + "/Users/testuser/Applications/kitty.app", + ), + ) + n := names(dts) + count := 0 + for _, name := range n { + if name == "kitty" { + count++ + } + } + assert.Equal(t, 1, count, "kitty should appear exactly once even when in both /Applications and ~/Applications") + + for _, dt := range dts { + if dt.Name == "kitty" { + assert.Equal(t, "/Applications/kitty.app/Contents/MacOS/kitty", dt.Command, + "/Applications should take precedence over ~/Applications") + } + } +} + +func TestDetectTerminals_DarwinUserHomeBundleSkippedWithoutHOME(t *testing.T) { + dts := DetectTerminals( + fakeLookPath(), + fakeGetenv(nil), + "darwin", + fakeStat("/Users/testuser/Applications/kitty.app"), + ) + n := names(dts) + assert.NotContains(t, n, "kitty", + "~/Applications probing should not run when HOME is empty") +} + +func TestDetectTerminals_DarwinITerm2FromUserHome(t *testing.T) { + dts := DetectTerminals( + fakeLookPath(), + fakeGetenv(map[string]string{"HOME": "/Users/testuser"}), + "darwin", + fakeStat("/Users/testuser/Applications/iTerm.app"), + ) + assert.Contains(t, names(dts), "iterm2", + "iTerm2 should be detected from ~/Applications/iTerm.app") +} + func TestDetectTerminals_PATHTakesPrecedenceOverBundle(t *testing.T) { dts := DetectTerminals( fakeLookPath("kitty"), diff --git a/pkg/launcher/profiles.go b/pkg/launcher/profiles.go index 35282267..173be6e0 100644 --- a/pkg/launcher/profiles.go +++ b/pkg/launcher/profiles.go @@ -345,8 +345,12 @@ func validateTerminalExists(terminal string) string { return fmt.Sprintf("osascript command not found in PATH; cluster login via %s may fail (requires macOS)", name) } if strings.ToLower(name) == "iterm2" { - if _, err := os.Stat("/Applications/iTerm.app"); err != nil { - return "iTerm2.app not found in /Applications; cluster login via iterm2 may fail" + systemPath := "/Applications/iTerm.app" + userPath := filepath.Join(os.Getenv("HOME"), "Applications", "iTerm.app") + if _, err := os.Stat(systemPath); err != nil { + if _, err := os.Stat(userPath); err != nil { + return "iTerm2.app not found in /Applications or ~/Applications; cluster login via iterm2 may fail" + } } } return "" diff --git a/pkg/tui/cluster_login_test.go b/pkg/tui/cluster_login_test.go index c827d637..2adfdf6e 100644 --- a/pkg/tui/cluster_login_test.go +++ b/pkg/tui/cluster_login_test.go @@ -240,6 +240,24 @@ func TestLoginProcessExitedMsg_WithErrorAndStderr(t *testing.T) { }) } +func TestLoginProcessExitedMsg_TCCDenialShowsGuidance(t *testing.T) { + t.Run("TCC -1743 error includes Automation guidance", func(t *testing.T) { + m := createTestModel() + + exitErr := errors.New("exit status 1") + stderr := "execution error: System Events got an error: osascript is not allowed to send keystrokes. (-1743)" + _, cmd := m.Update(loginProcessExitedMsg{exitErr: exitErr, stderr: stderr}) + + assert.NotNil(t, cmd, "should return a command wrapping errMsg") + msg := cmd() + em, ok := msg.(errMsg) + assert.True(t, ok, "returned command should produce errMsg") + assert.Contains(t, em.Error(), "-1743", "raw stderr preserved") + assert.Contains(t, em.Error(), "Privacy & Security", "should include TCC guidance") + assert.Contains(t, em.Error(), "Automation", "should mention Automation settings") + }) +} + func TestLoginProcessExitedMsg_Success(t *testing.T) { t.Run("no-op when process exits cleanly", func(t *testing.T) { m := createTestModel() diff --git a/pkg/tui/commands.go b/pkg/tui/commands.go index f91a9a7b..ca87de11 100644 --- a/pkg/tui/commands.go +++ b/pkg/tui/commands.go @@ -791,6 +791,25 @@ type loginProcessExitedMsg struct { stderr string } +// translateLoginStderr returns actionable guidance when stderr contains +// a known macOS error pattern. Returns empty if no pattern matches. +// The translation appends to, never replaces, the raw stderr. +func translateLoginStderr(stderr string) string { + lower := strings.ToLower(stderr) + if strings.Contains(stderr, "(-1743)") || strings.Contains(lower, "not authorized to send apple events") { + return "\n\nmacOS blocked the terminal automation request. " + + "Open System Settings > Privacy & Security > Automation and enable " + + "the toggle for the app you run srepd in. If it is already enabled " + + "(or missing), toggle it off then on. As a last resort, run " + + "`tccutil reset AppleEvents` in a terminal and retry." + } + if strings.Contains(stderr, "(-600)") || strings.Contains(lower, "application isn't running") || strings.Contains(stderr, "(-10810)") { + return "\n\nmacOS could not launch the terminal application. " + + "Please verify it is installed and can be opened manually." + } + return "" +} + // buildPagerDutyEnvVars constructs a slice of "-e", "KEY=VALUE" pairs for passing // PagerDuty incident context to ocm-container as individual environment variables. // Only alerts whose cluster_id matches clusterID contribute to ALERT_NAMES and diff --git a/pkg/tui/commands_test.go b/pkg/tui/commands_test.go index cd444862..c30f0da4 100644 --- a/pkg/tui/commands_test.go +++ b/pkg/tui/commands_test.go @@ -3199,3 +3199,64 @@ func TestUpdatedIncidentList_AutoAck_DispatchesFreshOnCallCheck(t *testing.T) { assert.GreaterOrEqual(t, mockClient.CallCounts["ListOnCallsWithContext"], 1, "on-call status must be checked live (ListOnCalls called), never cached") } + +func TestTranslateLoginStderr(t *testing.T) { + tests := []struct { + name string + stderr string + contains string + empty bool + }{ + { + name: "TCC denial with error code -1743", + stderr: "execution error: System Events got an error: osascript is not allowed to send keystrokes. (-1743)", + contains: "Privacy & Security", + }, + { + name: "TCC denial with text", + stderr: "Not authorized to send Apple events to Terminal", + contains: "Privacy & Security", + }, + { + name: "application not running error (-600)", + stderr: "execution error: iTerm2 got an error: Application isn't running. (-600)", + contains: "verify it is installed", + }, + { + name: "application not running text only (capital A)", + stderr: "Application isn't running.", + contains: "verify it is installed", + }, + { + name: "launch failure error (-10810)", + stderr: "execution error: An error of type (-10810) has occurred.", + contains: "verify it is installed", + }, + { + name: "bare -600 without parens does not match", + stderr: "error code -600 encountered", + empty: true, + }, + { + name: "unrecognized stderr", + stderr: "some other osascript error", + empty: true, + }, + { + name: "empty stderr", + stderr: "", + empty: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := translateLoginStderr(tt.stderr) + if tt.empty { + assert.Empty(t, result, "should return empty for unrecognized stderr") + } else { + assert.Contains(t, result, tt.contains) + } + }) + } +} diff --git a/pkg/tui/tui.go b/pkg/tui/tui.go index ea60beac..b2e5edda 100644 --- a/pkg/tui/tui.go +++ b/pkg/tui/tui.go @@ -1483,6 +1483,7 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { if msg.stderr != "" { detail = fmt.Sprintf("%s: %s", detail, msg.stderr) } + detail += translateLoginStderr(msg.stderr) loginErr := fmt.Errorf("terminal exited with error: %s", detail) log.Debug("tui.loginProcessExitedMsg()", "error", msg.exitErr, "stderr", msg.stderr) return m, func() tea.Msg { return errMsg{loginErr} }