Skip to content

fix(tmux): TmuxSessionManager critical bugs (#3, #4, #14, #24)#131

Merged
daiimus merged 6 commits intomainfrom
fix/tmux-session-manager-bugs
Mar 9, 2026
Merged

fix(tmux): TmuxSessionManager critical bugs (#3, #4, #14, #24)#131
daiimus merged 6 commits intomainfrom
fix/tmux-session-manager-bugs

Conversation

@daiimus
Copy link
Copy Markdown
Owner

@daiimus daiimus commented Mar 9, 2026

Summary

Fixes 4 bugs in TmuxSessionManager.swift identified during comprehensive code review (issue #129).

Changes

Finding #3: Double % prefix in paste-buffer command

focusedPaneId already contains the % prefix (e.g. "%0"). The paste-buffer -t %\(paneId) call produced paste-buffer -t %%0, which tmux rejects. Removed the extra %.

Finding #4: handleFocusedPaneChanged was log-only

When tmux notifies that the focused pane changed (e.g. via select-pane), this handler now calls setFocusedPane to update input routing. Without this, keystrokes continued going to the old pane via send-keys.

Finding #14: findSplitContainingWithSizeHelper only checked leftmost pane

The search for a pane's containing split only compared against split.left.leftmostPaneId, missing panes that were right children or not the leftmost leaf. Now uses contains(paneId:) on both children, with isPane check to distinguish direct children from deeper subtree matches.

Finding #24: pasteTmuxBuffer didn't escape $ or backtick

Added escaping for $ (tmux format variables) and ` (shell expansion) in clipboard content before passing to set-buffer, preventing injection.

Testing

  • ./ci.sh build passes
  • All changes are in a single file with clear, targeted fixes

Closes #129

This PR was authored with AI assistance (Claude/OpenCode).

fix(tmux): fix 4 TmuxSessionManager bugs — paste-buffer double %, foc…
1af88d0 
…us routing, split tree search, paste escaping

- Finding #3: Remove extra "%" prefix in paste-buffer command. focusedPaneId
  already contains the "%" prefix (e.g. "%0"), so "paste-buffer -t %\(paneId)"
  produced "paste-buffer -t %%0" which tmux rejects.

- Finding #4: handleFocusedPaneChanged now calls setFocusedPane when the
  window matches focusedWindowId. Previously log-only, meaning pane focus
  changes from tmux (e.g. select-pane) did not route input to the correct
  pane via send-keys.

- Finding #14: findSplitContainingWithSizeHelper now checks both children
  of a split, not just leftmostPaneId. The old code missed panes that were
  a right child or not the leftmost leaf in a subtree.

- Finding #24: pasteTmuxBuffer now escapes "$" and backtick characters
  in clipboard content. These are interpreted by tmux as format variables
  and shell expansion respectively, causing injection in set-buffer.
Copilot AI review requested due to automatic review settings March 9, 2026 02:09
Copilot AI reviewed Mar 9, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes 4 bugs in TmuxSessionManager.swift identified during a comprehensive code review (issue #129). It addresses a double % prefix in the paste-buffer command, a log-only handleFocusedPaneChanged handler that wasn't updating state, a split tree search that only checked the leftmost pane, and missing escaping for $ and backtick in clipboard content.

Changes:

  • Fixed double % prefix in paste-buffer -t command where focusedPaneId already contains the % prefix
  • Updated handleFocusedPaneChanged to call setFocusedPane when the focused window matches, so input routing follows pane changes triggered by tmux
  • Rewrote findSplitContainingWithSizeHelper to use contains(paneId:) and isPane() on both children, correctly finding the innermost split for any pane position in the tree
  • Added escaping for $ and backtick characters in pasteTmuxBuffer to prevent tmux format variable expansion and shell injection

fix(tmux): remove double % prefix in paste log message
9c5fb2b 
The log message used "%\(paneId)" but paneId already contains the
"%" prefix (e.g. "%0"), producing "%%0" in logs. The command on the
previous line was already fixed, but the log line was missed.
@daiimus daiimus requested a review from Copilot March 9, 2026 02:46
Copilot AI reviewed Mar 9, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

fix(tmux): use % prefix in paste test focusedPaneId to match production
f486a8a 
In production, focusedPaneId always includes the "%" prefix (e.g. "%5").
Tests were passing bare numeric IDs (e.g. "5"), which masked the
paste-buffer double-% bug fix from the previous commit. With the fix
removing the extra % from the command string, the % must come from
focusedPaneId itself.
@daiimus daiimus requested a review from Copilot March 9, 2026 03:16
Copilot AI reviewed Mar 9, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

fix(tmux): address Copilot round 3 review on TmuxSessionManager
4372214 
- Fix comment: $ is not for tmux format variables (those use #{}).
  Reworded to "defense-in-depth against potential expansion in edge cases"
- Add testPasteTmuxBufferEscapesDollarAndBacktick: verifies $ and
  backtick are escaped in set-buffer command
- Add testHandleFocusedPaneChangedUpdatesFocusForMatchingWindow: sets
  focusedWindowId to "@1", calls handleFocusedPaneChanged(windowId: 1,
  paneId: 5), asserts focusedPaneId becomes "%5"
- Add testHandleFocusedPaneChangedIgnoresNonMatchingWindow: verifies
  focusedPaneId unchanged when windowId does not match focusedWindowId
- Add setFocusedWindowIdForTesting() DEBUG helper for test access
@daiimus daiimus requested a review from Copilot March 9, 2026 03:39
Copilot AI reviewed Mar 9, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

fix(test): fix indentation and add right-child pane resize test
1fefdb5 
Fix inconsistent indentation (9 spaces → 8) on XCTAssertFalse call.

Add testSyncSplitRatioRightChildPaneSendsResizePane which validates
finding #14: the right child of a horizontal split can be correctly
found and resized. The old code only checked split.left.leftmostPaneId,
missing right children entirely.
@daiimus daiimus requested a review from Copilot March 9, 2026 03:57
Copilot AI reviewed Mar 9, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

fix(test): replace tautological dollar-sign assertion with regex
6f6d911 
The old assertion (contains("$100") && !contains("\\$100")) was
always false because the escaped form is a substring of the unescaped
pattern. Use a negative lookbehind regex to properly detect any
unescaped dollar sign.
@daiimus daiimus requested a review from Copilot March 9, 2026 04:10
Copilot AI reviewed Mar 9, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

daiimus pushed a commit that referenced this pull request Mar 9, 2026
refactor(tmux): Swift code quality improvements (findings #35-40)
341ed75 
Address 6 low-severity findings from comprehensive code review:

#35: Mark log-only notification handlers with MARK stub annotation
#36: Extract surfaceView(from:) helper — replaces 25 identical call sites
#37: Extract decodePayload(data:len:) helper — replaces 5 identical blocks
#38: Add TmuxId format validation guards to selectPane, pasteTmuxBuffer,
     closeWindow, renameWindow, selectWindow
#39: handleSessionRenamed now stores name in @published sessionName
     (moved out of stub section), reset on prepareForReattach
#40: Define TmuxNotificationKey enum for typed notification userInfo keys
     — replaces 10 raw string keys across posting and consuming sides

Also fixes double-% in paste-buffer command (same bug as S1 finding #3,
independently fixed here since this branch predates PR #131).

Closes #130
@daiimus daiimus mentioned this pull request Mar 9, 2026
Merged
@daiimus daiimus merged commit 55b52a6 into main Mar 9, 2026
5 checks passed
@daiimus daiimus deleted the fix/tmux-session-manager-bugs branch March 9, 2026 04:34
daiimus pushed a commit that referenced this pull request Mar 9, 2026
refactor(tmux): Swift code quality improvements (findings #35-40)
5535ff7 
Address 6 low-severity findings from comprehensive code review:

     closeWindow, renameWindow, selectWindow
     (moved out of stub section), reset on prepareForReattach
     — replaces 10 raw string keys across posting and consuming sides

Also fixes double-% in paste-buffer command (same bug as S1 finding #3,
independently fixed here since this branch predates PR #131).

Closes #130
daiimus added a commit that referenced this pull request Mar 9, 2026
@daiimus
refactor(tmux): Swift code quality improvements (findings #35-40) (#132)
b17ca48 
* refactor(tmux): Swift code quality improvements (findings #35-40)

Address 6 low-severity findings from comprehensive code review:

     closeWindow, renameWindow, selectWindow
     (moved out of stub section), reset on prepareForReattach
     — replaces 10 raw string keys across posting and consuming sides

Also fixes double-% in paste-buffer command (same bug as S1 finding #3,
independently fixed here since this branch predates PR #131).

Closes #130

* fix(tmux): address Copilot round 2 review comments on PR #132

1. Move handleFocusedPaneChanged and handleSubscriptionChanged above the
   Stub Handlers MARK since they do real work (update state), not just log.

2. Clear sessionName in both controlModeExited() and cleanup() for
   consistency with other session metadata resets.

3. Add tests for window ID validation guards: closeWindow, renameWindow,
   and selectWindow all reject malformed IDs without sending commands.

* fix(test): update stale doc comment for selectPane validation test

---------

Co-authored-by: daiimus <daiimus@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

fix: TmuxSessionManager critical/high bugs

2 participants

@daiimus