Merge branch 'main' of github.com:NousResearch/hermes-agent into bb/gui

ui-tui/src/__tests__/textInputCursorSourceOfTruth.test.ts

@@ -0,0 +1,50 @@
+import { readFileSync } from 'node:fs'
+import { dirname, join } from 'node:path'
+import { fileURLToPath } from 'node:url'
+
+import { describe, expect, it } from 'vitest'
+
+// Locate textInput.tsx relative to this test file so the assertion
+// survives moves of the test fixture itself.
+const TEXT_INPUT_PATH = join(dirname(fileURLToPath(import.meta.url)), '..', 'components', 'textInput.tsx')
+const source = readFileSync(TEXT_INPUT_PATH, 'utf8')
+
+// Closes Copilot follow-up on PR #26717: the original cursor-drift
+// fix bumped Ink's displayCursor / cursorDeclaration on fast-echo, but
+// if TextInput itself re-renders before the deferred 16ms `setCur`
+// flushes (parent state change, status-bar tick, spinner) the layout
+// effect inside `useDeclaredCursor` re-publishes a declaration
+// computed from the STALE React `cur` state and clobbers the Ink-level
+// bump. The fix is structural: read `curRef.current` (always
+// up-to-date) when computing the layout, not the `cur` state.
+//
+// This file pins that invariant. Switching back to `cur` state — or
+// re-introducing a memo keyed on `cur` that uses `curRef.current`
+// inside but stops re-computing on rerender — is a regression and
+// should be caught here, not via a flaky integration test that mounts
+// Ink + stdin.
+describe('textInput cursor-layout source of truth', () => {
+  it('reads curRef.current (not the cur React state) for cursorLayout', () => {
+    // The line we care about. We allow whitespace / formatting drift,
+    // but the call itself must use `curRef.current`.
+    expect(source).toMatch(/cursorLayout\(\s*display\s*,\s*curRef\.current\s*,\s*columns\s*\)/)
+  })
+
+  it('does not pass the bare `cur` React state into cursorLayout', () => {
+    // Any `cursorLayout(display, cur, columns)` invocation would
+    // reintroduce the stale-declaration window.
+    expect(source).not.toMatch(/cursorLayout\(\s*display\s*,\s*cur\s*,\s*columns\s*\)/)
+  })
+
+  it('keeps the fast-echo notifier calls paired with the stdout writes', () => {
+    // Both fast-echo paths must call noteCursorAdvance, otherwise Ink
+    // never learns about the out-of-band write and drifts again. We
+    // tolerate explanatory comments in between (the rationale block is
+    // intentionally long), but the pairing itself must hold.
+    const backspacePattern = /stdout!\.write\(['"`]\\b \\b['"`]\)[\s\S]{0,1000}?noteCursorAdvance\(-1\)/
+    expect(source).toMatch(backspacePattern)
+
+    const appendPattern = /stdout!\.write\(text\)[\s\S]{0,1000}?noteCursorAdvance\(text\.length\)/
+    expect(source).toMatch(appendPattern)
+  })
+})

ui-tui/src/__tests__/textInputFastEcho.test.ts

@@ -133,4 +133,42 @@ describe('canFastBackspaceShape', () => {
   it('rejects deleting an emoji', () => {
     expect(canFastBackspaceShape('hi🙂', 'hi🙂'.length)).toBe(false)
   })
+
+  // Closes Copilot PR #26717 round 3: the "\b \b" sequence cannot move
+  // the terminal cursor onto the previous visual row across a
+  // soft-wrap boundary. When the caret sits at visual column 0 of a
+  // wrapped row (column == 0 in the computed cursor layout), backspace
+  // would leave the physical cursor in place while the logical caret
+  // moves up to the end of the previous visual line — desyncing both
+  // Ink's displayCursor model and the user-visible position. The fast
+  // path must fall through in that case so the normal Ink render path
+  // can lay out the correct cursor position.
+  it('rejects fast-backspace at a soft-wrap boundary when columns is known', () => {
+    // value width 6 in a column of 6 → cursorLayout produces (line 1, col 0)
+    // i.e. the caret has overflowed onto the next visual line.
+    const value = 'hello '
+    expect(canFastBackspaceShape(value, value.length, 6)).toBe(false)
+  })
+
+  it('rejects fast-backspace at an exact multiple of columns (wide wrap)', () => {
+    // 12 chars at width 6 → two full visual rows, caret at (line 2, col 0).
+    const value = 'abcdefghijkl'
+    expect(canFastBackspaceShape(value, value.length, 6)).toBe(false)
+  })
+
+  it('still accepts fast-backspace inside a wrapped line', () => {
+    // Caret mid-visual-line — "\b \b" can move the cursor one cell left
+    // without crossing a wrap boundary.
+    expect(canFastBackspaceShape('hello world', 'hello world'.length, 20)).toBe(true)
+    expect(canFastBackspaceShape('abcdefghi', 9, 6)).toBe(true) // visual line 1, col 3 → ok
+  })
+
+  it('skips the wrap-boundary check when columns is omitted (legacy contract)', () => {
+    // Callers that don't pass `columns` fall back to the pre-wrap-aware
+    // behavior — the function does NOT magically reject anything that
+    // could be a wrap boundary without the width. Production callers
+    // must always pass `columns`; this case is for unit tests of the
+    // pre-wrap shape contract.
+    expect(canFastBackspaceShape('hello ', 'hello '.length)).toBe(true)
+  })
 })

ui-tui/src/components/textInput.tsx

@@ -16,13 +16,14 @@ import {
 
 type InkExt = typeof Ink & {
   stringWidth: (s: string) => number
+  useCursorAdvance: () => (dx: number, dy?: number) => void
   useDeclaredCursor: (a: { line: number; column: number; active: boolean }) => (el: any) => void
   useStdout: () => { stdout?: NodeJS.WriteStream }
   useTerminalFocus: () => boolean
 }
 
 const ink = Ink as unknown as InkExt
-const { Box, Text, useStdin, useInput, useStdout, stringWidth, useDeclaredCursor, useTerminalFocus } = ink
+const { Box, Text, useStdin, useInput, useStdout, stringWidth, useCursorAdvance, useDeclaredCursor, useTerminalFocus } = ink
 
 const ESC = '\x1b'
 const INV = `${ESC}[7m`
@@ -238,8 +239,26 @@ export function canFastAppendShape(
  * ASCII. Anything else (combining marks, IME compositions, wide chars,
  * tabs, ANSI fragments) goes through the normal render path so Ink can
  * recompute cell widths.
+ *
+ * When `columns` is supplied, ALSO rejects when the physical cursor
+ * sits at visual column 0 — i.e., right after a soft-wrap boundary.
+ * The "\b \b" sequence cannot move the cursor onto the previous visual
+ * row (terminals don't back-step across line wraps), so the physical
+ * cursor would stay put while the logical caret moves to the end of
+ * the previous visual line, desyncing both Ink's `displayCursor` model
+ * and the user-visible position.
+ *
+ * When `columns` is OMITTED, the wrap-boundary check is skipped
+ * entirely and the function reverts to the legacy non-wrap-aware
+ * contract — values like `'hello '` will return `true` even though
+ * they would be unsafe at a width of 6. Production callers (the
+ * composer's `canFastBackspace` helper) always pass `columns`;
+ * `columns` is optional only so unit tests of the pre-wrap shape
+ * contract can keep calling the helper without threading width
+ * through. Do NOT omit it from any new caller that relies on the
+ * wrap-boundary protection.
  */
-export function canFastBackspaceShape(current: string, cursor: number): boolean {
+export function canFastBackspaceShape(current: string, cursor: number, columns?: number): boolean {
   if (cursor !== current.length) {
     return false
   }
@@ -252,6 +271,13 @@ export function canFastBackspaceShape(current: string, cursor: number): boolean
     return false
   }
 
+  // If we know the wrap width, reject at the soft-wrap boundary: the
+  // caret's visual column is 0, so "\b \b" can't represent the physical
+  // move back to the previous visual line.
+  if (columns !== undefined && cursorLayout(current, cursor, columns).column === 0) {
+    return false
+  }
+
   const removed = current.slice(prevPos(current, cursor), cursor)
 
   return ASCII_PRINTABLE_RE.test(removed)
@@ -333,6 +359,7 @@ export function TextInput({
   const fwdDel = useFwdDelete(focus)
   const termFocus = useTerminalFocus()
   const { stdout } = useStdout()
+  const noteCursorAdvance = useCursorAdvance()
 
   const curRef = useRef(cur)
   const selRef = useRef<null | { end: number; start: number }>(null)
@@ -368,7 +395,19 @@ export function TextInput({
     [sel]
   )
 
-  const layout = useMemo(() => cursorLayout(display, cur, columns), [columns, cur, display])
+  // Read `curRef.current` (always up-to-date) rather than the `cur`
+  // React state. The fast-echo path defers the React `setCur` by 16ms
+  // to batch re-renders during heavy typing; if an unrelated render
+  // flushes this component during that window and we used the stale
+  // `cur` state here, the layout effect inside `useDeclaredCursor`
+  // would publish a stale cursor declaration and clobber the Ink-level
+  // bump from `noteCursorAdvance(...)`. `cur` is still in scope and
+  // referenced by setSel/setCur paths below, so React tracks the
+  // dependency naturally — we just don't use it as the source of truth
+  // for layout. The cursorLayout call is cheap (one wrap-text pass
+  // over a single-line string in the common case), so dropping useMemo
+  // is fine.
+  const layout = cursorLayout(display, curRef.current, columns)
 
   const boxRef = useDeclaredCursor({
     line: layout.line,
@@ -526,7 +565,7 @@ export function TextInput({
     canFastEchoBase() && canFastAppendShape(current, cursor, text, columns, lineWidthRef.current)
 
   const canFastBackspace = (current: string, cursor: number) =>
-    canFastEchoBase() && canFastBackspaceShape(current, cursor)
+    canFastEchoBase() && canFastBackspaceShape(current, cursor, columns)
 
   const commit = (
     next: string,
@@ -911,6 +950,12 @@ export function TextInput({
           v = v.slice(0, t) + v.slice(c)
           c = t
           stdout!.write('\b \b')
+          // The "\b \b" sequence ends with the cursor one column to the
+          // LEFT of where Ink last parked it. Tell Ink so its `displayCursor`
+          // (and log-update's relative-move basis on the next frame) stays
+          // in sync — otherwise the cursor parks one cell to the right of
+          // the caret on the next unrelated re-render.
+          noteCursorAdvance(-1)
           commit(v, c, true, false, false, Math.max(0, lineWidthRef.current - 1))
 
           return
@@ -998,6 +1043,14 @@ export function TextInput({
 
             if (simpleAppend) {
               stdout!.write(text)
+              // ASCII-printable text advances the physical cursor by exactly
+              // text.length cells (canFastAppendShape rejects non-ASCII,
+              // wide chars, newlines). Notify Ink so the cached displayCursor
+              // / log-update relative-move basis advances with it; otherwise
+              // any unrelated re-render that happens before the 16ms
+              // setCur/setParent flush parks the cursor text.length cells
+              // too far right (#cursor-drift).
+              noteCursorAdvance(text.length)
               commit(v, c, true, false, false, lineWidthRef.current + stringWidth(text))
 
               return

ui-tui/src/types/hermes-ink.d.ts

@@ -164,6 +164,7 @@ declare module '@hermes/ink' {
     readonly column: number
     readonly active: boolean
   }): (el: unknown) => void
+  export function useCursorAdvance(): (dx: number, dy?: number) => void
   export function useStdin(): {
     readonly stdin: NodeJS.ReadStream
     readonly setRawMode: (value: boolean) => void