Repository navigation
fix(spawn): pass CWD to tmux split-window via -c flag - #563
Conversation
feat: zero-touch install, session-per-folder, plugin rename, README rewrite
…530) * chore(version): bump to 3.260310.5 [skip ci] * fix(session): sanitize dots in window names for tmux targeting tmux uses '.' as a pane separator in targets (session:window.pane), so folder names like 'ravi.bot' cause "can't find pane: bot" errors. Sanitize dots to dashes in deriveWindowName. * test(session): add regression tests for dot sanitization in window names Covers the fix for tmux "can't find pane" error when folder names contain dots (e.g. ravi.bot). Tests sanitizeWindowName as a pure function — no mock.module needed. * fix(test): use ESM import instead of require() in session tests Replace `const { basename } = require('node:path')` with top-level ESM import for consistency with the rest of the file. * fix(test): add complete export stubs to mock.module in idle-timeout tests Bun's mock.module replaces the entire module cache process-wide. When a mock only provides a subset of exports, unrelated test files that import the same module get "Export named 'X' not found" errors. Add stubs for all agent-registry and tmux exports to prevent cache corruption causing flaky CI failures. * fix(test): replace mock.module with dependency injection in idle-timeout Bun's mock.module corrupts the global module cache process-wide, causing "Export named 'X' not found" errors in unrelated test files. Refactored idle-timeout to accept dependencies via optional IdleDeps parameter (defaults to real implementations). Tests inject mocks directly — no mock.module needed anywhere in the codebase now. * fix(session): sanitize window name before collision lookup Dotted folder names (e.g. `foo.bar`) bypassed collision detection because `resolveWindowName` searched for the unsanitized basename while existing windows were stored with sanitized names (`foo-bar`). This caused `focusTeamWindow` to attach to an unrelated window running a different project. Move `sanitizeWindowName()` call inside `resolveWindowName` so the tmux lookup matches what's actually stored. The outer `sanitizeWindowName` in `deriveWindowName` is now a harmless no-op (idempotent). Adds regression tests for idempotency and dot-dash collision equivalence. * fix(spawn): sanitize dots in team names before tmux targeting Apply sanitizeWindowName to teamName in team-auto-spawn.ts before passing to ensureTeamWindow and constructing tmux target strings. Prevents "can't find pane" errors when team names contain dots. * fix(spawn): return sanitizeWindowName consistently in ensureTeamLead sanitizeTeamName lowercases + strips non-alnum, while the tmux window is created with sanitizeWindowName (dots only). Return the actual window name so callers target the correct tmux window. --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
The v2 redesign gutted session.ts to only export helpers. Main still had the full session command. Dev is correct.
The worktree CWD override in handleWorkerSpawn set ctx.cwd correctly but tmux split-window inherited the parent pane's CWD instead. Adding -c flag ensures the spawned pane starts in the team's worktree. Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
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)
📝 Coding Plan
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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where newly spawned Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where spawned agents in tmux do not start in the correct working directory. The change correctly adds the -c flag to the tmux split-window command to pass the CWD. However, the implementation introduces a critical command injection vulnerability by not escaping the CWD path. I've added a comment with a suggested fix to address this security issue.
| let paneId: string; | ||
| try { | ||
| const splitCmd = `tmux split-window -d ${splitTarget} -P -F '#{pane_id}' ${ctx.fullCommand}`; | ||
| const cwdFlag = ctx.cwd ? `-c '${ctx.cwd}'` : ''; |
There was a problem hiding this comment.
The ctx.cwd variable is used to construct a shell command without proper escaping, which can lead to a command injection vulnerability. If ctx.cwd contains special shell characters like a single quote, it can break the command or allow arbitrary code execution. Please escape ctx.cwd to prevent this. A similar escaping pattern is used in src/lib/tmux.ts.
| const cwdFlag = ctx.cwd ? `-c '${ctx.cwd}'` : ''; | |
| const cwdFlag = ctx.cwd ? `-c '${ctx.cwd.replace(/'/g, "'\\''")}'` : ''; |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ccd43f7915
ℹ️ 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".
| let paneId: string; | ||
| try { | ||
| const splitCmd = `tmux split-window -d ${splitTarget} -P -F '#{pane_id}' ${ctx.fullCommand}`; | ||
| const cwdFlag = ctx.cwd ? `-c '${ctx.cwd}'` : ''; |
There was a problem hiding this comment.
Escape cwd when building tmux split command
Building cwdFlag as -c '${ctx.cwd}' injects an unescaped path into a shell command passed to execSync, so any --cwd (or resolved repo path) containing a single quote will break parsing (e.g., o'connor) and can also introduce shell-injection risk if the value is attacker-controlled. This makes genie spawn fail for valid filesystem paths in those environments; use argument-based process APIs or robust shell escaping for ctx.cwd before interpolation.
Useful? React with 👍 / 👎.
Summary
One-line fix: add
-c '${ctx.cwd}'totmux split-windowcommand so spawned agents start in the team's worktree directory instead of inheriting the parent pane's CWD.Root Cause
launchTmuxSpawn()line 505 rantmux split-windowwithout a-cflag, so the new pane inherited the leader'sprocess.cwd(). The worktree CWD override from PR #560 (ctx.cwd = teamConfig.worktreePath) was set correctly but never passed to tmux.Closes #562
Test plan