fix(tui): guard raiser-side multiplexer calls in action handlers#148
Conversation
Any raiser-side seam call (a method that raises MultiplexerError on a transport failure — has_session and friends) made from a Textual action handler with no try/except propagates out of the handler and crashes the whole TUI. Commit 8ed66e8 fixed one instance (attach_target_argv); this closes the rest of the class. Auditing every handler, only calls routing to has_session were both unguarded and reachable — the others are already guarded (attach_target_argv, new_* via LaunchError), contract-sentinel-side (available, list_windows, current_pane_id, select_window, kill_window, target, …), or not reached from the TUI (send_text, list_window_ids, window_alive). Three sites, three contexts: - launch._ensure_ctl_session: has_session sat outside the existing try, so its MultiplexerError escaped as-is and slipped past the launch/resume/ resolve handlers' `except LaunchError`. Moved inside → converts to LaunchError, hardening six handlers with no app.py change. - action_attach: the session_exists probe (the issue's named crash) now routes through a new _mux_guarded helper that toasts and aborts. - _cleanup_sessions_worker: prune_ctl_windows (probes has_session on the shared ctl session) guarded on the worker thread with a call_from_thread toast + return. The _mux_guarded helper unifies the foreground convention: it backs both the new action_attach guard and the refactored _attach_to_target guard. Fix A (LaunchError) and Fix C (worker call_from_thread) stay inline where the helper can't apply. Tests: one behavioral test per fix, modeled on the existing idioms. Closes #143
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe TUI now centralizes foreground multiplexer error handling, reports cleanup failures safely from the worker thread, and converts control-session setup failures into ChangesTUI multiplexer error handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bmad_loop/tui/app.py`:
- Around line 929-936: Update the MultiplexerError handler in the worker cleanup
flow around launch.prune_ctl_windows so it sets windows to an empty list after
notifying the error instead of returning. Allow subsequent reporting logic to
continue and include sessions already processed by runs.prune_sessions alongside
the error toast.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ecccc7e-fb79-4a65-9f0a-f1c409788ffb
📒 Files selected for processing (4)
src/bmad_loop/tui/app.pysrc/bmad_loop/tui/launch.pytests/test_tui_app.pytests/test_tui_launch.py
Review follow-up (CodeRabbit on #148): the _cleanup_sessions_worker guard returned early when prune_ctl_windows raised MultiplexerError, but runs.prune_sessions had already killed the agent sessions by then. The early return swallowed the "removed N session(s)" summary and, more importantly, the "unverifiable engine pid (may still be live)" warning for pruned unknown-pid sessions. Set windows = [] instead of returning so the reporting tail still runs — the ctl-window failure is surfaced as an error toast alongside the completed session work. prune_ctl_windows raises in _ctl_window_candidates before any kill_window, so zero windows are removed and [] is an honest count. Strengthen test_cleanup_sessions_mux_error_notifies: it previously stubbed prune_sessions to empties and only checked the error toast, so it passed with or without the fix. It now returns a non-empty killed+unknown partition and asserts the error toast, the unknown-pid warning, and the summary all land.
1 participant
Summary
Closes #143. Any raiser-side multiplexer seam call (one that raises
MultiplexerErroron a transport failure —has_sessionand friends) made from a Textual action handler with notry/exceptpropagates out of the handler and crashes the whole TUI. Commit8ed66e8fixed one instance (attach_target_argvin_attach_to_target); this PR closes the rest of the class.Auditing every handler, only calls routing to
has_sessionwere both unguarded and reachable. The other raiser-side methods are already guarded (attach_target_argv;new_*→LaunchError), contract-sentinel-side (available,list_windows,current_pane_id,select_window,kill_window,target, …), or not reached from the TUI at all (send_text,list_window_ids,window_alive— engine/probe only). The dashboard poll'shas_sessionwas already guarded (tui/data.py).Three fix sites, three contexts
launch._ensure_ctl_sessionhas_sessionsat outside the existingtry, so itsMultiplexerErrorescaped as-is and slipped past the launch/resume/resolve handlers'except launch.LaunchError. Moved inside thetry→ converts toLaunchError, hardening six handlers with zeroapp.pychange.action_attach(the issue's named line ~356)session_existsprobe now routes through a new_mux_guardedhelper that toasts the error and aborts the attach._cleanup_sessions_workerprune_ctl_windows(probeshas_sessionon the shared ctl session) is guarded on the worker thread with acall_from_threadtoast + return._mux_guardedhelperA new
BmadLoopApp._mux_guarded(probe) -> (ok, value)unifies the foreground convention: it backs both the newaction_attachguard and the refactored_attach_to_targetguard (behavior unchanged — the existingtest_attach_multiplexer_error_notifiesstill passes). Fix A (LaunchError conversion, launch layer) and Fix C (workercall_from_thread) stay inline where the helper can't apply — Textual forbidscall_from_threadfrom the main thread, so no single helper covers both.Out of scope
The CLI
cmd_attachpath (cli.py→launch.attach_plan→session_exists) is also unguarded, but this issue is TUI-scoped ("crashes the whole TUI"); a one-shot CLI surfacing a traceback + nonzero exit is a lesser problem. A tidy follow-up could mirror howcmd_muxalready handlesMultiplexerError.Tests
One behavioral test per fix, modeled on existing idioms:
test_ensure_ctl_session_probe_failure_raises_launch_error(mirrorstest_new_window_failure_raises): ahas-sessiontransport failure converts toLaunchError.test_attach_session_probe_error_notifies(mirrorstest_attach_multiplexer_error_notifies): a raisingsession_existsyields a toast and a soft-failed attach, app stays on the dashboard.test_cleanup_sessions_mux_error_notifies: a raisingprune_ctl_windowsin the worker yields a toast, no crash.Verification
ruffclean,trunk checkclean.Depends on #136/#137/#141/#142 (all merged).
Summary by CodeRabbit