Repository navigation
fix(spawn): bootstrap tmux session when cold-starting with --new-window - #1168
Conversation
`genie spawn <agent> --session <name> --new-window` failed silently when
the target tmux session did not yet exist. `createTmuxPane` ran
`tmux new-window -t <session>:` which tmux rejects with "can't find
session" when the session has not been created.
This is the cold-start path the TUI uses to launch an offline agent
(Nav.tsx spawnAgent → `genie spawn … --new-window`). Because the TUI
fires the child with `stdio: 'ignore'`, the error was swallowed and the
user saw nothing happen.
Check `has-session` first; if the session does not exist, bootstrap with
`new-session -d -s <name>` which creates the session and its first pane
in one call. Existing behaviour is preserved when the session is
already present.
Reproduced before fix:
$ tmux -L genie new-window -d -t 'khal-os:' -P -F '#{pane_id}' 'echo'
can't find session: khal-os
exit=1
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6682fc1797
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const cmd = `${tmuxPrefix}new-window -a -d -t ${shellQuote(`${session}:`)}${cwdFlag} -P -F '#{pane_id}' ${tmuxCommand}`; | ||
| let sessionExists = false; | ||
| try { | ||
| execSync(`${tmuxPrefix}has-session -t ${shellQuote(`=${session}`)}`, { stdio: 'ignore' }); |
There was a problem hiding this comment.
Use real session name for cold-start existence check
The new has-session probe treats session as a tmux session name, but in this code path session can come from teamWindow.windowId (from ensureTeamWindow), which is a window id (@...), not a session identifier. That makes has-session -t '=…' mis-targeted and pushes --new-window spawns into the new-session -s ... fallback with an invalid/wrong name, causing failures or spawning into the wrong session when --session is not provided. Resolve the actual session name (not window id) before the existence check.
Useful? React with 👍 / 👎.
| const cmd = sessionExists | ||
| ? `${tmuxPrefix}new-window -a -d -t ${shellQuote(`${session}:`)}${cwdFlag} -P -F '#{pane_id}' ${tmuxCommand}` | ||
| : `${tmuxPrefix}new-session -d -s ${shellQuote(session)}${cwdFlag} -P -F '#{pane_id}' ${tmuxCommand}`; |
There was a problem hiding this comment.
Make session bootstrap atomic under concurrent spawns
This introduces a check-then-create sequence (has-session followed by new-session) without handling duplicate session on create. If two genie spawn ... --new-window commands cold-start the same missing session concurrently, both can observe "missing" and one will fail on new-session, aborting that spawn. The tmux helpers elsewhere already avoid this TOCTOU by treating duplicate session as success; this branch should do the same.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Code Review
This pull request updates the tmux pane creation logic to handle cold-start scenarios by checking for session existence and using new-session when necessary. The review identifies several issues, including incorrect session naming using window IDs, redundant session checks when a window is already present, and the omission of standard UTF-8 environment variables, providing a comprehensive refactoring suggestion to address these concerns.
| const session = ctx.sessionOverride ?? teamWindow?.windowId?.split(':')[0] ?? ctx.validated.team; | ||
| const cwdFlag = ctx.cwd ? ` -c ${shellQuote(ctx.cwd)}` : ''; | ||
| const cmd = `${tmuxPrefix}new-window -a -d -t ${shellQuote(`${session}:`)}${cwdFlag} -P -F '#{pane_id}' ${tmuxCommand}`; | ||
| let sessionExists = false; | ||
| try { | ||
| execSync(`${tmuxPrefix}has-session -t ${shellQuote(`=${session}`)}`, { stdio: 'ignore' }); | ||
| sessionExists = true; | ||
| } catch { | ||
| sessionExists = false; | ||
| } | ||
| const cmd = sessionExists | ||
| ? `${tmuxPrefix}new-window -a -d -t ${shellQuote(`${session}:`)}${cwdFlag} -P -F '#{pane_id}' ${tmuxCommand}` | ||
| : `${tmuxPrefix}new-session -d -s ${shellQuote(session)}${cwdFlag} -P -F '#{pane_id}' ${tmuxCommand}`; |
There was a problem hiding this comment.
The logic for deriving the session name and checking its existence has a few issues:
- Window ID vs Session Name: teamWindow.windowId is a tmux window ID (e.g., @0). The expression teamWindow?.windowId?.split(':')[0] results in @0. Using this as a session name in has-session -t '=@0' will fail (as it looks for a session literally named @0), and new-session -s '@0' will then attempt to create a session with that ID as a name, which is likely unintended and will fail if the session already exists under its real name.
- Redundant Check: If teamWindow is present, the session is already guaranteed to exist because resolveSpawnTeamWindow (which calls ensureTeamWindow) was successful. The has-session check can be skipped in this case.
- Redundant Assignment: The sessionExists = false in the catch block is redundant as it's already initialized to false.
- Consistency: Other session creation logic in src/lib/tmux.ts sets UTF-8 environment variables (-e LC_ALL=C.UTF-8 -e LANG=C.UTF-8). It's recommended to include them here as well for a consistent environment in cold-started sessions.
Suggested refactoring to use the team name as a fallback and leverage teamWindow presence:
| const session = ctx.sessionOverride ?? teamWindow?.windowId?.split(':')[0] ?? ctx.validated.team; | |
| const cwdFlag = ctx.cwd ? ` -c ${shellQuote(ctx.cwd)}` : ''; | |
| const cmd = `${tmuxPrefix}new-window -a -d -t ${shellQuote(`${session}:`)}${cwdFlag} -P -F '#{pane_id}' ${tmuxCommand}`; | |
| let sessionExists = false; | |
| try { | |
| execSync(`${tmuxPrefix}has-session -t ${shellQuote(`=${session}`)}`, { stdio: 'ignore' }); | |
| sessionExists = true; | |
| } catch { | |
| sessionExists = false; | |
| } | |
| const cmd = sessionExists | |
| ? `${tmuxPrefix}new-window -a -d -t ${shellQuote(`${session}:`)}${cwdFlag} -P -F '#{pane_id}' ${tmuxCommand}` | |
| : `${tmuxPrefix}new-session -d -s ${shellQuote(session)}${cwdFlag} -P -F '#{pane_id}' ${tmuxCommand}`; | |
| const sessionName = ctx.sessionOverride ?? ctx.validated.team; | |
| const cwdFlag = ctx.cwd ? " -c " + shellQuote(ctx.cwd) : ""; | |
| let sessionExists = !!teamWindow; | |
| if (!sessionExists) { | |
| try { | |
| execSync(tmuxPrefix + "has-session -t " + shellQuote("=" + sessionName), { stdio: "ignore" }); | |
| sessionExists = true; | |
| } catch { | |
| // sessionExists remains false | |
| } | |
| } | |
| const target = teamWindow ? teamWindow.windowId : sessionName + ":"; | |
| const cmd = sessionExists | |
| ? tmuxPrefix + "new-window -a -d -t " + shellQuote(target) + cwdFlag + " -P -F '#{pane_id}' " + tmuxCommand | |
| : tmuxPrefix + "new-session -d -s " + shellQuote(sessionName) + " -e LC_ALL=C.UTF-8 -e LANG=C.UTF-8" + cwdFlag + " -P -F '#{pane_id}' " + tmuxCommand; |
Summary
genie spawn <agent> --session <name> --new-windowfailed silently when the target tmux session didn't exist yet — tmux rejectsnew-window -t <session>:withcan't find session.Nav.tsx→spawnAgent→genie spawn … --new-window) to launch an offline agent. The TUI invokes the child withstdio: 'ignore', so the error was swallowed and the user saw nothing happen when clicking "start" on an offline agent.has-sessionfirst; if the session doesn't exist, bootstraps withnew-session -d -s <name>(creates session + first pane in one call). Existing behaviour preserved when the session is already present.Repro (before fix)
After the fix, the same cold-start path takes the
new-sessionbranch and the pane is created correctly.Test plan