fix(tui-clipboard): skip native safety net on OSC52-capable terminals#20954
Merged
Conversation
On terminals with first-class OSC 52 support (Ghostty, kitty, WezTerm, Windows Terminal, VS Code), setClipboard() currently fires both OSC 52 AND a parallel native-tool write (wl-copy / xclip / pbcopy). On Wayland + wl-copy this corrupts the clipboard: probeLinuxCopy() runs wl-copy with empty stdin as an existence check (destructive — wipes clipboard to empty string), and the subsequent real wl-copy invocation races OSC 52 plus its own daemon's previous SIGTERM. Symptom: user on Arch + Ghostty + wl-copy (Wayland, no tmux, no SSH) had to press Ctrl+Shift+C three times before a selection landed. env -u WAYLAND_DISPLAY -u DISPLAY HERMES_TUI_FORCE_OSC52=1 (which short-circuits copyNative via the DISPLAY-absent early-return) made every copy work instantly — proving OSC 52 alone is sufficient on Ghostty and that copyNative() is actively destructive there. Add OSC52_CAPABLE_TERMINALS allowlist to terminal.ts (same pattern as the existing EXTENDED_KEYS_TERMINALS), and gate copyNative() on the terminal NOT being on it. The native safety net continues to fire on unrecognised terminals (xterm, GNOME Terminal, Konsole, Terminal.app, etc.) where OSC 52 is less reliable.
Contributor
🔎 Lint report:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to improve TUI clipboard reliability on terminals with strong OSC 52 support by skipping the parallel native clipboard “safety net” (wl-copy/xclip/pbcopy) to avoid destructive probes and race conditions (notably on Wayland + wl-copy).
Changes:
- Add an OSC52-capable terminal allowlist and a
supportsOsc52Clipboard()helper interminal.ts. - Gate
copyNative(text)insetClipboard()behind!supportsOsc52Clipboard()and expand the safety-net commentary. - Add unit tests for the
supportsOsc52Clipboard()allowlist helper.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
ui-tui/packages/hermes-ink/src/ink/termio/osc.ts |
Skips native clipboard fallback on allowlisted terminals to prevent wl-copy/probe races. |
ui-tui/packages/hermes-ink/src/ink/termio/osc.test.ts |
Adds tests for the OSC52-capable allowlist helper. |
ui-tui/packages/hermes-ink/src/ink/terminal.ts |
Introduces OSC52_CAPABLE_TERMINALS + supportsOsc52Clipboard() export. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Move OSC52_CAPABLE_TERMINALS + supportsOsc52Clipboard() from
ink/terminal.ts to utils/env.ts. ink/terminal.ts already imports
link from ink/termio/osc.ts; importing back into termio/osc.ts
introduced a circular dependency. utils/env.ts has no deps on
either file and already owns terminal detection (detectTerminal()),
so the helper sits naturally next to it.
- Replace the inline gating (!SSH_CONNECTION && !supportsOsc52Clipboard())
with a pure shouldUseNativeClipboard(env, terminal) helper. The old
expression skipped native on allowlisted terminals even when
setClipboard() wouldn't actually emit OSC 52 (e.g. inside
TMUX/STY where we use tmux load-buffer instead, or when the user
has set HERMES_TUI_FORCE_OSC52=0). That made the clipboard write
a no-op in those configurations. The new helper:
1. SSH_CONNECTION set -> false (existing behaviour)
2. TMUX or STY set -> true (we go through load-buffer, no race)
3. shouldEmitClipboardSequence() false -> true (native is the
only path left when OSC 52 is suppressed)
4. Otherwise: skip native iff terminal is allowlisted.
- Add 11 tests for shouldUseNativeClipboard covering the SSH guard,
TMUX/STY tmux-inside-Ghostty case, HERMES_TUI_FORCE_OSC52=0
override, allowlisted vs non-allowlisted terminals, precedence,
and default-args smoke. Tests follow the package's existing
parameterised-helper style (no vi.mock; helpers accept env and
terminal as arguments).
- Update test imports to the new utils/env.js path.
Collaborator
Author
|
@copilot review |
Collaborator
Author
|
@copilot review |
Collaborator
Author
|
@copilot review |
02356abc
pushed a commit
to 02356abc/hermes-agent
that referenced
this pull request
May 14, 2026
…NousResearch#20954) * fix(tui-clipboard): skip native safety net on OSC52-capable terminals On terminals with first-class OSC 52 support (Ghostty, kitty, WezTerm, Windows Terminal, VS Code), setClipboard() currently fires both OSC 52 AND a parallel native-tool write (wl-copy / xclip / pbcopy). On Wayland + wl-copy this corrupts the clipboard: probeLinuxCopy() runs wl-copy with empty stdin as an existence check (destructive — wipes clipboard to empty string), and the subsequent real wl-copy invocation races OSC 52 plus its own daemon's previous SIGTERM. Symptom: user on Arch + Ghostty + wl-copy (Wayland, no tmux, no SSH) had to press Ctrl+Shift+C three times before a selection landed. env -u WAYLAND_DISPLAY -u DISPLAY HERMES_TUI_FORCE_OSC52=1 (which short-circuits copyNative via the DISPLAY-absent early-return) made every copy work instantly — proving OSC 52 alone is sufficient on Ghostty and that copyNative() is actively destructive there. Add OSC52_CAPABLE_TERMINALS allowlist to terminal.ts (same pattern as the existing EXTENDED_KEYS_TERMINALS), and gate copyNative() on the terminal NOT being on it. The native safety net continues to fire on unrecognised terminals (xterm, GNOME Terminal, Konsole, Terminal.app, etc.) where OSC 52 is less reliable. * fix(tui-clipboard): address Copilot review feedback - Move OSC52_CAPABLE_TERMINALS + supportsOsc52Clipboard() from ink/terminal.ts to utils/env.ts. ink/terminal.ts already imports link from ink/termio/osc.ts; importing back into termio/osc.ts introduced a circular dependency. utils/env.ts has no deps on either file and already owns terminal detection (detectTerminal()), so the helper sits naturally next to it. - Replace the inline gating (!SSH_CONNECTION && !supportsOsc52Clipboard()) with a pure shouldUseNativeClipboard(env, terminal) helper. The old expression skipped native on allowlisted terminals even when setClipboard() wouldn't actually emit OSC 52 (e.g. inside TMUX/STY where we use tmux load-buffer instead, or when the user has set HERMES_TUI_FORCE_OSC52=0). That made the clipboard write a no-op in those configurations. The new helper: 1. SSH_CONNECTION set -> false (existing behaviour) 2. TMUX or STY set -> true (we go through load-buffer, no race) 3. shouldEmitClipboardSequence() false -> true (native is the only path left when OSC 52 is suppressed) 4. Otherwise: skip native iff terminal is allowlisted. - Add 11 tests for shouldUseNativeClipboard covering the SSH guard, TMUX/STY tmux-inside-Ghostty case, HERMES_TUI_FORCE_OSC52=0 override, allowlisted vs non-allowlisted terminals, precedence, and default-args smoke. Tests follow the package's existing parameterised-helper style (no vi.mock; helpers accept env and terminal as arguments). - Update test imports to the new utils/env.js path. * fix(tui-clipboard): address Copilot round 2 feedback * fix(tui-clipboard): address Copilot round 3 feedback * fix(tui-clipboard): address Copilot round 4 feedback
jsboige
pushed a commit
to jsboige/hermes-agent
that referenced
this pull request
May 14, 2026
…NousResearch#20954) * fix(tui-clipboard): skip native safety net on OSC52-capable terminals On terminals with first-class OSC 52 support (Ghostty, kitty, WezTerm, Windows Terminal, VS Code), setClipboard() currently fires both OSC 52 AND a parallel native-tool write (wl-copy / xclip / pbcopy). On Wayland + wl-copy this corrupts the clipboard: probeLinuxCopy() runs wl-copy with empty stdin as an existence check (destructive — wipes clipboard to empty string), and the subsequent real wl-copy invocation races OSC 52 plus its own daemon's previous SIGTERM. Symptom: user on Arch + Ghostty + wl-copy (Wayland, no tmux, no SSH) had to press Ctrl+Shift+C three times before a selection landed. env -u WAYLAND_DISPLAY -u DISPLAY HERMES_TUI_FORCE_OSC52=1 (which short-circuits copyNative via the DISPLAY-absent early-return) made every copy work instantly — proving OSC 52 alone is sufficient on Ghostty and that copyNative() is actively destructive there. Add OSC52_CAPABLE_TERMINALS allowlist to terminal.ts (same pattern as the existing EXTENDED_KEYS_TERMINALS), and gate copyNative() on the terminal NOT being on it. The native safety net continues to fire on unrecognised terminals (xterm, GNOME Terminal, Konsole, Terminal.app, etc.) where OSC 52 is less reliable. * fix(tui-clipboard): address Copilot review feedback - Move OSC52_CAPABLE_TERMINALS + supportsOsc52Clipboard() from ink/terminal.ts to utils/env.ts. ink/terminal.ts already imports link from ink/termio/osc.ts; importing back into termio/osc.ts introduced a circular dependency. utils/env.ts has no deps on either file and already owns terminal detection (detectTerminal()), so the helper sits naturally next to it. - Replace the inline gating (!SSH_CONNECTION && !supportsOsc52Clipboard()) with a pure shouldUseNativeClipboard(env, terminal) helper. The old expression skipped native on allowlisted terminals even when setClipboard() wouldn't actually emit OSC 52 (e.g. inside TMUX/STY where we use tmux load-buffer instead, or when the user has set HERMES_TUI_FORCE_OSC52=0). That made the clipboard write a no-op in those configurations. The new helper: 1. SSH_CONNECTION set -> false (existing behaviour) 2. TMUX or STY set -> true (we go through load-buffer, no race) 3. shouldEmitClipboardSequence() false -> true (native is the only path left when OSC 52 is suppressed) 4. Otherwise: skip native iff terminal is allowlisted. - Add 11 tests for shouldUseNativeClipboard covering the SSH guard, TMUX/STY tmux-inside-Ghostty case, HERMES_TUI_FORCE_OSC52=0 override, allowlisted vs non-allowlisted terminals, precedence, and default-args smoke. Tests follow the package's existing parameterised-helper style (no vi.mock; helpers accept env and terminal as arguments). - Update test imports to the new utils/env.js path. * fix(tui-clipboard): address Copilot round 2 feedback * fix(tui-clipboard): address Copilot round 3 feedback * fix(tui-clipboard): address Copilot round 4 feedback
1 task
AlexFoxD
pushed a commit
to AlexFoxD/hermes-agent
that referenced
this pull request
May 21, 2026
…NousResearch#20954) * fix(tui-clipboard): skip native safety net on OSC52-capable terminals On terminals with first-class OSC 52 support (Ghostty, kitty, WezTerm, Windows Terminal, VS Code), setClipboard() currently fires both OSC 52 AND a parallel native-tool write (wl-copy / xclip / pbcopy). On Wayland + wl-copy this corrupts the clipboard: probeLinuxCopy() runs wl-copy with empty stdin as an existence check (destructive — wipes clipboard to empty string), and the subsequent real wl-copy invocation races OSC 52 plus its own daemon's previous SIGTERM. Symptom: user on Arch + Ghostty + wl-copy (Wayland, no tmux, no SSH) had to press Ctrl+Shift+C three times before a selection landed. env -u WAYLAND_DISPLAY -u DISPLAY HERMES_TUI_FORCE_OSC52=1 (which short-circuits copyNative via the DISPLAY-absent early-return) made every copy work instantly — proving OSC 52 alone is sufficient on Ghostty and that copyNative() is actively destructive there. Add OSC52_CAPABLE_TERMINALS allowlist to terminal.ts (same pattern as the existing EXTENDED_KEYS_TERMINALS), and gate copyNative() on the terminal NOT being on it. The native safety net continues to fire on unrecognised terminals (xterm, GNOME Terminal, Konsole, Terminal.app, etc.) where OSC 52 is less reliable. * fix(tui-clipboard): address Copilot review feedback - Move OSC52_CAPABLE_TERMINALS + supportsOsc52Clipboard() from ink/terminal.ts to utils/env.ts. ink/terminal.ts already imports link from ink/termio/osc.ts; importing back into termio/osc.ts introduced a circular dependency. utils/env.ts has no deps on either file and already owns terminal detection (detectTerminal()), so the helper sits naturally next to it. - Replace the inline gating (!SSH_CONNECTION && !supportsOsc52Clipboard()) with a pure shouldUseNativeClipboard(env, terminal) helper. The old expression skipped native on allowlisted terminals even when setClipboard() wouldn't actually emit OSC 52 (e.g. inside TMUX/STY where we use tmux load-buffer instead, or when the user has set HERMES_TUI_FORCE_OSC52=0). That made the clipboard write a no-op in those configurations. The new helper: 1. SSH_CONNECTION set -> false (existing behaviour) 2. TMUX or STY set -> true (we go through load-buffer, no race) 3. shouldEmitClipboardSequence() false -> true (native is the only path left when OSC 52 is suppressed) 4. Otherwise: skip native iff terminal is allowlisted. - Add 11 tests for shouldUseNativeClipboard covering the SSH guard, TMUX/STY tmux-inside-Ghostty case, HERMES_TUI_FORCE_OSC52=0 override, allowlisted vs non-allowlisted terminals, precedence, and default-args smoke. Tests follow the package's existing parameterised-helper style (no vi.mock; helpers accept env and terminal as arguments). - Update test imports to the new utils/env.js path. * fix(tui-clipboard): address Copilot round 2 feedback * fix(tui-clipboard): address Copilot round 3 feedback * fix(tui-clipboard): address Copilot round 4 feedback
19 tasks
gweeteve
pushed a commit
to gweeteve/hermes-agent
that referenced
this pull request
Jun 2, 2026
…NousResearch#20954) * fix(tui-clipboard): skip native safety net on OSC52-capable terminals On terminals with first-class OSC 52 support (Ghostty, kitty, WezTerm, Windows Terminal, VS Code), setClipboard() currently fires both OSC 52 AND a parallel native-tool write (wl-copy / xclip / pbcopy). On Wayland + wl-copy this corrupts the clipboard: probeLinuxCopy() runs wl-copy with empty stdin as an existence check (destructive — wipes clipboard to empty string), and the subsequent real wl-copy invocation races OSC 52 plus its own daemon's previous SIGTERM. Symptom: user on Arch + Ghostty + wl-copy (Wayland, no tmux, no SSH) had to press Ctrl+Shift+C three times before a selection landed. env -u WAYLAND_DISPLAY -u DISPLAY HERMES_TUI_FORCE_OSC52=1 (which short-circuits copyNative via the DISPLAY-absent early-return) made every copy work instantly — proving OSC 52 alone is sufficient on Ghostty and that copyNative() is actively destructive there. Add OSC52_CAPABLE_TERMINALS allowlist to terminal.ts (same pattern as the existing EXTENDED_KEYS_TERMINALS), and gate copyNative() on the terminal NOT being on it. The native safety net continues to fire on unrecognised terminals (xterm, GNOME Terminal, Konsole, Terminal.app, etc.) where OSC 52 is less reliable. * fix(tui-clipboard): address Copilot review feedback - Move OSC52_CAPABLE_TERMINALS + supportsOsc52Clipboard() from ink/terminal.ts to utils/env.ts. ink/terminal.ts already imports link from ink/termio/osc.ts; importing back into termio/osc.ts introduced a circular dependency. utils/env.ts has no deps on either file and already owns terminal detection (detectTerminal()), so the helper sits naturally next to it. - Replace the inline gating (!SSH_CONNECTION && !supportsOsc52Clipboard()) with a pure shouldUseNativeClipboard(env, terminal) helper. The old expression skipped native on allowlisted terminals even when setClipboard() wouldn't actually emit OSC 52 (e.g. inside TMUX/STY where we use tmux load-buffer instead, or when the user has set HERMES_TUI_FORCE_OSC52=0). That made the clipboard write a no-op in those configurations. The new helper: 1. SSH_CONNECTION set -> false (existing behaviour) 2. TMUX or STY set -> true (we go through load-buffer, no race) 3. shouldEmitClipboardSequence() false -> true (native is the only path left when OSC 52 is suppressed) 4. Otherwise: skip native iff terminal is allowlisted. - Add 11 tests for shouldUseNativeClipboard covering the SSH guard, TMUX/STY tmux-inside-Ghostty case, HERMES_TUI_FORCE_OSC52=0 override, allowlisted vs non-allowlisted terminals, precedence, and default-args smoke. Tests follow the package's existing parameterised-helper style (no vi.mock; helpers accept env and terminal as arguments). - Update test imports to the new utils/env.js path. * fix(tui-clipboard): address Copilot round 2 feedback * fix(tui-clipboard): address Copilot round 3 feedback * fix(tui-clipboard): address Copilot round 4 feedback
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
On terminals with first-class OSC 52 support (Ghostty, kitty, WezTerm, Windows Terminal, VS Code),
setClipboard()currently fires both OSC 52 AND a parallel native-tool write (wl-copy/xclip/pbcopy). On Wayland + wl-copy this corrupts the clipboard:probeLinuxCopy()runswl-copywith empty stdin as an existence check, which is destructive (wipes clipboard to empty string), and the subsequent realwl-copyinvocation races OSC 52 plus its own daemon's previous SIGTERM.Symptoms reported: user on Arch + Ghostty + wl-copy (Wayland, no tmux, no SSH) had to press Ctrl+Shift+C three times before a selection landed in the clipboard.
/copywith a selection showed "copied N characters" but left the clipboard empty. Addingenv -u WAYLAND_DISPLAY -u DISPLAY HERMES_TUI_FORCE_OSC52=1(which short-circuitscopyNative()via theDISPLAY-absent early-return atosc.ts:285) made every copy work instantly — proving OSC 52 alone is sufficient on Ghostty and thatcopyNative()is actively destructive there.This PR adds an
OSC52_CAPABLE_TERMINALSallowlist toutils/env.ts(the constant +supportsOsc52Clipboard()helper live there to avoid a circular import —ink/terminal.tsalready imports fromink/termio/osc.ts, so the allowlist can't live interminal.ts), and gatescopyNative()on the terminal NOT being on it. The "native safety net" continues to fire on unrecognised terminals (xterm, GNOME Terminal, Konsole, Terminal.app, etc.) where OSC 52 is less reliable.Changes
ui-tui/packages/hermes-ink/src/utils/env.tsOSC52_CAPABLE_TERMINALSconstant +supportsOsc52Clipboard()export. Lives inutils/env.tsrather thanink/terminal.tsto avoid a circular import (ink/terminal.ts→ink/termio/osc.ts→ would-beink/terminal.ts); pattern still mirrorsEXTENDED_KEYS_TERMINALS/supportsExtendedKeys(). Helper accepts an optional terminal-name argument (default =env.terminal) for testability.ui-tui/packages/hermes-ink/src/ink/termio/osc.tssetClipboard(), gatecopyNative(text)on!supportsOsc52Clipboard(); update the existing safety-net comment block to document the skip condition and the Ghostty+Wayland symptom it fixes.ui-tui/packages/hermes-ink/src/ink/termio/osc.test.tssupportsOsc52Clipboard: allowlisted terminals return true, non-allowlisted return false,nullreturns false, default-argument call returns a boolean.Why this allowlist specifically
clipboard-write = allow)osc.ts:165)setClipboard()Terminal-name values match what
detectTerminal()inutils/env.tsreturns:TERM=xterm-ghostty→'ghostty',TERMcontainingkitty→'kitty',WT_SESSIONset →'windows-terminal',TERM_PROGRAM→WezTerm/vscodeas-is.Related issues / prior art
Validation
/copywith selectionwl-copy []probe wipes clipboard!SSH_CONNECTIONgate already short-circuits native)Tests:
./node_modules/.bin/vitest run packages/hermes-ink/src/ink/termio/osc.test.ts— 21 passed (5 existing + 16 new), 0 failed../node_modules/.bin/vitest run— 491 passed / 0 test-level failures. 15 test files fail to load due to a pre-existing issue importingpackages/hermes-ink/index.js→./dist/entry-exports.js(requires a priornpm run build); confirmed identical failure count onmainbefore my changes (main: 475 passed, 15 import-error files; PR: 491 passed, same 15 import-error files)../node_modules/.bin/tsc --noEmit -p tsconfig.json— clean (exit 0)../node_modules/.bin/eslinton touched files — clean../node_modules/.bin/prettier --checkon touched files — clean.Follow-ups (not in this PR, intentionally)
Non-destructive probe.
probeLinuxCopy()currently runswl-copy []/xclip -selection clipboard/xsel --clipboard --inputwith empty stdin to check if the binary exists. These are destructive. A follow-up PR should switch towhich-style detection or--versionprobes. This PR leaves that code untouched and simply avoids calling it on the terminals that don't need native at all.Misleading
/copymessage on no-selection path./copywith no selection currently emits "sent OSC52 copy sequence (terminal support required)". Existing open PR Feat/dashboard chat #13379 fixes this incidentally as part of dashboard-chat work; could also be pulled out standalone. Not addressed here.Allowlist expansion. Alacritty, GNOME Terminal (via recent VTE OSC 52 work), etc. could join the allowlist after verification. Intentionally conservative in this first pass.
Checklist
fix(tui-clipboard):prefix)vitest runpasses for touched test file (21/21)tsc --noEmitclean for touched files