feat(cli): Add OSC 52 clipboard support for remote sessions#9966
feat(cli): Add OSC 52 clipboard support for remote sessions#9966ismellpillows wants to merge 1 commit intogoogle-gemini:mainfrom
Conversation
Summary of ChangesHello @ismellpillows, 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 significantly enhances the user experience for developers using the Gemini CLI in remote environments, such as SSH sessions. It addresses a previous limitation where the Highlights
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 introduces a great feature for users in remote sessions by adding OSC 52 clipboard support. The implementation is well-structured, detecting remote sessions and falling back to the original mechanism. My main feedback is to make the new copyViaOSC52 function properly asynchronous to ensure reliability and prevent potential race conditions when writing to stdout. This change will make the implementation more robust.
02e6960 to
630db45
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces OSC 52 support for clipboard operations in remote sessions, which is an excellent enhancement for users on SSH, tmux, or screen. The implementation is clean and correctly placed. My review includes two suggestions to enhance robustness: first, by ensuring OSC 52 sequences are only used in an interactive TTY to prevent output corruption, and second, by adding a payload size check to avoid silent failures on terminals with size limitations for OSC sequences. These changes will make the new feature more reliable across different environments.
| */ | ||
| export const copyViaOSC52 = (text: string): Promise<void> => | ||
| new Promise((resolve, reject) => { | ||
| const base64Text = Buffer.from(text).toString('base64'); |
There was a problem hiding this comment.
The OSC 52 implementation should handle potential payload size limits to prevent silent data truncation. Many terminals, like xterm, have a limit on the length of OSC sequences (commonly around 75KB for the base64 payload). Exceeding this can cause the copy to fail silently or partially. It's safer to check the payload size and reject the promise with an informative error if it's too large.
const base64Text = Buffer.from(text).toString('base64');
// Prevent silent truncation on terminals with OSC 52 payload limits.
const OSC52_MAX_PAYLOAD = 74994;
if (base64Text.length > OSC52_MAX_PAYLOAD) {
return reject(
new Error(
`Content is too large to copy in a remote session (limit: ~${Math.floor(
OSC52_MAX_PAYLOAD / 1024
)}KB).`
)
);
}| // Copies a string snippet to the clipboard for different platforms | ||
| export const copyToClipboard = async (text: string): Promise<void> => { | ||
| // In remote or multiplexed sessions, OSC 52 is the most reliable way | ||
| // to access the user's local (system) clipboard. We prioritize it. | ||
| if (isRemoteOrMultiplexedSession()) { |
There was a problem hiding this comment.
The OSC 52 escape sequence should only be written when stdout is a TTY. Otherwise, the escape sequence could be written into a redirected file or pipe, corrupting the output. Adding a process.stdout.isTTY check ensures this feature is only used in an interactive terminal session, preventing unexpected behavior when the CLI's output is redirected.
if (isRemoteOrMultiplexedSession() && process.stdout.isTTY) {630db45 to
82a8fc7
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces valuable support for OSC 52, which will significantly improve the copy/paste experience in remote sessions. The logic for detecting different terminal environments and wrapping the escape sequences is well-handled. However, I've identified a high-severity issue in the supportsOSC52 function where the check for TTY is overly optimistic and could lead to silent copy failures. I've provided a suggestion to make this detection more conservative. Additionally, it's critical to add unit tests for the new OSC 52 functionality to ensure its correctness and prevent future regressions; the new logic is currently untested.
| if (process.stdout.isTTY || process.stderr.isTTY) { | ||
| return true; | ||
| } | ||
|
|
There was a problem hiding this comment.
The check process.stdout.isTTY || process.stderr.isTTY is too broad for detecting OSC 52 support. A TTY being present does not guarantee OSC 52 compatibility. This can lead to silent failures where the copy command appears to succeed but does nothing, because the underlying copyUsingOSC52 function cannot verify success. Please remove this check and rely on the more specific heuristics for TERM_PROGRAM, SSH_TTY, and TMUX to make detection more reliable.
82a8fc7 to
5343ff5
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for copying text to the clipboard in remote sessions by implementing the OSC 52 terminal escape sequence. The changes are well-structured, introducing clear helper functions to detect terminal capabilities, build the appropriate escape sequences, and handle fallbacks to native clipboard utilities. The logic for preferring OSC 52 in remote or headless environments is sound. I've identified one critical issue in the refactored runCmd utility where error handling for the child process's stdin was omitted, which could lead to unhandled exceptions and cause the application to crash. A code suggestion is provided to restore this important error handling.
| child.stdin?.write(text); | ||
| child.stdin?.end(); |
There was a problem hiding this comment.
The refactored runCmd function is missing error handling for the stdin stream of the child process. The previous implementation included child.stdin.on('error', reject), which is crucial for handling errors like EPIPE that can occur if the child process closes its stdin before the parent process has finished writing to it. Without this, an error on stdin.write could become an unhandled exception and crash the application. Additionally, the use of optional chaining (?.) without an else case means that if stdin is null, the promise will never be resolved or rejected, causing the command to hang indefinitely.
if (child.stdin) {
child.stdin.on('error', reject);
child.stdin.write(text);
child.stdin.end();
} else {
reject(new Error(`Child process for '${cmd}' has no stdin stream.`));
}5343ff5 to
660db2c
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a great enhancement, adding OSC 52 support for clipboard operations in remote sessions, which significantly improves the user experience for developers working over SSH or with terminal multiplexers. The implementation is well-structured, with clear separation of concerns into helper functions for detecting support, building the escape sequence, and handling different environments. The fallback logic between OSC 52 and native clipboard utilities is also robust. However, there are a couple of critical issues. Firstly, the new error handling logic in copyToClipboard and tryNativeClipboard makes unsafe assumptions about caught exceptions, which could lead to unexpressive error messages or runtime errors. I've left specific comments on how to fix this. Most importantly, the new functionality is completely untested. Given the complexity and reliance on environment variables (TERM, SSH_CONNECTION, TMUX, etc.) and platform differences, unit tests are essential to ensure the logic in supportsOsc52, preferOsc52, buildOsc52, and the overall orchestration in copyToClipboard is correct and robust against regressions. Please add comprehensive tests for the new code paths.
| throw new Error( | ||
| `All copy commands failed. "${primaryMsg}", "${fallbackMsg}". `, | ||
| `Native clipboard failed: ${(xclipErr as Error).message}; ${(xselErr as Error).message}`, | ||
| ); |
There was a problem hiding this comment.
The caught errors xclipErr and xselErr are of type unknown. Casting them directly to Error is unsafe. If they are not Error instances, accessing the .message property will not work as expected and could lead to unhelpful error messages. You should safely access the message from the caught errors.
const xclipMsg = xclipErr instanceof Error ? xclipErr.message : String(xclipErr);
const xselMsg = xselErr instanceof Error ? xselErr.message : String(xselErr);
throw new Error(
`Native clipboard failed: ${xclipMsg}; ${xselMsg}`
);| await step(); | ||
| return; | ||
| } catch (e) { | ||
| errors.push(e as Error); |
There was a problem hiding this comment.
The catch block receives a value of type unknown. Casting it directly to Error with e as Error is unsafe, as a promise can be rejected with any value, not just an Error instance. If e is not an Error object, accessing e.message later will result in undefined and will not produce a meaningful error message.
errors.push(e instanceof Error ? e : new Error(String(e)));660db2c to
caef651
Compare
TLDR
This pull request adds support for copying text to the clipboard in remote sessions (e.g., via SSH) by implementing the OSC 52 terminal escape sequence. This resolves a bug where the
/copycommand would fail when running the Gemini CLI in a remote terminal, improving the user experience for developers working on remote machines.Dive Deeper
Previously, the
/copycommand relied on platform-specific utilities likepbcopy(macOS) orxclip(Linux), which only work in a local desktop environment. When a user was connected to a remote machine via SSH, these tools were not available or could not access the local machine's clipboard, causing the copy operation to fail.This PR introduces the following changes:
Remote Session Detection: A new
isRemoteOrMultiplexedSessionutility function has been added to detect if the CLI is running within an SSH session,tmux, orscreen. It does this by checking for environment variables likeSSH_CONNECTIONandTMUX.OSC 52 Implementation: A
copyViaOSC52function was created to handle copying text using the OSC 52 escape sequence. This protocol is widely supported by modern terminal emulators and allows a remote application to access the local system's clipboard securely. The text is Base64-encoded and wrapped in the required escape sequence.Conditional Logic: The main
copyToClipboardfunction has been updated to first check if it's in a remote session. If it is, it uses the new OSC 52 method. If not, it falls back to the original implementation that uses local clipboard utilities.This approach ensures that the
/copycommand works seamlessly and reliably across both local and remote environments without requiring any special configuration from the user.Reviewer Test Plan
To validate this change, reviewers should test the
/copycommand in both a remote and a local environment.Remote Testing (Primary Test Case):
/copycommand to copy a block of text.Local Testing (Regression Test):
/copycommand.Testing Matrix
Linked issues / bugs