fix(tui): stop swallowing tmux errors in startTuiTmuxServer - #1673
Merged
Merged
Conversation
|
Important Review skippedToo many files! This PR contains 291 files, which is 141 over the limit of 150. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (9)
📒 Files selected for processing (291)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR enhances TUI startup resilience in the serve command by introducing robust tmux session management with repair/recovery logic, new error logging, and comprehensive test coverage with a custom per-test execution handler for execSync mocking. ChangesTUI Startup Resilience & Test Coverage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request enhances the resilience of the TUI startup process by refactoring the startTuiTmuxServer logic. Key changes include separating the session probe from the repair logic, implementing detailed error capturing with runTuiTmuxCapturing, and adding a recovery path that logs failures to tui-crash.log before recreating corrupt sessions. Additionally, a robust mocking strategy was introduced in the test suite to verify these recovery scenarios. Feedback was provided to ensure that pane ID parsing is defensive against empty strings, maintaining consistency across the implementation.
Comment on lines
+336
to
+338
| const panes = execSync(tuiTmux(`list-panes -t ${TUI_SESSION}:0 -F '#{pane_id}'`), { encoding: 'utf-8' }) | ||
| .trim() | ||
| .split('\n'); |
Contributor
There was a problem hiding this comment.
The split('\n') call can result in an array containing an empty string if the command output has trailing newlines or is empty. Adding a filter for non-empty strings ensures that the resulting panes array contains only valid identifiers, maintaining consistency with the logic used in the repair branch at line 385.
Suggested change
| const panes = execSync(tuiTmux(`list-panes -t ${TUI_SESSION}:0 -F '#{pane_id}'`), { encoding: 'utf-8' }) | |
| .trim() | |
| .split('\n'); | |
| const panes = execSync(tuiTmux("list-panes -t " + TUI_SESSION + ":0 -F '#{pane_id}'"), { encoding: 'utf-8' }) | |
| .trim() | |
| .split('\n') | |
| .filter((id) => id.length > 0); |
![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/term-commands/serve.ts`:
- Around line 371-375: The catch around execSync(tuiTmux(`has-session -t
${TUI_SESSION}`)) treats all errors as "session missing" and wrongly calls
freshCreateTuiSession(); replace the execSync probe with
runTuiTmuxCapturing(tuiTmux(...)) and inspect the captured stderr/message to
only treat known tmux absence/connectivity errors (match with a regex such as
/can't find session|no server|failed to connect/i) as grounds to call
freshCreateTuiSession(), and rethrow any other unexpected errors so real
tmux/socket/permission issues surface; keep references to tuiTmux, TUI_SESSION,
freshCreateTuiSession, and runTuiTmuxCapturing to locate the change.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0d13bc12-7114-4aa9-bc45-0f4395776f5a
⛔ Files ignored due to path filters (1)
CHANGELOG.mdis excluded by!*.md
📒 Files selected for processing (2)
src/term-commands/serve.test.tssrc/term-commands/serve.ts
Comment on lines
371
to
+375
| try { | ||
| execSync(tuiTmux(`has-session -t ${TUI_SESSION}`), { stdio: 'ignore' }); | ||
| // Session exists — reuse it, but ensure the split is healthy | ||
| } catch { | ||
| // Session genuinely doesn't exist — create it from scratch. | ||
| return freshCreateTuiSession(); |
There was a problem hiding this comment.
❓ Verification inconclusive
What stderr/exit behavior does tmux has-session -t <name> use when (1) the session is missing, (2) no server is running on the socket, and (3) the probe fails for other reasons such as socket/config/permission errors?
tmux has-session -t <name> 1) Session is missing - Exit status: 1. - stderr: an error message indicating the session was not found (for example, “can’t find session ”). [1][2] 2) No server is running on the socket - Exit status: tmux reports a “failed to connect to server” / “no server running on …” type error, and returns a non-zero status (commonly 1). [3][4] - stderr: the “failed to connect to server …” / “no server running on …” message is printed to standard error. [3] 3) Probe fails for other reasons (socket/config/permission errors) - Exit status: non-zero (tmux cannot complete the target-session check). In practice, you’ll see a “failed to connect to server …” style error on stderr when the client can’t talk to the server (e.g., permissions/accessibility issues to the tmux socket). [5] - stderr: the connect/permission problem is surfaced as an error message on standard error (e.g., “failed to connect to server: Connection refused …”); tmux does not treat this as the “session missing” case. [5] Practical takeaway for scripts - Treat “exit code 0” as “session exists (and the probe succeeded)”. - Treat “exit code 1 or other non-zero” as “either session missing or the client couldn’t reach/probe the server/sockets (socket/config/permission problems).” [1][3][5]
Citations:
- 1: https://man7.org/linux/man-pages/man1/tmux.1.html
- 2: has-session fails when multiple session names match pattern tmux/tmux#1618
- 3: https://superuser.com/questions/697660/tmux-check-if-server-is-up
- 4: https://tmuxai.dev/tmux-failed-to-connect-to-server
- 5: https://stackoverflow.com/questions/9668763/why-am-i-getting-a-failed-to-connect-to-server-message-from-tmux-when-i-try-to
Script executed:
# Check if runTuiTmuxCapturing exists and its implementation
rg -A 10 "function runTuiTmuxCapturing|const runTuiTmuxCapturing" src/term-commands/serve.tsRepository: automagik-dev/genie
Repository: automagik-dev/genie
Exit code: 0
stdout:
function runTuiTmuxCapturing(cmd: string): string {
try {
return execSync(tuiTmux(cmd), { encoding: 'utf-8', stdio: ['ignore', 'pipe', 'pipe'] });
} catch (err) {
const e = err as { stderr?: string | Buffer; stdout?: string | Buffer; message?: string };
const stderr = typeof e.stderr === 'string' ? e.stderr : e.stderr?.toString('utf-8');
const stdout = typeof e.stdout === 'string' ? e.stdout : e.stdout?.toString('utf-8');
const detail = (stderr ?? stdout ?? e.message ?? 'unknown tmux error').trim();
throw new Error(`tmux ${cmd}: ${detail}`);
}
}
Script executed:
# Get line counts and check overall structure around the reviewed lines
wc -l src/term-commands/serve.ts
sed -n '280,290p' src/term-commands/serve.ts
sed -n '360,380p' src/term-commands/serve.tsRepository: automagik-dev/genie
Repository: automagik-dev/genie
Exit code: 0
stdout:
1404 src/term-commands/serve.ts
/**
* Run a tmux command via execSync and re-throw with the captured stderr
* embedded in the message. Bun's execSync throws an `Error` whose default
* message is the unhelpful `output: [null, null, null]` when stdio is
* ignored — capturing stdout/stderr as strings gives us tmux's actual
* complaint (e.g. `duplicate session: genie-tui`).
*/
function runTuiTmuxCapturing(cmd: string): string {
try {
return execSync(tuiTmux(cmd), { encoding: 'utf-8', stdio: ['ignore', 'pipe', 'pipe'] });
} catch (err) {
* `new-session` and crashed with `duplicate session: genie-tui`.
* 2. If the session exists, run the repair branch in its own try. On
* failure: log to ~/.genie/logs/tui-crash.log, kill the corrupt
* session, and rebuild from scratch — the user just typed `genie`
* and wants a working TUI.
*
* Exported so unit tests can exercise it directly without monkey-patching
* `child_process` globally.
*/
export function startTuiTmuxServer(): { leftPane: string; rightPane: string } {
// Step 1: probe — the ONLY signal that decides "fresh create" vs "repair".
try {
execSync(tuiTmux(`has-session -t ${TUI_SESSION}`), { stdio: 'ignore' });
} catch {
// Session genuinely doesn't exist — create it from scratch.
return freshCreateTuiSession();
}
// Step 2: session exists — try to repair / reuse. Any throw in this branch
// means the session is in a bad state we can't reason about: log, kill,
// and rebuild rather than bubbling an opaque error to the user.
Gate the fallback on actual tmux error messages, not a blanket catch.
This catch silently treats all has-session failures as "session missing". If the probe fails for socket/connection/permission issues, you incorrectly call freshCreateTuiSession() instead of surfacing the real tmux error.
runTuiTmuxCapturing is already defined in this file and captures stderr. Switch to it and inspect the error message to distinguish acceptable failures (session missing, server absent, connection lost) from unexpected ones.
The regex pattern in the suggested fix should also cover "failed to connect to server" messages; refine it to /can't find session|no server|failed to connect/i or similar to handle all acceptable cases before rethrowing unexpected errors.
🧰 Tools
🪛 OpenGrep (1.20.0)
[ERROR] 372-372: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 Prompt for 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.
In `@src/term-commands/serve.ts` around lines 371 - 375, The catch around
execSync(tuiTmux(`has-session -t ${TUI_SESSION}`)) treats all errors as "session
missing" and wrongly calls freshCreateTuiSession(); replace the execSync probe
with runTuiTmuxCapturing(tuiTmux(...)) and inspect the captured stderr/message
to only treat known tmux absence/connectivity errors (match with a regex such as
/can't find session|no server|failed to connect/i) as grounds to call
freshCreateTuiSession(), and rethrow any other unexpected errors so real
tmux/socket/permission issues surface; keep references to tuiTmux, TUI_SESSION,
freshCreateTuiSession, and runTuiTmuxCapturing to locate the change.
The bare `catch {}` after `has-session` previously swallowed every error
from the repair branch and fell through to `new-session -d -s genie-tui`,
which then failed with `duplicate session: genie-tui` whenever the
session existed but had an unexpected layout. Combined with
`stdio: 'ignore'`, the user saw an opaque `output: [null, null, null]`
traceback from Bun's execSync shim.
This commit:
- Splits the `has-session` probe from the repair branch so `new-session`
is reachable ONLY when the session genuinely doesn't exist.
- On repair-branch failure, logs the original error to
`~/.genie/logs/tui-crash.log` (prefixed `[startTuiTmuxServer] <ISO>`),
kills the corrupt session, and rebuilds via `freshCreateTuiSession`.
- Replaces `stdio: 'ignore'` on `new-session`/`split-window` with
captured stdout/stderr; on failure re-throws an `Error` whose message
embeds tmux's actual stderr (e.g. `duplicate session: genie-tui`).
- Extracts the fresh-create steps into a dedicated helper to remove the
duplication between the new and recovery paths.
- Exports `startTuiTmuxServer` so the G2 unit test can drive it directly
without monkey-patching `child_process` globally.
Wish: genie-tui-startup-resilience (G1).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four cases covering every reachable state of the function:
(a) session exists, 2 panes → returns early; new-session never called
(b) session exists, 1 pane → split-window once; no new-session
(c) has-session fails → fresh new-session + split-window
(d) repair branch throws → kill-session + fresh recreate;
~/.genie/logs/tui-crash.log gets a
[startTuiTmuxServer] <ISO> line containing
the original tmux stderr
The bug being locked in: a single bare catch {} used to swallow every
inner-branch failure and fall through to new-session -d -s genie-tui,
which then collided with the still-existing session and crashed with
"duplicate session: genie-tui" surfaced as opaque "output: [null,null,null]"
through Bun's execSync shim.
Mocking: file-level mock.module('node:child_process', ...) installs a
per-test command handler that intercepts only -L genie-tui invocations
and lets every other execSync call (pgserve setup, etc.) hit the real
child_process module. GENIE_HOME points to a tmpdir for case (d) so the
crash-log assertion is hermetic.
Wish: genie-tui-startup-resilience
automagik-genie
force-pushed
the
fix/tui-startup-resilience
branch
from
May 6, 2026 20:10
ab822bd to
bbf0982
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
2 of 5 tasks
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
startTuiTmuxServersonew-session -d -s genie-tuiis only reachable when no session exists or after a recoverykill-sessionstdio: 'ignore'onnew-session/split-window) so users seeduplicate session: genie-tuiinstead of opaqueoutput: [ null, null, null ]~/.genie/logs/tui-crash.log(prefix[startTuiTmuxServer] <ISO>to disambiguate fromsendTuiLaunchScript's native-panic appends), runningkill-session, thenfreshCreateTuiSession()startTuiTmuxServeras a test seam; add a 4-case regression suite locking in every reachable stateOriginal bug
With a stale
genie-tuitmux session present,genie(no args) crashed with:Root cause: a single
try { has-session; repair } catch {}block swallowed every inner failure (split-window, applyTuiStyle, etc.) and fell through tonew-session, which collided with the still-existing session.stdio: 'ignore'hid tmux'sduplicate session: genie-tuistderr, leaving Bun's execSync shim to emit the opaqueoutput: [null, null, null].Reproduced directly:
Approach
has-sessionis now the only signal that decides "fresh create" vs "repair". Non-existence is the only path intonew-session.runTuiTmuxCapturinghelper — wrapsexecSyncfor the failure-prone calls withstdio: ['ignore', 'pipe', 'pipe'], re-throws asError('tmux <cmd>: <stderr>')so the user sees tmux's actual complaint.freshCreateTuiSessionhelper — extracts the new-session / split-window / list-panes / style / keybindings flow, used both for first-time creation and post-kill recovery.logTuiStartupFailure— best-effort append to~/.genie/logs/tui-crash.log, never throws.kill-sessionrecovery is safe: the genie-tui session is display-only (sendTuiLaunchScriptrewrites both panes from scratch on every TUI entry), no work is lost.Test plan
bun test src/term-commands/serve.test.ts— 15 pass / 0 fail (4 newstartTuiTmuxServercases plus the existing suite)bun run typecheckcleanbun build src/genie.ts ...succeedsserve.ts+serve.test.ts+CHANGELOG.mdgrep -n "new-session -d -s" src/term-commands/serve.tsshows exactly one occurrence (infreshCreateTuiSession)kill-server genie-tuiis destructive to any active TUI on the host; the unit tests cover all four reachable states. Reviewer can run the smoke locally with the steps in the original wish.Test cases
new-sessionnever calledsplit-windowcalled once; nonew-sessionhas-sessionfails →new-session+split-windoweach called oncekill-session+ freshnew-session;~/.genie/logs/tui-crash.logline contains[startTuiTmuxServer]prefix and the original tmux stderrWish
Workspace-local:
genie-tui-startup-resilience. Two execution groups (G1 = code fix + stderr capture, G2 = test + PR), both green.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes