Skip to content

fix(sandbox): track PTY state per SSH channel to fix terminal resize#687

Merged
johntmyers merged 1 commit intomainfrom
fix/543-pty-channel-tracking-v2
Mar 30, 2026
Merged

fix(sandbox): track PTY state per SSH channel to fix terminal resize#687
johntmyers merged 1 commit intomainfrom
fix/543-pty-channel-tracking-v2

Conversation

@johntmyers
Copy link
Copy Markdown
Collaborator

Summary

Supersedes #573. Reimplemented cleanly on a fresh branch with review feedback addressed.

  • Replace flat pty_master/input_sender/pty_request fields in SshHandler with HashMap<ChannelId, ChannelState> so each channel tracks its own PTY resources independently
  • Fix set_winsize to pass &winsize to ioctl, correcting undefined behavior on aarch64
  • Add warn! logging for unknown channels across all handlers (window_change_request, data, channel_eof) for debuggability
  • Add channel_close handler to clean up per-channel state and prevent FD leaks

Related Issue

Resolves #543

Changes

File Change
crates/openshell-sandbox/src/ssh.rs Introduce ChannelState struct; migrate per-channel fields into HashMap<ChannelId, ChannelState>; update all handlers to look up channel state by ID; fix ioctl UB; add warn! logging for unknown channels

Improvements over #573

  1. cargo fmt clean — no formatting violations (CI-blocking issue in fix(sandbox): track PTY state per SSH channel to fix terminal resize #573)
  2. Consistent unknown-channel loggingwindow_change_request, data, and channel_eof now all log warn! on unknown channels, matching the error-returning pattern in pty_request/subsystem_request/start_shell
  3. channel_close documented — comment explains it subsumes channel_eof for cleanup

Testing

  • set_winsize_applies_to_correct_pty — verifies two PTYs can be independently resized using real PTYs via openpty
  • channel_state_independent_input_senders — verifies data routing and EOF isolation between channels
  • All existing tests pass (cargo test -p openshell-sandbox)

Checklist

  • Tests added for new functionality
  • mise run pre-commit passes (license check failure is pre-existing in architecture/plans/)
  • No unrelated changes

@johntmyers johntmyers requested a review from a team as a code owner March 30, 2026 19:57
@johntmyers johntmyers self-assigned this Mar 30, 2026
@johntmyers johntmyers added the test:e2e Requires end-to-end coverage label Mar 30, 2026
Replace flat pty_master/input_sender/pty_request fields in SshHandler
with a HashMap<ChannelId, ChannelState> so each channel tracks its own
PTY resources independently. This fixes window_change_request resizing
the wrong PTY when multiple channels are open simultaneously.

Also fixes ioctl UB in set_winsize (pass &winsize not winsize by value)
and adds warn! logging for unknown channels across all handlers.

Resolves #543
@johntmyers johntmyers force-pushed the fix/543-pty-channel-tracking-v2 branch from 666bdfa to c0fe697 Compare March 30, 2026 20:19
@johntmyers johntmyers merged commit ed74a19 into main Mar 30, 2026
9 checks passed
@johntmyers johntmyers deleted the fix/543-pty-channel-tracking-v2 branch March 30, 2026 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(ssh): window-change requests not applied to remote PTY — terminal resize broken

2 participants