fix(minimald): fix detach-chord detection under zellij - #440
Conversation
📝 WalkthroughWalkthroughIn ChangesKitty ctrl-w detach support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
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 `@crates/minimald/src/session_host.rs`:
- Around line 927-930: The is_detach logic at lines 928-930 performs exact byte
equality checks on individual message chunks, but multi-byte CSI escape
sequences like CTRL_W_CSI_U and CTRL_W_CSI_27 can be fragmented across multiple
russh::ChannelMsg::Data messages, causing detach detection to fail. Replace the
per-chunk equality checks with a rolling buffer or state machine that
accumulates incoming bytes across multiple messages and matches against the
complete sequences. This ensures that even if a Ctrl-W sequence arrives
fragmented, it will still be properly detected.
🪄 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: 35f52f40-e407-4a10-8778-3028aab38248
📒 Files selected for processing (1)
crates/minimald/src/session_host.rs
| // ctrl-w | ||
| let is_detach = b.len() == 1 && b[0] == 0x17 || | ||
| b == CTRL_W_CSI_U || | ||
| b == CTRL_W_CSI_27; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does russh::ChannelMsg::Data guarantee input message boundaries for terminal keystroke streams, or can CSI escape sequences be split/coalesced across messages?
💡 Result:
The russh::ChannelMsg::Data variant does not guarantee input message boundaries for terminal keystroke streams or any other application-level data [1][2]. SSH channel data is treated as a continuous, unstructured byte stream [3][4]. Because the SSH connection protocol (RFC 4254) multiplexes data over channels without preserving application-layer framing, CSI escape sequences and other terminal control codes can be split, coalesced, or fragmented across multiple ChannelMsg::Data messages [3][4]. Key technical considerations: 1. Stream Orientation: The russh library exposes SSH channels as stream-oriented abstractions, such as ChannelStream, which implements AsyncRead and AsyncWrite [1][5]. These abstractions behave like standard TCP-like streams where the receiver must handle partial reads and potentially reassemble data fragments [1][5]. 2. Protocol Nature: SSH SSH_MSG_CHANNEL_DATA packets are designed to carry arbitrary byte streams [4][6]. There is no inherent protocol-level requirement for an SSH message to contain a "complete" sequence of terminal data, and implementations (both client and server) are free to segment data based on window sizes, buffer limits, or network packet boundaries [4]. 3. Handling Requirements: Applications consuming these streams must implement their own buffering and parsing logic to reliably identify and process complete CSI sequences (e.g., waiting until a full ESC [ sequence is received) [3]. You cannot rely on individual ChannelMsg::Data messages to contain atomically complete terminal escape codes [3][4].
Citations:
- 1: https://docs.rs/russh/latest/russh/struct.ChannelStream.html
- 2: https://docs.rs/russh/latest/russh/enum.ChannelMsg.html
- 3: https://www.cloudfellows.de/blog/ssh-applications-part-2.html
- 4: https://datatracker.ietf.org/doc/html/rfc4254.html
- 5: https://docs.rs/russh/latest/russh/struct.Channel.html
- 6: https://emailstuff.org/rfc/rfc4253
🏁 Script executed:
# Get context around lines 927-930 in the file
head -n 950 crates/minimald/src/session_host.rs | tail -n 50 | cat -nRepository: gominimal/minimal
Length of output: 2838
🏁 Script executed:
# Get more context to understand the data flow and any existing buffering
sed -n '900,960p' crates/minimald/src/session_host.rs | cat -nRepository: gominimal/minimal
Length of output: 3498
🏁 Script executed:
# Search for existing buffering logic or state machines in the file
rg -n "buffer|Buffer|state|State" crates/minimald/src/session_host.rs | head -20Repository: gominimal/minimal
Length of output: 715
🏁 Script executed:
# Check the function/method containing this code to understand the full context
sed -n '850,950p' crates/minimald/src/session_host.rs | cat -nRepository: gominimal/minimal
Length of output: 5439
🏁 Script executed:
# Find where remote_rx is defined and initialized
rg -n "remote_rx" crates/minimald/src/session_host.rs | head -20Repository: gominimal/minimal
Length of output: 302
🏁 Script executed:
# Search for where remote_rx gets its data (who sends to it)
rg -n "remote_rx\|Binding::run" crates/minimald/src/ -A 5 | grep -E "(fn run|send|remote_rx)" | head -30Repository: gominimal/minimal
Length of output: 43
🏁 Script executed:
# Find the Binding struct and run method to understand data flow
fd -t f "*.rs" crates/minimald/src/ | xargs rg -l "Binding" | head -5Repository: gominimal/minimal
Length of output: 399
🏁 Script executed:
# Search for where remote_tx is used (who sends data)
rg -n "remote_tx" crates/minimald/src/ -A 2 -B 2Repository: gominimal/minimal
Length of output: 1974
🏁 Script executed:
# Find Binding::run to understand data flow from russh
rg -n "impl.*Binding|fn run" crates/minimald/src/session_host.rs | head -20Repository: gominimal/minimal
Length of output: 113
🏁 Script executed:
# Look for the actual file that contains Binding::run or the connection handler
find crates/minimald/src -name "*.rs" -exec grep -l "Binding.*run\|remote_tx.send" {} \;Repository: gominimal/minimal
Length of output: 43
🏁 Script executed:
# Read the Binding::run method to understand data flow from russh
sed -n '236,350p' crates/minimald/src/session_host.rs | cat -nRepository: gominimal/minimal
Length of output: 6213
🏁 Script executed:
# Also find the Binding struct definition
sed -n '218,240p' crates/minimald/src/session_host.rs | cat -nRepository: gominimal/minimal
Length of output: 941
🏁 Script executed:
# Search for where data is sent via remote_tx in Binding::run
sed -n '236,400p' crates/minimald/src/session_host.rs | grep -n "remote_tx\|send\|Channel"Repository: gominimal/minimal
Length of output: 516
Detach detection logic depends on chunk boundaries and can miss Ctrl-W sequences.
At lines 928–930, is_detach checks exact equality against incoming bytes chunks. Since russh::ChannelMsg::Data does not guarantee message boundaries, CSI escape sequences (like CTRL_W_CSI_U) can be split across multiple messages. If a detach sequence fragments, neither chunk will match and detach will fail to trigger.
Implement stream-aware matching using a small rolling buffer or state machine instead of per-chunk equality checks.
🤖 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 `@crates/minimald/src/session_host.rs` around lines 927 - 930, The is_detach
logic at lines 928-930 performs exact byte equality checks on individual message
chunks, but multi-byte CSI escape sequences like CTRL_W_CSI_U and CTRL_W_CSI_27
can be fragmented across multiple russh::ChannelMsg::Data messages, causing
detach detection to fail. Replace the per-chunk equality checks with a rolling
buffer or state machine that accumulates incoming bytes across multiple messages
and matches against the complete sequences. This ensures that even if a Ctrl-W
sequence arrives fragmented, it will still be properly detected.
Summary by CodeRabbit