Fix UI beachball when a review session spins up

[dgershman]

Summary

• Review-session kickoff was synchronously blocking the main actor on every gh/git step, producing the macOS spinning beachball for the duration of the worst step (often a full gh repo clone).
• SessionService is @MainActor, so even though createReviewSession() is async, the old shell() helper used Process.waitUntilExit() — which blocked the main thread instead of yielding.
• Two surgical fixes restore UI responsiveness throughout review kickoff.

Fix 1 — shell() truly yields

Replaced process.waitUntilExit() with withCheckedThrowingContinuation + process.terminationHandler, and marked shell() (and a new runShellAsync static) nonisolated. Now await shell(...) actually suspends and frees the main actor while the subprocess runs in the background.

Fix 2 — createReviewSession() runs git/file-prep off-main

Extracted the gh pr view / gh repo clone / git fetch/checkout/pull / prompt-write / skill-copy block into a new nonisolated static prepareReviewClone(...) helper invoked via Task.detached(priority: .userInitiated). The main-actor tail only handles the cheap state mutations and the prepareTerminal call (still bounded by the existing 2s tmux watchdog).

The kickoff serialization queue in AppDelegate.enqueueReviewKickoff is unchanged — multiple PRs still process in order; the awaits just no longer wedge the UI.

Closes #404

Test plan

• swift build --arch arm64 clean
• arch -arm64 swift test --arch arm64 — 145 tests pass
• Trigger a review on a PR whose repo is NOT yet cloned (worst-case full-clone path). Click around the sidebar / switch sessions during kickoff — no beachball, no input lag.
• Trigger a review on a PR whose repo IS already cloned (fast path). Sidebar stays responsive; new review session appears when ready.
• Enable autoReviewWatcherEnabled, push a commit to an open PR, observe the watcher fire a kickoff — UI stays responsive.
• Burst test: enqueue 3+ PRs in quick succession via enqueueReviewKickoff. UI stays responsive; sessions appear in deterministic order (queue serialization preserved).

🐦‍⬛ Generated with Claude Code, orchestrated by Crow

[dhilgaertner]

Code & Security Review

Critical Issues

None.

Summary

The fix correctly addresses the main-actor beachball (#404) with two well-scoped changes:

1. shell() truly yields — replacing waitUntilExit() with withCheckedThrowingContinuation + terminationHandler, and marking it nonisolated. shell() uses no instance state, so dropping main-actor isolation is safe; all existing callers (worktree list, remote get-url, gh issue view, gh pr view, repo clone) still await it correctly.
2. prepareReviewClone off-main — the gh/git/file-prep block moves into a nonisolated static helper run via Task.detached(.userInitiated), leaving only cheap state mutations on the main actor. env is captured as ShellEnvironment (public final class … : Sendable), and Scaffolder.bundled* / buildReviewPrompt are plain non-isolated statics, so the off-main hop is concurrency-clean.

Security Review

Strengths:

• No new external input surface; PR URL parsing and command args are unchanged from the prior implementation. No shell string interpolation — args are passed as an array to /usr/bin/env, so no command-injection vector is introduced.
• Error-message construction now prefers stderr and falls back to stdout; neither is surfaced to users beyond NSLog, consistent with prior behavior.

Concerns: None.

Code Quality

• Continuation safety is correct. terminationHandler is installed before process.run(), so the success path resumes exactly once. The catch on launch failure defensively clears terminationHandler = nil before resuming, preventing a double-resume — a nice touch that matches the documented macOS launch-failure edge case.
• Behavioral parity preserved. PR-metadata fetch failure aborts kickoff (throw → caught → return nil + NSLog), matching the old guard; git fetch/checkout/pull and clone errors remain tolerated via try?. Kickoff serialization in AppDelegate.enqueueReviewKickoff is untouched, so PR ordering is preserved.

Findings

Green — pipe read-after-exit comment is misleading (Sources/Crow/App/SessionService.swift:830-833, 845-853). The doc comment claims reads-after-exit means "no deadlock from a full pipe blocking the child." That reasoning is backwards: draining readDataToEndOfFile only after terminationHandler fires is precisely the pattern that can deadlock if a child writes more than the ~64KB pipe buffer to stdout/stderr and then blocks on write(), never exiting — so the handler never fires and the continuation never resumes. This is not a regression (the old waitUntilExit() path had the identical property) and is harmless for the current callers, whose output is small (gh/git to a non-tty emit minimal progress). But since runShellAsync is now a reusable static helper, consider either correcting the comment or draining the pipes concurrently (e.g. readabilityHandler/background reads) before reusing it for high-volume commands like gh pr diff on a large PR.

Green — no direct unit coverage for runShellAsync. Subprocess runners are awkward to unit-test, and the existing 145-test suite exercises the surrounding paths, so this is acceptable; a small test against a trivial /usr/bin/env true/false would still be cheap insurance for the exactly-once-resume contract.

Note: I could not build locally — swift build fails on a missing Frameworks/GhosttyKit.xcframework binary artifact, which is an environmental/checkout issue unrelated to this diff. The change is self-contained to one file; the PR reports a clean swift build --arch arm64 and 145 passing tests.

Summary Table

Color	Meaning	Verdict effect
Red	Must fix	Request changes
Yellow	Should fix	Request changes
Green	Consider	Approve allowed

Recommendation: Approve — driven by [0 Red, 0 Yellow, 2 Green] findings. Correct, concurrency-clean fix that resolves the beachball without altering kickoff ordering or behavior on error paths.

---

🐦‍⬛ Reviewed by Crow via Claude Code