Conversation
Introduce a subprocess-under-PTY test harness so the shell's rendering can be exercised end-to-end instead of by hand in an example console. TestMain re-execs the test binary as the process-under-test (gated by READLINE_PTY_CHILD) and runs a single Readline() round with a deterministic prompt. The console helper spawns that child under a real PTY (creack/pty), mirrors its output into a vt10x virtual terminal for screen assertions, and auto-responds to ESC[6n cursor-position queries so GetCursorPos() does not block consuming keystrokes. TestPTYPromptAndInput covers prompt render -> input echo (with a first-line alignment guard for #98) -> line acceptance. go mod tidy also drops the unused golang.org/x/exp and golang.org/x/term direct requirements (no imports anywhere in the tree). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add golden-screen tests that pin down issue #98 (and its downstream report reeflective/console#78, "Prompt string overlaps with input at the bottom"). Findings, established with the PTY harness: - The rendering math is correct whenever startCols/startRows are correct (verified across wrapping and mid-line editing) -- #98 is a row/column bookkeeping-robustness problem, not a layout-logic problem. - The representative real-world trigger is a MULTI-LINE prompt rendered at the bottom of the window: ensureInputSpace() reserves room for the input rows (e.lineRows) but never for the prompt rows (prompt.PrimaryUsed()), so the terminal scrolls underneath the prompt and its last (input) line is lost. New tests: - TestPTYWrappingAlignment: regression guard, passes today. - TestRepro98MultilinePromptAtBottom: faithful repro, truthful terminal. - TestRepro98ProbeMisalignment: related facet, wrong ESC[6n reply (#101). The two repros assert the desired behavior and currently fail, so they are gated behind RUN_REPRO98 to keep CI green; unskip when the fix lands. The harness gains an injectable DSR probe responder and a prefill option (push the prompt down the window). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When a multi-line primary prompt is rendered at the bottom of the terminal window, the input area's trailing row does not exist, so the terminal clamps the engine's downward cursor moves while the paired upward moves still travel. The row bookkeeping (startRows/cursorRow) drifts and the prompt's lines get overwritten -- the prompt "overlaps with the input line" reported in #98 and reeflective/console#78. ensureInputSpace() previously reserved space only for the input rows and bailed out entirely for single-row input (lineRows <= 1), so a multi-line prompt with a normal input line at the bottom got no reservation at all. It now reserves lineRows+1 rows (the input rows plus one trailing row) below startRows and scrolls the screen up by the deficit, computed from the already-probed startRows so no extra ESC[6n query is issued. Tests move under internal/display (external display_test package, so they can drive the full shell without an import cycle) with issue-neutral names: - TestRenderPromptAndInput, TestRenderWrappingAlignment - TestRenderMultilinePromptAtBottom: regression guard, passes with the fix - TestRenderMisreportedCursor: a separate cursor-probe robustness gap, gated behind READLINE_RUN_KNOWN_GAPS so CI stays green Known remaining (pre-existing) edge case: a multi-line prompt AND a heavily wrapping input both at the very bottom of a small window can still scroll the topmost prompt line off, because upper prompt lines are printed once and not repainted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The upper lines of a multi-line primary prompt were printed only once (by PrimaryPrint at startup); Refresh only repaints the last prompt line via LastPrint. So when the view scrolled -- e.g. a multi-line prompt at the bottom of the window whose input also wraps -- the upper lines were left stale or scrolled away and never redrawn, dropping the top of the prompt. Add Prompt.UpperPrint(), which reprints every prompt line except the last, and call it from Refresh (via repaintPromptUpperLines) after ensureInputSpace has settled the start row. The repaint is anchored at startRows-primaryRows and happens while the cursor is hidden, so there is no flicker. This completes the bottom-of-window fix for the compound prompt-wraps-too case. Adds TestRenderMultilinePromptWrapAtBottom as a regression guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Some environments -- PTY test harnesses, minimal terminal emulators, constrained
CI -- do not reliably answer the cursor-position query (ESC[6n), which the
display engine uses to find the line's start column/row. Add a public option to
turn that probing off.
New var "cursor-position-probe" (default true) in the library's custom-option
registry (internal/keymap/config.go readlineOptions). Toggle via
rl.Config.Set("cursor-position-probe", false) or `.inputrc`:
set cursor-position-probe off.
When disabled, computeCoordinates skips GetCursorPos and falls back to the
printed prompt width for the start column (exact whenever the input starts at
column 0, the common case); startRows is left unknown and ensureInputSpace
no-ops (guarded on startRows < 1). Documented degraded behavior: bottom-of-window
scroll reservation is not performed without probing.
Tests (internal/display):
- TestCursorProbeEnabledByDefault: a normal render issues >= 1 ESC[6n query.
- TestRenderWithCursorProbeDisabled: with probing off, no ESC[6n is sent and the
line renders correctly even against a terminal that lies about the cursor.
The harness gains consoleConfig.noProbe and probeQueries(); this replaces the
previously gated known-gap test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add distinct hint lanes so passive hinting, async status reporting and completion hints coexist without clobbering one another (#103): - provided: passive hint computed from the current line by a provider (ui.Hint.SetProvider), re-evaluated on every refresh by the display engine. - transient: one-shot async status messages (ui.Hint.SetTransient / ClearTransient), NOT cleared by completion/isearch activity. Render order, top to bottom: persistent status, provided, transient, completion/isearch. ui.Hint gains a mutex so the transient lane is safe to write from another goroutine while the main loop renders. The public API stays on the already-exported rl.Hint; nothing is added to Shell. This is Phase 1 (lanes + API + thread-safe state): an async transient set while the shell is idle becomes visible at the next redisplay. Immediate repaint of an idle shell (the async-refresh wake) is deferred to Phase 2, which will share its wake primitive with completion regeneration (#99). Tests (internal/display, also run under -race, which instruments the re-exec'd child shell): provider tracks the line; provider renders above transient; transient survives incremental search and renders above the completion lane. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Readline loop blocks in a raw stdin read, so a UI update pushed from
another goroutine (e.g. ui.Hint.SetTransient) only appeared at the next
keystroke. Add a wake primitive so an idle loop repaints immediately, while
keeping all rendering on the Readline goroutine (background goroutines only
mutate locked state and request a wake — never touch the display).
Implementation (Unix): interrupt the single blocking read in readInputFiltered
with poll(2) over [stdin, wake-pipe]. A wake returns errInputWake (no keys);
WaitAvailableKeys returns empty and the main loop continues, repainting and
re-waiting. Real input is polled first so a coincident wake never delays a
keystroke. This leaves cursor probing, macros and the Windows reader untouched;
when no pollable fd / pipe is available it falls back to a plain blocking read
(updates then coalesce into the next keystroke), as on Windows (no-op stubs).
- core.Keys.RequestRefresh() is the general wake primitive, safe from any
goroutine (guarded by wakeMu vs InitWake/CloseWake). Reachable via the
already-public rl.Keys; nothing added to Shell.
- ui.Hint.SetTransient/ClearTransient now wake the loop through a refresh hook
the shell wires to keys.RequestRefresh.
This is the shared rail for #103 async hints and (next) #99 completion
regeneration + the pre-existing resize-watcher race.
Tests (internal/display, also under -race which instruments the re-exec'd
child): a transient pushed from another goroutine while idle appears with no
keystroke; the async repaint keeps the prompt rendered once and input aligned.
Also hardened TestCursorProbeEnabledByDefault against a pre-existing
probe-vs-prompt timing window exposed under -race.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add Shell.RefreshCompletions(): regenerate the currently active completion menu from the cached completer and repaint, so completions produced asynchronously (a background producer updating a cache the completer reads) appear in place without a keystroke. Safe from any goroutine; clean no-op when no menu is active; selection resets as the grid is rebuilt (as on resize). It rides the async-refresh wake rail: RequestRegen() sets an atomic flag from any goroutine, the Readline loop applies it (ApplyRegen -> GenerateCached) on its own goroutine before the refresh, and RequestRefresh() wakes an idle loop. Rendering and completion-state mutation thus stay single-writer; no mutex on the completion engine is needed. A Shell method (not a sub-object method like the Hint/Keys async APIs) because the completion engine is intentionally unexported — one method is a far smaller public surface than exposing the engine. Also re-route the Unix SIGWINCH resize watcher through the same rail (RequestRegen + RequestRefresh) instead of calling GenerateCached + Refresh directly from the signal goroutine, fixing a pre-existing latent data race between resize handling and the main loop. Windows resize is unchanged (no wake support there yet). Tests (internal/display, also under -race): an async producer's new candidate appears in an open menu with no keystroke; RefreshCompletions no-ops when no menu is active. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CoordinatesHint counted the rendered hint string by splitting on the per-line clear sequence and running LineSpan on each piece. LineSpan adds a row for every non-first piece (idx != 0), and the loop then added another row for the piece's content — double-counting every lane after the first. It returned 2N-1 rows for N hint lanes (3 for 2, 5 for 3, ...). That over-count fed the helper-area repaint (renderHelpers moves the cursor up by hintRows+1), so when two or more hint lanes were shown at once the cursor was moved up one row too far. Each refresh then re-probed the cursor one row higher, drifting the whole prompt/UI upward by one row per refresh. It surfaced wherever two lanes coincided — a passive provider hint plus an async transient hint, or a provider hint plus a completion usage hint — and was especially visible across async-refresh wakes (the prompt crept up with every async update). Count rows directly from the lanes instead: one row per non-empty lane, ceiling for wrapping and embedded newlines. This matches exactly what DisplayHint prints (one NewlineReturn per lane), so the repaint balances and the prompt stays put. Tests: internal/ui TestCoordinatesHintRowCount (1..4 lanes); internal/display drift tests + new harness hooks (autocomplete, repeated async transient, right-side prompt) for prompt-stability coverage. The bug only reproduces against a faithful terminal (vt10x self-corrects via its DSR replies when driving), so the unit test is the precise regression guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Previously, while a completion candidate was selected, an async
RefreshCompletions left the menu frozen: regeneration ran against the
*completed* line (the one carrying the virtually inserted candidate), so the
completion context was skewed and the grid could not update until the selection
was cleared.
Make async regeneration selection-aware (regenPreservingSelection, run on the
main loop from ApplyRegen):
- capture the selected candidate's identity by CONTENT (tag+value) — cell
coordinates and group pointers do not survive prepare(), which rebuilds and
re-sorts the groups as new results arrive;
- drop the virtual insertion so the completer regenerates against the REAL
line (otherwise the menu stays frozen);
- rebuild the candidate pool WITHOUT Generate's auto-accept-on-unique, so a
transient single result mid-stream does not commit the line and close menu;
- re-select the same candidate (escape-stripped match); if it is gone from the
new results, leave the menu active with no selection.
Crucially, regeneration falls back to the autocomplete completer when no cached
completer is set: as-you-type menus (the common case in richer frontends such as
reeflective/console) never set e.cached, and would otherwise stay frozen while a
candidate is selected. Incremental search keeps its own selection lifecycle and
is left untouched. All on the Readline goroutine, so no new locks.
Now a selected candidate both stays selected AND sees the menu grow live as
async results arrive, for both explicit and autocomplete-driven menus.
Tests: internal/display TestAsyncRefreshKeepsSelection (explicit menu; the added
value sorts first so the grid re-sorts, proving content-based restore). Also
fixes the harness so consoleConfig.autocomplete actually enables autocomplete in
the child (it was never wired through, so prior autocomplete coverage was inert).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Run the full golangci-lint suite over the codebase and resolve all findings, preserving behavior throughout (key/cursor/parser hot paths were annotated rather than rewritten). Config: - Add a repo-local .golangci.yml derived from the shared reeflective config, disabling the noisy style linters (varnamelen, goconst, mnd, godox) and aligning gocognit/nestif thresholds with cyclop/gocyclo. Code fixes (no behavior change): - gosec: annotate the intentional byte/fd conversions in the key code; swap crypto/md5 for sha256 in temp-file naming. - unused: drop dead code; keep the Windows-only Keys.resize field; fold term.go's duplicated std-stream vars into a single documented termFile (terminal size still measured from stderr, as before). - errcheck/staticcheck/gocritic/makezero/nakedret/recvcheck/prealloc/ revive/wsl/noctx/inamedparam/intrange/err113/dogsled: explicit error handling, fmt simplifications, if-else -> switch, capacity hints, unused params -> _, and assorted idiomatic cleanups. - Add ui.NewHint(keys) constructor wiring the async refresh primitive. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Raise the module go directive to 1.25.0 and update golang.org/x/sys to v0.45.0 and github.com/rivo/uniseg to v0.4.7. Run go mod tidy, dropping stray indirect requirements (cweill/gotests, x/mod, x/sync, x/tools) that had leaked in and are not imported anywhere. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Message and CompleteMessage had identical bodies. Drop the unused Message
(no internal, doc, or consumer references) and keep CompleteMessage, which
is used internally and matches the Complete* naming family. Fix a typo in
its doc comment ("ads" -> "adds").
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two input-reading robustness fixes, originally proposed by @liamg in #96 and #97, adapted to the current async-refresh code. #97: a non-EOF input read error (e.g. a revoked tty) was swallowed and retried forever, spinning the input loop. WaitAvailableKeys now records the first such error in Keys.readErr, exposed via ReadError(); Readline() returns it and bracketedPasteBegin() aborts on it. The async-refresh wake sentinel (errInputWake) is explicitly excluded so legitimate idle-repaint wakes are not mistaken for failures. #96: guard ReadKey()'s waiting branch against an empty/closed key buffer so Vim character-search motions (f/F/t/T) abort cleanly instead of indexing an empty slice and panicking. The default read branch was already hardened by the async-refresh rework. Adds regression tests for both (EOF vs non-EOF read errors, empty-read skip). Co-Authored-By: liamg <11870060+liamg@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
match() rebuilt the comparison string and, in regex mode, compiled a fresh regexp for every history entry examined — turning Ctrl-R / forward search over a large history into N regexp compilations per keystroke. The comparison string is invariant across the scan, so compute it once before the loop. And regexp.Compile(regexp.QuoteMeta(x)).MatchString(y) is exactly strings.Contains(y, x), so match substrings directly with no regexp at all. Behavior is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
getHighlights() compiled the constant ANSI-escape regex on every display refresh; hoist it to a package-level var. highlightLine() recompiled the comment-highlight pattern every refresh too; cache it on the Engine and rebuild only when the comment-begin option actually changes. Both run on the default per-keystroke render path. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
highlightDisplay compiled regexp.Compile("^"+prefix) once per candidate
per render to recolor the matched prefix. The prefix is an anchored
literal, so a strings.HasPrefix check plus a slice does the same work,
and also fixes a latent bug where a prefix containing regex metacharacters
(. * ( etc.) would fail to match. No behavior change for literal prefixes.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The RawValues Less method lowercased both operands on every comparison, so sorting N candidates did ~2*N*log N strings.ToLower allocations — paid on every keystroke in autocomplete mode. Add sortStable, which folds each value to lower case exactly once (paired sort) and is otherwise identical to sort.Stable(c). A test asserts the ordering matches the old path. Benchmark (n=1000): 18440 -> 1005 allocs/op, ~35% faster. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Every ANSI match begins with an ESC (0x1B) or CSI (0x9B) introducer, so a string containing neither has nothing to strip. Return it directly instead of running the regex and allocating a copy. Strip runs on the Display and Description of every completion candidate (and in selectCandidate's nested scan), and most values are plain. Adds a Strip test covering plain, UTF-8, and colored inputs. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Complete() deduplicated results with a linear scan (contains) plus a remove-and-reappend on every match — O(m^2) over the results shown, on every keystroke during Ctrl-R. The reordering had no effect on the output (the first occurrence in scan order is the one emitted), so replace the whole thing with a map seen-set: O(1) membership, identical output. Also preallocate the result slice and map, and drop the now-unused contains(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
setup-go pinned 1.22.6 while go.mod now requires 1.25.0, so Go auto-switched to a downloaded toolchain that fails 'go test -cover' with "no such tool covdata". Read the version from go.mod so CI installs a matching toolchain and tracks future directive bumps. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Windows readInputFiltered only returned on io.EOF and swallowed any other read error, letting WaitAvailableKeys spin on a dead stdin (and leaving ReadError unset). Return on any error so EOF is recorded and real failures surface, mirroring the Unix reader. Fixes TestWaitAvailableKeysRecordsNonEOFReadError on windows-latest. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Brings in PR #95 (liamg) bottom-of-terminal refresh fixes: skip the CUD-based clear when already on the last row (CUD is a no-op there) and drop the duplicate clear in renderHelpers. Complementary to the #98 work on dev (ensureInputSpace / upper-prompt repaint) — different functions, no conflict; full suite incl. display golden-screen guards passes on the merge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Rolls up the v1.2.0 work currently on
dev(20 commits, all GPG-signed). No content depends onRELEASE_NOTES.md, which stays out of this PR by design.Features (embeddable-frontend cluster)
core.Keys.RequestRefresh()): the idle loop can repaint without a keystroke. poll(2) over [stdin, wake-pipe]; real input is always polled first so a coincident wake never delays a keystroke. Windows: next-keystroke fallback.Shell.RefreshCompletions()([feature] Public API to regenerate/rebuild the currently open completion menu #99): regenerate an open completion menu in place from a goroutine; preserves the current selection by content, no-ops cleanly when no menu is active. Also fixes a pre-existing resize data race.rl.Hint.SetProvider/SetTransient/ClearTransient— passive, transient and completion hint channels no longer clobber each other; transient survives completion/isearch.cursor-position-probeoption ([feature] Public option to disable cursor-position probing (e.g. ESC[6n]) #101): inputrc bool (default on) to disableESC[6n, with a documented degraded fallback for PTY harnesses / constrained CI.Fixes
Readline()return + guardedReadKey(adapted from PRs fix: Handle panic in ReadKey() #96/fix(core): stop spinning on terminal read errors #97 without regressing the async-wake path).Performance (behavior-preserving, verified)
Complete()(O(m²)→O(m)).Stripfast-path for escape-free strings.Maintenance
Messagecompletion function.Testing
PTY + virtual-terminal golden-screen harness added; regression + unit tests across display/completion/core/color.
go build,go test ./..., andgolangci-lint run ./...all clean.