Skip to content

fix(tui): target the terminal client clipboard over SSH#4484

Merged
Hmbown merged 3 commits into
mainfrom
agent/091-ssh-clipboard
Jul 17, 2026
Merged

fix(tui): target the terminal client clipboard over SSH#4484
Hmbown merged 3 commits into
mainfrom
agent/091-ssh-clipboard

Conversation

@Hmbown

@Hmbown Hmbown commented Jul 17, 2026

Copy link
Copy Markdown
Owner

Summary

  • detect OpenSSH sessions and route copy to the terminal client instead of a remote-host clipboard
  • use OSC 52 outside tmux and stock tmux load-buffer -w inside tmux, with truthful failure reporting
  • keep client-to-remote paste terminal-owned through bracketed paste
  • preserve graphical text/image clipboard access only for credible forwarding or the explicit CODEWHALE_SSH_CLIPBOARD=graphical override
  • separate terminal text-paste and graphical text/image help in every complete locale
  • document the configuration and keybinding contract

Verification

  • cargo fmt --all -- --check
  • git diff --check
  • clipboard/help unit slice: 17 passed
  • SSH PTY copy/paste regressions: 2 passed
  • locale raw-key and MessageId parity passed
  • an isolated stock tmux 3.6b server with default allow-passthrough=off proved load-buffer -w both fills the buffer and emits the expected OSC 52 receipt to a real attached PTY client; this is now automated with bounded attach/receipt deadlines
  • scripts/release/check-ohos-deps.sh passed; the clipboard implementation stays excluded from target_env = "ohos" and no unsupported arboard/PTY/keyring chain re-enters that graph
  • clean no-conflict integration over current main 7d64a3c87b0f086c30492009557624909989d707
  • earlier exact-head full CI was green on macOS, Windows, lint, DCO, CodeQL, and npm smoke; fresh CI is now running on 2faedb5f98b0910ca31852d1580d50710104b839

A physical second-host/macOS-terminal policy matrix was not available, but both transport boundaries are covered by real PTY/tmux receipts and the platform-selection logic is unit-tested.

No release publication is part of this PR.

Copilot AI review requested due to automatic review settings July 17, 2026 18:42
@Hmbown Hmbown added this to the v0.9.1 milestone Jul 17, 2026
@cursor

cursor Bot commented Jul 17, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@claude

claude Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 4m 23s —— View job


Claude finished reviewing PR #4484

Todo list

  • Gather context (read diff, changed files)
  • Understand the request (code review per custom instructions)
  • Review correctness bugs / panics / error handling
  • Review provider/model/route safety (N/A — no model/provider code touched)
  • Review reuse & simplification
  • Review test coverage
  • Review security concerns
  • Post final review summary

Summary

This is a well-scoped fix for a real bug class: over SSH, a remote-host wl-copy/arboard write can succeed while writing to the wrong machine's clipboard. Routing writes through OSC 52 when SSH_CLIENT/SSH_CONNECTION/SSH_TTY are detected, and routing reads back to a "use your terminal's paste" hint (since a synchronous remote read can never see the client's real clipboard), is the right fix for the underlying architecture problem. The diff is focused, the new TerminalClipboardContext/ClipboardWriteOrder split is clean, and the added unit + PTY tests exercise the actual failure mode (mac-client-to-linux-SSH bracketed paste) rather than just the plumbing.

