mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-27 06:11:40 +00:00
fix(tui-clipboard): skip native safety net on OSC52-capable terminals (#20954)
Some checks failed
Deploy Site / deploy-vercel (push) Waiting to run
Deploy Site / deploy-docs (push) Waiting to run
Docker Build and Publish / build-amd64 (push) Waiting to run
Docker Build and Publish / build-arm64 (push) Waiting to run
Docker Build and Publish / merge (push) Blocked by required conditions
Docker Build and Publish / move-latest (push) Blocked by required conditions
Lint (ruff + ty) / ruff + ty diff (push) Waiting to run
Lint (ruff + ty) / ruff enforcement (blocking) (push) Waiting to run
Lint (ruff + ty) / Windows footguns (blocking) (push) Waiting to run
Nix / nix (macos-latest) (push) Waiting to run
Nix / nix (ubuntu-latest) (push) Waiting to run
OSV-Scanner / Scan lockfiles (push) Waiting to run
Tests / test (push) Waiting to run
Tests / e2e (push) Waiting to run
uv.lock check / uv lock --check (push) Waiting to run
Nix Lockfile Fix / auto-fix-main (push) Has been cancelled
Nix Lockfile Fix / fix (push) Has been cancelled
Build Skills Index / build-index (push) Has been cancelled
Build Skills Index / deploy-with-index (push) Has been cancelled
Some checks failed
Deploy Site / deploy-vercel (push) Waiting to run
Deploy Site / deploy-docs (push) Waiting to run
Docker Build and Publish / build-amd64 (push) Waiting to run
Docker Build and Publish / build-arm64 (push) Waiting to run
Docker Build and Publish / merge (push) Blocked by required conditions
Docker Build and Publish / move-latest (push) Blocked by required conditions
Lint (ruff + ty) / ruff + ty diff (push) Waiting to run
Lint (ruff + ty) / ruff enforcement (blocking) (push) Waiting to run
Lint (ruff + ty) / Windows footguns (blocking) (push) Waiting to run
Nix / nix (macos-latest) (push) Waiting to run
Nix / nix (ubuntu-latest) (push) Waiting to run
OSV-Scanner / Scan lockfiles (push) Waiting to run
Tests / test (push) Waiting to run
Tests / e2e (push) Waiting to run
uv.lock check / uv lock --check (push) Waiting to run
Nix Lockfile Fix / auto-fix-main (push) Has been cancelled
Nix Lockfile Fix / fix (push) Has been cancelled
Build Skills Index / build-index (push) Has been cancelled
Build Skills Index / deploy-with-index (push) Has been cancelled
* 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 commit is contained in:
parent
e85592591e
commit
3c23b15f81
3 changed files with 260 additions and 8 deletions
|
|
@ -4,7 +4,7 @@
|
|||
|
||||
import { Buffer } from 'buffer'
|
||||
|
||||
import { env } from '../../utils/env.js'
|
||||
import { env as envModule, supportsOsc52Clipboard } from '../../utils/env.js'
|
||||
import { execFileNoThrow } from '../../utils/execFileNoThrow.js'
|
||||
|
||||
import { BEL, ESC, ESC_TYPE, SEP } from './ansi.js'
|
||||
|
|
@ -20,7 +20,7 @@ export const ST = ESC + '\\'
|
|||
/** Generate an OSC sequence: ESC ] p1;p2;...;pN <terminator>
|
||||
* Uses ST terminator for Kitty (avoids beeps), BEL for others */
|
||||
export function osc(...parts: (string | number)[]): string {
|
||||
const terminator = env.terminal === 'kitty' ? ST : BEL
|
||||
const terminator = envModule.terminal === 'kitty' ? ST : BEL
|
||||
|
||||
return `${OSC_PREFIX}${parts.join(SEP)}${terminator}`
|
||||
}
|
||||
|
|
@ -102,6 +102,77 @@ export function shouldEmitClipboardSequence(env: NodeJS.ProcessEnv = process.env
|
|||
return !!env['SSH_CONNECTION'] || (!env['TMUX'] && !env['STY'])
|
||||
}
|
||||
|
||||
/**
|
||||
* Decide whether setClipboard() should also fire the native clipboard tool
|
||||
* (pbcopy / wl-copy / xclip / xsel / clip.exe) as a safety net alongside
|
||||
* OSC 52 / tmux load-buffer.
|
||||
*
|
||||
* The default is "yes, native fires" — it's the historical safety net for
|
||||
* terminals where OSC 52 may not work (iTerm2 disables OSC 52 by default,
|
||||
* Apple_Terminal / GNOME Terminal / xterm coverage is patchy). The two
|
||||
* cases where we suppress it:
|
||||
*
|
||||
* 1. SSH session: native tools would write to the *remote* machine's
|
||||
* clipboard. OSC 52 (which travels back over the pty to the user's
|
||||
* local terminal) is the right path. Existing behaviour.
|
||||
*
|
||||
* 2. Allowlisted OSC-52-capable terminal AND we're actually going to
|
||||
* emit an OSC 52 sequence AND we're not inside tmux/screen. On these
|
||||
* terminals (Ghostty / kitty / WezTerm / Windows Terminal / VS Code)
|
||||
* the OSC 52 write is reliable on its own, and racing it with a
|
||||
* native tool is destructive — wl-copy on Wayland in particular
|
||||
* wipes the clipboard during its existence-probe and forks a daemon
|
||||
* that races the terminal's own write (~30% empty-clipboard rate
|
||||
* reported on Ghostty + Wayland; symptom: ctrl+shift+c works on the
|
||||
* 3rd attempt).
|
||||
*
|
||||
* The TMUX/STY guard is important: detectTerminal() in utils/env.ts
|
||||
* prefers TERM_PROGRAM over TMUX, so a tmux session inside Ghostty
|
||||
* reports terminal='ghostty'. But inside tmux setClipboard() doesn't
|
||||
* emit raw OSC 52 — it goes through tmux load-buffer (which loads
|
||||
* the tmux paste buffer and, with -w, asks tmux to forward an OSC 52
|
||||
* to the OUTER terminal via its own emission path). The native
|
||||
* safety net is still useful there because tmux load-buffer's
|
||||
* outer-terminal forwarding depends on `set -g set-clipboard` and
|
||||
* `allow-passthrough`, which many users don't have configured.
|
||||
*
|
||||
* The OSC-52-will-emit guard matters too: if the user has set
|
||||
* HERMES_TUI_FORCE_OSC52=0, no OSC 52 sequence will be written. If
|
||||
* we ALSO skip native, the clipboard write becomes a no-op. So skip
|
||||
* native only when OSC 52 will actually carry the data.
|
||||
*/
|
||||
export function shouldUseNativeClipboard(
|
||||
env: NodeJS.ProcessEnv = process.env,
|
||||
terminal: string | null = envModule.terminal
|
||||
): boolean {
|
||||
// Over SSH the native tools would write to the wrong machine's clipboard.
|
||||
if (env.SSH_CONNECTION) {
|
||||
return false
|
||||
}
|
||||
|
||||
// Inside tmux/screen, OSC 52 is normally suppressed and we rely on
|
||||
// tmux load-buffer instead — so the wl-copy/OSC-52 race usually doesn't
|
||||
// apply. Even when HERMES_TUI_FORCE_OSC52=1 forces a tmux-passthrough
|
||||
// OSC 52 emission, we keep native enabled as a safety net: tmux's
|
||||
// outer-terminal forwarding depends on `allow-passthrough` in the
|
||||
// user's tmux config, so a forced OSC 52 may silently never reach the
|
||||
// host terminal. Native (pbcopy/wl-copy/xclip) covers that gap.
|
||||
if (env.TMUX || env.STY) {
|
||||
return true
|
||||
}
|
||||
|
||||
// If OSC 52 won't actually emit (user override or env state), the
|
||||
// native tool is the only path left — keep it on.
|
||||
if (!shouldEmitClipboardSequence(env)) {
|
||||
return true
|
||||
}
|
||||
|
||||
// OSC 52 is going to emit AND the terminal is in the allowlist of
|
||||
// terminals where OSC 52 alone is reliable: skip native to avoid the
|
||||
// wl-copy race documented above.
|
||||
return !supportsOsc52Clipboard(terminal)
|
||||
}
|
||||
|
||||
/**
|
||||
* Wrap a payload in tmux's DCS passthrough: ESC P tmux ; <payload> ESC \
|
||||
* tmux forwards the payload to the outer terminal, bypassing its own parser.
|
||||
|
|
@ -193,11 +264,27 @@ export async function setClipboard(text: string): Promise<ClipboardResult> {
|
|||
// AFTER awaiting tmux load-buffer, adding ~50-100ms of subprocess latency
|
||||
// before pbcopy even started — fast cmd+tab → paste would beat it
|
||||
// (https://anthropic.slack.com/archives/C07VBSHV7EV/p1773943921788829).
|
||||
// Gated on SSH_CONNECTION (not SSH_TTY) since tmux panes inherit SSH_TTY
|
||||
// forever but SSH_CONNECTION is in tmux's default update-environment and
|
||||
// clears on local attach. Fire-and-forget, but `copyNativeAttempted`
|
||||
// tells us whether ANY native path will be tried on this platform.
|
||||
const nativeAttempted = !process.env['SSH_CONNECTION'] && copyNative(text)
|
||||
// Skipped entirely on terminals with first-class OSC 52 support (see
|
||||
// `shouldUseNativeClipboard()` above): running wl-copy/xclip/pbcopy in
|
||||
// parallel with OSC 52 on those terminals can corrupt the clipboard.
|
||||
// wl-copy on Wayland is the worst offender — `probeLinuxCopy()` runs it
|
||||
// with empty stdin to check if the binary exists (which destructively
|
||||
// wipes the clipboard), and the subsequent real invocation forks a
|
||||
// background daemon that races the terminal's own OSC 52 write plus its
|
||||
// own prior daemon's SIGTERM. On Ghostty + Wayland this produced a ~30%
|
||||
// clipboard-empty rate (symptom: user had to press ctrl+shift+c three
|
||||
// times before the selection landed). Native still fires inside
|
||||
// tmux/screen — we primarily rely on tmux load-buffer there rather
|
||||
// than raw OSC 52, so the wl-copy race usually doesn't apply, and
|
||||
// native is kept as a safety net because tmux passthrough forwarding
|
||||
// depends on the user's `allow-passthrough` config (note: when
|
||||
// HERMES_TUI_FORCE_OSC52=1 we DO additionally emit a tmux-passthrough
|
||||
// OSC 52, but it can be silently dropped without that setting).
|
||||
// Native also fires when the user has disabled OSC 52 emission via
|
||||
// HERMES_TUI_FORCE_OSC52=0 (otherwise the clipboard write becomes a
|
||||
// complete no-op). Fire-and-forget, but `nativeAttempted` tells us
|
||||
// whether ANY native path will be tried.
|
||||
const nativeAttempted = shouldUseNativeClipboard(process.env, envModule.terminal) && copyNative(text)
|
||||
|
||||
const tmuxBufferLoaded = await tmuxLoadBuffer(text)
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue