mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-21 05:11:26 +00:00
* tui: make URLs clickable + hover-highlight in any terminal Problem ------- URLs printed by `hermes --tui` were not clickable in basic macOS Terminal.app. Cmd+click did nothing, the cursor didn't change shape — like nothing was detected — even though arrow buttons and other Box onClick handlers worked fine. Root cause ---------- Two layers of dead plumbing: 1. `<Link>` only emitted the underlying `<ink-link>` (which carries the hyperlink metadata into the screen buffer) when `supportsHyperlinks()` said yes. On Apple_Terminal that's false, so the per-cell hyperlink field stayed empty, so `Ink.getHyperlinkAt()` had nothing to return on click. The visible underline was just decorative. 2. `Ink.openHyperlink()` calls `this.onHyperlinkClick?.(url)`, but `onHyperlinkClick` was never assigned anywhere in the codebase. The click pipeline (`App.tsx → onOpenHyperlink → Ink.openHyperlink`) ran but bailed silently on the optional chain. Bonus discovery: even when wired up, there was no hover affordance — terminal apps can't change the system mouse cursor, so users had no visual signal that a cell was clickable. Arrow buttons in the chrome worked because they had explicit `<Box onClick>` styling; inline link URLs didn't. Fix --- - `Link.tsx`: always emit `<ink-link>` regardless of terminal capability. The renderer's `wrapWithOsc8Link` already gates the actual OSC 8 escape on `supportsHyperlinks()` further down — so terminals that don't understand OSC 8 still don't see the escape, but the screen-buffer metadata (which the click dispatcher reads) is now populated everywhere. - `ink.tsx + root.ts`: add `onHyperlinkClick?: (url: string) => void` to `Options` / `RenderOptions`, wire it to the existing `Ink.onHyperlinkClick` field in the constructor. - `src/lib/openExternalUrl.ts`: small platform-aware opener using `child_process.spawn` with arg-array (no shell) — http(s) only, rejects `file:`, `javascript:`, `data:`, etc., so a hostile model can't trigger arbitrary local handlers via `<Link url="file:///...">`. Detached + stdio ignore so closing the TUI doesn't kill the browser and Chrome stderr doesn't leak into the alt screen. - `entry.tsx`: pass `onHyperlinkClick: openExternalUrl` to `ink.render`. - `hyperlinkHover.ts` + Ink hover wiring: track the URL under the pointer in `Ink.hoveredHyperlink`, update it from `dispatchHover`, and inverse- highlight every cell of the matching link in the render-pass overlay (same pattern as `applySearchHighlight`). This is the cursor-hover affordance for clickable links — terminals don't expose cursor shape, so we light up the link itself. - `types/hermes-ink.d.ts`: add `onHyperlinkClick` to the `RenderOptions` shim so consumers (`entry.tsx`) type-check against the new option. Tests ----- - `src/lib/openExternalUrl.test.ts` (15 cases): http(s) accepted; file/js/ data/mailto/ftp/ssh rejected; macOS open(1), Windows cmd.exe start with empty title slot, Linux xdg-open dispatch; shell-metacharacter URLs pass through unmolested as a single argv element; synchronous spawn failure returns false. Verified empirically in Apple Terminal 455.1 (macOS 15.7.3): clicking a URL opens in default browser, hovering inverts the link cells, and moving away clears the highlight. Full TUI suite: 713 passing, 0 type errors. Reverts ------- The earlier attempt that version-gated Apple_Terminal in `supports-hyperlinks.ts` was based on a wrong assumption — Terminal.app silently strips OSC 8 sequences but does not render them as clickable hyperlinks. Reverted to the original allowlist. * tui: address Copilot review — explorer.exe on win32 + comment fixes - openExternalUrl: switch win32 from `cmd.exe /c start` to `explorer.exe`. cmd.exe's `start` builtin reparses the URL through cmd's tokenizer, so `&`, `|`, `^`, `<`, `>` either split the command or get reinterpreted — breaking both the protocol-allowlist safety story AND plain http(s) URLs with `&` in query strings. `explorer.exe <url>` invokes the registered protocol handler directly with no shell. - openExternalUrl.test.ts: rename the win32 test to reflect the new contract and add two regression tests — one with `&|^<>` metachars, one with the common analytics-URL `&` query-param pattern — both pinned to single-argv-element delivery via explorer.exe. - Link.tsx: fix misleading comment. OSC 8 escapes are emitted unconditionally by the renderer (`wrapWithOsc8Link` in render-node-to-output.ts, `oscLink` in log-update.ts). Non-supporting terminals silently strip the sequence, which is why hover/click affordance has to come from the in-process overlay rather than the terminal's own link rendering. Verified: 715/715 tests pass, type-check + build clean. * tui: address Copilot review #2 — async spawn errors + hover scope + docs 1. openExternalUrl: attach a no-op `'error'` listener on the spawned child BEFORE unref(). spawn() returns a ChildProcess synchronously even when the binary is missing (ENOENT on xdg-open / explorer.exe), unreachable, or otherwise unusable; the failure surfaces later as an 'error' event. An unhandled 'error' on an EventEmitter crashes Node, which would tear down the whole TUI. The listener is a deliberate no-op — we already returned `true` synchronously and the user just doesn't see the browser pop. 2. openExternalUrl.test.ts: add a regression test using a real EventEmitter to simulate the async-error path. Pins both the listener-attached contract and the "doesn't throw on emit" behavior. Was 17/17, now 18/18. 3. ink.tsx dispatchHover: bypass `getHyperlinkAt()` and read `cellAt(...).hyperlink` directly. `getHyperlinkAt` falls back to `findPlainTextUrlAt` for cells without an OSC 8 hyperlink, but the render-pass overlay (`applyHyperlinkHoverHighlight`) only matches on `cell.hyperlink === hoveredUrl` — so plain-text URLs would burn re-renders without ever producing the highlight. Hover is now a strictly 1:1 fit for what the overlay can paint. Plain-text URLs still get the click action via the existing dispatch path. 4. root.ts + ink.tsx doc comments: replace the misleading "typically `open` / `xdg-open` / `start` shell" wording with the actual safe recipe — argv-array spawn into `open` / `xdg-open` / `explorer.exe`, with an explicit warning that `cmd.exe /c start` reparses the URL through cmd's tokenizer and is unsafe + breaks `&`-query URLs. Verified: 716/716 tests pass, type-check + build clean. * tui: address Copilot review #3 — hover damage, alt-screen cleanup, opener allowlist 1. ink.tsx onRender: stop folding steady-state hover into hlActive. hlActive forces a full-screen damage diff so previous-frame inverted cells get re-emitted when the highlight set changes. The transition IS the trigger — enter / leave / change-to-other-link. While the pointer just sits on a link the painted cells don't change and the per-cell diff handles the no-op. Folding the steady state in would burn a full-screen diff on every frame. Added a lastRenderedHoveredHyperlink tracker and gate the hlActive bump on `hovered !== lastRendered`. 2. ink.tsx setAltScreenActive: clear hoveredHyperlink (and the tracker) when toggling alt-screen state. Hover dispatch is alt-screen-gated, so once we leave there's no path to clear it. Without this, remounting <AlternateScreen> would paint a phantom hover from the previous session until the next mouse-move arrived. 3. openExternalUrl.ts openCommand: allowlist linux + the BSD family for xdg-open and return null for everything else (aix, sunos, cygwin, haiku, etc.). Previously the default-fallback always returned xdg-open, which made the caller's `if (!command) return false` dead and yielded a misleading `true` on platforms that probably don't have xdg-open. New tests cover the null path AND the openExternalUrl-returns-false-without-spawning behavior. Verified: 718/718 tests pass, type-check + build clean. * tui: address Copilot review #4 — doc comment accuracy 1. openExternalUrl return-value doc: now lists all three false paths (URL rejected / no opener for platform / synchronous spawn throw) plus a note that async 'error' events still return true because the spawn was attempted. 2. ink.tsx onHyperlinkClick field doc: clarifies the callback receives either an OSC 8 hyperlink OR a plain-text URL detected by findPlainTextUrlAt — App.tsx routes both into the same callback. 3. hyperlinkHover applyHyperlinkHoverHighlight doc: drops the misleading 'caller forces full-frame damage' promise. Caller decides; for hover the current caller only forces full damage on transitions. No behavior change. 718/718 tests pass. * tui: address Copilot review #5 — lint fixes 1. ink.tsx: reorder `./hyperlinkHover.js` import before `./screen.js` to satisfy perfectionist/sort-imports. 2. Link.tsx: drop unused `fallback` parameter destructuring + the trailing `void (null as ...)` dead-statement (would trip no-unused-expressions). Kept `fallback?: ReactNode` on the Props interface as a documented compat shim so existing call sites still compile, with a comment explaining why it's no longer wired up. 3. openExternalUrl.test.ts: replace `typeof import('node:child_process').spawn` inline annotations (forbidden by @typescript-eslint/consistent-type-imports) with a `SpawnLike` type alias backed by a real `import type { spawn as SpawnFn }`. No behavior change. 718/718 tests pass, type-check clean, lint clean on all modified files.
158 lines
5.8 KiB
TypeScript
158 lines
5.8 KiB
TypeScript
import { spawn, type SpawnOptions } from 'node:child_process'
|
|
import { platform } from 'node:os'
|
|
|
|
/**
|
|
* Opens an external URL in the user's default browser/handler.
|
|
*
|
|
* Wired into the Ink instance via `onHyperlinkClick` in entry.tsx, so any
|
|
* mouse click on a `<Link>` cell (or a row containing a plain-text URL the
|
|
* renderer detected) goes here. Mouse tracking inside the TUI prevents
|
|
* Terminal.app's native Cmd+click from firing — the click is captured
|
|
* before the terminal application sees it — so we have to handle the open
|
|
* ourselves.
|
|
*
|
|
* Safety:
|
|
* - http(s) only. Anything else (`file:`, `data:`, `javascript:`, etc.) is
|
|
* rejected — a hostile model could otherwise emit `<Link url="file:///">`
|
|
* and trick a click into running an arbitrary local handler.
|
|
* - Hostname is parsed via `URL`; only well-formed URLs are forwarded.
|
|
* - Spawned via `child_process.spawn` with arg array (no shell), so a URL
|
|
* containing shell metacharacters (`;`, `&`, backticks) cannot be
|
|
* interpreted as a command.
|
|
*
|
|
* Returns `true` if the spawn was attempted, `false` if the open could
|
|
* not proceed — covers (a) URL rejected by `parseSafeUrl` (non-http(s),
|
|
* malformed, etc.), (b) no known opener for the current platform
|
|
* (`openCommand` returned null), or (c) `spawn()` threw synchronously
|
|
* before the child was created. Async failures after spawn (`'error'`
|
|
* event because the binary couldn't exec) still return `true` because
|
|
* the spawn was attempted — the no-op error listener absorbs the event
|
|
* so the TUI doesn't crash, and the user just doesn't see their browser
|
|
* pop.
|
|
*/
|
|
export function openExternalUrl(rawUrl: string, dependencies: OpenDependencies = {}): boolean {
|
|
const url = parseSafeUrl(rawUrl)
|
|
|
|
if (!url) {
|
|
return false
|
|
}
|
|
|
|
const spawnFn = dependencies.spawn ?? spawn
|
|
const platformId = dependencies.platform?.() ?? platform()
|
|
|
|
const command = openCommand(platformId)
|
|
|
|
if (!command) {
|
|
return false
|
|
}
|
|
|
|
try {
|
|
const child = spawnFn(command.command, [...command.args, url.toString()], {
|
|
// Detach so closing the TUI later doesn't kill the browser process,
|
|
// and ignore stdio so we don't leak FDs into our raw-mode terminal.
|
|
// Without `ignore` here, Chrome's stderr can land in the alt screen.
|
|
detached: true,
|
|
stdio: 'ignore'
|
|
} satisfies SpawnOptions)
|
|
|
|
// Async failure path: spawn returns a ChildProcess synchronously even
|
|
// when the binary is missing (ENOENT on `xdg-open` / `explorer.exe`),
|
|
// unreachable (EACCES), or otherwise unusable — the failure surfaces
|
|
// later as an 'error' event. Without a handler, an unhandled 'error'
|
|
// on an EventEmitter crashes Node, which would tear down the whole
|
|
// TUI. Attach a no-op listener BEFORE unref() so the event has a
|
|
// consumer; we already returned `true` synchronously, so the user
|
|
// just won't see their browser open — same as if the URL had been
|
|
// rejected upstream.
|
|
child.once('error', () => {
|
|
// Intentional no-op. The TUI keeps running; user gets no browser
|
|
// pop, which is the failure mode we promised in the doc comment.
|
|
})
|
|
|
|
child.unref()
|
|
|
|
return true
|
|
} catch {
|
|
// spawn can also throw synchronously on argv-validation failures
|
|
// (e.g. NUL in the path). Treat it as a no-op rather than crashing.
|
|
return false
|
|
}
|
|
}
|
|
|
|
export type OpenDependencies = {
|
|
spawn?: typeof spawn
|
|
platform?: () => string
|
|
}
|
|
|
|
/**
|
|
* Validate and normalize a URL for opening externally.
|
|
* Exported for testing.
|
|
*/
|
|
export function parseSafeUrl(value: string): null | URL {
|
|
if (!value || typeof value !== 'string') {
|
|
return null
|
|
}
|
|
|
|
let parsed: URL
|
|
|
|
try {
|
|
parsed = new URL(value)
|
|
} catch {
|
|
return null
|
|
}
|
|
|
|
// http(s) only — opening file://, data:, javascript:, vbscript:, etc.
|
|
// would let a malicious model run a local handler with attacker-controlled
|
|
// input on a single click.
|
|
if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') {
|
|
return null
|
|
}
|
|
|
|
// Reject empty or all-whitespace hostnames defensively. URL parsing
|
|
// accepts URLs like 'http:///foo' on some Node versions; we don't want
|
|
// to forward those to `open`.
|
|
if (!parsed.hostname.trim()) {
|
|
return null
|
|
}
|
|
|
|
return parsed
|
|
}
|
|
|
|
type OpenCommand = { command: string; args: readonly string[] }
|
|
|
|
/**
|
|
* Per-platform open command. We deliberately avoid `cmd.exe /c start` on
|
|
* Windows even though it's the canonical example, because `start` is a cmd
|
|
* builtin: the URL string is reparsed by cmd's command-line tokenizer and
|
|
* characters like `&`, `|`, `^`, `<`, `>` either break the command or get
|
|
* interpreted as additional commands. That undermines the protocol
|
|
* allowlist's safety story and also breaks plain http(s) URLs with `&` in
|
|
* query strings. `explorer.exe <url>` is the safe, non-shell alternative —
|
|
* it invokes the registered protocol handler for http(s) without going
|
|
* through cmd. Linux/BSD use `xdg-open` directly with no shell wrapping.
|
|
*
|
|
* Returns null for platforms where we don't know a safe opener (e.g. `aix`,
|
|
* `sunos`, `cygwin`). The caller's `if (!command) return false` path then
|
|
* surfaces "no opener" instead of optimistically trying `xdg-open` on a
|
|
* platform that probably doesn't have it.
|
|
*/
|
|
export function openCommand(platformId: string): OpenCommand | null {
|
|
if (platformId === 'darwin') {
|
|
return { command: 'open', args: [] }
|
|
}
|
|
|
|
if (platformId === 'win32') {
|
|
return { command: 'explorer.exe', args: [] }
|
|
}
|
|
|
|
// Linux + the BSD family ship xdg-open via xdg-utils. Everything else
|
|
// (aix, sunos, cygwin, haiku, etc.) returns null so openExternalUrl's
|
|
// command-not-found fallback fires honestly.
|
|
const XDG_OPEN_PLATFORMS = new Set(['linux', 'freebsd', 'openbsd', 'netbsd', 'dragonfly'])
|
|
|
|
if (XDG_OPEN_PLATFORMS.has(platformId)) {
|
|
return { command: 'xdg-open', args: [] }
|
|
}
|
|
|
|
return null
|
|
}
|