Two inline notes posted, both non-blocking:

  1. crates/tui/src/tui/clipboard.rs:84-89 — the comment claims SSH_TTY "cannot be the only signal," but the implementation is a plain OR across all three env vars, so it can be the only signal. Also flagged the well-known tmux staleness caveat (SSH_CLIENT/CONNECTION/TTY persist in a tmux pane's environment across detach/reattach, so a later local reattach can still be misdetected as SSH). Not something this PR needs to solve, but worth a doc note given endpoint selection is the PR's whole point.
  2. crates/tui/src/tui/key_shortcuts.rs:27is_copy_shortcut/is_paste_shortcut function bodies are unchanged by this diff (only doc comments moved); they already matched both Cmd+* and Ctrl+Shift+*/Ctrl+* regardless of compiled host OS. The real behavioral fix is in keybindings.rs's help-overlay chord strings, which were previously hardcoded to "Ctrl+V"/"Ctrl+Shift+C" even on macOS builds — worth noting for accurate scoping, not a bug.

Other things checked, no issues found

  • ClipboardHandler::read() unconditionally returns None in SSH sessions — verified this is consistent with paste_from_clipboard()/paste_api_key_from_clipboard() checking terminal_paste_hint() first, and that Event::Paste (bracketed paste, including the onboarding API-key flow at ui.rs:4082) is handled on a separate path unaffected by this change.
  • paste_api_key_from_clipboard() now returning bool so ui.rs:4655 only calls sync_api_key_validation_status on an actual paste is a genuine latent-bug fix (previously ran on failed/no-op paste too), not just plumbing for the SSH case.
  • write_text()'s SSH branch (clipboard.rs:287-290) skips native clipboard entirely and goes straight to OSC 52 — intentional per the PR description, and there's no image-write path to worry about (images are read-only from the clipboard in this file).
  • qa_pty.rs's spawn_minimal_with_env correctly threads extra_env in after .clear_env()/.seal_home() and before .spawn(), so the new boot_minimal_over_ssh() test harness doesn't leak the CI process's real env.
  • No provider/model prefix or routing logic touched — the EPIC v0.8.65 EPIC: Separate provider facts, model facts, offerings, and route resolution #2608 invariant doesn't apply here.
  • No secret handling, shell/exec, or input-validation concerns beyond what already existed (OSC 52 payload is base64 of the user's own copied text, size-capped at OSC52_MAX_BYTES).

Comment thread crates/tui/src/tui/clipboard.rs Outdated
Comment thread crates/tui/src/tui/key_shortcuts.rs
@Hmbown
Hmbown marked this pull request as draft July 17, 2026 18:49
Hmbown added 2 commits July 17, 2026 12:21
Prefer OSC 52 over host-native clipboard writes in SSH sessions so a successful remote Linux clipboard cannot shadow the terminal client's clipboard. Keep tmux passthrough intact, route remote paste through terminal bracketed-paste input, and document client-neutral shortcuts with focused PTY coverage.

Signed-off-by: Hunter B <hmbown@gmail.com>
Use tmux load-buffer -w under stock tmux, propagate transport failures, and retain native reads only for forwarded or explicitly selected graphical clipboards.\n\nAlign help and docs with the in-app Ctrl+C selection path, localize SSH paste guidance, and cover the real SSH PTY/tmux flow.

Signed-off-by: Hunter B <hmbown@gmail.com>
@Hmbown
Hmbown force-pushed the agent/091-ssh-clipboard branch from a4f428a to 781b1c5 Compare July 17, 2026 19:22
@Hmbown

Hmbown commented Jul 17, 2026

Copy link
Copy Markdown
Owner Author

Remote handoff checkpoint: corrected, rebased head 781b1c5 is now pushed. Keep this PR draft until fresh CI is green and the two remaining audit notes in the body are resolved or explicitly accepted. The old a4f428a head must not be merged.

Assert that stock tmux load-buffer -w emits an OSC 52 request to an attached client, not only that it fills the paste buffer.

Separate terminal-owned text paste from graphical text/image clipboard access in help across every complete locale, and align the SSH_TTY comment with the detection logic.

Signed-off-by: Hunter B <hmbown@gmail.com>
@Hmbown
Hmbown marked this pull request as ready for review July 17, 2026 20:14
@claude

claude Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 2m 45s —— View job


Claude finished re-reviewing PR #4484

Todo list

  • Gather context (read diff, changed files, prior review threads)
  • Verify the two previously flagged audit notes (SSH_TTY comment/logic, tmux staleness doc) are resolved in the latest commit 2faedb5
  • Full review pass: correctness, provider/model safety (N/A — no such code touched), reuse, tests, security
  • Post final review summary

Summary

Re-reviewed after Hunter's handoff (781b1c5 rebase + 2faedb5 "close SSH clipboard audit gaps"). This commit history matches the PR body exactly, and it closes out both notes from the earlier review pass:

  1. SSH_TTY comment/logic mismatch — resolved. crates/tui/src/tui/clipboard.rs:120-122 now reads "SSH_TTY is an additional PTY-only marker and is independently sufficient when wrappers preserve it without the other two," which correctly describes the OR-across-three-vars logic in from_env_values. No behavior change was needed — this was a doc-only fix, as expected.
  2. Terminal vs. graphical clipboard help text — resolved. KbTerminalPaste / KbPasteAttach / ClipboardSshPasteHint are new message IDs, added consistently across all 7 locales (en, es-419, ja, ko, pt-BR, vi, zh-Hans — spot-checked each diff), and a new unit test in keybindings.rs (clipboard_help_distinguishes_terminal_text_graphical_image_and_in_app_copy) asserts the terminal-paste description never mentions "image" while the graphical one does, matching the intent.

The tmux staleness caveat I flagged (SSH env vars persisting across a tmux detach/reattach) was correctly not addressed in code — it's inherent to any env-var heuristic and was noted as non-blocking / follow-up material, not something this PR needs to solve.

New in this commit, and it's a genuine strengthening of the test suite: tmux_load_buffer_w_reaches_attached_client_with_default_passthrough_disabled (clipboard.rs:882-1042) now asserts the isolated tmux server actually emits an OSC 52 receipt to a real attached PTY client (bounded 3s attach + 3s receipt deadlines), not just that show-buffer reflects the write. That was a real gap — the old version only proved the tmux paste buffer was filled, not that load-buffer -w actually notified the attached client, which is the entire point of using it over DCS passthrough.

Verified consistent with the PR's own claims

  • paste_from_clipboard() / paste_api_key_from_clipboard() now return bool, and ui.rs:4655 only calls sync_api_key_validation_status on a real paste — both call sites (ui.rs:5998, mouse_ui.rs:1278) compile fine ignoring the bool since it isn't #[must_use].
  • is_copy_shortcut/is_paste_shortcut in key_shortcuts.rs are unchanged behaviorally in this diff — only doc comments and a new cross-platform unit test were added, confirming my earlier note that the OS-independent shortcut fix lives in keybindings.rs's help-overlay chord strings, not the matcher.
  • qa_pty.rs adds boot_minimal_over_ssh() / spawn_minimal_with_env() and two new PTY regressions: a mac-client-into-Linux-SSH bracketed-paste-stays-in-composer test, and an end-to-end SSH+tmux Ctrl+C-copy-reaches-tmux-buffer test. Both exercise real failure modes, not just plumbing.
  • No new provider/model/route logic touched — the EPIC v0.8.65 EPIC: Separate provider facts, model facts, offerings, and route resolution #2608 invariant doesn't apply to this PR.

Not independently verified

I don't have cargo/rustc available in this run's allowed tools, so I could not independently run cargo fmt --all -- --check, cargo test, or the tmux PTY test locally to confirm the claims in the PR body — I relied on reading the diff and existing test code. Fresh CI (CI, PR #4484) is currently still in progress as of this review; DCO has passed.

No blocking issues found. This looks ready to come out of draft once CI is green.

@Hmbown
Hmbown merged commit 2f7a70d into main Jul 17, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants