mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-06-25 11:02:03 +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.
217 lines
8.8 KiB
TypeScript
217 lines
8.8 KiB
TypeScript
import type { ChildProcess, spawn as SpawnFn } from 'node:child_process'
|
|
import { EventEmitter } from 'node:events'
|
|
|
|
import { describe, expect, it, vi } from 'vitest'
|
|
|
|
import { openCommand, openExternalUrl, parseSafeUrl } from './openExternalUrl.js'
|
|
|
|
type SpawnLike = typeof SpawnFn
|
|
|
|
describe('parseSafeUrl', () => {
|
|
it('accepts http and https URLs', () => {
|
|
expect(parseSafeUrl('https://example.com')?.href).toBe('https://example.com/')
|
|
expect(parseSafeUrl('http://example.com/path?q=1')?.href).toBe('http://example.com/path?q=1')
|
|
})
|
|
|
|
it('rejects file: URLs (would let a hostile model trigger arbitrary local handlers)', () => {
|
|
expect(parseSafeUrl('file:///etc/passwd')).toBeNull()
|
|
})
|
|
|
|
it('rejects javascript:, data:, and vbscript: URLs', () => {
|
|
expect(parseSafeUrl('javascript:alert(1)')).toBeNull()
|
|
expect(parseSafeUrl('data:text/html,<script>alert(1)</script>')).toBeNull()
|
|
expect(parseSafeUrl('vbscript:msgbox')).toBeNull()
|
|
})
|
|
|
|
it('rejects mailto:, ftp:, and other non-web protocols', () => {
|
|
expect(parseSafeUrl('mailto:test@example.com')).toBeNull()
|
|
expect(parseSafeUrl('ftp://example.com')).toBeNull()
|
|
expect(parseSafeUrl('ssh://example.com')).toBeNull()
|
|
})
|
|
|
|
it('rejects unparseable strings', () => {
|
|
expect(parseSafeUrl('not a url')).toBeNull()
|
|
expect(parseSafeUrl('')).toBeNull()
|
|
})
|
|
|
|
it('rejects non-string inputs defensively', () => {
|
|
expect(parseSafeUrl(undefined as unknown as string)).toBeNull()
|
|
expect(parseSafeUrl(null as unknown as string)).toBeNull()
|
|
expect(parseSafeUrl(123 as unknown as string)).toBeNull()
|
|
})
|
|
})
|
|
|
|
describe('openCommand', () => {
|
|
it('returns macOS open(1) on darwin', () => {
|
|
expect(openCommand('darwin')).toEqual({ command: 'open', args: [] })
|
|
})
|
|
|
|
it('routes through explorer.exe on win32 — not cmd.exe — so URLs with & | ^ < > stay safe', () => {
|
|
// win32 must not route through cmd.exe — see comment in openCommand.
|
|
// Test pins the contract that we use explorer.exe (non-shell) so URLs
|
|
// with `&`/`|`/`^`/`<`/`>` aren't reparsed by cmd's tokenizer.
|
|
const cmd = openCommand('win32')
|
|
expect(cmd?.command).toBe('explorer.exe')
|
|
expect(cmd?.args).toEqual([])
|
|
})
|
|
|
|
it('falls back to xdg-open on linux/bsd', () => {
|
|
expect(openCommand('linux')).toEqual({ command: 'xdg-open', args: [] })
|
|
expect(openCommand('freebsd')).toEqual({ command: 'xdg-open', args: [] })
|
|
expect(openCommand('openbsd')).toEqual({ command: 'xdg-open', args: [] })
|
|
})
|
|
|
|
it('returns null for unknown platforms (aix, sunos, cygwin, etc.)', () => {
|
|
// Avoid optimistically dispatching xdg-open on platforms where it
|
|
// probably isn't installed — the caller's `if (!command) return false`
|
|
// path surfaces "no opener" honestly instead.
|
|
expect(openCommand('aix')).toBeNull()
|
|
expect(openCommand('sunos')).toBeNull()
|
|
expect(openCommand('cygwin')).toBeNull()
|
|
expect(openCommand('haiku')).toBeNull()
|
|
expect(openCommand('')).toBeNull()
|
|
})
|
|
})
|
|
|
|
describe('openExternalUrl on unsupported platforms', () => {
|
|
it('returns false without spawning when the platform has no known opener', () => {
|
|
const spawn = vi.fn() as unknown as SpawnLike
|
|
|
|
expect(openExternalUrl('https://example.com/', { spawn, platform: () => 'aix' })).toBe(false)
|
|
expect(spawn).not.toHaveBeenCalled()
|
|
})
|
|
})
|
|
|
|
describe('openExternalUrl', () => {
|
|
// Tracks the most recent fake child so tests can inspect its 'error'
|
|
// handlers and emit on it. Use a loose EventEmitter alias rather than
|
|
// ChildProcess — the latter's `unref` signature is strictly `() => void`
|
|
// and doesn't accept `vi.fn()` without a generic.
|
|
type FakeChild = EventEmitter & { unref: () => void }
|
|
|
|
function mockSpawn(): {
|
|
spawn: SpawnLike
|
|
calls: Array<{ command: string; args: readonly string[] }>
|
|
lastChild: () => FakeChild | undefined
|
|
} {
|
|
const calls: Array<{ command: string; args: readonly string[] }> = []
|
|
let lastChild: FakeChild | undefined
|
|
|
|
const spawn = vi.fn((command: string, args: readonly string[]) => {
|
|
calls.push({ command, args })
|
|
|
|
// Use a real EventEmitter so .once('error', cb) wires up correctly
|
|
// and we can synthesize async failures by emitting 'error' from the
|
|
// test. The cast is the same one Node uses internally — ChildProcess
|
|
// extends EventEmitter.
|
|
const child = new EventEmitter() as FakeChild
|
|
|
|
child.unref = () => {}
|
|
lastChild = child
|
|
|
|
return child as unknown as ChildProcess
|
|
}) as unknown as SpawnLike
|
|
|
|
return { spawn, calls, lastChild: () => lastChild }
|
|
}
|
|
|
|
it('opens a normal https URL via the platform command', () => {
|
|
const { spawn, calls } = mockSpawn()
|
|
|
|
expect(openExternalUrl('https://example.com/foo', { spawn, platform: () => 'darwin' })).toBe(true)
|
|
expect(calls).toHaveLength(1)
|
|
expect(calls[0]!.command).toBe('open')
|
|
expect(calls[0]!.args).toEqual(['https://example.com/foo'])
|
|
})
|
|
|
|
it('uses xdg-open on linux', () => {
|
|
const { spawn, calls } = mockSpawn()
|
|
|
|
openExternalUrl('https://example.com/', { spawn, platform: () => 'linux' })
|
|
expect(calls[0]!.command).toBe('xdg-open')
|
|
})
|
|
|
|
it('refuses to open file: URLs and does not spawn', () => {
|
|
const { spawn, calls } = mockSpawn()
|
|
|
|
expect(openExternalUrl('file:///etc/passwd', { spawn, platform: () => 'darwin' })).toBe(false)
|
|
expect(calls).toHaveLength(0)
|
|
})
|
|
|
|
it('refuses to open javascript: URLs and does not spawn', () => {
|
|
const { spawn, calls } = mockSpawn()
|
|
|
|
expect(openExternalUrl('javascript:alert(1)', { spawn, platform: () => 'darwin' })).toBe(false)
|
|
expect(calls).toHaveLength(0)
|
|
})
|
|
|
|
it('passes URLs containing shell metacharacters as plain args (no shell interpolation)', () => {
|
|
const { spawn, calls } = mockSpawn()
|
|
|
|
// A URL with `; & ` plus URL-encoded backticks. spawn(..., args) without
|
|
// shell:true means the OS receives these as a single argv element.
|
|
const hostile = 'https://example.com/path%3Bevil%20%26%20rm%20-rf'
|
|
|
|
openExternalUrl(hostile, { spawn, platform: () => 'darwin' })
|
|
expect(calls).toHaveLength(1)
|
|
expect(calls[0]!.args[calls[0]!.args.length - 1]).toBe(hostile)
|
|
})
|
|
|
|
it('on win32, a URL with & | ^ < > is forwarded as a single argv element via explorer.exe', () => {
|
|
const { spawn, calls } = mockSpawn()
|
|
|
|
// Plain http URL with & in query (very common, e.g. analytics params)
|
|
// plus other cmd metacharacters that would split or reinterpret the
|
|
// command if win32 routed through cmd.exe /c start. Note that the URL
|
|
// parser percent-encodes `<` and `>` (which is fine — encoded forms
|
|
// can't be reinterpreted by any shell), but `&`, `|`, `^` survive
|
|
// and would tokenize cmd.exe if we ever regressed back to it.
|
|
const meta = 'https://example.com/q?a=1&b=2|c^d<e>f'
|
|
|
|
expect(openExternalUrl(meta, { spawn, platform: () => 'win32' })).toBe(true)
|
|
expect(calls).toHaveLength(1)
|
|
expect(calls[0]!.command).toBe('explorer.exe')
|
|
// The URL must arrive as exactly one argv element — not split on &/|/^/etc.
|
|
const forwarded = calls[0]!.args[0]!
|
|
expect(calls[0]!.args).toHaveLength(1)
|
|
expect(forwarded).toContain('a=1&b=2')
|
|
expect(forwarded).toContain('|c^d')
|
|
})
|
|
|
|
it('on win32, common http URLs with & query params are forwarded intact', () => {
|
|
const { spawn, calls } = mockSpawn()
|
|
const url = 'https://example.com/search?q=foo&page=2&utm_source=hermes'
|
|
|
|
openExternalUrl(url, { spawn, platform: () => 'win32' })
|
|
expect(calls[0]!.args).toEqual([url])
|
|
})
|
|
|
|
it('returns false on synchronous spawn failure', () => {
|
|
const spawn = vi.fn(() => {
|
|
throw new Error('ENOENT')
|
|
}) as unknown as SpawnLike
|
|
|
|
expect(openExternalUrl('https://example.com/', { spawn, platform: () => 'linux' })).toBe(false)
|
|
})
|
|
|
|
it('does not crash the host when the spawned process emits an async error', () => {
|
|
// Real-world case: `xdg-open` / `explorer.exe` missing on PATH. spawn()
|
|
// returns a ChildProcess synchronously, then emits 'error' once the
|
|
// exec actually fails. Without a registered 'error' listener, Node
|
|
// re-throws the event as an uncaught exception → TUI dies. We attach
|
|
// a no-op listener inside openExternalUrl; this test pins that contract.
|
|
const { spawn, lastChild } = mockSpawn()
|
|
|
|
expect(openExternalUrl('https://example.com/', { spawn, platform: () => 'linux' })).toBe(true)
|
|
|
|
const child = lastChild()
|
|
expect(child).toBeDefined()
|
|
// Must have a listener registered BEFORE we emit, or EventEmitter will
|
|
// throw synchronously here (which is exactly the crash we're preventing).
|
|
expect(child!.listenerCount('error')).toBeGreaterThan(0)
|
|
|
|
// Emit and assert it doesn't throw. If the listener weren't attached,
|
|
// this would throw 'Unhandled error' and fail the test.
|
|
expect(() => child!.emit('error', new Error('ENOENT: xdg-open not found'))).not.toThrow()
|
|
})
|
|
})
|